[PATCH] mm/slub: fix to return errno if kmalloc() fails

[PATCH] mm/slub: fix to return errno if kmalloc() fails

[Chao Yu]

From: Chao Yu <chao.yu@oppo.com>

In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
out-of-memory, if it fails, return errno correctly rather than
triggering panic via BUG_ON();

kernel BUG at mm/slub.c:5893!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP

Call trace:
 sysfs_slab_add+0x258/0x260 mm/slub.c:5973
 __kmem_cache_create+0x60/0x118 mm/slub.c:4899
 create_cache mm/slab_common.c:229 [inline]
 kmem_cache_create_usercopy+0x19c/0x31c mm/slab_common.c:335
 kmem_cache_create+0x1c/0x28 mm/slab_common.c:390
 f2fs_kmem_cache_create fs/f2fs/f2fs.h:2766 [inline]
 f2fs_init_xattr_caches+0x78/0xb4 fs/f2fs/xattr.c:808
 f2fs_fill_super+0x1050/0x1e0c fs/f2fs/super.c:4149
 mount_bdev+0x1b8/0x210 fs/super.c:1400
 f2fs_mount+0x44/0x58 fs/f2fs/super.c:4512
 legacy_get_tree+0x30/0x74 fs/fs_context.c:610
 vfs_get_tree+0x40/0x140 fs/super.c:1530
 do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
 path_mount+0x358/0x914 fs/namespace.c:3370
 do_mount fs/namespace.c:3383 [inline]
 __do_sys_mount fs/namespace.c:3591 [inline]
 __se_sys_mount fs/namespace.c:3568 [inline]
 __arm64_sys_mount+0x2f8/0x408 fs/namespace.c:3568

Cc: <stable@kernel.org>
Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com
Signed-off-by: Chao Yu <chao.yu@oppo.com>
---
 mm/slub.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index 862dbd9af4f5..e6f3727b9ad2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5890,7 +5890,8 @@ static char *create_unique_id(struct kmem_cache *s)
 	char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
 	char *p = name;
 
-	BUG_ON(!name);
+	if (!name)
+		return ERR_PTR(-ENOMEM);
 
 	*p++ = ':';
 	/*
@@ -5948,6 +5949,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
 		 * for the symlinks.
 		 */
 		name = create_unique_id(s);
+		if (IS_ERR(name))
+			return PTR_ERR(name);
 	}
 
 	s->kobj.kset = kset;
-- 
2.25.1

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Muchun Song]

> On Aug 30, 2022, at 22:10, Chao Yu <chao@kernel.org> wrote:
> 
> From: Chao Yu <chao.yu@oppo.com>
> 
> In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
> out-of-memory, if it fails, return errno correctly rather than
> triggering panic via BUG_ON();

I tend to agree with you. A mount operation shouldn’t panic the
kernel.

> 
> kernel BUG at mm/slub.c:5893!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> 
> Call trace:
> sysfs_slab_add+0x258/0x260 mm/slub.c:5973
> __kmem_cache_create+0x60/0x118 mm/slub.c:4899
> create_cache mm/slab_common.c:229 [inline]
> kmem_cache_create_usercopy+0x19c/0x31c mm/slab_common.c:335
> kmem_cache_create+0x1c/0x28 mm/slab_common.c:390
> f2fs_kmem_cache_create fs/f2fs/f2fs.h:2766 [inline]
> f2fs_init_xattr_caches+0x78/0xb4 fs/f2fs/xattr.c:808
> f2fs_fill_super+0x1050/0x1e0c fs/f2fs/super.c:4149
> mount_bdev+0x1b8/0x210 fs/super.c:1400
> f2fs_mount+0x44/0x58 fs/f2fs/super.c:4512
> legacy_get_tree+0x30/0x74 fs/fs_context.c:610
> vfs_get_tree+0x40/0x140 fs/super.c:1530
> do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
> path_mount+0x358/0x914 fs/namespace.c:3370
> do_mount fs/namespace.c:3383 [inline]
> __do_sys_mount fs/namespace.c:3591 [inline]
> __se_sys_mount fs/namespace.c:3568 [inline]
> __arm64_sys_mount+0x2f8/0x408 fs/namespace.c:3568
> 
> Cc: <stable@kernel.org>
> Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com
> Signed-off-by: Chao Yu <chao.yu@oppo.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Hyeonggon Yoo]

