| Message ID | 20220130183653.491292-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Commit | 05a5ff1e0c9581e4ceb8cdc2cbcce5ed0e9fa2b7 |
| Headers | show |
| Series | lib/test_string.c: Add test for strlen() | expand |
On Sun, Jan 30, 2022 at 8:36 PM Kees Cook <keescook@chromium.org> wrote: > > Add a simple test for strlen() functionality, including using it as a > constant expression. ... > +/* > + * Unlike many other string functions, strlen() can be used in > + * static initializers when string lengths are known at compile > + * time. (i.e. Under these conditions, strlen() is a constant > + * expression.) Make sure it can be used this way. > + */ > +static const int strlen_ce = strlen("tada, a constant expression"); So, the compiler will replace this by a constant and then eliminate the condition completely from the code. Did I understand this correctly? > +static __init int strlen_selftest(void) > +{ > + /* String length ruler: 123456789012345 */ > + static const char normal[] = "I am normal"; > + static const char *ptr = "where do I go?"; > + static const char trailing[] = "hidden NULLs\0\0\0"; > + static const char leading[] = "\0\0hidden text"; > + > + if (strlen(normal) != 11) > + return 0x100001; > + if (strlen(ptr++) != 14) > + return 0x100002; > + if (strlen(ptr++) != 13) > + return 0x100003; > + if (strlen(trailing) != 12) > + return 0x100004; > + if (strlen(leading) != 0) > + return 0x100005; > + if (strlen_ce != 27) > + return 0x100006; ...so this part won't ever appear in the assembly (assuming -O2). Same to the rest? If so, why is this not a part of the compiler tests? > + return 0; > +}
Hi Kees,
I love your patch! Yet something to improve:
[auto build test ERROR on linux/master]
[also build test ERROR on linus/master kees/for-next/pstore v5.17-rc2 next-20220128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220131/202201310303.HQlCsvvd-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/d22fe883339c5141953dbca980b51466ac3e2329
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726
git checkout d22fe883339c5141953dbca980b51466ac3e2329
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash M=lib/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> lib/test_string.c:188:30: error: initializer element is not constant
188 | static const int strlen_ce = strlen("tada, a constant expression");
| ^~~~~~
vim +188 lib/test_string.c
181
182 /*
183 * Unlike many other string functions, strlen() can be used in
184 * static initializers when string lengths are known at compile
185 * time. (i.e. Under these conditions, strlen() is a constant
186 * expression.) Make sure it can be used this way.
187 */
> 188 static const int strlen_ce = strlen("tada, a constant expression");
189
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Sun, Jan 30, 2022 at 08:56:40PM +0200, Andy Shevchenko wrote: > On Sun, Jan 30, 2022 at 8:36 PM Kees Cook <keescook@chromium.org> wrote: > > > > Add a simple test for strlen() functionality, including using it as a > > constant expression. > > ... > > > +/* > > + * Unlike many other string functions, strlen() can be used in > > + * static initializers when string lengths are known at compile > > + * time. (i.e. Under these conditions, strlen() is a constant > > + * expression.) Make sure it can be used this way. > > + */ > > +static const int strlen_ce = strlen("tada, a constant expression"); > > So, the compiler will replace this by a constant and then eliminate > the condition completely from the code. Did I understand this > correctly? Yup! See: https://godbolt.org/z/nTqPaszTh There a few rare places in the kernel that do this, which is how I noticed. (I broke strlen() with the recent FORTIFY changes.) > > +static __init int strlen_selftest(void) > > +{ > > + /* String length ruler: 123456789012345 */ > > + static const char normal[] = "I am normal"; > > + static const char *ptr = "where do I go?"; > > + static const char trailing[] = "hidden NULLs\0\0\0"; > > + static const char leading[] = "\0\0hidden text"; > > + > > + if (strlen(normal) != 11) > > + return 0x100001; > > + if (strlen(ptr++) != 14) > > + return 0x100002; > > + if (strlen(ptr++) != 13) > > + return 0x100003; > > + if (strlen(trailing) != 12) > > + return 0x100004; > > + if (strlen(leading) != 0) > > + return 0x100005; > > > + if (strlen_ce != 27) > > + return 0x100006; > > ...so this part won't ever appear in the assembly (assuming -O2). Correct, unless strlen() breaks. > Same to the rest? If so, why is this not a part of the compiler tests? I wanted to keep everything together -- this includes a macro side-effect test as well ("ptr++"). -Kees
On Mon, Jan 31, 2022 at 04:13:30AM +0800, kernel test robot wrote: > Hi Kees, > > I love your patch! Yet something to improve: > > [auto build test ERROR on linux/master] > [also build test ERROR on linus/master kees/for-next/pstore v5.17-rc2 next-20220128] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726 > base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39 > config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220131/202201310303.HQlCsvvd-lkp@intel.com/config) > compiler: sh4-linux-gcc (GCC) 11.2.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/0day-ci/linux/commit/d22fe883339c5141953dbca980b51466ac3e2329 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Kees-Cook/lib-test_string-c-Add-test-for-strlen/20220131-023726 > git checkout d22fe883339c5141953dbca980b51466ac3e2329 > # save the config file to linux build tree > mkdir build_dir > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash M=lib/ > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > >> lib/test_string.c:188:30: error: initializer element is not constant > 188 | static const int strlen_ce = strlen("tada, a constant expression"); > | ^~~~~~ Thanks; this is expected -- the strlen() in -next is currently broken (and will be fixed in the next -next). -Kees > > > vim +188 lib/test_string.c > > 181 > 182 /* > 183 * Unlike many other string functions, strlen() can be used in > 184 * static initializers when string lengths are known at compile > 185 * time. (i.e. Under these conditions, strlen() is a constant > 186 * expression.) Make sure it can be used this way. > 187 */ > > 188 static const int strlen_ce = strlen("tada, a constant expression"); > 189 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Sun, Jan 30, 2022 at 10:36:53AM -0800, Kees Cook wrote: > Add a simple test for strlen() functionality, including using it as a > constant expression. > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Peter Rosin <peda@axentia.se> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > I'll be taking this as part of my Clang FORTIFY_SOURCE series. > --- > lib/test_string.c | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/lib/test_string.c b/lib/test_string.c > index 9dfd6f52de92..59994f552c48 100644 > --- a/lib/test_string.c > +++ b/lib/test_string.c > @@ -179,6 +179,38 @@ static __init int strnchr_selftest(void) > return 0; > } > > +/* > + * Unlike many other string functions, strlen() can be used in > + * static initializers when string lengths are known at compile > + * time. (i.e. Under these conditions, strlen() is a constant > + * expression.) Make sure it can be used this way. > + */ > +static const int strlen_ce = strlen("tada, a constant expression"); > + This results in: lib/test_string.c:188:30: error: initializer element is not constant 188 | static const int strlen_ce = strlen("tada, a constant expression"); for several of my tests. I don't think you can mandate that a compiler implements this. Guenter > +static __init int strlen_selftest(void) > +{ > + /* String length ruler: 123456789012345 */ > + static const char normal[] = "I am normal"; > + static const char *ptr = "where do I go?"; > + static const char trailing[] = "hidden NULLs\0\0\0"; > + static const char leading[] = "\0\0hidden text"; > + > + if (strlen(normal) != 11) > + return 0x100001; > + if (strlen(ptr++) != 14) > + return 0x100002; > + if (strlen(ptr++) != 13) > + return 0x100003; > + if (strlen(trailing) != 12) > + return 0x100004; > + if (strlen(leading) != 0) > + return 0x100005; > + if (strlen_ce != 27) > + return 0x100006; > + > + return 0; > +} > + > static __exit void string_selftest_remove(void) > { > } > @@ -212,6 +244,11 @@ static __init int string_selftest_init(void) > if (subtest) > goto fail; > > + test = 5; > + subtest = strlen_selftest(); > + if (subtest) > + goto fail; > + > pr_info("String selftests succeeded\n"); > return 0; > fail: > -- > 2.30.2 >
On Wed, Feb 02, 2022 at 08:01:49AM -0800, Guenter Roeck wrote: > On Sun, Jan 30, 2022 at 10:36:53AM -0800, Kees Cook wrote: ... > > +static const int strlen_ce = strlen("tada, a constant expression"); > > This results in: > > lib/test_string.c:188:30: error: initializer element is not constant > 188 | static const int strlen_ce = strlen("tada, a constant expression"); > > for several of my tests. I don't think you can mandate that a compiler > implements this. With -O2 probably one can, otherwise I agree.
On Wed, Feb 02, 2022 at 08:01:49AM -0800, Guenter Roeck wrote: > On Sun, Jan 30, 2022 at 10:36:53AM -0800, Kees Cook wrote: > > Add a simple test for strlen() functionality, including using it as a > > constant expression. > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Cc: Peter Rosin <peda@axentia.se> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > I'll be taking this as part of my Clang FORTIFY_SOURCE series. > > --- > > lib/test_string.c | 37 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/lib/test_string.c b/lib/test_string.c > > index 9dfd6f52de92..59994f552c48 100644 > > --- a/lib/test_string.c > > +++ b/lib/test_string.c > > @@ -179,6 +179,38 @@ static __init int strnchr_selftest(void) > > return 0; > > } > > > > +/* > > + * Unlike many other string functions, strlen() can be used in > > + * static initializers when string lengths are known at compile > > + * time. (i.e. Under these conditions, strlen() is a constant > > + * expression.) Make sure it can be used this way. > > + */ > > +static const int strlen_ce = strlen("tada, a constant expression"); > > + > > This results in: > > lib/test_string.c:188:30: error: initializer element is not constant > 188 | static const int strlen_ce = strlen("tada, a constant expression"); > > for several of my tests. I don't think you can mandate that a compiler > implements this. Which tests? This property of strlen() is already required by our builds (this is how I tripped over it). For example: drivers/firmware/xilinx/zynqmp-debug.c: #define PM_API(id) {id, #id, strlen(#id)} static struct pm_api_info pm_api_list[] = { PM_API(PM_GET_API_VERSION), PM_API(PM_QUERY_DATA), }; > > Guenter > > > +static __init int strlen_selftest(void) > > +{ > > + /* String length ruler: 123456789012345 */ > > + static const char normal[] = "I am normal"; > > + static const char *ptr = "where do I go?"; > > + static const char trailing[] = "hidden NULLs\0\0\0"; > > + static const char leading[] = "\0\0hidden text"; > > + > > + if (strlen(normal) != 11) > > + return 0x100001; > > + if (strlen(ptr++) != 14) > > + return 0x100002; > > + if (strlen(ptr++) != 13) > > + return 0x100003; > > + if (strlen(trailing) != 12) > > + return 0x100004; > > + if (strlen(leading) != 0) > > + return 0x100005; > > + if (strlen_ce != 27) > > + return 0x100006; > > + > > + return 0; > > +} > > + > > static __exit void string_selftest_remove(void) > > { > > } > > @@ -212,6 +244,11 @@ static __init int string_selftest_init(void) > > if (subtest) > > goto fail; > > > > + test = 5; > > + subtest = strlen_selftest(); > > + if (subtest) > > + goto fail; > > + > > pr_info("String selftests succeeded\n"); > > return 0; > > fail: > > -- > > 2.30.2 > >
On 2/2/22 12:52, Kees Cook wrote: > On Wed, Feb 02, 2022 at 08:01:49AM -0800, Guenter Roeck wrote: >> On Sun, Jan 30, 2022 at 10:36:53AM -0800, Kees Cook wrote: >>> Add a simple test for strlen() functionality, including using it as a >>> constant expression. >>> >>> Cc: Geert Uytterhoeven <geert@linux-m68k.org> >>> Cc: Peter Rosin <peda@axentia.se> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> I'll be taking this as part of my Clang FORTIFY_SOURCE series. >>> --- >>> lib/test_string.c | 37 +++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 37 insertions(+) >>> >>> diff --git a/lib/test_string.c b/lib/test_string.c >>> index 9dfd6f52de92..59994f552c48 100644 >>> --- a/lib/test_string.c >>> +++ b/lib/test_string.c >>> @@ -179,6 +179,38 @@ static __init int strnchr_selftest(void) >>> return 0; >>> } >>> >>> +/* >>> + * Unlike many other string functions, strlen() can be used in >>> + * static initializers when string lengths are known at compile >>> + * time. (i.e. Under these conditions, strlen() is a constant >>> + * expression.) Make sure it can be used this way. >>> + */ >>> +static const int strlen_ce = strlen("tada, a constant expression"); >>> + >> >> This results in: >> >> lib/test_string.c:188:30: error: initializer element is not constant >> 188 | static const int strlen_ce = strlen("tada, a constant expression"); >> >> for several of my tests. I don't think you can mandate that a compiler >> implements this. > > Which tests? > Some examples: Build reference: next-20220202 Compiler version: m68k-linux-gcc (GCC) 11.2.0 Building m68k:defconfig ... failed -------------- Error log: lib/test_string.c:188:30: error: initializer element is not constant 188 | static const int strlen_ce = strlen("tada, a constant expression"); Building mips:malta_defconfig:nocd:smp:net,e1000:initrd ... failed ------------ Error log: lib/test_string.c:188:30: error: initializer element is not constant static const int strlen_ce = strlen("tada, a constant expression"); Building i386:q35:Broadwell:defconfig:smp:ata:net,rtl8139:hd ... failed ------------ Error log: lib/test_string.c:188:30: error: initializer element is not constant 188 | static const int strlen_ce = strlen("tada, a constant expression"); i386 and is defconfig + CONFIG_STRING_SELFTEST=y; mips is malta_defconfig + CONFIG_STRING_SELFTEST=y. All use gcc 11.2. There may be more, but there are so many failures in -next right now that I may be missing some. > This property of strlen() is already required by our builds (this is how > I tripped over it). For example: > > drivers/firmware/xilinx/zynqmp-debug.c: > > #define PM_API(id) {id, #id, strlen(#id)} > static struct pm_api_info pm_api_list[] = { > PM_API(PM_GET_API_VERSION), > PM_API(PM_QUERY_DATA), > }; I do not think that it is a C standard that strlen() on a constant string must be compile-time evaluated and result in a constant. Anyway, key difference, I think, is the presence of an architecture-specific version of strlen(), or the maybe non-presence of __HAVE_ARCH_STRLEN, or the definition of strlen() in include/linux/fortify-string.h. Guenter
Hi Günter, On Thu, Feb 3, 2022 at 12:12 AM Guenter Roeck <linux@roeck-us.net> wrote: > On 2/2/22 12:52, Kees Cook wrote: > > On Wed, Feb 02, 2022 at 08:01:49AM -0800, Guenter Roeck wrote: > >> On Sun, Jan 30, 2022 at 10:36:53AM -0800, Kees Cook wrote: > >>> Add a simple test for strlen() functionality, including using it as a > >>> constant expression. > >>> > >>> Cc: Geert Uytterhoeven <geert@linux-m68k.org> > >>> Cc: Peter Rosin <peda@axentia.se> > >>> Signed-off-by: Kees Cook <keescook@chromium.org> > >>> --- > >>> I'll be taking this as part of my Clang FORTIFY_SOURCE series. > >>> --- > >>> lib/test_string.c | 37 +++++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 37 insertions(+) > >>> > >>> diff --git a/lib/test_string.c b/lib/test_string.c > >>> index 9dfd6f52de92..59994f552c48 100644 > >>> --- a/lib/test_string.c > >>> +++ b/lib/test_string.c > >>> @@ -179,6 +179,38 @@ static __init int strnchr_selftest(void) > >>> return 0; > >>> } > >>> > >>> +/* > >>> + * Unlike many other string functions, strlen() can be used in > >>> + * static initializers when string lengths are known at compile > >>> + * time. (i.e. Under these conditions, strlen() is a constant > >>> + * expression.) Make sure it can be used this way. > >>> + */ > >>> +static const int strlen_ce = strlen("tada, a constant expression"); > >>> + > >> > >> This results in: > >> > >> lib/test_string.c:188:30: error: initializer element is not constant > >> 188 | static const int strlen_ce = strlen("tada, a constant expression"); > >> > >> for several of my tests. I don't think you can mandate that a compiler > >> implements this. > > > > Which tests? > > > > Some examples: > > Build reference: next-20220202 > Compiler version: m68k-linux-gcc (GCC) 11.2.0 > > Building m68k:defconfig ... failed > -------------- > Error log: > lib/test_string.c:188:30: error: initializer element is not constant > 188 | static const int strlen_ce = strlen("tada, a constant expression"); > > Building mips:malta_defconfig:nocd:smp:net,e1000:initrd ... failed > ------------ > Error log: > lib/test_string.c:188:30: error: initializer element is not constant > static const int strlen_ce = strlen("tada, a constant expression"); > > Building i386:q35:Broadwell:defconfig:smp:ata:net,rtl8139:hd ... failed > ------------ > Error log: > lib/test_string.c:188:30: error: initializer element is not constant > 188 | static const int strlen_ce = strlen("tada, a constant expression"); > > i386 and is defconfig + CONFIG_STRING_SELFTEST=y; mips is > malta_defconfig + CONFIG_STRING_SELFTEST=y. All use gcc 11.2. > > There may be more, but there are so many failures in -next right now > that I may be missing some. > > > This property of strlen() is already required by our builds (this is how > > I tripped over it). For example: > > > > drivers/firmware/xilinx/zynqmp-debug.c: > > > > #define PM_API(id) {id, #id, strlen(#id)} > > static struct pm_api_info pm_api_list[] = { > > PM_API(PM_GET_API_VERSION), > > PM_API(PM_QUERY_DATA), > > }; > > I do not think that it is a C standard that strlen() on a constant string > must be compile-time evaluated and result in a constant. Not if -ffreestanding, which is what several architectures are using nowadays, to a.o. prevent gcc from replacing calls to stdlib functions to other stdlib functions (e.g. strncat() -> strlen() + store, strncmp() -> strcmp()), which breaks linking if the latter is only provided inline. > Anyway, key difference, I think, is the presence of an architecture-specific > version of strlen(), or the maybe non-presence of __HAVE_ARCH_STRLEN, > or the definition of strlen() in include/linux/fortify-string.h. It works after dropping -ffreestanding. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote: > Hi Günter, > > On Thu, Feb 3, 2022 at 12:12 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 2/2/22 12:52, Kees Cook wrote: > > > On Wed, Feb 02, 2022 at 08:01:49AM -0800, Guenter Roeck wrote: > > >> On Sun, Jan 30, 2022 at 10:36:53AM -0800, Kees Cook wrote: > > >>> Add a simple test for strlen() functionality, including using it as a > > >>> constant expression. > > >>> > > >>> Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > >>> Cc: Peter Rosin <peda@axentia.se> > > >>> Signed-off-by: Kees Cook <keescook@chromium.org> > > >>> --- > > >>> I'll be taking this as part of my Clang FORTIFY_SOURCE series. > > >>> --- > > >>> lib/test_string.c | 37 +++++++++++++++++++++++++++++++++++++ > > >>> 1 file changed, 37 insertions(+) > > >>> > > >>> diff --git a/lib/test_string.c b/lib/test_string.c > > >>> index 9dfd6f52de92..59994f552c48 100644 > > >>> --- a/lib/test_string.c > > >>> +++ b/lib/test_string.c > > >>> @@ -179,6 +179,38 @@ static __init int strnchr_selftest(void) > > >>> return 0; > > >>> } > > >>> > > >>> +/* > > >>> + * Unlike many other string functions, strlen() can be used in > > >>> + * static initializers when string lengths are known at compile > > >>> + * time. (i.e. Under these conditions, strlen() is a constant > > >>> + * expression.) Make sure it can be used this way. > > >>> + */ > > >>> +static const int strlen_ce = strlen("tada, a constant expression"); > > >>> + > > >> > > >> This results in: > > >> > > >> lib/test_string.c:188:30: error: initializer element is not constant > > >> 188 | static const int strlen_ce = strlen("tada, a constant expression"); > > >> > > >> for several of my tests. I don't think you can mandate that a compiler > > >> implements this. > > > > > > Which tests? > > > > > > > Some examples: > > > > Build reference: next-20220202 > > Compiler version: m68k-linux-gcc (GCC) 11.2.0 > > > > Building m68k:defconfig ... failed > > -------------- > > Error log: > > lib/test_string.c:188:30: error: initializer element is not constant > > 188 | static const int strlen_ce = strlen("tada, a constant expression"); > > > > Building mips:malta_defconfig:nocd:smp:net,e1000:initrd ... failed > > ------------ > > Error log: > > lib/test_string.c:188:30: error: initializer element is not constant > > static const int strlen_ce = strlen("tada, a constant expression"); > > > > Building i386:q35:Broadwell:defconfig:smp:ata:net,rtl8139:hd ... failed > > ------------ > > Error log: > > lib/test_string.c:188:30: error: initializer element is not constant > > 188 | static const int strlen_ce = strlen("tada, a constant expression"); > > > > i386 and is defconfig + CONFIG_STRING_SELFTEST=y; mips is > > malta_defconfig + CONFIG_STRING_SELFTEST=y. All use gcc 11.2. > > > > There may be more, but there are so many failures in -next right now > > that I may be missing some. > > > > > This property of strlen() is already required by our builds (this is how > > > I tripped over it). For example: > > > > > > drivers/firmware/xilinx/zynqmp-debug.c: > > > > > > #define PM_API(id) {id, #id, strlen(#id)} > > > static struct pm_api_info pm_api_list[] = { > > > PM_API(PM_GET_API_VERSION), > > > PM_API(PM_QUERY_DATA), > > > }; > > > > I do not think that it is a C standard that strlen() on a constant string > > must be compile-time evaluated and result in a constant. > > Not if -ffreestanding, which is what several architectures are > using nowadays, to a.o. prevent gcc from replacing calls to stdlib > functions to other stdlib functions (e.g. strncat() -> strlen() + > store, strncmp() -> strcmp()), which breaks linking if the latter is > only provided inline. > > > Anyway, key difference, I think, is the presence of an architecture-specific > > version of strlen(), or the maybe non-presence of __HAVE_ARCH_STRLEN, > > or the definition of strlen() in include/linux/fortify-string.h. > > It works after dropping -ffreestanding. Ah-ha, thanks for the clue here. I'll see if there is some way to detect this (or, I guess, drop the patch). I don't like that this requirement depends on the architecture, so I'd rather it behave the same everywhere. Hmm
On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote: > Not if -ffreestanding, which is what several architectures are > using nowadays, to a.o. prevent gcc from replacing calls to stdlib > functions to other stdlib functions (e.g. strncat() -> strlen() + > store, strncmp() -> strcmp()), which breaks linking if the latter is > only provided inline. Hah, for i386: arch/x86/Makefile # temporary until string.h is fixed KBUILD_CFLAGS += -ffreestanding This "temporary" is from 2006. ;) 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin") Removing that appears to solve it, and appears to build correctly. I'll continue testing. > It works after dropping -ffreestanding. I wonder if the other architectures were just copying x86?
Hi Kees, On Thu, Feb 3, 2022 at 6:15 PM Kees Cook <keescook@chromium.org> wrote: > On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote: > > Not if -ffreestanding, which is what several architectures are > > using nowadays, to a.o. prevent gcc from replacing calls to stdlib > > functions to other stdlib functions (e.g. strncat() -> strlen() + > > store, strncmp() -> strcmp()), which breaks linking if the latter is > > only provided inline. > > Hah, for i386: > > arch/x86/Makefile > # temporary until string.h is fixed > KBUILD_CFLAGS += -ffreestanding > > This "temporary" is from 2006. ;) And before that, we had it in the main Makefile. > 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin") > > Removing that appears to solve it, and appears to build correctly. I'll > continue testing. > > > It works after dropping -ffreestanding. > > I wonder if the other architectures were just copying x86? At least on m68k it was added because gcc added new optimizations that broke if an architecture provides some functions as inline. As the kernel is not supposed to be linked with the standard C library, -ffreestanding should be correct, isn't it? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Feb 3, 2022 at 10:10 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Kees, > > On Thu, Feb 3, 2022 at 6:15 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote: > > > Not if -ffreestanding, which is what several architectures are > > > using nowadays, to a.o. prevent gcc from replacing calls to stdlib > > > functions to other stdlib functions (e.g. strncat() -> strlen() + > > > store, strncmp() -> strcmp()), which breaks linking if the latter is > > > only provided inline. > > > > Hah, for i386: > > > > arch/x86/Makefile > > # temporary until string.h is fixed > > KBUILD_CFLAGS += -ffreestanding > > > > This "temporary" is from 2006. ;)\ IIRC I sent a patch removing that. Yeah, Kees even signed off on it. https://lore.kernel.org/lkml/20200817220212.338670-5-ndesaulniers@google.com/#t I still think that's the right way to go, perhaps worth a resend to rekick discussions. > > And before that, we had it in the main Makefile. > > > 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin") > > > > Removing that appears to solve it, and appears to build correctly. I'll > > continue testing. > > > > > It works after dropping -ffreestanding. > > > > I wonder if the other architectures were just copying x86? > > At least on m68k it was added because gcc added new optimizations > that broke if an architecture provides some functions as inline. > As the kernel is not supposed to be linked with the standard C > library, -ffreestanding should be correct, isn't it? The kernel does not link against a libc; but it does provide many symbols that libc would provide, with the same or similar enough semantics that I would strongly recommend we _don't_ use -ffreestanding in order to get such libcall optimizations (there are a lot; see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp for some examples) and simply use -fno-builtin-* when necessary, or fix the kernel implementations individually.
On Thu, Feb 03, 2022 at 11:50:34AM -0800, Nick Desaulniers wrote: > On Thu, Feb 3, 2022 at 10:10 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > Hi Kees, > > > > On Thu, Feb 3, 2022 at 6:15 PM Kees Cook <keescook@chromium.org> wrote: > > > On Thu, Feb 03, 2022 at 09:04:22AM +0100, Geert Uytterhoeven wrote: > > > > Not if -ffreestanding, which is what several architectures are > > > > using nowadays, to a.o. prevent gcc from replacing calls to stdlib > > > > functions to other stdlib functions (e.g. strncat() -> strlen() + > > > > store, strncmp() -> strcmp()), which breaks linking if the latter is > > > > only provided inline. > > > > > > Hah, for i386: > > > > > > arch/x86/Makefile > > > # temporary until string.h is fixed > > > KBUILD_CFLAGS += -ffreestanding > > > > > > This "temporary" is from 2006. ;)\ > > IIRC I sent a patch removing that. Yeah, Kees even signed off on it. > https://lore.kernel.org/lkml/20200817220212.338670-5-ndesaulniers@google.com/#t > I still think that's the right way to go, perhaps worth a resend to > rekick discussions. Hah. Yay. I'll go pick this into the memcpy topic branch. Building x86_64 and ia32 differently makes no sense (and this solves the head-scratching compile-time test failures I was seeing on ia32 too). > > > > And before that, we had it in the main Makefile. > > > > > 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin") > > > > > > Removing that appears to solve it, and appears to build correctly. I'll > > > continue testing. > > > > > > > It works after dropping -ffreestanding. > > > > > > I wonder if the other architectures were just copying x86? > > > > At least on m68k it was added because gcc added new optimizations > > that broke if an architecture provides some functions as inline. > > As the kernel is not supposed to be linked with the standard C > > library, -ffreestanding should be correct, isn't it? > > The kernel does not link against a libc; but it does provide many > symbols that libc would provide, with the same or similar enough > semantics that I would strongly recommend we _don't_ use > -ffreestanding in order to get such libcall optimizations (there are a > lot; see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp > for some examples) and simply use -fno-builtin-* when necessary, or > fix the kernel implementations individually. Right, so, I think for x86 it's straight forward. For the other architectures it may need more careful checking, so I'm just going to drop the new test from the memcpy topic branch, and maybe start a "-ffreestanding removal" topic branch so there isn't a cross-dependency here.
On Thu, Feb 03, 2022 at 11:50:34AM -0800, Nick Desaulniers wrote: > The kernel does not link against a libc; but it does provide many > symbols that libc would provide, with the same or similar enough > semantics that I would strongly recommend we _don't_ use > -ffreestanding in order to get such libcall optimizations (there are a > lot; see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp > for some examples) and simply use -fno-builtin-* when necessary, or > fix the kernel implementations individually. Right, we really don't want -ffreestanding. Rather, we want to not link against libgcc. This is mostly true already, though some of the smaller architectures still do: $ git grep print-libgcc arch/arc/Makefile:LIBGCC = $(shell $(CC) $(cflags-y) --print-libgcc-file-name) arch/csky/Makefile: $(shell $(CC) $(KBUILD_CFLAGS) $(KCFLAGS) -print-libgcc-file-name) arch/h8300/boot/compressed/Makefile:LIBGCC := $(shell $(CROSS-COMPILE)$(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name 2>/dev/null) arch/nios2/Makefile:LIBGCC := $(shell $(CC) $(KBUILD_CFLAGS) $(KCFLAGS) -print-libgcc-file-name) arch/openrisc/Makefile:LIBGCC := $(shell $(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name) arch/parisc/Makefile:LIBGCC := $(shell $(CC) -print-libgcc-file-name) arch/xtensa/Makefile:LIBGCC := $(shell $(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name) arch/xtensa/boot/boot-redboot/Makefile:LIBGCC := $(shell $(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name)
diff --git a/lib/test_string.c b/lib/test_string.c index 9dfd6f52de92..59994f552c48 100644 --- a/lib/test_string.c +++ b/lib/test_string.c @@ -179,6 +179,38 @@ static __init int strnchr_selftest(void) return 0; } +/* + * Unlike many other string functions, strlen() can be used in + * static initializers when string lengths are known at compile + * time. (i.e. Under these conditions, strlen() is a constant + * expression.) Make sure it can be used this way. + */ +static const int strlen_ce = strlen("tada, a constant expression"); + +static __init int strlen_selftest(void) +{ + /* String length ruler: 123456789012345 */ + static const char normal[] = "I am normal"; + static const char *ptr = "where do I go?"; + static const char trailing[] = "hidden NULLs\0\0\0"; + static const char leading[] = "\0\0hidden text"; + + if (strlen(normal) != 11) + return 0x100001; + if (strlen(ptr++) != 14) + return 0x100002; + if (strlen(ptr++) != 13) + return 0x100003; + if (strlen(trailing) != 12) + return 0x100004; + if (strlen(leading) != 0) + return 0x100005; + if (strlen_ce != 27) + return 0x100006; + + return 0; +} + static __exit void string_selftest_remove(void) { } @@ -212,6 +244,11 @@ static __init int string_selftest_init(void) if (subtest) goto fail; + test = 5; + subtest = strlen_selftest(); + if (subtest) + goto fail; + pr_info("String selftests succeeded\n"); return 0; fail:
Add a simple test for strlen() functionality, including using it as a constant expression. Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Peter Rosin <peda@axentia.se> Signed-off-by: Kees Cook <keescook@chromium.org> --- I'll be taking this as part of my Clang FORTIFY_SOURCE series. --- lib/test_string.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)