Re: [PATCH RFC v1 05/26] printk_safe: externalize printk_context

[Alexander Potapenko <glider@google.com>]

On Mon, Oct 21, 2019 at 11:09 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Fri 2019-10-18 11:42:43, glider@google.com wrote:
> > Petr Mladek suggested we use
> >   this_cpu_read(printk_context) & PRINTK_SAFE_CONTEXT_MASK
> > instead of checking the spinlock status in kmsan_pr_err()
>
> I would like to understand why the check is needed.
>
> My guess is that it prevents a infinite recursion.
> Is this the case? It might be possible to debug this using
> trace_printk().
This indeed used to prevent infinite recursion, however looks like the
check isn't needed anymore.
When I remove it, KMSAN doesn't hang, probably because printk is doing
a better job at deadlock prevention.

What I'm seeing now is that e.g. in the following case:
  ptr = kmalloc(sizeof(int), GFP_KERNEL);
  if (ptr)
    pr_info("true\n");
  else
    pr_info("false\n");

KMSAN detects errors within pr_info(), namely in vprintk_store().
If I understand correctly, printing from that point causes printk to
use the per-cpu buffer, which is flushed once we're done with the
original printing:

[   58.984971][ T8390] BUG: KMSAN: uninit-value in
kmsan_handle_vprintk+0xa0/0xf0
...
[   59.061976][    C0] BUG: KMSAN: uninit-value in vsnprintf+0x3276/0x32b0
...
[   59.062457][    C0] BUG: KMSAN: uninit-value in format_decode+0x17f/0x1900
...
[   59.062961][    C0] BUG: KMSAN: uninit-value in format_decode+0x17f/0x1900
...
[   59.063338][    C0] Lost 6207 message(s)!

So it turns out we'd better disable reporting under logbuf_lock,
otherwise these lost messages will confuse the developers.
Because the first pr_info() isn't called by KMSAN itself, the tool
still needs a way to know it's running inside printk.

> It is important. If it is the recursion then there are
> two possibilities how to prevent it. Either prevent
> calling the recursive printk(). Or prevent calling
> the memory checker recursive. I am not sure what makes
> more sense.
>
> Is printk() the only piece of code where you need to avoid
> recursion? I wonder how it is prevented in the other cases.
>
> > This appears to be less intrusive, although we'll need to declare
> > printk_context in some printk header.
>
> It is easier than the original approach. But the main motivation
> is that it is more reliable. The spinlock is a global lock.
> But it seems that it is enough to check the state of the local
> CPU.
>
> Finally, could you please CC me to the patch(es) that are using
> this variable? I would actually prefer to be in CC of entire
> patchsets so that I see the full context.
>
> Best Regards,
> Petr



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg