Re: [PATCH v4 3/3] mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()

[Vlastimil Babka]

On 9/13/24 17:00, Hyeonggon Yoo wrote:
> 
> 
>> On Sep 13, 2024, at 11:10 PM, Vlastimil Babka <vbabka@xxxxxxx> wrote:
>> 
>> On 9/6/24 10:10, Jinjie Ruan wrote:
>>> 
>>> 
>>> On 2024/9/5 21:59, Hyeonggon Yoo wrote:
>>>> On Thu, Sep 5, 2024 at 12:41 PM Jinjie Ruan <ruanjinjie@xxxxxxxxxx> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 2022/11/12 19:46, Liu Shixin wrote:
>>>>>> There is a memory leak of kobj->name in sysfs_slab_add():
>>>>>> 
>>>>>> unreferenced object 0xffff88817e446440 (size 32):
>>>>>>   comm "insmod", pid 4085, jiffies 4296564501 (age 126.272s)
>>>>>>   hex dump (first 32 bytes):
>>>>>>     75 62 69 66 73 5f 69 6e 6f 64 65 5f 73 6c 61 62  ubifs_inode_slab
>>>>>>     00 65 44 7e 81 88 ff ff 00 00 00 00 00 00 00 00  .eD~............
>>>>>>   backtrace:
>>>>>>     [<000000005b30fbbd>] __kmalloc_node_track_caller+0x4e/0x150
>>>>>>     [<000000002f70da0c>] kstrdup_const+0x4b/0x80
>>>>>>     [<00000000c6712c61>] kobject_set_name_vargs+0x2f/0xb0
>>>>>>     [<00000000b151218e>] kobject_init_and_add+0xb0/0x120
>>>>>>     [<00000000e56a4cf5>] sysfs_slab_add+0x17d/0x220
>>>>>>     [<000000009326fd57>] __kmem_cache_create+0x406/0x590
>>>>>>     [<00000000dde33cff>] kmem_cache_create_usercopy+0x1fc/0x300
>>>>>>     [<00000000fe90cedb>] kmem_cache_create+0x12/0x20
>>>>>>     [<000000007a6531c8>] 0xffffffffa02d802d
>>>>>>     [<000000000e3b13c7>] do_one_initcall+0x87/0x2a0
>>>>>>     [<00000000995ecdcf>] do_init_module+0xdf/0x320
>>>>>>     [<000000008821941f>] load_module+0x2f98/0x3330
>>>>>>     [<00000000ef51efa4>] __do_sys_finit_module+0x113/0x1b0
>>>>>>     [<000000009339fbce>] do_syscall_64+0x35/0x80
>>>>>>     [<000000006b7f2033>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
>>>>> 
>>>>> 
>>>>> Hi，every one,
>>>> 
>>>> Hi.
>>>> 
>>>>> I found the same problem and it solve this problem with the patch, is
>>>>> there any plan to update the patch and solve it.
>> 
>> Hmm looks like back in 2022, Hyeonggon had some feedback to the series which
>> was not answered and then it got forgotten. Feel free to take over and send
>> an updated version.
> 
> 
> I was thinking of what the fix would be with my feedback,
> and I still think passing different kobj_type (with a dummy release function) for early kmem_caches
> will be a more appropriate approach.
> 
> However, there is one concern: people that wrote kobject.rst might not like it :(
> 
> in Documentation/core-api/kobject.rst:
>> One important point cannot be overstated: every kobject must have a release() method,
>> and the kobject must persist (in a consistent state) until that method is called. If these constraints are not met,
>> the code is flawed. Note that the kernel will warn you if you forget to provide a release() method.
>> Do not try to get rid of this warning by providing an "empty" release function.
> 
> But obviously we don't want to release caches just because the kernel failed to add it to sysfs.
> 
>>>> What kernel version do you use,
>>> 
>>> 6.11.0-rc6
>>> 
>>>> and when do you encounter it or how do you reproduce it?
>>> 
>>> Hi, Hyeonggon,
>>> 
>>> Thank you, I encounter it when doing inject fault test while modprobe
>>> amdgpu.ko.
>> 
>> So I wonder where's the problem that results in kobject_init_and_add()
>> failing. If it's genuinely duplicate name as commit 80da026a8e5d suggests,
>> 6.12-rc1 will have a warning to prevent that. Delayed destruction of
>> SLAB_TYPESAFE_BY_RCU caches should also no longer happen with 6.12-rc1. So
>> worth retrying with that and if it's still failing, we should look at the
>> root cause perhaps.
> 
> I thought it was because the memory allocation for a name string failed due to fault injection?

Well in any case 6.12-rc1 introduced a new one, fixed by:
https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.12-rc1/fixes&id=77ced98f0f03fdc196561d1afbe652899c318073

So once that's mainline, we can see if anything remains

>> 
>>>> 
>>>> --
>>>> Hyeonggon
> 
>