On Tue, Aug 30, 2022 at 10:10:09PM +0800, Chao Yu wrote:
> From: Chao Yu <chao.yu@oppo.com>
> 
> In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
> out-of-memory, if it fails, return errno correctly rather than
> triggering panic via BUG_ON();
> 
> kernel BUG at mm/slub.c:5893!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> 
> Call trace:
>  sysfs_slab_add+0x258/0x260 mm/slub.c:5973
>  __kmem_cache_create+0x60/0x118 mm/slub.c:4899
>  create_cache mm/slab_common.c:229 [inline]
>  kmem_cache_create_usercopy+0x19c/0x31c mm/slab_common.c:335
>  kmem_cache_create+0x1c/0x28 mm/slab_common.c:390
>  f2fs_kmem_cache_create fs/f2fs/f2fs.h:2766 [inline]
>  f2fs_init_xattr_caches+0x78/0xb4 fs/f2fs/xattr.c:808
>  f2fs_fill_super+0x1050/0x1e0c fs/f2fs/super.c:4149
>  mount_bdev+0x1b8/0x210 fs/super.c:1400
>  f2fs_mount+0x44/0x58 fs/f2fs/super.c:4512
>  legacy_get_tree+0x30/0x74 fs/fs_context.c:610
>  vfs_get_tree+0x40/0x140 fs/super.c:1530
>  do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
>  path_mount+0x358/0x914 fs/namespace.c:3370
>  do_mount fs/namespace.c:3383 [inline]
>  __do_sys_mount fs/namespace.c:3591 [inline]
>  __se_sys_mount fs/namespace.c:3568 [inline]
>  __arm64_sys_mount+0x2f8/0x408 fs/namespace.c:3568
> 
> Cc: <stable@kernel.org>
> Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com

Fixes: 81819f0fc8285 ("SLUB core")

> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
>  mm/slub.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 862dbd9af4f5..e6f3727b9ad2 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5890,7 +5890,8 @@ static char *create_unique_id(struct kmem_cache *s)
>  	char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
>  	char *p = name;
>  
> -	BUG_ON(!name);
> +	if (!name)
> +		return ERR_PTR(-ENOMEM);
>  
>  	*p++ = ':';
>  	/*
> @@ -5948,6 +5949,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
>  		 * for the symlinks.
>  		 */
>  		name = create_unique_id(s);
> +		if (IS_ERR(name))
> +			return PTR_ERR(name);
>  	}
>  
>  	s->kobj.kset = kset;
> -- 
> 2.25.1
> 
> 

Looks good to me.

Reviewed-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>

Thanks!

-- 
Thanks,
Hyeonggon

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[David Rientjes]

On Tue, 30 Aug 2022, Chao Yu wrote:

> From: Chao Yu <chao.yu@oppo.com>
> 
> In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
> out-of-memory, if it fails, return errno correctly rather than
> triggering panic via BUG_ON();
> 
> kernel BUG at mm/slub.c:5893!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> 
> Call trace:
>  sysfs_slab_add+0x258/0x260 mm/slub.c:5973
>  __kmem_cache_create+0x60/0x118 mm/slub.c:4899
>  create_cache mm/slab_common.c:229 [inline]
>  kmem_cache_create_usercopy+0x19c/0x31c mm/slab_common.c:335
>  kmem_cache_create+0x1c/0x28 mm/slab_common.c:390
>  f2fs_kmem_cache_create fs/f2fs/f2fs.h:2766 [inline]
>  f2fs_init_xattr_caches+0x78/0xb4 fs/f2fs/xattr.c:808
>  f2fs_fill_super+0x1050/0x1e0c fs/f2fs/super.c:4149
>  mount_bdev+0x1b8/0x210 fs/super.c:1400
>  f2fs_mount+0x44/0x58 fs/f2fs/super.c:4512
>  legacy_get_tree+0x30/0x74 fs/fs_context.c:610
>  vfs_get_tree+0x40/0x140 fs/super.c:1530
>  do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
>  path_mount+0x358/0x914 fs/namespace.c:3370
>  do_mount fs/namespace.c:3383 [inline]
>  __do_sys_mount fs/namespace.c:3591 [inline]
>  __se_sys_mount fs/namespace.c:3568 [inline]
>  __arm64_sys_mount+0x2f8/0x408 fs/namespace.c:3568
> 
> Cc: <stable@kernel.org>
> Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com
> Signed-off-by: Chao Yu <chao.yu@oppo.com>

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

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Vlastimil Babka (SUSE)]

On 8/31/22 05:09, Muchun Song wrote:
> 
> 
>> On Aug 30, 2022, at 22:10, Chao Yu <chao@kernel.org> wrote:

