| Message ID | 20250325110639.49399-2-thorsten.blum@linux.dev (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [net-next] mptcp: pm: Fix undefined behavior in mptcp_remove_anno_list_by_saddr() | expand |
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@linux.dev> >>> --- >> >> 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. I'll submit a new patch for after the merge window.
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@linux.dev> >>>> --- >>> >>> 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@kernel.org > 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
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index 18b19dbccbba..5a6d5e4897dd 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -151,10 +151,15 @@ bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk, const struct mptcp_addr_info *addr) { struct mptcp_pm_add_entry *entry; + bool ret = false; entry = mptcp_pm_del_add_timer(msk, addr, false); - kfree(entry); - return entry; + if (entry) { + ret = true; + kfree(entry); + } + + return ret; } bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk)
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@linux.dev> --- net/mptcp/pm.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)