Re: [PATCH v2] mptcp: pm: Return local variable instead of freed pointer

[Matthieu Baerts <matttbe@kernel.org>]

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.