linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Luis Chamberlain <mcgrof@kernel.org>
To: Iurii Zaikin <yzaikin@google.com>,
	linux-api@vger.kernel.org,
	"Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Brendan Higgins <brendanhiggins@google.com>,
	frowand.list@gmail.com, gregkh@linuxfoundation.org,
	jpoimboe@redhat.com, Kees Cook <keescook@google.com>,
	kieran.bingham@ideasonboard.com, peterz@infradead.org,
	robh@kernel.org, Stephen Boyd <sboyd@kernel.org>,
	shuah@kernel.org, tytso@mit.edu, yamada.masahiro@socionext.com,
	devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
	kunit-dev@googlegroups.com, linux-doc@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-kbuild@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	linux-nvdimm@lists.01.org, linux-um@lists.infradead.org,
	Alexander.Levin@microsoft.com, Tim.Bird@sony.com,
	amir73il@gmail.com, dan.carpenter@oracle.com,
	Daniel Vetter <daniel@ffwll.ch>,
	jdike@addtoit.com, joel@jms.id.au, julia.lawall@lip6.fr,
	khilman@baylibre.com, knut.omang@oracle.com, logang@deltatee.com,
	mpe@ellerman.id.au, pmladek@suse.com, rdunlap@infradead.org,
	richard@nod.at, David Rientjes <rientjes@google.com>,
	rostedt@goodmis.org, wfg@linux.intel.com
Subject: Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
Date: Thu, 27 Jun 2019 06:10:21 +0000	[thread overview]
Message-ID: <20190627061021.GE19023@42.do-not-panic.com> (raw)
In-Reply-To: <CAAXuY3p+kVhjQ4LYtzormqVcH2vKu1abc_K9Z0XY=JX=bp8NcQ@mail.gmail.com>

On Wed, Jun 26, 2019 at 09:07:43PM -0700, Iurii Zaikin wrote:
> On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > +static void sysctl_test_dointvec_table_maxlen_unset(struct kunit *test)
> > > +{
> > > +     struct ctl_table table = {
> > > +             .procname = "foo",
> > > +             .data           = &test_data.int_0001,
> > > +             .maxlen         = 0,
> > > +             .mode           = 0644,
> > > +             .proc_handler   = proc_dointvec,
> > > +             .extra1         = &i_zero,
> > > +             .extra2         = &i_one_hundred,
> > > +     };
> > > +     void  *buffer = kunit_kzalloc(test, sizeof(int), GFP_USER);
> > > +     size_t len;
> > > +     loff_t pos;
> > > +
> > > +     len = 1234;
> > > +     KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 0, buffer, &len, &pos));
> > > +     KUNIT_EXPECT_EQ(test, (size_t)0, len);
> > > +     len = 1234;
> > > +     KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, buffer, &len, &pos));
> > > +     KUNIT_EXPECT_EQ(test, (size_t)0, len);
> > > +}
> >
> > In a way this is also testing for general kernel API changes. This is and the
> > last one were good examples. So this is not just testing functionality
> > here. There is no wrong or write answer if 0 or -EINVAL was returned
> > other than the fact that we have been doing this for years.
> >
> > Its a perhaps small but important difference for some of these tests.  I
> > *do* think its worth clarifying through documentation which ones are
> > testing for API consistency Vs proper correctness.
>
> You make a good point that the test codifies the existing behavior of
> the function in lieu of formal documentation.  However, the test cases
> were derived from examining the source code of the function under test
> and attempting to cover all branches. The assertions were added only
> for the values that appeared to be set deliberately in the
> implementation. And it makes sense to me to test that the code does
> exactly what the implementation author intended.

I'm not arguing against adding them. I'm suggesting that it is different
to test for API than for correctness of intended functionality, and
it would be wise to make it clear which test cases are for API and which
for correctness.

This will come up later for other kunit tests and it would be great
to set precendent so that other kunit tests can follow similar
practices to ensure its clear what is API realted Vs correctness of
intended functionality.

In fact, I'm not yet sure if its possible to test public kernel API to
userspace with kunit, but if it is possible... well, that could make
linux-api folks happy as they could enable us to codify interpreation of
what is expected into kunit test cases, and we'd ensure that the
codified interpretation is not only documented in man pages but also
through formal kunit test cases.

A regression in linux-api then could be formalized through a proper
kunit tests case. And if an API evolves, it would force developers to
update the respective kunit which codifies that contract.

> > > +static void sysctl_test_dointvec_single_less_int_min(struct kunit *test)
> > > +{
> > > +     struct ctl_table table = {
> > > +             .procname = "foo",
> > > +             .data           = &test_data.int_0001,
> > > +             .maxlen         = sizeof(int),
> > > +             .mode           = 0644,
> > > +             .proc_handler   = proc_dointvec,
> > > +             .extra1         = &i_zero,
> > > +             .extra2         = &i_one_hundred,
> > > +     };
> > > +     char input[32];
> > > +     size_t len = sizeof(input) - 1;
> > > +     loff_t pos = 0;
> > > +     unsigned long abs_of_less_than_min = (unsigned long)INT_MAX
> > > +                                          - (INT_MAX + INT_MIN) + 1;
> > > +
> > > +     KUNIT_EXPECT_LT(test,
> > > +                     (size_t)snprintf(input, sizeof(input), "-%lu",
> > > +                                      abs_of_less_than_min),
> > > +                     sizeof(input));
> > > +
> > > +     table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> > > +     KUNIT_EXPECT_EQ(test, -EINVAL,
> > > +                     proc_dointvec(&table, 1, input, &len, &pos));
> > > +     KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> > > +     KUNIT_EXPECT_EQ(test, 0, ((int *)table.data)[0]);
> > > +}
> >
> > API test.
> >
> Not sure why.

