| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <xptwb3uwbzposd4xf7khj52ifv4tchcjdgllhv7aabi6d7wgef@2msurl564v53>
Date: Wed, 21 Jan 2026 15:30:28 +0100
From: Joel Granados <joel.granados@...nel.org>
To: wen.yang@...ux.dev
Cc: linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/4] sysctl: add SYSCTL_ENTRY/SYSCTL_RANGE_ENTRY
helpers for ctl_table init
Hey Wen
Going to comment only on this patch. As I want to concentrate in getting
the macro just right. After that we can continue with further changes.
With this in mind, you might just want to drop the other commits for
now... up to you.
On Wed, Jan 14, 2026 at 01:40:30AM +0800, wen.yang@...ux.dev wrote:
> From: Wen Yang <wen.yang@...ux.dev>
>
> Introduce SYSCTL_ENTRY() and SYSCTL_RANGE_ENTRY() (built on __SYSCTL_ENTRY())
> to consolidate common struct ctl_table initializers in one place.
> This is a preparatory refactor in support of future tree-wide sysctl
> cleanups by making the how to build a ctl_table entry logic uniform.
>
> Also add SYSCTL_NULL for explicitly passing a NULL .data pointer through
> the new macros, and convert a few in-tree users (kernel/sysctl.c) to
> validate the new helpers.
>
> No functional change intended.
>
> Selftest were also made:
>
> [19:31:40] Starting KUnit Kernel (1/1)...
> [19:31:40] ============================================================
> Running tests with:
> $ .kunit/linux kunit.filter_glob=sysctl_test kunit.enable=1 mem=1G console=tty kunit_shutdown=halt
> [19:31:40] ================ sysctl_test (10 subtests) =================
> [19:31:40] [PASSED] sysctl_test_api_dointvec_null_tbl_data
> [19:31:40] [PASSED] sysctl_test_api_dointvec_table_maxlen_unset
> [19:31:40] [PASSED] sysctl_test_api_dointvec_table_len_is_zero
> [19:31:40] [PASSED] sysctl_test_api_dointvec_table_read_but_position_set
> [19:31:40] [PASSED] sysctl_test_dointvec_read_happy_single_positive
> [19:31:40] [PASSED] sysctl_test_dointvec_read_happy_single_negative
> [19:31:40] [PASSED] sysctl_test_dointvec_write_happy_single_positive
> [19:31:40] [PASSED] sysctl_test_dointvec_write_happy_single_negative
> [19:31:40] [PASSED] sysctl_test_api_dointvec_write_single_less_int_min
> [19:31:40] [PASSED] sysctl_test_api_dointvec_write_single_greater_int_max
> [19:31:40] =================== [PASSED] sysctl_test ===================
> [19:31:40] ============================================================
> [19:31:40] Testing complete. Ran 10 tests: passed: 10
> [19:31:40] Elapsed time: 26.755s total, 0.002s configuring, 26.632s building, 0.102s running
Thx for getting the ball rolling by the way. And it is a very good idea
to include the tests early on. Can you pls create two commits from this
one: one for the introduction of the macro and the other for the changes
in the kunit sysctl tests.
>
> Suggested-by: Joel Granados <joel.granados@...nel.org>
> Link: https://lore.kernel.org/all/7mevcgca7xsd5gckrap22byjinhrgqnelu4y7hzo2r5t2cq7w4@nyaa42khwqez/#t
> Signed-off-by: Wen Yang <wen.yang@...ux.dev>
> ---
> include/linux/sysctl.h | 18 ++++++
> kernel/sysctl-test.c | 131 ++++++++++-------------------------------
> kernel/sysctl.c | 43 ++------------
> 3 files changed, 56 insertions(+), 136 deletions(-)
>
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 2886fbceb5d6..683422bc3e7f 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -53,6 +53,7 @@ struct ctl_dir;
> #define SYSCTL_MAXOLDUID ((void *)&sysctl_vals[10])
> #define SYSCTL_NEG_ONE ((void *)&sysctl_vals[11])
>
> +#define SYSCTL_NULL ((void *)NULL)
> extern const int sysctl_vals[];
>
> #define SYSCTL_LONG_ZERO ((void *)&sysctl_long_vals[0])
> @@ -175,6 +176,23 @@ struct ctl_table {
> void *extra2;
> } __randomize_layout;
>
> +#define __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, HANDLER, SMIN, SMAX)\
> + { \
> + .procname = NAME, \
> + .data = DATA, \
> + .maxlen = sizeof(TYPE), \
> + .mode = MODE, \
> + .proc_handler = HANDLER, \
> + .extra1 = SMIN, \
> + .extra2 = SMAX, \
> + }
> +
> +#define SYSCTL_ENTRY(NAME, DATA, TYPE, MODE) \
> + __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, proc_do##TYPE##vec, NULL, NULL)
> +
> +#define SYSCTL_RANGE_ENTRY(NAME, DATA, TYPE, MODE, SMIN, SMAX) \
> + __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, proc_do##TYPE##vec_minmax, SMIN, SMAX)
> +
I included type in my initial prototype, but I'm wondering if we can get
away with removing TYPE and figuring out what proc_doXXXXXvec function
we call based on DATA (maybe __same_type? or __builtin_types_compatible_p?
or something else?) This would make the MACRO even easier to use as it
would just be "SYSCTL_ENTRY(NAME, DATA, MODE)"
The user can always pass a DATA that is a different type from the TYPE
and if they are compatible, the compiler will probably not scream. We
have probably removed most of these, but I would like to have a compiler
warning if it sees that the type does not agree with the handler
type.
> struct ctl_node {
> struct rb_node node;
> struct ctl_table_header *header;
> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> index 92f94ea28957..7fb0e7f1e62f 100644
> --- a/kernel/sysctl-test.c
> +++ b/kernel/sysctl-test.c
> @@ -15,20 +15,14 @@
> */
> static void sysctl_test_api_dointvec_null_tbl_data(struct kunit *test)
> {
> - struct ctl_table null_data_table = {
> - .procname = "foo",
> - /*
> - * Here we are testing that proc_dointvec behaves correctly when
> - * we give it a NULL .data field. Normally this would point to a
> - * piece of memory where the value would be stored.
> - */
> - .data = NULL,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> + /*
> + * Here we are testing that proc_dointvec behaves correctly when
> + * we give it a NULL .data field. Normally this would point to a
> + * piece of memory where the value would be stored.
> + */
> + struct ctl_table null_data_table = SYSCTL_RANGE_ENTRY("foo", \
> + SYSCTL_NULL, int, 0644, SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> +
> /*
> * proc_dointvec expects a buffer in user space, so we allocate one. We
> * also need to cast it to __user so sparse doesn't get mad.
> @@ -66,19 +60,14 @@ static void sysctl_test_api_dointvec_null_tbl_data(struct kunit *test)
> static void sysctl_test_api_dointvec_table_maxlen_unset(struct kunit *test)
> {
> int data = 0;
> - struct ctl_table data_maxlen_unset_table = {
> - .procname = "foo",
> - .data = &data,
> - /*
> - * So .data is no longer NULL, but we tell proc_dointvec its
> - * length is 0, so it still shouldn't try to use it.
> - */
> - .maxlen = 0,
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> +
> + /*
> + * So .data is no longer NULL, but we tell proc_dointvec its
> + * length is 0, so it still shouldn't try to use it.
> + */
> + struct ctl_table data_maxlen_unset_table = SYSCTL_RANGE_ENTRY("foo", \
> + &data, int, 0644, SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> + data_maxlen_unset_table.maxlen = 0;
> void __user *buffer = (void __user *)kunit_kzalloc(test, sizeof(int),
> GFP_USER);
> size_t len;
> @@ -113,15 +102,8 @@ static void sysctl_test_api_dointvec_table_len_is_zero(struct kunit *test)
> {
> int data = 0;
> /* Good table. */
> - struct ctl_table table = {
> - .procname = "foo",
> - .data = &data,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> + struct ctl_table table = SYSCTL_RANGE_ENTRY("foo", &data, int, 0644,\
> + SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> void __user *buffer = (void __user *)kunit_kzalloc(test, sizeof(int),
> GFP_USER);
> /*
> @@ -147,15 +129,8 @@ static void sysctl_test_api_dointvec_table_read_but_position_set(
> {
> int data = 0;
> /* Good table. */
> - struct ctl_table table = {
> - .procname = "foo",
> - .data = &data,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> + struct ctl_table table = SYSCTL_RANGE_ENTRY("foo", &data, int, 0644,\
> + SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> void __user *buffer = (void __user *)kunit_kzalloc(test, sizeof(int),
> GFP_USER);
> /*
> @@ -182,15 +157,8 @@ static void sysctl_test_dointvec_read_happy_single_positive(struct kunit *test)
> {
> int data = 0;
> /* Good table. */
> - struct ctl_table table = {
> - .procname = "foo",
> - .data = &data,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> + struct ctl_table table = SYSCTL_RANGE_ENTRY("foo", &data, int, 0644, \
> + SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> size_t len = 4;
> loff_t pos = 0;
> char *buffer = kunit_kzalloc(test, len, GFP_USER);
> @@ -213,15 +181,8 @@ static void sysctl_test_dointvec_read_happy_single_negative(struct kunit *test)
> {
> int data = 0;
> /* Good table. */
> - struct ctl_table table = {
> - .procname = "foo",
> - .data = &data,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> + struct ctl_table table = SYSCTL_RANGE_ENTRY("foo", &data, int, 0644, \
> + SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> size_t len = 5;
> loff_t pos = 0;
> char *buffer = kunit_kzalloc(test, len, GFP_USER);
> @@ -242,15 +203,8 @@ static void sysctl_test_dointvec_write_happy_single_positive(struct kunit *test)
> {
> int data = 0;
> /* Good table. */
> - struct ctl_table table = {
> - .procname = "foo",
> - .data = &data,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> + struct ctl_table table = SYSCTL_RANGE_ENTRY("foo", &data, int, 0644, \
> + SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> char input[] = "9";
> size_t len = sizeof(input) - 1;
> loff_t pos = 0;
> @@ -272,15 +226,8 @@ static void sysctl_test_dointvec_write_happy_single_positive(struct kunit *test)
> static void sysctl_test_dointvec_write_happy_single_negative(struct kunit *test)
> {
> int data = 0;
> - struct ctl_table table = {
> - .procname = "foo",
> - .data = &data,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> + struct ctl_table table = SYSCTL_RANGE_ENTRY("foo", &data, int, 0644, \
> + SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> char input[] = "-9";
> size_t len = sizeof(input) - 1;
> loff_t pos = 0;
> @@ -304,15 +251,8 @@ static void sysctl_test_api_dointvec_write_single_less_int_min(
> struct kunit *test)
> {
> int data = 0;
> - struct ctl_table table = {
> - .procname = "foo",
> - .data = &data,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> + struct ctl_table table = SYSCTL_RANGE_ENTRY("foo", &data, int, 0644,\
> + SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> size_t max_len = 32, len = max_len;
> loff_t pos = 0;
> char *buffer = kunit_kzalloc(test, max_len, GFP_USER);
> @@ -342,15 +282,8 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max(
> struct kunit *test)
> {
> int data = 0;
> - struct ctl_table table = {
> - .procname = "foo",
> - .data = &data,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - .extra1 = SYSCTL_ZERO,
> - .extra2 = SYSCTL_ONE_HUNDRED,
> - };
> + struct ctl_table table = SYSCTL_RANGE_ENTRY("foo", &data, int, 0644,\
> + SYSCTL_ZERO, SYSCTL_ONE_HUNDRED);
> size_t max_len = 32, len = max_len;
> loff_t pos = 0;
> char *buffer = kunit_kzalloc(test, max_len, GFP_USER);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 9d3a666ffde1..2e769550b268 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1373,47 +1373,16 @@ int proc_do_static_key(const struct ctl_table *table, int dir,
>
> static const struct ctl_table sysctl_subsys_table[] = {
> #ifdef CONFIG_PROC_SYSCTL
> - {
> - .procname = "sysctl_writes_strict",
> - .data = &sysctl_writes_strict,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec_minmax,
> - .extra1 = SYSCTL_NEG_ONE,
> - .extra2 = SYSCTL_ONE,
> - },
> + SYSCTL_RANGE_ENTRY("sysctl_writes_strict", &sysctl_writes_strict, int, \
> + 0644, SYSCTL_NEG_ONE, SYSCTL_ONE),
> #endif
> - {
> - .procname = "ngroups_max",
> - .data = (void *)&ngroups_max,
> - .maxlen = sizeof (int),
> - .mode = 0444,
> - .proc_handler = proc_dointvec,
> - },
> - {
> - .procname = "cap_last_cap",
> - .data = (void *)&cap_last_cap,
> - .maxlen = sizeof(int),
> - .mode = 0444,
> - .proc_handler = proc_dointvec,
> - },
> + SYSCTL_ENTRY("ngroups_max", (void *)&ngroups_max, int, 0444),
> + SYSCTL_ENTRY("cap_last_cap", (void *)&cap_last_cap, int, 0444),
> #ifdef CONFIG_SYSCTL_ARCH_UNALIGN_ALLOW
> - {
> - .procname = "unaligned-trap",
> - .data = &unaligned_enabled,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - },
> + SYSCTL_ENTRY("unaligned-trap", &unaligned_enabled, int, 0644),
> #endif
> #ifdef CONFIG_SYSCTL_ARCH_UNALIGN_NO_WARN
> - {
> - .procname = "ignore-unaligned-usertrap",
> - .data = &no_unaligned_warning,
> - .maxlen = sizeof (int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - },
> + SYSCTL_ENTRY("ignore-unaligned-usertrap", &no_unaligned_warning, int, 0644),
> #endif
Again, very nice that you included the testing. Does it cover all the
ways of using the API? What I mean is, do we test for 1. When the data
is NULL ptr, 2. for when there is only one (instead of both) "extra" pointer
thx
Best
--
Joel Granados
Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)
Powered by blists - more mailing lists