Re: [PATCH RFC v3 31/36] kmsan: disable strscpy() optimization under KMSAN

[Alexander Potapenko <glider@google.com>]

On Tue, Dec 3, 2019 at 12:25 PM Marco Elver <elver@google.com> wrote:
>
> On Tue, 3 Dec 2019 at 12:19, Alexander Potapenko <glider@google.com> wrote:
> >
> > On Mon, Dec 2, 2019 at 5:23 PM Alexander Potapenko <glider@google.com> wrote:
> > >
> > > On Mon, Dec 2, 2019 at 4:51 PM Marco Elver <elver@google.com> wrote:
> > > >
> > > > On Fri, 22 Nov 2019 at 12:28, <glider@google.com> wrote:
> > > > >
> > > > > Disable the efficient 8-byte reading under KMSAN to avoid false positives.
> > > > >
> > > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > > To: Alexander Potapenko <glider@google.com>
> > > > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > > Cc: linux-mm@kvack.org
> > > > >
> > > > > ---
> > > > >
> > > > > Change-Id: I25d1acf5c3df6eff85894cd94f5ddbe93308271c
> > > > > ---
> > > > >  lib/string.c | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/lib/string.c b/lib/string.c
> > > > > index 08ec58cc673b..15efdc51bda6 100644
> > > > > --- a/lib/string.c
> > > > > +++ b/lib/string.c
> > > > > @@ -186,7 +186,10 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
> > > > >         if (count == 0 || WARN_ON_ONCE(count > INT_MAX))
> > > > >                 return -E2BIG;
> > > > >
> > > > > -#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> > > > > +/**
> > > >
> > > > Why a doc comment?
> > > Will fix, thanks!
> > > > > + * Disable the efficient 8-byte reading under KMSAN to avoid false positives.
> > > > > + */
> > > >
> > > > AFAIK the CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS case is about
> > > > unaligned accesses crossing page boundaries. In the #else case it's
> > > > still going to do word-at-a-time if both src and dest are aligned, so
> > > > the comment above is somewhat inaccurate.
> > > Yes, this makes little sense.
> > > Reading word-at-a-time shouldn't induce any errors, although it may
> > > generate redundant stack IDs for values that will never be used.
> > > I'll try to drop this patch.
> > Turns out the patch is still needed, as read_word_at_a_time may read
> > uninitialized bytes which are then used in comparisons.
> > I've changed the patch to always set max=0 under KMSAN:
> > https://github.com/google/kmsan/commit/3ff43863bf53dd871a3d4dc4fbb2a76d79b4db4f
> > Will include this version in v4 series.
>
> So was the previous version working because all strscpy where this was
> relevant were unaligned?
Right.
> Re new patch: I think here it should be trivial to use 'if
> (IS_ENABLED(CONFIG_KMSAN))' instead of #ifdef.
Yes, sorry, I always forget about IS_ENABLED.
> Thanks,
> -- Marco
>
> > > > > +#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && !defined(CONFIG_KMSAN)
> > > > >         /*
> > > > >          * If src is unaligned, don't cross a page boundary,
> > > > >          * since we don't know if the next page is mapped.
> > > > > --
> > > > > 2.24.0.432.g9d3f5f5b63-goog
> > > > >
> > >
> > >
> > >
> > > --
> > > 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



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