[PATCH net-next v2 0/5] udp_tunnel: GRO optimization follow-up

[PATCH net-next v2 0/5] udp_tunnel: GRO optimization follow-up

[Paolo Abeni]

Syzkaller and Nathan reported a few issues on the reference changeset.
This series tries to address them all and additionally does some little
cleanup.

See the individual patches for the details.
---
Should this prove to be too invasive, too late or ineffective, I'll be
ok with a revert before the upcoming net-next PR.

Paolo Abeni (5):
  udp_tunnel: properly deal with xfrm gro encap.
  udp_tunnel: fix compile warning
  udp_tunnel: fix UaF in GRO accounting
  udp_tunnel: avoid inconsistent local variables usage
  udp_tunnel: prevent GRO lookup optimization for user-space sockets

 include/net/udp_tunnel.h   | 4 ----
 net/ipv4/udp.c             | 5 +++++
 net/ipv4/udp_offload.c     | 9 ++++++---
 net/ipv4/udp_tunnel_core.c | 7 ++++---
 4 files changed, 15 insertions(+), 10 deletions(-)

-- 
2.48.1

[PATCH net-next v2 1/5] 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.

Add the GRO accounting for XFRM tunnel when GRO is enabled, and
adjust the known gro types accordingly.

Note that we can't use setup_udp_tunnel_sock() here, as the xfrm
tunnel setup can be "incremental" - e.g. the encapsulation is created
first and GRO is enabled later.

Also we can not allow GRO sk lookup optimization for XFRM tunnels, as
the socket could match the selection criteria at enable time, and
later on the user-space could disconnect/bind it breaking such
criteria.

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>
---
v1 -> v2:
 - do proper account for xfrm, retain the warning
---
 net/ipv4/udp.c         | 5 +++++
 net/ipv4/udp_offload.c | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index db606f7e41638..79efbf465fb04 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2903,10 +2903,15 @@ static void set_xfrm_gro_udp_encap_rcv(__u16 encap_type, unsigned short family,
 {
 #ifdef CONFIG_XFRM
 	if (udp_test_bit(GRO_ENABLED, sk) && encap_type == UDP_ENCAP_ESPINUDP) {
+		bool old_enabled = !!udp_sk(sk)->gro_receive;
+
 		if (family == AF_INET)
 			WRITE_ONCE(udp_sk(sk)->gro_receive, xfrm4_gro_udp_encap_rcv);
 		else if (IS_ENABLED(CONFIG_IPV6) && family == AF_INET6)
 			WRITE_ONCE(udp_sk(sk)->gro_receive, ipv6_stub->xfrm6_gro_udp_encap_rcv);
+
+		if (!old_enabled && udp_sk(sk)->gro_receive)
+			udp_tunnel_update_gro_rcv(sk, true);
 	}
 #endif
 }
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 088aa8cb8ac0c..02365b818f1af 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -37,9 +37,11 @@ struct udp_tunnel_type_entry {
 	refcount_t count;
 };
 
+/* vxlan, fou and xfrm have 2 different gro_receive hooks each */
 #define UDP_MAX_TUNNEL_TYPES (IS_ENABLED(CONFIG_GENEVE) + \
 			      IS_ENABLED(CONFIG_VXLAN) * 2 + \
-			      IS_ENABLED(CONFIG_NET_FOU) * 2)
+			      IS_ENABLED(CONFIG_NET_FOU) * 2 + \
+			      IS_ENABLED(CONFIG_XFRM) * 2)
 
 DEFINE_STATIC_CALL(udp_tunnel_gro_rcv, dummy_gro_rcv);
 static DEFINE_STATIC_KEY_FALSE(udp_tunnel_static_call);
-- 
2.48.1

