slab: remove __alloc_size attribute from __kmalloc_track_caller

PATCH In review Greg Kroah-Hartman 5 participants 8 messages Feb 18, 2022 – Feb 19, 2022

Subsystems (3)

Stable branch Memory management Slab allocator

lore

Share & tools

lore

Greg Kroah-Hartman

author maintainer

2022-02-18 13:14 UTC #0 lore raw

PATCH 1/1 slab: remove __alloc_size attribute from __kmalloc_track_caller

Commit c37495d6254c ("slab: add __alloc_size attributes for better bounds checking") added __alloc_size attributes to a bunch of kmalloc function prototypes. Unfortunately the change to __kmalloc_track_caller seems to cause clang to generate broken code and the first time this is called when booting, the box will crash.

While the compiler problems are being reworked and attempted to be solved, let's just drop the attribute to solve the issue now. Once it is resolved it can be added back.

Fixes c37495d6254c ("slab: add __alloc_size attributes for better bounds checking")

Cc stable <[email protected]>

Cc Kees Cook <[email protected]>

Cc Daniel Micay <[email protected]>

Cc Nick Desaulniers <[email protected]>

Cc Christoph Lameter <[email protected]>

Cc Pekka Enberg <[email protected]>

Cc David Rientjes <[email protected]>

Cc Joonsoo Kim <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Vlastimil Babka <[email protected]>

Cc Nathan Chancellor <[email protected]>

Cc [email protected]

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 include/linux/slab.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

include/linux/slab.h +1 -2

index 37bde99b74af..5b6193fd8bd9 100644
@@ -660,8 +660,7 @@ static inline __alloc_size(1, 2) void *kcalloc(size_t n, size_t size, gfp_t flag
  * allocator where we care about the real place the memory allocation
  * request comes from.
  */
-extern void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
-				   __alloc_size(1);
+extern void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller);
 #define kmalloc_track_caller(size, flags) \
 	__kmalloc_track_caller(size, flags, _RET_IP_)

Signature · 1 line

2.35.1

Vlastimil Babka

2022-02-18 17:16 UTC #1 lore raw

Quoted 9 lines · Greg Kroah-Hartman

Commit c37495d6254c ("slab: add __alloc_size attributes for better bounds checking") added __alloc_size attributes to a bunch of kmalloc function prototypes. Unfortunately the change to __kmalloc_track_caller seems to cause clang to generate broken code and the first time this is called when booting, the box will crash.
While the compiler problems are being reworked and attempted to be solved, let's just drop the attribute to solve the issue now. Once it is resolved it can be added back.

Could we instead wrap it in some #ifdef that' only true for clang build? That would make the workaround more precise and self-documented. Even better if it can trigger using clang version range and once a fixed clang version is here, it can be updated to stay true for older clangs.

Quoted 34 lines

Fixes c37495d6254c ("slab: add __alloc_size attributes for better bounds checking")
Cc stable <[email protected]>
Cc Kees Cook <[email protected]>
Cc Daniel Micay <[email protected]>
Cc Nick Desaulniers <[email protected]>
Cc Christoph Lameter <[email protected]>
Cc Pekka Enberg <[email protected]>
Cc David Rientjes <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Vlastimil Babka <[email protected]>
Cc Nathan Chancellor <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 include/linux/slab.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
include/linux/slab.h +1 -2
index 37bde99b74af..5b6193fd8bd9 100644
@@ -660,8 +660,7 @@ static inline __alloc_size(1, 2) void *kcalloc(size_t n, size_t size, gfp_t flag
  * allocator where we care about the real place the memory allocation
  * request comes from.
  */
-extern void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
-				   __alloc_size(1);
+extern void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller);
 #define kmalloc_track_caller(size, flags) \
 	__kmalloc_track_caller(size, flags, _RET_IP_)
 

Greg Kroah-Hartman

author maintainer

2022-02-18 17:57 UTC #2 lore raw

