[PATCH net] udp: drop secpath before storing an skb in a receive queue

[PATCH net] udp: drop secpath before storing an skb in a receive queue

[Eric Dumazet]

Michal reported and bisected an issue after recent adoption
of skb_attempt_defer_free() in UDP.

We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
("tcp: drop secpath at the same time as we currently drop dst")

Many thanks to Michal and Sabrina.

Fixes: 6471658dc66c ("udp: use skb_attempt_defer_free()")
Reported-and-bisected-by: Michal Kubecek <mkubecek@suse.cz>
Closes: https://lore.kernel.org/netdev/gpjh4lrotyephiqpuldtxxizrsg6job7cvhiqrw72saz2ubs3h@g6fgbvexgl3r/
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Sabrina Dubroca <sd@queasysnail.net>
---
 net/ipv4/udp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 95241093b7f0..3f05ee70029c 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1709,6 +1709,8 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb)
 	int dropcount;
 	int nb = 0;
 
+	secpath_reset(skb);
+
 	rmem = atomic_read(&sk->sk_rmem_alloc);
 	rcvbuf = READ_ONCE(sk->sk_rcvbuf);
 	size = skb->truesize;
-- 
2.51.0.788.g6d19910ace-goog

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Sabrina Dubroca]

2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
> Michal reported and bisected an issue after recent adoption
> of skb_attempt_defer_free() in UDP.
> 
> We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
> ("tcp: drop secpath at the same time as we currently drop dst")

I'm not convinced this is the same bug. The TCP one was a "leaked"
reference (delayed put). This looks more like a double put/missing
hold to me (we get to the destroy path without having done the proper
delete, which would set XFRM_STATE_DEAD).

And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
x->tunnel as we delete x").

> Many thanks to Michal and Sabrina.
> 
> Fixes: 6471658dc66c ("udp: use skb_attempt_defer_free()")
> Reported-and-bisected-by: Michal Kubecek <mkubecek@suse.cz>
> Closes: https://lore.kernel.org/netdev/gpjh4lrotyephiqpuldtxxizrsg6job7cvhiqrw72saz2ubs3h@g6fgbvexgl3r/
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  net/ipv4/udp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 95241093b7f0..3f05ee70029c 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1709,6 +1709,8 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb)
>  	int dropcount;
>  	int nb = 0;
>  
> +	secpath_reset(skb);

See also the comment for udp_try_make_stateless:

/* all head states (dst, sk, nf conntrack) except skb extensions are
 * cleared by udp_rcv().
 *
 * We need to preserve secpath, if present, to eventually process
 * IP_CMSG_PASSSEC at recvmsg() time.
 *
 * Other extensions can be cleared.
 */


It looks like this patch would re-introduce the problem fixed by
dce4551cb2ad ("udp: preserve head state for IP_CMSG_PASSSEC").

> +
>  	rmem = atomic_read(&sk->sk_rmem_alloc);
>  	rcvbuf = READ_ONCE(sk->sk_rcvbuf);
>  	size = skb->truesize;
> -- 
> 2.51.0.788.g6d19910ace-goog
> 

-- 
Sabrina

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Eric Dumazet]

On Mon, Oct 13, 2025 at 11:37 PM Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> 2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
> > Michal reported and bisected an issue after recent adoption
> > of skb_attempt_defer_free() in UDP.
> >
> > We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
> > ("tcp: drop secpath at the same time as we currently drop dst")
>
> I'm not convinced this is the same bug. The TCP one was a "leaked"
> reference (delayed put). This looks more like a double put/missing
> hold to me (we get to the destroy path without having done the proper
> delete, which would set XFRM_STATE_DEAD).
>

Hmm, this was bisected to use of skb_attempt_defer_free(), surely holding
xfrm in a per-cpu queue looks the same to me.

We also had recent syzbot reports hinting at that.

