[PATCH 0/3] ARM/arm64: Fix loading of modules with an exit section

[PATCH 0/3] ARM/arm64: Fix loading of modules with an exit section

[James Morse]

Adam reports that Yocto can't load modules on arm64. This turns out to be due
to the arch code disagreeing with the core code when it comes to the layout
of the modules exit text, resulting in a shortage of PLTs and a bunch of
warnings.

arm and arm64 are unusual here as they are counting the PLTs based on the
section name. This series exposes the helper that core code uses to decide
the layout.

I've been unable to reproduce the behaviour on 32bit - but it looks like
its possible to reach the BUG_ON() in get_module_plt(). To test this, disable
CONFIG_MODULE_UNLOAD, and try to load modules with relocations in their exit
text.

This series is based on v6.5-rc4, and can be retrieved from:
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git arm64/modules/exit_sections/v1

Thanks,

James Morse (3):
  module: Expose module_init_layout_section()
  arm64: module: Use module_init_layout_section() to spot init sections
  ARM: module: Use module_init_layout_section() to spot init sections

 arch/arm/kernel/module-plts.c   | 2 +-
 arch/arm64/kernel/module-plts.c | 2 +-
 include/linux/moduleloader.h    | 5 +++++
 kernel/module/main.c            | 2 +-
 4 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.39.2

[PATCH 1/3] module: Expose module_init_layout_section()

[James Morse]

module_init_layout_section() choses whether the core module loader
considers a section as init or not. This affects the placement of the
exit section when module unloading is disabled. This code will never run,
so it can be free()d once the module has been initialised.

arm and arm64 need to count the number of PLTs they need before applying
relocations based on the section name. The init PLTs are stored separately
so they can be free()d. arm and arm64 both use within_module_init() to
decide which list of PLTs to use when applying the relocation.

Because within_module_init()'s behaviour changes when module unloading
is disabled, both architecture would need to take this into account when
counting the PLTs.

Today neither architecture does this, meaning when module unloading is
disabled there are insufficient PLTs in the init section to load some
modules, resulting in warnings:
| WARNING: CPU: 2 PID: 51 at arch/arm64/kernel/module-plts.c:99 module_emit_plt_entry+0x184/0x1cc
| Modules linked in: crct10dif_common
| CPU: 2 PID: 51 Comm: modprobe Not tainted 6.5.0-rc4-yocto-standard-dirty #15208
| Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
| pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
| pc : module_emit_plt_entry+0x184/0x1cc
| lr : module_emit_plt_entry+0x94/0x1cc
| sp : ffffffc0803bba60
[...]
| Call trace:
|  module_emit_plt_entry+0x184/0x1cc
|  apply_relocate_add+0x2bc/0x8e4
|  load_module+0xe34/0x1bd4
|  init_module_from_file+0x84/0xc0
|  __arm64_sys_finit_module+0x1b8/0x27c
|  invoke_syscall.constprop.0+0x5c/0x104
|  do_el0_svc+0x58/0x160
|  el0_svc+0x38/0x110
|  el0t_64_sync_handler+0xc0/0xc4
|  el0t_64_sync+0x190/0x194

Instead of duplicating module_init_layout_section()s logic, expose it.

Reported-by: Adam Johnston <adam.johnston@arm.com>
Fixes: 055f23b74b20 ("module: check for exit sections in layout_sections() instead of module_init_section()")
Cc: stable@vger.kernel.org
Signed-off-by: James Morse <james.morse@arm.com>
---
Adam has seen this in Yocto using the v6.1 stable kernel
---
 include/linux/moduleloader.h | 5 +++++
 kernel/module/main.c         | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 03be088fb439..001b2ce83832 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -42,6 +42,11 @@ bool module_init_section(const char *name);
  */
 bool module_exit_section(const char *name);
 
