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

[sashiko-bot@kernel.org]

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.
- [High] `__UNIQUE_ID` is used without arguments, preventing its expansion and causing redefinition compiler errors.
- [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.
- [Medium] The MODULE_SYSCTL_TABLE macro silently accepts pointers instead of arrays, resulting in an invalid table_size.
--

--- Patch [3]: [PATCH RFC v2 03/13] sysctl, mod_devicetable: add macro MODULE_SYSCTL_TABLE ---
commit 4b00a2bf658ca058127bbaaa61c3c168a3a4574c
Author: Mauricio Faria de Oliveira <mfo@igalia.com>

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;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-sysctl-module-aliases-v2-0-d5a69dae5798@igalia.com?part=3