> And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
> x->tunnel as we delete x").
>
> > Many thanks to Michal and Sabrina.
> >
> > Fixes: 6471658dc66c ("udp: use skb_attempt_defer_free()")
> > Reported-and-bisected-by: Michal Kubecek <mkubecek@suse.cz>
> > Closes: https://lore.kernel.org/netdev/gpjh4lrotyephiqpuldtxxizrsg6job7cvhiqrw72saz2ubs3h@g6fgbvexgl3r/
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Sabrina Dubroca <sd@queasysnail.net>
> > ---
> >  net/ipv4/udp.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 95241093b7f0..3f05ee70029c 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1709,6 +1709,8 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb)
> >       int dropcount;
> >       int nb = 0;
> >
> > +     secpath_reset(skb);
>
> See also the comment for udp_try_make_stateless:
>
> /* all head states (dst, sk, nf conntrack) except skb extensions are
>  * cleared by udp_rcv().
>  *
>  * We need to preserve secpath, if present, to eventually process
>  * IP_CMSG_PASSSEC at recvmsg() time.
>  *
>  * Other extensions can be cleared.
>  */
>
>
> It looks like this patch would re-introduce the problem fixed by
> dce4551cb2ad ("udp: preserve head state for IP_CMSG_PASSSEC").

Arg, I tried to not slow down the consumer part, too bad for XFRM then.

What about then :

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 95241093b7f0..ac45e4056c51 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1853,6 +1853,7 @@ void skb_consume_udp(struct sock *sk, struct
sk_buff *skb, int len)
        if (!skb_shared(skb)) {
                if (unlikely(udp_skb_has_head_state(skb)))
                        skb_release_head_state(skb);
+               secpath_reset(skb);
                skb_attempt_defer_free(skb);
                return;
        }

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Paolo Abeni]

On 10/14/25 8:37 AM, Sabrina Dubroca wrote:
> 2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
>> Michal reported and bisected an issue after recent adoption
>> of skb_attempt_defer_free() in UDP.
>>
>> We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
>> ("tcp: drop secpath at the same time as we currently drop dst")
> 
> I'm not convinced this is the same bug. The TCP one was a "leaked"
> reference (delayed put). This looks more like a double put/missing
> hold to me (we get to the destroy path without having done the proper
> delete, which would set XFRM_STATE_DEAD).
> 
> And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
> x->tunnel as we delete x").

I think Sabrina is right. If the skb carries a secpath,
UDP_SKB_IS_STATELESS is not set, and skb_release_head_state() will be
called by skb_consume_udp().

skb_ext_put() does not clear skb->extensions nor ext->refcnt, if
skb_attempt_defer_free() enters the slow path (kfree_skb_napi_cache()),
the skb will go through again skb_release_head_state(), with a double free.

I think something alike the following (completely untested) should work:
---
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 95241093b7f0..4a308fd6aa6c 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
sk_buff *skb, int len)
 		sk_peek_offset_bwd(sk, len);

 	if (!skb_shared(skb)) {
-		if (unlikely(udp_skb_has_head_state(skb)))
+		if (unlikely(udp_skb_has_head_state(skb))) {
 			skb_release_head_state(skb);
+			skb->active_extensions = 0;
+		}
 		skb_attempt_defer_free(skb);
 		return;
 	}
---

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Eric Dumazet]

On Tue, Oct 14, 2025 at 12:32 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
>
>
> On 10/14/25 8:37 AM, Sabrina Dubroca wrote:
> > 2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
> >> Michal reported and bisected an issue after recent adoption
> >> of skb_attempt_defer_free() in UDP.
> >>
> >> We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
> >> ("tcp: drop secpath at the same time as we currently drop dst")
> >
> > I'm not convinced this is the same bug. The TCP one was a "leaked"
> > reference (delayed put). This looks more like a double put/missing
> > hold to me (we get to the destroy path without having done the proper
> > delete, which would set XFRM_STATE_DEAD).
> >
> > And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
> > x->tunnel as we delete x").
>
> I think Sabrina is right. If the skb carries a secpath,
> UDP_SKB_IS_STATELESS is not set, and skb_release_head_state() will be
> called by skb_consume_udp().
>
> skb_ext_put() does not clear skb->extensions nor ext->refcnt, if
> skb_attempt_defer_free() enters the slow path (kfree_skb_napi_cache()),
> the skb will go through again skb_release_head_state(), with a double free.
>
> I think something alike the following (completely untested) should work:
> ---
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 95241093b7f0..4a308fd6aa6c 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
> sk_buff *skb, int len)
>                 sk_peek_offset_bwd(sk, len);
>
>         if (!skb_shared(skb)) {
> -               if (unlikely(udp_skb_has_head_state(skb)))
> +               if (unlikely(udp_skb_has_head_state(skb))) {
>                         skb_release_head_state(skb);
> +                       skb->active_extensions = 0;

Wait, what, skb_ext_put() can not be called twice ?

Because we prefer not dirtying skb in skb_release_head_state() ?

Perhaps add a big comment in skb_release_head_state() to avoid mistakes.

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index bc12790017b0b5c0be99f8fb9d362b3730fa4eb0..8d274ddd54ad4c59cc29f821fc371c89052bf875
100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1141,6 +1141,10 @@ void skb_release_head_state(struct sk_buff *skb)
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
        nf_conntrack_put(skb_nfct(skb));
 #endif
+
+       /* Note: we do not call skb_ext_reset() to avoid dirtying
+        * a cache line. Callers might have to clear skb->active_extensions.
+        */
        skb_ext_put(skb);
 }

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Sabrina Dubroca]

