Re: [PATCHv5 net-next 1/2] udp: call udp_encap_enable for v6 sockets when enabling encap

[Antonio Quartulli <antonio@openvpn.net>]

Hi all,

On 03/02/2021 09:54, Xin Long wrote:
> When enabling encap for a ipv6 socket without udp_encap_needed_key
> increased, UDP GRO won't work for v4 mapped v6 address packets as
> sk will be NULL in udp4_gro_receive().
> 
> This patch is to enable it by increasing udp_encap_needed_key for
> v6 sockets in udp_tunnel_encap_enable(), and correspondingly
> decrease udp_encap_needed_key in udpv6_destroy_sock().
> 

This is a non-negligible issue that other users (in or out of tree) may 
hit as well.

At OpenVPN we are developing a kernel device driver that has the same 
problem as UDP GRO. So far the only workaround is to let users upgrade 
to v5.12+.

I would like to propose to take this patch in stable releases.
Greg, is this an option?

Commit in the linux kernel is:
a4a600dd301ccde6ea239804ec1f19364a39d643

Thanks a lot.

Best Regards,

> v1->v2:
>    - add udp_encap_disable() and export it.
> v2->v3:
>    - add the change for rxrpc and bareudp into one patch, as Alex
>      suggested.
> v3->v4:
>    - move rxrpc part to another patch.
> 
> Acked-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>   drivers/net/bareudp.c    | 6 ------
>   include/net/udp.h        | 1 +
>   include/net/udp_tunnel.h | 3 +--
>   net/ipv4/udp.c           | 6 ++++++
>   net/ipv6/udp.c           | 4 +++-
>   5 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> index 1b8f597..7511bca 100644
> --- a/drivers/net/bareudp.c
> +++ b/drivers/net/bareudp.c
> @@ -240,12 +240,6 @@ static int bareudp_socket_create(struct bareudp_dev *bareudp, __be16 port)
>   	tunnel_cfg.encap_destroy = NULL;
>   	setup_udp_tunnel_sock(bareudp->net, sock, &tunnel_cfg);
>   
> -	/* As the setup_udp_tunnel_sock does not call udp_encap_enable if the
> -	 * socket type is v6 an explicit call to udp_encap_enable is needed.
> -	 */
> -	if (sock->sk->sk_family == AF_INET6)
> -		udp_encap_enable();
> -
>   	rcu_assign_pointer(bareudp->sock, sock);
>   	return 0;
>   }
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 01351ba..5ddbb42 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -467,6 +467,7 @@ void udp_init(void);
>   
>   DECLARE_STATIC_KEY_FALSE(udp_encap_needed_key);
>   void udp_encap_enable(void);
> +void udp_encap_disable(void);
>   #if IS_ENABLED(CONFIG_IPV6)
>   DECLARE_STATIC_KEY_FALSE(udpv6_encap_needed_key);
>   void udpv6_encap_enable(void);
> diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
> index 282d10e..afc7ce7 100644
> --- a/include/net/udp_tunnel.h
> +++ b/include/net/udp_tunnel.h
> @@ -181,9 +181,8 @@ static inline void udp_tunnel_encap_enable(struct socket *sock)
>   #if IS_ENABLED(CONFIG_IPV6)
>   	if (sock->sk->sk_family == PF_INET6)
>   		ipv6_stub->udpv6_encap_enable();
> -	else
>   #endif
> -		udp_encap_enable();
> +	udp_encap_enable();
>   }
>   
>   #define UDP_TUNNEL_NIC_MAX_TABLES	4
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 69ea765..48208fb 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -596,6 +596,12 @@ void udp_encap_enable(void)
>   }
>   EXPORT_SYMBOL(udp_encap_enable);
>   
> +void udp_encap_disable(void)
> +{
> +	static_branch_dec(&udp_encap_needed_key);
> +}
> +EXPORT_SYMBOL(udp_encap_disable);
> +
>   /* Handler for tunnels with arbitrary destination ports: no socket lookup, go
>    * through error handlers in encapsulations looking for a match.
>    */
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index b9f3dfd..d754292 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1608,8 +1608,10 @@ void udpv6_destroy_sock(struct sock *sk)
>   			if (encap_destroy)
>   				encap_destroy(sk);
>   		}
> -		if (up->encap_enabled)
> +		if (up->encap_enabled) {
>   			static_branch_dec(&udpv6_encap_needed_key);
> +			udp_encap_disable();
> +		}
>   	}
>   
>   	inet6_destroy_sock(sk);

-- 
Antonio Quartulli
OpenVPN Inc.