Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
On Fri, Jun 28, 2019 at 01:01:54AM -0700, Brendan Higgins wrote: > On Wed, Jun 26, 2019 at 11:10 PM Luis Chamberlain wrote: > > > > On Wed, Jun 26, 2019 at 09:07:43PM -0700, Iurii Zaikin wrote: > > > On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain > > > 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. > > I see later on that some of the API stuff you are talking about is > public APIs from the standpoint of user (outside of LInux) visible. Right, UAPI. > To > be clear, is that what you mean by public APIs throughout, or would > you distinguish between correctness tests, internal API tests, and > external API tests? I would definitely recommend distingishing between all of these. Kernel API (or just call it API), UAPI, and 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. > > Yep, I think that is long term hope. Some of the file system interface > stuff that requires a filesystem to be mounted somewhere might get a > little weird/difficult, but I suspect we should be able to do it > eventually. I mean it's all just C code right? Should mostly boil down > to someone figuring out how to do it the first time. There used to be hacks in the kernel the call syscalls in a few places. This was cleaned up and addressed via Dominik Brodowski's series last year in March: http://lkml.kernel.org/r/20180325162527.ga17...@light.dominikbrodowski.net An example commit: d300b610812f3 ("kernel: use kernel_wait4() instead of sys_wait4()"). So it would seem the work is done, and you'd just have to use the respective exposed kernel_syscallname() calls, or add some if you want to test a specific syscall in kernel space. Luis
Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
On Wed, Jun 26, 2019 at 11:10 PM Luis Chamberlain wrote: > > On Wed, Jun 26, 2019 at 09:07:43PM -0700, Iurii Zaikin wrote: > > On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain 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. I see later on that some of the API stuff you are talking about is public APIs from the standpoint of user (outside of LInux) visible. To be clear, is that what you mean by public APIs throughout, or would you distinguish between correctness tests, internal API tests, and external API tests? > 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. Yep, I think that is long term hope. Some of the file system interface stuff that requires a filesystem to be mounted somewhere might get a little weird/difficult, but I suspect we should be able to do it eventually. I mean it's all just C code right? Should mostly boil down to someone figuring out how to do it the first time. > > > > +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); >
Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
On Wed, Jun 26, 2019 at 09:07:43PM -0700, Iurii Zaikin wrote: > On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain 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 = { > > > +
Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain wrote: > > On Mon, Jun 17, 2019 at 01:26:12AM -0700, Brendan Higgins wrote: > > From: Iurii Zaikin > > > > KUnit tests for initialized data behavior of proc_dointvec that is > > explicitly checked in the code. Includes basic parsing tests including > > int min/max overflow. > > First, thanks for this work! My review below. > > > > Signed-off-by: Iurii Zaikin > > Signed-off-by: Brendan Higgins > > Reviewed-by: Greg Kroah-Hartman > > Reviewed-by: Logan Gunthorpe > > --- > > Changes Since Last Revision: > > - Iurii did some clean up (thanks Iurii!) as suggested by Stephen Boyd. > > --- > > kernel/Makefile | 2 + > > kernel/sysctl-test.c | 242 +++ > > lib/Kconfig.debug| 10 ++ > > 3 files changed, 254 insertions(+) > > create mode 100644 kernel/sysctl-test.c > > > > diff --git a/kernel/Makefile b/kernel/Makefile > > index a8d923b5481ba..50fd511cd0ee0 100644 > > --- a/kernel/Makefile > > +++ b/kernel/Makefile > > @@ -114,6 +114,8 @@ obj-$(CONFIG_HAS_IOMEM) += iomem.o > > obj-$(CONFIG_ZONE_DEVICE) += memremap.o > > obj-$(CONFIG_RSEQ) += rseq.o > > > > +obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o > > And we have lib/test_sysctl.c of selftests. > > I'm fine with this going in as-is to its current place, but if we have > to learn from selftests I'd say we try to stick to a convention so > folks know what framework a test is for, and to ensure folks can > easily tell if its test code or not. > > Perhaps simply a directory for kunit tests would suffice alone. > > > + > > obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak.o > > KASAN_SANITIZE_stackleak.o := n > > KCOV_INSTRUMENT_stackleak.o := n > > diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c > > new file mode 100644 > > index 0..cb61ad3c7db63 > > --- /dev/null > > +++ b/kernel/sysctl-test.c > > @@ -0,0 +1,242 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * KUnit test of proc sysctl. > > + */ > > + > > +#include > > +#include > > + > > +static int i_zero; > > +static int i_one_hundred = 100; > > + > > +struct test_sysctl_data { > > + int int_0001; > > + int int_0002; > > + int int_0003[4]; > > + > > + unsigned int uint_0001; > > + > > + char string_0001[65]; > > +}; > > + > > +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)", > > +}; > > + > > +static void sysctl_test_dointvec_null_tbl_data(struct kunit *test) > > +{ > > + struct ctl_table table = { > > + .procname = "foo", > > + .data = NULL, > > + .maxlen = sizeof(int), > > + .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); > > It is a bit odd, but it does happen, for a developer to be calling > proc_dointvec() directly, instead typically folks just register a table > and let it do its thing. That said, someone not too familiar with proc > code would see this and really have no clue exactly what is being > tested. > > Even as a maintainer, I had to read the code for proc_dointvec() a bit > to understand that the above is a *read* attempt to the .data field > being allocated. Because its a write, the len set to a bogus does not > matter as we are expecting the proc_dointvec() to update len for us. > > If a test fails, it would be good to for anyone to easily grasp what is > being tested. So... a few words documenting each test case would be nice. > > > + len = 1234; > > + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, buffer, &len, > > &pos)); > > + KUNIT_EXPECT_EQ(test, (size_t)0, len); > > And this is a write... > > A nice tests given the data on the table allocated is not assigned. > > I don't see any other areas in the kernel where we open code a > proc_dointvec() call where the second argument is a digit, it > always is with a variable. As such there would be no need for > us to expose helpers to make it clear if one is a read or write. > But for *this* case, I think it would be useful to add two wrappers > inside this kunit test module which sprinkles the 0 or 1, this way > a reader can easily know what mode is being tested. > > kunit_proc_dointvec_read() > kunit_proc_dointvec_write() > > Or just use #define KUNIT_PROC_READ 0, #define KUNIT_PROC_WRITE 1. > Whatever makes this code more legible. Went with the #de
Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
On Mon, Jun 17, 2019 at 01:26:12AM -0700, Brendan Higgins wrote: > From: Iurii Zaikin > > KUnit tests for initialized data behavior of proc_dointvec that is > explicitly checked in the code. Includes basic parsing tests including > int min/max overflow. First, thanks for this work! My review below. > > Signed-off-by: Iurii Zaikin > Signed-off-by: Brendan Higgins > Reviewed-by: Greg Kroah-Hartman > Reviewed-by: Logan Gunthorpe > --- > Changes Since Last Revision: > - Iurii did some clean up (thanks Iurii!) as suggested by Stephen Boyd. > --- > kernel/Makefile | 2 + > kernel/sysctl-test.c | 242 +++ > lib/Kconfig.debug| 10 ++ > 3 files changed, 254 insertions(+) > create mode 100644 kernel/sysctl-test.c > > diff --git a/kernel/Makefile b/kernel/Makefile > index a8d923b5481ba..50fd511cd0ee0 100644 > --- a/kernel/Makefile > +++ b/kernel/Makefile > @@ -114,6 +114,8 @@ obj-$(CONFIG_HAS_IOMEM) += iomem.o > obj-$(CONFIG_ZONE_DEVICE) += memremap.o > obj-$(CONFIG_RSEQ) += rseq.o > > +obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o And we have lib/test_sysctl.c of selftests. I'm fine with this going in as-is to its current place, but if we have to learn from selftests I'd say we try to stick to a convention so folks know what framework a test is for, and to ensure folks can easily tell if its test code or not. Perhaps simply a directory for kunit tests would suffice alone. > + > obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak.o > KASAN_SANITIZE_stackleak.o := n > KCOV_INSTRUMENT_stackleak.o := n > diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c > new file mode 100644 > index 0..cb61ad3c7db63 > --- /dev/null > +++ b/kernel/sysctl-test.c > @@ -0,0 +1,242 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * KUnit test of proc sysctl. > + */ > + > +#include > +#include > + > +static int i_zero; > +static int i_one_hundred = 100; > + > +struct test_sysctl_data { > + int int_0001; > + int int_0002; > + int int_0003[4]; > + > + unsigned int uint_0001; > + > + char string_0001[65]; > +}; > + > +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)", > +}; > + > +static void sysctl_test_dointvec_null_tbl_data(struct kunit *test) > +{ > + struct ctl_table table = { > + .procname = "foo", > + .data = NULL, > + .maxlen = sizeof(int), > + .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); It is a bit odd, but it does happen, for a developer to be calling proc_dointvec() directly, instead typically folks just register a table and let it do its thing. That said, someone not too familiar with proc code would see this and really have no clue exactly what is being tested. Even as a maintainer, I had to read the code for proc_dointvec() a bit to understand that the above is a *read* attempt to the .data field being allocated. Because its a write, the len set to a bogus does not matter as we are expecting the proc_dointvec() to update len for us. If a test fails, it would be good to for anyone to easily grasp what is being tested. So... a few words documenting each test case would be nice. > + len = 1234; > + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, buffer, &len, &pos)); > + KUNIT_EXPECT_EQ(test, (size_t)0, len); And this is a write... A nice tests given the data on the table allocated is not assigned. I don't see any other areas in the kernel where we open code a proc_dointvec() call where the second argument is a digit, it always is with a variable. As such there would be no need for us to expose helpers to make it clear if one is a read or write. But for *this* case, I think it would be useful to add two wrappers inside this kunit test module which sprinkles the 0 or 1, this way a reader can easily know what mode is being tested. kunit_proc_dointvec_read() kunit_proc_dointvec_write() Or just use #define KUNIT_PROC_READ 0, #define KUNIT_PROC_WRITE 1. Whatever makes this code more legible. > +} > + > +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, > +