2025-10-14, 09:32:09 +0200, Paolo Abeni wrote:
> 
> 
> On 10/14/25 8:37 AM, Sabrina Dubroca wrote:
> > 2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
> >> Michal reported and bisected an issue after recent adoption
> >> of skb_attempt_defer_free() in UDP.
> >>
> >> We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
> >> ("tcp: drop secpath at the same time as we currently drop dst")
> > 
> > I'm not convinced this is the same bug. The TCP one was a "leaked"
> > reference (delayed put). This looks more like a double put/missing
> > hold to me (we get to the destroy path without having done the proper
> > delete, which would set XFRM_STATE_DEAD).
> > 
> > And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
> > x->tunnel as we delete x").
> 
> I think Sabrina is right. If the skb carries a secpath,
> UDP_SKB_IS_STATELESS is not set, and skb_release_head_state() will be
> called by skb_consume_udp().
> 
> skb_ext_put() does not clear skb->extensions nor ext->refcnt, if
> skb_attempt_defer_free() enters the slow path (kfree_skb_napi_cache()),
> the skb will go through again skb_release_head_state(), with a double free.

Thanks Paolo, I was going down the same path, you got there faster :)

> I think something alike the following (completely untested) should work:
> ---
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 95241093b7f0..4a308fd6aa6c 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
> sk_buff *skb, int len)
>  		sk_peek_offset_bwd(sk, len);
> 
>  	if (!skb_shared(skb)) {
> -		if (unlikely(udp_skb_has_head_state(skb)))
> +		if (unlikely(udp_skb_has_head_state(skb))) {
>  			skb_release_head_state(skb);
> +			skb->active_extensions = 0;
> +		}
>  		skb_attempt_defer_free(skb);
>  		return;
>  	}

-- 
Sabrina

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Eric Dumazet]

