[PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

[PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

[Paolo Abeni]

The blamed commit below does not take in account that xfrm
can enable GRO over UDP encapsulation without going through
setup_udp_tunnel_sock().

At deletion time such socket will still go through
udp_tunnel_cleanup_gro(), and the failed GRO type lookup will
trigger the reported warning.

We can safely remove such warning, simply performing no action
on failed GRO type lookup at deletion time.

Reported-by: syzbot+8c469a2260132cd095c1@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8c469a2260132cd095c1
Fixes: 311b36574ceac ("udp_tunnel: use static call for GRO hooks when possible")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv4/udp_offload.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 088aa8cb8ac0c..2e0b52ae665bc 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -110,14 +110,7 @@ void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
 		cur = &udp_tunnel_gro_types[udp_tunnel_gro_type_nr++];
 		refcount_set(&cur->count, 1);
 		cur->gro_receive = up->gro_receive;
-	} else {
-		/*
-		 * The stack cleanups only successfully added tunnel, the
-		 * lookup on removal should never fail.
-		 */
-		if (WARN_ON_ONCE(!cur))
-			goto out;
-
+	} else if (cur) {
 		if (!refcount_dec_and_test(&cur->count))
 			goto out;
 
-- 
2.48.1

Re: [PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

[Willem de Bruijn]

Paolo Abeni wrote:
> The blamed commit below does not take in account that xfrm
> can enable GRO over UDP encapsulation without going through
> setup_udp_tunnel_sock().
> 
> At deletion time such socket will still go through
> udp_tunnel_cleanup_gro(), and the failed GRO type lookup will
> trigger the reported warning.
> 
> We can safely remove such warning, simply performing no action
> on failed GRO type lookup at deletion time.
> 
> Reported-by: syzbot+8c469a2260132cd095c1@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=8c469a2260132cd095c1
> Fixes: 311b36574ceac ("udp_tunnel: use static call for GRO hooks when possible")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Because XFRM does not call udp_tunnel_update_gro_rcv when enabling its
UDP GRO offload, from set_xfrm_gro_udp_encap_rcv. But it does call it
when disabling the offload, as called for all udp sockest from
udp(v6)_destroy_sock. (Just to verify my understanding.)

Not calling udp_tunnel_update_gro_rcv on add will have the unintended
side effect of enabling the static call if one other tunnel is also
active, breaking UDP GRO for XFRM socket, right? Not a significant
consequence. But eventually XFRM would want to be counted, I suppose.

Reviewed-by: Willem de Bruijn <willemb@google.com>


> ---
>  net/ipv4/udp_offload.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 088aa8cb8ac0c..2e0b52ae665bc 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -110,14 +110,7 @@ void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
>  		cur = &udp_tunnel_gro_types[udp_tunnel_gro_type_nr++];
>  		refcount_set(&cur->count, 1);
>  		cur->gro_receive = up->gro_receive;
> -	} else {
> -		/*
> -		 * The stack cleanups only successfully added tunnel, the
> -		 * lookup on removal should never fail.
> -		 */
> -		if (WARN_ON_ONCE(!cur))
> -			goto out;
> -
> +	} else if (cur) {
>  		if (!refcount_dec_and_test(&cur->count))
>  			goto out;
>  
> -- 
> 2.48.1
> 

Re: [PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

[Paolo Abeni]

On 3/19/25 3:35 PM, Willem de Bruijn wrote:
> Paolo Abeni wrote:
>> The blamed commit below does not take in account that xfrm
>> can enable GRO over UDP encapsulation without going through
>> setup_udp_tunnel_sock().
>>
>> At deletion time such socket will still go through
>> udp_tunnel_cleanup_gro(), and the failed GRO type lookup will
>> trigger the reported warning.
>>
>> We can safely remove such warning, simply performing no action
>> on failed GRO type lookup at deletion time.
>>
>> Reported-by: syzbot+8c469a2260132cd095c1@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=8c469a2260132cd095c1
>> Fixes: 311b36574ceac ("udp_tunnel: use static call for GRO hooks when possible")
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> Because XFRM does not call udp_tunnel_update_gro_rcv when enabling its
> UDP GRO offload, from set_xfrm_gro_udp_encap_rcv. But it does call it
> when disabling the offload, as called for all udp sockest from
> udp(v6)_destroy_sock. (Just to verify my understanding.)

Exactly.

> Not calling udp_tunnel_update_gro_rcv on add will have the unintended
> side effect of enabling the static call if one other tunnel is also
> active, breaking UDP GRO for XFRM socket, right?

Ouch, right again. I think we can/should do better.

Given syzkaller has found another splat with no reproducer on the other
UDP GRO change of mine [1] and we are almost at merge window time, I'm
considering reverting entirely such changes and re-submit later
(hopefully fixed). WDYT?

Thanks,

Paolo

[1] https://syzkaller.appspot.com/bug?extid=1fb3291cc1beeb3c315a
I *think* moving:

	if (!up->tunnel_list.pprev)

from udp_tunnel_cleanup_gro() into udp_tunnel_update_gro_lookup(), under
the udp_tunnel_gro_lock spinlock should fix it, but without a repro it's
a bit risky,

Re: [PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

[Willem de Bruijn]

Paolo Abeni wrote:
> 
> 
> On 3/19/25 3:35 PM, Willem de Bruijn wrote:
> > Paolo Abeni wrote:
> >> The blamed commit below does not take in account that xfrm
> >> can enable GRO over UDP encapsulation without going through
> >> setup_udp_tunnel_sock().
> >>
> >> At deletion time such socket will still go through
> >> udp_tunnel_cleanup_gro(), and the failed GRO type lookup will
> >> trigger the reported warning.
> >>
> >> We can safely remove such warning, simply performing no action
> >> on failed GRO type lookup at deletion time.
> >>
> >> Reported-by: syzbot+8c469a2260132cd095c1@syzkaller.appspotmail.com
> >> Closes: https://syzkaller.appspot.com/bug?extid=8c469a2260132cd095c1
> >> Fixes: 311b36574ceac ("udp_tunnel: use static call for GRO hooks when possible")
> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > 
> > Because XFRM does not call udp_tunnel_update_gro_rcv when enabling its
> > UDP GRO offload, from set_xfrm_gro_udp_encap_rcv. But it does call it
> > when disabling the offload, as called for all udp sockest from
> > udp(v6)_destroy_sock. (Just to verify my understanding.)
> 
> Exactly.
> 
> > Not calling udp_tunnel_update_gro_rcv on add will have the unintended
> > side effect of enabling the static call if one other tunnel is also
> > active, breaking UDP GRO for XFRM socket, right?
> 
> Ouch, right again. I think we can/should do better.
> 
> Given syzkaller has found another splat with no reproducer on the other
> UDP GRO change of mine [1] and we are almost at merge window time, I'm
> considering reverting entirely such changes and re-submit later
> (hopefully fixed). WDYT?

Your call. I suspect that we can forward fix this. But yes, that is
always the riskier approach. And from a first quick look at the
report, the right fix is not immediately glaringly obvious indeed.

> Thanks,
> 
> Paolo
> 
> [1] https://syzkaller.appspot.com/bug?extid=1fb3291cc1beeb3c315a
> I *think* moving:
> 
> 	if (!up->tunnel_list.pprev)
> 
> from udp_tunnel_cleanup_gro() into udp_tunnel_update_gro_lookup(), under
> the udp_tunnel_gro_lock spinlock should fix it, but without a repro it's
> a bit risky,
> 

Re: [PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

[Paolo Abeni]

On 3/19/25 6:44 PM, Willem de Bruijn wrote:
> Paolo Abeni wrote:
>> Given syzkaller has found another splat with no reproducer on the other
>> UDP GRO change of mine [1] and we are almost at merge window time, I'm
>> considering reverting entirely such changes and re-submit later
>> (hopefully fixed). WDYT?
> 
> Your call. I suspect that we can forward fix this. But yes, that is
> always the riskier approach. And from a first quick look at the
> report, the right fix is not immediately glaringly obvious indeed.

One problem to me is that I have my hands significantly full, since the
revert looks like the faster way out it looks the more appealing
candidate to me.

WRT the other issue, I think the problem is in udp_tunnel_cleanup_gro();
this check:

        if (!up->tunnel_list.pprev)
                return;

at sk deletion time is performed outside any lock. The current CPU could
see the old list value (empty) even if another core previously added the
sk into the UDP tunnel GRO list, thus skipping the removal from such
list. Later list operation will do UaF while touching the same list.

Moving the check under the udp_tunnel_gro_lock spinlock should solve the
issue.

Thanks,

Paolo

Re: [PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

[Willem de Bruijn]

Paolo Abeni wrote:
> On 3/19/25 6:44 PM, Willem de Bruijn wrote:
> > Paolo Abeni wrote:
> >> Given syzkaller has found another splat with no reproducer on the other
> >> UDP GRO change of mine [1] and we are almost at merge window time, I'm
> >> considering reverting entirely such changes and re-submit later
> >> (hopefully fixed). WDYT?
> > 
> > Your call. I suspect that we can forward fix this. But yes, that is
> > always the riskier approach. And from a first quick look at the
> > report, the right fix is not immediately glaringly obvious indeed.
> 
> One problem to me is that I have my hands significantly full, since the
> revert looks like the faster way out it looks the more appealing
> candidate to me.
> 
> WRT the other issue, I think the problem is in udp_tunnel_cleanup_gro();
> this check:
> 
>         if (!up->tunnel_list.pprev)
>                 return;
> 
> at sk deletion time is performed outside any lock. The current CPU could
> see the old list value (empty) even if another core previously added the
> sk into the UDP tunnel GRO list, thus skipping the removal from such
> list. Later list operation will do UaF while touching the same list.
> 
> Moving the check under the udp_tunnel_gro_lock spinlock should solve the
> issue.

FWIW, the full revert doesn't have to happen in the net-next timeframe.
Can always revert to that in -rcX. And try a forward fix first, after
the merge is over.

Re: [PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

[Sabrina Dubroca]

2025-03-19, 16:49:21 +0100, Paolo Abeni wrote:
> 
> 
> On 3/19/25 3:35 PM, Willem de Bruijn wrote:
> > Paolo Abeni wrote:
> >> The blamed commit below does not take in account that xfrm
> >> can enable GRO over UDP encapsulation without going through
> >> setup_udp_tunnel_sock().
> >>
> >> At deletion time such socket will still go through
> >> udp_tunnel_cleanup_gro(), and the failed GRO type lookup will
> >> trigger the reported warning.
> >>
> >> We can safely remove such warning, simply performing no action
> >> on failed GRO type lookup at deletion time.
> >>
> >> Reported-by: syzbot+8c469a2260132cd095c1@syzkaller.appspotmail.com
> >> Closes: https://syzkaller.appspot.com/bug?extid=8c469a2260132cd095c1
> >> Fixes: 311b36574ceac ("udp_tunnel: use static call for GRO hooks when possible")
> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > 
> > Because XFRM does not call udp_tunnel_update_gro_rcv when enabling its
> > UDP GRO offload, from set_xfrm_gro_udp_encap_rcv. But it does call it
> > when disabling the offload, as called for all udp sockest from
> > udp(v6)_destroy_sock. (Just to verify my understanding.)
> 
> Exactly.
> 
> > Not calling udp_tunnel_update_gro_rcv on add will have the unintended
> > side effect of enabling the static call if one other tunnel is also
> > active, breaking UDP GRO for XFRM socket, right?
> 
> Ouch, right again. I think we can/should do better.

We should be able to adapt xfrm to use setup_udp_tunnel_sock, but it's
not a simple conversion because GRO could be enabled separately from
the encap itself. I'm not sure there's much benefit except for a bit
more consistency when we enable the encap with GRO at once (but we'd
still have that odd set_xfrm_gro_udp_encap_rcv to enable GRO after
ESPINUDP has been set up). A few of the UDP encaps that precede
setup_udp_tunnel_sock have been converted, I don't know why ipsec was
left.

-- 
Sabrina