| Message ID | 20200817220212.338670-3-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | -ffreestanding/-fno-builtin-* patches | expand |
On Mon, Aug 17, 2020 at 03:02:10PM -0700, Nick Desaulniers wrote: > This reverts commit 5f074f3e192f10c9fade898b9b3b8812e3d83342. > > Use `-fno-builtin-bcmp` instead. > > The issue with using `-fno-builtin-*` flags was that they were not > retained during an LTO link with LLVM. This was fixed in clang-11 by > https://reviews.llvm.org/D71193 > (0508c994f0b14144041f2cfd3ba9f9a80f03de08), which is also the minimum > supported version of clang for LTO. > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > Makefile | 1 + > include/linux/string.h | 3 --- > lib/string.c | 20 -------------------- > 3 files changed, 1 insertion(+), 23 deletions(-) > > diff --git a/Makefile b/Makefile > index 211a1b6f6478..722ff5864275 100644 > --- a/Makefile > +++ b/Makefile > @@ -964,6 +964,7 @@ endif > # to provide implementations of these routines, then prevent the compiler from > # emitting calls to what will be undefined symbols. > KBUILD_CFLAGS += -fno-builtin-stpcpy > +KBUILD_CFLAGS += -fno-builtin-bcmp I personally think that this hunk should be its own patch before this one then have this patch just be the revert, that way there is no regression across a bisect (if one were to ever occur) and so the revert is a straight 'git revert', rather than have something else mixed in that requires reading the actual changelog text. No objections if you disagree though. > # include additional Makefiles when needed > include-y := scripts/Makefile.extrawarn > diff --git a/include/linux/string.h b/include/linux/string.h > index b1f3894a0a3e..f3bdb74bc230 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -155,9 +155,6 @@ extern void * memscan(void *,int,__kernel_size_t); > #ifndef __HAVE_ARCH_MEMCMP > extern int memcmp(const void *,const void *,__kernel_size_t); > #endif > -#ifndef __HAVE_ARCH_BCMP > -extern int bcmp(const void *,const void *,__kernel_size_t); > -#endif > #ifndef __HAVE_ARCH_MEMCHR > extern void * memchr(const void *,int,__kernel_size_t); > #endif > diff --git a/lib/string.c b/lib/string.c > index 6012c385fb31..69328b8353e1 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -922,26 +922,6 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) > EXPORT_SYMBOL(memcmp); > #endif > > -#ifndef __HAVE_ARCH_BCMP > -/** > - * bcmp - returns 0 if and only if the buffers have identical contents. > - * @a: pointer to first buffer. > - * @b: pointer to second buffer. > - * @len: size of buffers. > - * > - * The sign or magnitude of a non-zero return value has no particular > - * meaning, and architectures may implement their own more efficient bcmp(). So > - * while this particular implementation is a simple (tail) call to memcmp, do > - * not rely on anything but whether the return value is zero or non-zero. > - */ > -#undef bcmp > -int bcmp(const void *a, const void *b, size_t len) > -{ > - return memcmp(a, b, len); > -} > -EXPORT_SYMBOL(bcmp); > -#endif > - > #ifndef __HAVE_ARCH_MEMSCAN > /** > * memscan - Find a character in an area of memory. > -- > 2.28.0.220.ged08abb693-goog > Cheers, Nathan
On Mon, Aug 17, 2020 at 10:44 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Mon, Aug 17, 2020 at 03:02:10PM -0700, Nick Desaulniers wrote: > > This reverts commit 5f074f3e192f10c9fade898b9b3b8812e3d83342. > > > > Use `-fno-builtin-bcmp` instead. > > > > The issue with using `-fno-builtin-*` flags was that they were not > > retained during an LTO link with LLVM. This was fixed in clang-11 by > > https://reviews.llvm.org/D71193 > > (0508c994f0b14144041f2cfd3ba9f9a80f03de08), which is also the minimum > > supported version of clang for LTO. > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > --- > > Makefile | 1 + > > include/linux/string.h | 3 --- > > lib/string.c | 20 -------------------- > > 3 files changed, 1 insertion(+), 23 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 211a1b6f6478..722ff5864275 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -964,6 +964,7 @@ endif > > # to provide implementations of these routines, then prevent the compiler from > > # emitting calls to what will be undefined symbols. > > KBUILD_CFLAGS += -fno-builtin-stpcpy > > +KBUILD_CFLAGS += -fno-builtin-bcmp > > I personally think that this hunk should be its own patch before this > one then have this patch just be the revert, that way there is no > regression across a bisect (if one were to ever occur) and so the revert > is a straight 'git revert', rather than have something else mixed in > that requires reading the actual changelog text. > > No objections if you disagree though. That's a great idea. I considered it before sending, but I think it would be interesting to divorce the KBUILD changes which can be picked up quickly from the latter changes. Will send a V2. > > > # include additional Makefiles when needed > > include-y := scripts/Makefile.extrawarn > > diff --git a/include/linux/string.h b/include/linux/string.h > > index b1f3894a0a3e..f3bdb74bc230 100644 > > --- a/include/linux/string.h > > +++ b/include/linux/string.h > > @@ -155,9 +155,6 @@ extern void * memscan(void *,int,__kernel_size_t); > > #ifndef __HAVE_ARCH_MEMCMP > > extern int memcmp(const void *,const void *,__kernel_size_t); > > #endif > > -#ifndef __HAVE_ARCH_BCMP > > -extern int bcmp(const void *,const void *,__kernel_size_t); > > -#endif > > #ifndef __HAVE_ARCH_MEMCHR > > extern void * memchr(const void *,int,__kernel_size_t); > > #endif > > diff --git a/lib/string.c b/lib/string.c > > index 6012c385fb31..69328b8353e1 100644 > > --- a/lib/string.c > > +++ b/lib/string.c > > @@ -922,26 +922,6 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) > > EXPORT_SYMBOL(memcmp); > > #endif > > > > -#ifndef __HAVE_ARCH_BCMP > > -/** > > - * bcmp - returns 0 if and only if the buffers have identical contents. > > - * @a: pointer to first buffer. > > - * @b: pointer to second buffer. > > - * @len: size of buffers. > > - * > > - * The sign or magnitude of a non-zero return value has no particular > > - * meaning, and architectures may implement their own more efficient bcmp(). So > > - * while this particular implementation is a simple (tail) call to memcmp, do > > - * not rely on anything but whether the return value is zero or non-zero. > > - */ > > -#undef bcmp > > -int bcmp(const void *a, const void *b, size_t len) > > -{ > > - return memcmp(a, b, len); > > -} > > -EXPORT_SYMBOL(bcmp); > > -#endif > > - > > #ifndef __HAVE_ARCH_MEMSCAN > > /** > > * memscan - Find a character in an area of memory. > > -- > > 2.28.0.220.ged08abb693-goog > > > > Cheers, > Nathan
On Tue, Aug 18, 2020 at 11:00:01AM -0700, Nick Desaulniers wrote: > On Mon, Aug 17, 2020 at 10:44 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Mon, Aug 17, 2020 at 03:02:10PM -0700, Nick Desaulniers wrote: > > > This reverts commit 5f074f3e192f10c9fade898b9b3b8812e3d83342. > > > > > > Use `-fno-builtin-bcmp` instead. > > > > > > The issue with using `-fno-builtin-*` flags was that they were not > > > retained during an LTO link with LLVM. This was fixed in clang-11 by > > > https://reviews.llvm.org/D71193 > > > (0508c994f0b14144041f2cfd3ba9f9a80f03de08), which is also the minimum > > > supported version of clang for LTO. > > > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > --- > > > Makefile | 1 + > > > include/linux/string.h | 3 --- > > > lib/string.c | 20 -------------------- > > > 3 files changed, 1 insertion(+), 23 deletions(-) > > > > > > diff --git a/Makefile b/Makefile > > > index 211a1b6f6478..722ff5864275 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -964,6 +964,7 @@ endif > > > # to provide implementations of these routines, then prevent the compiler from > > > # emitting calls to what will be undefined symbols. > > > KBUILD_CFLAGS += -fno-builtin-stpcpy > > > +KBUILD_CFLAGS += -fno-builtin-bcmp > > > > I personally think that this hunk should be its own patch before this > > one then have this patch just be the revert, that way there is no > > regression across a bisect (if one were to ever occur) and so the revert > > is a straight 'git revert', rather than have something else mixed in > > that requires reading the actual changelog text. > > > > No objections if you disagree though. > > That's a great idea. I considered it before sending, but I think it > would be interesting to divorce the KBUILD changes which can be picked > up quickly from the latter changes. Will send a V2. Yeah, I had the same thoughts as Nathan. With that change: Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > > > > > > # include additional Makefiles when needed > > > include-y := scripts/Makefile.extrawarn > > > diff --git a/include/linux/string.h b/include/linux/string.h > > > index b1f3894a0a3e..f3bdb74bc230 100644 > > > --- a/include/linux/string.h > > > +++ b/include/linux/string.h > > > @@ -155,9 +155,6 @@ extern void * memscan(void *,int,__kernel_size_t); > > > #ifndef __HAVE_ARCH_MEMCMP > > > extern int memcmp(const void *,const void *,__kernel_size_t); > > > #endif > > > -#ifndef __HAVE_ARCH_BCMP > > > -extern int bcmp(const void *,const void *,__kernel_size_t); > > > -#endif > > > #ifndef __HAVE_ARCH_MEMCHR > > > extern void * memchr(const void *,int,__kernel_size_t); > > > #endif > > > diff --git a/lib/string.c b/lib/string.c > > > index 6012c385fb31..69328b8353e1 100644 > > > --- a/lib/string.c > > > +++ b/lib/string.c > > > @@ -922,26 +922,6 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) > > > EXPORT_SYMBOL(memcmp); > > > #endif > > > > > > -#ifndef __HAVE_ARCH_BCMP > > > -/** > > > - * bcmp - returns 0 if and only if the buffers have identical contents. > > > - * @a: pointer to first buffer. > > > - * @b: pointer to second buffer. > > > - * @len: size of buffers. > > > - * > > > - * The sign or magnitude of a non-zero return value has no particular > > > - * meaning, and architectures may implement their own more efficient bcmp(). So > > > - * while this particular implementation is a simple (tail) call to memcmp, do > > > - * not rely on anything but whether the return value is zero or non-zero. > > > - */ > > > -#undef bcmp > > > -int bcmp(const void *a, const void *b, size_t len) > > > -{ > > > - return memcmp(a, b, len); > > > -} > > > -EXPORT_SYMBOL(bcmp); > > > -#endif > > > - > > > #ifndef __HAVE_ARCH_MEMSCAN > > > /** > > > * memscan - Find a character in an area of memory. > > > -- > > > 2.28.0.220.ged08abb693-goog > > > > > > > Cheers, > > Nathan > > > > -- > Thanks, > ~Nick Desaulniers
diff --git a/Makefile b/Makefile index 211a1b6f6478..722ff5864275 100644 --- a/Makefile +++ b/Makefile @@ -964,6 +964,7 @@ endif # to provide implementations of these routines, then prevent the compiler from # emitting calls to what will be undefined symbols. KBUILD_CFLAGS += -fno-builtin-stpcpy +KBUILD_CFLAGS += -fno-builtin-bcmp # include additional Makefiles when needed include-y := scripts/Makefile.extrawarn diff --git a/include/linux/string.h b/include/linux/string.h index b1f3894a0a3e..f3bdb74bc230 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -155,9 +155,6 @@ extern void * memscan(void *,int,__kernel_size_t); #ifndef __HAVE_ARCH_MEMCMP extern int memcmp(const void *,const void *,__kernel_size_t); #endif -#ifndef __HAVE_ARCH_BCMP -extern int bcmp(const void *,const void *,__kernel_size_t); -#endif #ifndef __HAVE_ARCH_MEMCHR extern void * memchr(const void *,int,__kernel_size_t); #endif diff --git a/lib/string.c b/lib/string.c index 6012c385fb31..69328b8353e1 100644 --- a/lib/string.c +++ b/lib/string.c @@ -922,26 +922,6 @@ __visible int memcmp(const void *cs, const void *ct, size_t count) EXPORT_SYMBOL(memcmp); #endif -#ifndef __HAVE_ARCH_BCMP -/** - * bcmp - returns 0 if and only if the buffers have identical contents. - * @a: pointer to first buffer. - * @b: pointer to second buffer. - * @len: size of buffers. - * - * The sign or magnitude of a non-zero return value has no particular - * meaning, and architectures may implement their own more efficient bcmp(). So - * while this particular implementation is a simple (tail) call to memcmp, do - * not rely on anything but whether the return value is zero or non-zero. - */ -#undef bcmp -int bcmp(const void *a, const void *b, size_t len) -{ - return memcmp(a, b, len); -} -EXPORT_SYMBOL(bcmp); -#endif - #ifndef __HAVE_ARCH_MEMSCAN /** * memscan - Find a character in an area of memory.
This reverts commit 5f074f3e192f10c9fade898b9b3b8812e3d83342. Use `-fno-builtin-bcmp` instead. The issue with using `-fno-builtin-*` flags was that they were not retained during an LTO link with LLVM. This was fixed in clang-11 by https://reviews.llvm.org/D71193 (0508c994f0b14144041f2cfd3ba9f9a80f03de08), which is also the minimum supported version of clang for LTO. Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Makefile | 1 + include/linux/string.h | 3 --- lib/string.c | 20 -------------------- 3 files changed, 1 insertion(+), 23 deletions(-)