On Tue, Oct 14, 2025 at 12:43 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Oct 14, 2025 at 12:32 AM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> >
> >
> > On 10/14/25 8:37 AM, Sabrina Dubroca wrote:
> > > 2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
> > >> Michal reported and bisected an issue after recent adoption
> > >> of skb_attempt_defer_free() in UDP.
> > >>
> > >> We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
> > >> ("tcp: drop secpath at the same time as we currently drop dst")
> > >
> > > I'm not convinced this is the same bug. The TCP one was a "leaked"
> > > reference (delayed put). This looks more like a double put/missing
> > > hold to me (we get to the destroy path without having done the proper
> > > delete, which would set XFRM_STATE_DEAD).
> > >
> > > And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
> > > x->tunnel as we delete x").
> >
> > I think Sabrina is right. If the skb carries a secpath,
> > UDP_SKB_IS_STATELESS is not set, and skb_release_head_state() will be
> > called by skb_consume_udp().
> >
> > skb_ext_put() does not clear skb->extensions nor ext->refcnt, if
> > skb_attempt_defer_free() enters the slow path (kfree_skb_napi_cache()),
> > the skb will go through again skb_release_head_state(), with a double free.
> >
> > I think something alike the following (completely untested) should work:
> > ---
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 95241093b7f0..4a308fd6aa6c 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
> > sk_buff *skb, int len)
> >                 sk_peek_offset_bwd(sk, len);
> >
> >         if (!skb_shared(skb)) {
> > -               if (unlikely(udp_skb_has_head_state(skb)))
> > +               if (unlikely(udp_skb_has_head_state(skb))) {
> >                         skb_release_head_state(skb);
> > +                       skb->active_extensions = 0;

We probably also want to clear CONNTRACK state as well.

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Eric Dumazet]

On Tue, Oct 14, 2025 at 1:01 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Oct 14, 2025 at 12:43 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Oct 14, 2025 at 12:32 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > >
> > >
> > >
> > > On 10/14/25 8:37 AM, Sabrina Dubroca wrote:
> > > > 2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
> > > >> Michal reported and bisected an issue after recent adoption
> > > >> of skb_attempt_defer_free() in UDP.
> > > >>
> > > >> We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
> > > >> ("tcp: drop secpath at the same time as we currently drop dst")
> > > >
> > > > I'm not convinced this is the same bug. The TCP one was a "leaked"
> > > > reference (delayed put). This looks more like a double put/missing
> > > > hold to me (we get to the destroy path without having done the proper
> > > > delete, which would set XFRM_STATE_DEAD).
> > > >
> > > > And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
> > > > x->tunnel as we delete x").
> > >
> > > I think Sabrina is right. If the skb carries a secpath,
> > > UDP_SKB_IS_STATELESS is not set, and skb_release_head_state() will be
> > > called by skb_consume_udp().
> > >
> > > skb_ext_put() does not clear skb->extensions nor ext->refcnt, if
> > > skb_attempt_defer_free() enters the slow path (kfree_skb_napi_cache()),
> > > the skb will go through again skb_release_head_state(), with a double free.
> > >
> > > I think something alike the following (completely untested) should work:
> > > ---
> > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > > index 95241093b7f0..4a308fd6aa6c 100644
> > > --- a/net/ipv4/udp.c
> > > +++ b/net/ipv4/udp.c
> > > @@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
> > > sk_buff *skb, int len)
> > >                 sk_peek_offset_bwd(sk, len);
> > >
> > >         if (!skb_shared(skb)) {
> > > -               if (unlikely(udp_skb_has_head_state(skb)))
> > > +               if (unlikely(udp_skb_has_head_state(skb))) {
> > >                         skb_release_head_state(skb);
> > > +                       skb->active_extensions = 0;
>
> We probably also want to clear CONNTRACK state as well.

Perhaps not use skb_release_head_state() ?

We know there is no dst, and no destructor.

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 95241093b7f0..98628486c4c5 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
sk_buff *skb, int len)
                sk_peek_offset_bwd(sk, len);

        if (!skb_shared(skb)) {
-               if (unlikely(udp_skb_has_head_state(skb)))
-                       skb_release_head_state(skb);
+               if (unlikely(udp_skb_has_head_state(skb))) {
+                       nf_reset_ct(skb);
+                       skb_ext_reset(skb);
+               }
                skb_attempt_defer_free(skb);
                return;
        }

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Eric Dumazet]

