Re: [PATCH net-next] mptcp: pm: Fix undefined behavior in mptcp_remove_anno_list_by_saddr()

[Matthieu Baerts]

Hi Thorsten,

On 25/03/2025 13:51, Thorsten Blum wrote:
> On 25. Mar 2025, at 13:30, Jakub Kicinski wrote:
>> On Tue, 25 Mar 2025 12:33:11 +0100 Thorsten Blum wrote:
>>> On 25. Mar 2025, at 12:06, Thorsten Blum wrote:
>>>>
>>>> Commit e4c28e3d5c090 ("mptcp: pm: move generic PM helpers to pm.c")
>>>> removed a necessary if-check, leading to undefined behavior because
>>>> the freed pointer is subsequently returned from the function.
>>>>
>>>> Reintroduce the if-check to fix this and add a local return variable to
>>>> prevent further checkpatch warnings, which originally led to the removal
>>>> of the if-check.
>>>>
>>>> Fixes: e4c28e3d5c090 ("mptcp: pm: move generic PM helpers to pm.c")
>>>> Signed-off-by: Thorsten Blum <thorsten.blum@xxxxxxxxx>
>>>> ---  
>>>
>>> Never mind, technically it's not actually undefined behavior because of
>>> the implicit bool conversion, but returning a freed pointer still seems
>>> confusing.
>>
>> CCing the list back in.
> 
> Thanks!
> 
> The change imo still makes sense, but the commit message should be
> updated.

Yes, I think it still makes sense, and I understand the confusion.
Another reason to change that is to avoid future issues when kfree()
will be able to set variables to NULL [1].

Instead of re-introducing the if-statement, why not assigning 'ret' to
'entry'?

  entry = mptcp_pm_del_add_timer(...);
  ret = entry; // or assign it at the previous line? ret = entry = (...)
  kfree(entry);

[1] https://lore.kernel.org/20250321202620.work.175-kees@xxxxxxxxxx

> I'll submit a new patch for after the merge window.
Thank you!

An alternative if you want to send it before is to rebase it on top of
the MPTCP "export" branch, and to send it only to the MPTCP ML. I can
apply the new version in our tree, and send it to Netdev later with
other patches.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.