On Tue, 2021-03-23 at 21:45 -0400, Willem de Bruijn wrote:
> On Mon, Mar 22, 2021 at 12:36 PM Paolo Abeni <pab...@redhat.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 <pab...@redhat.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 <pab...@redhat.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