+/* Describes whether within_module_init() will consider this an init section
+ * or not. This behaviour changes with CONFIG_MODULE_UNLOAD.
+ */
+bool module_init_layout_section(const char *sname);
+
 /*
  * Apply the given relocation to the (simplified) ELF.  Return -error
  * or 0.
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 59b1d067e528..ff7cc4e29299 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1484,7 +1484,7 @@ long module_get_offset_and_type(struct module *mod, enum mod_mem_type type,
 	return offset | mask;
 }
 
-static bool module_init_layout_section(const char *sname)
+bool module_init_layout_section(const char *sname)
 {
 #ifndef CONFIG_MODULE_UNLOAD
 	if (module_exit_section(sname))
-- 
2.39.2

[PATCH 2/3] arm64: module: Use module_init_layout_section() to spot init sections

[James Morse]

Today module_frob_arch_sections() spots init sections from their
'init' prefix, and uses this to keep the init PLTs separate from the rest.

module_emit_plt_entry() uses within_module_init() to determine if a
location is in the init text or not, but this depends on whether
core code thought this was an init section.

Naturally the logic is different.

module_init_layout_section() groups the init and exit text together if
module unloading is disabled, as the exit code will never run. The result
is kernels with this configuration can't load all their modules because
there are not enough PLTs for the combined init+exit section.

This results in the following:
| WARNING: CPU: 2 PID: 51 at arch/arm64/kernel/module-plts.c:99 module_emit_plt_entry+0x184/0x1cc
| Modules linked in: crct10dif_common
| CPU: 2 PID: 51 Comm: modprobe Not tainted 6.5.0-rc4-yocto-standard-dirty #15208
| Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
| pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
| pc : module_emit_plt_entry+0x184/0x1cc
| lr : module_emit_plt_entry+0x94/0x1cc
| sp : ffffffc0803bba60
[...]
| Call trace:
|  module_emit_plt_entry+0x184/0x1cc
|  apply_relocate_add+0x2bc/0x8e4
|  load_module+0xe34/0x1bd4
|  init_module_from_file+0x84/0xc0
|  __arm64_sys_finit_module+0x1b8/0x27c
|  invoke_syscall.constprop.0+0x5c/0x104
|  do_el0_svc+0x58/0x160
|  el0_svc+0x38/0x110
|  el0t_64_sync_handler+0xc0/0xc4
|  el0t_64_sync+0x190/0x194

A previous patch exposed module_init_layout_section(), use that so the
logic is the same.

Reported-by: Adam Johnston <adam.johnston@arm.com>
Tested-by: Adam Johnston <adam.johnston@arm.com>
Fixes: 055f23b74b20 ("module: check for exit sections in layout_sections() instead of module_init_section()")
Cc: <stable@vger.kernel.org> # 5.15.x: 60a0aab7463ee69 arm64: module-plts: inline linux/moduleloader.h
Cc: <stable@vger.kernel.org> # 5.15.x
Signed-off-by: James Morse <james.morse@arm.com>
---
---
 arch/arm64/kernel/module-plts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index ad02058756b5..bd69a4e7cd60 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -339,7 +339,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 		if (nents)
 			sort(rels, nents, sizeof(Elf64_Rela), cmp_rela, NULL);
 
-		if (!str_has_prefix(secstrings + dstsec->sh_name, ".init"))
+		if (!module_init_layout_section(secstrings + dstsec->sh_name))
 			core_plts += count_plts(syms, rels, numrels,
 						sechdrs[i].sh_info, dstsec);
 		else
-- 
2.39.2

[PATCH 3/3] ARM: module: Use module_init_layout_section() to spot init sections

[James Morse]

Today module_frob_arch_sections() spots init sections from their
'init' prefix, and uses this to keep the init PLTs separate from the rest.

get_module_plt() uses within_module_init() to determine if a
location is in the init text or not, but this depends on whether
core code thought this was an init section.

Naturally the logic is different.

module_init_layout_section() groups the init and exit text together if
module unloading is disabled, as the exit code will never run. The result
is kernels with this configuration can't load all their modules because
there are not enough PLTs for the combined init+exit section.

A previous patch exposed module_init_layout_section(), use that so the
logic is the same.

Fixes: 055f23b74b20 ("module: check for exit sections in layout_sections() instead of module_init_section()")
Cc: stable@vger.kernel.org
Signed-off-by: James Morse <james.morse@arm.com>
---
I've not managed to get 32bit to call get_module_plt(), but it looks
possible to hit the BUG_ON() because of this.
---
 arch/arm/kernel/module-plts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
index f5a43fd8c163..da2ee8d6ef1a 100644
--- a/arch/arm/kernel/module-plts.c
+++ b/arch/arm/kernel/module-plts.c
@@ -251,7 +251,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 		/* sort by type and symbol index */
 		sort(rels, numrels, sizeof(Elf32_Rel), cmp_rel, NULL);
 