On Tue, Oct 14, 2025 at 1:06 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Oct 14, 2025 at 1:01 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Oct 14, 2025 at 12:43 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Tue, Oct 14, 2025 at 12:32 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > >
> > > >
> > > >
> > > > On 10/14/25 8:37 AM, Sabrina Dubroca wrote:
> > > > > 2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
> > > > >> Michal reported and bisected an issue after recent adoption
> > > > >> of skb_attempt_defer_free() in UDP.
> > > > >>
> > > > >> We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
> > > > >> ("tcp: drop secpath at the same time as we currently drop dst")
> > > > >
> > > > > I'm not convinced this is the same bug. The TCP one was a "leaked"
> > > > > reference (delayed put). This looks more like a double put/missing
> > > > > hold to me (we get to the destroy path without having done the proper
> > > > > delete, which would set XFRM_STATE_DEAD).
> > > > >
> > > > > And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
> > > > > x->tunnel as we delete x").
> > > >
> > > > I think Sabrina is right. If the skb carries a secpath,
> > > > UDP_SKB_IS_STATELESS is not set, and skb_release_head_state() will be
> > > > called by skb_consume_udp().
> > > >
> > > > skb_ext_put() does not clear skb->extensions nor ext->refcnt, if
> > > > skb_attempt_defer_free() enters the slow path (kfree_skb_napi_cache()),
> > > > the skb will go through again skb_release_head_state(), with a double free.
> > > >
> > > > I think something alike the following (completely untested) should work:
> > > > ---
> > > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > > > index 95241093b7f0..4a308fd6aa6c 100644
> > > > --- a/net/ipv4/udp.c
> > > > +++ b/net/ipv4/udp.c
> > > > @@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
> > > > sk_buff *skb, int len)
> > > >                 sk_peek_offset_bwd(sk, len);
> > > >
> > > >         if (!skb_shared(skb)) {
> > > > -               if (unlikely(udp_skb_has_head_state(skb)))
> > > > +               if (unlikely(udp_skb_has_head_state(skb))) {
> > > >                         skb_release_head_state(skb);
> > > > +                       skb->active_extensions = 0;
> >
> > We probably also want to clear CONNTRACK state as well.
>
> Perhaps not use skb_release_head_state() ?
>
> We know there is no dst, and no destructor.
>

An no conntrack either from UDP

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 95241093b7f0..932c21838b9b 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1851,8 +1851,13 @@ void skb_consume_udp(struct sock *sk, struct
sk_buff *skb, int len)
                sk_peek_offset_bwd(sk, len);

        if (!skb_shared(skb)) {
-               if (unlikely(udp_skb_has_head_state(skb)))
-                       skb_release_head_state(skb);
+               if (unlikely(udp_skb_has_head_state(skb))) {
+                       /* Make sure that skb_release_head_state()
will have nothing to do. */
+                       DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
+                       DEBUG_NET_WARN_ON_ONCE(skb->destructor);
+                       DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
+                       skb_ext_reset(skb);
+               }
                skb_attempt_defer_free(skb);
                return;
        }

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Sabrina Dubroca]

2025-10-14, 01:06:04 -0700, Eric Dumazet wrote:
> On Tue, Oct 14, 2025 at 1:01 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Oct 14, 2025 at 12:43 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Tue, Oct 14, 2025 at 12:32 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > >
> > > >
> > > >
> > > > On 10/14/25 8:37 AM, Sabrina Dubroca wrote:
> > > > > 2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
> > > > >> Michal reported and bisected an issue after recent adoption
> > > > >> of skb_attempt_defer_free() in UDP.
> > > > >>
> > > > >> We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
> > > > >> ("tcp: drop secpath at the same time as we currently drop dst")
> > > > >
> > > > > I'm not convinced this is the same bug. The TCP one was a "leaked"
> > > > > reference (delayed put). This looks more like a double put/missing
> > > > > hold to me (we get to the destroy path without having done the proper
> > > > > delete, which would set XFRM_STATE_DEAD).
> > > > >
> > > > > And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
> > > > > x->tunnel as we delete x").
> > > >
> > > > I think Sabrina is right. If the skb carries a secpath,
> > > > UDP_SKB_IS_STATELESS is not set, and skb_release_head_state() will be
> > > > called by skb_consume_udp().
> > > >
> > > > skb_ext_put() does not clear skb->extensions nor ext->refcnt, if
> > > > skb_attempt_defer_free() enters the slow path (kfree_skb_napi_cache()),
> > > > the skb will go through again skb_release_head_state(), with a double free.
> > > >
> > > > I think something alike the following (completely untested) should work:
> > > > ---
> > > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > > > index 95241093b7f0..4a308fd6aa6c 100644
> > > > --- a/net/ipv4/udp.c
> > > > +++ b/net/ipv4/udp.c
> > > > @@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
> > > > sk_buff *skb, int len)
> > > >                 sk_peek_offset_bwd(sk, len);
> > > >
> > > >         if (!skb_shared(skb)) {
> > > > -               if (unlikely(udp_skb_has_head_state(skb)))
> > > > +               if (unlikely(udp_skb_has_head_state(skb))) {
> > > >                         skb_release_head_state(skb);
> > > > +                       skb->active_extensions = 0;
> >
> > We probably also want to clear CONNTRACK state as well.
> 
> Perhaps not use skb_release_head_state() ?
> 
> We know there is no dst, and no destructor.

Then, do we need to do anything before calling skb_attempt_defer_free()?
skb_attempt_defer_free() only wants no dst and no destructor, and the
secpath issue that we dealt with in TCP is not a problem anymore.

Can we just drop the udp_skb_has_head_state() special handling and
simply call skb_attempt_defer_free()?


> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 95241093b7f0..98628486c4c5 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
> sk_buff *skb, int len)
>                 sk_peek_offset_bwd(sk, len);
> 
>         if (!skb_shared(skb)) {
> -               if (unlikely(udp_skb_has_head_state(skb)))
> -                       skb_release_head_state(skb);
> +               if (unlikely(udp_skb_has_head_state(skb))) {
> +                       nf_reset_ct(skb);
> +                       skb_ext_reset(skb);
> +               }
>                 skb_attempt_defer_free(skb);
>                 return;
>         }

-- 
Sabrina

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Eric Dumazet]