Quoted 7 lines · Vlastimil Babka

Quoted 6 lines · Greg Kroah-Hartman
Commit c37495d6254c ("slab: add __alloc_size attributes for better bounds checking") added __alloc_size attributes to a bunch of kmalloc function prototypes. Unfortunately the change to __kmalloc_track_caller seems to cause clang to generate broken code and the first time this is called when booting, the box will crash.

Quoted 8 lines

Quoted 3 lines
While the compiler problems are being reworked and attempted to be solved, let's just drop the attribute to solve the issue now. Once it is resolved it can be added back.
Could we instead wrap it in some #ifdef that' only true for clang build? That would make the workaround more precise and self-documented. Even better if it can trigger using clang version range and once a fixed clang version is here, it can be updated to stay true for older clangs.

It's not doing all that much good like this, let's just remove it for now until it does actually provide a benifit and not just crash the box :)

This is only 1 function, that is used in only a very small number of callers. I do not think it will be missed.

thanks,

greg k-h

Vlastimil Babka

2022-02-18 18:54 UTC #3 lore raw

Quoted 17 lines · Greg Kroah-Hartman

Quoted 7 lines · Vlastimil Babka
Quoted 6 lines · Greg Kroah-Hartman
Commit c37495d6254c ("slab: add __alloc_size attributes for better bounds checking") added __alloc_size attributes to a bunch of kmalloc function prototypes. Unfortunately the change to __kmalloc_track_caller seems to cause clang to generate broken code and the first time this is called when booting, the box will crash.
Quoted 8 lines
Quoted 3 lines
While the compiler problems are being reworked and attempted to be solved, let's just drop the attribute to solve the issue now. Once it is resolved it can be added back.
Could we instead wrap it in some #ifdef that' only true for clang build? That would make the workaround more precise and self-documented. Even better if it can trigger using clang version range and once a fixed clang version is here, it can be updated to stay true for older clangs.

Quoted 5 lines

It's not doing all that much good like this, let's just remove it for now until it does actually provide a benifit and not just crash the box :)
This is only 1 function, that is used in only a very small number of callers. I do not think it will be missed.

Fair enough, added to the slab tree:

https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/log/?h=for-5.17/fixup5

Quoted 4 lines

thanks,
greg k-h

Greg Kroah-Hartman

author maintainer

2022-02-19 07:19 UTC #4 lore raw

Quoted 21 lines · Vlastimil Babka

Quoted 20 lines · Greg Kroah-Hartman
Quoted 7 lines · Vlastimil Babka
Quoted 6 lines · Greg Kroah-Hartman
Commit c37495d6254c ("slab: add __alloc_size attributes for better bounds checking") added __alloc_size attributes to a bunch of kmalloc function prototypes. Unfortunately the change to __kmalloc_track_caller seems to cause clang to generate broken code and the first time this is called when booting, the box will crash.
Quoted 8 lines
Quoted 3 lines
While the compiler problems are being reworked and attempted to be solved, let's just drop the attribute to solve the issue now. Once it is resolved it can be added back.
Could we instead wrap it in some #ifdef that' only true for clang build? That would make the workaround more precise and self-documented. Even better if it can trigger using clang version range and once a fixed clang version is here, it can be updated to stay true for older clangs.
It's not doing all that much good like this, let's just remove it for now until it does actually provide a benifit and not just crash the box :)

Quoted 7 lines

Quoted 2 lines
This is only 1 function, that is used in only a very small number of callers. I do not think it will be missed.
Fair enough, added to the slab tree:
https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/log/?h=for-5.17/fixup5

Thanks!

Nick Desaulniers

Acked-by on v1 1/1

2022-02-18 17:20 UTC #5 lore raw

In reply to Greg Kroah-Hartman

Quoted 10 lines · Greg Kroah-Hartman

