Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1571942 > unrolled thread

Re: [PATCH] sysctl: add proper unsigned int support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] sysctl: add proper unsigned int support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-01 21:00 +0100
    Re: [PATCH] sysctl: add proper unsigned int support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-09 02:40 +0100
    Re: [PATCH] sysctl: add proper unsigned int support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-09 02:50 +0100
      [PATCH v2 5/9] test_sysctl: add generic script to expand on tests "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100
      [PATCH v2 9/9] test_sysctl: test against int proc_dointvec() array support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100
      [PATCH v2 4/9] test_sysctl: add dedicated proc sysctl test driver "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100
      [PATCH v2 6/9] test_sysctl: test against PAGE_SIZE for int "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100
      [PATCH v2 1/9] sysctl: fix lax sysctl_check_table() sanity check "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100
      [PATCH v2 2/9] sysctl: add proper unsigned int support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100
      [PATCH v2 3/9] sysctl: add unsigned int range support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100
      [PATCH v2 7/9] test_sysctl: add simple proc_dointvec() case "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100
      [PATCH v2 0/9] sysctl: add and fix proper unsigned int support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100
        [PATCH v2 8/9] test_sysctl: add simple proc_douintvec() case "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-02-11 01:40 +0100

#1571942 — Re: [PATCH] sysctl: add proper unsigned int support

On Mon, Jan 30, 2017 at 03:56:20PM +0300, Alexey Dobriyan wrote:
> On Sun, Jan 29, 2017 at 10:29 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > 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 <subashab@codeaurora.org>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Fixes: e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >
> > 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/20161208184801.1689-1-mcgrof@kernel.org
> >
> >  kernel/sysctl.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 115 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 8dbaec0e4f7f..118341d3a139 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -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 > (unsigned long) UINT_MAX)
> 
> Cast is unnecessary here.

Fixed, thanks!

> > +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. A manual search however
reveals one so far:

ipv4_local_port_range(). There may be others. So, do we deal with the
old cases by leaving them as-is or provide a new separate interface?
I take it the array stuff is long tested, so don't the harm in providing
similar functionality for unsigned int. I'm perfectly happy in just
ripping out unsigned int array support though. Who makes this call ?

  Luis

[toc] | [next] | [standalone]

#1577243

On Wed, Feb 8, 2017 at 5:28 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> So unless anyone finds evidence to the contrary I will be ripping
> out array support from unsigned int

Oh so I should probably explain: the reason for this is that even
though I found one possible user for array for unsigned int, we
support ranges with int, and that should suffice for that driver.

 Luis

[toc] | [prev] | [next] | [standalone]

#1577250

On Wed, Feb 01, 2017 at 08:56:46PM +0100, Luis R. Rodriguez wrote:
> On Mon, Jan 30, 2017 at 03:56:20PM +0300, Alexey Dobriyan wrote:
> > On Sun, Jan 29, 2017 at 10:29 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > > 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 <subashab@codeaurora.org>
> > > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: "David S. Miller" <davem@davemloft.net>
> > > Cc: Ingo Molnar <mingo@redhat.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Fixes: e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > > ---
> > >
> > > 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/20161208184801.1689-1-mcgrof@kernel.org
> > >
> > >  kernel/sysctl.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 115 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > > index 8dbaec0e4f7f..118341d3a139 100644
> > > --- a/kernel/sysctl.c
> > > +++ b/kernel/sysctl.c
> > > @@ -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 > (unsigned long) UINT_MAX)
> > 
> > Cast is unnecessary here.
> 
> Fixed, thanks!
> 
> > > +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.

The more I studied this the more I supported the idea of ripping
the array crap out from unsigned int support.

> 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. 

I've now completed a preliminary evaluation using Coccinelle 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.

So unless anyone finds evidence to the contrary I will be ripping
out array support from unsigned int. I'll also go ahead and extend
the test cases and add a test_sysctl driver for selftests.

  Luis

[toc] | [prev] | [next] | [standalone]

#1578887 — [PATCH v2 5/9] test_sysctl: add generic script to expand on tests

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 <mcgrof@kernel.org>
---
 tools/testing/selftests/sysctl/Makefile         |   3 +-
 tools/testing/selftests/sysctl/common_tests     | 109 ------
 tools/testing/selftests/sysctl/run_numerictests |  10 -
 tools/testing/selftests/sysctl/run_stringtests  |  77 ----
 tools/testing/selftests/sysctl/sysctl.sh        | 459 ++++++++++++++++++++++++
 5 files changed, 460 insertions(+), 198 deletions(-)
 delete mode 100644 tools/testing/selftests/sysctl/common_tests
 delete mode 100755 tools/testing/selftests/sysctl/run_numerictests
 delete mode 100755 tools/testing/selftests/sysctl/run_stringtests
 create mode 100755 tools/testing/selftests/sysctl/sysctl.sh

diff --git a/tools/testing/selftests/sysctl/Makefile b/tools/testing/selftests/sysctl/Makefile
index b3c33e071f10..95c320b354e8 100644
--- a/tools/testing/selftests/sysctl/Makefile
+++ b/tools/testing/selftests/sysctl/Makefile
@@ -4,8 +4,7 @@
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests".
 all:
 