On Tue, Oct 14, 2025 at 1:28 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> 2025-10-14, 01:06:04 -0700, Eric Dumazet wrote:
> > On Tue, Oct 14, 2025 at 1:01 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Tue, Oct 14, 2025 at 12:43 AM Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > > On Tue, Oct 14, 2025 at 12:32 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 10/14/25 8:37 AM, Sabrina Dubroca wrote:
> > > > > > 2025-10-14, 06:04:54 +0000, Eric Dumazet wrote:
> > > > > >> Michal reported and bisected an issue after recent adoption
> > > > > >> of skb_attempt_defer_free() in UDP.
> > > > > >>
> > > > > >> We had the same issue for TCP, that Sabrina fixed in commit 9b6412e6979f
> > > > > >> ("tcp: drop secpath at the same time as we currently drop dst")
> > > > > >
> > > > > > I'm not convinced this is the same bug. The TCP one was a "leaked"
> > > > > > reference (delayed put). This looks more like a double put/missing
> > > > > > hold to me (we get to the destroy path without having done the proper
> > > > > > delete, which would set XFRM_STATE_DEAD).
> > > > > >
> > > > > > And this shouldn't be an issue after b441cf3f8c4b ("xfrm: delete
> > > > > > x->tunnel as we delete x").
> > > > >
> > > > > I think Sabrina is right. If the skb carries a secpath,
> > > > > UDP_SKB_IS_STATELESS is not set, and skb_release_head_state() will be
> > > > > called by skb_consume_udp().
> > > > >
> > > > > skb_ext_put() does not clear skb->extensions nor ext->refcnt, if
> > > > > skb_attempt_defer_free() enters the slow path (kfree_skb_napi_cache()),
> > > > > the skb will go through again skb_release_head_state(), with a double free.
> > > > >
> > > > > I think something alike the following (completely untested) should work:
> > > > > ---
> > > > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > > > > index 95241093b7f0..4a308fd6aa6c 100644
> > > > > --- a/net/ipv4/udp.c
> > > > > +++ b/net/ipv4/udp.c
> > > > > @@ -1851,8 +1851,10 @@ void skb_consume_udp(struct sock *sk, struct
> > > > > sk_buff *skb, int len)
> > > > >                 sk_peek_offset_bwd(sk, len);
> > > > >
> > > > >         if (!skb_shared(skb)) {
> > > > > -               if (unlikely(udp_skb_has_head_state(skb)))
> > > > > +               if (unlikely(udp_skb_has_head_state(skb))) {
> > > > >                         skb_release_head_state(skb);
> > > > > +                       skb->active_extensions = 0;
> > >
> > > We probably also want to clear CONNTRACK state as well.
> >
> > Perhaps not use skb_release_head_state() ?
> >
> > We know there is no dst, and no destructor.
>
> Then, do we need to do anything before calling skb_attempt_defer_free()?
> skb_attempt_defer_free() only wants no dst and no destructor, and the
> secpath issue that we dealt with in TCP is not a problem anymore.
>
> Can we just drop the udp_skb_has_head_state() special handling and
> simply call skb_attempt_defer_free()?

Good point ! I need a second cup of coffee !

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Paolo Abeni]

On 10/14/25 10:27 AM, Eric Dumazet wrote:
> On Tue, Oct 14, 2025 at 1:06 AM Eric Dumazet <edumazet@google.com> wrote:
>> Perhaps not use skb_release_head_state() ?
>>
>> We know there is no dst, and no destructor.
>>
> 
> An no conntrack either from UDP
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 95241093b7f0..932c21838b9b 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1851,8 +1851,13 @@ void skb_consume_udp(struct sock *sk, struct
> sk_buff *skb, int len)
>                 sk_peek_offset_bwd(sk, len);
> 
>         if (!skb_shared(skb)) {
> -               if (unlikely(udp_skb_has_head_state(skb)))
> -                       skb_release_head_state(skb);
> +               if (unlikely(udp_skb_has_head_state(skb))) {
> +                       /* Make sure that skb_release_head_state()
> will have nothing to do. */
> +                       DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
> +                       DEBUG_NET_WARN_ON_ONCE(skb->destructor);
> +                       DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
> +                       skb_ext_reset(skb);
> +               }
>                 skb_attempt_defer_free(skb);
>                 return;
>         }

LGTM, thanks!

/P

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

On Tue, Oct 14, 2025 at 01:27:13AM GMT, Eric Dumazet wrote:
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 95241093b7f0..932c21838b9b 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1851,8 +1851,13 @@ void skb_consume_udp(struct sock *sk, struct
> sk_buff *skb, int len)
>                 sk_peek_offset_bwd(sk, len);
> 
>         if (!skb_shared(skb)) {
> -               if (unlikely(udp_skb_has_head_state(skb)))
> -                       skb_release_head_state(skb);
> +               if (unlikely(udp_skb_has_head_state(skb))) {
> +                       /* Make sure that skb_release_head_state()
> will have nothing to do. */
> +                       DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
> +                       DEBUG_NET_WARN_ON_ONCE(skb->destructor);
> +                       DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
> +                       skb_ext_reset(skb);
> +               }
>                 skb_attempt_defer_free(skb);
>                 return;
>         }

Tested this version on my system (with DEBUG_NET enabled) and everything
seems to work fine so far.

Tested-by: Michal Kubecek <mkubecek@suse.cz>

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Eric Dumazet]

