[PATCH] slab: remove __alloc_size attribute from __kmalloc_track_caller

[PATCH] slab: remove __alloc_size attribute from __kmalloc_track_caller

[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.

Fixes: c37495d6254c ("slab: add __alloc_size attributes for better bounds checking")
Cc: stable <stable@vger.kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Daniel Micay <danielmicay@gmail.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Cc: llvm@lists.linux.dev
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/slab.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 37bde99b74af..5b6193fd8bd9 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -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

Re: [PATCH] slab: remove __alloc_size attribute from __kmalloc_track_caller

[Vlastimil Babka]

On 2/18/22 14:13, Greg Kroah-Hartman wrote:
> 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.

> Fixes: c37495d6254c ("slab: add __alloc_size attributes for better bounds checking")
> Cc: stable <stable@vger.kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Daniel Micay <danielmicay@gmail.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Cc: llvm@lists.linux.dev
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  include/linux/slab.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 37bde99b74af..5b6193fd8bd9 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -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_)
>  

Re: [PATCH] slab: remove __alloc_size attribute from __kmalloc_track_caller

[Greg Kroah-Hartman]

On Fri, Feb 18, 2022 at 06:14:55PM +0100, Vlastimil Babka wrote:
> On 2/18/22 14:13, Greg Kroah-Hartman wrote:
> > 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.

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

Re: [PATCH] slab: remove __alloc_size attribute from __kmalloc_track_caller

[Vlastimil Babka]

On 2/18/22 18:57, Greg Kroah-Hartman wrote:
> On Fri, Feb 18, 2022 at 06:14:55PM +0100, Vlastimil Babka wrote:
>> On 2/18/22 14:13, Greg Kroah-Hartman wrote:
>> > 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.
> 
> 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

> thanks,
> 
> greg k-h
> 

Re: [PATCH] slab: remove __alloc_size attribute from __kmalloc_track_caller

[Greg Kroah-Hartman]

On Fri, Feb 18, 2022 at 07:54:14PM +0100, Vlastimil Babka wrote:
> On 2/18/22 18:57, Greg Kroah-Hartman wrote:
> > On Fri, Feb 18, 2022 at 06:14:55PM +0100, Vlastimil Babka wrote:
> >> On 2/18/22 14:13, Greg Kroah-Hartman wrote:
> >> > 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.
> > 
> > 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
> 

Thanks!

Re: [PATCH] slab: remove __alloc_size attribute from __kmalloc_track_caller

[Nick Desaulniers]

On Fri, Feb 18, 2022 at 5:14 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> 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 <ndesaulniers@google.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/1599

>
> Fixes: c37495d6254c ("slab: add __alloc_size attributes for better bounds checking")
> Cc: stable <stable@vger.kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Daniel Micay <danielmicay@gmail.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Cc: llvm@lists.linux.dev
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  include/linux/slab.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 37bde99b74af..5b6193fd8bd9 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -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
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] slab: remove __alloc_size attribute from __kmalloc_track_caller

[Kees Cook]

On Fri, Feb 18, 2022 at 09:19:51AM -0800, Nick Desaulniers wrote:
> On Fri, Feb 18, 2022 at 5:14 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > 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 <ndesaulniers@google.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1599

Thanks for the issue link!

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH] slab: remove __alloc_size attribute from __kmalloc_track_caller

[David Rientjes]

On Fri, 18 Feb 2022, Greg Kroah-Hartman wrote:

> 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 <stable@vger.kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Daniel Micay <danielmicay@gmail.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Cc: llvm@lists.linux.dev
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: David Rientjes <rientjes@google.com>