* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-08-30 14:10 [PATCH] mm/slub: fix to return errno if kmalloc() fails Chao Yu
@ 2022-08-31 3:09 ` Muchun Song
2022-09-08 21:25 ` Vlastimil Babka (SUSE)
2022-08-31 13:33 ` Hyeonggon Yoo
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Muchun Song @ 2022-08-31 3:09 UTC (permalink / raw)
To: Chao Yu
Cc: Linux MM, Andrew Morton, linux-kernel, jaegeuk, Chao Yu, stable,
syzbot+81684812ea68216e08c5
> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-08-31 3:09 ` Muchun Song
@ 2022-09-08 21:25 ` Vlastimil Babka (SUSE)
2022-09-09 20:06 ` Matthew Wilcox
2022-09-13 3:27 ` Chao Yu
0 siblings, 2 replies; 12+ messages in thread
From: Vlastimil Babka (SUSE) @ 2022-09-08 21:25 UTC (permalink / raw)
To: Muchun Song, Chao Yu
Cc: Linux MM, Andrew Morton, linux-kernel, jaegeuk, Chao Yu, stable,
syzbot+81684812ea68216e08c5, David Rientjes, Hyeonggon Yoo,
Christoph Lameter
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.
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-09-08 21:25 ` Vlastimil Babka (SUSE)
@ 2022-09-09 20:06 ` Matthew Wilcox
2022-09-09 20:21 ` Vlastimil Babka (SUSE)
2022-09-13 3:27 ` Chao Yu
1 sibling, 1 reply; 12+ messages in thread
From: Matthew Wilcox @ 2022-09-09 20:06 UTC (permalink / raw)
To: Vlastimil Babka (SUSE)
Cc: Muchun Song, Chao Yu, Linux MM, Andrew Morton, linux-kernel,
jaegeuk, Chao Yu, stable, syzbot+81684812ea68216e08c5,
David Rientjes, Hyeonggon Yoo, Christoph Lameter
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-09-09 20:06 ` Matthew Wilcox
@ 2022-09-09 20:21 ` Vlastimil Babka (SUSE)
0 siblings, 0 replies; 12+ messages in thread
From: Vlastimil Babka (SUSE) @ 2022-09-09 20:21 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Muchun Song, Chao Yu, Linux MM, Andrew Morton, linux-kernel,
jaegeuk, Chao Yu, stable, syzbot+81684812ea68216e08c5,
David Rientjes, Hyeonggon Yoo, Christoph Lameter,
Christophe JAILLET
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-09-08 21:25 ` Vlastimil Babka (SUSE)
2022-09-09 20:06 ` Matthew Wilcox
@ 2022-09-13 3:27 ` Chao Yu
1 sibling, 0 replies; 12+ messages in thread
From: Chao Yu @ 2022-09-13 3:27 UTC (permalink / raw)
To: Vlastimil Babka (SUSE), Muchun Song
Cc: Linux MM, Andrew Morton, linux-kernel, jaegeuk, Chao Yu, stable,
syzbot+81684812ea68216e08c5, David Rientjes, Hyeonggon Yoo,
Christoph Lameter
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.
>>
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-08-30 14:10 [PATCH] mm/slub: fix to return errno if kmalloc() fails Chao Yu
2022-08-31 3:09 ` Muchun Song
@ 2022-08-31 13:33 ` Hyeonggon Yoo
2022-09-06 21:33 ` David Rientjes
2022-09-09 16:47 ` Christophe JAILLET
3 siblings, 0 replies; 12+ messages in thread
From: Hyeonggon Yoo @ 2022-08-31 13:33 UTC (permalink / raw)
To: Chao Yu
Cc: linux-mm, akpm, linux-kernel, jaegeuk, Chao Yu, stable,
syzbot+81684812ea68216e08c5
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-08-30 14:10 [PATCH] mm/slub: fix to return errno if kmalloc() fails Chao Yu
2022-08-31 3:09 ` Muchun Song
2022-08-31 13:33 ` Hyeonggon Yoo
@ 2022-09-06 21:33 ` David Rientjes
2022-09-09 16:47 ` Christophe JAILLET
3 siblings, 0 replies; 12+ messages in thread
From: David Rientjes @ 2022-09-06 21:33 UTC (permalink / raw)
To: Chao Yu
Cc: linux-mm, akpm, linux-kernel, jaegeuk, Chao Yu, stable,
syzbot+81684812ea68216e08c5
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-08-30 14:10 [PATCH] mm/slub: fix to return errno if kmalloc() fails Chao Yu
` (2 preceding siblings ...)
2022-09-06 21:33 ` David Rientjes
@ 2022-09-09 16:47 ` Christophe JAILLET
2022-09-13 3:42 ` Chao Yu
3 siblings, 1 reply; 12+ messages in thread
From: Christophe JAILLET @ 2022-09-09 16:47 UTC (permalink / raw)
To: Chao Yu, linux-mm
Cc: akpm, linux-kernel, jaegeuk, Chao Yu, vbabka, muchun.song
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;
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-09-09 16:47 ` Christophe JAILLET
@ 2022-09-13 3:42 ` Chao Yu
2022-09-13 5:26 ` Marion & Christophe JAILLET
0 siblings, 1 reply; 12+ messages in thread
From: Chao Yu @ 2022-09-13 3:42 UTC (permalink / raw)
To: Christophe JAILLET, Vlastimil Babka (SUSE), linux-mm
Cc: akpm, linux-kernel, Chao Yu, muchun.song
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;
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-09-13 3:42 ` Chao Yu
@ 2022-09-13 5:26 ` Marion & Christophe JAILLET
2022-09-16 22:58 ` Vlastimil Babka (SUSE)
0 siblings, 1 reply; 12+ messages in thread
From: Marion & Christophe JAILLET @ 2022-09-13 5:26 UTC (permalink / raw)
To: Chao Yu, Vlastimil Babka (SUSE), linux-mm
Cc: akpm, linux-kernel, Chao Yu, muchun.song
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;
>>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] mm/slub: fix to return errno if kmalloc() fails
2022-09-13 5:26 ` Marion & Christophe JAILLET
@ 2022-09-16 22:58 ` Vlastimil Babka (SUSE)
0 siblings, 0 replies; 12+ messages in thread
From: Vlastimil Babka (SUSE) @ 2022-09-16 22:58 UTC (permalink / raw)
To: Marion & Christophe JAILLET, Chao Yu, linux-mm
Cc: akpm, linux-kernel, Chao Yu, muchun.song
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.
^ permalink raw reply [flat|nested] 12+ messages in thread