-TEST_PROGS := run_numerictests run_stringtests
-TEST_FILES := common_tests
+TEST_PROGS := sysctl.sh
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/sysctl/common_tests b/tools/testing/selftests/sysctl/common_tests
deleted file mode 100644
index 17d534b1b7b4..000000000000
--- a/tools/testing/selftests/sysctl/common_tests
+++ /dev/null
@@ -1,109 +0,0 @@
-#!/bin/sh
-
-TEST_FILE=$(mktemp)
-
-echo "== Testing sysctl behavior against ${TARGET} =="
-
-set_orig()
-{
-	echo "${ORIG}" > "${TARGET}"
-}
-
-set_test()
-{
-	echo "${TEST_STR}" > "${TARGET}"
-}
-
-verify()
-{
-	local seen
-	seen=$(cat "$1")
-	if [ "${seen}" != "${TEST_STR}" ]; then
-		return 1
-	fi
-	return 0
-}
-
-trap 'set_orig; rm -f "${TEST_FILE}"' EXIT
-
-rc=0
-
-echo -n "Writing test file ... "
-echo "${TEST_STR}" > "${TEST_FILE}"
-if ! verify "${TEST_FILE}"; then
-	echo "FAIL" >&2
-	exit 1
-else
-	echo "ok"
-fi
-
-echo -n "Checking sysctl is not set to test value ... "
-if verify "${TARGET}"; then
-	echo "FAIL" >&2
-	exit 1
-else
-	echo "ok"
-fi
-
-echo -n "Writing sysctl from shell ... "
-set_test
-if ! verify "${TARGET}"; then
-	echo "FAIL" >&2
-	exit 1
-else
-	echo "ok"
-fi
-
-echo -n "Resetting sysctl to original value ... "
-set_orig
-if verify "${TARGET}"; then
-	echo "FAIL" >&2
-	exit 1
-else
-	echo "ok"
-fi
-
-# Now that we've validated the sanity of "set_test" and "set_orig",
-# we can use those functions to set starting states before running
-# specific behavioral tests.
-
-echo -n "Writing entire sysctl in single write ... "
-set_orig
-dd if="${TEST_FILE}" of="${TARGET}" bs=4096 2>/dev/null
-if ! verify "${TARGET}"; then
-	echo "FAIL" >&2
-	rc=1
-else
-	echo "ok"
-fi
-
-echo -n "Writing middle of sysctl after synchronized seek ... "
-set_test
-dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 skip=1 2>/dev/null
-if ! verify "${TARGET}"; then
-	echo "FAIL" >&2
-	rc=1
-else
-	echo "ok"
-fi
-
-echo -n "Writing beyond end of sysctl ... "
-set_orig
-dd if="${TEST_FILE}" of="${TARGET}" bs=20 seek=2 2>/dev/null
-if verify "${TARGET}"; then
-        echo "FAIL" >&2
-        rc=1
-else
-        echo "ok"
-fi
-
-echo -n "Writing sysctl with multiple long writes ... "
-set_orig
-(perl -e 'print "A" x 50;'; echo "${TEST_STR}") | \
-	dd of="${TARGET}" bs=50 2>/dev/null
-if verify "${TARGET}"; then
-	echo "FAIL" >&2
-	rc=1
-else
-	echo "ok"
-fi
diff --git a/tools/testing/selftests/sysctl/run_numerictests b/tools/testing/selftests/sysctl/run_numerictests
deleted file mode 100755
index cdfeef96568c..000000000000
--- a/tools/testing/selftests/sysctl/run_numerictests
+++ /dev/null
@@ -1,10 +0,0 @@
-#!/bin/sh
-
-SYSCTL="/proc/sys/debug/test_sysctl/"
-TARGET="${SYSCTL}/int_0001"
-ORIG=$(cat "${TARGET}")
-TEST_STR=$(( $ORIG + 1 ))
-
-. ./common_tests
-
-exit $rc
diff --git a/tools/testing/selftests/sysctl/run_stringtests b/tools/testing/selftests/sysctl/run_stringtests
deleted file mode 100755
index 15a646b2c527..000000000000
--- a/tools/testing/selftests/sysctl/run_stringtests
+++ /dev/null
@@ -1,77 +0,0 @@
-#!/bin/sh
-
-SYSCTL="/proc/sys/debug/test_sysctl/"
-TARGET="${SYSCTL}/string_0001"
-ORIG=$(cat "${TARGET}")
-TEST_STR="Testing sysctl"
-
-. ./common_tests
-
-# Only string sysctls support seeking/appending.
-MAXLEN=65
-
-echo -n "Writing entire sysctl in short writes ... "
-set_orig
-dd if="${TEST_FILE}" of="${TARGET}" bs=1 2>/dev/null
-if ! verify "${TARGET}"; then
-	echo "FAIL" >&2
-	rc=1
-else
-	echo "ok"
-fi
-
-echo -n "Writing middle of sysctl after unsynchronized seek ... "
-set_test
-dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 2>/dev/null
-if verify "${TARGET}"; then
-	echo "FAIL" >&2
-	rc=1
-else
-	echo "ok"
-fi
-
-echo -n "Checking sysctl maxlen is at least $MAXLEN ... "
-set_orig
-perl -e 'print "A" x ('"${MAXLEN}"'-2), "B";' | \
-	dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
-if ! grep -q B "${TARGET}"; then
-	echo "FAIL" >&2
-	rc=1
-else
-	echo "ok"
-fi
-
-echo -n "Checking sysctl keeps original string on overflow append ... "
-set_orig
-perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
-	dd of="${TARGET}" bs=$(( MAXLEN - 1 )) 2>/dev/null
-if grep -q B "${TARGET}"; then
-	echo "FAIL" >&2
-	rc=1
-else
-	echo "ok"
-fi
-
-echo -n "Checking sysctl stays NULL terminated on write ... "
-set_orig
-perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
-	dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
-if grep -q B "${TARGET}"; then
-	echo "FAIL" >&2
-	rc=1
-else
-	echo "ok"
-fi
-
-echo -n "Checking sysctl stays NULL terminated on overwrite ... "
-set_orig
-perl -e 'print "A" x ('"${MAXLEN}"'-1), "BB";' | \
-	dd of="${TARGET}" bs=$(( $MAXLEN + 1 )) 2>/dev/null
-if grep -q B "${TARGET}"; then
-	echo "FAIL" >&2
-	rc=1
-else
-	echo "ok"
-fi
-
-exit $rc
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
new file mode 100755
index 000000000000..f8f29092063d
--- /dev/null
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -0,0 +1,459 @@
+#!/bin/bash
+# Copyright (C) 2016 Luis R. Rodriguez <mcgrof@kernel.org>
+#
+# 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 performs a series tests against the proc sysctl interface.
+
+TEST_NAME="sysctl"
+TEST_DRIVER="test_${TEST_NAME}"
+TEST_DIR=$(dirname $0)
+TEST_FILE=$(mktemp)
+
+# This represents
+#
+# TEST_ID:TEST_COUNT:ENABLED
+#
+# TEST_ID: is the test id number
+# TEST_COUNT: number of times we should run the test
+# ENABLED: 1 if enabled, 0 otherwise
+#
+# Once these are enabled please leave them as-is. Write your own test,
+# we have tons of space.
+ALL_TESTS="0001:1:1"
+ALL_TESTS="$ALL_TESTS 0002:1:1"
+
+test_modprobe()
+{
+       if [ ! -d $DIR ]; then
+               echo "$0: $DIR not present" >&2
+               echo "You must have the following enabled in your kernel:" >&2
+               cat $TEST_DIR/config >&2
+               exit 1
+       fi
+}
+
+function allow_user_defaults()
+{
+	if [ -z $DIR ]; then
+		DIR="/sys/module/test_sysctl/"
+	fi
+	if [ -z $DEFAULT_NUM_TESTS ]; then
+		DEFAULT_NUM_TESTS=50
+	fi
+	if [ -z $SYSCTL ]; then
+		SYSCTL="/proc/sys/debug/test_sysctl"
+	fi
+}
+
+test_reqs()
+{
+	uid=$(id -u)
+	if [ $uid -ne 0 ]; then
+		echo $msg must be run as root >&2
+		exit 0
+	fi
+
+	if ! which perl 2> /dev/null > /dev/null; then
+		echo "$0: You need perl installed"
+		exit 1
+	fi
+}
+
+function load_req_mod()
+{
+	trap "test_modprobe" EXIT
+
+	if [ ! -d $DIR ]; then
+		modprobe $TEST_DRIVER
+		if [ $? -ne 0 ]; then
+			exit
+		fi
+	fi
+}
+
+set_orig()
+{
+	if [ ! -z $TARGET ]; then
+		echo "${ORIG}" > "${TARGET}"
+	fi
+}
+
+set_test()
+{
+	echo "${TEST_STR}" > "${TARGET}"
+}
+
+verify()
+{
+	local seen
+	seen=$(cat "$1")
+	if [ "${seen}" != "${TEST_STR}" ]; then
+		return 1
+	fi
+	return 0
+}
+
+test_rc()
+{
+	if [[ $rc != 0 ]]; then
+		echo "Failed test, return value: $rc" >&2
+		exit $rc
+	fi
+}
+
+test_finish()
+{
+	set_orig
+	rm -f "${TEST_FILE}"
+}
+
+run_numerictests()
+{
+	echo "== Testing sysctl behavior against ${TARGET} =="
+
+	rc=0
+
+	echo -n "Writing test file ... "
+	echo "${TEST_STR}" > "${TEST_FILE}"
+	if ! verify "${TEST_FILE}"; then
+		echo "FAIL" >&2
+		exit 1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Checking sysctl is not set to test value ... "
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		exit 1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Writing sysctl from shell ... "
+	set_test
+	if ! verify "${TARGET}"; then
+		echo "FAIL" >&2
+		exit 1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Resetting sysctl to original value ... "
+	set_orig
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		exit 1
+	else
+		echo "ok"
+	fi
+
+	# Now that we've validated the sanity of "set_test" and "set_orig",
+	# we can use those functions to set starting states before running
+	# specific behavioral tests.
+
+	echo -n "Writing entire sysctl in single write ... "
+	set_orig
+	dd if="${TEST_FILE}" of="${TARGET}" bs=4096 2>/dev/null
+	if ! verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Writing middle of sysctl after synchronized seek ... "
+	set_test
+	dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 skip=1 2>/dev/null
+	if ! verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Writing beyond end of sysctl ... "
+	set_orig
+	dd if="${TEST_FILE}" of="${TARGET}" bs=20 seek=2 2>/dev/null
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Writing sysctl with multiple long writes ... "
+	set_orig
+	(perl -e 'print "A" x 50;'; echo "${TEST_STR}") | \
+		dd of="${TARGET}" bs=50 2>/dev/null
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	test_rc
+}
+
+run_stringtests()
+{
+	echo -n "Writing entire sysctl in short writes ... "
+	set_orig
+	dd if="${TEST_FILE}" of="${TARGET}" bs=1 2>/dev/null
+	if ! verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Writing middle of sysctl after unsynchronized seek ... "
+	set_test
+	dd if="${TEST_FILE}" of="${TARGET}" bs=1 seek=1 2>/dev/null
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Checking sysctl maxlen is at least $MAXLEN ... "
+	set_orig
+	perl -e 'print "A" x ('"${MAXLEN}"'-2), "B";' | \
+		dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
+	if ! grep -q B "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Checking sysctl keeps original string on overflow append ... "
+	set_orig
+	perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
+		dd of="${TARGET}" bs=$(( MAXLEN - 1 )) 2>/dev/null
+	if grep -q B "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Checking sysctl stays NULL terminated on write ... "
+	set_orig
+	perl -e 'print "A" x ('"${MAXLEN}"'-1), "B";' | \
+		dd of="${TARGET}" bs="${MAXLEN}" 2>/dev/null
+	if grep -q B "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	echo -n "Checking sysctl stays NULL terminated on overwrite ... "
+	set_orig
+	perl -e 'print "A" x ('"${MAXLEN}"'-1), "BB";' | \
+		dd of="${TARGET}" bs=$(( $MAXLEN + 1 )) 2>/dev/null
+	if grep -q B "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	test_rc
+}
+
+sysctl_test_0001()
+{
+	TARGET="${SYSCTL}/int_0001"
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+
+	run_numerictests
+}
+
+sysctl_test_0002()
+{
+	TARGET="${SYSCTL}/string_0001"
+	ORIG=$(cat "${TARGET}")
+	TEST_STR="Testing sysctl"
+	# Only string sysctls support seeking/appending.
+	MAXLEN=65
+
+	run_numerictests
+	run_stringtests
+}
+
+list_tests()
+{
+	echo "Test ID list:"
+	echo
+	echo "TEST_ID x NUM_TEST"
+	echo "TEST_ID:   Test ID"
+	echo "NUM_TESTS: Number of recommended times to run the test"
+	echo
+	echo "0001 x $(get_test_count 0001) - tests proc_dointvec_minmax()"
+	echo "0002 x $(get_test_count 0002) - tests proc_dostring()"
+}
+
+test_reqs
+
+usage()
+{
+	NUM_TESTS=$(grep -o ' ' <<<"$ALL_TESTS" | grep -c .)
+	let NUM_TESTS=$NUM_TESTS+1
+	MAX_TEST=$(printf "%04d\n" $NUM_TESTS)
+	echo "Usage: $0 [ -t <4-number-digit> ] | [ -w <4-number-digit> ] |"
+	echo "		 [ -s <4-number-digit> ] | [ -c <4-number-digit> <test- count>"
+	echo "           [ all ] [ -h | --help ] [ -l ]"
+	echo ""
+	echo "Valid tests: 0001-$MAX_TEST"
+	echo ""
+	echo "    all     Runs all tests (default)"
+	echo "    -t      Run test ID the number amount of times is recommended"
+	echo "    -w      Watch test ID run until it runs into an error"
+	echo "    -c      Run test ID once"
+	echo "    -s      Run test ID x test-count number of times"
+	echo "    -l      List all test ID list"
+	echo " -h|--help  Help"
+	echo
+	echo "If an error every occurs execution will immediately terminate."
+	echo "If you are adding a new test try using -w <test-ID> first to"
+	echo "make sure the test passes a series of tests."
+	echo
+	echo Example uses:
+	echo
+	echo "$TEST_NAME.sh            -- executes all tests"
+	echo "$TEST_NAME.sh -t 0002    -- Executes test ID 0002 number of times is recomended"
+	echo "$TEST_NAME.sh -w 0002    -- Watch test ID 0002 run until an error occurs"
+	echo "$TEST_NAME.sh -s 0002    -- Run test ID 0002 once"
+	echo "$TEST_NAME.sh -c 0002 3  -- Run test ID 0002 three times"
+	echo
+	list_tests
+	exit 1
+}
+
+function test_num()
+{
+	re='^[0-9]+$'
+	if ! [[ $1 =~ $re ]]; then
+		usage
+	fi
+}
+
+function get_test_count()
+{
+	test_num $1
+	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$1'}')
+	LAST_TWO=${TEST_DATA#*:*}
+	echo ${LAST_TWO%:*}
+}
+
+function get_test_enabled()
+{
+	test_num $1
+	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$1'}')
+	echo ${TEST_DATA#*:*:}
+}
+
+function run_all_tests()
+{
+	for i in $ALL_TESTS ; do
+		TEST_ID=${i%:*:*}
+		ENABLED=$(get_test_enabled $TEST_ID)
+		TEST_COUNT=$(get_test_count $TEST_ID)
+		if [[ $ENABLED -eq "1" ]]; then
+			test_case $TEST_ID $TEST_COUNT
+		fi
+	done
+}
+
+function watch_log()
+{
+	if [ $# -ne 3 ]; then
+		clear
+	fi
+	date
+	echo "Running test: $2 - run #$1"
+}
+
+function watch_case()
+{
+	i=0
+	while [ 1 ]; do
+
+		if [ $# -eq 1 ]; then
+			test_num $1
+			watch_log $i ${TEST_NAME}_test_$1
+			${TEST_NAME}_test_$1
+		else
+			watch_log $i all
+			run_all_tests
+		fi
+		let i=$i+1
+	done
+}
+
+function test_case()
+{
+	NUM_TESTS=$DEFAULT_NUM_TESTS
+	if [ $# -eq 2 ]; then
+		NUM_TESTS=$2
+	fi
+
+	i=0
+	while [ $i -lt $NUM_TESTS ]; do
+		test_num $1
+		watch_log $i ${TEST_NAME}_test_$1 noclear
+		RUN_TEST=${TEST_NAME}_test_$1
+		$RUN_TEST
+		let i=$i+1
+	done
+}
+
+function parse_args()
+{
+	if [ $# -eq 0 ]; then
+		run_all_tests
+	else
+		if [[ "$1" = "all" ]]; then
+			run_all_tests
+		elif [[ "$1" = "-w" ]]; then
+			shift
+			watch_case $@
+		elif [[ "$1" = "-t" ]]; then
+			shift
+			test_num $1
+			test_case $1 $(get_test_count $1)
+		elif [[ "$1" = "-c" ]]; then
+			shift
+			test_num $1
+			test_num $2
+			test_case $1 $2
+		elif [[ "$1" = "-s" ]]; then
+			shift
+			test_case $1 1
+		elif [[ "$1" = "-l" ]]; then
+			list_tests
+		elif [[ "$1" = "-h" || "$1" = "--help" ]]; then
+			usage
+		else
+			usage
+		fi
+	fi
+}
+
+test_reqs
+allow_user_defaults
+load_req_mod
+
+trap "test_finish" EXIT
+
+parse_args $@
+
+exit 0
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1578888 — [PATCH v2 9/9] test_sysctl: test against int proc_dointvec() array support

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 <mcgrof@kernel.org>
---
 lib/test_sysctl.c                        | 13 +++++
 tools/testing/selftests/sysctl/sysctl.sh | 89 ++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)

diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 1654f41961b7..603c24b2f6cb 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -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),
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index eedfba6f0a57..963d572155b1 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -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()
@@ -137,6 +142,12 @@ verify()
 	return 0
 }
 
+verify_diff_w()
+{
+	echo "$TEST_STR" | diff -w -u - $1 2>&1 > /dev/null
+	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

[toc] | [prev] | [next] | [standalone]

#1578890 — [PATCH v2 4/9] test_sysctl: add dedicated proc sysctl test driver

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 <mcgrof@kernel.org>
---
 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

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 64c03b07ad2f..d753fac41f78 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -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
diff --git a/lib/Makefile b/lib/Makefile
index 445a39c21f46..ac832c440d41 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -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
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
new file mode 100644
index 000000000000..9b9ae1a95ab3
--- /dev/null
+++ b/lib/test_sysctl.c
@@ -0,0 +1,106 @@
+/*
+ * proc sysctl test driver
+ *
+ * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@kernel.org>
+ *
+ * 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>
+#include <linux/async.h>
+#include <linux/delay.h>
+#include <linux/vmalloc.h>
+
+static int i_zero;
+static int i_one_hundred = 100;
+
+struct test_sysctl_data {
+	int int_0001;
+	char string_0001[65];
+};
+
+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 <mcgrof@kernel.org>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/sysctl/config b/tools/testing/selftests/sysctl/config
new file mode 100644
index 000000000000..6ca14800d755
--- /dev/null
+++ b/tools/testing/selftests/sysctl/config
@@ -0,0 +1 @@
+CONFIG_TEST_SYSCTL=y
diff --git a/tools/testing/selftests/sysctl/run_numerictests b/tools/testing/selftests/sysctl/run_numerictests
index 8510f93f2d14..cdfeef96568c 100755
--- a/tools/testing/selftests/sysctl/run_numerictests
+++ b/tools/testing/selftests/sysctl/run_numerictests
@@ -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 ))
 
diff --git a/tools/testing/selftests/sysctl/run_stringtests b/tools/testing/selftests/sysctl/run_stringtests
index 90a9293d520c..15a646b2c527 100755
--- a/tools/testing/selftests/sysctl/run_stringtests
+++ b/tools/testing/selftests/sysctl/run_stringtests
@@ -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

[toc] | [prev] | [next] | [standalone]

#1578891 — [PATCH v2 6/9] test_sysctl: test against PAGE_SIZE for int

Add the following tests to ensure we do not regress:

  o Test using a buffer full of space (PAGE_SIZE-1) followed by a
    single digit works

  o Test using a buffer full of spaces (PAGE_SIZE or over) will fail

As tests increase instead of unloading the module and reloading it
we can just do a shell reset_vals() with a reset to values we know
are set at init on the driver.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 tools/testing/selftests/sysctl/sysctl.sh | 65 ++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index f8f29092063d..14b9d875db42 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -46,6 +46,12 @@ function allow_user_defaults()
 	if [ -z $SYSCTL ]; then
 		SYSCTL="/proc/sys/debug/test_sysctl"
 	fi
+	if [ -z $PAGE_SIZE ]; then
+		PAGE_SIZE=$(getconf PAGESIZE)
+	fi
+	if [ -z $MAX_DIGITS ]; then
+		MAX_DIGITS=$(($PAGE_SIZE/8))
+	fi
 }
 
 test_reqs()
@@ -60,6 +66,10 @@ test_reqs()
 		echo "$0: You need perl installed"
 		exit 1
 	fi
+	if ! which getconf 2> /dev/null > /dev/null; then
+		echo "$0: You need getconf installed"
+		exit 1
+	fi
 }
 
 function load_req_mod()
@@ -74,6 +84,23 @@ function load_req_mod()
 	fi
 }
 
+reset_vals()
+{
+	VAL=""
+	TRIGGER=$(basename ${TARGET})
+	case "$TRIGGER" in
+		int_0001)
+			VAL="60"
+			;;
+		string_0001)
+			VAL="(none)"
+			;;
+		*)
+			;;
+	esac
+	echo -n $VAL > $TARGET
+}
+
 set_orig()
 {
 	if [ ! -z $TARGET ]; then
@@ -195,7 +222,42 @@ run_numerictests()
 	else
 		echo "ok"
 	fi
+	test_rc
+}
+
+# Your test must accept digits 3 and 4 to use this
+run_limit_digit()
+{
+	echo -n "Checking ignoring spaces up to PAGE_SIZE works on write ..."
+	reset_vals
+
+	LIMIT=$((MAX_DIGITS -1))
+	TEST_STR="3"
+	(perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
+		dd of="${TARGET}" 2>/dev/null
+
+	if ! verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+
+	echo -n "Checking passing PAGE_SIZE of spaces fails on write ..."
+	reset_vals
 
+	LIMIT=$((MAX_DIGITS))
+	TEST_STR="4"
+	(perl -e 'print " " x '$LIMIT';'; echo "${TEST_STR}") | \
+		dd of="${TARGET}" 2>/dev/null
+
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
 	test_rc
 }
 
@@ -271,15 +333,18 @@ run_stringtests()
 sysctl_test_0001()
 {
 	TARGET="${SYSCTL}/int_0001"
+	reset_vals
 	ORIG=$(cat "${TARGET}")
 	TEST_STR=$(( $ORIG + 1 ))
 
 	run_numerictests
+	run_limit_digit
 }
 
 sysctl_test_0002()
 {
 	TARGET="${SYSCTL}/string_0001"
+	reset_vals
 	ORIG=$(cat "${TARGET}")
 	TEST_STR="Testing sysctl"
 	# Only string sysctls support seeking/appending.
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1578892 — [PATCH v2 1/9] sysctl: fix lax sysctl_check_table() sanity check

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 <mcgrof@kernel.org>
---
 fs/proc/proc_sysctl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index d4e37acd4821..d22ee738d2eb 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1036,7 +1036,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) ||
@@ -1047,15 +1047,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");
 		}
 		if (!table->proc_handler)
-			err = sysctl_err(path, table, "No proc_handler");
+			err |= sysctl_err(path, table, "No proc_handler");
 
 		if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode)
-			err = sysctl_err(path, table, "bogus .mode 0%o",
+			err |= sysctl_err(path, table, "bogus .mode 0%o",
 				table->mode);
 	}
 	return err;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1578894 — [PATCH v2 2/9] 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
  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 <subashab@codeaurora.org>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: e7d316a02f68 ("sysctl: handle error writing UINT_MAX to u32 fields")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 fs/proc/proc_sysctl.c |  15 +++++
 kernel/sysctl.c       | 161 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 170 insertions(+), 6 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index d22ee738d2eb..73696a73a1ec 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1031,6 +1031,18 @@ static int sysctl_err(const char *path, struct ctl_table *table, char *fmt, ...)
 	return -EINVAL;
 }
 
+static int sysctl_check_table_array(const char *path, struct ctl_table *table)
+{
+	int err = 0;
+
+	if (table->proc_handler == proc_douintvec) {
+		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");
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 1aea594a54db..493bc05e546a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -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) {
+		switch (sysctl_writes_strict) {
+		case SYSCTL_WRITES_STRICT:
+			goto bail_early;
+		case SYSCTL_WRITES_WARN:
+			warn_sysctl_write(table);
+			break;
+		default:
+			break;
+		}
+	}
+
+	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;
+	}
+
+	if (!conv)
+		conv = do_proc_douintvec_conv;
+
+	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);
+}
+
+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

[toc] | [prev] | [next] | [standalone]

#1578895 — [PATCH v2 3/9] sysctl: add unsigned int range support

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 <subashab@codeaurora.org>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 fs/proc/proc_sysctl.c  |  4 ++-
 include/linux/sysctl.h |  3 +++
 kernel/sysctl.c        | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 73696a73a1ec..bc231740239d 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1035,7 +1035,8 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
 {
 	int err = 0;
 
-	if (table->proc_handler == proc_douintvec) {
+	if ((table->proc_handler == proc_douintvec) ||
+	    (table->proc_handler == proc_douintvec_minmax)) {
 		if (table->maxlen != sizeof(unsigned int))
 			err |= sysctl_err(path, table, "array now allowed");
 	}
@@ -1053,6 +1054,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_douintvec_minmax) ||
 		    (table->proc_handler == proc_dointvec_minmax) ||
 		    (table->proc_handler == proc_dointvec_jiffies) ||
 		    (table->proc_handler == proc_dointvec_userhz_jiffies) ||
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index adf4e51cf597..a35d40ecc211 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -47,6 +47,9 @@ extern int proc_douintvec(struct ctl_table *, int,
 			 void __user *, size_t *, loff_t *);
 extern int proc_dointvec_minmax(struct ctl_table *, int,
 				void __user *, size_t *, loff_t *);
+extern int proc_douintvec_minmax(struct ctl_table *table, int write,
+				 void __user *buffer, size_t *lenp,
+				 loff_t *ppos);
 extern int proc_dointvec_jiffies(struct ctl_table *, int,
 				 void __user *, size_t *, loff_t *);
 extern int proc_dointvec_userhz_jiffies(struct ctl_table *, int,
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 493bc05e546a..b286f57e9abe 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2533,6 +2533,65 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 				do_proc_dointvec_minmax_conv, &param);
 }
 
+struct do_proc_douintvec_minmax_conv_param {
+	unsigned int *min;
+	unsigned int *max;
+};
+
+static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
+					 unsigned int *valp,
+					 int write, void *data)
+{
+	struct do_proc_douintvec_minmax_conv_param *param = data;
+
+	if (write) {
+		unsigned int val = *lvalp;
+
+		if ((param->min && *param->min > val) ||
+		    (param->max && *param->max < val))
+			return -ERANGE;
+
+		if (*lvalp > UINT_MAX)
+			return -EINVAL;
+		*valp = val;
+	} else {
+		unsigned int val = *valp;
+		*lvalp = (unsigned long) val;
+	}
+
+	return 0;
+}
+
+/**
+ * proc_douintvec_minmax - read a vector of unsigned ints with min/max values
+ * @table: the sysctl table
+ * @write: %TRUE if this is a write to the sysctl file
+ * @buffer: the user buffer
+ * @lenp: the size of the user buffer
+ * @ppos: file position
+ *
+ * Reads/writes up to table->maxlen/sizeof(unsigned int) unsigned integer
+ * values from/to the user buffer, treated as an ASCII string. Negative
+ * strings are not allowed.
+ *
+ * This routine will ensure the values are within the range specified by
+ * table->extra1 (min) and table->extra2 (max). There is a final sanity
+ * check for UINT_MAX to avoid having to support wrap around uses from
+ * userspace.
+ *
+ * Returns 0 on success.
+ */
+int proc_douintvec_minmax(struct ctl_table *table, int write,
+			  void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct do_proc_douintvec_minmax_conv_param param = {
+		.min = (unsigned int *) table->extra1,
+		.max = (unsigned int *) table->extra2,
+	};
+	return do_proc_douintvec(table, write, buffer, lenp, ppos,
+				 do_proc_douintvec_minmax_conv, &param);
+}
+
 static void validate_coredump_safety(void)
 {
 #ifdef CONFIG_COREDUMP
@@ -3041,6 +3100,12 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 	return -ENOSYS;
 }
 
+int proc_douintvec_minmax(struct ctl_table *table, int write,
+			  void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	return -ENOSYS;
+}
+
 int proc_dointvec_jiffies(struct ctl_table *table, int write,
 		    void __user *buffer, size_t *lenp, loff_t *ppos)
 {
@@ -3083,6 +3148,7 @@ EXPORT_SYMBOL(proc_dointvec);
 EXPORT_SYMBOL(proc_douintvec);
 EXPORT_SYMBOL(proc_dointvec_jiffies);
 EXPORT_SYMBOL(proc_dointvec_minmax);
+EXPORT_SYMBOL_GPL(proc_douintvec_minmax);
 EXPORT_SYMBOL(proc_dointvec_userhz_jiffies);
 EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
 EXPORT_SYMBOL(proc_dostring);
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1578896 — [PATCH v2 7/9] test_sysctl: add simple proc_dointvec() case

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 <mcgrof@kernel.org>
---
 lib/test_sysctl.c                        | 11 ++++++
 tools/testing/selftests/sysctl/sysctl.sh | 62 ++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 9b9ae1a95ab3..c36a024d7351 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -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),
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 14b9d875db42..45fd2ee5739c 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -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
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+
+	echo -n "Testing INT_MAX + 1 will fail as expected..."
+	reset_vals
+	TEST_STR=$(($INT_MAX+1))
+	echo -n $TEST_STR > $TARGET 2> /dev/null
+
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+
+	echo -n "Testing negative values will work as expected..."
+	reset_vals
+	TEST_STR="-3"
+	echo -n $TEST_STR > $TARGET 2> /dev/null
+	if ! verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+}
+
 run_stringtests()
 {
 	echo -n "Writing entire sysctl in short writes ... "
@@ -354,6 +403,18 @@ sysctl_test_0002()
 	run_stringtests
 }
 
+sysctl_test_0003()
+{
+	TARGET="${SYSCTL}/int_0002"
+	reset_vals
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+
+	run_numerictests
+	run_limit_digit
+	run_limit_digit_int
+}
+
 list_tests()
 {
 	echo "Test ID list:"
@@ -364,6 +425,7 @@ list_tests()
 	echo
 	echo "0001 x $(get_test_count 0001) - tests proc_dointvec_minmax()"
 	echo "0002 x $(get_test_count 0002) - tests proc_dostring()"
+	echo "0003 x $(get_test_count 0003) - tests proc_dointvec()"
 }
 
 test_reqs
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1578897 — [PATCH v2 0/9] sysctl: add and fix proper unsigned int support

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

 fs/proc/proc_sysctl.c                           |  27 +-
 include/linux/sysctl.h                          |   3 +
 kernel/sysctl.c                                 | 227 +++++++-
 lib/Kconfig.debug                               |  11 +
 lib/Makefile                                    |   1 +
 lib/test_sysctl.c                               | 141 +++++
 tools/testing/selftests/sysctl/Makefile         |   3 +-
 tools/testing/selftests/sysctl/common_tests     | 109 ----
 tools/testing/selftests/sysctl/config           |   1 +
 tools/testing/selftests/sysctl/run_numerictests |  10 -
 tools/testing/selftests/sysctl/run_stringtests  |  77 ---
 tools/testing/selftests/sysctl/sysctl.sh        | 738 ++++++++++++++++++++++++
 12 files changed, 1139 insertions(+), 209 deletions(-)
 create mode 100644 lib/test_sysctl.c
 delete mode 100644 tools/testing/selftests/sysctl/common_tests
 create mode 100644 tools/testing/selftests/sysctl/config
 delete mode 100755 tools/testing/selftests/sysctl/run_numerictests
 delete mode 100755 tools/testing/selftests/sysctl/run_stringtests
 create mode 100755 tools/testing/selftests/sysctl/sysctl.sh

-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1578898 — [PATCH v2 8/9] test_sysctl: add simple proc_douintvec() case

Test against a simple proc_douintvec() case. While at it, add
a test against UINT_MAX. Make sure UINT_MAX works, and UINT_MAX+1
will fail and that negative values are not accepted.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 lib/test_sysctl.c                        | 11 ++++++
 tools/testing/selftests/sysctl/sysctl.sh | 63 ++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index c36a024d7351..1654f41961b7 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -36,6 +36,8 @@ struct test_sysctl_data {
 	int int_0001;
 	int int_0002;
 
+	unsigned int uint_0001;
+
 	char string_0001[65];
 };
 
@@ -43,6 +45,8 @@ static struct test_sysctl_data test_data = {
 	.int_0001 = 60,
 	.int_0002 = 1,
 
+	.uint_0001 = 314,
+
 	.string_0001 = "(none)",
 };
 
@@ -65,6 +69,13 @@ static struct ctl_table test_table[] = {
 		.proc_handler	= proc_dointvec,
 	},
 	{
+		.procname	= "uint_0001",
+		.data		= &test_data.uint_0001,
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec,
+	},
+	{
 		.procname	= "string_0001",
 		.data		= &test_data.string_0001,
 		.maxlen		= sizeof(test_data.string_0001),
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 45fd2ee5739c..eedfba6f0a57 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -25,6 +25,7 @@ TEST_FILE=$(mktemp)
 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"
 
 test_modprobe()
 {
@@ -56,6 +57,9 @@ function allow_user_defaults()
 	if [ -z $INT_MAX ]; then
 		INT_MAX=$(getconf INT_MAX)
 	fi
+	if [ -z $UINT_MAX ]; then
+		UINT_MAX=$(getconf UINT_MAX)
+	fi
 }
 
 test_reqs()
@@ -99,6 +103,9 @@ reset_vals()
 		int_0002)
 			VAL="1"
 			;;
+		uint_0001)
+			VAL="314"
+			;;
 		string_0001)
 			VAL="(none)"
 			;;
