[PATCH 4/4] test_printf: test printf family at runtime
This adds a simple module for testing the kernel's printf
facilities. Previously, some %p extensions have caused a wrong return
value in case the entire output didn't fit and/or been unusable in
kasprintf(). This should help catch such issues. Also, it should help
ensure that changes to the formatting algorithms don't break anything.
I'm not sure if we have a struct dentry or struct file lying around at
boot time or if we can fake one, but most %p extensions should be
testable, as should the ordinary number and string formatting.
The nature of vararg functions means we can't use a more conventional
table-driven approach.
For now, this is mostly a skeleton; contributions are very
welcome. Some tests are/will be slightly annoying to write, since the
expected output depends on stuff like CONFIG_*, sizeof(long), runtime
values etc.
Signed-off-by: Rasmus Villemoes
---
lib/Kconfig.debug | 3 +
lib/Makefile | 1 +
lib/test_printf.c | 364 ++
3 files changed, 368 insertions(+)
create mode 100644 lib/test_printf.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ab76b99adc85..c23fc42dc659 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1685,6 +1685,9 @@ config TEST_STRING_HELPERS
config TEST_KSTRTOX
tristate "Test kstrto*() family of functions at runtime"
+config TEST_PRINTF
+ tristate "Test printf() family of functions at runtime"
+
config TEST_RHASHTABLE
tristate "Perform selftest on resizable hash table"
default n
diff --git a/lib/Makefile b/lib/Makefile
index 13a7c6ae3fec..775de427ea92 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
+obj-$(CONFIG_TEST_PRINTF) += test_printf.o
ifeq ($(CONFIG_DEBUG_KOBJECT),y)
CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/test_printf.c b/lib/test_printf.c
new file mode 100644
index ..d9a2741c2909
--- /dev/null
+++ b/lib/test_printf.c
@@ -0,0 +1,364 @@
+/*
+ * Test cases for printf facility.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#define BUF_SIZE 256
+#define FILL_CHAR '$'
+
+#define PTR1 ((void*)0x01234567)
+#define PTR2 ((void*)(long)(int)0xfedcba98)
+
+#if BITS_PER_LONG == 64
+#define PTR1_ZEROES "0"
+#define PTR1_SPACES " "
+#define PTR1_STR "1234567"
+#define PTR2_STR "fedcba98"
+#define PTR_WIDTH 16
+#else
+#define PTR1_ZEROES "0"
+#define PTR1_SPACES " "
+#define PTR1_STR "1234567"
+#define PTR2_STR "fedcba98"
+#define PTR_WIDTH 8
+#endif
+#define PTR_WIDTH_STR stringify(PTR_WIDTH)
+
+static unsigned total_tests __initdata;
+static unsigned failed_tests __initdata;
+static char *test_buffer __initdata;
+
+static int __printf(4, 0) __init
+do_test(int bufsize, const char *expect, int elen,
+ const char *fmt, va_list ap)
+{
+ va_list aq;
+ int ret, written;
+
+ total_tests++;
+
+ memset(test_buffer, FILL_CHAR, BUF_SIZE);
+ va_copy(aq, ap);
+ ret = vsnprintf(test_buffer, bufsize, fmt, aq);
+ va_end(aq);
+
+ if (ret != elen) {
+ pr_warn("bad return value, expected %d, got %d, format was
'%s'\n",
+ elen, ret, fmt);
+ return 1;
+ }
+
+ if (!bufsize) {
+ if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE)) {
+ pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to
buffer\n",
+ fmt);
+ return 1;
+ }
+ return 0;
+ }
+
+ written = min(bufsize-1, elen);
+ if (test_buffer[written]) {
+ pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not nul-terminate
buffer\n",
+ bufsize, fmt);
+ return 1;
+ }
+
+ if (memcmp(test_buffer, expect, written)) {
+ pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected
'%.*s'\n",
+ bufsize, fmt, test_buffer, written, expect);
+ return 1;
+ }
+ return 0;
+}
+
+
+static void __printf(3, 4) __init
+__test(const char *expect, int elen, const char *fmt, ...)
+{
+ va_list ap;
+ int rand;
+ char *p;
+
+ BUG_ON(elen >= BUF_SIZE);
+
+ va_start(ap, fmt);
+
+ /*
+* Every fmt+args is subjected to four tests: Three where we
+* tell vsnprintf varying buffer sizes (plenty, not quite
+* enough and 0), and then we also test that kvasprintf would
+* be able to print it as expected.
+*/
+ failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap);
+ rand = 1 + prandom_u32_max(elen+1);
+ /* Since elen < BUF_SIZE, we have 1
Re: [PATCH 4/4] test_printf: test printf family at runtime
On Mon, 2015-09-28 at 22:55 +0200, Rasmus Villemoes wrote:
> On Mon, Sep 28 2015, Andy Shevchenko <
> andriy.shevche...@linux.intel.com> wrote:
>
> > On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote:
> > > This adds a simple module for testing the kernel's printf
> > > facilities. Previously, some %p extensions have caused a wrong
> > > return
> > > value in case the entire output didn't fit and/or been unusable
> > > in
> > > kasprintf(). This should help catch such issues. Also, it should
> > > help
> > > ensure that changes to the formatting algorithms don't break
> > > anything.
> > >
> > > I'm not sure if we have a struct dentry or struct file lying
> > > around
> > > at
> > > boot time or if we can fake one, but most %p extensions should be
> > > testable, as should the ordinary number and string formatting.
> > >
> > > The nature of vararg functions means we can't use a more
> > > conventional
> > > table-driven approach.
> > >
> > > For now, this is mostly a skeleton; contributions are very
> > > welcome. Some tests are/will be slightly annoying to write, since
> > > the
> > > expected output depends on stuff like CONFIG_*, sizeof(long),
> > > runtime
> > > values etc.
> >
> > Few comments below.
> > > +#define test(expect, fmt, ...)
> > > \
> > > + __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
> >
> > Would be __test_m[em] / __test_s[tr] to distinguish them by name?
>
> Erh, no. The 'mem' version will only be used in a very few cases, and
> I
> really want the simple name "test" for the common case.
> > > +static void __init
> > > +test_basic(void)
> > > +{
> > > + test("", "");
> > > + test("100%", "100%%");
> > > + test("xxx%yyy", "xxx%cyyy", '%');
> > > + __test("xxx\0yyy", 7, "xxx%cyyy", '\0');
> >
> > And such pieces will be look better
> >
> > __test_str("xxx%yyy", "xxx%cyyy", '%');
> > __test_mem("xxx\0yyy", 7, "xxx%cyyy", '\0');
>
> I don't agree.
It's still better to distinguish what function does by names
test vs. __test confuses me.
But whatever, this is a test suite, not an actual code anyway.
> >Maybe commentary delimiter here and above where you have double
> >empty
> >line.
>
> And say what? I can avoid double empty lines if they bother you.
So, what was the reason to add them in the first place?
> > > +
> > > +MODULE_AUTHOR("Rasmus Villemoes ");
> > > +MODULE_LICENSE("GPL");
> >
> > GPL or ?..
>
> Honestly, I don't really care. Would you like BSD/GPL or what? I just
> copied from the majority of MODULE_LICENSE() instances.
You are the author, your choice. I'm okay with any applicable type.
--
Andy Shevchenko
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] test_printf: test printf family at runtime
On Wed, Sep 30 2015, Andy Shevchenko wrote:
> It's still better to distinguish what function does by names
> test vs. __test confuses me.
Think of test() as the ordinary "public" interface and __test() as
"private, but we expose it because you might need it, if you know what
you're doing". Having two related functions named like this is a
perfectly normal pattern in the kernel (even if one isn't necessarily
implemented in terms of the other); examples are
legio. krealloc/__krealloc (the latter is not called krealloc_nofree),
fls/__fls (not fls_I_know_word_is_nonzero, thank $deity), ...
>> >Maybe commentary delimiter here and above where you have double
>> >empty line.
>>
>> And say what? I can avoid double empty lines if they bother you.
>
> So, what was the reason to add them in the first place?
Just so we could have this interesting little chat. I don't think it was
intentional, but don't worry, they're gone.
>> > > +MODULE_AUTHOR("Rasmus Villemoes ");
>> > > +MODULE_LICENSE("GPL");
>> >
>> > GPL or ?..
>>
>> Honestly, I don't really care. Would you like BSD/GPL or what? I just
>> copied from the majority of MODULE_LICENSE() instances.
>
> You are the author, your choice. I'm okay with any applicable type.
Then I'll stick to GPL, even if that's more cargo-culting than
deliberate informed choice.
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] test_printf: test printf family at runtime
On Tue, Sep 29 2015, Kees Cook wrote: > On Tue, Sep 29, 2015 at 12:10 AM, Rasmus Villemoes > wrote: > >> I guess I could, but do we really want to intentionally trigger >> WARN_ON_ONCEs? Say some distro chooses to load this module at boot >> time, then we'd both spam the kernel log with "false positives", and >> we'd have effectively disabled the WARN_ON_ONCEs for the actual >> kernel code. > > Distros don't tend to run the test modules by default. The most common > case is that it's part of a selftests run, in which case the machine > has usually been freshly booted, etc. I think it's more important to > catch regressions. > >> Maybe we can hide such things behind some module parameter, so that the >> user explicitly has to ask for them. Also, we can't really probe the >> "success" if these sanity checks from within the module (can we?) - the >> user would have to check dmesg manually anyway. > > I think it's best that tests run with as few options as possible. > Surely we can test the behavior? The bstr returns 0, so the string > should be truncated? I haven't looked closely, but it seemed testable. Well, yes, obviously we can check that part, but I also think it would be nice to check that it actually resulted in a warning, which is what I think would require manual inspection. I'm still not convinced intentionally triggering a WARN on module load is a good idea, even if the module wouldn't be loaded by normal distros. Especially because of the _ONCE part, so that actual bugs might not be warned about for the rest of that boot's lifetime. I'd certainly like to hear what others think about this. >>> I love tests! Thank you. :) One suggestion would be to wire it up to >>> the tools/testing/selftests tree; it should be trivial once you change >>> the test_printf_init return code. >> >> I'll look into that. Not sure I have too much time to work on this this >> side of the merge window, and since these all seem to be things that can >> be incrementally added, I'd prefer seeing something go into 4.4 instead >> of waiting till it's "perfect". So unless I hear otherwise, I'll post a >> v2 with the minor things addressed and ask Andrew to take that through >> -mm. > > I'll send the glue patch... Thanks. v2 coming up. For now I'll just change the return code; we can always add tests for the sanity checks later, when we figure out the best way to do them. Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] test_printf: test printf family at runtime
On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote:
> This adds a simple module for testing the kernel's printf
> facilities. Previously, some %p extensions have caused a wrong return
> value in case the entire output didn't fit and/or been unusable in
> kasprintf(). This should help catch such issues. Also, it should help
> ensure that changes to the formatting algorithms don't break
> anything.
>
> I'm not sure if we have a struct dentry or struct file lying around
> at
> boot time or if we can fake one, but most %p extensions should be
> testable, as should the ordinary number and string formatting.
>
> The nature of vararg functions means we can't use a more conventional
> table-driven approach.
>
> For now, this is mostly a skeleton; contributions are very
> welcome. Some tests are/will be slightly annoying to write, since the
> expected output depends on stuff like CONFIG_*, sizeof(long), runtime
> values etc.
Few comments below.
>
> Signed-off-by: Rasmus Villemoes
> ---
> lib/Kconfig.debug | 3 +
> lib/Makefile | 1 +
> lib/test_printf.c | 364
> ++
> 3 files changed, 368 insertions(+)
> create mode 100644 lib/test_printf.c
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ab76b99adc85..c23fc42dc659 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1685,6 +1685,9 @@ config TEST_STRING_HELPERS
> config TEST_KSTRTOX
> tristate "Test kstrto*() family of functions at runtime"
>
> +config TEST_PRINTF
> + tristate "Test printf() family of functions at runtime"
> +
> config TEST_RHASHTABLE
> tristate "Perform selftest on resizable hash table"
> default n
> diff --git a/lib/Makefile b/lib/Makefile
> index 13a7c6ae3fec..775de427ea92 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> +obj-$(CONFIG_TEST_PRINTF) += test_printf.o
>
> ifeq ($(CONFIG_DEBUG_KOBJECT),y)
> CFLAGS_kobject.o += -DDEBUG
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> new file mode 100644
> index ..d9a2741c2909
> --- /dev/null
> +++ b/lib/test_printf.c
> @@ -0,0 +1,364 @@
> +/*
> + * Test cases for printf facility.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#define BUF_SIZE 256
> +#define FILL_CHAR '$'
> +
> +#define PTR1 ((void*)0x01234567)
> +#define PTR2 ((void*)(long)(int)0xfedcba98)
> +
> +#if BITS_PER_LONG == 64
> +#define PTR1_ZEROES "0"
> +#define PTR1_SPACES " "
> +#define PTR1_STR "1234567"
> +#define PTR2_STR "fedcba98"
> +#define PTR_WIDTH 16
> +#else
> +#define PTR1_ZEROES "0"
> +#define PTR1_SPACES " "
> +#define PTR1_STR "1234567"
> +#define PTR2_STR "fedcba98"
> +#define PTR_WIDTH 8
> +#endif
> +#define PTR_WIDTH_STR stringify(PTR_WIDTH)
> +
> +static unsigned total_tests __initdata;
> +static unsigned failed_tests __initdata;
> +static char *test_buffer __initdata;
> +
> +static int __printf(4, 0) __init
> +do_test(int bufsize, const char *expect, int elen,
> + const char *fmt, va_list ap)
> +{
> + va_list aq;
> + int ret, written;
> +
> + total_tests++;
> +
> + memset(test_buffer, FILL_CHAR, BUF_SIZE);
> + va_copy(aq, ap);
> + ret = vsnprintf(test_buffer, bufsize, fmt, aq);
> + va_end(aq);
> +
> + if (ret != elen) {
> + pr_warn("bad return value, expected %d, got %d,
> format was '%s'\n",
> + elen, ret, fmt);
> + return 1;
> + }
> +
> + if (!bufsize) {
> + if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE)) {
> + pr_warn("vsnprintf(buf, 0, \"%s\", ...)
> wrote to buffer\n",
> + fmt);
> + return 1;
> + }
> + return 0;
> + }
> +
> + written = min(bufsize-1, elen);
> + if (test_buffer[written]) {
> + pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not nul
> -terminate buffer\n",
> + bufsize, fmt);
> + return 1;
> + }
> +
> + if (memcmp(test_buffer, expect, written)) {
> + pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s',
> expected '%.*s'\n",
> + bufsize, fmt, test_buffer, written, expect);
> + return 1;
> + }
> + return 0;
> +}
> +
> +
> +static void __printf(3, 4) __init
> +__test(const char *expect, int elen, const char *fmt, ...)
> +{
> + va_list ap;
> + int rand;
> + char *p;
> +
> + BUG_ON(elen >= BUF_SIZE);
> +
> + va_start(ap, fmt);
> +
> + /*
> + * Every fmt+args is subjected to four tests: Three where we
> + * tell vsnp
Re: [PATCH 4/4] test_printf: test printf family at runtime
On Mon, Sep 28 2015, Andy Shevchenko wrote:
> On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote:
>> This adds a simple module for testing the kernel's printf
>> facilities. Previously, some %p extensions have caused a wrong return
>> value in case the entire output didn't fit and/or been unusable in
>> kasprintf(). This should help catch such issues. Also, it should help
>> ensure that changes to the formatting algorithms don't break
>> anything.
>>
>> I'm not sure if we have a struct dentry or struct file lying around
>> at
>> boot time or if we can fake one, but most %p extensions should be
>> testable, as should the ordinary number and string formatting.
>>
>> The nature of vararg functions means we can't use a more conventional
>> table-driven approach.
>>
>> For now, this is mostly a skeleton; contributions are very
>> welcome. Some tests are/will be slightly annoying to write, since the
>> expected output depends on stuff like CONFIG_*, sizeof(long), runtime
>> values etc.
>
> Few comments below.
>
>> +
>> +#define test(expect, fmt, ...) \
>> +__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
>
> Would be __test_m[em] / __test_s[tr] to distinguish them by name?
Erh, no. The 'mem' version will only be used in a very few cases, and I
really want the simple name "test" for the common case.
> And might be inline function?
That'd make the vararg handling more cumbersome.
>> +static void __init
>> +test_basic(void)
>> +{
>> +test("", "");
>> +test("100%", "100%%");
>> +test("xxx%yyy", "xxx%cyyy", '%');
>> +__test("xxx\0yyy", 7, "xxx%cyyy", '\0');
>
> And such pieces will be look better
>
> __test_str("xxx%yyy", "xxx%cyyy", '%');
> __test_mem("xxx\0yyy", 7, "xxx%cyyy", '\0');
I don't agree.
>> +
>> +static void __init
>> +netdev_features(void)
>> +{
>> +}
>> +
>> +
>
> Maybe commentary delimiter here and above where you have double empty
> line.
And say what? I can avoid double empty lines if they bother you.
>> +
>> +return 0;
>
> Do we need this module in a memory?
I guess not. At first I thought it didn't really matter since all
functions and data are __init, but I suppose a little metadata would
stick around if loading is "successful". Will fix.
>> +
>> +MODULE_AUTHOR("Rasmus Villemoes ");
>> +MODULE_LICENSE("GPL");
>
> GPL or ?..
Honestly, I don't really care. Would you like BSD/GPL or what? I just
copied from the majority of MODULE_LICENSE() instances.
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] test_printf: test printf family at runtime
On Fri, Sep 25, 2015 at 10:41 AM, Rasmus Villemoes
wrote:
> This adds a simple module for testing the kernel's printf
> facilities. Previously, some %p extensions have caused a wrong return
> value in case the entire output didn't fit and/or been unusable in
> kasprintf(). This should help catch such issues. Also, it should help
> ensure that changes to the formatting algorithms don't break anything.
>
> I'm not sure if we have a struct dentry or struct file lying around at
> boot time or if we can fake one, but most %p extensions should be
> testable, as should the ordinary number and string formatting.
>
> The nature of vararg functions means we can't use a more conventional
> table-driven approach.
>
> For now, this is mostly a skeleton; contributions are very
> welcome. Some tests are/will be slightly annoying to write, since the
> expected output depends on stuff like CONFIG_*, sizeof(long), runtime
> values etc.
>
> Signed-off-by: Rasmus Villemoes
> ---
> lib/Kconfig.debug | 3 +
> lib/Makefile | 1 +
> lib/test_printf.c | 364
> ++
> 3 files changed, 368 insertions(+)
> create mode 100644 lib/test_printf.c
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ab76b99adc85..c23fc42dc659 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1685,6 +1685,9 @@ config TEST_STRING_HELPERS
> config TEST_KSTRTOX
> tristate "Test kstrto*() family of functions at runtime"
>
> +config TEST_PRINTF
> + tristate "Test printf() family of functions at runtime"
> +
> config TEST_RHASHTABLE
> tristate "Perform selftest on resizable hash table"
> default n
> diff --git a/lib/Makefile b/lib/Makefile
> index 13a7c6ae3fec..775de427ea92 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> +obj-$(CONFIG_TEST_PRINTF) += test_printf.o
>
> ifeq ($(CONFIG_DEBUG_KOBJECT),y)
> CFLAGS_kobject.o += -DDEBUG
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> new file mode 100644
> index ..d9a2741c2909
> --- /dev/null
> +++ b/lib/test_printf.c
> @@ -0,0 +1,364 @@
> +/*
> + * Test cases for printf facility.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#define BUF_SIZE 256
> +#define FILL_CHAR '$'
> +
> +#define PTR1 ((void*)0x01234567)
> +#define PTR2 ((void*)(long)(int)0xfedcba98)
> +
> +#if BITS_PER_LONG == 64
> +#define PTR1_ZEROES "0"
> +#define PTR1_SPACES " "
> +#define PTR1_STR "1234567"
> +#define PTR2_STR "fedcba98"
> +#define PTR_WIDTH 16
> +#else
> +#define PTR1_ZEROES "0"
> +#define PTR1_SPACES " "
> +#define PTR1_STR "1234567"
> +#define PTR2_STR "fedcba98"
> +#define PTR_WIDTH 8
> +#endif
> +#define PTR_WIDTH_STR stringify(PTR_WIDTH)
> +
> +static unsigned total_tests __initdata;
> +static unsigned failed_tests __initdata;
> +static char *test_buffer __initdata;
> +
> +static int __printf(4, 0) __init
> +do_test(int bufsize, const char *expect, int elen,
> + const char *fmt, va_list ap)
> +{
> + va_list aq;
> + int ret, written;
> +
> + total_tests++;
> +
> + memset(test_buffer, FILL_CHAR, BUF_SIZE);
> + va_copy(aq, ap);
> + ret = vsnprintf(test_buffer, bufsize, fmt, aq);
> + va_end(aq);
> +
> + if (ret != elen) {
> + pr_warn("bad return value, expected %d, got %d, format was
> '%s'\n",
> + elen, ret, fmt);
> + return 1;
> + }
> +
> + if (!bufsize) {
> + if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE)) {
> + pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to
> buffer\n",
> + fmt);
> + return 1;
> + }
> + return 0;
> + }
> +
> + written = min(bufsize-1, elen);
> + if (test_buffer[written]) {
> + pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not
> nul-terminate buffer\n",
> + bufsize, fmt);
> + return 1;
> + }
> +
> + if (memcmp(test_buffer, expect, written)) {
> + pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected
> '%.*s'\n",
> + bufsize, fmt, test_buffer, written, expect);
> + return 1;
> + }
> + return 0;
> +}
> +
> +
> +static void __printf(3, 4) __init
> +__test(const char *expect, int elen, const char *fmt, ...)
> +{
> + va_list ap;
> + int rand;
> + char *p;
> +
> + BUG_ON(elen >= BUF_SIZE);
> +
> + va_start(ap, fmt);
> +
> + /*
> +* Every fmt+args is subjected
Re: [PATCH 4/4] test_printf: test printf family at runtime
On Tue, Sep 29 2015, Kees Cook wrote:
>> +static void __init
>> +test_string(void)
>> +{
>> + test("", "%s%.0s", "", "123");
>> + test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456");
>> + test("1 | 2|3 | 4|5 ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3,
>> "3", 3, "4", -3, "5");
>> + /*
>> +* POSIX and C99 say that a missing precision should be
>> +* treated as a precision of 0. However, the kernel's printf
>> +* implementation treats this case as if the . wasn't
>> +* present. Let's add a test case documenting the current
>> +* behaviour; should anyone ever feel the need to follow the
>> +* standards more closely, this can be revisited.
>> +*/
>> + test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
>> + test("a | | ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
>> +}
>
> Could you add a test for your 2/4 patch (bstr_printf size > INT_MAX
> change) as well?
I suppose you'd also want checks for the somewhat more important
vsnprintf size check and unknown specifiers? I guess I could, but do we
really want to intentionally trigger WARN_ON_ONCEs? Say some distro
chooses to load this module at boot time, then we'd both spam the kernel
log with "false positives", and we'd have effectively disabled the
WARN_ON_ONCEs for the actual kernel code.
Maybe we can hide such things behind some module parameter, so that the
user explicitly has to ask for them. Also, we can't really probe the
"success" if these sanity checks from within the module (can we?) - the
user would have to check dmesg manually anyway.
>> +
>> +static void __init
>> +dentry(void)
>> +{
>> +}
>> +
>> +static void __init
>> +struct_va_format(void)
>> +{
>> +}
>> +
>> +static void __init
>> +struct_clk(void)
>> +{
>> +}
>
> For the empty functions, maybe just add a pr_info("TODO: struct_clk")
> or something?
I think that would be unnecessarily spammy. It should be obvious from
the code that it is just waiting for someone to fill in the blanks.
>> +static int __init
>> +test_printf_init(void)
>> +{
>> + test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
>> + if (!test_buffer)
>> + return -ENOMEM;
>> +
>> + test_basic();
>> + test_number();
>> + test_string();
>> + test_pointer();
>> +
>> + kfree(test_buffer);
>> +
>> + if (failed_tests == 0)
>> + pr_info("all %u tests passed\n", total_tests);
>> + else
>> + pr_warn("failed %u out of %u tests\n", failed_tests,
>> total_tests);
>> +
>> + return 0;
>> +}
>
> I actually have different feedback on leaving the module loaded: I
> think it should succeed to load when the tests pass and fail when they
> don't. This makes it a one-step test to check things ("what is
> modprobe's return code?"), instead of needed to parse dmesg.
Hm, I guess that makes sense. But, assuming we go with the module param
suggested above, would it be possible to (unload and) load with a
different set of parameters?
> I love tests! Thank you. :) One suggestion would be to wire it up to
> the tools/testing/selftests tree; it should be trivial once you change
> the test_printf_init return code.
I'll look into that. Not sure I have too much time to work on this this
side of the merge window, and since these all seem to be things that can
be incrementally added, I'd prefer seeing something go into 4.4 instead
of waiting till it's "perfect". So unless I hear otherwise, I'll post a
v2 with the minor things addressed and ask Andrew to take that through
-mm.
Thanks,
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] test_printf: test printf family at runtime
On Tue, Sep 29, 2015 at 12:10 AM, Rasmus Villemoes
wrote:
> On Tue, Sep 29 2015, Kees Cook wrote:
>
>>> +static void __init
>>> +test_string(void)
>>> +{
>>> + test("", "%s%.0s", "", "123");
>>> + test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456");
>>> + test("1 | 2|3 | 4|5 ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3,
>>> "3", 3, "4", -3, "5");
>>> + /*
>>> +* POSIX and C99 say that a missing precision should be
>>> +* treated as a precision of 0. However, the kernel's printf
>>> +* implementation treats this case as if the . wasn't
>>> +* present. Let's add a test case documenting the current
>>> +* behaviour; should anyone ever feel the need to follow the
>>> +* standards more closely, this can be revisited.
>>> +*/
>>> + test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
>>> + test("a | | ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
>>> +}
>>
>> Could you add a test for your 2/4 patch (bstr_printf size > INT_MAX
>> change) as well?
>
> I suppose you'd also want checks for the somewhat more important
> vsnprintf size check and unknown specifiers? I guess I could, but do we
> really want to intentionally trigger WARN_ON_ONCEs? Say some distro
> chooses to load this module at boot time, then we'd both spam the kernel
> log with "false positives", and we'd have effectively disabled the
> WARN_ON_ONCEs for the actual kernel code.
Distros don't tend to run the test modules by default. The most common
case is that it's part of a selftests run, in which case the machine
has usually been freshly booted, etc. I think it's more important to
catch regressions.
> Maybe we can hide such things behind some module parameter, so that the
> user explicitly has to ask for them. Also, we can't really probe the
> "success" if these sanity checks from within the module (can we?) - the
> user would have to check dmesg manually anyway.
I think it's best that tests run with as few options as possible.
Surely we can test the behavior? The bstr returns 0, so the string
should be truncated? I haven't looked closely, but it seemed testable.
>
>>> +
>>> +static void __init
>>> +dentry(void)
>>> +{
>>> +}
>>> +
>>> +static void __init
>>> +struct_va_format(void)
>>> +{
>>> +}
>>> +
>>> +static void __init
>>> +struct_clk(void)
>>> +{
>>> +}
>>
>> For the empty functions, maybe just add a pr_info("TODO: struct_clk")
>> or something?
>
> I think that would be unnecessarily spammy. It should be obvious from
> the code that it is just waiting for someone to fill in the blanks.
Ok.
>
>>> +static int __init
>>> +test_printf_init(void)
>>> +{
>>> + test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
>>> + if (!test_buffer)
>>> + return -ENOMEM;
>>> +
>>> + test_basic();
>>> + test_number();
>>> + test_string();
>>> + test_pointer();
>>> +
>>> + kfree(test_buffer);
>>> +
>>> + if (failed_tests == 0)
>>> + pr_info("all %u tests passed\n", total_tests);
>>> + else
>>> + pr_warn("failed %u out of %u tests\n", failed_tests,
>>> total_tests);
>>> +
>>> + return 0;
>>> +}
>>
>> I actually have different feedback on leaving the module loaded: I
>> think it should succeed to load when the tests pass and fail when they
>> don't. This makes it a one-step test to check things ("what is
>> modprobe's return code?"), instead of needed to parse dmesg.
>
> Hm, I guess that makes sense. But, assuming we go with the module param
> suggested above, would it be possible to (unload and) load with a
> different set of parameters?
Sure, you've freed memory already, it should be entirely safe to
unload (which is what the selftests script should do anyway). I still
don't think it should have options, though.
>> I love tests! Thank you. :) One suggestion would be to wire it up to
>> the tools/testing/selftests tree; it should be trivial once you change
>> the test_printf_init return code.
>
> I'll look into that. Not sure I have too much time to work on this this
> side of the merge window, and since these all seem to be things that can
> be incrementally added, I'd prefer seeing something go into 4.4 instead
> of waiting till it's "perfect". So unless I hear otherwise, I'll post a
> v2 with the minor things addressed and ask Andrew to take that through
> -mm.
I'll send the glue patch...
-Kees
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
