On 10/16/24 16:08, Muchun Song wrote:
>
>
>> On Oct 16, 2024, at 19:43, Vlastimil Babka <vbabka@xxxxxxx> wrote:
>> On 10/16/24 04:21, Muchun Song wrote:
>>>
>>>
>>>> On Oct 16, 2024, at 09:25, chenridong <chenridong@xxxxxxxxxx> wrote:
>>>> On 2024/10/15 14:55, Anshuman Khandual wrote:
>>>>> On 10/14/24 16:59, Kirill A. Shutemov wrote:
>>>>>> On Mon, Oct 14, 2024 at 03:23:36AM +0000, Chen Ridong wrote:
>>>>>>> From: Chen Ridong <chenridong@xxxxxxxxxx>
>>>>>>> A memleak was found as bellow:
>>>>>>> unreferenced object 0xffff8881010d2a80 (size 32):
>>>>>>> comm "mkdir", pid 1559, jiffies 4294932666
>>>>>>> hex dump (first 32 bytes):
>>>>>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>>>>>>> 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 @...............
>>>>>>> backtrace (crc 2e7ef6fa):
>>>>>>> [<ffffffff81372754>] __kmalloc_node_noprof+0x394/0x470
>>>>>>> [<ffffffff813024ab>] alloc_shrinker_info+0x7b/0x1a0
>>>>>>> [<ffffffff813b526a>] mem_cgroup_css_online+0x11a/0x3b0
>>>>>>> [<ffffffff81198dd9>] online_css+0x29/0xa0
>>>>>>> [<ffffffff811a243d>] cgroup_apply_control_enable+0x20d/0x360
>>>>>>> [<ffffffff811a5728>] cgroup_mkdir+0x168/0x5f0
>>>>>>> [<ffffffff8148543e>] kernfs_iop_mkdir+0x5e/0x90
>>>>>>> [<ffffffff813dbb24>] vfs_mkdir+0x144/0x220
>>>>>>> [<ffffffff813e1c97>] do_mkdirat+0x87/0x130
>>>>>>> [<ffffffff813e1de9>] __x64_sys_mkdir+0x49/0x70
>>>>>>> [<ffffffff81f8c928>] do_syscall_64+0x68/0x140
>>>>>>> [<ffffffff8200012f>] entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>>>>> In the alloc_shrinker_info function, when shrinker_unit_alloc return
>>>>>>> err, the info won't be freed. Just fix it.
>>>>>>> Fixes: 307bececcd12 ("mm: shrinker: add a secondary array for shrinker_info::{map, nr_deferred}")
>>>>>>> Signed-off-by: Chen Ridong <chenridong@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> mm/shrinker.c | 1 +
>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>> diff --git a/mm/shrinker.c b/mm/shrinker.c
>>>>>>> index dc5d2a6fcfc4..92270413190d 100644
>>>>>>> --- a/mm/shrinker.c
>>>>>>> +++ b/mm/shrinker.c
>>>>>>> @@ -97,6 +97,7 @@ int alloc_shrinker_info(struct mem_cgroup *memcg)
>>>>>>> err:
>>>>>>> mutex_unlock(&shrinker_mutex);
>>>>>>> + kvfree(info);
>>>>>>> free_shrinker_info(memcg);
>>>>>>> return -ENOMEM;
>>>>>>> }
>>>>>> NAK. If in the future there going to one more error case after
>>>>>> rcu_assign_pointer() we will end up with double free.
>>>>>> This should be safer:
>>>>>> diff --git a/mm/shrinker.c b/mm/shrinker.c
>>>>>> index dc5d2a6fcfc4..763fd556bc7d 100644
>>>>>> --- a/mm/shrinker.c
>>>>>> +++ b/mm/shrinker.c
>>>>>> @@ -87,8 +87,10 @@ int alloc_shrinker_info(struct mem_cgroup *memcg)
>>>>>> if (!info)
>>>>>> goto err;
>>>>>> info->map_nr_max = shrinker_nr_max;
>>>>>> - if (shrinker_unit_alloc(info, NULL, nid))
>>>>>> + if (shrinker_unit_alloc(info, NULL, nid)) {
>>>>>> + kvfree(info);
>>>>>> goto err;
>>>>>> + }
>>>>>> rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, info);
>>>>>> }
>>>>>> mutex_unlock(&shrinker_mutex);
>>>>> Agreed, this is what I mentioned earlier as well.
>>>>> ------------------------------------------------------------------
>>>>> I guess kvfree() should be called just after shrinker_unit_alloc()
>>>>> fails but before calling into "goto err"
>>>>> ------------------------------------------------------------------
>>>> After discussion, it seems that v1 is acceptable.
>>>> Hi, Muchun, do you have any other opinions?
>>>
>>> I insist on my opinion, not mixing two different approaches
>>> to do release resources.
>>
>> So instead we mix the cleanup of the whole function with the cleanup of what
>> is effectively a per-iteration temporary variable?
>>
>> The fact there was already a confusion in this thread about whether it's
>> safe and relies on kvfree(NULL) to be a no-op, should be a hint.
>
> Yes. I think someone is confused about my opinion.
> I don’t care about whether we should apply this hit.
> If we think the hint is tricky, we could add another
> label to fix it like I suggested previously. Because
> we already use goto-based approaches to
> cleanup the resources, why not keeping
> consistent?
I think we're rather pragmatic than striving to be consistent for the sake
of consistency. goto is not the nicest thing in the world, but we (unlike
other projects) use it where it makes sense to avoid if/else nesting
explosion. Here for the info it's not the most pragmatic option.
> It will be easier for us to add a new
> "if" statement and handle the failure case in the future.
Let's not overengineer things for hypothetical future.
> For example, if we use his v1 proposal, we should do
> the cleanups again for info. But for goto-based
> version, we just add another label to do the
> cleanups and go to the new label for failure case. goto-based fix is what I insisted on. I copied my previous suggested fix here to clarify my opinion.
Again, info is a loop-iteration-local variable, v1 fix making it truly local
is the way to go. If there are further cleanups added in the loop itself in
the future, they could hopefully keep being local to the loop as well.
Cleanup of info outside the loop iteration is breaking its real scope.
>
> --- a/mm/shrinker.c
> +++ b/mm/shrinker.c
> @@ -88,13 +88,14 @@ int alloc_shrinker_info(struct mem_cgroup *memcg)
> goto err;
> info->map_nr_max = shrinker_nr_max;
> if (shrinker_unit_alloc(info, NULL, nid))
> - goto err;
> + goto free;
> rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, info);
> }
> mutex_unlock(&shrinker_mutex);
>
> return ret;
> -
> +free:
> + kvfree(info);
> err:
> mutex_unlock(&shrinker_mutex);
> free_shrinker_info(memcg);
>
> Muchun,
> Thanks.
>
>>
>> So no, I a gree with Kirill and others. Ideally the fix would also move the
>> declaration of info into the for loop to make its scope more obvious.
>>
>>> Thanks.
>>>
>>>> Best regards,
>>>> Ridong
