sysctl: add proper unsigned int support

Commit e7d316a02f6838 ("sysctl: handle error writing UINT_MAX to u32 fields") added proc_douintvec() to start help adding support for unsigned int, this however was only half the work needed, all these issues are present with the current implementation:

o Printing the values shows a negative value, this happens since do_proc_dointvec() and this uses proc_put_long()
o We can easily wrap around the int values: UINT_MAX is

    4294967295, if we echo in 4294967295 + 1 we end up with 0,
    using 4294967295 + 2 we end up with 1.

o We echo negative values in and they are accepted

Fix all these issues by adding our own do_proc_douintvec().

Cc Subash Abhinov Kasiviswanathan <[email protected]>

Cc Heinrich Schuchardt <[email protected]>

Cc Kees Cook <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Fixes e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")

Signed-off-by Luis R. Rodriguez <[email protected]>

---

I split this off as its own atomic fix from a larger RFC series [0]. I've only provided the fix here, and split off further functionality into a separate patch for the future. Although this is a fix I don't think its super critical, and specially due to its size do not think it can be stable material.

I do have proc_douintvec_minmax() but since we have no users for it it can wait until I add something that makes use of it. If someone needs it now though please let me know.

Likewise adding proc_douintvec_minmax_sysadmin() is very trivial but I have no immediate users for it so it can wait even longer.

[0] https://lkml.kernel.org/r/[email protected]

 kernel/sysctl.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 115 insertions(+), 6 deletions(-)

kernel/sysctl.c +0 -0

index 8dbaec0e4f7f..118341d3a139 100644
@@ -2125,12 +2125,12 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
        return 0;
 }
 

Quoted 52 lines · Luis R. Rodriguez

Commit e7d316a02f6838 ("sysctl: handle error writing UINT_MAX to u32 fields") added proc_douintvec() to start help adding support for unsigned int, this however was only half the work needed, all these issues are present with the current implementation:

o Printing the values shows a negative value, this happens since do_proc_dointvec() and this uses proc_put_long()
o We can easily wrap around the int values: UINT_MAX is

    4294967295, if we echo in 4294967295 + 1 we end up with 0,
    using 4294967295 + 2 we end up with 1.

o We echo negative values in and they are accepted

Fix all these issues by adding our own do_proc_douintvec().

Cc Subash Abhinov Kasiviswanathan <[email protected]>

Cc Heinrich Schuchardt <[email protected]>

Cc Kees Cook <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Fixes e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")

Signed-off-by Luis R. Rodriguez <[email protected]>

---

I split this off as its own atomic fix from a larger RFC series [0]. I've only provided the fix here, and split off further functionality into a separate patch for the future. Although this is a fix I don't think its super critical, and specially due to its size do not think it can be stable material.

I do have proc_douintvec_minmax() but since we have no users for it it can wait until I add something that makes use of it. If someone needs it now though please let me know.

Likewise adding proc_douintvec_minmax_sysadmin() is very trivial but I have no immediate users for it so it can wait even longer.

[0] https://lkml.kernel.org/r/[email protected]

 kernel/sysctl.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 115 insertions(+), 6 deletions(-)

kernel/sysctl.c +0 -0

index 8dbaec0e4f7f..118341d3a139 100644
@@ -2125,12 +2125,12 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
        return 0;
 }
 

kernel/sysctl.c static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, · line 2133

Quoted 10 lines

kernel/sysctl.c +4 -4

