Re: [PATCH 4/4] test_printf: test printf family at runtime

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Tue, Sep 29 2015, Kees Cook <keescook@chromium.org> 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