On Tue, Oct 14, 2025 at 4:20 AM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> On Tue, Oct 14, 2025 at 01:27:13AM GMT, Eric Dumazet wrote:
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 95241093b7f0..932c21838b9b 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1851,8 +1851,13 @@ void skb_consume_udp(struct sock *sk, struct
> > sk_buff *skb, int len)
> >                 sk_peek_offset_bwd(sk, len);
> >
> >         if (!skb_shared(skb)) {
> > -               if (unlikely(udp_skb_has_head_state(skb)))
> > -                       skb_release_head_state(skb);
> > +               if (unlikely(udp_skb_has_head_state(skb))) {
> > +                       /* Make sure that skb_release_head_state()
> > will have nothing to do. */
> > +                       DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
> > +                       DEBUG_NET_WARN_ON_ONCE(skb->destructor);
> > +                       DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
> > +                       skb_ext_reset(skb);
> > +               }
> >                 skb_attempt_defer_free(skb);
> >                 return;
> >         }
>
> Tested this version on my system (with DEBUG_NET enabled) and everything
> seems to work fine so far.
>
> Tested-by: Michal Kubecek <mkubecek@suse.cz>

Thanks for testing. I will follow Sabrina suggestion and send :

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 95241093b7f0..d66f273f9070 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1851,8 +1851,13 @@ void skb_consume_udp(struct sock *sk, struct
sk_buff *skb, int len)
                sk_peek_offset_bwd(sk, len);

        if (!skb_shared(skb)) {
-               if (unlikely(udp_skb_has_head_state(skb)))
-                       skb_release_head_state(skb);
+               /* Make sure that this skb has no dst, destructor
+                * or conntracking parts, because it might stay
+                * in a remote cpu list for a very long time.
+                */
+               DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
+               DEBUG_NET_WARN_ON_ONCE(skb->destructor);
+               DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
                skb_attempt_defer_free(skb);
                return;
        }

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 2609 bytes --]

On Tue, Oct 14, 2025 at 04:34:52AM GMT, Eric Dumazet wrote:
> On Tue, Oct 14, 2025 at 4:20 AM Michal Kubecek <mkubecek@suse.cz> wrote:
> >
> > On Tue, Oct 14, 2025 at 01:27:13AM GMT, Eric Dumazet wrote:
> > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > > index 95241093b7f0..932c21838b9b 100644
> > > --- a/net/ipv4/udp.c
> > > +++ b/net/ipv4/udp.c
> > > @@ -1851,8 +1851,13 @@ void skb_consume_udp(struct sock *sk, struct
> > > sk_buff *skb, int len)
> > >                 sk_peek_offset_bwd(sk, len);
> > >
> > >         if (!skb_shared(skb)) {
> > > -               if (unlikely(udp_skb_has_head_state(skb)))
> > > -                       skb_release_head_state(skb);
> > > +               if (unlikely(udp_skb_has_head_state(skb))) {
> > > +                       /* Make sure that skb_release_head_state()
> > > will have nothing to do. */
> > > +                       DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
> > > +                       DEBUG_NET_WARN_ON_ONCE(skb->destructor);
> > > +                       DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
> > > +                       skb_ext_reset(skb);
> > > +               }
> > >                 skb_attempt_defer_free(skb);
> > >                 return;
> > >         }
> >
> > Tested this version on my system (with DEBUG_NET enabled) and everything
> > seems to work fine so far.
> >
> > Tested-by: Michal Kubecek <mkubecek@suse.cz>
> 
> Thanks for testing. I will follow Sabrina suggestion and send :
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 95241093b7f0..d66f273f9070 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1851,8 +1851,13 @@ void skb_consume_udp(struct sock *sk, struct
> sk_buff *skb, int len)
>                 sk_peek_offset_bwd(sk, len);
> 
>         if (!skb_shared(skb)) {
> -               if (unlikely(udp_skb_has_head_state(skb)))
> -                       skb_release_head_state(skb);
> +               /* Make sure that this skb has no dst, destructor
> +                * or conntracking parts, because it might stay
> +                * in a remote cpu list for a very long time.
> +                */
> +               DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
> +               DEBUG_NET_WARN_ON_ONCE(skb->destructor);
> +               DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
>                 skb_attempt_defer_free(skb);
>                 return;
>         }