Please use scripts/get_maintainer.pl next time, I could have missed this.

>> From: Chao Yu <chao.yu@oppo.com>
>> 
>> In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
>> out-of-memory, if it fails, return errno correctly rather than
>> triggering panic via BUG_ON();
> 
> I tend to agree with you. A mount operation shouldn’t panic the
> kernel.

Hmm kmalloc(64) shouldn't normally due that due to the the underlying page
allocation falling into the "too small to fail" category, wonder if
syzkaller was doing anything special here?

But yeah we should get rid of all BUG_ONs eventually, just not sure if
stable@ is needed here.

>> 
>> kernel BUG at mm/slub.c:5893!
>> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>> 
>> Call trace:
>> sysfs_slab_add+0x258/0x260 mm/slub.c:5973
>> __kmem_cache_create+0x60/0x118 mm/slub.c:4899
>> create_cache mm/slab_common.c:229 [inline]
>> kmem_cache_create_usercopy+0x19c/0x31c mm/slab_common.c:335
>> kmem_cache_create+0x1c/0x28 mm/slab_common.c:390
>> f2fs_kmem_cache_create fs/f2fs/f2fs.h:2766 [inline]
>> f2fs_init_xattr_caches+0x78/0xb4 fs/f2fs/xattr.c:808
>> f2fs_fill_super+0x1050/0x1e0c fs/f2fs/super.c:4149
>> mount_bdev+0x1b8/0x210 fs/super.c:1400
>> f2fs_mount+0x44/0x58 fs/f2fs/super.c:4512
>> legacy_get_tree+0x30/0x74 fs/fs_context.c:610
>> vfs_get_tree+0x40/0x140 fs/super.c:1530
>> do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
>> path_mount+0x358/0x914 fs/namespace.c:3370
>> do_mount fs/namespace.c:3383 [inline]
>> __do_sys_mount fs/namespace.c:3591 [inline]
>> __se_sys_mount fs/namespace.c:3568 [inline]
>> __arm64_sys_mount+0x2f8/0x408 fs/namespace.c:3568
>> 
>> Cc: <stable@kernel.org>
>> Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com
>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> 
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> 
> Thanks.
> 
> 

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Christophe JAILLET]

Le 30/08/2022 à 16:10, Chao Yu a écrit :
> From: Chao Yu <chao.yu@oppo.com>
> 
> In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
> out-of-memory, if it fails, return errno correctly rather than
> triggering panic via BUG_ON();
> 
> kernel BUG at mm/slub.c:5893!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> 
> Call trace:
>   sysfs_slab_add+0x258/0x260 mm/slub.c:5973
>   __kmem_cache_create+0x60/0x118 mm/slub.c:4899
>   create_cache mm/slab_common.c:229 [inline]
>   kmem_cache_create_usercopy+0x19c/0x31c mm/slab_common.c:335
>   kmem_cache_create+0x1c/0x28 mm/slab_common.c:390
>   f2fs_kmem_cache_create fs/f2fs/f2fs.h:2766 [inline]
>   f2fs_init_xattr_caches+0x78/0xb4 fs/f2fs/xattr.c:808
>   f2fs_fill_super+0x1050/0x1e0c fs/f2fs/super.c:4149
>   mount_bdev+0x1b8/0x210 fs/super.c:1400
>   f2fs_mount+0x44/0x58 fs/f2fs/super.c:4512
>   legacy_get_tree+0x30/0x74 fs/fs_context.c:610
>   vfs_get_tree+0x40/0x140 fs/super.c:1530
>   do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
>   path_mount+0x358/0x914 fs/namespace.c:3370
>   do_mount fs/namespace.c:3383 [inline]
>   __do_sys_mount fs/namespace.c:3591 [inline]
>   __se_sys_mount fs/namespace.c:3568 [inline]
>   __arm64_sys_mount+0x2f8/0x408 fs/namespace.c:3568
> 
> Cc: <stable@kernel.org>
> Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
>   mm/slub.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 862dbd9af4f5..e6f3727b9ad2 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5890,7 +5890,8 @@ static char *create_unique_id(struct kmem_cache *s)
>   	char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);

Hi,

looks that ID_STR_LENGTH could even be reduced to 32 or 16.

The 2nd BUG_ON at the end of the function could certainly be just 
removed as well or remplaced by a:
    	if (p > name + ID_STR_LENGTH - 1) {
		kfree(name);
		return -E<something>;
	}

Just my 2c,