-static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp,
-                                int *valp,
-                                int write, void *data)
+static int do_proc_douintvec_conv(unsigned long *lvalp,
+                                 unsigned int *valp,
+                                 int write, void *data)
 {
        if (write) {
-               if (*negp)
+               if (*lvalp > (unsigned long) UINT_MAX)

Cast is unnecessary here.

kernel/sysctl.c static int do_proc_dointvec(struct ctl_table *table, int write, · line 2292

Quoted 3 lines

kernel/sysctl.c +2 -0

+static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
 
+       for (; left && vleft--; i++, first=false) {

I'd suggest to not implement "array of unsigned int" unless such sysctl already exists. Much of the complexity arises from this case.

Quoted 53 lines · Alexey Dobriyan

Quoted 52 lines · Luis R. Rodriguez

Commit e7d316a02f6838 ("sysctl: handle error writing UINT_MAX to u32 fields") added proc_douintvec() to start help adding support for unsigned int, this however was only half the work needed, all these issues are present with the current implementation:

o Printing the values shows a negative value, this happens since do_proc_dointvec() and this uses proc_put_long()
o We can easily wrap around the int values: UINT_MAX is

    4294967295, if we echo in 4294967295 + 1 we end up with 0,
    using 4294967295 + 2 we end up with 1.

o We echo negative values in and they are accepted

Fix all these issues by adding our own do_proc_douintvec().

Cc Subash Abhinov Kasiviswanathan <[email protected]>

Cc Heinrich Schuchardt <[email protected]>

Cc Kees Cook <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Fixes e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")

Signed-off-by Luis R. Rodriguez <[email protected]>

---

I split this off as its own atomic fix from a larger RFC series [0]. I've only provided the fix here, and split off further functionality into a separate patch for the future. Although this is a fix I don't think its super critical, and specially due to its size do not think it can be stable material.

I do have proc_douintvec_minmax() but since we have no users for it it can wait until I add something that makes use of it. If someone needs it now though please let me know.

Likewise adding proc_douintvec_minmax_sysadmin() is very trivial but I have no immediate users for it so it can wait even longer.

[0] https://lkml.kernel.org/r/[email protected]

 kernel/sysctl.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 115 insertions(+), 6 deletions(-)

kernel/sysctl.c +0 -0

index 8dbaec0e4f7f..118341d3a139 100644
@@ -2125,12 +2125,12 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
        return 0;
 }
 

Quoted 12 lines

kernel/sysctl.c static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, · line 2133

Quoted 10 lines

kernel/sysctl.c +4 -4

-static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp,
-                                int *valp,
-                                int write, void *data)
+static int do_proc_douintvec_conv(unsigned long *lvalp,
+                                 unsigned int *valp,
+                                 int write, void *data)
 {
        if (write) {
-               if (*negp)
+               if (*lvalp > (unsigned long) UINT_MAX)

Cast is unnecessary here.

Fixed, thanks!

Quoted 6 lines

kernel/sysctl.c static int do_proc_dointvec(struct ctl_table *table, int write, · line 2292

Quoted 3 lines

kernel/sysctl.c +2 -0

+static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
 
+       for (; left && vleft--; i++, first=false) {

I'd suggest to not implement "array of unsigned int" unless such sysctl already exists. Much of the complexity arises from this case.

Uh, yeah that is such crap.

As much as I'd like to I'm afraid the problem with this is there may be array int setups which should / could be ported to unsigned int. I tried to do a grammatical search with Coccinelle but ran into issues, I'll follow up with Julia about that.

So unless anyone finds evidence to the contrary I will be ripping out array support from unsigned int

Commit 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks") improved sanity checks considerbly, however the enhancements on sysctl_check_table() meant adding a functional change so that only the last table entry's sanity error is propagated. It also changed the way errors were propagated so that each new check reset the err value, this means only last sanity check computed is used for an error. This has been in the kernel since v3.4 days.

Fix this by carrying on errors from previous checks and iterations as we traverse the table and ensuring we keep any error from previous checks. We keep iterating on the table even if an error is found so we can complain for all errors found in one shot. This works as -EINVAL is always returned on error anyway, and the check for error is any non-zero value.

Fixes 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks")

Signed-off-by Luis R. Rodriguez <[email protected]>

Commit e7d316a02f6838 ("sysctl: handle error writing UINT_MAX to u32 fields") added proc_douintvec() to start help adding support for unsigned int, this however was only half the work needed, all these issues are present with the current implementation:

o Printing the values shows a negative value, this happens since do_proc_dointvec() and this uses proc_put_long()
o We can easily wrap around the int values: UINT_MAX is 4294967295, if we echo in 4294967295 + 1 we end up with 0, using 4294967295 + 2 we end up with 1.
o We echo negative values in and they are accepted
o sysctl_check_table() was never extended for proc_douintvec()

Fix all these issues by adding our own do_proc_douintvec() and adding proc_douintvec() to sysctl_check_table().

Historically sysctl proc helpers have supported arrays, due to the complexity this adds though we've taken a step back to evaluate array users to determine if its worth upkeeping for unsigned int. An evaluation using Coccinelle has been done to perform a grammatical search to ask ourselves:

o How many sysctl proc_dointvec() (int) users exist which likely should be moved over to proc_douintvec() (unsigned int) ?
Answer: about 8
- Of these how many are array users ?
Answer: Probably only 1
o How many sysctl array users exist ?
Answer: about 12

This last question gives us an idea just how popular arrays: they are not. Array support should probably just be kept for strings.

The identified uint ports are:

drivers/infiniband/core/ucma.c - max_backlog drivers/infiniband/core/iwcm.c - default_backlog net/core/sysctl_net_core.c - rps_sock_flow_sysctl() net/netfilter/nf_conntrack_timestamp.c - nf_conntrack_timestamp -- bool net/netfilter/nf_conntrack_acct.c nf_conntrack_acct -- bool net/netfilter/nf_conntrack_ecache.c - nf_conntrack_events -- bool net/netfilter/nf_conntrack_helper.c - nf_conntrack_helper -- bool net/phonet/sysctl.c proc_local_port_range()

The only possible array users is proc_local_port_range() but it does not seem worth it to add array support just for this given the range support works just as well. Unsigned int support should be desirable more for when you *need* more than INT_MAX or using int min/max support then does not suffice for your ranges.

If you forget and by mistake happen to register an unsigned int proc entry with an array, the driver will fail and you will get something as follows:

sysctl table check failed: debug/test_sysctl//uint_0002 array now allowed

CPU: 2 PID: 1342 Comm: modprobe Tainted: G        W   E <etc>
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS <etc>
Call Trace:
 dump_stack+0x63/0x81
 __register_sysctl_table+0x350/0x650
 ? kmem_cache_alloc_trace+0x107/0x240
 __register_sysctl_paths+0x1b3/0x1e0
 ? 0xffffffffc005f000
 register_sysctl_table+0x1f/0x30
 test_sysctl_init+0x10/0x1000 [test_sysctl]
 do_one_initcall+0x52/0x1a0
 ? kmem_cache_alloc_trace+0x107/0x240
 do_init_module+0x5f/0x200
 load_module+0x1867/0x1bd0
 ? __symbol_put+0x60/0x60
 SYSC_finit_module+0xdf/0x110
 SyS_finit_module+0xe/0x10
 entry_SYSCALL_64_fastpath+0x1e/0xad
RIP: 0033:0x7f042b22d119

<etc>

Cc Subash Abhinov Kasiviswanathan <[email protected]>

Cc Heinrich Schuchardt <[email protected]>

Cc Kees Cook <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

Fixes e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 fs/proc/proc_sysctl.c |  15 +++++
 kernel/sysctl.c       | 161 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 170 insertions(+), 6 deletions(-)

fs/proc/proc_sysctl.c +0 -0

index d22ee738d2eb..73696a73a1ec 100644

fs/proc/proc_sysctl.c +10 -0

+               if (table->maxlen != sizeof(unsigned int))
+                       err |= sysctl_err(path, table, "array now allowed");
+       }
+
+       return err;
+}
+
 static int sysctl_check_table(const char *path, struct ctl_table *table)
 {
        int err = 0;

@@ -1040,6 +1052,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
 
                if ((table->proc_handler == proc_dostring) ||
                    (table->proc_handler == proc_dointvec) ||
+                   (table->proc_handler == proc_douintvec) ||
                    (table->proc_handler == proc_dointvec_minmax) ||
                    (table->proc_handler == proc_dointvec_jiffies) ||
                    (table->proc_handler == proc_dointvec_userhz_jiffies) ||

@@ -1050,6 +1063,8 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
                                err |= sysctl_err(path, table, "No data");
                        if (!table->maxlen)
                                err |= sysctl_err(path, table, "No maxlen");
+                       else
+                               err |= sysctl_check_table_array(path, table);
                }
                if (!table->proc_handler)
                        err |= sysctl_err(path, table, "No proc_handler");

kernel/sysctl.c +22 -4

index 1aea594a54db..493bc05e546a 100644
@@ -2125,12 +2125,12 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
        return 0;
 }
 
-static int do_proc_douintvec_conv(bool *negp, unsigned long *lvalp,
-                                int *valp,
-                                int write, void *data)
+static int do_proc_douintvec_conv(unsigned long *lvalp,
+                                 unsigned int *valp,
+                                 int write, void *data)
 {
        if (write) {
-               if (*negp)
+               if (*lvalp > UINT_MAX)
                        return -EINVAL;
                *valp = *lvalp;
        } else {

@@ -2243,6 +2243,155 @@ static int do_proc_dointvec(struct ctl_table *table, int write,
                        buffer, lenp, ppos, conv, data);
 }
 
+static int do_proc_douintvec_w(unsigned int *tbl_data,
+                              struct ctl_table *table,
+                              void __user *buffer,
+                              size_t *lenp, loff_t *ppos,
+                              int (*conv)(unsigned long *lvalp,
+                                          unsigned int *valp,
+                                          int write, void *data),
+                              void *data)
+{
+       unsigned long lval;
+       int err = 0;
+       size_t left;
+       bool neg;
+       char *kbuf = NULL, *p;
+
+       left = *lenp;
+
+       if (*ppos) {

kernel/sysctl.c +99 -0

+
+       if (left > PAGE_SIZE - 1)
+               left = PAGE_SIZE - 1;
+
+       p = kbuf = memdup_user_nul(buffer, left);
+       if (IS_ERR(kbuf))
+               return -EINVAL;
+
+       left -= proc_skip_spaces(&p);
+       if (!left) {
+               err = -EINVAL;
+               goto out_free;
+       }
+
+       err = proc_get_long(&p, &left, &lval, &neg,
+                            proc_wspace_sep,
+                            sizeof(proc_wspace_sep), NULL);
+       if (err || neg) {
+               err = -EINVAL;
+               goto out_free;
+       }
+
+       if (conv(&lval, tbl_data, 1, data)) {
+               err = -EINVAL;
+               goto out_free;
+       }
+
+       if (!err && left)
+               left -= proc_skip_spaces(&p);
+
+out_free:
+       kfree(kbuf);
+       if (err)
+               return -EINVAL;
+
+       return 0;
+
+       /* This is in keeping with old __do_proc_dointvec() */
+bail_early:
+       *ppos += *lenp;
+       return err;
+}
+
+static int do_proc_douintvec_r(unsigned int *tbl_data, void __user *buffer,
+                              size_t *lenp, loff_t *ppos,
+                              int (*conv)(unsigned long *lvalp,
+                                          unsigned int *valp,
+                                          int write, void *data),
+                              void *data)
+{
+       unsigned long lval;
+       int err = 0;
+       size_t left;
+
+       left = *lenp;
+
+       if (conv(&lval, tbl_data, 0, data)) {
+               err = -EINVAL;
+               goto out;
+       }
+
+       err = proc_put_long(&buffer, &left, lval, false);
+       if (err || !left)
+               goto out;
+
+       err = proc_put_char(&buffer, &left, '\n');
+
+out:
+       *lenp -= left;
+       *ppos += *lenp;
+
+       return err;
+}
+
+static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
+                              int write, void __user *buffer,
+                              size_t *lenp, loff_t *ppos,
+                              int (*conv)(unsigned long *lvalp,
+                                          unsigned int *valp,
+                                          int write, void *data),
+                              void *data)
+{
+       unsigned int *i, vleft;
+
+       if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
+               *lenp = 0;
+               return 0;
+       }
+
+       i = (unsigned int *) tbl_data;
+       vleft = table->maxlen / sizeof(*i);
+
+       /*
+        * Arrays are not supported, keep this simple. *Do not* add
+        * support for them.
+        */
+       if (vleft != 1) {
+               *lenp = 0;
+               return -EINVAL;

kernel/sysctl.c +14 -3

+
+static int do_proc_douintvec(struct ctl_table *table, int write,
+                            void __user *buffer, size_t *lenp, loff_t *ppos,
+                            int (*conv)(unsigned long *lvalp,
+                                        unsigned int *valp,
+                                        int write, void *data),
+                            void *data)
+{
+       return __do_proc_douintvec(table->data, table, write,
+                                  buffer, lenp, ppos, conv, data);
+}
+
 /**
  * proc_dointvec - read a vector of integers
  * @table: the sysctl table

@@ -2278,8 +2427,8 @@ int proc_dointvec(struct ctl_table *table, int write,
 int proc_douintvec(struct ctl_table *table, int write,
                     void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-       return do_proc_dointvec(table, write, buffer, lenp, ppos,
-                               do_proc_douintvec_conv, NULL);
+       return do_proc_douintvec(table, write, buffer, lenp, ppos,
+                                do_proc_douintvec_conv, NULL);
 }
 
 /*
--

2.11.0

Quoted 13 lines · Luis R. Rodriguez

---
 fs/proc/proc_sysctl.c |  15 +++++
 kernel/sysctl.c       | 161 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 170 insertions(+), 6 deletions(-)

fs/proc/proc_sysctl.c +0 -0

index d22ee738d2eb..73696a73a1ec 100644
@@ -1031,6 +1031,18 @@ static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...)
        return -EINVAL;
 }
 

fs/proc/proc_sysctl.c static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...) · line 1038

Quoted 5 lines

fs/proc/proc_sysctl.c +5 -0

+static int sysctl_check_table_array(const char *path, struct ctl_table *table)
+{
+       int err = 0;
+
+       if (table->proc_handler == proc_douintvec) {

Should this be inverted? i.e. explicitly allow proc_handlers instead of only rejecting douintvec?

fs/proc/proc_sysctl.c static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...) · line 1041

Quoted 3 lines

fs/proc/proc_sysctl.c +3 -0

+               if (table->maxlen != sizeof(unsigned int))
+                       err |= sysctl_err(path, table, "array now allowed");
+       }

Quoted 30 lines

kernel/sysctl.c +22 -0

index 1aea594a54db..493bc05e546a 100644
@@ -2243,6 +2243,155 @@ static int do_proc_dointvec(struct ctl_table *table, int write,
                        buffer, lenp, ppos, conv, data);
 }
 
+static int do_proc_douintvec_w(unsigned int *tbl_data,
+                              struct ctl_table *table,
+                              void __user *buffer,
+                              size_t *lenp, loff_t *ppos,
+                              int (*conv)(unsigned long *lvalp,
+                                          unsigned int *valp,
+                                          int write, void *data),
+                              void *data)
+{
+       unsigned long lval;
+       int err = 0;
+       size_t left;
+       bool neg;
+       char *kbuf = NULL, *p;
+
+       left = *lenp;
+
+       if (*ppos) {
+               switch (sysctl_writes_strict) {
+               case SYSCTL_WRITES_STRICT:
+                       goto bail_early;
+               case SYSCTL_WRITES_WARN:

kernel/sysctl.c static int do_proc_dointvec(struct ctl_table *table, int write, · line 2273

Quoted 6 lines

kernel/sysctl.c +6 -0

+                       warn_sysctl_write(table);
+                       break;
+               default:
+                       break;
+               }
+       }

I wonder if this SYSCTL_WRITES_* test copy/pasting needs to be a function?

Quoted 29 lines

kernel/sysctl.c +29 -0

+static int __do_proc_douintvec(void *tbl_data, struct ctl_table *table,
+                              int write, void __user *buffer,
+                              size_t *lenp, loff_t *ppos,
+                              int (*conv)(unsigned long *lvalp,
+                                          unsigned int *valp,
+                                          int write, void *data),
+                              void *data)
+{
+       unsigned int *i, vleft;
+
+       if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
+               *lenp = 0;
+               return 0;
+       }
+
+       i = (unsigned int *) tbl_data;
+       vleft = table->maxlen / sizeof(*i);
+
+       /*
+        * Arrays are not supported, keep this simple. *Do not* add
+        * support for them.
+        */
+       if (vleft != 1) {
+               *lenp = 0;
+               return -EINVAL;
+       }
+
+       if (!conv)
+               conv = do_proc_douintvec_conv;

kernel/sysctl.c static int do_proc_dointvec(struct ctl_table *table, int write, · line 2382

Quoted 6 lines

kernel/sysctl.c +6 -0

+
+       if (write)
+               return do_proc_douintvec_w(i, table, buffer, lenp, ppos,
+                                          conv, data);
+       return do_proc_douintvec_r(i, buffer, lenp, ppos, conv, data);
+}

I like the split of read/write. That makes things easier to review.

To keep parity with regular int interfaces provide the an unsigned int proc_douintvec_minmax() which allows you to specify a range of allowed valid numbers.

Adding proc_douintvec_minmax_sysadmin() is easy but we can wait for an actual user for that.

Cc Subash Abhinov Kasiviswanathan <[email protected]>

Cc Heinrich Schuchardt <[email protected]>

Cc Kees Cook <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

Although we have had tools/testing/selftests/sysctl/ with two test cases these use existing kernel sysctl interfaces. We want to expand test coverage, we can't just be looking for random safe production values to poke at, instead just dedicate a test driver for debugging purposes and port the existing scripts to use it. This will make it easier for further tests to be added.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 lib/Kconfig.debug                               |  11 +++
 lib/Makefile                                    |   1 +
 lib/test_sysctl.c                               | 106 ++++++++++++++++++++++++
 tools/testing/selftests/sysctl/config           |   1 +
 tools/testing/selftests/sysctl/run_numerictests |   4 +-
 tools/testing/selftests/sysctl/run_stringtests  |   4 +-
 6 files changed, 123 insertions(+), 4 deletions(-)
 create mode 100644 lib/test_sysctl.c
 create mode 100644 tools/testing/selftests/sysctl/config

lib/Kconfig.debug +11 -0

index 64c03b07ad2f..d753fac41f78 100644
@@ -1975,6 +1975,17 @@ config TEST_FIRMWARE
 
          If unsure, say N.
 
+config TEST_SYSCTL
+       tristate "sysctl test driver"
+       default n
+       depends on PROC_SYSCTL
+       help
+         This builds the "test_sysctl" module. This driver enables to test the
+         proc sysctl interfaces available to drivers safely without affecting
+         production knobs which might alter system functionality.
+
+         If unsure, say N.
+
 config TEST_UDELAY
        tristate "udelay test driver"
        default n

lib/Makefile +1 -0

index 445a39c21f46..ac832c440d41 100644
@@ -45,6 +45,7 @@ obj-$(CONFIG_TEST_HEXDUMP) += test_hexdump.o
 obj-y += kstrtox.o
 obj-$(CONFIG_TEST_BPF) += test_bpf.o
 obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
+obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o
 obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o
 obj-$(CONFIG_TEST_KASAN) += test_kasan.o
 obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o

lib/test_sysctl.c new file +27 -0

new file mode 100644
index 000000000000..9b9ae1a95ab3
@@ -0,0 +1,106 @@
+/*
+ * proc sysctl test driver
+ *
+ * Copyright (C) 2017 Luis R. Rodriguez <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of copyleft-next (version 0.3.1 or later) as published
+ * at http://copyleft-next.org/.
+ */
+
+/*
+ * This module provides an interface to the the proc sysctl interfaces.  This
+ * driver requires CONFIG_PROC_SYSCTL. It will not normally be loaded by the
+ * system unless explicitly requested by name. You can also build this driver
+ * into your kernel.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>

lib/test_sysctl.c new file +69 -0

+};
+
+static struct test_sysctl_data test_data = {
+       .int_0001 = 60,
+       .string_0001 = "(none)",
+};
+
+/* These are all under /proc/sys/debug/test_sysctl/ */
+static struct ctl_table test_table[] = {
+       {
+               .procname       = "int_0001",
+               .data           = &test_data.int_0001,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = proc_dointvec_minmax,
+               .extra1         = &i_zero,
+               .extra2         = &i_one_hundred,
+       },
+       {
+               .procname       = "string_0001",
+               .data           = &test_data.string_0001,
+               .maxlen         = sizeof(test_data.string_0001),
+               .mode           = 0644,
+               .proc_handler   = proc_dostring,
+       },
+       { }
+};
+
+static struct ctl_table test_sysctl_table[] = {
+       {
+               .procname       = "test_sysctl",
+               .maxlen         = 0,
+               .mode           = 0555,
+               .child          = test_table,
+       },
+       { }
+};
+
+static struct ctl_table test_sysctl_root_table[] = {
+       {
+               .procname       = "debug",
+               .maxlen         = 0,
+               .mode           = 0555,
+               .child          = test_sysctl_table,
+       },
+       { }
+};
+
+static struct ctl_table_header *test_sysctl_header;
+
+static int __init test_sysctl_init(void)
+{
+       test_sysctl_header = register_sysctl_table(test_sysctl_root_table);
+       if (!test_sysctl_header)
+               return -ENOMEM;
+       return 0;
+}
+late_initcall(test_sysctl_init);
+
+static void __exit test_sysctl_exit(void)
+{
+       if (test_sysctl_header)
+               unregister_sysctl_table(test_sysctl_header);
+}
+
+module_exit(test_sysctl_exit);
+
+MODULE_AUTHOR("Luis R. Rodriguez <[email protected]>");
+MODULE_LICENSE("GPL");

tools/testing/selftests/sysctl/config new file +0 -0

new file mode 100644
index 000000000000..6ca14800d755

tools/testing/selftests/sysctl/run_numerictests +2 -2

index 8510f93f2d14..cdfeef96568c 100755
@@ -1,7 +1,7 @@
 #!/bin/sh
 
-SYSCTL="/proc/sys"
-TARGET="${SYSCTL}/vm/swappiness"
+SYSCTL="/proc/sys/debug/test_sysctl/"
+TARGET="${SYSCTL}/int_0001"
 ORIG=$(cat "${TARGET}")
 TEST_STR=$(( $ORIG + 1 ))
 

tools/testing/selftests/sysctl/run_stringtests +2 -3

index 90a9293d520c..15a646b2c527 100755
@@ -1,7 +1,7 @@
 #!/bin/sh
 
-SYSCTL="/proc/sys"
-TARGET="${SYSCTL}/kernel/domainname"
+SYSCTL="/proc/sys/debug/test_sysctl/"
+TARGET="${SYSCTL}/string_0001"
 ORIG=$(cat "${TARGET}")
 TEST_STR="Testing sysctl"
 
--

2.11.0

This adds a generic script to let us more easily add more tests cases. Since we really have only two types of tests cases just fold them into the one file. Each test unit is now identified into its separate function:

  # ./sysctl.sh -l
Test ID list:

TEST_ID x NUM_TEST

TEST_ID:   Test ID
NUM_TESTS: Number of recommended times to run the test

0001 x 1 - tests proc_dointvec_minmax()
0002 x 1 - tests proc_dostring()

For now we start off with what we had before, and run only each test once. We can now watch a test case until it fails:

./sysctl.sh -w 0002

We can also run a test case x number of times, say we want to run a test case 100 times:

./sysctl.sh -c 0001 100

To run a test case only once, for example:

./sysctl.sh -s 0002

The default settings are specified at the top of sysctl.sh.

Signed-off-by Luis R. Rodriguez <[email protected]>

Quoted 27 lines · Luis R. Rodriguez

This adds a generic script to let us more easily add more tests cases. Since we really have only two types of tests cases just fold them into the one file. Each test unit is now identified into its separate function:

  # ./sysctl.sh -l
Test ID list:

TEST_ID x NUM_TEST

TEST_ID:   Test ID
NUM_TESTS: Number of recommended times to run the test

0001 x 1 - tests proc_dointvec_minmax()
0002 x 1 - tests proc_dostring()

For now we start off with what we had before, and run only each test once. We can now watch a test case until it fails:

./sysctl.sh -w 0002

We can also run a test case x number of times, say we want to run a test case 100 times:

./sysctl.sh -c 0001 100

To run a test case only once, for example:

Quoted 5 lines

./sysctl.sh -s 0002

The default settings are specified at the top of sysctl.sh.

Signed-off-by Luis R. Rodriguez <[email protected]>

I'm not a fan of this: it consolidates tests when it's not needed

and creates a test running infrastructure at the wrong level of abstraction.

I'd like to see individual tests that are one-off runnable.

Whatever consumes the tools/testing/selftests/ tree is what should be doing the -w, -c, etc style options.

Test against a simple proc_dointvec() case. While at it, add a test against INT_MAX. Make sure INT_MAX works, and INT_MAX+1 will fail. Also test negative values work.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 lib/test_sysctl.c                        | 11 ++++++
 tools/testing/selftests/sysctl/sysctl.sh | 62 ++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

lib/test_sysctl.c +11 -0

index 9b9ae1a95ab3..c36a024d7351 100644
@@ -34,11 +34,15 @@ static int i_one_hundred = 100;
 
 struct test_sysctl_data {
        int int_0001;
+       int int_0002;
+
        char string_0001[65];
 };
 
 static struct test_sysctl_data test_data = {
        .int_0001 = 60,
+       .int_0002 = 1,
+
        .string_0001 = "(none)",
 };
 

@@ -54,6 +58,13 @@ static struct ctl_table test_table[] = {
                .extra2         = &i_one_hundred,
        },
        {
+               .procname       = "int_0002",
+               .data           = &test_data.int_0002,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = proc_dointvec,
+       },
+       {
                .procname       = "string_0001",
                .data           = &test_data.string_0001,
                .maxlen         = sizeof(test_data.string_0001),

tools/testing/selftests/sysctl/sysctl.sh +16 -0

index 14b9d875db42..45fd2ee5739c 100755
@@ -24,6 +24,7 @@ TEST_FILE=$(mktemp)
 # we have tons of space.
 ALL_TESTS="0001:1:1"
 ALL_TESTS="$ALL_TESTS 0002:1:1"
+ALL_TESTS="$ALL_TESTS 0003:1:1"
 
 test_modprobe()
 {

@@ -52,6 +53,9 @@ function allow_user_defaults()
        if [ -z $MAX_DIGITS ]; then
                MAX_DIGITS=$(($PAGE_SIZE/8))
        fi
+       if [ -z $INT_MAX ]; then
+               INT_MAX=$(getconf INT_MAX)
+       fi
 }
 
 test_reqs()

@@ -92,6 +96,9 @@ reset_vals()
                int_0001)
                        VAL="60"
                        ;;
+               int_0002)
+                       VAL="1"
+                       ;;
                string_0001)
                        VAL="(none)"
                        ;;

@@ -261,6 +268,48 @@ run_limit_digit()
        test_rc
 }
 
+# You are using an int
+run_limit_digit_int()
+{
+       echo -n "Testing INT_MAX works ..."
+       reset_vals
+       TEST_STR="$INT_MAX"
+       echo -n $TEST_STR > $TARGET
+
+       if ! verify "${TARGET}"; then

Quoted 23 lines · Luis R. Rodriguez

tools/testing/selftests/sysctl/sysctl.sh +14 -0

index 14b9d875db42..45fd2ee5739c 100755
@@ -24,6 +24,7 @@ TEST_FILE=$(mktemp)

@@ -261,6 +268,48 @@ run_limit_digit()
        test_rc
 }
 
+# You are using an int
+run_limit_digit_int()
+{
+       echo -n "Testing INT_MAX works ..."
+       reset_vals
+       TEST_STR="$INT_MAX"
+       echo -n $TEST_STR > $TARGET
+
+       if ! verify "${TARGET}"; then
+               echo "FAIL" >&2
+               rc=1
+       else
+               echo "ok"
+       fi

tools/testing/selftests/sysctl/sysctl.sh run_limit_digit() · line 289

Quoted 5 lines

tools/testing/selftests/sysctl/sysctl.sh +5 -0

+       test_rc
+
+       echo -n "Testing INT_MAX + 1 will fail as expected..."
+       reset_vals
+       TEST_STR=$(($INT_MAX+1))

Is the shell always going to do the right thing here? Maybe these test values should be explicitly hard-coded? I'm on the fence...

Add a few initial respective tests for an array:

o Echoing values separated by spaces works
o Echoing only first elements will set first elements
o Confirm PAGE_SIZE limit still applies even if an array is used

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 lib/test_sysctl.c                        | 13 +++++
 tools/testing/selftests/sysctl/sysctl.sh | 89 ++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)

lib/test_sysctl.c +13 -0

index 1654f41961b7..603c24b2f6cb 100644
@@ -35,6 +35,7 @@ static int i_one_hundred = 100;
 struct test_sysctl_data {
        int int_0001;
        int int_0002;
+       int int_0003[4];
 
        unsigned int uint_0001;
 

@@ -45,6 +46,11 @@ static struct test_sysctl_data test_data = {
        .int_0001 = 60,
        .int_0002 = 1,
 
+       .int_0003[0] = 0,
+       .int_0003[1] = 1,
+       .int_0003[2] = 2,
+       .int_0003[3] = 3,
+
        .uint_0001 = 314,
 
        .string_0001 = "(none)",

@@ -69,6 +75,13 @@ static struct ctl_table test_table[] = {
                .proc_handler   = proc_dointvec,
        },
        {
+               .procname       = "int_0003",
+               .data           = &test_data.int_0003,
+               .maxlen         = sizeof(test_data.int_0003),
+               .mode           = 0644,
+               .proc_handler   = proc_dointvec,
+       },
+       {
                .procname       = "uint_0001",
                .data           = &test_data.uint_0001,
                .maxlen         = sizeof(unsigned int),

tools/testing/selftests/sysctl/sysctl.sh +5 -0

index eedfba6f0a57..963d572155b1 100755
@@ -26,6 +26,7 @@ ALL_TESTS="0001:1:1"
 ALL_TESTS="$ALL_TESTS 0002:1:1"
 ALL_TESTS="$ALL_TESTS 0003:1:1"
 ALL_TESTS="$ALL_TESTS 0004:1:1"
+ALL_TESTS="$ALL_TESTS 0005:3:1"
 
 test_modprobe()
 {

@@ -78,6 +79,10 @@ test_reqs()
                echo "$0: You need getconf installed"
                exit 1
        fi
+       if ! which diff 2> /dev/null > /dev/null; then
+               echo "$0: You need diff installed"
+               exit 1
+       fi
 }
 
 function load_req_mod()

tools/testing/selftests/sysctl/sysctl.sh +81 -1

+       return $?
+}
+
 test_rc()
 {
        if [[ $rc != 0 ]]; then

@@ -317,6 +328,74 @@ run_limit_digit_int()
        test_rc
 }
 
+# You used an int array
+run_limit_digit_int_array()
+{
+       echo -n "Testing array works as expected ... "
+       TEST_STR="4 3 2 1"
+       echo -n $TEST_STR > $TARGET
+
+       if ! verify_diff_w "${TARGET}"; then
+               echo "FAIL" >&2
+               rc=1
+       else
+               echo "ok"
+       fi
+       test_rc
+
+       echo -n "Testing skipping trailing array elements works ... "
+       # Do not reset_vals, carry on the values from the last test.
+       # If we only echo in two digits the last two are left intact
+       TEST_STR="100 101"
+       echo -n $TEST_STR > $TARGET
+       # After we echo in, to help diff we need to set on TEST_STR what
+       # we expect the result to be.
+       TEST_STR="100 101 2 1"
+
+       if ! verify_diff_w "${TARGET}"; then
+               echo "FAIL" >&2
+               rc=1
+       else
+               echo "ok"
+       fi
+       test_rc
+
+       echo -n "Testing PAGE_SIZE limit on array works ... "
+       # Do not reset_vals, carry on the values from the last test.
+       # Even if you use an int array, you are still restricted to
+       # MAX_DIGITS, this is a known limitation. Test limit works.
+       LIMIT=$((MAX_DIGITS -1))
+       TEST_STR="9"
+       (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
+               dd of="${TARGET}" 2>/dev/null
+
+       TEST_STR="9 101 2 1"
+       if ! verify_diff_w "${TARGET}"; then
+               echo "FAIL" >&2
+               rc=1
+       else
+               echo "ok"
+       fi
+       test_rc
+
+       echo -n "Testing exceeding PAGE_SIZE limit fails as expected ... "
+       # Do not reset_vals, carry on the values from the last test.
+       # Now go over limit.
+       LIMIT=$((MAX_DIGITS))
+       TEST_STR="7"
+       (perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
+               dd of="${TARGET}" 2>/dev/null
+
+       TEST_STR="7 101 2 1"
+       if verify_diff_w "${TARGET}"; then
+               echo "FAIL" >&2
+               rc=1
+       else
+               echo "ok"
+       fi
+       test_rc
+}
+
 # You are using an unsigned int
 run_limit_digit_uint()
 {

@@ -477,6 +556,15 @@ sysctl_test_0004()
        run_limit_digit_uint
 }
 
+sysctl_test_0005()
+{
+       TARGET="${SYSCTL}/int_0003"
+       reset_vals
+       ORIG=$(cat "${TARGET}")
+
+       run_limit_digit_int_array
+}
+
 list_tests()
 {
        echo "Test ID list:"

@@ -489,6 +577,7 @@ list_tests()
        echo "0002 x $(get_test_count 0002) - tests proc_dostring()"
        echo "0003 x $(get_test_count 0003) - tests proc_dointvec()"
        echo "0004 x $(get_test_count 0004) - tests proc_douintvec()"
+       echo "0005 x $(get_test_count 0005) - tests proc_douintvec() array"
 }
 
 test_reqs
--

2.11.0

Quoted 8 lines · Luis R. Rodriguez

tools/testing/selftests/sysctl/sysctl.sh +0 -0

index eedfba6f0a57..963d572155b1 100755
@@ -137,6 +142,12 @@ verify()
        return 0
 }
 

tools/testing/selftests/sysctl/sysctl.sh verify() · line 147

Quoted 3 lines

tools/testing/selftests/sysctl/sysctl.sh +3 -0

+verify_diff_w()
+{
+       echo "$TEST_STR" | diff -w -u - $1 2>&1 > /dev/null

Instead of shell redirection, just use -q ?

I love seeing these tests added. I have one other change I'd like to add to sysctl,

but I haven't had time to make sure it doesn't break stuff. I haven't been able to prove it to myself, but I think it's safe; I just need to update the tests to handle it:

http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/commit/?h=sysctl/writes_strict&id=b63a38ca45bd9fb61545ce6ce66093147eb96a7c

It'd need an update for the uint handler...

On this v2 I've taken Alexey's recommendation and looked at array users of the proc sysctl interface which complicate the interfece to see if we can instead just simplify the unsigned int implementation. I could not find any clear candidate. As such I've just ripped out array support.

Since some future unsigned int proc sysctl users might think there is array support I've taken measures to do sanity checks on initialization and warn the kernel if such users creep up. To validate this I ended up just writing a simple test driver, and extending our tests. In doing this I also found a really old issue with sysctl_check_table(), and yet another issue with the first incarnation of proc_douintvec().

I hammered on proc_douintvec() as much as I could, and extended tests for this to ensure we don't regress should some int users convert over.

I noticed one more issue but I did not fix as I figured it was worth
discussing: proc_doi*_minmax() handlers have historically allowed users to register even if their own data does not match the expressed min/max values. When this happens the value is exposed on /proc/sys but reading or writing does not work against it. I'm of the opinion that sysctl_check_table() should just validate this and bail preventing such entries from ever creeping up. The only reason I didn't do this is this *could* mean some tables don't get registered in some cases -- I haven't done the vetting. If we're fine with this I can add it later.

Luis R. Rodriguez (9):
sysctl: fix lax sysctl_check_table() sanity check
sysctl: add proper unsigned int support
sysctl: add unsigned int range support
test_sysctl: add dedicated proc sysctl test driver
test_sysctl: add generic script to expand on tests
test_sysctl: test against PAGE_SIZE for int
test_sysctl: add simple proc_dointvec() case
test_sysctl: add simple proc_douintvec() case
test_sysctl: test against int proc_dointvec() array support

Commit 7c60c48f58a7 ("sysctl: Improve the sysctl sanity checks") improved sanity checks considerbly, however the enhancements on sysctl_check_table() meant adding a functional change so that only the last table entry's sanity error is propagated. It also changed the way errors were propagated so that each new check reset the err value, this means only last sanity check computed is used for an error. This has been in the kernel since v3.4 days.

Fix this by carrying on errors from previous checks and iterations as we traverse the table and ensuring we keep any error from previous checks. We keep iterating on the table even if an error is found so we can complain for all errors found in one shot. This works as -EINVAL is always returned on error anyway, and the check for error is any non-zero value.

...

fs/proc/proc_sysctl.c +3 -3

@@ -1066,7 +1066,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
 	int err = 0;
 	for (; table->procname; table++) {
 		if (table->child)
-			err = sysctl_err(path, table, "Not a file");
+			err |= sysctl_err(path, table, "Not a file");
 
 		if ((table->proc_handler == proc_dostring) ||
 		    (table->proc_handler == proc_dointvec) ||

@@ -1078,15 +1078,15 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
 		    (table->proc_handler == proc_doulongvec_minmax) ||
 		    (table->proc_handler == proc_doulongvec_ms_jiffies_minmax)) {
 			if (!table->data)
-				err = sysctl_err(path, table, "No data");
+				err |= sysctl_err(path, table, "No data");
 			if (!table->maxlen)
-				err = sysctl_err(path, table, "No maxlen");
+				err |= sysctl_err(path, table, "No maxlen");
 		}