Re: [PATCH RFC v1 19/26] kmsan: call KMSAN hooks where needed

[Alexander Potapenko <glider@google.com>]

On Mon, Oct 21, 2019 at 11:25 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Fri 2019-10-18 11:42:57, glider@google.com wrote:
> > Insert KMSAN hooks that check for potential memory errors and/or make
> > necessary bookkeeping changes:
> >  - allocate/split/deallocate metadata pages in
> >    alloc_pages()/split_page()/free_page();
> >  - clear page shadow and origins in clear_page(), copy_user_highpage();
> >  - copy page metadata in copy_highpage(), wp_page_copy();
> >  - handle vmap()/vunmap()/iounmap();
> >  - handle task creation and deletion;
> >  - handle vprintk();
>
> I looked only at the printk part.
>
> >  - call softirq entry/exit hooks in kernel/softirq.c;
> >  - check/initialize memory sent to/read from USB, I2C, and network
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > To: Alexander Potapenko <glider@google.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: Petr Mladek <pmladek@suse.com>
> > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: linux-mm@kvack.org
>
> Could you please add into CC also the other printk co-maitainers?
Will do in the next version of this patch.

>   + Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>   + Steven Rostedt <rostedt@goodmis.org>
>
>
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index ca65327a6de8..f77fdcb5f861 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1914,7 +1914,12 @@ int vprintk_store(int facility, int level,
> >        * The printf needs to come first; we need the syslog
> >        * prefix which might be passed-in as a parameter.
> >        */
> > -     text_len = vscnprintf(text, sizeof(textbuf), fmt, args);
> > +     /*
> > +      * We've checked the printk arguments in vprintk_emit() already.
> > +      * Initialize |text_len| to prevent the errors from spreading.
> > +      */
> > +     text_len = KMSAN_INIT_VALUE(vscnprintf(text, sizeof(textbuf), fmt,
> > +                                            args));
>
> I am a bit confused by the comment.
Fixed in v2.
> What is the exact meaning of KMSAN_INIT_VALUE(), please?
Hope it'll be clear from the "kmsan: add KMSAN runtime" patch.
In fact KMSAN_INIT_VALUE is a wrapper that turns an uninitialized
value into an initialized one:
https://github.com/google/kmsan/blob/3207d604ad68f7e5defdbbd6e63cb97750f380c1/include/linux/kmsan-checks.h#L58
It passes the value into an identity function that always returns
unpoisoned values.
As a result, from now on text_len is considered initialized,
regardless of what vscnprintf() returned.
Without KMSAN_INIT_VALUE the tool will report a huge number of errors
when text_len is used in vprintk_store() and log_output().
> Does it prevent checking fmt again?
> Does make the text_len variable special? In which way?
>
>
> >       /* mark and strip a trailing newline */
> >       if (text_len && text[text_len-1] == '\n') {
> > @@ -1972,6 +1977,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> >       boot_delay_msec(level);
> >       printk_delay();
> >
> > +     kmsan_handle_vprintk(&fmt, args);
>
> What does this function, please?
> Could I find more details in another patch?
>
> >       /* This stops the holder of console_sem just where we want him */
> >       logbuf_lock_irqsave(flags);
> >       curr_log_seq = log_next_seq;
>
> 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