-		if (strncmp(secstrings + dstsec->sh_name, ".init", 5) != 0)
+		if (!module_init_layout_section(secstrings + dstsec->sh_name))
 			core_plts += count_plts(syms, dstsec->sh_addr, rels,
 						numrels, s->sh_info);
 		else
-- 
2.39.2

Re: [PATCH 0/3] ARM/arm64: Fix loading of modules with an exit section

[Luis Chamberlain]

On Tue, Aug 01, 2023 at 02:54:06PM +0000, James Morse wrote:
> Adam reports that Yocto can't load modules on arm64. This turns out to be due
> to the arch code disagreeing with the core code when it comes to the layout
> of the modules exit text, resulting in a shortage of PLTs and a bunch of
> warnings.
> 
> arm and arm64 are unusual here as they are counting the PLTs based on the
> section name. This series exposes the helper that core code uses to decide
> the layout.
> 
> I've been unable to reproduce the behaviour on 32bit - but it looks like
> its possible to reach the BUG_ON() in get_module_plt(). To test this, disable
> CONFIG_MODULE_UNLOAD, and try to load modules with relocations in their exit
> text.
> 
> This series is based on v6.5-rc4, and can be retrieved from:
> https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git arm64/modules/exit_sections/v1
> 

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

Do you want this to go through the modules tree or do you want to take
this in your tree? Either way is fine by me, at this point there should
be no conflicts.

  Luis

Re: [PATCH 0/3] ARM/arm64: Fix loading of modules with an exit section

[James Morse]

Hi Luis,

On 01/08/2023 18:14, Luis Chamberlain wrote:
> On Tue, Aug 01, 2023 at 02:54:06PM +0000, James Morse wrote:
>> Adam reports that Yocto can't load modules on arm64. This turns out to be due
>> to the arch code disagreeing with the core code when it comes to the layout
>> of the modules exit text, resulting in a shortage of PLTs and a bunch of
>> warnings.
>>
>> arm and arm64 are unusual here as they are counting the PLTs based on the
>> section name. This series exposes the helper that core code uses to decide
>> the layout.
>>
>> I've been unable to reproduce the behaviour on 32bit - but it looks like
>> its possible to reach the BUG_ON() in get_module_plt(). To test this, disable
>> CONFIG_MODULE_UNLOAD, and try to load modules with relocations in their exit
>> text.
>>
>> This series is based on v6.5-rc4, and can be retrieved from:
>> https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git arm64/modules/exit_sections/v1
>>
> 
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

Thanks!


> Do you want this to go through the modules tree or do you want to take
> this in your tree? Either way is fine by me, at this point there should
> be no conflicts.

If Russell agrees the problem exists on 32bit arm, then I think it would be best to keep
these three together - going via the modules tree would make the most sense.

This has been broken for a while, so it can wait for v6.6-rc1.
I think the yocto folk plan to carry this out of tree until its in their chosen stable
version.


Thanks,

James

Re: [PATCH 2/3] arm64: module: Use module_init_layout_section() to spot init sections

[Catalin Marinas]

