linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] static_call() vs __exit fixes
@ 2021-03-18 11:31 Peter Zijlstra
  2021-03-18 11:31 ` [PATCH 1/3] static_call: Fix static_call_set_init() Peter Zijlstra
                   ` (4 more replies)
  0 siblings, 5 replies; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-18 11:31 UTC (permalink / raw)
  To: x86, jpoimboe, jbaron, rostedt, ardb
  Cc: linux-kernel, peterz, sumit.garg, oliver.sang, jarkko

Hi,

After more poking a new set of patches to fix static_call() vs __exit
functions. These patches replace the patch I posted yesterday:

  https://lkml.kernel.org/r/YFH6BR61b5GK8ITo@hirez.programming.kicks-ass.net

Since I've reproduced the problem locally, and these patches do seem to fully
cure things, I'll shortly queue them for tip/locking/urgent.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/3] static_call: Fix static_call_set_init()
  2021-03-18 11:31 [PATCH 0/3] static_call() vs __exit fixes Peter Zijlstra
@ 2021-03-18 11:31 ` Peter Zijlstra
  2021-03-19 12:25   ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra
  2021-03-18 11:31 ` [PATCH 2/3] static_call: Align static_call_is_init() patching condition Peter Zijlstra
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-18 11:31 UTC (permalink / raw)
  To: x86, jpoimboe, jbaron, rostedt, ardb
  Cc: linux-kernel, peterz, sumit.garg, oliver.sang, jarkko

It turns out that static_call_set_init() does not preserve the other
flags; IOW. it clears TAIL if it was set.

Fixes: 9183c3f9ed710 ("static_call: Add inline static call infrastructure")
Reported-by: Sumit Garg <sumit.garg@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/static_call.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -35,27 +35,30 @@ static inline void *static_call_addr(str
 	return (void *)((long)site->addr + (long)&site->addr);
 }
 
+static inline unsigned long __static_call_key(const struct static_call_site *site)
+{
+	return (long)site->key + (long)&site->key;
+}
 
 static inline struct static_call_key *static_call_key(const struct static_call_site *site)
 {
-	return (struct static_call_key *)
-		(((long)site->key + (long)&site->key) & ~STATIC_CALL_SITE_FLAGS);
+	return (void *)(__static_call_key(site) & ~STATIC_CALL_SITE_FLAGS);
 }
 
 /* These assume the key is word-aligned. */
 static inline bool static_call_is_init(struct static_call_site *site)
 {
-	return ((long)site->key + (long)&site->key) & STATIC_CALL_SITE_INIT;
+	return __static_call_key(site) & STATIC_CALL_SITE_INIT;
 }
 
 static inline bool static_call_is_tail(struct static_call_site *site)
 {
-	return ((long)site->key + (long)&site->key) & STATIC_CALL_SITE_TAIL;
+	return __static_call_key(site) & STATIC_CALL_SITE_TAIL;
 }
 
 static inline void static_call_set_init(struct static_call_site *site)
 {
-	site->key = ((long)static_call_key(site) | STATIC_CALL_SITE_INIT) -
+	site->key = (__static_call_key(site) | STATIC_CALL_SITE_INIT) -
 		    (long)&site->key;
 }
 
@@ -190,7 +193,7 @@ void __static_call_update(struct static_
 			}
 
 			arch_static_call_transform(site_addr, NULL, func,
-				static_call_is_tail(site));
+						   static_call_is_tail(site));
 		}
 	}
 
