| Message ID | 20250325163251.135510-2-thorsten.blum@linux.dev (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 5172fc837d0c938e5d73a0cf700e1566646fb63f |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [v2] mptcp: pm: Return local variable instead of freed pointer | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
| matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | warning | Unstable: 1 failed test(s): bpftest_test_progs-no_alu32_mptcp |
Hi Thorsten, On 25/03/2025 17:32, Thorsten Blum wrote: > Commit e4c28e3d5c090 ("mptcp: pm: move generic PM helpers to pm.c") > removed an unnecessary if-check, which resulted in returning a freed > pointer. > > This still works due to the implicit boolean conversion when returning > the freed pointer from mptcp_remove_anno_list_by_saddr(), but it can be > confusing and potentially error-prone. To improve clarity, add a local > variable to explicitly return a boolean value instead. Thank you, the patch looks good to me! Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> It looks like the CI had some issues to apply your patch [1]. Strange, it was fine for me to apply it with 'b4 shazam'. I manually sent it to the CI, I will wait for it to validate it before applying it. https://patchew.org/MPTCP/20250325163251.135510-2-thorsten.blum@linux.dev/logs/git/ > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev> > --- > Changes in v2: > - Remove the if-check again as suggested by Matthieu Baerts > - Rephrase the commit message > - Link to v1: https://lore.kernel.org/r/20250325110639.49399-2-thorsten.blum@linux.dev/ > --- > net/mptcp/pm.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c > index 18b19dbccbba..f4e32187e2c3 100644 > --- a/net/mptcp/pm.c > +++ b/net/mptcp/pm.c > @@ -151,10 +151,12 @@ 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; > > entry = mptcp_pm_del_add_timer(msk, addr, false); > + ret = entry; > kfree(entry); > - return entry; > + return ret; (detail: while at it, we can probably add a new line before the 'return'. I can add that when applying the patch) > } > > bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk) > Cheers, Matt
On 25. Mar 2025, at 19:28, Matthieu Baerts wrote: > > It looks like the CI had some issues to apply your patch [1]. Strange, > it was fine for me to apply it with 'b4 shazam'. I manually sent it to > the CI, I will wait for it to validate it before applying it. > > https://patchew.org/MPTCP/20250325163251.135510-2-thorsten.blum@linux.dev/logs/git/ I get a 404 Not Found when opening the link. Weird, I made sure the patch applies to mptcp/export. Thanks, Thorsten
On 25/03/2025 19:45, Thorsten Blum wrote: > On 25. Mar 2025, at 19:28, Matthieu Baerts wrote: >> >> It looks like the CI had some issues to apply your patch [1]. Strange, >> it was fine for me to apply it with 'b4 shazam'. I manually sent it to >> the CI, I will wait for it to validate it before applying it. >> >> https://patchew.org/MPTCP/20250325163251.135510-2-thorsten.blum@linux.dev/logs/git/ > > I get a 404 Not Found when opening the link. Better with this one, then click on "apply log"? https://patchew.org/MPTCP/20250325163251.135510-2-thorsten.blum@linux.dev/ Here is the log: > Switched to a new branch '20250325163251.135510-2-thorsten.blum@linux.dev' > Applying: mptcp: pm: Return local variable instead of freed pointer > error: corrupt patch at line 27 > error: could not build fake ancestor > hint: Use 'git am --show-current-patch=diff' to see the failed patch > Patch failed at 0001 mptcp: pm: Return local variable instead of freed pointer > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". I didn't have that on my side. > Weird, I made sure the patch applies to mptcp/export. Yes, strange. There was an issue with the v1 as well apparently: https://patchew.org/MPTCP/20250325110639.49399-2-thorsten.blum@linux.dev/ I will check later if it was not an issue with Patchew. Cheers, Matt
Hi Thorsten, Thank you for your modifications, that's great! Our CI did some validations and here is its report: - KVM Validation: normal: Success! ✅ - KVM Validation: debug: Success! ✅ - KVM Validation: btf-normal (only bpftest_all): Success! ✅ - KVM Validation: btf-debug (only bpftest_all): Unstable: 1 failed test(s): bpftest_test_progs-no_alu32_mptcp
Hi Thorsten, On 25/03/2025 17:32, Thorsten Blum wrote: > Commit e4c28e3d5c090 ("mptcp: pm: move generic PM helpers to pm.c") > removed an unnecessary if-check, which resulted in returning a freed > pointer. > > This still works due to the implicit boolean conversion when returning > the freed pointer from mptcp_remove_anno_list_by_saddr(), but it can be > confusing and potentially error-prone. To improve clarity, add a local > variable to explicitly return a boolean value instead. Thank you for your patch! Now in our tree (feat. for net-next): New patches for t/upstream: - 5172fc837d0c: mptcp: pm: Return local variable instead of freed pointer - Results: c0c93d7a6bc6..7723f461d379 (export) Tests are now in progress: - export: https://github.com/multipath-tcp/mptcp_net-next/commit/5bdccde61635c4e5d2ce60205a3d7356f2b917ea/checks Cheers, Matt
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index 18b19dbccbba..f4e32187e2c3 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -151,10 +151,12 @@ 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; entry = mptcp_pm_del_add_timer(msk, addr, false); + ret = entry; kfree(entry); - return 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 an unnecessary if-check, which resulted in returning a freed pointer. This still works due to the implicit boolean conversion when returning the freed pointer from mptcp_remove_anno_list_by_saddr(), but it can be confusing and potentially error-prone. To improve clarity, add a local variable to explicitly return a boolean value instead. Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev> --- Changes in v2: - Remove the if-check again as suggested by Matthieu Baerts - Rephrase the commit message - Link to v1: https://lore.kernel.org/r/20250325110639.49399-2-thorsten.blum@linux.dev/ --- net/mptcp/pm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)