Commit c37495d6254c ("slab: add __alloc_size attributes for better bounds checking") added __alloc_size attributes to a bunch of kmalloc function prototypes. Unfortunately the change to __kmalloc_track_caller seems to cause clang to generate broken code and the first time this is called when booting, the box will crash.
While the compiler problems are being reworked and attempted to be solved, let's just drop the attribute to solve the issue now. Once it is resolved it can be added back.

Sorry about the mess; we'll get it cleaned up!

Acked-by Nick Desaulniers <[email protected]>

Link https://github.com/ClangBuiltLinux/linux/issues/1599

Quoted 38 lines

Fixes c37495d6254c ("slab: add __alloc_size attributes for better bounds checking")
Cc stable <[email protected]>
Cc Kees Cook <[email protected]>
Cc Daniel Micay <[email protected]>
Cc Nick Desaulniers <[email protected]>
Cc Christoph Lameter <[email protected]>
Cc Pekka Enberg <[email protected]>
Cc David Rientjes <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Vlastimil Babka <[email protected]>
Cc Nathan Chancellor <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 include/linux/slab.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
include/linux/slab.h +1 -3
index 37bde99b74af..5b6193fd8bd9 100644
@@ -660,8 +660,7 @@ static inline __alloc_size(1, 2) void *kcalloc(size_t n, size_t size, gfp_t flag
  * allocator where we care about the real place the memory allocation
  * request comes from.
  */
-extern void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller)
-                                  __alloc_size(1);
+extern void *__kmalloc_track_caller(size_t size, gfp_t flags, unsigned long caller);
 #define kmalloc_track_caller(size, flags) \
        __kmalloc_track_caller(size, flags, _RET_IP_)
 
--
2.35.1

Signature · 2 lines

Thanks,
~Nick Desaulniers

Kees Cook

Acked-by

2022-02-19 23:58 UTC #6 lore raw

Quoted 9 lines · Nick Desaulniers

Quoted 7 lines · Greg Kroah-Hartman
Commit c37495d6254c ("slab: add __alloc_size attributes for better bounds checking") added __alloc_size attributes to a bunch of kmalloc function prototypes. Unfortunately the change to __kmalloc_track_caller seems to cause clang to generate broken code and the first time this is called when booting, the box will crash.

Quoted 7 lines

Quoted 3 lines
While the compiler problems are being reworked and attempted to be solved, let's just drop the attribute to solve the issue now. Once it is resolved it can be added back.
Sorry about the mess; we'll get it cleaned up!
Acked-by Nick Desaulniers <[email protected]>
Link https://github.com/ClangBuiltLinux/linux/issues/1599

Thanks for the issue link!

Acked-by Kees Cook <[email protected]>

Signature · 1 line

Kees Cook

David Rientjes

reviewer Acked-by on v1 1/1

2022-02-18 18:47 UTC #7 lore raw

In reply to Greg Kroah-Hartman

Quoted 10 lines · Greg Kroah-Hartman

Commit c37495d6254c ("slab: add __alloc_size attributes for better bounds checking") added __alloc_size attributes to a bunch of kmalloc function prototypes. Unfortunately the change to __kmalloc_track_caller seems to cause clang to generate broken code and the first time this is called when booting, the box will crash.
While the compiler problems are being reworked and attempted to be solved, let's just drop the attribute to solve the issue now. Once it is resolved it can be added back.

Quoted 16 lines

Fixes c37495d6254c ("slab: add __alloc_size attributes for better bounds checking")
Cc stable <[email protected]>
Cc Kees Cook <[email protected]>
Cc Daniel Micay <[email protected]>
Cc Nick Desaulniers <[email protected]>
Cc Christoph Lameter <[email protected]>
Cc Pekka Enberg <[email protected]>
Cc David Rientjes <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Vlastimil Babka <[email protected]>
Cc Nathan Chancellor <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by Greg Kroah-Hartman <[email protected]>

Acked-by David Rientjes <[email protected]>