Because you are codifying that we *definitely* return -EINVAL on
overlow. Some parts of the kernel return -ERANGE for overflows for
instance.

It would be a generic test for overflow if it would just test
for any error.

It is a fine and good test to keep. All these tests are good to keep.

> I believe there has been a real bug with int overflow in
> proc_dointvec.
> Covering it with test seems like a good idea.

Oh definitely.

> > > +static void sysctl_test_dointvec_single_greater_int_max(struct kunit *test)
> > > +{
> > > +     struct ctl_table table = {
> > > +             .procname = "foo",
> > > +             .data           = &test_data.int_0001,
> > > +             .maxlen         = sizeof(int),
> > > +             .mode           = 0644,
> > > +             .proc_handler   = proc_dointvec,
> > > +             .extra1         = &i_zero,
> > > +             .extra2         = &i_one_hundred,
> > > +     };
> > > +     char input[32];
> > > +     size_t len = sizeof(input) - 1;
> > > +     loff_t pos = 0;
> > > +     unsigned long greater_than_max = (unsigned long)INT_MAX + 1;
> > > +
> > > +     KUNIT_EXPECT_GT(test, greater_than_max, (unsigned long)INT_MAX);
> > > +     KUNIT_EXPECT_LT(test, (size_t)snprintf(input, sizeof(input), "%lu",
> > > +                                            greater_than_max),
> > > +                     sizeof(input));
> > > +     table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> > > +     KUNIT_EXPECT_EQ(test, -EINVAL,
> > > +                     proc_dointvec(&table, 1, input, &len, &pos));
> > > +     KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> > > +     KUNIT_EXPECT_EQ(test, 0, ((int *)table.data)[0]);
> > > +}
> > > +
> >
> > API test.
> >
> > > +static struct kunit_case sysctl_test_cases[] = {
> > > +     KUNIT_CASE(sysctl_test_dointvec_null_tbl_data),
> > > +     KUNIT_CASE(sysctl_test_dointvec_table_maxlen_unset),
> > > +     KUNIT_CASE(sysctl_test_dointvec_table_len_is_zero),
> > > +     KUNIT_CASE(sysctl_test_dointvec_table_read_but_position_set),
> > > +     KUNIT_CASE(sysctl_test_dointvec_happy_single_positive),
> > > +     KUNIT_CASE(sysctl_test_dointvec_happy_single_negative),
> > > +     KUNIT_CASE(sysctl_test_dointvec_single_less_int_min),
> > > +     KUNIT_CASE(sysctl_test_dointvec_single_greater_int_max),
> > > +     {}
> > > +};
> >
> > Oh all are API tests.. perhaps then just rename then
> > sysctl_test_cases to sysctl_api_test_cases.
> >
> > Would be good to add at least *two* other tests cases for this
> > example, one which does a valid read and one which does a valid write.
> Added valid reads. There already are 2 valid writes.

Thanks.

> > If that is done either we add another kunit test module for correctness
> > or just extend the above and use prefix / postfixes on the functions
> > to distinguish between API / correctness somehow.
> >
> > > +
> > > +static struct kunit_module sysctl_test_module = {
> > > +     .name = "sysctl_test",
> > > +     .test_cases = sysctl_test_cases,
> > > +};
> > > +
> > > +module_test(sysctl_test_module);
> > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > index cbdfae3798965..389b8986f5b77 100644
> > > --- a/lib/Kconfig.debug
> > > +++ b/lib/Kconfig.debug
> > > @@ -1939,6 +1939,16 @@ config TEST_SYSCTL
> > >
> > >         If unsure, say N.
> > >
> > > +config SYSCTL_KUNIT_TEST
> > > +     bool "KUnit test for sysctl"
> > > +     depends on KUNIT
> > > +     help
> > > +       This builds the proc sysctl unit test, which runs on boot. For more
> > > +       information on KUnit and unit tests in general please refer to the
> > > +       KUnit documentation in Documentation/dev-tools/kunit/.
> >
> > A little more description here would help. It is testing for API and
> > hopefully also correctness (if extended with those two examples I
> > mentioned).
> >
> Added "Tests the API contract and implementation correctness of sysctl."

