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(-)

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)

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
-- 
Sponsored by the NGI0 Core fund.

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 🔴
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/14067288712

Initiator: Matthieu Baerts (NGI0)
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/0852b4c9fbd3
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=947230


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

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
-- 
Sponsored by the NGI0 Core fund.

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
-- 
Sponsored by the NGI0 Core fund.