Re: [PATCH net-next v2 1/5] 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.
> 
> Add the GRO accounting for XFRM tunnel when GRO is enabled, and
> adjust the known gro types accordingly.
> 
> Note that we can't use setup_udp_tunnel_sock() here, as the xfrm
> tunnel setup can be "incremental" - e.g. the encapsulation is created
> first and GRO is enabled later.
> 
> Also we can not allow GRO sk lookup optimization for XFRM tunnels, as
> the socket could match the selection criteria at enable time, and
> later on the user-space could disconnect/bind it breaking such
> criteria.
> 
> 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>
> ---
> v1 -> v2:
>  - do proper account for xfrm, retain the warning
> ---
>  net/ipv4/udp.c         | 5 +++++
>  net/ipv4/udp_offload.c | 4 +++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index db606f7e41638..79efbf465fb04 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2903,10 +2903,15 @@ static void set_xfrm_gro_udp_encap_rcv(__u16 encap_type, unsigned short family,
>  {
>  #ifdef CONFIG_XFRM
>  	if (udp_test_bit(GRO_ENABLED, sk) && encap_type == UDP_ENCAP_ESPINUDP) {
> +		bool old_enabled = !!udp_sk(sk)->gro_receive;
> +
>  		if (family == AF_INET)
>  			WRITE_ONCE(udp_sk(sk)->gro_receive, xfrm4_gro_udp_encap_rcv);
>  		else if (IS_ENABLED(CONFIG_IPV6) && family == AF_INET6)
>  			WRITE_ONCE(udp_sk(sk)->gro_receive, ipv6_stub->xfrm6_gro_udp_encap_rcv);
> +
> +		if (!old_enabled && udp_sk(sk)->gro_receive)
> +			udp_tunnel_update_gro_rcv(sk, true);

The second part of the condition is always true right?

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

[Paolo Abeni]

Hi,

On 3/21/25 5:33 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.
>>
>> Add the GRO accounting for XFRM tunnel when GRO is enabled, and
>> adjust the known gro types accordingly.
>>
>> Note that we can't use setup_udp_tunnel_sock() here, as the xfrm
>> tunnel setup can be "incremental" - e.g. the encapsulation is created
>> first and GRO is enabled later.
>>
>> Also we can not allow GRO sk lookup optimization for XFRM tunnels, as
>> the socket could match the selection criteria at enable time, and
>> later on the user-space could disconnect/bind it breaking such
>> criteria.
>>
>> 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>
>> ---
>> v1 -> v2:
>>  - do proper account for xfrm, retain the warning
>> ---
>>  net/ipv4/udp.c         | 5 +++++
>>  net/ipv4/udp_offload.c | 4 +++-
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index db606f7e41638..79efbf465fb04 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -2903,10 +2903,15 @@ static void set_xfrm_gro_udp_encap_rcv(__u16 encap_type, unsigned short family,
>>  {
>>  #ifdef CONFIG_XFRM
>>  	if (udp_test_bit(GRO_ENABLED, sk) && encap_type == UDP_ENCAP_ESPINUDP) {
>> +		bool old_enabled = !!udp_sk(sk)->gro_receive;
>> +
>>  		if (family == AF_INET)
>>  			WRITE_ONCE(udp_sk(sk)->gro_receive, xfrm4_gro_udp_encap_rcv);
>>  		else if (IS_ENABLED(CONFIG_IPV6) && family == AF_INET6)
>>  			WRITE_ONCE(udp_sk(sk)->gro_receive, ipv6_stub->xfrm6_gro_udp_encap_rcv);
>> +
>> +		if (!old_enabled && udp_sk(sk)->gro_receive)
>> +			udp_tunnel_update_gro_rcv(sk, true);
> 
> The second part of the condition is always true right?

Jakub noted my initial reply did not land on the ML, sorry.

Yes, AFAICS the second part of the condition should be always true, or
at least I fail to see how to reach there otherwise.

Still syzkaller is too good to prove me wrong, I guess it's good to err
on the pedantic/safe side here - checking that condition explicitly.
It's also IMHO more readable.

Cheers,

Paolo

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

[Sabrina Dubroca]

2025-03-21, 12:52:52 +0100, 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.
> 
> Add the GRO accounting for XFRM tunnel when GRO is enabled, and
> adjust the known gro types accordingly.
> 
> Note that we can't use setup_udp_tunnel_sock() here, as the xfrm
> tunnel setup can be "incremental" - e.g. the encapsulation is created
> first and GRO is enabled later.
> 
> Also we can not allow GRO sk lookup optimization for XFRM tunnels, as
> the socket could match the selection criteria at enable time, and
> later on the user-space could disconnect/bind it breaking such
> criteria.
> 
> 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>
> ---
> v1 -> v2:
>  - do proper account for xfrm, retain the warning
> ---
>  net/ipv4/udp.c         | 5 +++++
>  net/ipv4/udp_offload.c | 4 +++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index db606f7e41638..79efbf465fb04 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2903,10 +2903,15 @@ static void set_xfrm_gro_udp_encap_rcv(__u16 encap_type, unsigned short family,
>  {
>  #ifdef CONFIG_XFRM
>  	if (udp_test_bit(GRO_ENABLED, sk) && encap_type == UDP_ENCAP_ESPINUDP) {
> +		bool old_enabled = !!udp_sk(sk)->gro_receive;
> +
>  		if (family == AF_INET)
>  			WRITE_ONCE(udp_sk(sk)->gro_receive, xfrm4_gro_udp_encap_rcv);
>  		else if (IS_ENABLED(CONFIG_IPV6) && family == AF_INET6)
>  			WRITE_ONCE(udp_sk(sk)->gro_receive, ipv6_stub->xfrm6_gro_udp_encap_rcv);
> +
> +		if (!old_enabled && udp_sk(sk)->gro_receive)
> +			udp_tunnel_update_gro_rcv(sk, true);

We're not under any lock at this point, so this is a bit racy. I think
we'll "only" end up leaking a ref on cur->count if this happens. Not
ideal, but much better than the current situation.

>  	}
>  #endif
>  }
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 088aa8cb8ac0c..02365b818f1af 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -37,9 +37,11 @@ struct udp_tunnel_type_entry {
>  	refcount_t count;
>  };
>  
> +/* vxlan, fou and xfrm have 2 different gro_receive hooks each */
>  #define UDP_MAX_TUNNEL_TYPES (IS_ENABLED(CONFIG_GENEVE) + \
>  			      IS_ENABLED(CONFIG_VXLAN) * 2 + \
> -			      IS_ENABLED(CONFIG_NET_FOU) * 2)
> +			      IS_ENABLED(CONFIG_NET_FOU) * 2 + \
> +			      IS_ENABLED(CONFIG_XFRM) * 2)
>  
>  DEFINE_STATIC_CALL(udp_tunnel_gro_rcv, dummy_gro_rcv);
>  static DEFINE_STATIC_KEY_FALSE(udp_tunnel_static_call);
> -- 
> 2.48.1
> 
> 

-- 
Sabrina

[PATCH net-next v2 2/5] udp_tunnel: fix compile warning

[Paolo Abeni]

Nathan reported that the compiler is not happy to use a zero
size udp_tunnel_gro_types array:

  net/ipv4/udp_offload.c:130:8: warning: array index 0 is past the end of the array (that has type 'struct udp_tunnel_type_entry[0]') [-Warray-bounds]
    130 |                                    udp_tunnel_gro_types[0].gro_receive);
        |                                    ^                    ~
  include/linux/static_call.h:154:42: note: expanded from macro 'static_call_update'
    154 |         typeof(&STATIC_CALL_TRAMP(name)) __F = (func);                  \
        |                                                 ^~~~
  net/ipv4/udp_offload.c:47:1: note: array 'udp_tunnel_gro_types' declared here
     47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
        | ^
  1 warning generated.

In such (unusual) configuration we should skip entirely the
static call optimization accounting.

Reported-by: syzbot+8c469a2260132cd095c1@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/cover.1741718157.git.pabeni@redhat.com/T/#m6e309a49f04330de81a618c3c166368252ba42e4
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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 02365b818f1af..fd2b8e3830beb 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -83,7 +83,7 @@ void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
 	struct udp_sock *up = udp_sk(sk);
 	int i, old_gro_type_nr;
 
-	if (!up->gro_receive)
+	if (!UDP_MAX_TUNNEL_TYPES || !up->gro_receive)
 		return;
 
 	mutex_lock(&udp_tunnel_gro_type_lock);
-- 
2.48.1

Re: [PATCH net-next v2 2/5] udp_tunnel: fix compile warning

[Willem de Bruijn]

Paolo Abeni wrote:
> Nathan reported that the compiler is not happy to use a zero
> size udp_tunnel_gro_types array:
> 
>   net/ipv4/udp_offload.c:130:8: warning: array index 0 is past the end of the array (that has type 'struct udp_tunnel_type_entry[0]') [-Warray-bounds]
>     130 |                                    udp_tunnel_gro_types[0].gro_receive);
>         |                                    ^                    ~
>   include/linux/static_call.h:154:42: note: expanded from macro 'static_call_update'
>     154 |         typeof(&STATIC_CALL_TRAMP(name)) __F = (func);                  \
>         |                                                 ^~~~
>   net/ipv4/udp_offload.c:47:1: note: array 'udp_tunnel_gro_types' declared here
>      47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
>         | ^
>   1 warning generated.
> 
> In such (unusual) configuration we should skip entirely the
> static call optimization accounting.
> 
> Reported-by: syzbot+8c469a2260132cd095c1@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/cover.1741718157.git.pabeni@redhat.com/T/#m6e309a49f04330de81a618c3c166368252ba42e4
> Fixes: 311b36574ceac ("udp_tunnel: use static call for GRO hooks when possible")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Should CONFIG_NET_UDP_TUNNEL just not be user selectable and only
enabled by implementations of UDP tunnels like vxlan and geneve?

> ---
>  net/ipv4/udp_offload.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 02365b818f1af..fd2b8e3830beb 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -83,7 +83,7 @@ void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
>  	struct udp_sock *up = udp_sk(sk);
>  	int i, old_gro_type_nr;
>  
> -	if (!up->gro_receive)
> +	if (!UDP_MAX_TUNNEL_TYPES || !up->gro_receive)
>  		return;

If that is too risky, I suppose this workaround is sufficient.
But having a zero length array seems a bit odd.

Re: [PATCH net-next v2 2/5] udp_tunnel: fix compile warning

[Sabrina Dubroca]

2025-03-21, 12:35:34 -0400, Willem de Bruijn wrote:
> Paolo Abeni wrote:
> > Nathan reported that the compiler is not happy to use a zero
> > size udp_tunnel_gro_types array:
> > 
> >   net/ipv4/udp_offload.c:130:8: warning: array index 0 is past the end of the array (that has type 'struct udp_tunnel_type_entry[0]') [-Warray-bounds]
> >     130 |                                    udp_tunnel_gro_types[0].gro_receive);
> >         |                                    ^                    ~
> >   include/linux/static_call.h:154:42: note: expanded from macro 'static_call_update'
> >     154 |         typeof(&STATIC_CALL_TRAMP(name)) __F = (func);                  \
> >         |                                                 ^~~~
> >   net/ipv4/udp_offload.c:47:1: note: array 'udp_tunnel_gro_types' declared here
> >      47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
> >         | ^
> >   1 warning generated.
> > 
> > In such (unusual) configuration we should skip entirely the
> > static call optimization accounting.
> > 
> > Reported-by: syzbot+8c469a2260132cd095c1@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/netdev/cover.1741718157.git.pabeni@redhat.com/T/#m6e309a49f04330de81a618c3c166368252ba42e4
> > Fixes: 311b36574ceac ("udp_tunnel: use static call for GRO hooks when possible")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> Should CONFIG_NET_UDP_TUNNEL just not be user selectable and only
> enabled by implementations of UDP tunnels like vxlan and geneve?

It's already not user selectable?

config NET_UDP_TUNNEL
	tristate

(no string after bool/tristate, so no manual config)

But there are tunnels that don't do GRO, so they're not counted in
UDP_MAX_TUNNEL_TYPES, and if only those types are enabled, we'll have
CONFIG_NET_UDP_TUNNEL=y with UDP_MAX_TUNNEL_TYPES == 0.


> > ---
> >  net/ipv4/udp_offload.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 02365b818f1af..fd2b8e3830beb 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -83,7 +83,7 @@ void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
> >  	struct udp_sock *up = udp_sk(sk);
> >  	int i, old_gro_type_nr;
> >  
> > -	if (!up->gro_receive)
> > +	if (!UDP_MAX_TUNNEL_TYPES || !up->gro_receive)
> >  		return;
> 
> If that is too risky, I suppose this workaround is sufficient.
> But having a zero length array seems a bit odd.

I think the alternative would be to add

    #if UDP_MAX_TUNNEL_TYPES > 0

around some of this code.

-- 
Sabrina

Re: [PATCH net-next v2 2/5] udp_tunnel: fix compile warning

[Sabrina Dubroca]

2025-03-21, 12:52:53 +0100, Paolo Abeni wrote:
> Nathan reported that the compiler is not happy to use a zero
> size udp_tunnel_gro_types array:
> 
>   net/ipv4/udp_offload.c:130:8: warning: array index 0 is past the end of the array (that has type 'struct udp_tunnel_type_entry[0]') [-Warray-bounds]
>     130 |                                    udp_tunnel_gro_types[0].gro_receive);
>         |                                    ^                    ~
>   include/linux/static_call.h:154:42: note: expanded from macro 'static_call_update'
>     154 |         typeof(&STATIC_CALL_TRAMP(name)) __F = (func);                  \
>         |                                                 ^~~~
>   net/ipv4/udp_offload.c:47:1: note: array 'udp_tunnel_gro_types' declared here
>      47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
>         | ^
>   1 warning generated.
> 
> In such (unusual) configuration we should skip entirely the
> static call optimization accounting.
> 
> Reported-by: syzbot+8c469a2260132cd095c1@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/cover.1741718157.git.pabeni@redhat.com/T/#m6e309a49f04330de81a618c3c166368252ba42e4
> Fixes: 311b36574ceac ("udp_tunnel: use static call for GRO hooks when possible")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

[PATCH net-next v2 3/5] udp_tunnel: fix UaF in GRO accounting

[Paolo Abeni]

Siyzkaller reported a race in UDP tunnel GRO accounting, leading to
UaF:

BUG: KASAN: slab-use-after-free in udp_tunnel_update_gro_lookup+0x23c/0x2c0 net/ipv4/udp_offload.c:65
Read of size 8 at addr ffff88801235ebe8 by task syz.2.655/7921

CPU: 1 UID: 0 PID: 7921 Comm: syz.2.655 Not tainted 6.14.0-rc6-syzkaller-01313-g23c9ff659140 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2025
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x241/0x360 lib/dump_stack.c:120
 print_address_description mm/kasan/report.c:408 [inline]
 print_report+0x16e/0x5b0 mm/kasan/report.c:521
 kasan_report+0x143/0x180 mm/kasan/report.c:634
 udp_tunnel_update_gro_lookup+0x23c/0x2c0 net/ipv4/udp_offload.c:65
 sk_common_release+0x71/0x2e0 net/core/sock.c:3896
 inet_release+0x17d/0x200 net/ipv4/af_inet.c:435
 __sock_release net/socket.c:647 [inline]
 sock_release+0x82/0x150 net/socket.c:675
 sock_free drivers/net/wireguard/socket.c:339 [inline]
 wg_socket_reinit+0x215/0x380 drivers/net/wireguard/socket.c:435
 wg_stop+0x59f/0x600 drivers/net/wireguard/device.c:133
 __dev_close_many+0x3a6/0x700 net/core/dev.c:1717
 dev_close_many+0x24e/0x4c0 net/core/dev.c:1742
 unregister_netdevice_many_notify+0x629/0x24f0 net/core/dev.c:11923
 rtnl_delete_link net/core/rtnetlink.c:3512 [inline]
 rtnl_dellink+0x526/0x8c0 net/core/rtnetlink.c:3554
 rtnetlink_rcv_msg+0x791/0xcf0 net/core/rtnetlink.c:6945
 netlink_rcv_skb+0x206/0x480 net/netlink/af_netlink.c:2534
 netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
 netlink_unicast+0x7f6/0x990 net/netlink/af_netlink.c:1339
 netlink_sendmsg+0x8de/0xcb0 net/netlink/af_netlink.c:1883
 sock_sendmsg_nosec net/socket.c:709 [inline]
 __sock_sendmsg+0x221/0x270 net/socket.c:724
 ____sys_sendmsg+0x53a/0x860 net/socket.c:2564
 ___sys_sendmsg net/socket.c:2618 [inline]
 __sys_sendmsg+0x269/0x350 net/socket.c:2650
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f35ab38d169
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f35ac28f038 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00007f35ab5a6160 RCX: 00007f35ab38d169
RDX: 0000000000000000 RSI: 0000400000000000 RDI: 0000000000000004
RBP: 00007f35ab40e2a0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 0000000000000001 R14: 00007f35ab5a6160 R15: 00007ffdddd781b8
 </TASK>

Allocated by task 7770:
 kasan_save_stack mm/kasan/common.c:47 [inline]
 kasan_save_track+0x3f/0x80 mm/kasan/common.c:68
 unpoison_slab_object mm/kasan/common.c:319 [inline]
 __kasan_slab_alloc+0x66/0x80 mm/kasan/common.c:345
 kasan_slab_alloc include/linux/kasan.h:250 [inline]
 slab_post_alloc_hook mm/slub.c:4115 [inline]
 slab_alloc_node mm/slub.c:4164 [inline]
 kmem_cache_alloc_noprof+0x1d9/0x380 mm/slub.c:4171
 sk_prot_alloc+0x58/0x210 net/core/sock.c:2190
 sk_alloc+0x3e/0x370 net/core/sock.c:2249
 inet_create+0x648/0xea0 net/ipv4/af_inet.c:326
 __sock_create+0x4c0/0xa30 net/socket.c:1539
 sock_create net/socket.c:1597 [inline]
 __sys_socket_create net/socket.c:1634 [inline]
 __sys_socket+0x150/0x3c0 net/socket.c:1681
 __do_sys_socket net/socket.c:1695 [inline]
 __se_sys_socket net/socket.c:1693 [inline]
 __x64_sys_socket+0x7a/0x90 net/socket.c:1693
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Freed by task 7768:
 kasan_save_stack mm/kasan/common.c:47 [inline]
 kasan_save_track+0x3f/0x80 mm/kasan/common.c:68
 kasan_save_free_info+0x40/0x50 mm/kasan/generic.c:576
 poison_slab_object mm/kasan/common.c:247 [inline]
 __kasan_slab_free+0x59/0x70 mm/kasan/common.c:264
 kasan_slab_free include/linux/kasan.h:233 [inline]
 slab_free_hook mm/slub.c:2353 [inline]
 slab_free mm/slub.c:4609 [inline]
 kmem_cache_free+0x195/0x410 mm/slub.c:4711
 sk_prot_free net/core/sock.c:2230 [inline]
 __sk_destruct+0x4fd/0x690 net/core/sock.c:2327
 inet_release+0x17d/0x200 net/ipv4/af_inet.c:435
 __sock_release net/socket.c:647 [inline]
 sock_close+0xbc/0x240 net/socket.c:1389
 __fput+0x3e9/0x9f0 fs/file_table.c:464
 task_work_run+0x24f/0x310 kernel/task_work.c:227
 resume_user_mode_work include/linux/resume_user_mode.h:50 [inline]
 exit_to_user_mode_loop kernel/entry/common.c:114 [inline]
 exit_to_user_mode_prepare include/linux/entry-common.h:329 [inline]
 __syscall_exit_to_user_mode_work kernel/entry/common.c:207 [inline]
 syscall_exit_to_user_mode+0x13f/0x340 kernel/entry/common.c:218
 do_syscall_64+0x100/0x230 arch/x86/entry/common.c:89
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

The buggy address belongs to the object at ffff88801235e4c0
 which belongs to the cache UDP of size 1856
The buggy address is located 1832 bytes inside of
 freed 1856-byte region [ffff88801235e4c0, ffff88801235ec00)

At disposal time, to avoid unconditionally acquiring a spin lock, UDP
tunnel sockets are conditionally removed from the known tunnels list
only if the socket is actually present in such a list.

Such check happens outside the socket lock scope: the current CPU
could observe an uninitialized list entry even if the tunnel has been
actually registered by a different core.

Address the issue moving the blamed check under the relevant list
spin lock.

Reported-by: syzbot+1fb3291cc1beeb3c315a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1fb3291cc1beeb3c315a
Fixes: 8d4880db37835 ("udp_tunnel: create a fastpath GRO lookup.")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/udp_tunnel.h | 4 ----
 net/ipv4/udp_offload.c   | 3 ++-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index a7b230867eb14..13b54e6856414 100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -214,14 +214,10 @@ static inline void udp_tunnel_update_gro_rcv(struct sock *sk, bool add) {}
 
 static inline void udp_tunnel_cleanup_gro(struct sock *sk)
 {
-	struct udp_sock *up = udp_sk(sk);
 	struct net *net = sock_net(sk);
 
 	udp_tunnel_update_gro_rcv(sk, false);
 
-	if (!up->tunnel_list.pprev)
-		return;
-
 	udp_tunnel_update_gro_lookup(net, sk, false);
 }
 
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index fd2b8e3830beb..b124355a36aee 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -57,10 +57,11 @@ void udp_tunnel_update_gro_lookup(struct net *net, struct sock *sk, bool add)
 	struct udp_tunnel_gro *udp_tunnel_gro;
 
 	spin_lock(&udp_tunnel_gro_lock);
+
 	udp_tunnel_gro = &net->ipv4.udp_tunnel_gro[is_ipv6];
 	if (add)
 		hlist_add_head(&up->tunnel_list, &udp_tunnel_gro->list);
-	else
+	else if (up->tunnel_list.pprev)
 		hlist_del_init(&up->tunnel_list);
 
 	if (udp_tunnel_gro->list.first &&
-- 
2.48.1

Re: [PATCH net-next v2 3/5] udp_tunnel: fix UaF in GRO accounting

[Willem de Bruijn]

Paolo Abeni wrote:
> Siyzkaller reported a race in UDP tunnel GRO accounting, leading to
> UaF:
> 
> BUG: KASAN: slab-use-after-free in udp_tunnel_update_gro_lookup+0x23c/0x2c0 net/ipv4/udp_offload.c:65
> Read of size 8 at addr ffff88801235ebe8 by task syz.2.655/7921
> 
> CPU: 1 UID: 0 PID: 7921 Comm: syz.2.655 Not tainted 6.14.0-rc6-syzkaller-01313-g23c9ff659140 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2025
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:94 [inline]
>  dump_stack_lvl+0x241/0x360 lib/dump_stack.c:120
>  print_address_description mm/kasan/report.c:408 [inline]
>  print_report+0x16e/0x5b0 mm/kasan/report.c:521
>  kasan_report+0x143/0x180 mm/kasan/report.c:634
>  udp_tunnel_update_gro_lookup+0x23c/0x2c0 net/ipv4/udp_offload.c:65
>  sk_common_release+0x71/0x2e0 net/core/sock.c:3896
>  inet_release+0x17d/0x200 net/ipv4/af_inet.c:435
>  __sock_release net/socket.c:647 [inline]
>  sock_release+0x82/0x150 net/socket.c:675
>  sock_free drivers/net/wireguard/socket.c:339 [inline]
>  wg_socket_reinit+0x215/0x380 drivers/net/wireguard/socket.c:435
>  wg_stop+0x59f/0x600 drivers/net/wireguard/device.c:133
>  __dev_close_many+0x3a6/0x700 net/core/dev.c:1717
>  dev_close_many+0x24e/0x4c0 net/core/dev.c:1742
>  unregister_netdevice_many_notify+0x629/0x24f0 net/core/dev.c:11923
>  rtnl_delete_link net/core/rtnetlink.c:3512 [inline]
>  rtnl_dellink+0x526/0x8c0 net/core/rtnetlink.c:3554
>  rtnetlink_rcv_msg+0x791/0xcf0 net/core/rtnetlink.c:6945
>  netlink_rcv_skb+0x206/0x480 net/netlink/af_netlink.c:2534
>  netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
>  netlink_unicast+0x7f6/0x990 net/netlink/af_netlink.c:1339
>  netlink_sendmsg+0x8de/0xcb0 net/netlink/af_netlink.c:1883
>  sock_sendmsg_nosec net/socket.c:709 [inline]
>  __sock_sendmsg+0x221/0x270 net/socket.c:724
>  ____sys_sendmsg+0x53a/0x860 net/socket.c:2564
>  ___sys_sendmsg net/socket.c:2618 [inline]
>  __sys_sendmsg+0x269/0x350 net/socket.c:2650
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f35ab38d169
> Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f35ac28f038 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 00007f35ab5a6160 RCX: 00007f35ab38d169
> RDX: 0000000000000000 RSI: 0000400000000000 RDI: 0000000000000004
> RBP: 00007f35ab40e2a0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 0000000000000001 R14: 00007f35ab5a6160 R15: 00007ffdddd781b8
>  </TASK>
> 
> Allocated by task 7770:
>  kasan_save_stack mm/kasan/common.c:47 [inline]
>  kasan_save_track+0x3f/0x80 mm/kasan/common.c:68
>  unpoison_slab_object mm/kasan/common.c:319 [inline]
>  __kasan_slab_alloc+0x66/0x80 mm/kasan/common.c:345
>  kasan_slab_alloc include/linux/kasan.h:250 [inline]
>  slab_post_alloc_hook mm/slub.c:4115 [inline]
>  slab_alloc_node mm/slub.c:4164 [inline]
>  kmem_cache_alloc_noprof+0x1d9/0x380 mm/slub.c:4171
>  sk_prot_alloc+0x58/0x210 net/core/sock.c:2190
>  sk_alloc+0x3e/0x370 net/core/sock.c:2249
>  inet_create+0x648/0xea0 net/ipv4/af_inet.c:326
>  __sock_create+0x4c0/0xa30 net/socket.c:1539
>  sock_create net/socket.c:1597 [inline]
>  __sys_socket_create net/socket.c:1634 [inline]
>  __sys_socket+0x150/0x3c0 net/socket.c:1681
>  __do_sys_socket net/socket.c:1695 [inline]
>  __se_sys_socket net/socket.c:1693 [inline]
>  __x64_sys_socket+0x7a/0x90 net/socket.c:1693
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Freed by task 7768:
>  kasan_save_stack mm/kasan/common.c:47 [inline]
>  kasan_save_track+0x3f/0x80 mm/kasan/common.c:68
>  kasan_save_free_info+0x40/0x50 mm/kasan/generic.c:576
>  poison_slab_object mm/kasan/common.c:247 [inline]
>  __kasan_slab_free+0x59/0x70 mm/kasan/common.c:264
>  kasan_slab_free include/linux/kasan.h:233 [inline]
>  slab_free_hook mm/slub.c:2353 [inline]
>  slab_free mm/slub.c:4609 [inline]
>  kmem_cache_free+0x195/0x410 mm/slub.c:4711
>  sk_prot_free net/core/sock.c:2230 [inline]
>  __sk_destruct+0x4fd/0x690 net/core/sock.c:2327
>  inet_release+0x17d/0x200 net/ipv4/af_inet.c:435
>  __sock_release net/socket.c:647 [inline]
>  sock_close+0xbc/0x240 net/socket.c:1389
>  __fput+0x3e9/0x9f0 fs/file_table.c:464
>  task_work_run+0x24f/0x310 kernel/task_work.c:227
>  resume_user_mode_work include/linux/resume_user_mode.h:50 [inline]
>  exit_to_user_mode_loop kernel/entry/common.c:114 [inline]
>  exit_to_user_mode_prepare include/linux/entry-common.h:329 [inline]
>  __syscall_exit_to_user_mode_work kernel/entry/common.c:207 [inline]
>  syscall_exit_to_user_mode+0x13f/0x340 kernel/entry/common.c:218
>  do_syscall_64+0x100/0x230 arch/x86/entry/common.c:89
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> The buggy address belongs to the object at ffff88801235e4c0
>  which belongs to the cache UDP of size 1856
> The buggy address is located 1832 bytes inside of
>  freed 1856-byte region [ffff88801235e4c0, ffff88801235ec00)
> 
> At disposal time, to avoid unconditionally acquiring a spin lock, UDP
> tunnel sockets are conditionally removed from the known tunnels list
> only if the socket is actually present in such a list.
> 
> Such check happens outside the socket lock scope: the current CPU
> could observe an uninitialized list entry even if the tunnel has been
> actually registered by a different core.
> 
> Address the issue moving the blamed check under the relevant list
> spin lock.
> 
> Reported-by: syzbot+1fb3291cc1beeb3c315a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1fb3291cc1beeb3c315a
> Fixes: 8d4880db37835 ("udp_tunnel: create a fastpath GRO lookup.")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

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

Re: [PATCH net-next v2 3/5] udp_tunnel: fix UaF in GRO accounting

[Sabrina Dubroca]

2025-03-21, 12:52:54 +0100, Paolo Abeni wrote:
> Siyzkaller reported a race in UDP tunnel GRO accounting, leading to
> UaF:
> 
> BUG: KASAN: slab-use-after-free in udp_tunnel_update_gro_lookup+0x23c/0x2c0 net/ipv4/udp_offload.c:65
> Read of size 8 at addr ffff88801235ebe8 by task syz.2.655/7921
> 
> CPU: 1 UID: 0 PID: 7921 Comm: syz.2.655 Not tainted 6.14.0-rc6-syzkaller-01313-g23c9ff659140 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2025
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:94 [inline]
>  dump_stack_lvl+0x241/0x360 lib/dump_stack.c:120
>  print_address_description mm/kasan/report.c:408 [inline]
>  print_report+0x16e/0x5b0 mm/kasan/report.c:521
>  kasan_report+0x143/0x180 mm/kasan/report.c:634
>  udp_tunnel_update_gro_lookup+0x23c/0x2c0 net/ipv4/udp_offload.c:65
>  sk_common_release+0x71/0x2e0 net/core/sock.c:3896
>  inet_release+0x17d/0x200 net/ipv4/af_inet.c:435
>  __sock_release net/socket.c:647 [inline]
>  sock_release+0x82/0x150 net/socket.c:675
>  sock_free drivers/net/wireguard/socket.c:339 [inline]
>  wg_socket_reinit+0x215/0x380 drivers/net/wireguard/socket.c:435
>  wg_stop+0x59f/0x600 drivers/net/wireguard/device.c:133
>  __dev_close_many+0x3a6/0x700 net/core/dev.c:1717
>  dev_close_many+0x24e/0x4c0 net/core/dev.c:1742
>  unregister_netdevice_many_notify+0x629/0x24f0 net/core/dev.c:11923
>  rtnl_delete_link net/core/rtnetlink.c:3512 [inline]
>  rtnl_dellink+0x526/0x8c0 net/core/rtnetlink.c:3554
>  rtnetlink_rcv_msg+0x791/0xcf0 net/core/rtnetlink.c:6945
>  netlink_rcv_skb+0x206/0x480 net/netlink/af_netlink.c:2534
>  netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
>  netlink_unicast+0x7f6/0x990 net/netlink/af_netlink.c:1339
>  netlink_sendmsg+0x8de/0xcb0 net/netlink/af_netlink.c:1883
>  sock_sendmsg_nosec net/socket.c:709 [inline]
>  __sock_sendmsg+0x221/0x270 net/socket.c:724
>  ____sys_sendmsg+0x53a/0x860 net/socket.c:2564
>  ___sys_sendmsg net/socket.c:2618 [inline]
>  __sys_sendmsg+0x269/0x350 net/socket.c:2650
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f35ab38d169
> Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f35ac28f038 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 00007f35ab5a6160 RCX: 00007f35ab38d169
> RDX: 0000000000000000 RSI: 0000400000000000 RDI: 0000000000000004
> RBP: 00007f35ab40e2a0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 0000000000000001 R14: 00007f35ab5a6160 R15: 00007ffdddd781b8
>  </TASK>
> 
> Allocated by task 7770:
>  kasan_save_stack mm/kasan/common.c:47 [inline]
>  kasan_save_track+0x3f/0x80 mm/kasan/common.c:68
>  unpoison_slab_object mm/kasan/common.c:319 [inline]
>  __kasan_slab_alloc+0x66/0x80 mm/kasan/common.c:345
>  kasan_slab_alloc include/linux/kasan.h:250 [inline]
>  slab_post_alloc_hook mm/slub.c:4115 [inline]
>  slab_alloc_node mm/slub.c:4164 [inline]
>  kmem_cache_alloc_noprof+0x1d9/0x380 mm/slub.c:4171
>  sk_prot_alloc+0x58/0x210 net/core/sock.c:2190
>  sk_alloc+0x3e/0x370 net/core/sock.c:2249
>  inet_create+0x648/0xea0 net/ipv4/af_inet.c:326
>  __sock_create+0x4c0/0xa30 net/socket.c:1539
>  sock_create net/socket.c:1597 [inline]
>  __sys_socket_create net/socket.c:1634 [inline]
>  __sys_socket+0x150/0x3c0 net/socket.c:1681
>  __do_sys_socket net/socket.c:1695 [inline]
>  __se_sys_socket net/socket.c:1693 [inline]
>  __x64_sys_socket+0x7a/0x90 net/socket.c:1693
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Freed by task 7768:
>  kasan_save_stack mm/kasan/common.c:47 [inline]
>  kasan_save_track+0x3f/0x80 mm/kasan/common.c:68
>  kasan_save_free_info+0x40/0x50 mm/kasan/generic.c:576
>  poison_slab_object mm/kasan/common.c:247 [inline]
>  __kasan_slab_free+0x59/0x70 mm/kasan/common.c:264
>  kasan_slab_free include/linux/kasan.h:233 [inline]
>  slab_free_hook mm/slub.c:2353 [inline]
>  slab_free mm/slub.c:4609 [inline]
>  kmem_cache_free+0x195/0x410 mm/slub.c:4711
>  sk_prot_free net/core/sock.c:2230 [inline]
>  __sk_destruct+0x4fd/0x690 net/core/sock.c:2327
>  inet_release+0x17d/0x200 net/ipv4/af_inet.c:435
>  __sock_release net/socket.c:647 [inline]
>  sock_close+0xbc/0x240 net/socket.c:1389
>  __fput+0x3e9/0x9f0 fs/file_table.c:464
>  task_work_run+0x24f/0x310 kernel/task_work.c:227
>  resume_user_mode_work include/linux/resume_user_mode.h:50 [inline]
>  exit_to_user_mode_loop kernel/entry/common.c:114 [inline]
>  exit_to_user_mode_prepare include/linux/entry-common.h:329 [inline]
>  __syscall_exit_to_user_mode_work kernel/entry/common.c:207 [inline]
>  syscall_exit_to_user_mode+0x13f/0x340 kernel/entry/common.c:218
>  do_syscall_64+0x100/0x230 arch/x86/entry/common.c:89
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> The buggy address belongs to the object at ffff88801235e4c0
>  which belongs to the cache UDP of size 1856
> The buggy address is located 1832 bytes inside of
>  freed 1856-byte region [ffff88801235e4c0, ffff88801235ec00)
> 
> At disposal time, to avoid unconditionally acquiring a spin lock, UDP
> tunnel sockets are conditionally removed from the known tunnels list
> only if the socket is actually present in such a list.
> 
> Such check happens outside the socket lock scope: the current CPU
> could observe an uninitialized list entry even if the tunnel has been
> actually registered by a different core.
> 
> Address the issue moving the blamed check under the relevant list
> spin lock.
> 
> Reported-by: syzbot+1fb3291cc1beeb3c315a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1fb3291cc1beeb3c315a
> Fixes: 8d4880db37835 ("udp_tunnel: create a fastpath GRO lookup.")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

[PATCH net-next v2 4/5] udp_tunnel: avoid inconsistent local variables usage

[Paolo Abeni]

In setup_udp_tunnel_sock() 'sk' and 'sock->sk' are alias. The code
I introduced there uses alternatively both variables, for no good
reasons.

Stick to 'sk' usage, to be consistent with the prior code.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv4/udp_tunnel_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
index c49fceea83139..aa9016619c25a 100644
--- a/net/ipv4/udp_tunnel_core.c
+++ b/net/ipv4/udp_tunnel_core.c
@@ -90,10 +90,10 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
 
 	udp_tunnel_encap_enable(sk);
 
-	udp_tunnel_update_gro_rcv(sock->sk, true);
+	udp_tunnel_update_gro_rcv(sk, true);
 
-	if (!sk->sk_dport && !sk->sk_bound_dev_if && sk_saddr_any(sock->sk))
-		udp_tunnel_update_gro_lookup(net, sock->sk, true);
+	if (!sk->sk_dport && !sk->sk_bound_dev_if && sk_saddr_any(sk))
+		udp_tunnel_update_gro_lookup(net, sk, true);
 }
 EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
 
-- 
2.48.1

Re: [PATCH net-next v2 4/5] udp_tunnel: avoid inconsistent local variables usage

[Willem de Bruijn]

Paolo Abeni wrote:
> In setup_udp_tunnel_sock() 'sk' and 'sock->sk' are alias. The code
> I introduced there uses alternatively both variables, for no good
> reasons.
> 
> Stick to 'sk' usage, to be consistent with the prior code.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

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

Re: [PATCH net-next v2 4/5] udp_tunnel: avoid inconsistent local variables usage

[Sabrina Dubroca]

2025-03-21, 12:52:55 +0100, Paolo Abeni wrote:
> In setup_udp_tunnel_sock() 'sk' and 'sock->sk' are alias. The code
> I introduced there uses alternatively both variables, for no good
> reasons.
> 
> Stick to 'sk' usage, to be consistent with the prior code.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

[PATCH net-next v2 5/5] udp_tunnel: prevent GRO lookup optimization for user-space sockets

[Paolo Abeni]

UDP tunnel sockets owned by the kernel are never disconnected/rebound
after setup_udp_tunnel_sock(), but sockets owned by the user-space could
go through such changes after being matching the criteria to enable
the GRO lookup optimization, breaking them.

Explicitly prevent user-space owned sockets from leveraging such
optimization.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv4/udp_tunnel_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
index aa9016619c25a..2326548997d3f 100644
--- a/net/ipv4/udp_tunnel_core.c
+++ b/net/ipv4/udp_tunnel_core.c
@@ -92,7 +92,8 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
 
 	udp_tunnel_update_gro_rcv(sk, true);
 
-	if (!sk->sk_dport && !sk->sk_bound_dev_if && sk_saddr_any(sk))
+	if (!sk->sk_dport && !sk->sk_bound_dev_if && sk_saddr_any(sk) &&
+	    sk->sk_kern_sock)
 		udp_tunnel_update_gro_lookup(net, sk, true);
 }
 EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
