[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 10 months 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 9 months, 1 week 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 10 months 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 10 months 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 10 months 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 10 months 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)