Running 6.18-rc1 with this patch right now, everything seems to work as
well. Thanks to everyone for quick response.

Tested-by: Michal Kubecek <mkubecek@suse.cz>

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Florian Westphal]

Eric Dumazet <edumazet@google.com> wrote:
> Thanks for testing. I will follow Sabrina suggestion and send :
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 95241093b7f0..d66f273f9070 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1851,8 +1851,13 @@ void skb_consume_udp(struct sock *sk, struct
> sk_buff *skb, int len)
>                 sk_peek_offset_bwd(sk, len);
> 
>         if (!skb_shared(skb)) {
> -               if (unlikely(udp_skb_has_head_state(skb)))
> -                       skb_release_head_state(skb);
> +               /* Make sure that this skb has no dst, destructor
> +                * or conntracking parts, because it might stay
> +                * in a remote cpu list for a very long time.
> +                */
> +               DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
> +               DEBUG_NET_WARN_ON_ONCE(skb->destructor);
> +               DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
>                 skb_attempt_defer_free(skb);

Are there cases where we expect to pass skb which violate this to
skb_attempt_defer_free()?

If no, then maybe move existing checks in skb_attempt_defer_free() up and
apped the skb_nfct check there so syzbot can blame other offenders too.

Re: [PATCH net] udp: drop secpath before storing an skb in a receive queue

[Eric Dumazet]

On Tue, Oct 14, 2025 at 6:40 AM Florian Westphal <fw@strlen.de> wrote:
>
> Eric Dumazet <edumazet@google.com> wrote:
> > Thanks for testing. I will follow Sabrina suggestion and send :
> >
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 95241093b7f0..d66f273f9070 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1851,8 +1851,13 @@ void skb_consume_udp(struct sock *sk, struct
> > sk_buff *skb, int len)
> >                 sk_peek_offset_bwd(sk, len);
> >
> >         if (!skb_shared(skb)) {
> > -               if (unlikely(udp_skb_has_head_state(skb)))
> > -                       skb_release_head_state(skb);
> > +               /* Make sure that this skb has no dst, destructor
> > +                * or conntracking parts, because it might stay
> > +                * in a remote cpu list for a very long time.
> > +                */
> > +               DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
> > +               DEBUG_NET_WARN_ON_ONCE(skb->destructor);
> > +               DEBUG_NET_WARN_ON_ONCE(skb_nfct(skb));
> >                 skb_attempt_defer_free(skb);
>
> Are there cases where we expect to pass skb which violate this to
> skb_attempt_defer_free()?

So far UDP makes sure these fields are cleared.

>
> If no, then maybe move existing checks in skb_attempt_defer_free() up and
> apped the skb_nfct check there so syzbot can blame other offenders too.

You are right, I forgot skb_attempt_defer_free() already had:

commit e8e1ce8454c9cc8ad2e4422bef346428e52455e3
Author: Eric Dumazet <edumazet@google.com>
Date:   Fri Apr 21 09:43:53 2023 +0000

    net: add debugging checks in skb_attempt_defer_free()

    Make sure skbs that are stored in softnet_data.defer_list
    do not have a dst attached.

    Also make sure the the skb was orphaned.

    Link: https://lore.kernel.org/netdev/CANn89iJuEVe72bPmEftyEJHLzzN=QNR2yueFjTxYXCEpS5S8HQ@mail.gmail.com/T/
    Signed-off-by: Eric Dumazet <edumazet@google.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 0d998806b377..bd815a00d2af 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -6881,6 +6881,9 @@ nodefer:  __kfree_skb(skb);
                return;
        }

+       DEBUG_NET_WARN_ON_ONCE(skb_dst(skb));
+       DEBUG_NET_WARN_ON_ONCE(skb->destructor);
+
        sd = &per_cpu(softnet_data, cpu);
        defer_max = READ_ONCE(sysctl_skb_defer_max);
        if (READ_ONCE(sd->defer_count) >= defer_max)