CJ

>   	char *p = name;
>   
> -	BUG_ON(!name);
> +	if (!name)
> +		return ERR_PTR(-ENOMEM);
>   
>   	*p++ = ':';
>   	/*
> @@ -5948,6 +5949,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
>   		 * for the symlinks.
>   		 */
>   		name = create_unique_id(s);
> +		if (IS_ERR(name))
> +			return PTR_ERR(name);
>   	}
>   
>   	s->kobj.kset = kset;

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Matthew Wilcox]

On Thu, Sep 08, 2022 at 11:25:08PM +0200, Vlastimil Babka (SUSE) wrote:
> > I tend to agree with you. A mount operation shouldn’t panic the
> > kernel.
> 
> Hmm kmalloc(64) shouldn't normally due that due to the the underlying page
> allocation falling into the "too small to fail" category, wonder if
> syzkaller was doing anything special here?

Here's the repro:

https://syzkaller.appspot.com/x/repro.c?x=17cd7fa3080000

you can see it does:

  fd = open("/proc/thread-self/fail-nth", O_RDWR);
  if (fd == -1)
    exit(1);
  char buf[16];
  sprintf(buf, "%d", nth);
  if (write(fd, buf, strlen(buf)) != (ssize_t)strlen(buf))

so this is the kind of stupid nitpicky bug that we shouldn't be
reporting, let alone fixing, IMO.

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Vlastimil Babka (SUSE)]

On 9/9/22 22:06, Matthew Wilcox wrote:
> On Thu, Sep 08, 2022 at 11:25:08PM +0200, Vlastimil Babka (SUSE) wrote:
>> > I tend to agree with you. A mount operation shouldn’t panic the
>> > kernel.
>> 
>> Hmm kmalloc(64) shouldn't normally due that due to the the underlying page
>> allocation falling into the "too small to fail" category, wonder if
>> syzkaller was doing anything special here?
> 
> Here's the repro:
> 
> https://syzkaller.appspot.com/x/repro.c?x=17cd7fa3080000
> 
> you can see it does:
> 
>   fd = open("/proc/thread-self/fail-nth", O_RDWR);
>   if (fd == -1)
>     exit(1);
>   char buf[16];
>   sprintf(buf, "%d", nth);
>   if (write(fd, buf, strlen(buf)) != (ssize_t)strlen(buf))
> 
> so this is the kind of stupid nitpicky bug that we shouldn't be
> reporting, let alone fixing, IMO.

Ah, thanks.

Well I'm ok with eventually removing all such BUG_ONs including what
Christophe Jaillet suggested, but it certainly isn't urgent nor deserves Cc:
stable then.

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Chao Yu]

On 2022/9/9 5:25, Vlastimil Babka (SUSE) wrote:
> On 8/31/22 05:09, Muchun Song wrote:
>>
>>
>>> On Aug 30, 2022, at 22:10, Chao Yu <chao@kernel.org> wrote:
> 
> Please use scripts/get_maintainer.pl next time, I could have missed this.

Oh, my bad, will Cc all maintainers next time.

Thanks,

> 
>>> From: Chao Yu <chao.yu@oppo.com>
>>>
>>> In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
>>> out-of-memory, if it fails, return errno correctly rather than
>>> triggering panic via BUG_ON();
>>
>> I tend to agree with you. A mount operation shouldn’t panic the
>> kernel.
> 
> Hmm kmalloc(64) shouldn't normally due that due to the the underlying page
> allocation falling into the "too small to fail" category, wonder if
> syzkaller was doing anything special here?
> 
> But yeah we should get rid of all BUG_ONs eventually, just not sure if
> stable@ is needed here.
> 
>>>
>>> kernel BUG at mm/slub.c:5893!
>>> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>>>
>>> Call trace:
>>> sysfs_slab_add+0x258/0x260 mm/slub.c:5973
>>> __kmem_cache_create+0x60/0x118 mm/slub.c:4899
>>> create_cache mm/slab_common.c:229 [inline]
>>> kmem_cache_create_usercopy+0x19c/0x31c mm/slab_common.c:335
>>> kmem_cache_create+0x1c/0x28 mm/slab_common.c:390
>>> f2fs_kmem_cache_create fs/f2fs/f2fs.h:2766 [inline]
>>> f2fs_init_xattr_caches+0x78/0xb4 fs/f2fs/xattr.c:808
>>> f2fs_fill_super+0x1050/0x1e0c fs/f2fs/super.c:4149
>>> mount_bdev+0x1b8/0x210 fs/super.c:1400
>>> f2fs_mount+0x44/0x58 fs/f2fs/super.c:4512
>>> legacy_get_tree+0x30/0x74 fs/fs_context.c:610
>>> vfs_get_tree+0x40/0x140 fs/super.c:1530
>>> do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
>>> path_mount+0x358/0x914 fs/namespace.c:3370
>>> do_mount fs/namespace.c:3383 [inline]
>>> __do_sys_mount fs/namespace.c:3591 [inline]
>>> __se_sys_mount fs/namespace.c:3568 [inline]
>>> __arm64_sys_mount+0x2f8/0x408 fs/namespace.c:3568
>>>
>>> Cc: <stable@kernel.org>
>>> Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com
>>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>>
>> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
>>
>> Thanks.
>>
>>
> 

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Chao Yu]

