Re: [PATCH RFC v2 03/13] sysctl, mod_devicetable: add macro MODULE_SYSCTL_TABLE

On 2026-08-18 23:40, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] The file2alias.c handler for sysctl device tables is entirely missing from the patch, resulting in no module aliases being generated.

This is handled in a later patch.

> - [High] `__UNIQUE_ID` is used without arguments, preventing its expansion and causing redefinition compiler errors.

Fixed.

> - [High] Pointers in device table structs cannot be dereferenced by modpost without relocation resolution.
> - [High] modpost cannot parse `struct ctl_table` due to randomized layout and nested pointers.
> - [High] Macro generates a single struct, but modpost expects a NULL-terminated array.

These are handled in later patches.

> - [Medium] The MODULE_SYSCTL_TABLE macro silently accepts pointers instead of arrays, resulting in an invalid table_size.

Fixed.

Note: this case has been checked during design, and it fails due to
pointers not being constant but used as static initializers (must be
constant); however, there's indeed a exposure with constant pointers
(e.g., 'static const struct ctl_table *const ptr' -- not seen in
practice, but theoretically possible).


> --
> 
> --- Patch [3]: [PATCH RFC v2 03/13] sysctl, mod_devicetable: add macro MODULE_SYSCTL_TABLE ---
> commit 4b00a2bf658ca058127bbaaa61c3c168a3a4574c
> Author: Mauricio Faria de Oliveira <[email protected]>
> 
> sysctl, mod_devicetable: add macro MODULE_SYSCTL_TABLE
> 
> This commit adds the MODULE_SYSCTL_TABLE macro to emit a struct
> module_sysctl_table variable containing pointers to a sysctl table's path
> and entries. It also adds the necessary struct definitions and field
> offsets for modpost integration.
> 
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index a397213bedace..818a4a53448db 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
> 
> [ ... ]
> 
>> @@ -78,4 +78,11 @@ struct cpu_feature {
>>  	__u16	feature;
>>  };
>> 
>> +struct module_sysctl_table {
>> +	kernel_ulong_t path;
>> +	kernel_ulong_t table;
> 
> [Severity: High]
> Does modpost know how to read these external pointers?
> 
> Modpost's file2alias.c extracts raw bytes from the struct and typically
> does not resolve relocations to dereference .rodata or .data pointers. Will
> this prevent modpost from actually reading the path or table strings/data
> from unlinked ELF object files (.o)?
> 
>> +	__u16 table_size;
>> +	__u16 entry_size;
>> +};
>> +
>>  #endif /* LINUX_MOD_DEVICETABLE_H */
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index e5d7226ab6f5a..6960fe22c50bf 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
> 
> [ ... ]
> 
>> @@ -293,6 +293,48 @@ struct ctl_table_root {
>>  	int (*permissions)(struct ctl_table_header *head, const struct ctl_table *table);
>>  };
>> 
>> +/*
>> + * Creates a symbol so file2alias.c can find the sysctl table.
>> + *
>> + * MODULE_SYSCTL_TABLE() is the sysctl equivalent of MODULE_DEVICE_TABLE(),
>> + * with pointers to a sysctl table's path and entries, and table/entry sizes.
>> + *
>> + * The parameters 'path' and 'table' must be static (e.g., string literal or
>> + * static char[]; static struct ctl_table[]) as both are used as initializer
>> + * elements for a static struct variable.
>> + *
>> + * The generated symbol name contains 'table' by default. However, if 'table'
>> + * is a struct field (contains '.' or '->'), in order to avoid syntax errors,
>> + * define the macro SYSCTL_MODULE_ALIASES_UNIQUE_ID.
>> + *
>> + * Define the macro SYSCTL_MODULE_ALIASES_DISABLE to disable this.
>> + */
>> +#if defined(CONFIG_SYSCTL_MODULE_ALIASES) && defined(MODULE) &&			\
>> +    !defined(SYSCTL_MODULE_ALIASES_DISABLE)
>> +
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +
>> +#if defined(SYSCTL_MODULE_ALIASES_UNIQUE_ID)
>> +#define __MODULE_SYSCTL_TABLE_NAME(table)					\
>> +	__mod_device_table(sysctl, __UNIQUE_ID)
> 
> [Severity: High]
> Does this use of __UNIQUE_ID work correctly without an argument?
> 
> Since __UNIQUE_ID is a function-like macro, using it without parentheses
> prevents its expansion. This could result in the preprocessor treating it
> as a literal token, generating a variable name like ...__sysctl____UNIQUE_ID
> and causing redefinition errors if invoked multiple times in the same file.
> 
>> +#else
>> +#define __MODULE_SYSCTL_TABLE_NAME(table)					\
>> +	__mod_device_table(sysctl, table)
>> +#endif
>> +
>> +#define MODULE_SYSCTL_TABLE(path, table)					\
>> +	static struct module_sysctl_table __used				\
>> +		__MODULE_SYSCTL_TABLE_NAME(table) = {				\
> 
> [Severity: High]
> Will this work with modpost's do_table() logic?
> 
> This macro generates a single struct instance, but modpost expects all
> __mod_device_table symbols to point to an array terminated by an all-zero
> element. Scanning this single struct could cause a build-time
> "not terminated with a NULL entry" error when it sees non-zero bytes.
> 
>> +			(kernel_ulong_t) path,					\
>> +			(kernel_ulong_t) table,					\
> 
> [Severity: High]
> If modpost attempts to parse struct ctl_table through this pointer, how does
> it handle a randomized struct layout?
> 
> Since struct ctl_table can be marked with __randomize_layout when
> CONFIG_RANDSTRUCT is enabled, a host tool like modpost cannot reliably extract
> information from these entries without having the field offsets explicitly
> exported in devicetable-offsets.c.
> 
>> +			(u16) sizeof(table),					\
> 
> [Severity: Medium]
> Can this silently evaluate to a pointer size instead of the array size?
> 
> If a pointer to a ctl_table is passed instead of a static array, sizeof(table)
> evaluates to the pointer size, leading to an incorrect table size being
> emitted. Could this be protected using __must_be_array(table)?
> 
>> +			(u16) sizeof(const struct ctl_table)			\
>> +		}
>> +#else
>> +#define MODULE_SYSCTL_TABLE(path, table)
>> +#endif
>> +
>>  #define register_sysctl(path, table)	\
>>  	register_sysctl_sz(path, table, ARRAY_SIZE(table))
>> 
>> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
>> index b4178c42d08f5..e3b8855e6ebcf 100644
>> --- a/scripts/mod/devicetable-offsets.c
>> +++ b/scripts/mod/devicetable-offsets.c
> 
> [ ... ]
> 
>> @@ -284,5 +284,11 @@ int main(void)
>>  	DEVID(coreboot_device_id);
>>  	DEVID_FIELD(coreboot_device_id, tag);
>> 
>> +	DEVID(module_sysctl_table);
> 
> [Severity: High]
> Where is the file2alias.c handler for sysctl device tables?
> 
> The commit message mentions this symbol will be handled by file2alias.c,
> but the patch does not appear to add the corresponding entry in the
> devtable[] array to actually parse the new struct. Will this result in
> no module aliases being generated at all?
> 
>> +	DEVID_FIELD(module_sysctl_table, path);
>> +	DEVID_FIELD(module_sysctl_table, table);
>> +	DEVID_FIELD(module_sysctl_table, table_size);
>> +	DEVID_FIELD(module_sysctl_table, entry_size);
>> +
>>  	return 0;
>>  }

-- 
Mauricio