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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
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;
}
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.
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
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.
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 <[email protected]>
AuthorDate: Thu, 18 Mar 2021 11:31:51 +01:00
Committer: Peter Zijlstra <[email protected]>
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 <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Acked-by: Jarkko Sakkinen <[email protected]>
Tested-by: Sumit Garg <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
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;
}
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).
On Fri, 19 Mar 2021 13:57:38 +0100
Peter Zijlstra <[email protected]> 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
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().
On Fri, 19 Mar 2021 20:34:24 +0100
Peter Zijlstra <[email protected]> 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
+++ Steven Rostedt [19/03/21 14:00 -0400]:
>On Fri, 19 Mar 2021 13:57:38 +0100
>Peter Zijlstra <[email protected]> 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.
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 <[email protected]> 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.
+++ Steven Rostedt [19/03/21 16:57 -0400]:
>On Fri, 19 Mar 2021 20:34:24 +0100
>Peter Zijlstra <[email protected]> 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. */
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. */
>
+++ 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