Re: Checksum behaviour of bpf_redirected packets

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 6 May 2020 17:24:43 +0100 Lorenz Bauer wrote:
> On Wed, 6 May 2020 at 02:28, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Mon, May 4, 2020 at 9:12 AM Lorenz Bauer <lmb@cloudflare.com> wrote:  
> > >
> > > In our TC classifier cls_redirect [1], we use the following sequence
> > > of helper calls to
> > > decapsulate a GUE (basically IP + UDP + custom header) encapsulated packet:
> > >
> > >   skb_adjust_room(skb, -encap_len,
> > > BPF_ADJ_ROOM_MAC, BPF_F_ADJ_ROOM_FIXED_GSO)
> > >   bpf_redirect(skb->ifindex, BPF_F_INGRESS)
> > >
> > > It seems like some checksums of the inner headers are not validated in
> > > this case.
> > > For example, a TCP SYN packet with invalid TCP checksum is still accepted by the
> > > network stack and elicits a SYN ACK.
> > >
> > > Is this known but undocumented behaviour or a bug? In either case, is
> > > there a work
> > > around I'm not aware of?  
> >
> > I thought inner and outer csums are covered by different flags and driver
> > suppose to set the right one depending on level of in-hw checking it did.  
> 
> I've figured out what the problem is. We receive the following packet from
> the driver:
> 
>     | ETH | IP | UDP | GUE | IP | TCP |
>     skb->ip_summed == CHECKSUM_UNNECESSARY
> 
> ip_summed is CHECKSUM_UNNECESSARY because our NICs do rx
> checksum offloading. On this packet we run skb_adjust_room_mac(-encap),
> and get the following:
> 
>     | ETH | IP | TCP |
>     skb->ip_summed == CHECKSUM_UNNECESSARY
> 
> Note that ip_summed is still CHECKSUM_UNNECESSARY. After
> bpf_redirect()ing into the ingress, we end up in tcp_v4_rcv. There
> skb_checksum_init is turned into a no-op due to
> CHECKSUM_UNNECESSARY.
> 
> I think this boils down to bpf_skb_generic_pop not adjusting ip_summed
> accordingly. 

Sounds like we need a call to __skb_decr_checksum_unnecessary(),
but as you indicate below when and where to call it is challenging :S

> Unfortunately I don't understand how checksums work
> sufficiently. Daniel, it seems like you wrote the helper, could you
> take a look?