Message-ID: <5143c873078583ef0f12d08ccf966d6b4640b9ee.camel@redhat.com>
Date:   Wed, 24 Mar 2021 15:37:07 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Alexander Lobakin <alobakin@...me>
Subject: Re: [PATCH net-next 1/8] udp: fixup csum for GSO receive slow path

On Tue, 2021-03-23 at 21:45 -0400, Willem de Bruijn wrote:
> On Mon, Mar 22, 2021 at 12:36 PM Paolo Abeni <pabeni@...hat.com> wrote:
> > On Mon, 2021-03-22 at 09:18 -0400, Willem de Bruijn wrote:
> > > On Sun, Mar 21, 2021 at 1:01 PM Paolo Abeni <pabeni@...hat.com> wrote:
> > > > When looping back UDP GSO over UDP tunnel packets to an UDP socket,
> > > > the individual packet csum is currently set to CSUM_NONE. That causes
> > > > unexpected/wrong csum validation errors later in the UDP receive path.
> > > > 
> > > > We could possibly addressing the issue with some additional check and
> > > > csum mangling in the UDP tunnel code. Since the issue affects only
> > > > this UDP receive slow path, let's set a suitable csum status there.
> > > > 
> > > > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > > > ---
> > > >  include/net/udp.h | 18 ++++++++++++++++++
> > > >  net/ipv4/udp.c    | 10 ++++++++++
> > > >  net/ipv6/udp.c    |  5 +++++
> > > >  3 files changed, 33 insertions(+)
> > > > 
> > > > diff --git a/include/net/udp.h b/include/net/udp.h
> > > > index d4d064c592328..007683eb3e113 100644
> > > > --- a/include/net/udp.h
> > > > +++ b/include/net/udp.h
> > > > @@ -515,6 +515,24 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
> > > >         return segs;
> > > >  }
> > > > 
> > > > +static inline void udp_post_segment_fix_csum(struct sk_buff *skb, int level)
> > > > +{
> > > > +       /* UDP-lite can't land here - no GRO */
> > > > +       WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);
> > > > +
> > > > +       /* GRO already validated the csum up to 'level', and we just
> > > > +        * consumed one header, update the skb accordingly
> > > > +        */
> > > > +       UDP_SKB_CB(skb)->cscov = skb->len;
> > > > +       if (level) {
> > > > +               skb->ip_summed = CHECKSUM_UNNECESSARY;
> > > > +               skb->csum_level = 0;
> > > > +       } else {
> > > > +               skb->ip_summed = CHECKSUM_NONE;
> > > > +               skb->csum_valid = 1;
> > > > +       }
> > > 
> > > why does this function also update these fields for non-tunneled
> > > packets? the commit only describes an issue with tunneled packets.
> > > 
> > > > +}
> > > > +
> > > >  #ifdef CONFIG_BPF_SYSCALL
> > > >  struct sk_psock;
> > > >  struct proto *udp_bpf_get_proto(struct sock *sk, struct sk_psock *psock);
> > > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > > > index 4a0478b17243a..ff54135c51ffa 100644
> > > > --- a/net/ipv4/udp.c
> > > > +++ b/net/ipv4/udp.c
> > > > @@ -2168,6 +2168,7 @@ static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
> > > >  static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> > > >  {
> > > >         struct sk_buff *next, *segs;
> > > > +       int csum_level;
> > > >         int ret;
> > > > 
> > > >         if (likely(!udp_unexpected_gso(sk, skb)))
> > > > @@ -2175,9 +2176,18 @@ static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> > > > 
> > > >         BUILD_BUG_ON(sizeof(struct udp_skb_cb) > SKB_GSO_CB_OFFSET);
> > > >         __skb_push(skb, -skb_mac_offset(skb));
> > > > +       csum_level = !!(skb_shinfo(skb)->gso_type &
> > > > +                       (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM));
> > > >         segs = udp_rcv_segment(sk, skb, true);
> > > >         skb_list_walk_safe(segs, skb, next) {
> > > >                 __skb_pull(skb, skb_transport_offset(skb));
> > > > +
> > > > +               /* UDP GSO packets looped back after adding UDP encap land here with CHECKSUM none,
> > > > +                * instead of adding another check in the tunnel fastpath, we can force valid
> > > > +                * csums here (packets are locally generated).
> > > > +                * Additionally fixup the UDP CB
> > > > +                */
> > > > +               udp_post_segment_fix_csum(skb, csum_level);
> > > 
> > > How does this code differentiates locally generated packets with udp
> > > tunnel headers from packets arriving from the wire, for which the
> > > inner checksum may be incorrect?
> > 
> > First thing first, thank you for the detailed review. Digesting all the
> > comments will take time, so please excuse for some latency.
> 
> Apologies for my own delayed response. I also need to take time to
> understand the existing code and diffs :) And have a few questions.
> 
> > I'll try to reply to both your question here because the replies are
> > related.
> > 
> > My understanding is that UDP GRO, when processing UDP over UDP traffic
> 
> This is a UDP GSO packet egress packet that was further encapsulated
> with a GSO_UDP_TUNNEL on egress, then looped to the ingress path?
> 
> Then in the ingress path it has traversed the GRO layer.
> 
> Is this veth with XDP? That seems unlikely for GSO packets. But there
> aren't many paths that will loop a packet through napi_gro_receive or
> napi_gro_frags.

This patch addresses the following scenario:

sk ->vxlan -> veth -> (xdp in use, TSO disabled, GRO on) -> veth -> vxlan -> sk

What I meant here is that the issue is not visible with:

(baremetal, NETIF_F_GRO_UDP_FWD | NETIF_F_GRO_FRAGLIST enabled -> vxlan -> sk

> > with the appropriate features bit set, will validate the checksum for
> > both the inner and the outer header - udp{4,6}_gro_receive will be
> > traversed twice, the fist one for the outer header, the 2nd for the
> > inner.
> 
> GRO will validate multiple levels of checksums with CHECKSUM_COMPLETE.
> It can only validate the outer checksum with CHECKSUM_UNNECESSARY, I
> believe?

I possibly miss some bits here ?!?

AFAICS:

udp4_gro_receive() -> skb_gro_checksum_validate_zero_check() ->
__skb_gro_checksum_validate -> (if  not already valid)
__skb_gro_checksum_validate_complete() -> (if not CHECKSUM_COMPLETE)
__skb_gro_checksum_complete()

the latter will validate the UDP checksum at the current nesting level
(and set the csum-related bits in the GRO skb cb to the same status
as CHECKSUM_COMPLETE)

When processing UDP over UDP tunnel packet, the gro call chain will be:

[l2/l3 GRO] -> udp4_gro_receive (validate outher header csum) -> 
udp_sk(sk)->gro_receive -> [other gro layers] -> 
udp4_gro_receive (validate inner header csum) -> ...

> As for looped packets with CHECKSUM_PARTIAL: we definitely have found
> bugs in that path before. I think it's fine to set csum_valid on any
> packets that can unambiguously be identified as such. Hence the
> detailed questions above on which exact packets this code is
> targeting, so that there are not accidental false positives that look
> the same but have a different ip_summed.

I see this change is controversial. Since the addressed scenario is
really a corner case, a simpler alternative would be
replacing udp_post_segment_fix_csum with:

static inline void udp_post_segment_fix_cb(struct sk_buff *skb, int level)
{
	/* UDP-lite can't land here - no GRO */
	WARN_ON_ONCE(UDP_SKB_CB(skb)->partial_cov);

	/* UDP CB mirrors the GSO packet, we must re-init it */
	UDP_SKB_CB(skb)->cscov = skb->len;
}

the downside will be an additional, later, unneeded csum validation for the 

sk ->vxlan -> veth -> (xdp in use, TSO disabled, GRO on) -> veth -> vxlan -> sk

scenario. WDYT?

Thanks!

Paolo

Powered by blists - more mailing lists