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

[Alexander Potapenko <glider@google.com>]

On Wed, Oct 23, 2019 at 7:57 PM Alexander Potapenko <glider@google.com> wrote:
>
> 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.
I am very sorry about this. I'm going to re-send the patches soon and
CC everyone on every patch.
What I thought to be reducing the email burden on every reviewer
turned out to be really inconvenient.
> > 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



-- 
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