On Tue, Aug 01, 2023 at 02:54:08PM +0000, James Morse wrote:
> Today module_frob_arch_sections() spots init sections from their
> 'init' prefix, and uses this to keep the init PLTs separate from the rest.
> 
> module_emit_plt_entry() uses within_module_init() to determine if a
> location is in the init text or not, but this depends on whether
> core code thought this was an init section.
> 
> Naturally the logic is different.
> 
> module_init_layout_section() groups the init and exit text together if
> module unloading is disabled, as the exit code will never run. The result
> is kernels with this configuration can't load all their modules because
> there are not enough PLTs for the combined init+exit section.
> 
> This results in the following:
> | WARNING: CPU: 2 PID: 51 at arch/arm64/kernel/module-plts.c:99 module_emit_plt_entry+0x184/0x1cc
> | Modules linked in: crct10dif_common
> | CPU: 2 PID: 51 Comm: modprobe Not tainted 6.5.0-rc4-yocto-standard-dirty #15208
> | Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> | pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> | pc : module_emit_plt_entry+0x184/0x1cc
> | lr : module_emit_plt_entry+0x94/0x1cc
> | sp : ffffffc0803bba60
> [...]
> | Call trace:
> |  module_emit_plt_entry+0x184/0x1cc
> |  apply_relocate_add+0x2bc/0x8e4
> |  load_module+0xe34/0x1bd4
> |  init_module_from_file+0x84/0xc0
> |  __arm64_sys_finit_module+0x1b8/0x27c
> |  invoke_syscall.constprop.0+0x5c/0x104
> |  do_el0_svc+0x58/0x160
> |  el0_svc+0x38/0x110
> |  el0t_64_sync_handler+0xc0/0xc4
> |  el0t_64_sync+0x190/0x194
> 
> A previous patch exposed module_init_layout_section(), use that so the
> logic is the same.
> 
> Reported-by: Adam Johnston <adam.johnston@arm.com>
> Tested-by: Adam Johnston <adam.johnston@arm.com>
> Fixes: 055f23b74b20 ("module: check for exit sections in layout_sections() instead of module_init_section()")
> Cc: <stable@vger.kernel.org> # 5.15.x: 60a0aab7463ee69 arm64: module-plts: inline linux/moduleloader.h
> Cc: <stable@vger.kernel.org> # 5.15.x
> Signed-off-by: James Morse <james.morse@arm.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH 0/3] ARM/arm64: Fix loading of modules with an exit section

[Russell King (Oracle)]

On Wed, Aug 02, 2023 at 05:28:10PM +0100, James Morse wrote:
> Hi Luis,
> 
> On 01/08/2023 18:14, Luis Chamberlain wrote:
> > On Tue, Aug 01, 2023 at 02:54:06PM +0000, James Morse wrote:
> >> Adam reports that Yocto can't load modules on arm64. This turns out to be due
> >> to the arch code disagreeing with the core code when it comes to the layout
> >> of the modules exit text, resulting in a shortage of PLTs and a bunch of
> >> warnings.
> >>
> >> arm and arm64 are unusual here as they are counting the PLTs based on the
> >> section name. This series exposes the helper that core code uses to decide
> >> the layout.
> >>
> >> I've been unable to reproduce the behaviour on 32bit - but it looks like
> >> its possible to reach the BUG_ON() in get_module_plt(). To test this, disable
> >> CONFIG_MODULE_UNLOAD, and try to load modules with relocations in their exit
> >> text.
> >>
> >> This series is based on v6.5-rc4, and can be retrieved from:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git arm64/modules/exit_sections/v1
> >>
> > 
> > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
> 
> Thanks!
> 
> 
> > Do you want this to go through the modules tree or do you want to take
> > this in your tree? Either way is fine by me, at this point there should
> > be no conflicts.
> 
> If Russell agrees the problem exists on 32bit arm, then I think it would be best to keep
> these three together - going via the modules tree would make the most sense.
> 
> This has been broken for a while, so it can wait for v6.6-rc1.
> I think the yocto folk plan to carry this out of tree until its in their chosen stable
> version.

The thing about PLTs is that it's something I've never had the need to
make use of - because I've never been in the situation where the
arm32 module space has been close to overflowing. The addition of PLT
support for 32-bit arm did make my eyebrows raise for this very reason,
but I guess there are a small number of people who want to use really
large modules.

As such, I couldn't say whether it's broken or not - but it seems
sensible to keep both the 64-bit and 32-bit code tracking each other.

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

Re: [PATCH 0/3] ARM/arm64: Fix loading of modules with an exit section

[Luis Chamberlain]

On Thu, Aug 03, 2023 at 11:20:06AM +0100, Russell King (Oracle) wrote:
> On Wed, Aug 02, 2023 at 05:28:10PM +0100, James Morse wrote:
> > If Russell agrees the problem exists on 32bit arm, then I think it
> > would be best to keep these three together - going via the modules
> > tree would make the most sense.
> 
> The thing about PLTs is that it's something I've never had the need to
> make use of - because I've never been in the situation where the
> arm32 module space has been close to overflowing. The addition of PLT
> support for 32-bit arm did make my eyebrows raise for this very reason,
> but I guess there are a small number of people who want to use really
> large modules.
> 
> As such, I couldn't say whether it's broken or not - but it seems
> sensible to keep both the 64-bit and 32-bit code tracking each other.

Alrighty, merged and pushed onto modules-next.

  Luis