[PATCH net] net: free the policies if a socket's clone fails late

[PATCH net] net: free the policies if a socket's clone fails late

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

sk_clone() clones the IPsec policies of the parent and then its BPF socket
storage. If the latter fails, the new socket is freed with sk_destruct
cleared, as it is still a raw copy of the parent. No code which knows about
the policies of a socket gets to see it, and the two policies which have
just been cloned are lost. They also stay on the list of all policies of
the network namespace and in its count of socket policies.

bpf_sk_storage_clone() fails if memory is short, or if the storage doesn't
fit into net.core.optmem_max.

Free the policies before leaving. This can't be done for all users of the
label: if the socket filter could not be charged, sk_policy[] still holds
the pointers of the parent.

Fixes: 8f51dfc73bf1 ("bpf: support cloning sk storage on accept()")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/sock.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index e8551df8330f..d311f56803ed 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2577,8 +2577,10 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
 		goto free;
 	}
 
-	if (bpf_sk_storage_clone(sk, newsk))
+	if (bpf_sk_storage_clone(sk, newsk)) {
+		xfrm_sk_free_policy(newsk);
 		goto free;
+	}
 
 	/* Clear sk_user_data if parent had the pointer tagged
 	 * as not suitable for copying when cloning.
-- 
2.55.0

Re: [PATCH net] net: free the policies if a socket's clone fails late

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: free the policies if a socket's clone fails late

[kuniyu]

On Sat, 17 Oct 2026 01:39:12 +0000, Alexei Starovoitov wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> sk_clone() clones the IPsec policies of the parent and then its BPF socket
> storage. If the latter fails, the new socket is freed with sk_destruct
> cleared, as it is still a raw copy of the parent. No code which knows about
> the policies of a socket gets to see it, and the two policies which have
> just been cloned are lost. They also stay on the list of all policies of
> the network namespace and in its count of socket policies.
> 
> bpf_sk_storage_clone() fails if memory is short, or if the storage doesn't
> fit into net.core.optmem_max.
> 
> Free the policies before leaving. This can't be done for all users of the
> label: if the socket filter could not be charged, sk_policy[] still holds
> the pointers of the parent.
> 
> Fixes: 8f51dfc73bf1 ("bpf: support cloning sk storage on accept()")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>

Re: [PATCH net] net: free the policies if a socket's clone fails late

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: xfrm policy leak in sk_clone() when xfrm_sk_clone_policy() fails
--

commit a4aae636f8ebbd9f602d754c2bcb096a55363b3d
Author: Josef Bacik <josef@toxicpanda.com>

net: free the policies if a socket's clone fails late

This commit calls xfrm_sk_free_policy() in sk_clone() when
bpf_sk_storage_clone() fails so that cloned IPsec socket policies are not
leaked when freeing the new socket.

> diff --git a/net/core/sock.c b/net/core/sock.c
> index 1302c1e848b2a..cdbd49ff71b0a 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2578,8 +2578,10 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
>  		goto free;
>  	}
>  
> -	if (bpf_sk_storage_clone(sk, newsk))
> +	if (bpf_sk_storage_clone(sk, newsk)) {
> +		xfrm_sk_free_policy(newsk);
>  		goto free;
> +	}
>  
>  	/* Clear sk_user_data if parent had the pointer tagged
>  	 * as not suitable for copying when cloning.

[Severity: Medium]
Can newsk->sk_policy[0] still leak if xfrm_sk_clone_policy() fails on the
second policy?

In net/core/sock.c:sk_clone(), just above this block:

	if (unlikely(!is_charged || xfrm_sk_clone_policy(newsk, sk))) {
		if (!is_charged)
			RCU_INIT_POINTER(newsk->sk_filter, NULL);

		goto free;
	}

When is_charged is true and the parent socket has both sk_policy[0] and
sk_policy[1] installed, xfrm_sk_clone_policy() clears newsk->sk_policy[0..1]
and calls __xfrm_sk_clone_policy():

net/xfrm/xfrm_policy.c:__xfrm_sk_clone_policy() {
	for (i = 0; i < 2; i++) {
		p = rcu_dereference(osk->sk_policy[i]);
		if (p) {
			np = clone_policy(p, i);
			if (unlikely(!np)) {
				ret = -ENOMEM;
				break;
			}
			rcu_assign_pointer(sk->sk_policy[i], np);
		}
	}
}

If clone_policy(p, 0) succeeds (allocating the policy, linking it into
net->xfrm.policy_all, and incrementing net->xfrm.policy_count[XFRM_POLICY_MAX])
and clone_policy(p, 1) then fails, __xfrm_sk_clone_policy() returns -ENOMEM
with newsk->sk_policy[0] still populated.

Because sk_clone() jumps directly to free without calling
xfrm_sk_free_policy(newsk) on that path, wouldn't newsk->sk_policy[0] still
be leaked?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017013912.396296-1-alexei.starovoitov@gmail.com?part=1

Re: [PATCH net] net: free the policies if a socket's clone fails late

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch is incomplete.

--

net: free the policies if a socket's clone fails late

This patch makes sk_clone() call xfrm_sk_free_policy() on the new socket
when bpf_sk_storage_clone() fails. Without it, the cloned IPsec policies
are lost and stay on the netns policy list. The other user of the free
label is left alone, because sk_policy[] may still hold the parent's
pointers there.

> diff --git a/net/core/sock.c b/net/core/sock.c
> index 1302c1e848b2a..cdbd49ff71b0a 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2578,8 +2578,10 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
>  		goto free;
>  	}
>
> -	if (bpf_sk_storage_clone(sk, newsk))
> +	if (bpf_sk_storage_clone(sk, newsk)) {
> +		xfrm_sk_free_policy(newsk);
>  		goto free;
> +	}

[Severity: High]
Can the same policy leak still happen through the branch just above this,
when xfrm_sk_clone_policy() itself fails partway?

The "goto free" in the context above comes from this branch in sk_clone():

net/core/sock.c:sk_clone() {
    ...
	if (unlikely(!is_charged || xfrm_sk_clone_policy(newsk, sk))) {
		if (!is_charged)
			RCU_INIT_POINTER(newsk->sk_filter, NULL);

		goto free;
	}
    ...
}

xfrm_sk_clone_policy() clears both newsk->sk_policy[] slots and then calls
__xfrm_sk_clone_policy(). That loops over both directions:

net/xfrm/xfrm_policy.c:__xfrm_sk_clone_policy() {
    ...
	for (i = 0; i < 2; i++) {
		p = rcu_dereference(osk->sk_policy[i]);
		if (p) {
			np = clone_policy(p, i);
			if (unlikely(!np)) {
				ret = -ENOMEM;
				break;
			}
			rcu_assign_pointer(sk->sk_policy[i], np);
		}
	}
    ...
}

Suppose clone_policy() succeeds for i == 0. The clone is then linked into
net->xfrm.policy_all through xfrm_sk_policy_link() and counted in
policy_count.

Now suppose it fails for i == 1, either because xfrm_policy_alloc() with
GFP_ATOMIC fails or because security_xfrm_policy_clone() fails. The
function returns -ENOMEM and leaves sk_policy[0] attached to newsk.

In that case is_charged is true, so sk_clone() jumps straight to free:.
That path clears sk_destruct and calls sk_free(), and nothing releases
sk_policy[0]. Wouldn't this leave the policy[0] clone on policy_all? That
is the situation the commit message describes for the
bpf_sk_storage_clone() failure.

xfrm_policy_flush() skips socket policies (dir >= XFRM_POLICY_MAX), so the
entry looks like it would still be there at netns teardown and trigger the
WARN_ON(!list_empty(&net->xfrm.policy_all)) in xfrm_policy_fini().

The commit message gives this reason for not freeing on the other user of
the label:

    Free the policies before leaving. This can't be done for all users of the
    label: if the socket filter could not be charged, sk_policy[] still holds
    the pointers of the parent.

This seems to hold only when !is_charged short-circuits the condition, so
that xfrm_sk_clone_policy() never runs. Once xfrm_sk_clone_policy() has
run, sk_policy[] holds only fresh clones or NULL.

Would it be safe to call xfrm_sk_free_policy(newsk) in that sub-case, for
example in an else branch of the !is_charged check? Alternatively,
__xfrm_sk_clone_policy() could roll back the already cloned policy on
partial failure.

For comparison, tcp_v4_send_reset() always calls
xfrm_sk_free_policy(ctl_sk) after xfrm_sk_clone_policy(ctl_sk, sk),
whatever the clone returned.

>
>  	/* Clear sk_user_data if parent had the pointer tagged
>  	 * as not suitable for copying when cloning.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017013912.396296-1-alexei.starovoitov%40gmail.com

Re: [PATCH net] net: free the policies if a socket's clone fails late

[Alexei Starovoitov]

On Sat, Oct 17, 2026 at 07:15 PM netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> wrote:
> [Severity: High]
> Can the same policy leak still happen through the branch just above this,
> when xfrm_sk_clone_policy() itself fails partway?

yeah. It's an old bug, not from this patch. It needs both policies on
the listener and an allocation failure on the second clone.

> Would it be safe to call xfrm_sk_free_policy(newsk) in that sub-case, for
> example in an else branch of the !is_charged check? Alternatively,
> __xfrm_sk_clone_policy() could roll back the already cloned policy on
> partial failure.

Yes.
Somebody would need to send a followup.