| Message ID | 20250108-strlen_use_builtin_constant_p-v1-1-611b52e80a9f@wanadoo.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fortify: turn strlen() into an inline function using __builtin_constant_p() | expand |
On Wed, Jan 08, 2025 at 11:27:51PM +0900, Vincent Mailhol wrote: > The strlen(p) function-like macro uses: > > __is_constexpr(__builtin_strlen(p)) > > in which GCC would only yield true if the argument p is a string > literal. Otherwise, GCC would return false even if p is a const > string. > > In contrary, by using: > > __builtin_constant_p(__builtin_strlen(p)) > > then GCC can also recognizes when p is a compile time constant string. > > The above is illustrated in [1]. > > N.B.: clang is not impacted by any of this and gives the same results > with either __is_constexpr() and __builting_constant_p(). > > Use __builtin_constant_p() instead of __is_constexpr() so that GCC can > do the folding on constant strings. This done, strlen() does not > require any more to be a function-like macro, so turn it into a static > inline function. In the process, __fortify_strlen() had to be moved > above strlen() so that it became visible to strlen(). This is what __compiletime_strlen() ended up doing, so this seems reasonable to me. > On a side note, strlen() did a double expansion of its argument p. It did? Ah, was it due to __is_constexpr() wrapping? The other expressions should have been side-effect free: __builtin_choose_expr(__is_constexpr(__builtin_strlen(p)), \ __builtin_strlen(p), __fortify_strlen(p)) I don't think you build-tested this with Clang, though? CC scripts/mod/devicetable-offsets.s In file included from ../scripts/mod/devicetable-offsets.c:3: In file included from ../include/linux/mod_devicetable.h:14: In file included from ../include/linux/uuid.h:11: In file included from ../include/linux/string.h:389: ../include/linux/fortify-string.h:272:17: error: redeclaration of 'strlen' must not have the 'overloadable' attribute 272 | __kernel_size_t strlen(const char *p) | ^ ../include/linux/string.h:200:24: note: previous unmarked overload of function is here 200 | extern __kernel_size_t strlen(const char *); | ^ The externs will need to be reworked if it's no longer depending on asm renaming. > Turning it into an inline function also resolved this side issue. > > [1] https://godbolt.org/z/rqr3YvoP4 > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > This patch is the successor of patch [1] which was part of a longer > series [2]. Meanwhile, I decided to split it, so I am sending this again, > but as a stand-alone patch. > > Changelog since [1]: use __builtin_constant_p() instead and turn > strlen() into an inline function > > [1] https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-6-4e4cbaecc216@wanadoo.fr/ > [2] https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-0-4e4cbaecc216@wanadoo.fr/ > --- > include/linux/fortify-string.h | 34 +++++++++++++++++++--------------- > 1 file changed, 19 insertions(+), 15 deletions(-) > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h > index e4ce1cae03bf770047ce8a7c032b183683388cd5..bd22dd66e5f5b66ad839df42247e6436e0afd053 100644 > --- a/include/linux/fortify-string.h > +++ b/include/linux/fortify-string.h > @@ -4,7 +4,6 @@ > > #include <linux/bitfield.h> > #include <linux/bug.h> > -#include <linux/const.h> > #include <linux/limits.h> > > #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable > @@ -241,6 +240,21 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size > * possible for strlen() to be used on compile-time strings for use in > * static initializers (i.e. as a constant expression). ^^ This comment, however, is I think what sinks this patch. Please see commit 67ebc3ab4462 ("fortify: Make sure strlen() may still be used as a constant expression") which required that strlen() not be an inline. I'm pretty sure the build will start failing again (though perhaps only on older GCC versions). The lib/test_fortify/ target still successfully builds, so that's good though. :)
On Thu. 9 Jan 2025 at 06:46, Kees Cook <kees@kernel.org> wrote: > On Wed, Jan 08, 2025 at 11:27:51PM +0900, Vincent Mailhol wrote: > > The strlen(p) function-like macro uses: > > > > __is_constexpr(__builtin_strlen(p)) > > > > in which GCC would only yield true if the argument p is a string > > literal. Otherwise, GCC would return false even if p is a const > > string. > > > > In contrary, by using: > > > > __builtin_constant_p(__builtin_strlen(p)) > > > > then GCC can also recognizes when p is a compile time constant string. > > > > The above is illustrated in [1]. > > > > N.B.: clang is not impacted by any of this and gives the same results > > with either __is_constexpr() and __builting_constant_p(). > > > > Use __builtin_constant_p() instead of __is_constexpr() so that GCC can > > do the folding on constant strings. This done, strlen() does not > > require any more to be a function-like macro, so turn it into a static > > inline function. In the process, __fortify_strlen() had to be moved > > above strlen() so that it became visible to strlen(). > > This is what __compiletime_strlen() ended up doing, so this seems > reasonable to me. > > > On a side note, strlen() did a double expansion of its argument p. > > It did? Ah, was it due to __is_constexpr() wrapping? The other > expressions should have been side-effect free: > > __builtin_choose_expr(__is_constexpr(__builtin_strlen(p)), \ > __builtin_strlen(p), __fortify_strlen(p)) This does not have a side-effect. The issue is not the side effect but the double expansion itself (actually here it is a triple expansion) because it may grow exponentially. Linus explained this in greater details here: https://lore.kernel.org/all/CAHk-=wjpN4GWtnsWQ8XJvf=gBQ3UvBk512xK1S35=nGXA6yTiw@mail.gmail.com/ Note that I do not believe this to be a big deal here. Unless the strlen() macro gets called in others macro which themselves do double parameter expansion, the issue should not surface. And honestly, I do not see this happening with strings. That's why I put this as a side note, it's more of a "follow a best practice" than solving an actual problem. On double thought, the risk does not seem real here. I will simply drop this paragraph about double expansion in v2. > I don't think you build-tested this with Clang, though? I just did and... ./include/linux/fortify-string.h:272:17: error: redeclaration of 'strlen' must not have the 'overloadable' attribute 272 | __kernel_size_t strlen(const char *p) | ^ ./include/linux/string.h:200:24: note: previous unmarked overload of function is here 200 | extern __kernel_size_t strlen(const char *); | ^ 1 error generated. OK, guilty here! Changing the function prototype to: __FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1) __kernel_size_t strlen(const char * const POS p) resolves the issue. I was wondering what this POS was for, guess I have my answer. > CC scripts/mod/devicetable-offsets.s > In file included from ../scripts/mod/devicetable-offsets.c:3: > In file included from ../include/linux/mod_devicetable.h:14: > In file included from ../include/linux/uuid.h:11: > In file included from ../include/linux/string.h:389: > ../include/linux/fortify-string.h:272:17: error: redeclaration of 'strlen' must not have the 'overloadable' attribute > 272 | __kernel_size_t strlen(const char *p) > | ^ > ../include/linux/string.h:200:24: note: previous unmarked overload of function is here > 200 | extern __kernel_size_t strlen(const char *); > | ^ > > The externs will need to be reworked if it's no longer depending on asm > renaming. > > > Turning it into an inline function also resolved this side issue. > > > > [1] https://godbolt.org/z/rqr3YvoP4 > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > --- > > This patch is the successor of patch [1] which was part of a longer > > series [2]. Meanwhile, I decided to split it, so I am sending this again, > > but as a stand-alone patch. > > > > Changelog since [1]: use __builtin_constant_p() instead and turn > > strlen() into an inline function > > > > [1] https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-6-4e4cbaecc216@wanadoo.fr/ > > [2] https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-0-4e4cbaecc216@wanadoo.fr/ > > --- > > include/linux/fortify-string.h | 34 +++++++++++++++++++--------------- > > 1 file changed, 19 insertions(+), 15 deletions(-) > > > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h > > index e4ce1cae03bf770047ce8a7c032b183683388cd5..bd22dd66e5f5b66ad839df42247e6436e0afd053 100644 > > --- a/include/linux/fortify-string.h > > +++ b/include/linux/fortify-string.h > > @@ -4,7 +4,6 @@ > > > > #include <linux/bitfield.h> > > #include <linux/bug.h> > > -#include <linux/const.h> > > #include <linux/limits.h> > > > > #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable > > @@ -241,6 +240,21 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size > > * possible for strlen() to be used on compile-time strings for use in > > * static initializers (i.e. as a constant expression). > > ^^ This comment, however, is I think what sinks this patch. Please see > commit 67ebc3ab4462 ("fortify: Make sure strlen() may still be used as a > constant expression") which required that strlen() not be an inline. I'm > pretty sure the build will start failing again (though perhaps only on > older GCC versions). Strange. I tested it with GCC 5.1 on godbolt and it worked fine. After more investigation, I only got complains from GCC up to 4.9.4: https://godbolt.org/z/rW8r74vE3 I also just did a successful defconfig build using GCC 5.4 (sorry, I do not have an environment with GCC 5.1). So, it looks like an issue of the past to me. But in 67ebc3ab4462, the minimum compiler version was already 5.1: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/min-tool-version.sh?id=67ebc3ab4462#n20 In the end, I am not sure what was the issue you encountered at that time. Well, I am not able to reproduce, but if you tell me this is an issue, then I can just keep the s/__is_constexpr/__builtin_constant_p/g change in v2 and drop the inline function part (thus keeping the triple expansion). Let me know if you still think that having it as a function is a no go. In the end, the main purpose here is to get rid of the __is_constexpr. Turning the macro into a function still looks slightly better to me, but at the end I do not really mind. Yours sincerely, Vincent Mailhol
Hi Vincent, kernel test robot noticed the following build errors: [auto build test ERROR on 9d89551994a430b50c4fffcb1e617a057fa76e20] url: https://github.com/intel-lab-lkp/linux/commits/Vincent-Mailhol/fortify-turn-strlen-into-an-inline-function-using-__builtin_constant_p/20250108-223159 base: 9d89551994a430b50c4fffcb1e617a057fa76e20 patch link: https://lore.kernel.org/r/20250108-strlen_use_builtin_constant_p-v1-1-611b52e80a9f%40wanadoo.fr patch subject: [PATCH] fortify: turn strlen() into an inline function using __builtin_constant_p() config: x86_64-randconfig-001-20250109 (https://download.01.org/0day-ci/archive/20250109/202501092158.Gp5vNOZy-lkp@intel.com/config) compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250109/202501092158.Gp5vNOZy-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202501092158.Gp5vNOZy-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from scripts/mod/devicetable-offsets.c:3: In file included from include/linux/mod_devicetable.h:14: In file included from include/linux/uuid.h:11: In file included from include/linux/string.h:389: >> include/linux/fortify-string.h:272:17: error: redeclaration of 'strlen' must not have the 'overloadable' attribute 272 | __kernel_size_t strlen(const char *p) | ^ include/linux/string.h:200:24: note: previous unmarked overload of function is here 200 | extern __kernel_size_t strlen(const char *); | ^ 1 error generated. make[3]: *** [scripts/Makefile.build:102: scripts/mod/devicetable-offsets.s] Error 1 shuffle=1556232066 make[3]: Target 'scripts/mod/' not remade because of errors. make[2]: *** [Makefile:1262: prepare0] Error 2 shuffle=1556232066 make[2]: Target 'prepare' not remade because of errors. make[1]: *** [Makefile:251: __sub-make] Error 2 shuffle=1556232066 make[1]: Target 'prepare' not remade because of errors. make: *** [Makefile:251: __sub-make] Error 2 shuffle=1556232066 make: Target 'prepare' not remade because of errors. vim +272 include/linux/fortify-string.h 257 258 /** 259 * strlen - Return count of characters in a NUL-terminated string 260 * 261 * @p: pointer to NUL-terminated string to count. 262 * 263 * Do not use this function unless the string length is known at 264 * compile-time. When @p is unterminated, this function may crash 265 * or return unexpected counts that could lead to memory content 266 * exposures. Prefer strnlen(). 267 * 268 * Returns number of characters in @p (NOT including the final NUL). 269 * 270 */ 271 __FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1) > 272 __kernel_size_t strlen(const char *p) 273 { 274 if (__builtin_constant_p(__builtin_strlen(p))) 275 return __builtin_strlen(p); 276 return __fortify_strlen(p); 277 } 278
Hi Vincent,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 9d89551994a430b50c4fffcb1e617a057fa76e20]
url: https://github.com/intel-lab-lkp/linux/commits/Vincent-Mailhol/fortify-turn-strlen-into-an-inline-function-using-__builtin_constant_p/20250108-223159
base: 9d89551994a430b50c4fffcb1e617a057fa76e20
patch link: https://lore.kernel.org/r/20250108-strlen_use_builtin_constant_p-v1-1-611b52e80a9f%40wanadoo.fr
patch subject: [PATCH] fortify: turn strlen() into an inline function using __builtin_constant_p()
config: s390-randconfig-r064-20250111 (https://download.01.org/0day-ci/archive/20250111/202501110943.PA4ylzB9-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project f5cd181ffbb7cb61d582fe130d46580d5969d47a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250111/202501110943.PA4ylzB9-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501110943.PA4ylzB9-lkp@intel.com/
All warnings (new ones prefixed by >>):
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/most/most_cdev.c:8:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:454:22: warning: array index 3 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
454 | case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/most/most_cdev.c:8:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:454:10: warning: array index 7 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
454 | case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:454:42: warning: array index 6 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
454 | case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:454:53: warning: array index 3 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
454 | case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/most/most_cdev.c:8:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:456:22: warning: array index 2 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
456 | case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/most/most_cdev.c:8:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:456:10: warning: array index 5 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
456 | case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:456:42: warning: array index 4 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
456 | case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:456:53: warning: array index 2 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
456 | case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/most/most_cdev.c:8:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:458:22: warning: array index 1 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
458 | case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/most/most_cdev.c:8:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:458:10: warning: array index 3 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
458 | case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:458:42: warning: array index 2 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
458 | case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:458:53: warning: array index 1 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
458 | case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
>> drivers/most/most_cdev.c:449:2: warning: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
449 | INIT_KFIFO(c->fifo);
| ^~~~~~~~~~~~~~~~~~~
include/linux/kfifo.h:135:69: note: expanded from macro 'INIT_KFIFO'
135 | __kfifo->mask = __is_kfifo_ptr(__tmp) ? 0 : ARRAY_SIZE(__tmp->buf) - 1;\
| ~ ~~~~~~~~~~~~~~~~~~~~~~~^~~
62 warnings and 1 error generated.
--
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:454:42: warning: array index 6 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
454 | case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:454:53: warning: array index 3 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
454 | case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/tty/tty_port.c:8:
In file included from include/linux/tty.h:9:
In file included from include/linux/tty_driver.h:9:
In file included from include/linux/cdev.h:8:
In file included from include/linux/device.h:32:
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:456:22: warning: array index 2 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
456 | case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/tty/tty_port.c:8:
In file included from include/linux/tty.h:9:
In file included from include/linux/tty_driver.h:9:
In file included from include/linux/cdev.h:8:
In file included from include/linux/device.h:32:
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:456:10: warning: array index 5 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
456 | case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:456:42: warning: array index 4 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
456 | case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:456:53: warning: array index 2 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
456 | case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/tty/tty_port.c:8:
In file included from include/linux/tty.h:9:
In file included from include/linux/tty_driver.h:9:
In file included from include/linux/cdev.h:8:
In file included from include/linux/device.h:32:
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:458:22: warning: array index 1 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
458 | case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
In file included from drivers/tty/tty_port.c:8:
In file included from include/linux/tty.h:9:
In file included from include/linux/tty_driver.h:9:
In file included from include/linux/cdev.h:8:
In file included from include/linux/device.h:32:
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:168:
include/linux/compat.h:458:10: warning: array index 3 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
458 | case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:458:42: warning: array index 2 is past the end of the array (that has type 'compat_sigset_word[2]' (aka 'unsigned int[2]')) [-Warray-bounds]
458 | case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
| ^ ~
include/linux/compat.h:130:2: note: array 'sig' declared here
130 | compat_sigset_word sig[_COMPAT_NSIG_WORDS];
| ^
include/linux/compat.h:458:53: warning: array index 1 is past the end of the array (that has type 'const unsigned long[1]') [-Warray-bounds]
458 | case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
| ^ ~
arch/s390/include/asm/signal.h:22:9: note: array 'sig' declared here
22 | unsigned long sig[_NSIG_WORDS];
| ^
>> drivers/tty/tty_port.c:266:2: warning: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
266 | INIT_KFIFO(port->xmit_fifo);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/kfifo.h:135:69: note: expanded from macro 'INIT_KFIFO'
135 | __kfifo->mask = __is_kfifo_ptr(__tmp) ? 0 : ARRAY_SIZE(__tmp->buf) - 1;\
| ~ ~~~~~~~~~~~~~~~~~~~~~~~^~~
62 warnings and 1 error generated.
vim +449 drivers/most/most_cdev.c
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 400
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 401 /**
1fd923f38610a8 drivers/staging/most/cdev/cdev.c Christian Gromm 2017-11-21 402 * comp_probe - probe function of the driver module
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 403 * @iface: pointer to interface instance
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 404 * @channel_id: channel index/ID
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 405 * @cfg: pointer to actual channel configuration
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 406 * @name: name of the device to be created
fba3993e86cc44 drivers/most/most_cdev.c Randy Dunlap 2023-01-12 407 * @args: pointer to array of component parameters (from configfs)
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 408 *
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 409 * This allocates a channel object and creates the device node in /dev
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 410 *
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 411 * Returns 0 on success or error code otherwise.
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 412 */
1fd923f38610a8 drivers/staging/most/cdev/cdev.c Christian Gromm 2017-11-21 413 static int comp_probe(struct most_interface *iface, int channel_id,
dfee92dd50464c drivers/staging/most/cdev/cdev.c Christian Gromm 2019-04-03 414 struct most_channel_config *cfg, char *name, char *args)
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 415 {
ef0fbbbb9a6004 drivers/staging/most/cdev/cdev.c Christian Gromm 2017-11-21 416 struct comp_channel *c;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 417 unsigned long cl_flags;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 418 int retval;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 419 int current_minor;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 420
e8e0f7fd7715de drivers/staging/most/cdev/cdev.c Christian Gromm 2020-06-22 421 if (!cfg || !name)
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 422 return -EINVAL;
61fd971eddecdc drivers/staging/most/cdev/cdev.c Christian Gromm 2020-06-22 423
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 424 c = get_channel(iface, channel_id);
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 425 if (c)
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 426 return -EEXIST;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 427
b737a221702c7b drivers/most/most_cdev.c Christophe JAILLET 2024-06-08 428 current_minor = ida_alloc(&comp.minor_id, GFP_KERNEL);
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 429 if (current_minor < 0)
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 430 return current_minor;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 431
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 432 c = kzalloc(sizeof(*c), GFP_KERNEL);
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 433 if (!c) {
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 434 retval = -ENOMEM;
bddd3c2546e9c4 drivers/staging/most/cdev/cdev.c Christian Gromm 2018-09-21 435 goto err_remove_ida;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 436 }
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 437
c73d915dd293a4 drivers/staging/most/cdev/cdev.c Christian Gromm 2017-11-21 438 c->devno = MKDEV(comp.major, current_minor);
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 439 cdev_init(&c->cdev, &channel_fops);
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 440 c->cdev.owner = THIS_MODULE;
5ae890780e1b4d drivers/staging/most/cdev/cdev.c Colin Ian King 2019-02-02 441 retval = cdev_add(&c->cdev, c->devno, 1);
5ae890780e1b4d drivers/staging/most/cdev/cdev.c Colin Ian King 2019-02-02 442 if (retval < 0)
5ae890780e1b4d drivers/staging/most/cdev/cdev.c Colin Ian King 2019-02-02 443 goto err_free_c;
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 444 c->iface = iface;
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 445 c->cfg = cfg;
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 446 c->channel_id = channel_id;
b3c9f3c56c41cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 447 c->access_ref = 0;
fa96b8ed9cc562 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 448 spin_lock_init(&c->unlink);
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 @449 INIT_KFIFO(c->fifo);
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 450 retval = kfifo_alloc(&c->fifo, cfg->num_buffers, GFP_KERNEL);
ebf256e36754fc drivers/staging/most/cdev/cdev.c Keyur Patel 2019-07-14 451 if (retval)
bddd3c2546e9c4 drivers/staging/most/cdev/cdev.c Christian Gromm 2018-09-21 452 goto err_del_cdev_and_free_channel;
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 453 init_waitqueue_head(&c->wq);
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 454 mutex_init(&c->io_mutex);
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 455 spin_lock_irqsave(&ch_list_lock, cl_flags);
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 456 list_add_tail(&c->list, &channel_list);
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 457 spin_unlock_irqrestore(&ch_list_lock, cl_flags);
c73d915dd293a4 drivers/staging/most/cdev/cdev.c Christian Gromm 2017-11-21 458 c->dev = device_create(comp.class, NULL, c->devno, NULL, "%s", name);
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 459
ea39854712ba8a drivers/staging/most/aim-cdev/cdev.c Sudip Mukherjee 2016-02-08 460 if (IS_ERR(c->dev)) {
ea39854712ba8a drivers/staging/most/aim-cdev/cdev.c Sudip Mukherjee 2016-02-08 461 retval = PTR_ERR(c->dev);
bddd3c2546e9c4 drivers/staging/most/cdev/cdev.c Christian Gromm 2018-09-21 462 goto err_free_kfifo_and_del_list;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 463 }
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 464 kobject_uevent(&c->dev->kobj, KOBJ_ADD);
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 465 return 0;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 466
bddd3c2546e9c4 drivers/staging/most/cdev/cdev.c Christian Gromm 2018-09-21 467 err_free_kfifo_and_del_list:
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 468 kfifo_free(&c->fifo);
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 469 list_del(&c->list);
bddd3c2546e9c4 drivers/staging/most/cdev/cdev.c Christian Gromm 2018-09-21 470 err_del_cdev_and_free_channel:
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 471 cdev_del(&c->cdev);
5ae890780e1b4d drivers/staging/most/cdev/cdev.c Colin Ian King 2019-02-02 472 err_free_c:
d8b082e6c625cb drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-12-22 473 kfree(c);
bddd3c2546e9c4 drivers/staging/most/cdev/cdev.c Christian Gromm 2018-09-21 474 err_remove_ida:
b737a221702c7b drivers/most/most_cdev.c Christophe JAILLET 2024-06-08 475 ida_free(&comp.minor_id, current_minor);
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 476 return retval;
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 477 }
9bc79bbcd0c526 drivers/staging/most/aim-cdev/cdev.c Christian Gromm 2015-07-24 478
On Thu. 9 Jan 2025 at 16:52, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: (...) > > > * possible for strlen() to be used on compile-time strings for use in > > > * static initializers (i.e. as a constant expression). > > > > ^^ This comment, however, is I think what sinks this patch. Please see > > commit 67ebc3ab4462 ("fortify: Make sure strlen() may still be used as a > > constant expression") which required that strlen() not be an inline. I'm > > pretty sure the build will start failing again (though perhaps only on > > older GCC versions). > > Strange. I tested it with GCC 5.1 on godbolt and it worked fine. After > more investigation, I only got complains from GCC up to 4.9.4: > > https://godbolt.org/z/rW8r74vE3 > > I also just did a successful defconfig build using GCC 5.4 (sorry, I > do not have an environment with GCC 5.1). > > So, it looks like an issue of the past to me. But in 67ebc3ab4462, the > minimum compiler version was already 5.1: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/min-tool-version.sh?id=67ebc3ab4462#n20 > > In the end, I am not sure what was the issue you encountered at that time. > > Well, I am not able to reproduce, but if you tell me this is an issue, > then I can just keep the s/__is_constexpr/__builtin_constant_p/g > change in v2 and drop the inline function part (thus keeping the > triple expansion). > > Let me know if you still think that having it as a function is a no > go. In the end, the main purpose here is to get rid of the > __is_constexpr. Turning the macro into a function still looks slightly > better to me, but at the end I do not really mind. Actually, I did more investigation and it is working for some strange reasons. Whenever the argument of a function named strlen() is a compile time constant, the compiler (both GCC and clang) will replace it with the string length on the argument, even if strlen() is programmed to return something else: https://godbolt.org/z/nK4b3fnM7 So it is only working because the compiler uses its builtin strlen() instead of the function we provided. Actually, we could just do: extern inline size_t strlen(const char *p) { return __fortify_strlen(p); } and it would work: the compiler ignores the content whenever p is a compile time constant ¯\_(ツ)_/¯ Well, all this is really ugly. I will drop the inline function and send a v2 just with the s/__is_constexpr/__builtin_constant_p/g replacement. Yours sincerely, Vincent Mailhol
On Sat, 11 Jan 2025 23:40:41 +0900 Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > On Thu. 9 Jan 2025 at 16:52, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: ... > Actually, I did more investigation and it is working for some strange > reasons. Whenever the argument of a function named strlen() is a > compile time constant, the compiler (both GCC and clang) will replace > it with the string length on the argument, even if strlen() is > programmed to return something else: > > https://godbolt.org/z/nK4b3fnM7 > > So it is only working because the compiler uses its builtin strlen() > instead of the function we provided. It depends on whether -ffreestanding is set. If not set gcc/clang assume a lot of the basic libc functions have their expected behaviour. David
On 12/01/2025 at 01:58, David Laight wrote: > On Sat, 11 Jan 2025 23:40:41 +0900 > Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > >> On Thu. 9 Jan 2025 at 16:52, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > ... >> Actually, I did more investigation and it is working for some strange >> reasons. Whenever the argument of a function named strlen() is a >> compile time constant, the compiler (both GCC and clang) will replace >> it with the string length on the argument, even if strlen() is >> programmed to return something else: >> >> https://godbolt.org/z/nK4b3fnM7 >> >> So it is only working because the compiler uses its builtin strlen() >> instead of the function we provided. > > It depends on whether -ffreestanding is set. > If not set gcc/clang assume a lot of the basic libc functions have their expected > behaviour. Thanks! This rings a bell. More precisely, rather than -ffreestanding, it depends on -fno-builtin (which is implied by -ffreestanding). But some architectures do not provide these flags. Most famously, x86-64 has neither of -fno-builtin nor -ffreestanding c.f.: https://lore.kernel.org/all/20220306171009.1973074-1-mailhol.vincent@wanadoo.fr/T/#u (above thread is a bit old, but I did a make V=1 to confirm that the flags are still missing as of today). And with either of -fno-builtin or -ffreestanding, the strlen() inline function doesn't work anymore as a static initializer (at least it didn't work in my tests on godbolt). So now, I am convinced that turning this strlen() in a function was a bad idea. I am glad I removed it in v2. Yours sincerely, Vincent Mailhol
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index e4ce1cae03bf770047ce8a7c032b183683388cd5..bd22dd66e5f5b66ad839df42247e6436e0afd053 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -4,7 +4,6 @@ #include <linux/bitfield.h> #include <linux/bug.h> -#include <linux/const.h> #include <linux/limits.h> #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable @@ -241,6 +240,21 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size * possible for strlen() to be used on compile-time strings for use in * static initializers (i.e. as a constant expression). */ +__FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1) +__kernel_size_t __fortify_strlen(const char * const POS p) +{ + const size_t p_size = __member_size(p); + __kernel_size_t ret; + + /* Give up if we don't know how large p is. */ + if (p_size == SIZE_MAX) + return __underlying_strlen(p); + ret = strnlen(p, p_size); + if (p_size <= ret) + fortify_panic(FORTIFY_FUNC_strlen, FORTIFY_READ, p_size, ret + 1, ret); + return ret; +} + /** * strlen - Return count of characters in a NUL-terminated string * @@ -254,22 +268,12 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size * Returns number of characters in @p (NOT including the final NUL). * */ -#define strlen(p) \ - __builtin_choose_expr(__is_constexpr(__builtin_strlen(p)), \ - __builtin_strlen(p), __fortify_strlen(p)) __FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1) -__kernel_size_t __fortify_strlen(const char * const POS p) +__kernel_size_t strlen(const char *p) { - const size_t p_size = __member_size(p); - __kernel_size_t ret; - - /* Give up if we don't know how large p is. */ - if (p_size == SIZE_MAX) - return __underlying_strlen(p); - ret = strnlen(p, p_size); - if (p_size <= ret) - fortify_panic(FORTIFY_FUNC_strlen, FORTIFY_READ, p_size, ret + 1, ret); - return ret; + if (__builtin_constant_p(__builtin_strlen(p))) + return __builtin_strlen(p); + return __fortify_strlen(p); } /* Defined after fortified strnlen() to reuse it. */
The strlen(p) function-like macro uses: __is_constexpr(__builtin_strlen(p)) in which GCC would only yield true if the argument p is a string literal. Otherwise, GCC would return false even if p is a const string. In contrary, by using: __builtin_constant_p(__builtin_strlen(p)) then GCC can also recognizes when p is a compile time constant string. The above is illustrated in [1]. N.B.: clang is not impacted by any of this and gives the same results with either __is_constexpr() and __builting_constant_p(). Use __builtin_constant_p() instead of __is_constexpr() so that GCC can do the folding on constant strings. This done, strlen() does not require any more to be a function-like macro, so turn it into a static inline function. In the process, __fortify_strlen() had to be moved above strlen() so that it became visible to strlen(). On a side note, strlen() did a double expansion of its argument p. Turning it into an inline function also resolved this side issue. [1] https://godbolt.org/z/rqr3YvoP4 Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- This patch is the successor of patch [1] which was part of a longer series [2]. Meanwhile, I decided to split it, so I am sending this again, but as a stand-alone patch. Changelog since [1]: use __builtin_constant_p() instead and turn strlen() into an inline function [1] https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-6-4e4cbaecc216@wanadoo.fr/ [2] https://lore.kernel.org/all/20241203-is_constexpr-refactor-v1-0-4e4cbaecc216@wanadoo.fr/ --- include/linux/fortify-string.h | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) --- base-commit: 9d89551994a430b50c4fffcb1e617a057fa76e20 change-id: 20250105-strlen_use_builtin_constant_p-515aca505ca4 Best regards,