netdev - Re: [RFC PATCH v3 06/10] udp: cope with UDP GRO packet misdirection

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-LbaAEF7XS8BT8i1uaZTrGpmAEYvf3Yw7DHFKUKMmGzNA@mail.gmail.com>
Date:   Thu, 1 Nov 2018 17:01:12 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>,
        steffen.klassert@...unet.com,
        Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
Subject: Re: [RFC PATCH v3 06/10] udp: cope with UDP GRO packet misdirection

On Wed, Oct 31, 2018 at 5:57 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Tue, 2018-10-30 at 18:24 +0100, Paolo Abeni wrote:
> > --- a/include/net/udp.h
> > +++ b/include/net/udp.h
> > @@ -406,17 +406,24 @@ static inline int copy_linear_skb(struct sk_buff *skb, int len, int off,
> >  } while(0)
> >
> >  #if IS_ENABLED(CONFIG_IPV6)
> > -#define __UDPX_INC_STATS(sk, field)                                  \
> > -do {                                                                 \
> > -     if ((sk)->sk_family == AF_INET)                                 \
> > -             __UDP_INC_STATS(sock_net(sk), field, 0);                \
> > -     else                                                            \
> > -             __UDP6_INC_STATS(sock_net(sk), field, 0);               \
> > -} while (0)
> > +#define __UDPX_MIB(sk, ipv4)                                         \
> > +({                                                                   \
> > +     ipv4 ? (IS_UDPLITE(sk) ? sock_net(sk)->mib.udplite_statistics : \
> > +                              sock_net(sk)->mib.udp_statistics) :    \
> > +             (IS_UDPLITE(sk) ? sock_net(sk)->mib.udplite_stats_in6 : \
> > +                              sock_net(sk)->mib.udp_stats_in6);      \
> > +})
> >  #else
> > -#define __UDPX_INC_STATS(sk, field) __UDP_INC_STATS(sock_net(sk), field, 0)
> > +#define __UDPX_MIB(sk, ipv4)                                         \
> > +({                                                                   \
> > +     IS_UDPLITE(sk) ? sock_net(sk)->mib.udplite_statistics :         \
> > +                      sock_net(sk)->mib.udp_statistics;              \
> > +})
> >  #endif
> >
> > +#define __UDPX_INC_STATS(sk, field) \
> > +     __SNMP_INC_STATS(__UDPX_MIB(sk, (sk)->sk_family == AF_INET, field)
> > +
>
> This is broken (complains only if CONFIG_AF_RXRPC is set), will fix in
> next iteration (thanks kbuildbot).
>
> But I'd prefer to keep the above helper: it can be used in a follow-up
> patch to cleanup a bit udp6_recvmsg().
>
> >  #ifdef CONFIG_PROC_FS
> >  struct udp_seq_afinfo {
> >       sa_family_t                     family;
> > @@ -450,4 +457,32 @@ DECLARE_STATIC_KEY_FALSE(udpv6_encap_needed_key);
> >  void udpv6_encap_enable(void);
> >  #endif
> >
> > +static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
> > +                                           struct sk_buff *skb)
> > +{
> > +     bool ipv4 = skb->protocol == htons(ETH_P_IP);
>
> And this cause a compile warning when # CONFIG_IPV6 is not set, I will
> fix in the next iteration (again thanks kbuildbot)

Can also just pass it as argument. This skb->protocol should work correctly
with tunneled packets, but it wasn't as immediately obvious to me.

Also

+       if (unlikely(!segs))
+               goto drop;

this should not happen. But if it could and the caller treats it the
same as error (both now return NULL), then skb needs to be freed.