-- 
2.48.1

Re: [PATCH net-next v2 5/5] udp_tunnel: prevent GRO lookup optimization for user-space sockets

[Willem de Bruijn]

Paolo Abeni wrote:
> UDP tunnel sockets owned by the kernel are never disconnected/rebound
> after setup_udp_tunnel_sock(), but sockets owned by the user-space could
> go through such changes after being matching the criteria to enable
> the GRO lookup optimization, breaking them.
> 
> Explicitly prevent user-space owned sockets from leveraging such
> optimization.

UDP tunnel sockets are generally initialized with udp_sock_create,
which calls sock_create_kern.

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

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

Re: [PATCH net-next v2 5/5] udp_tunnel: prevent GRO lookup optimization for user-space sockets

[Sabrina Dubroca]

2025-03-21, 12:52:56 +0100, Paolo Abeni wrote:
> UDP tunnel sockets owned by the kernel are never disconnected/rebound
> after setup_udp_tunnel_sock(), but sockets owned by the user-space could
> go through such changes after being matching the criteria to enable
> the GRO lookup optimization, breaking them.
> 
> Explicitly prevent user-space owned sockets from leveraging such
> optimization.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

Re: [PATCH net-next v2 0/5] udp_tunnel: GRO optimization follow-up

[Jakub Kicinski]

On Fri, 21 Mar 2025 12:52:51 +0100 Paolo Abeni wrote:
> Should this prove to be too invasive, too late or ineffective, I'll be
> ok with a revert before the upcoming net-next PR.

Looks like the conversation isn't dying down and the patches were
merged quite recently, so revert isn't too risky. I'll go ahead
and revert.
-- 
pw-bot: nap