linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] module: check for exit sections in layout_sections() instead of module_init_section()
@ 2021-05-12 14:46 Jessica Yu
  2021-05-12 16:06 ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 3+ messages in thread
From: Jessica Yu @ 2021-05-12 14:46 UTC (permalink / raw)
  To: linux-kernel; +Cc: Peter Zijlstra, Russell King, Jessica Yu

Previously, when CONFIG_MODULE_UNLOAD=n, the module loader just does not
attempt to load exit sections since it never expects that any code in those
sections will ever execute. However, dynamic code patching (alternatives,
jump_label and static_call) can have sites in __exit code, even if __exit is
never executed. Therefore __exit must be present at runtime, at least for as
long as __init code is.

Commit 33121347fb1c ("module: treat exit sections the same as init
sections when !CONFIG_MODULE_UNLOAD") solves the requirements of
jump_labels and static_calls by putting the exit sections in the init
region of the module so that they are at least present at init, and
discarded afterwards. It does this by including a check for exit
sections in module_init_section(), so that it also returns true for exit
sections, and the module loader will automatically sort them in the init
region of the module.

However, the solution there was not completely arch-independent. ARM is
a special case where it supplies its own module_{init, exit}_section()
functions. Instead of pushing the exit section checks into
module_init_section(), just implement the exit section check in
layout_sections(), so that we don't have to touch arch-dependent code.

Fixes: 33121347fb1c ("module: treat exit sections the same as init sections when !CONFIG_MODULE_UNLOAD")
Signed-off-by: Jessica Yu <jeyu@kernel.org>
---
 kernel/module.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 173a09175511..a5c9842371b1 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2430,6 +2430,9 @@ static void layout_sections(struct module *mod, struct load_info *info)
 			if ((s->sh_flags & masks[m][0]) != masks[m][0]
 			    || (s->sh_flags & masks[m][1])
 			    || s->sh_entsize != ~0UL
+#ifndef CONFIG_MODULE_UNLOAD
+			    || module_exit_section(sname)
+#endif
 			    || module_init_section(sname))
 				continue;
 			s->sh_entsize = get_offset(mod, &mod->core_layout.size, s, i);
@@ -2463,7 +2466,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
 			if ((s->sh_flags & masks[m][0]) != masks[m][0]
 			    || (s->sh_flags & masks[m][1])
 			    || s->sh_entsize != ~0UL
+#ifndef CONFIG_MODULE_UNLOAD
+			    || (!module_init_section(sname) && !module_exit_section(sname)))
+#else
 			    || !module_init_section(sname))
+#endif
 				continue;
 			s->sh_entsize = (get_offset(mod, &mod->init_layout.size, s, i)
 					 | INIT_OFFSET_MASK);
@@ -2802,11 +2809,7 @@ void * __weak module_alloc(unsigned long size)
 
 bool __weak module_init_section(const char *name)
 {
-#ifndef CONFIG_MODULE_UNLOAD
-	return strstarts(name, ".init") || module_exit_section(name);
-#else
 	return strstarts(name, ".init");
-#endif
 }
 
 bool __weak module_exit_section(const char *name)
-- 
2.31.1


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

* Re: [PATCH] module: check for exit sections in layout_sections() instead of module_init_section()
  2021-05-12 14:46 [PATCH] module: check for exit sections in layout_sections() instead of module_init_section() Jessica Yu
@ 2021-05-12 16:06 ` Russell King - ARM Linux admin
  2021-05-14  7:12   ` Jessica Yu
  0 siblings, 1 reply; 3+ messages in thread
From: Russell King - ARM Linux admin @ 2021-05-12 16:06 UTC (permalink / raw)
  To: Jessica Yu; +Cc: linux-kernel, Peter Zijlstra

Hi,

On Wed, May 12, 2021 at 04:46:53PM +0200, Jessica Yu wrote:
> diff --git a/kernel/module.c b/kernel/module.c
> index 173a09175511..a5c9842371b1 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2430,6 +2430,9 @@ static void layout_sections(struct module *mod, struct load_info *info)
>  			if ((s->sh_flags & masks[m][0]) != masks[m][0]
>  			    || (s->sh_flags & masks[m][1])
>  			    || s->sh_entsize != ~0UL
> +#ifndef CONFIG_MODULE_UNLOAD
> +			    || module_exit_section(sname)
> +#endif
>  			    || module_init_section(sname))