@@ -349,7 +352,7 @@ static int static_call_add_module(struct
 	struct static_call_site *site;
 
 	for (site = start; site != stop; site++) {
-		unsigned long s_key = (long)site->key + (long)&site->key;
+		unsigned long s_key = __static_call_key(site);
 		unsigned long addr = s_key & ~STATIC_CALL_SITE_FLAGS;
 		unsigned long key;
 



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 2/3] static_call: Align static_call_is_init() patching condition
  2021-03-18 11:31 [PATCH 0/3] static_call() vs __exit fixes Peter Zijlstra
  2021-03-18 11:31 ` [PATCH 1/3] static_call: Fix static_call_set_init() Peter Zijlstra
@ 2021-03-18 11:31 ` Peter Zijlstra
  2021-03-19 12:25   ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra
  2021-03-19 13:31   ` [PATCH 2/3] " Rasmus Villemoes
  2021-03-18 11:31 ` [PATCH 3/3] static_call: Fix static_call_update() sanity check Peter Zijlstra
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-18 11:31 UTC (permalink / raw)
  To: x86, jpoimboe, jbaron, rostedt, ardb
  Cc: linux-kernel, peterz, sumit.garg, oliver.sang, jarkko

The intent is to avoid writing init code after init (because the text
might have been freed). The code is needlessly different between
jump_label and static_call and not obviously correct.

The existing code relies on the fact that the module loader clears the
init layout, such that within_module_init() always fails, while
jump_label relies on the module state which is more obvious and
matches the kernel logic.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/static_call.c |   14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -149,6 +149,7 @@ void __static_call_update(struct static_
 	};
 
 	for (site_mod = &first; site_mod; site_mod = site_mod->next) {
+		bool init = system_state < SYSTEM_RUNNING;
 		struct module *mod = site_mod->mod;
 
 		if (!site_mod->sites) {
@@ -168,6 +169,7 @@ void __static_call_update(struct static_
 		if (mod) {
 			stop = mod->static_call_sites +
 			       mod->num_static_call_sites;
+			init = mod->state == MODULE_STATE_COMING;
 		}
 #endif
 
@@ -175,16 +177,8 @@ void __static_call_update(struct static_
 		     site < stop && static_call_key(site) == key; site++) {
 			void *site_addr = static_call_addr(site);
 
-			if (static_call_is_init(site)) {
-				/*
-				 * Don't write to call sites which were in
-				 * initmem and have since been freed.
-				 */
-				if (!mod && system_state >= SYSTEM_RUNNING)
-					continue;
-				if (mod && !within_module_init((unsigned long)site_addr, mod))
-					continue;
-			}
+			if (!init && static_call_is_init(site))
+				continue;
 
 			if (!kernel_text_address((unsigned long)site_addr)) {
 				WARN_ONCE(1, "can't patch static call site at %pS",



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-18 11:31 [PATCH 0/3] static_call() vs __exit fixes Peter Zijlstra
  2021-03-18 11:31 ` [PATCH 1/3] static_call: Fix static_call_set_init() Peter Zijlstra
  2021-03-18 11:31 ` [PATCH 2/3] static_call: Align static_call_is_init() patching condition Peter Zijlstra
@ 2021-03-18 11:31 ` Peter Zijlstra
  2021-03-18 11:42   ` Peter Zijlstra
                     ` (2 more replies)
  2021-03-18 12:15 ` [PATCH 0/3] static_call() vs __exit fixes Sumit Garg
  2021-03-18 19:38 ` Jarkko Sakkinen
  4 siblings, 3 replies; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-18 11:31 UTC (permalink / raw)
  To: x86, jpoimboe, jbaron, rostedt, ardb
  Cc: linux-kernel, peterz, sumit.garg, oliver.sang, jarkko

Sites that match init_section_contains() get marked as INIT. For
built-in code init_sections contains both __init and __exit text. OTOH
kernel_text_address() only explicitly includes __init text (and there
are no __exit text markers).

Match what jump_label already does and ignore the warning for INIT
sites. Also see the excellent changelog for commit: 8f35eaa5f2de
("jump_label: Don't warn on __exit jump entries")

Fixes: 9183c3f9ed710 ("static_call: Add inline static call infrastructure")
Reported-by: Sumit Garg <sumit.garg@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/jump_label.c  |    8 ++++++++
 kernel/static_call.c |   11 ++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -407,6 +407,14 @@ static bool jump_label_can_update(struct
 		return false;
 
 	if (!kernel_text_address(jump_entry_code(entry))) {
+		/*
+		 * This skips patching __exit, which is part of
+		 * init_section_contains() but is not part of
+		 * kernel_text_address().
+		 *
+		 * Skipping __exit is fine since it will never
+		 * be executed.
+		 */
 		WARN_ONCE(!jump_entry_is_init(entry),
 			  "can't patch jump_label at %pS",
 			  (void *)jump_entry_code(entry));
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -181,7 +181,16 @@ void __static_call_update(struct static_
 				continue;
 
 			if (!kernel_text_address((unsigned long)site_addr)) {
-				WARN_ONCE(1, "can't patch static call site at %pS",
+				/*
+				 * This skips patching __exit, which is part of
+				 * init_section_contains() but is not part of
+				 * kernel_text_address().
+				 *
+				 * Skipping __exit is fine since it will never
+				 * be executed.
+				 */
+				WARN_ONCE(!static_call_is_init(site),
+					  "can't patch static call site at %pS",
 					  site_addr);
 				continue;
 			}



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-18 11:31 ` [PATCH 3/3] static_call: Fix static_call_update() sanity check Peter Zijlstra
@ 2021-03-18 11:42   ` Peter Zijlstra
  2021-03-18 16:13   ` Josh Poimboeuf
  2021-03-19 12:25   ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra
  2 siblings, 0 replies; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-18 11:42 UTC (permalink / raw)
  To: x86, jpoimboe, jbaron, rostedt, ardb
  Cc: linux-kernel, sumit.garg, oliver.sang, jarkko

On Thu, Mar 18, 2021 at 12:31:59PM +0100, Peter Zijlstra wrote:
> Sites that match init_section_contains() get marked as INIT. For
> built-in code init_sections contains both __init and __exit text. OTOH
> kernel_text_address() only explicitly includes __init text (and there
> are no __exit text markers).
> 
> Match what jump_label already does and ignore the warning for INIT
> sites. Also see the excellent changelog for commit: 8f35eaa5f2de
> ("jump_label: Don't warn on __exit jump entries")

Note that I initially had a different fix and thought jump_label was
broken for not patching, but then found the above commit.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/3] static_call() vs __exit fixes
  2021-03-18 11:31 [PATCH 0/3] static_call() vs __exit fixes Peter Zijlstra
                   ` (2 preceding siblings ...)
  2021-03-18 11:31 ` [PATCH 3/3] static_call: Fix static_call_update() sanity check Peter Zijlstra
@ 2021-03-18 12:15 ` Sumit Garg
  2021-03-18 19:38 ` Jarkko Sakkinen
  4 siblings, 0 replies; 26+ messages in thread
From: Sumit Garg @ 2021-03-18 12:15 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: x86, Josh Poimboeuf, jbaron, Steven Rostedt, Ard Biesheuvel,
	Linux Kernel Mailing List, kernel test robot, Jarkko Sakkinen

On Thu, 18 Mar 2021 at 17:10, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Hi,
>
> After more poking a new set of patches to fix static_call() vs __exit
> functions. These patches replace the patch I posted yesterday:
>
>   https://lkml.kernel.org/r/YFH6BR61b5GK8ITo@hirez.programming.kicks-ass.net
>
> Since I've reproduced the problem locally, and these patches do seem to fully
> cure things, I'll shortly queue them for tip/locking/urgent.
>

Thanks Peter for these fixes, works fine for me.

FWIW:

Tested-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-18 11:31 ` [PATCH 3/3] static_call: Fix static_call_update() sanity check Peter Zijlstra
  2021-03-18 11:42   ` Peter Zijlstra
@ 2021-03-18 16:13   ` Josh Poimboeuf
  2021-03-18 16:58     ` Peter Zijlstra
  2021-03-19 12:25   ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra
  2 siblings, 1 reply; 26+ messages in thread
From: Josh Poimboeuf @ 2021-03-18 16:13 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: x86, jbaron, rostedt, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko

On Thu, Mar 18, 2021 at 12:31:59PM +0100, Peter Zijlstra wrote:
>  			if (!kernel_text_address((unsigned long)site_addr)) {
> -				WARN_ONCE(1, "can't patch static call site at %pS",
> +				/*
> +				 * This skips patching __exit, which is part of
> +				 * init_section_contains() but is not part of
> +				 * kernel_text_address().
> +				 *
> +				 * Skipping __exit is fine since it will never
> +				 * be executed.
> +				 */
> +				WARN_ONCE(!static_call_is_init(site),
> +					  "can't patch static call site at %pS",
>  					  site_addr);
>  				continue;
>  			}

It might be good to clarify the situation for __exit in modules in the
comment and/or changelog, as they both seem to be implicitly talking
only about __exit in vmlinux.

For CONFIG_MODULE_UNLOAD, the code ends up in the normal text area, so
static_call_is_init() is false and kernel_text_address() is true.

For !CONFIG_MODULE_UNLOAD, the code gets discarded during module load,
so static_call_is_init() and kernel_text_address() are both false.  I
guess that will trigger a warning?

-- 
Josh


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-18 16:13   ` Josh Poimboeuf
@ 2021-03-18 16:58     ` Peter Zijlstra
  2021-03-19 12:57       ` Peter Zijlstra
  0 siblings, 1 reply; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-18 16:58 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: x86, jbaron, rostedt, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko

On Thu, Mar 18, 2021 at 11:13:08AM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 18, 2021 at 12:31:59PM +0100, Peter Zijlstra wrote:
> >  			if (!kernel_text_address((unsigned long)site_addr)) {
> > -				WARN_ONCE(1, "can't patch static call site at %pS",
> > +				/*
> > +				 * This skips patching __exit, which is part of

				  This skips patching built-in __exit, ...
?

> > +				 * init_section_contains() but is not part of
> > +				 * kernel_text_address().
> > +				 *
> > +				 * Skipping __exit is fine since it will never

		+ built-in, again

> > +				 * be executed.
> > +				 */
> > +				WARN_ONCE(!static_call_is_init(site),
> > +					  "can't patch static call site at %pS",
> >  					  site_addr);
> >  				continue;
> >  			}
> 
> It might be good to clarify the situation for __exit in modules in the
> comment and/or changelog, as they both seem to be implicitly talking
> only about __exit in vmlinux.

Correct.

> For CONFIG_MODULE_UNLOAD, the code ends up in the normal text area, so
> static_call_is_init() is false and kernel_text_address() is true.
> 
> For !CONFIG_MODULE_UNLOAD, the code gets discarded during module load,
> so static_call_is_init() and kernel_text_address() are both false.  I
> guess that will trigger a warning?

Oh gawd, more variants.

Afaict MODULE_UNLOAD, by virtue of that #ifdef in
rewrite_section_headers() won't even load the .exit sections. Afaict
that will break: alterative, jump_label and static_call patching all in
one go.



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/3] static_call() vs __exit fixes
  2021-03-18 11:31 [PATCH 0/3] static_call() vs __exit fixes Peter Zijlstra
                   ` (3 preceding siblings ...)
  2021-03-18 12:15 ` [PATCH 0/3] static_call() vs __exit fixes Sumit Garg
@ 2021-03-18 19:38 ` Jarkko Sakkinen
  4 siblings, 0 replies; 26+ messages in thread
From: Jarkko Sakkinen @ 2021-03-18 19:38 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: x86, jpoimboe, jbaron, rostedt, ardb, linux-kernel, sumit.garg,
	oliver.sang

On Thu, Mar 18, 2021 at 12:31:56PM +0100, Peter Zijlstra wrote:
> Hi,
> 
> After more poking a new set of patches to fix static_call() vs __exit
> functions. These patches replace the patch I posted yesterday:
> 
>   https://lkml.kernel.org/r/YFH6BR61b5GK8ITo@hirez.programming.kicks-ass.net
> 
> Since I've reproduced the problem locally, and these patches do seem to fully
> cure things, I'll shortly queue them for tip/locking/urgent.

For all:

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkkko

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [tip: locking/urgent] static_call: Fix static_call_update() sanity check
  2021-03-18 11:31 ` [PATCH 3/3] static_call: Fix static_call_update() sanity check Peter Zijlstra
  2021-03-18 11:42   ` Peter Zijlstra
  2021-03-18 16:13   ` Josh Poimboeuf
@ 2021-03-19 12:25   ` tip-bot2 for Peter Zijlstra
  2 siblings, 0 replies; 26+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2021-03-19 12:25 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Sumit Garg, Peter Zijlstra (Intel), Jarkko Sakkinen, x86, linux-kernel

The following commit has been merged into the locking/urgent branch of tip:

Commit-ID:     38c93587375053c5b9ef093f4a5ea754538cba32
Gitweb:        https://git.kernel.org/tip/38c93587375053c5b9ef093f4a5ea754538cba32
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Thu, 18 Mar 2021 11:31:51 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 19 Mar 2021 13:16:44 +01:00

static_call: Fix static_call_update() sanity check

Sites that match init_section_contains() get marked as INIT. For
built-in code init_sections contains both __init and __exit text. OTOH
kernel_text_address() only explicitly includes __init text (and there
are no __exit text markers).

Match what jump_label already does and ignore the warning for INIT
sites. Also see the excellent changelog for commit: 8f35eaa5f2de
("jump_label: Don't warn on __exit jump entries")

Fixes: 9183c3f9ed710 ("static_call: Add inline static call infrastructure")
Reported-by: Sumit Garg <sumit.garg@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Tested-by: Sumit Garg <sumit.garg@linaro.org>
Link: https://lkml.kernel.org/r/20210318113610.739542434@infradead.org
---
 kernel/jump_label.c  |  8 ++++++++
 kernel/static_call.c | 11 ++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index c6a39d6..ba39fbb 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -407,6 +407,14 @@ static bool jump_label_can_update(struct jump_entry *entry, bool init)
 		return false;
 
 	if (!kernel_text_address(jump_entry_code(entry))) {
+		/*
+		 * This skips patching built-in __exit, which
+		 * is part of init_section_contains() but is
+		 * not part of kernel_text_address().
+		 *
+		 * Skipping built-in __exit is fine since it
+		 * will never be executed.
+		 */
 		WARN_ONCE(!jump_entry_is_init(entry),
 			  "can't patch jump_label at %pS",
 			  (void *)jump_entry_code(entry));
diff --git a/kernel/static_call.c b/kernel/static_call.c
index fc22590..2c5950b 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -181,7 +181,16 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
 				continue;
 
 			if (!kernel_text_address((unsigned long)site_addr)) {
-				WARN_ONCE(1, "can't patch static call site at %pS",
+				/*
+				 * This skips patching built-in __exit, which
+				 * is part of init_section_contains() but is
+				 * not part of kernel_text_address().
+				 *
+				 * Skipping built-in __exit is fine since it
+				 * will never be executed.
+				 */
+				WARN_ONCE(!static_call_is_init(site),
+					  "can't patch static call site at %pS",
 					  site_addr);
 				continue;
 			}

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [tip: locking/urgent] static_call: Fix static_call_set_init()
  2021-03-18 11:31 ` [PATCH 1/3] static_call: Fix static_call_set_init() Peter Zijlstra
@ 2021-03-19 12:25   ` tip-bot2 for Peter Zijlstra
  0 siblings, 0 replies; 26+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2021-03-19 12:25 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Sumit Garg, Peter Zijlstra (Intel), Jarkko Sakkinen, x86, linux-kernel

The following commit has been merged into the locking/urgent branch of tip:

Commit-ID:     68b1eddd421d2b16c6655eceb48918a1e896bbbc
Gitweb:        https://git.kernel.org/tip/68b1eddd421d2b16c6655eceb48918a1e896bbbc
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Thu, 18 Mar 2021 11:27:19 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 19 Mar 2021 13:16:44 +01:00

static_call: Fix static_call_set_init()

It turns out that static_call_set_init() does not preserve the other
flags; IOW. it clears TAIL if it was set.

Fixes: 9183c3f9ed710 ("static_call: Add inline static call infrastructure")
Reported-by: Sumit Garg <sumit.garg@linaro.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Tested-by: Sumit Garg <sumit.garg@linaro.org>
Link: https://lkml.kernel.org/r/20210318113610.519406371@infradead.org
---
 kernel/static_call.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/kernel/static_call.c b/kernel/static_call.c
index ae82529..080c8a9 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -35,27 +35,30 @@ static inline void *static_call_addr(struct static_call_site *site)
 	return (void *)((long)site->addr + (long)&site->addr);
 }
 
+static inline unsigned long __static_call_key(const struct static_call_site *site)
+{
+	return (long)site->key + (long)&site->key;
+}
 
 static inline struct static_call_key *static_call_key(const struct static_call_site *site)
 {
-	return (struct static_call_key *)
-		(((long)site->key + (long)&site->key) & ~STATIC_CALL_SITE_FLAGS);
+	return (void *)(__static_call_key(site) & ~STATIC_CALL_SITE_FLAGS);
 }
 
 /* These assume the key is word-aligned. */
 static inline bool static_call_is_init(struct static_call_site *site)
 {
-	return ((long)site->key + (long)&site->key) & STATIC_CALL_SITE_INIT;
+	return __static_call_key(site) & STATIC_CALL_SITE_INIT;
 }
 
 static inline bool static_call_is_tail(struct static_call_site *site)
 {
-	return ((long)site->key + (long)&site->key) & STATIC_CALL_SITE_TAIL;
+	return __static_call_key(site) & STATIC_CALL_SITE_TAIL;
 }
 
 static inline void static_call_set_init(struct static_call_site *site)
 {
-	site->key = ((long)static_call_key(site) | STATIC_CALL_SITE_INIT) -
+	site->key = (__static_call_key(site) | STATIC_CALL_SITE_INIT) -
 		    (long)&site->key;
 }
 
@@ -190,7 +193,7 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
 			}
 
 			arch_static_call_transform(site_addr, NULL, func,
-				static_call_is_tail(site));
+						   static_call_is_tail(site));
 		}
 	}
 
@@ -349,7 +352,7 @@ static int static_call_add_module(struct module *mod)
 	struct static_call_site *site;
 
 	for (site = start; site != stop; site++) {
-		unsigned long s_key = (long)site->key + (long)&site->key;
+		unsigned long s_key = __static_call_key(site);
 		unsigned long addr = s_key & ~STATIC_CALL_SITE_FLAGS;
 		unsigned long key;
 

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [tip: locking/urgent] static_call: Align static_call_is_init() patching condition
  2021-03-18 11:31 ` [PATCH 2/3] static_call: Align static_call_is_init() patching condition Peter Zijlstra
@ 2021-03-19 12:25   ` tip-bot2 for Peter Zijlstra
  2021-03-19 13:31   ` [PATCH 2/3] " Rasmus Villemoes
  1 sibling, 0 replies; 26+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2021-03-19 12:25 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Peter Zijlstra (Intel), Jarkko Sakkinen, Sumit Garg, x86, linux-kernel

The following commit has been merged into the locking/urgent branch of tip:

Commit-ID:     698bacefe993ad2922c9d3b1380591ad489355e9
Gitweb:        https://git.kernel.org/tip/698bacefe993ad2922c9d3b1380591ad489355e9
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Thu, 18 Mar 2021 11:29:56 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 19 Mar 2021 13:16:44 +01:00

static_call: Align static_call_is_init() patching condition

The intent is to avoid writing init code after init (because the text
might have been freed). The code is needlessly different between
jump_label and static_call and not obviously correct.

The existing code relies on the fact that the module loader clears the
init layout, such that within_module_init() always fails, while
jump_label relies on the module state which is more obvious and
matches the kernel logic.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Tested-by: Sumit Garg <sumit.garg@linaro.org>
Link: https://lkml.kernel.org/r/20210318113610.636651340@infradead.org
---
 kernel/static_call.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/kernel/static_call.c b/kernel/static_call.c
index 080c8a9..fc22590 100644
--- a/kernel/static_call.c
+++ b/kernel/static_call.c
@@ -149,6 +149,7 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
 	};
 
 	for (site_mod = &first; site_mod; site_mod = site_mod->next) {
+		bool init = system_state < SYSTEM_RUNNING;
 		struct module *mod = site_mod->mod;
 
 		if (!site_mod->sites) {
@@ -168,6 +169,7 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
 		if (mod) {
 			stop = mod->static_call_sites +
 			       mod->num_static_call_sites;
+			init = mod->state == MODULE_STATE_COMING;
 		}
 #endif
 
@@ -175,16 +177,8 @@ void __static_call_update(struct static_call_key *key, void *tramp, void *func)
 		     site < stop && static_call_key(site) == key; site++) {
 			void *site_addr = static_call_addr(site);
 
-			if (static_call_is_init(site)) {
-				/*
-				 * Don't write to call sites which were in
-				 * initmem and have since been freed.
-				 */
-				if (!mod && system_state >= SYSTEM_RUNNING)
-					continue;
-				if (mod && !within_module_init((unsigned long)site_addr, mod))
-					continue;
-			}
+			if (!init && static_call_is_init(site))
+				continue;
 
 			if (!kernel_text_address((unsigned long)site_addr)) {
 				WARN_ONCE(1, "can't patch static call site at %pS",

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-18 16:58     ` Peter Zijlstra
@ 2021-03-19 12:57       ` Peter Zijlstra
  2021-03-19 18:00         ` Steven Rostedt
  0 siblings, 1 reply; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-19 12:57 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: x86, jbaron, rostedt, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko, jeyu

On Thu, Mar 18, 2021 at 05:58:38PM +0100, Peter Zijlstra wrote:

> > For CONFIG_MODULE_UNLOAD, the code ends up in the normal text area, so
> > static_call_is_init() is false and kernel_text_address() is true.
> > 
> > For !CONFIG_MODULE_UNLOAD, the code gets discarded during module load,
> > so static_call_is_init() and kernel_text_address() are both false.  I
> > guess that will trigger a warning?
> 
> Oh gawd, more variants.
> 
> Afaict MODULE_UNLOAD, by virtue of that #ifdef in

!MODULE_UNLOAD (obv)

> rewrite_section_headers() won't even load the .exit sections. Afaict
> that will break: alterative, jump_label and static_call patching all in
> one go.

Jessica, can you explain how !MODULE_UNLOAD is supposed to work?
Alternatives, jump_labels and static_call all can have relocations into
__exit code. Not loading it at all would be BAD.

For alternatives all we really need it to discard it after init, for
jump_label and static_call we additinoally need to the code to identify
as init (ie, within_module_init() must return true for it).

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/3] static_call: Align static_call_is_init() patching condition
  2021-03-18 11:31 ` [PATCH 2/3] static_call: Align static_call_is_init() patching condition Peter Zijlstra
  2021-03-19 12:25   ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra
@ 2021-03-19 13:31   ` Rasmus Villemoes
  2021-03-19 14:13     ` Peter Zijlstra
  1 sibling, 1 reply; 26+ messages in thread
From: Rasmus Villemoes @ 2021-03-19 13:31 UTC (permalink / raw)
  To: Peter Zijlstra, x86, jpoimboe, jbaron, rostedt, ardb
  Cc: linux-kernel, sumit.garg, oliver.sang, jarkko

On 18/03/2021 12.31, Peter Zijlstra wrote:
> The intent is to avoid writing init code after init (because the text
> might have been freed). The code is needlessly different between
> jump_label and static_call and not obviously correct.
> 
> The existing code relies on the fact that the module loader clears the
> init layout, such that within_module_init() always fails, while
> jump_label relies on the module state which is more obvious and
> matches the kernel logic.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/static_call.c |   14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> --- a/kernel/static_call.c
> +++ b/kernel/static_call.c
> @@ -149,6 +149,7 @@ void __static_call_update(struct static_
>  	};
>  
>  	for (site_mod = &first; site_mod; site_mod = site_mod->next) {
> +		bool init = system_state < SYSTEM_RUNNING;

I recently had occasion to look at whether that would be a suitable
condition for knowing whether __init stuff was gone, but concluded that
it's not. Maybe I'm wrong. init/main.c:

        free_initmem();
        mark_readonly();

        /*
         * Kernel mappings are now finalized - update the userspace
page-table
         * to finalize PTI.
         */
        pti_finalize();

        system_state = SYSTEM_RUNNING;

So ISTM there's window where system_state < SYSTEM_RUNNING but accessing
init stuff is a bad idea. If you're PID1 it's all fine, but I suppose
other kernel threads could end up calling static_call_update. And just
moving the system_state setting up before the *free_initmem() calls
doesn't really solve anything because TOCTOU.

Dunno, probably overkill, but perhaps we could have an atomic_t (or
refcount, whatever) init_ref inited to 1, with init_ref_get() doing an
inc_unless_zero, and iff you get a ref, you're free to call (/patch)
__init functions and access __initdata, but must do init_ref_put(), with
PID1 dropping its initial ref and waiting for it to drop to 0 before
doing the *free_initmem() calls.

Rasmus

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/3] static_call: Align static_call_is_init() patching condition
  2021-03-19 13:31   ` [PATCH 2/3] " Rasmus Villemoes
@ 2021-03-19 14:13     ` Peter Zijlstra
  2021-03-19 14:40       ` Rasmus Villemoes
  0 siblings, 1 reply; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-19 14:13 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: x86, jpoimboe, jbaron, rostedt, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko

On Fri, Mar 19, 2021 at 02:31:19PM +0100, Rasmus Villemoes wrote:
> On 18/03/2021 12.31, Peter Zijlstra wrote:
> > The intent is to avoid writing init code after init (because the text
> > might have been freed). The code is needlessly different between
> > jump_label and static_call and not obviously correct.
> > 
> > The existing code relies on the fact that the module loader clears the
> > init layout, such that within_module_init() always fails, while
> > jump_label relies on the module state which is more obvious and
> > matches the kernel logic.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  kernel/static_call.c |   14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> > 
> > --- a/kernel/static_call.c
> > +++ b/kernel/static_call.c
> > @@ -149,6 +149,7 @@ void __static_call_update(struct static_
> >  	};
> >  
> >  	for (site_mod = &first; site_mod; site_mod = site_mod->next) {
> > +		bool init = system_state < SYSTEM_RUNNING;
> 
> I recently had occasion to look at whether that would be a suitable
> condition for knowing whether __init stuff was gone, but concluded that
> it's not. Maybe I'm wrong. init/main.c:

Ha, me too:

 https://lkml.kernel.org/r/YFMToXI/3qjlMur4@hirez.programming.kicks-ass.net

and I share your concern.

> Dunno, probably overkill, but perhaps we could have an atomic_t (or
> refcount, whatever) init_ref inited to 1, with init_ref_get() doing an
> inc_unless_zero, and iff you get a ref, you're free to call (/patch)
> __init functions and access __initdata, but must do init_ref_put(), with
> PID1 dropping its initial ref and waiting for it to drop to 0 before
> doing the *free_initmem() calls.

I'd as soon simply add another SYSTEM state. That way we don't have to
worry about who else looks at RUNNING for what etc..



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/3] static_call: Align static_call_is_init() patching condition
  2021-03-19 14:13     ` Peter Zijlstra
@ 2021-03-19 14:40       ` Rasmus Villemoes
  2021-03-19 15:44         ` Rasmus Villemoes
  2021-03-22 16:59         ` Peter Zijlstra
  0 siblings, 2 replies; 26+ messages in thread
From: Rasmus Villemoes @ 2021-03-19 14:40 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: x86, jpoimboe, jbaron, rostedt, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko

On 19/03/2021 15.13, Peter Zijlstra wrote:

>> Dunno, probably overkill, but perhaps we could have an atomic_t (or
>> refcount, whatever) init_ref inited to 1, with init_ref_get() doing an
>> inc_unless_zero, and iff you get a ref, you're free to call (/patch)
>> __init functions and access __initdata, but must do init_ref_put(), with
>> PID1 dropping its initial ref and waiting for it to drop to 0 before
>> doing the *free_initmem() calls.
> 
> I'd as soon simply add another SYSTEM state. That way we don't have to
> worry about who else looks at RUNNING for what etc..

I don't understand. How would that solve the

PID1                           PIDX
                               ok = system_state < INIT_MEM_GONE;
system_state = INIT_MEM_GONE;
free_initmem();
system_state = RUNNING;
                               if (ok)
                                   poke init mem

race? I really don't see any way arbitrary threads can reliably check
how far PID1 has progressed at one point in time and use that
information (a few lines) later to access init memory without
synchronizing with PID1.

AFAICT, having an atomic_t object representing the init memory and a
"no, you can't have a new reference" would guarantee correctness of the
jump_label/static_call patching: If we get a reference, we do the
patching just as for the rest of the kernel .text. If we don't, i.e.
observe atomic_load(init_ref)==0, that may not necessarily mean that
PID1 has actually discarded the memory yet, but no thread can currently
or in the future actually run __init code, so it need not be patched.

Rasmus

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/3] static_call: Align static_call_is_init() patching condition
  2021-03-19 14:40       ` Rasmus Villemoes
@ 2021-03-19 15:44         ` Rasmus Villemoes
  2021-03-22 16:59         ` Peter Zijlstra
  1 sibling, 0 replies; 26+ messages in thread
From: Rasmus Villemoes @ 2021-03-19 15:44 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: x86, jpoimboe, jbaron, rostedt, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko

On 19/03/2021 15.40, Rasmus Villemoes wrote:
> On 19/03/2021 15.13, Peter Zijlstra wrote:
> 
>>> Dunno, probably overkill, but perhaps we could have an atomic_t (or
>>> refcount, whatever) init_ref inited to 1, with init_ref_get() doing an
>>> inc_unless_zero, and iff you get a ref, you're free to call (/patch)
>>> __init functions and access __initdata, but must do init_ref_put(), with
>>> PID1 dropping its initial ref and waiting for it to drop to 0 before
>>> doing the *free_initmem() calls.
>>
>> I'd as soon simply add another SYSTEM state. That way we don't have to
>> worry about who else looks at RUNNING for what etc..
> 
> I don't understand. How would that solve the
> 
> PID1                           PIDX
>                                ok = system_state < INIT_MEM_GONE;
> system_state = INIT_MEM_GONE;
> free_initmem();
> system_state = RUNNING;
>                                if (ok)
>                                    poke init mem
> 
> race? I really don't see any way arbitrary threads can reliably check
> how far PID1 has progressed at one point in time and use that
> information (a few lines) later to access init memory without
> synchronizing with PID1.
> 
> AFAICT, having an atomic_t object representing the init memory 

something like

--- a/init/main.c
+++ b/init/main.c
@@ -1417,6 +1417,18 @@ void __weak free_initmem(void)
        free_initmem_default(POISON_FREE_INITMEM);
 }

+static atomic_t init_mem_ref = ATOMIC_INIT(1);
+static DECLARE_COMPLETION(init_mem_may_go);
+bool init_mem_get(void)
+{
+       return atomic_inc_not_zero(&init_mem_ref);
+}
+void init_mem_put(void)
+{
+       if (atomic_dec_and_test(&init_mem_ref))
+               complete(&init_mem_may_go);
+}
+
 static int __ref kernel_init(void *unused)
 {
        int ret;
@@ -1424,6 +1436,8 @@ static int __ref kernel_init(void *unused)
        kernel_init_freeable();
        /* need to finish all async __init code before freeing the memory */
        async_synchronize_full();
+       init_mem_put();
+       wait_for_completion(&init_mem_may_go);
        kprobe_free_init_mem();
        ftrace_free_init_mem();
        kgdb_free_init_mem();


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-19 12:57       ` Peter Zijlstra
@ 2021-03-19 18:00         ` Steven Rostedt
  2021-03-19 19:34           ` Peter Zijlstra
  2021-03-22 13:07           ` Jessica Yu
  0 siblings, 2 replies; 26+ messages in thread
From: Steven Rostedt @ 2021-03-19 18:00 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Josh Poimboeuf, x86, jbaron, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko, jeyu

On Fri, 19 Mar 2021 13:57:38 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> Jessica, can you explain how !MODULE_UNLOAD is supposed to work?
> Alternatives, jump_labels and static_call all can have relocations into
> __exit code. Not loading it at all would be BAD.

According to the description:

" Without this option you will not be able to unload any
  modules (note that some modules may not be unloadable anyway), which
  makes your kernel smaller, faster and simpler.
  If unsure, say Y."

Seems there's no reason to load the "exit" portion, as that's what makes it
"smaller".

Would making __exit code the same as init code work? That is, load it just
like module init code is loaded, and free it when the init code is freed
(hopefully keeping the kernel still "smaller, faster and simpler").


-- Steve


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-19 18:00         ` Steven Rostedt
@ 2021-03-19 19:34           ` Peter Zijlstra
  2021-03-19 20:57             ` Steven Rostedt
  2021-03-22 13:07           ` Jessica Yu
  1 sibling, 1 reply; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-19 19:34 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Josh Poimboeuf, x86, jbaron, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko, jeyu

On Fri, Mar 19, 2021 at 02:00:05PM -0400, Steven Rostedt wrote:
> Would making __exit code the same as init code work? That is, load it just
> like module init code is loaded, and free it when the init code is freed

As stated, yes. But it must then also identify as init through
within_module_init().

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-19 19:34           ` Peter Zijlstra
@ 2021-03-19 20:57             ` Steven Rostedt
  2021-03-22 14:50               ` Jessica Yu
  0 siblings, 1 reply; 26+ messages in thread
From: Steven Rostedt @ 2021-03-19 20:57 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Josh Poimboeuf, x86, jbaron, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko, jeyu

On Fri, 19 Mar 2021 20:34:24 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Mar 19, 2021 at 02:00:05PM -0400, Steven Rostedt wrote:
> > Would making __exit code the same as init code work? That is, load it just
> > like module init code is loaded, and free it when the init code is freed  
> 
> As stated, yes. But it must then also identify as init through
> within_module_init().

I think that's doable. Since the usecases for that appear to be mostly
about "think code may no longer exist after it is used". Thus, having exit
code act just like init code when UNLOAD is not set, appears appropriate.

Jessica, please correct me if I'm wrong.

Thanks,

-- Steve

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-19 18:00         ` Steven Rostedt
  2021-03-19 19:34           ` Peter Zijlstra
@ 2021-03-22 13:07           ` Jessica Yu
  2021-03-22 14:06             ` Peter Zijlstra
  1 sibling, 1 reply; 26+ messages in thread
From: Jessica Yu @ 2021-03-22 13:07 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Peter Zijlstra, Josh Poimboeuf, x86, jbaron, ardb, linux-kernel,
	sumit.garg, oliver.sang, jarkko

+++ Steven Rostedt [19/03/21 14:00 -0400]:
>On Fri, 19 Mar 2021 13:57:38 +0100
>Peter Zijlstra <peterz@infradead.org> wrote:
>
>> Jessica, can you explain how !MODULE_UNLOAD is supposed to work?
>> Alternatives, jump_labels and static_call all can have relocations into
>> __exit code. Not loading it at all would be BAD.
>
>According to the description:
>
>" Without this option you will not be able to unload any
>  modules (note that some modules may not be unloadable anyway), which
>  makes your kernel smaller, faster and simpler.
>  If unsure, say Y."
>
>Seems there's no reason to load the "exit" portion, as that's what makes it
>"smaller".

Exactly. If you disable MODULE_UNLOAD, then you don't intend to ever
unload any modules, and so you'll never end up calling the module's
cleanup/exit function. That code would basically be never used, so
that's why it's not loaded in the first place.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-22 13:07           ` Jessica Yu
@ 2021-03-22 14:06             ` Peter Zijlstra
  0 siblings, 0 replies; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-22 14:06 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Steven Rostedt, Josh Poimboeuf, x86, jbaron, ardb, linux-kernel,
	sumit.garg, oliver.sang, jarkko

On Mon, Mar 22, 2021 at 02:07:54PM +0100, Jessica Yu wrote:
> +++ Steven Rostedt [19/03/21 14:00 -0400]:
> > On Fri, 19 Mar 2021 13:57:38 +0100
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > Jessica, can you explain how !MODULE_UNLOAD is supposed to work?
> > > Alternatives, jump_labels and static_call all can have relocations into
> > > __exit code. Not loading it at all would be BAD.
> > 
> > According to the description:
> > 
> > " Without this option you will not be able to unload any
> >  modules (note that some modules may not be unloadable anyway), which
> >  makes your kernel smaller, faster and simpler.
> >  If unsure, say Y."
> > 
> > Seems there's no reason to load the "exit" portion, as that's what makes it
> > "smaller".
> 
> Exactly. If you disable MODULE_UNLOAD, then you don't intend to ever
> unload any modules, and so you'll never end up calling the module's
> cleanup/exit function. That code would basically be never used, so
> that's why it's not loaded in the first place.

As explained, that's broken. Has always been for as long as we've had
alternatives.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-19 20:57             ` Steven Rostedt
@ 2021-03-22 14:50               ` Jessica Yu
  2021-03-22 16:54                 ` Peter Zijlstra
  0 siblings, 1 reply; 26+ messages in thread
From: Jessica Yu @ 2021-03-22 14:50 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Peter Zijlstra, Josh Poimboeuf, x86, jbaron, ardb, linux-kernel,
	sumit.garg, oliver.sang, jarkko

+++ Steven Rostedt [19/03/21 16:57 -0400]:
>On Fri, 19 Mar 2021 20:34:24 +0100
>Peter Zijlstra <peterz@infradead.org> wrote:
>
>> On Fri, Mar 19, 2021 at 02:00:05PM -0400, Steven Rostedt wrote:
>> > Would making __exit code the same as init code work? That is, load it just
>> > like module init code is loaded, and free it when the init code is freed
>>
>> As stated, yes. But it must then also identify as init through
>> within_module_init().
>
>I think that's doable. Since the usecases for that appear to be mostly
>about "think code may no longer exist after it is used". Thus, having exit
>code act just like init code when UNLOAD is not set, appears appropriate.
>
>Jessica, please correct me if I'm wrong.

It should be doable. If you want the exit sections to be treated the same as
module init, the following patch should stuff any exit sections into the module
init "region" (completely untested). Hence it should be freed together with the
init sections and it would identify as init through within_module_init(). Let
me know if this works for you.

---

diff --git a/kernel/module.c b/kernel/module.c
index 30479355ab85..1c3396a9dd8b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2802,7 +2802,11 @@ void * __weak module_alloc(unsigned long size)

  bool __weak module_init_section(const char *name)
  {
-       return strstarts(name, ".init");
+#ifndef CONFIG_UNLOAD_MODULE
+       return strstarts(name, ".init") || module_exit_section(name);
+#else
+       return strstarts(name, ".init")
+#endif
  }

  bool __weak module_exit_section(const char *name)
@@ -3116,11 +3120,6 @@ static int rewrite_section_headers(struct load_info *info, int flags)
                  */
                 shdr->sh_addr = (size_t)info->hdr + shdr->sh_offset;

-#ifndef CONFIG_MODULE_UNLOAD
-               /* Don't load .exit sections */
-               if (module_exit_section(info->secstrings+shdr->sh_name))
-                       shdr->sh_flags &= ~(unsigned long)SHF_ALLOC;
-#endif
         }

         /* Track but don't keep modinfo and version sections. */


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-22 14:50               ` Jessica Yu
@ 2021-03-22 16:54                 ` Peter Zijlstra
  2021-03-22 17:36                   ` Jessica Yu
  0 siblings, 1 reply; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-22 16:54 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Steven Rostedt, Josh Poimboeuf, x86, jbaron, ardb, linux-kernel,
	sumit.garg, oliver.sang, jarkko

On Mon, Mar 22, 2021 at 03:50:14PM +0100, Jessica Yu wrote:

> It should be doable. If you want the exit sections to be treated the same as
> module init, the following patch should stuff any exit sections into the module
> init "region" (completely untested). Hence it should be freed together with the
> init sections and it would identify as init through within_module_init(). Let
> me know if this works for you.

That does indeed seem to DTRT from a quick scan of module.c. Very nice
tidy patch. I was afraid it'd be much worse.

Assuming it actually works; for your Changelog:

"Dynamic code patching (alternatives, jump_label and static_call) can
have sites in __exit code, even it __exit is never executed. Therefore
__exit must be present at runtime, at least for as long as __init code
is.

Additionally, for jump_label and static_call, the __exit sites must also
identify as within_module_init(), such that the infrastructure is aware
to never touch them after module init -- alternatives are only ran once
at init and hence don't have this particular constraint.

By making __exit identify as __init for UNLOAD_MODULE, the above is
satisfied."

Thanks!

> ---
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 30479355ab85..1c3396a9dd8b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2802,7 +2802,11 @@ void * __weak module_alloc(unsigned long size)
> 
>  bool __weak module_init_section(const char *name)
>  {
> -       return strstarts(name, ".init");
> +#ifndef CONFIG_UNLOAD_MODULE
> +       return strstarts(name, ".init") || module_exit_section(name);
> +#else
> +       return strstarts(name, ".init")
> +#endif
>  }
> 
>  bool __weak module_exit_section(const char *name)
> @@ -3116,11 +3120,6 @@ static int rewrite_section_headers(struct load_info *info, int flags)
>                  */
>                 shdr->sh_addr = (size_t)info->hdr + shdr->sh_offset;
> 
> -#ifndef CONFIG_MODULE_UNLOAD
> -               /* Don't load .exit sections */
> -               if (module_exit_section(info->secstrings+shdr->sh_name))
> -                       shdr->sh_flags &= ~(unsigned long)SHF_ALLOC;
> -#endif
>         }
> 
>         /* Track but don't keep modinfo and version sections. */
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/3] static_call: Align static_call_is_init() patching condition
  2021-03-19 14:40       ` Rasmus Villemoes
  2021-03-19 15:44         ` Rasmus Villemoes
@ 2021-03-22 16:59         ` Peter Zijlstra
  1 sibling, 0 replies; 26+ messages in thread
From: Peter Zijlstra @ 2021-03-22 16:59 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: x86, jpoimboe, jbaron, rostedt, ardb, linux-kernel, sumit.garg,
	oliver.sang, jarkko

On Fri, Mar 19, 2021 at 03:40:46PM +0100, Rasmus Villemoes wrote:
> On 19/03/2021 15.13, Peter Zijlstra wrote:
> 
> >> Dunno, probably overkill, but perhaps we could have an atomic_t (or
> >> refcount, whatever) init_ref inited to 1, with init_ref_get() doing an
> >> inc_unless_zero, and iff you get a ref, you're free to call (/patch)
> >> __init functions and access __initdata, but must do init_ref_put(), with
> >> PID1 dropping its initial ref and waiting for it to drop to 0 before
> >> doing the *free_initmem() calls.
> > 
> > I'd as soon simply add another SYSTEM state. That way we don't have to
> > worry about who else looks at RUNNING for what etc..
> 
> I don't understand. How would that solve the
> 
> PID1                           PIDX
>                                ok = system_state < INIT_MEM_GONE;
> system_state = INIT_MEM_GONE;
> free_initmem();
> system_state = RUNNING;
>                                if (ok)
>                                    poke init mem
> 
> race?

Argh, I meant to put it before SMP bringup, but then still have to run
the smp_init calls :/

N/m, you're quite right.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/3] static_call: Fix static_call_update() sanity check
  2021-03-22 16:54                 ` Peter Zijlstra
@ 2021-03-22 17:36                   ` Jessica Yu
  0 siblings, 0 replies; 26+ messages in thread
From: Jessica Yu @ 2021-03-22 17:36 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Steven Rostedt, Josh Poimboeuf, x86, jbaron, ardb, linux-kernel,
	sumit.garg, oliver.sang, jarkko

+++ Peter Zijlstra [22/03/21 17:54 +0100]:
>On Mon, Mar 22, 2021 at 03:50:14PM +0100, Jessica Yu wrote:
>
>> It should be doable. If you want the exit sections to be treated the same as
>> module init, the following patch should stuff any exit sections into the module
>> init "region" (completely untested). Hence it should be freed together with the
>> init sections and it would identify as init through within_module_init(). Let
>> me know if this works for you.
>
>That does indeed seem to DTRT from a quick scan of module.c. Very nice
>tidy patch. I was afraid it'd be much worse.
>
>Assuming it actually works; for your Changelog:
>
>"Dynamic code patching (alternatives, jump_label and static_call) can
>have sites in __exit code, even it __exit is never executed. Therefore
>__exit must be present at runtime, at least for as long as __init code
>is.
>
>Additionally, for jump_label and static_call, the __exit sites must also
>identify as within_module_init(), such that the infrastructure is aware
>to never touch them after module init -- alternatives are only ran once
>at init and hence don't have this particular constraint.
>
>By making __exit identify as __init for UNLOAD_MODULE, the above is
>satisfied."

Thanks a lot for the changelog :-) I'll turn this into a formal patch
after some testing tomorrow.

Jessica

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-03-22 17:37 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18 11:31 [PATCH 0/3] static_call() vs __exit fixes Peter Zijlstra
2021-03-18 11:31 ` [PATCH 1/3] static_call: Fix static_call_set_init() Peter Zijlstra
2021-03-19 12:25   ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra
2021-03-18 11:31 ` [PATCH 2/3] static_call: Align static_call_is_init() patching condition Peter Zijlstra
2021-03-19 12:25   ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra
2021-03-19 13:31   ` [PATCH 2/3] " Rasmus Villemoes
2021-03-19 14:13     ` Peter Zijlstra
2021-03-19 14:40       ` Rasmus Villemoes
2021-03-19 15:44         ` Rasmus Villemoes
2021-03-22 16:59         ` Peter Zijlstra
2021-03-18 11:31 ` [PATCH 3/3] static_call: Fix static_call_update() sanity check Peter Zijlstra
2021-03-18 11:42   ` Peter Zijlstra
2021-03-18 16:13   ` Josh Poimboeuf
2021-03-18 16:58     ` Peter Zijlstra
2021-03-19 12:57       ` Peter Zijlstra
2021-03-19 18:00         ` Steven Rostedt
2021-03-19 19:34           ` Peter Zijlstra
2021-03-19 20:57             ` Steven Rostedt
2021-03-22 14:50               ` Jessica Yu
2021-03-22 16:54                 ` Peter Zijlstra
2021-03-22 17:36                   ` Jessica Yu
2021-03-22 13:07           ` Jessica Yu
2021-03-22 14:06             ` Peter Zijlstra
2021-03-19 12:25   ` [tip: locking/urgent] " tip-bot2 for Peter Zijlstra
2021-03-18 12:15 ` [PATCH 0/3] static_call() vs __exit fixes Sumit Garg
2021-03-18 19:38 ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).