@@ -310,6 +317,49 @@ run_limit_digit_int()
 	test_rc
 }
 
+# You are using an unsigned int
+run_limit_digit_uint()
+{
+	echo -n "Testing UINT_MAX works ..."
+	reset_vals
+	TEST_STR="$UINT_MAX"
+	echo -n $TEST_STR > $TARGET
+
+	if ! verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+
+	echo -n "Testing UINT_MAX + 1 will fail as expected..."
+	reset_vals
+	TEST_STR=$(($UINT_MAX+1))
+	echo -n $TEST_STR > $TARGET 2> /dev/null
+
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+
+	echo -n "Testing negative values will not work as expected ..."
+	reset_vals
+	TEST_STR="-3"
+	echo -n $TEST_STR > $TARGET 2> /dev/null
+
+	if verify "${TARGET}"; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+	test_rc
+}
+
 run_stringtests()
 {
 	echo -n "Writing entire sysctl in short writes ... "
@@ -415,6 +465,18 @@ sysctl_test_0003()
 	run_limit_digit_int
 }
 
+sysctl_test_0004()
+{
+	TARGET="${SYSCTL}/uint_0001"
+	reset_vals
+	ORIG=$(cat "${TARGET}")
+	TEST_STR=$(( $ORIG + 1 ))
+
+	run_numerictests
+	run_limit_digit
+	run_limit_digit_uint
+}
+
 list_tests()
 {
 	echo "Test ID list:"
@@ -426,6 +488,7 @@ list_tests()
 	echo "0001 x $(get_test_count 0001) - tests proc_dointvec_minmax()"
 	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()"
 }
 
 test_reqs
-- 
2.11.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web