How about a helper to make this a bit easier in both these places to
make the code more undertsandable? I think the great value comes from
the resulting change in the second hunk.

static bool module_evictable_section(const char *sname)
{
#ifndef CONFIG_MODULE_UNLOAD
	if (module_exit_section(sname))
		return true;
#endif
	return module_init_section(sname);
}

and then just use that above?

>  				continue;
>  			s->sh_entsize = get_offset(mod, &mod->core_layout.size, s, i);
> @@ -2463,7 +2466,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>  			if ((s->sh_flags & masks[m][0]) != masks[m][0]
>  			    || (s->sh_flags & masks[m][1])
>  			    || s->sh_entsize != ~0UL
> +#ifndef CONFIG_MODULE_UNLOAD
> +			    || (!module_init_section(sname) && !module_exit_section(sname)))
> +#else
>  			    || !module_init_section(sname))
> +#endif

I find this a tad confusing, and this is the reason for my suggestion
above. With that, this becomes:

			    || !module_evictable_section(sname))

which can be clearly seen is the opposite condition from the above
without doing mental logic gymnastics.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

* Re: [PATCH] module: check for exit sections in layout_sections() instead of module_init_section()
  2021-05-12 16:06 ` Russell King - ARM Linux admin
@ 2021-05-14  7:12   ` Jessica Yu
  0 siblings, 0 replies; 3+ messages in thread
From: Jessica Yu @ 2021-05-14  7:12 UTC (permalink / raw)
  To: Russell King - ARM Linux admin; +Cc: linux-kernel, Peter Zijlstra

+++ Russell King - ARM Linux admin [12/05/21 17:06 +0100]:
>Hi,
>
>On Wed, May 12, 2021 at 04:46:53PM +0200, Jessica Yu wrote:
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 173a09175511..a5c9842371b1 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -2430,6 +2430,9 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>  			if ((s->sh_flags & masks[m][0]) != masks[m][0]
>>  			    || (s->sh_flags & masks[m][1])
>>  			    || s->sh_entsize != ~0UL
>> +#ifndef CONFIG_MODULE_UNLOAD
>> +			    || module_exit_section(sname)
>> +#endif
>>  			    || module_init_section(sname))
>
>How about a helper to make this a bit easier in both these places to
>make the code more undertsandable? I think the great value comes from
>the resulting change in the second hunk.
>
>static bool module_evictable_section(const char *sname)
>{
>#ifndef CONFIG_MODULE_UNLOAD
>	if (module_exit_section(sname))
>		return true;
>#endif
>	return module_init_section(sname);
>}
>
>and then just use that above?
>
>>  				continue;
>>  			s->sh_entsize = get_offset(mod, &mod->core_layout.size, s, i);
>> @@ -2463,7 +2466,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>  			if ((s->sh_flags & masks[m][0]) != masks[m][0]
>>  			    || (s->sh_flags & masks[m][1])
>>  			    || s->sh_entsize != ~0UL
>> +#ifndef CONFIG_MODULE_UNLOAD
>> +			    || (!module_init_section(sname) && !module_exit_section(sname)))
>> +#else
>>  			    || !module_init_section(sname))
>> +#endif
>
>I find this a tad confusing, and this is the reason for my suggestion
>above. With that, this becomes:
>
>			    || !module_evictable_section(sname))
>
>which can be clearly seen is the opposite condition from the above
>without doing mental logic gymnastics.

Thanks Russell for the feedback! Yeah, agreed that it could be made
easier to read - will respin with a helper function.

Jessica

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

end of thread, other threads:[~2021-05-14  7:12 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-12 14:46 [PATCH] module: check for exit sections in layout_sections() instead of module_init_section() Jessica Yu
2021-05-12 16:06 ` Russell King - ARM Linux admin
2021-05-14  7:12   ` Jessica Yu

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).