Yes, much clearer, thanks!

  Luis


  reply	other threads:[~2019-06-27  6:10 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-17  8:25 [PATCH v5 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework Brendan Higgins
2019-06-17  8:25 ` [PATCH v5 01/18] kunit: test: add KUnit test runner core Brendan Higgins
2019-06-20  0:15   ` Stephen Boyd
2019-06-25 20:28     ` Brendan Higgins
2019-06-25 21:44       ` Luis Chamberlain
2019-06-25 22:14         ` Brendan Higgins
2019-06-25 23:02           ` Luis Chamberlain
2019-06-26  6:41             ` Brendan Higgins
2019-06-26 22:02               ` Luis Chamberlain
2019-06-27  0:05                 ` Brendan Higgins
2019-06-26  3:40       ` Stephen Boyd
2019-06-26 23:00         ` Brendan Higgins
2019-06-27 18:16           ` Stephen Boyd
2019-06-28  8:09             ` Brendan Higgins
2019-06-25 22:33   ` Luis Chamberlain
2019-06-26  0:07     ` Brendan Higgins
2019-06-26  3:36       ` Luis Chamberlain
2019-06-26 22:16         ` Brendan Higgins
2019-06-17  8:25 ` [PATCH v5 02/18] kunit: test: add test resource management API Brendan Higgins
2019-06-17  8:25 ` [PATCH v5 03/18] kunit: test: add string_stream a std::stream like string builder Brendan Higgins
2019-06-17  8:25 ` [PATCH v5 04/18] kunit: test: add kunit_stream a std::stream like logger Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 05/18] kunit: test: add the concept of expectations Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 06/18] kbuild: enable building KUnit Brendan Higgins
2019-06-25 22:13   ` Luis Chamberlain
2019-06-25 22:41     ` Brendan Higgins
2019-06-25 23:03       ` Luis Chamberlain
2019-06-17  8:26 ` [PATCH v5 07/18] kunit: test: add initial tests Brendan Higgins
2019-06-25 23:22   ` Luis Chamberlain
2019-06-26  7:53     ` Brendan Higgins
2019-07-02 17:52       ` Brendan Higgins
2019-07-02 20:57         ` Luis Chamberlain
2019-06-17  8:26 ` [PATCH v5 08/18] objtool: add kunit_try_catch_throw to the noreturn list Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 09/18] kunit: test: add support for test abort Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 10/18] kunit: test: add tests for kunit " Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 11/18] kunit: test: add the concept of assertions Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 12/18] kunit: test: add tests for KUnit managed resources Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 13/18] kunit: tool: add Python wrappers for running KUnit tests Brendan Higgins
2019-06-26  0:01   ` Luis Chamberlain
2019-06-26  8:02     ` Brendan Higgins
2019-06-26 22:03       ` Luis Chamberlain
2019-06-27  0:23         ` Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 14/18] kunit: defconfig: add defconfigs for building " Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 15/18] Documentation: kunit: add documentation for KUnit Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 16/18] MAINTAINERS: add entry for KUnit the unit testing framework Brendan Higgins
2019-06-17  8:26 ` [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec() Brendan Higgins
2019-06-26  2:17   ` Luis Chamberlain
2019-06-27  4:07     ` Iurii Zaikin
2019-06-27  6:10       ` Luis Chamberlain [this message]
2019-06-28  8:01         ` Brendan Higgins
2019-06-28 21:37           ` Luis Chamberlain
2019-06-17  8:26 ` [PATCH v5 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC SYSCTL section Brendan Higgins
2019-06-26  2:19   ` Luis Chamberlain
2019-06-20  1:17 ` [PATCH v5 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework Frank Rowand
2019-06-21 14:59   ` shuah
2019-06-21 18:13     ` Theodore Ts'o
2019-06-21 19:20       ` shuah
2019-06-22  0:54         ` Brendan Higgins
2019-07-03 23:40           ` Brendan Higgins
2019-06-21 23:35   ` Brendan Higgins
2019-06-26  2:38   ` Luis Chamberlain

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190627061021.GE19023@42.do-not-panic.com \
    --to=mcgrof@kernel.org \
    --cc=Alexander.Levin@microsoft.com \
    --cc=Tim.Bird@sony.com \
    --cc=amir73il@gmail.com \
    --cc=brendanhiggins@google.com \
    --cc=dan.carpenter@oracle.com \
    --cc=daniel@ffwll.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=frowand.list@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jdike@addtoit.com \
    --cc=joel@jms.id.au \
    --cc=jpoimboe@redhat.com \
    --cc=julia.lawall@lip6.fr \
    --cc=keescook@google.com \
    --cc=khilman@baylibre.com \
    --cc=kieran.bingham@ideasonboard.com \
    --cc=knut.omang@oracle.com \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    --cc=linux-um@lists.infradead.org \
    --cc=logang@deltatee.com \
    --cc=mpe@ellerman.id.au \
    --cc=mtk.manpages@gmail.com \
    --cc=peterz@infradead.org \
    --cc=pmladek@suse.com \
    --cc=rdunlap@infradead.org \
    --cc=richard@nod.at \
    --cc=rientjes@google.com \
    --cc=robh@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=sboyd@kernel.org \
    --cc=shuah@kernel.org \
    --cc=tytso@mit.edu \
    --cc=wfg@linux.intel.com \
    --cc=yamada.masahiro@socionext.com \
    --cc=yzaikin@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).