On 2022/9/10 0:47, Christophe JAILLET wrote:
> Le 30/08/2022 à 16:10, Chao Yu a écrit :
>> From: Chao Yu <chao.yu@oppo.com>
>>
>> In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
>> out-of-memory, if it fails, return errno correctly rather than
>> triggering panic via BUG_ON();
>>
>> kernel BUG at mm/slub.c:5893!
>> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>>
>> Call trace:
>>   sysfs_slab_add+0x258/0x260 mm/slub.c:5973
>>   __kmem_cache_create+0x60/0x118 mm/slub.c:4899
>>   create_cache mm/slab_common.c:229 [inline]
>>   kmem_cache_create_usercopy+0x19c/0x31c mm/slab_common.c:335
>>   kmem_cache_create+0x1c/0x28 mm/slab_common.c:390
>>   f2fs_kmem_cache_create fs/f2fs/f2fs.h:2766 [inline]
>>   f2fs_init_xattr_caches+0x78/0xb4 fs/f2fs/xattr.c:808
>>   f2fs_fill_super+0x1050/0x1e0c fs/f2fs/super.c:4149
>>   mount_bdev+0x1b8/0x210 fs/super.c:1400
>>   f2fs_mount+0x44/0x58 fs/f2fs/super.c:4512
>>   legacy_get_tree+0x30/0x74 fs/fs_context.c:610
>>   vfs_get_tree+0x40/0x140 fs/super.c:1530
>>   do_new_mount+0x1dc/0x4e4 fs/namespace.c:3040
>>   path_mount+0x358/0x914 fs/namespace.c:3370
>>   do_mount fs/namespace.c:3383 [inline]
>>   __do_sys_mount fs/namespace.c:3591 [inline]
>>   __se_sys_mount fs/namespace.c:3568 [inline]
>>   __arm64_sys_mount+0x2f8/0x408 fs/namespace.c:3568
>>
>> Cc: <stable@kernel.org>
>> Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com
>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>> ---
>>   mm/slub.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 862dbd9af4f5..e6f3727b9ad2 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -5890,7 +5890,8 @@ static char *create_unique_id(struct kmem_cache *s)
>>       char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
> 
> Hi,
> 
> looks that ID_STR_LENGTH could even be reduced to 32 or 16.
> 
> The 2nd BUG_ON at the end of the function could certainly be just removed as well or remplaced by a:
>         if (p > name + ID_STR_LENGTH - 1) {
>          kfree(name);
>          return -E<something>;
>      }

Hi Christophe, Vlastimil,

Should I include this in v3? or may be in another patch?

Thanks,

> 
> Just my 2c,
> 
> CJ
> 
>>       char *p = name;
>> -    BUG_ON(!name);
>> +    if (!name)
>> +        return ERR_PTR(-ENOMEM);
>>       *p++ = ':';
>>       /*
>> @@ -5948,6 +5949,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
>>            * for the symlinks.
>>            */
>>           name = create_unique_id(s);
>> +        if (IS_ERR(name))
>> +            return PTR_ERR(name);
>>       }
>>       s->kobj.kset = kset;
> 

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Marion & Christophe JAILLET]

