On 3/13/25 00:21, Sami Tolvanen wrote: > Hi Petr, > > On Wed, Mar 12, 2025 at 5:05 AM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote: >> >> On 3/7/25 01:12, Sami Tolvanen wrote: >>> On Thu, Mar 06, 2025 at 06:28:58PM +0100, Christophe Leroy wrote: >>>> Le 06/03/2025 à 14:13, Petr Pavlu a écrit : >>>>> Section .static_call_sites holds data structures that need to be sorted and >>>>> processed only at module load time. This initial processing happens in >>>>> static_call_add_module(), which is invoked as a callback to the >>>>> MODULE_STATE_COMING notification from prepare_coming_module(). >>>>> >>>>> The section is never modified afterwards. Make it therefore read-only after >>>>> module initialization to avoid any (non-)accidental modifications. >>>> >>>> Maybe this suggestion is stupid, I didn't investigate the feasability but: >>>> why don't we group everything that is ro_after_init in a single section just >>>> like we do in vmlinux ? That would avoid having to add every new possible >>>> section in the C code. >>>> >>>> Like we have in asm-generic/vmlinux.lds.h: >>>> >>>> #define RO_AFTER_INIT_DATA \ >>>> . = ALIGN(8); \ >>>> __start_ro_after_init = .; \ >>>> *(.data..ro_after_init) \ >>>> JUMP_TABLE_DATA \ >>>> STATIC_CALL_DATA \ >>>> __end_ro_after_init = .; >>> >>> I like this idea. Grouping the sections in the module linker script >>> feels cleaner than having an array of section names in the code. To be >>> fair, I think this code predates v5.10, where scripts/module.lds.S was >>> first added. >> >> I agree in principle. I like that the information about ro_after_init >> sections for vmlinux and modules would be in the same source form, in >> linker scripts. This could eventually allow us to share the definition >> of ro_after_init sections between vmlinux and modules. >> >> The problem is however how to find the location of the __jump_table and >> static_call_sites data. In vmlinux, as a final binary, they are >> annotated with start and end symbols. In modules, as relocatable files, >> the approach is to rely on them being separate sections, see function >> find_module_sections(). >> >> I could add start+end symbols for __jump_table and static_call_sites >> data in scripts/module.lds.S and use them by the module loader, but this >> would create an inconsistency in how various data is looked up. > > That's a fair point. Perhaps it makes sense to keep these sections > separate for consistency, and look into cleaning this up later if > needed. > >> Another problem is that I can't find a way to tell the linker to add these >> symbols only if the specific data is actually present. > > You can use the preprocessor to add the symbols only if the relevant > kernel config is present, similarly to how STATIC_CALL_DATA is defined > in include/asm-generic/vmlinux.lds.h. Right, that works but only statically. Let's say I update module.lds.S to: SECTIONS { [...] .data..ro_after_init { *(.data..ro_after_init) __start___jump_table = .; *(__jump_table) __end___jump_table = .; #ifdef CONFIG_HAVE_STATIC_CALL_INLINE __start_static_call_sites = .; *(.static_call_sites) __end_static_call_sites = .; #endif } } What I had in mind is that you can configure the kernel with CONFIG_HAVE_STATIC_CALL_INLINE, but some modules might not contain any static calls and so wouldn't have a .static_call_sites section. When using the above script, such modules would still end up with unnecessary symbols __start_static_call_sites and __end_static_call_sites defining an empty range. Worse, if the module doesn't contain any ro_after_init data, the added symbols would force the linker to create an empty .data..ro_after_init section. > > In any case, the code looks correct to me. For the series: > > Reviewed-by: Sami Tolvanen <samitolvanen@xxxxxxxxxx> -- Thanks, Petr