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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] MODULE_SYSCTL_TABLE exports pointers to modpost, violating the devicetable design pattern and breaking module alias extraction.
- [Medium] Silent truncation of sysctl table size for large arrays.
- [Low] Brittle symbol name generation requires manual user workaround for struct field arguments.
- [Low] The commit message incorrectly documents the names of the control macros.
--

--- Patch [3]: [PATCH RFC v3 03/13] sysctl, mod_devicetable: add macro MODULE_SYSCTL_TABLE ---
commit 65937d6d2c35e0f567f1295d6273adbf6b625450
Author: Mauricio Faria de Oliveira <[email protected]>

sysctl, mod_devicetable: add macro MODULE_SYSCTL_TABLE

The MODULE_SYSCTL_TABLE macro emits a struct module_sysctl_table variable
with pointers to a sysctl table's path and entries, and table/entry sizes.

> The option macros MODULE_SYSCTL_TABLE_{DISABLE,UNIQUE_ID} can be used on
> a per-file basis to disable the macro and change the symbol name pattern.

[Severity: Low]
This isn't a bug, but the commit message documents these macros as
MODULE_SYSCTL_TABLE_DISABLE and MODULE_SYSCTL_TABLE_UNIQUE_ID, while the
code actually implements SYSCTL_MODULE_ALIASES_DISABLE and
SYSCTL_MODULE_ALIASES_UNIQUE_ID. Could the commit message be updated to
match the code?

> 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]
Will modpost be able to resolve these pointers? file2alias.c parses raw
section data from unlinked ELF objects to extract module aliases. Because
path and table are pointers rather than inline string arrays or structures,
modpost will read raw unresolved offsets instead of actual data.

This would break alias extraction on most architectures since file2alias.c
lacks a generalized relocation engine to resolve pointers across ELF sections.

> +	__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..0adc056ff54d8 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h

[ ... ]

> +#if defined(SYSCTL_MODULE_ALIASES_UNIQUE_ID)
> +#define __MODULE_SYSCTL_TABLE_NAME(table)					\
> +	__mod_device_table(sysctl, __UNIQUE_ID(sysctl))
> +#else
> +#define __MODULE_SYSCTL_TABLE_NAME(table)					\
> +	__mod_device_table(sysctl, table)
> +#endif

[Severity: Low]
Does this symbol name generation unnecessarily force manual workarounds? By
default, if table is passed as a struct member (like my_module.table), this
creates an invalid C identifier containing a dot. Would it be simpler to just
use __UNIQUE_ID unconditionally to avoid syntax errors?

> +
> +#define MODULE_SYSCTL_TABLE(path, table)					\
> +	static struct module_sysctl_table __used				\
> +		__MODULE_SYSCTL_TABLE_NAME(table) = {				\
> +			(kernel_ulong_t) path,					\
> +			(kernel_ulong_t) table,					\
> +			(u16) sizeof(table) + __must_be_array(table),		\

[Severity: Medium]
Could casting sizeof(table) to a u16 cause a silent truncation for very large
sysctl tables? 

While no current single static sysctl table in the kernel exceeds 65,535
bytes, casting size_t to a 16-bit integer creates a structural limit that
will silently report the wrong size if a table ever exceeds roughly 1,024
elements.

> +			(u16) sizeof(const struct ctl_table)			\
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3