BPF path manager

[PATCH mptcp-next v2 04/36] mptcp: use sock_kfree_s instead of kfree

Posted by Geliang Tang 5 months, 4 weeks ago

From: Geliang Tang <tanggeliang@kylinos.cn>

The local address entries on the userspace_pm_local_addr_list are allocated
by sock_kmalloc(). It's better to use sock_kfree_s() to free them, instead
of using kfree().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 00a7f9dd90cf..3fb5713cd988 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -91,6 +91,7 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
 						struct mptcp_pm_addr_entry *addr)
 {
 	struct mptcp_pm_addr_entry *entry, *tmp;
+	struct sock *sk = (struct sock *)msk;
 
 	mptcp_for_each_address_safe(msk, entry, tmp) {
 		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
@@ -98,7 +99,7 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
 			 * be used multiple times (e.g. fullmesh mode).
 			 */
 			list_del_rcu(&entry->list);
-			kfree(entry);
+			sock_kfree_s(sk, entry, sizeof(*entry));
 			msk->pm.local_addr_used--;
 			return 0;
 		}
-- 
2.45.2

Re: [PATCH mptcp-next v2 04/36] mptcp: use sock_kfree_s instead of kfree

Posted by Matthieu Baerts 5 months, 2 weeks ago

Hi Geliang,

On 22/10/2024 11:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The local address entries on the userspace_pm_local_addr_list are allocated
> by sock_kmalloc(). It's better to use sock_kfree_s() to free them, instead

Good catch! Do you mind developing a bit why "it is better to use
sock_kfree_s()"?

To me, it looks like a bug fix: we should use sock_kfree_s() to adjust
the allocated size. In this case, a 'Fixes' tag should be added, and
target mptcp-net. WDYT?

> of using kfree().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_userspace.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 00a7f9dd90cf..3fb5713cd988 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -91,6 +91,7 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>  						struct mptcp_pm_addr_entry *addr)
>  {
>  	struct mptcp_pm_addr_entry *entry, *tmp;
> +	struct sock *sk = (struct sock *)msk;
>  
>  	mptcp_for_each_address_safe(msk, entry, tmp) {
>  		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
> @@ -98,7 +99,7 @@ static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
>  			 * be used multiple times (e.g. fullmesh mode).
>  			 */
>  			list_del_rcu(&entry->list);
> -			kfree(entry);
> +			sock_kfree_s(sk, entry, sizeof(*entry));
>  			msk->pm.local_addr_used--;
>  			return 0;
>  		}

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

Re: [PATCH mptcp-next v2 04/36] mptcp: use sock_kfree_s instead of kfree

Posted by Geliang Tang 5 months, 2 weeks ago

Hi Matt,

Thanks for your review.

On Wed, 2024-10-30 at 19:21 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/10/2024 11:14, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > The local address entries on the userspace_pm_local_addr_list are
> > allocated
> > by sock_kmalloc(). It's better to use sock_kfree_s() to free them,
> > instead
> 
> Good catch! Do you mind developing a bit why "it is better to use
> sock_kfree_s()"?
> 
> To me, it looks like a bug fix: we should use sock_kfree_s() to
> adjust
> the allocated size. In this case, a 'Fixes' tag should be added, and
> target mptcp-net. WDYT?

I'll send a v2 of this patch alone out of "BPF path manager" set to our
ML, with "mptcp-net" prefix and "Fixes" tag, and update the commit log
too.

-Geliang

> 
> > of using kfree().
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/pm_userspace.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index 00a7f9dd90cf..3fb5713cd988 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -91,6 +91,7 @@ static int
> > mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
> >  						struct
> > mptcp_pm_addr_entry *addr)
> >  {
> >  	struct mptcp_pm_addr_entry *entry, *tmp;
> > +	struct sock *sk = (struct sock *)msk;
> >  
> >  	mptcp_for_each_address_safe(msk, entry, tmp) {
> >  		if (mptcp_addresses_equal(&entry->addr, &addr-
> > >addr, false)) {
> > @@ -98,7 +99,7 @@ static int
> > mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
> >  			 * be used multiple times (e.g. fullmesh
> > mode).
> >  			 */
> >  			list_del_rcu(&entry->list);
> > -			kfree(entry);
> > +			sock_kfree_s(sk, entry, sizeof(*entry));
> >  			msk->pm.local_addr_used--;
> >  			return 0;
> >  		}
> 
> Cheers,
> Matt