Le 13/09/2022 à 05:42, Chao Yu a écrit :
> On 2022/9/10 0:47, Christophe JAILLET wrote:
>> Le 30/08/2022 à 16:10, Chao Yu a écrit :
>>> From: Chao Yu <chao.yu@oppo.com>
>>>
>>> In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
>>> out-of-memory, if it fails, return errno correctly rather than
>>> triggering panic via BUG_ON();
>>>
>>> kernel BUG at mm/slub.c:5893!
>>> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>>>
>>> Call trace:
[...]
>>>
>>> Cc: <stable@kernel.org>
>>> Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com
>>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>>> ---
>>>   mm/slub.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/slub.c b/mm/slub.c
>>> index 862dbd9af4f5..e6f3727b9ad2 100644
>>> --- a/mm/slub.c
>>> +++ b/mm/slub.c
>>> @@ -5890,7 +5890,8 @@ static char *create_unique_id(struct 
>>> kmem_cache *s)
>>>       char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
>>
>> Hi,
>>
>> looks that ID_STR_LENGTH could even be reduced to 32 or 16.
>>
>> The 2nd BUG_ON at the end of the function could certainly be just 
>> removed as well or remplaced by a:
>>         if (p > name + ID_STR_LENGTH - 1) {
>>          kfree(name);
>>          return -E<something>;
>>      }
>
> Hi Christophe, Vlastimil,
>
> Should I include this in v3? or may be in another patch?

Hi,

My own preference would be for 3 patches.

Yours, as-is.
It fixes a specific issue spotted by syzbot.

Another one for removing a BUG_ON() (that, IIUC can't happen!)
Mostly a clean-up or a good practice in order to remove BUG_ON() from 
the kernel we it can be handled another way.

Eventually a 3rd one for reducing ID_STR_LENGTH.
I guess that it is safe to reduce it to 32 or 16, but the impact on RL 
would be so small, that I wonder if it worth proposing it.

Just my 2c,

CJ



>
> Thanks,
>
>>
>> Just my 2c,
>>
>> CJ
>>
>>>       char *p = name;
>>> -    BUG_ON(!name);
>>> +    if (!name)
>>> +        return ERR_PTR(-ENOMEM);
>>>       *p++ = ':';
>>>       /*
>>> @@ -5948,6 +5949,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
>>>            * for the symlinks.
>>>            */
>>>           name = create_unique_id(s);
>>> +        if (IS_ERR(name))
>>> +            return PTR_ERR(name);
>>>       }
>>>       s->kobj.kset = kset;
>>

Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails

[Vlastimil Babka (SUSE)]

On 9/13/22 07:26, Marion & Christophe JAILLET wrote:
> 
> Le 13/09/2022 à 05:42, Chao Yu a écrit :
>> On 2022/9/10 0:47, Christophe JAILLET wrote:
>>> Le 30/08/2022 à 16:10, Chao Yu a écrit :
>>>> From: Chao Yu <chao.yu@oppo.com>
>>>>
>>>> In create_unique_id(), kmalloc(, GFP_KERNEL) can fail due to
>>>> out-of-memory, if it fails, return errno correctly rather than
>>>> triggering panic via BUG_ON();
>>>>
>>>> kernel BUG at mm/slub.c:5893!
>>>> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>>>>
>>>> Call trace:
> [...]
>>>>
>>>> Cc: <stable@kernel.org>
>>>> Reported-by: syzbot+81684812ea68216e08c5@syzkaller.appspotmail.com
>>>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>>>> ---
>>>>   mm/slub.c | 5 ++++-
>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/slub.c b/mm/slub.c
>>>> index 862dbd9af4f5..e6f3727b9ad2 100644
>>>> --- a/mm/slub.c
>>>> +++ b/mm/slub.c
>>>> @@ -5890,7 +5890,8 @@ static char *create_unique_id(struct kmem_cache *s)
>>>>       char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
>>>
>>> Hi,
>>>
>>> looks that ID_STR_LENGTH could even be reduced to 32 or 16.
>>>
>>> The 2nd BUG_ON at the end of the function could certainly be just removed
>>> as well or remplaced by a:
>>>         if (p > name + ID_STR_LENGTH - 1) {
>>>          kfree(name);
>>>          return -E<something>;
>>>      }
>>
>> Hi Christophe, Vlastimil,
>>
>> Should I include this in v3? or may be in another patch?
> 
> Hi,
> 
> My own preference would be for 3 patches.
> 
> Yours, as-is.
> It fixes a specific issue spotted by syzbot.

Yeah and it's already in git.

> Another one for removing a BUG_ON() (that, IIUC can't happen!)
> Mostly a clean-up or a good practice in order to remove BUG_ON() from the
> kernel we it can be handled another way.
> 
> Eventually a 3rd one for reducing ID_STR_LENGTH.
> I guess that it is safe to reduce it to 32 or 16, but the impact on RL would
> be so small, that I wonder if it worth proposing it.

Agree. Doing 2+3 in the same patch would be OK with me too.