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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 20 Jan 2021 10:55:32 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     network dev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Martin Varghese <martin.varghese@...ia.com>
Subject: Re: [PATCHv3 net-next 1/2] udp: call udp_encap_enable for v6 sockets
 when enabling encap

On Wed, Jan 20, 2021 at 6:17 AM Alexander Duyck
<alexander.duyck@...il.com> wrote:
>
> On Fri, Jan 15, 2021 at 8:34 PM Xin Long <lucien.xin@...il.com> 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().
> >
> > v1->v2:
> >   - add udp_encap_disable() and export it.
> >
> > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > ---
> >  include/net/udp.h        | 1 +
> >  include/net/udp_tunnel.h | 3 +--
> >  net/ipv4/udp.c           | 6 ++++++
> >  net/ipv6/udp.c           | 4 +++-
> >  4 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/net/udp.h b/include/net/udp.h
> > index 877832b..1e7b6cd 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 7103b0a..28bfe60 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.
> >   */
>
> So this seems unbalanced to me. We are adding/modifying one spot where
> we are calling the enable function, but the other callers don't call
> the disable function? Specifically I am curious about how to deal with
> the rxrpc_open_socket usage.
as long as it's a UDP sock, when it's being destroyed,
udp(v6)_destroy_sock will be called, where the key(s) get decreased.

Sorry, I missed there's a call to udp_encap_enable() in rxrpc, the issue is
the similar to the one in bareudp, it should be:

-       udp_encap_enable();
 #if IS_ENABLED(CONFIG_AF_RXRPC_IPV6)
        if (local->srx.transport.family == AF_INET6)
                udpv6_encap_enable();
+       else
 #endif
+       udp_encap_enable();
+

I will get all these into one patch and repost.

Interesting it doesn't use udp_tunnel APIs here, I will check.

Thanks.

>
> If we don't balance out all the callers I am not sure adding the
> udp_encap_disable makes much sense.
>
> > 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);
> > --
> > 2.1.0
> >