Re: [PATCH] printk: Userspace format enumeration support

Chris Down
2021-02-07T14:15:25Z

Joe Perches writes:
>> There are certainly printks which can't be trivially monitored using the printk
>> format alone, but the vast majority of the ones that are monitored _do_ have
>> meaningful formats and can be monitored over time. No solution to this is going
>> to catch every single case, especially when so much of the information can be
>> generated dyamically, but this patchset still goes a long way to making printk
>> monitoring more tractable for use cases like the one described in the
>> changelog.
>
>For the _vast_ majority of printk strings, this can easily be found
>and compared using a trivial modification to strings.

There are several issues with your proposed approach that make it unsuitable
for use as part of a reliable production environment:

1. It misses printk() formats without KERN_SOH

printk() formats without KERN_SOH are legal and use MESSAGE_LOGLEVEL_DEFAULT.
On my test kernel, your proposed patch loses >5% of printk formats -- over 200
messages -- due to this, including critical ones like those about hardware or
other errors.

2. Users don't always have the kernel image available

Many of our machines and many of the machines of others like us do not boot
using local storage, but instead use PXE or other technologies where the kernel
may not be stored during runtime.

As is described in the changelog, it is necessary to be able to vary
remediations not only based on what is already in /dev/kmsg, but also to be
able to make decisions about our methodology based on what's _supported_ in the
running kernel at runtime, and your proposed approach makes this not viable.

3. `KERN_SOH + level' can appear in other places than just printk strings

KERN_SOH is just ASCII '\001' -- it's not distinctive or unique, even when
paired with a check for something that looks like a level after it. For this
reason, your proposed patch results in a non-trivial amount of non-printk
related garbage in its output. For example:

% binutils/strings -k /tmp/vmlinux | head -5
3L)s
3L)s
c,[]A\
c(L)c
d$pL)d$`u4

Fundamentally, one cannot use a tool which just determines whether something is
printable to determine semantic intent.

4. strings(1) output cannot differentiate embedded newlines and new formats

The following has exactly the same output from strings(1), but will manifest
completely differently at printk() time:

printk(KERN_ERR "line one\nline two\nline three\n");
printk("line four\n");

With strings, the hypothetical output would be:*

3line one\nline two\nline three\nline four\n

* "line four\n" would also be missing with your current -k check.

But this makes it impossible to distinguish between this, compared to:

printk(KERN_ERR "line one\nline two\n");
printk("line three\n");
printk("line four\n");

The originally posted patch _does_ differentiate between these cases, using \0
as a reliable separator. Its outputs are, respectively:

\0013line one\nline two\nline three\0\nline four\n\0
\0013line one\nline two\n\0line three\nline four\n\0

This isn't just a theoretical concern -- there are plenty of places which use
multiline printks, and we must be able to distinguish between that and
_multiple_ printks. Not being able to differentiate cases like these would
dramatically reduce the effectiveness of printk enumeration, as we can no
longer ascertain which formats will always be used together (for example, in
the case of sequences of printks guarded by conditionals, which are all over
the place).

5. strings(1) is not contextually aware, and cannot be made to act as if it is

strings has no idea about what it is reading, which is why it is more than
happy to output the kind of meaningless output shown in #3. There are plenty of
places across the kernel where there might be a sequence of bytes which the
strings utility happens to interpret as being semantically meaningful, but in
reality just happens to be an unrelated sequence of coincidentally printable
bytes that just happens to contain a \001.

I appreciate your willingness to propose other solutions, but for these
reasons, the proposed strings(1) patch would not suffice as an interface for
printk enumeration.