[v1] mptcp: pm: Defer freeing of MPTCP userspace path manager entries

[mptcp-net] mptcp: pm: Defer freeing of MPTCP userspace path manager entries

Posted by Mat Martineau 8 months, 1 week ago

When path manager entries are deleted from the local address list, they
are first unlinked from the address list using list_del_rcu(). The
entries must not be freed until after the RCU grace period, but the
existing code immediately frees the entry.

Use kfree_rcu_mightsleep() and adjust sk_omem_alloc in open code instead
of using the sock_kfree_s() helper. This code path is only called in a
netlink handler, so the "might sleep" function is preferable to adding
a rarely-used rcu_head member to struct mptcp_pm_addr_entry.

Signed-off-by: Mat Martineau <martineau@kernel.org>
---

My first implementation did add a sock_kfree_rcu_s() function like
Geliang suggested, but kfree_rcu() is a macro so that approach got
complicated. sock_kfree_rcu_s_mightsleep() seemed cumbersome, so I went
ahead and open-coded it. This should be applied after Geliang's v2,
which will delete similar code in a helper function.

---
 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 7fc19b844384..959af1e42a97 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -337,7 +337,8 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 
 	release_sock(sk);
 
-	sock_kfree_s(sk, match, sizeof(*match));
+	kfree_rcu_mightsleep(match);
+	atomic_sub(sizeof(*match), &sk->sk_omem_alloc);
 
 	err = 0;
 out:
-- 
2.49.0

Re: [mptcp-net] mptcp: pm: Defer freeing of MPTCP userspace path manager entries

Posted by Geliang Tang 7 months, 3 weeks ago

Hi Mat,

On Wed, 2025-04-09 at 19:59 -0700, Mat Martineau wrote:
> When path manager entries are deleted from the local address list,
> they
> are first unlinked from the address list using list_del_rcu(). The
> entries must not be freed until after the RCU grace period, but the
> existing code immediately frees the entry.
> 
> Use kfree_rcu_mightsleep() and adjust sk_omem_alloc in open code
> instead
> of using the sock_kfree_s() helper. This code path is only called in
> a
> netlink handler, so the "might sleep" function is preferable to
> adding
> a rarely-used rcu_head member to struct mptcp_pm_addr_entry.
> 
> Signed-off-by: Mat Martineau <martineau@kernel.org>
> ---
> 
> My first implementation did add a sock_kfree_rcu_s() function like
> Geliang suggested, but kfree_rcu() is a macro so that approach got
> complicated. sock_kfree_rcu_s_mightsleep() seemed cumbersome, so I

I thought about this again recently, and I think sock_krfree_s() is a
better name, and it matches sock_kfree_s() and sock_kzfree_s() better.
I just sent a patch named "sock: add sock_krfree_s helper" for this to
MPTCP ML, please review it for me.

Thanks,
-Geliang

> went
> ahead and open-coded it. This should be applied after Geliang's v2,
> which will delete similar code in a helper function.
> 
> ---
>  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 7fc19b844384..959af1e42a97 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -337,7 +337,8 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb,
> struct genl_info *info)
>  
>  	release_sock(sk);
>  
> -	sock_kfree_s(sk, match, sizeof(*match));
> +	kfree_rcu_mightsleep(match);
> +	atomic_sub(sizeof(*match), &sk->sk_omem_alloc);
>  
>  	err = 0;
>  out:

Re: [mptcp-net] mptcp: pm: Defer freeing of MPTCP userspace path manager entries

Posted by Matthieu Baerts 8 months, 1 week ago

Hi Mat,

On 10/04/2025 04:59, Mat Martineau wrote:
> When path manager entries are deleted from the local address list, they
> are first unlinked from the address list using list_del_rcu(). The
> entries must not be freed until after the RCU grace period, but the
> existing code immediately frees the entry.
> 
> Use kfree_rcu_mightsleep() and adjust sk_omem_alloc in open code instead
> of using the sock_kfree_s() helper. This code path is only called in a
> netlink handler, so the "might sleep" function is preferable to adding
> a rarely-used rcu_head member to struct mptcp_pm_addr_entry.

Thank you for having looked at this!

Should we add a Fixes tag? The following one or was it already there before?

Fixes: 88d097316371 ("mptcp: drop free_list for deleting entries")

Also, should we not also modify mptcp_userspace_pm_delete_local_addr()
doing the following?

  list_del_rcu(&entry->list);
  sock_kfree_s(sk, entry, sizeof(*entry));

If yes, and while at it, do you think it might be good to add a short
comment above "atomic_sub()" in the code explaining this is doing the
same as sock_kfree_s(), but with RCU support? In case sock_kfree_s is
modified later, and to understand what's being done here without using
blame.

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

Re: [mptcp-net] mptcp: pm: Defer freeing of MPTCP userspace path manager entries

Posted by Mat Martineau 8 months, 1 week ago

On Thu, 10 Apr 2025, Matthieu Baerts wrote:

> Hi Mat,
>
> On 10/04/2025 04:59, Mat Martineau wrote:
>> When path manager entries are deleted from the local address list, they
>> are first unlinked from the address list using list_del_rcu(). The
>> entries must not be freed until after the RCU grace period, but the
>> existing code immediately frees the entry.
>>
>> Use kfree_rcu_mightsleep() and adjust sk_omem_alloc in open code instead
>> of using the sock_kfree_s() helper. This code path is only called in a
>> netlink handler, so the "might sleep" function is preferable to adding
>> a rarely-used rcu_head member to struct mptcp_pm_addr_entry.
>
> Thank you for having looked at this!
>
> Should we add a Fixes tag? The following one or was it already there before?
>
> Fixes: 88d097316371 ("mptcp: drop free_list for deleting entries")

Hi Matthieu -

Yes, for -net I should have included a Fixes tag.

>
>
> Also, should we not also modify mptcp_userspace_pm_delete_local_addr()
> doing the following?
>
>  list_del_rcu(&entry->list);
>  sock_kfree_s(sk, entry, sizeof(*entry));
>

Geliang's series deletes this code, so I skipped this change intentionally 
(as explained in my note after the signoff).

> If yes, and while at it, do you think it might be good to add a short
> comment above "atomic_sub()" in the code explaining this is doing the
> same as sock_kfree_s(), but with RCU support? In case sock_kfree_s is
> modified later, and to understand what's being done here without using
> blame.

Good point, I can add that comment. It's a small change, but I'll send a 
v2 with that and the Fixes tag, and get the [PATCH mptcp-net] subject 
prefix correct this time :)


- Mat

Re: [mptcp-net] mptcp: pm: Defer freeing of MPTCP userspace path manager entries

Posted by MPTCP CI 8 months, 1 week ago

Hi Mat,

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): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/14371524037

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b2ab62831ba7
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=951816


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)

Re: [mptcp-net] mptcp: pm: Defer freeing of MPTCP userspace path manager entries

Posted by MPTCP CI 8 months, 1 week ago

Hi Mat,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Critical: Global Timeout ❌
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/14371524037

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/b2ab62831ba7
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=951816


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)