| Message ID | 20191122112621.204798-32-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add KernelMemorySanitizer infrastructure | expand |
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? > + * 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. > +#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 >
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. > > +#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 > >
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. > > > > +#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
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? Re new patch: I think here it should be trivial to use 'if (IS_ENABLED(CONFIG_KMSAN))' instead of #ifdef. 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
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
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 +/** + * Disable the efficient 8-byte reading under KMSAN to avoid false positives. + */ +#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.
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(-)