Re: [PATCH bpf-next V2 5/6] bpf: Add MTU check for TC-BPF packets after egress hook

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Wed, 07 Oct 2020 20:19:39 -0700
John Fastabend <john.fastabend@gmail.com> wrote:

> Maciej Żenczykowski wrote:
> > On Wed, Oct 7, 2020 at 3:37 PM John Fastabend <john.fastabend@gmail.com> wrote:  
> > >
> > > Daniel Borkmann wrote:  
> > > > On 10/7/20 6:23 PM, Jesper Dangaard Brouer wrote:
> > > > [...]  
> > > > >   net/core/dev.c |   24 ++++++++++++++++++++++--
> > > > >   1 file changed, 22 insertions(+), 2 deletions(-)  
> > >
> > > Couple high-level comments. Whats the problem with just letting the driver
> > > consume the packet? I would chalk it up to a buggy BPF program that is
> > > sending these packets. The drivers really shouldn't panic or do anything
> > > horrible under this case because even today I don't think we can be
> > > 100% certain MTU on skb matches set MTU. Imagine the case where I change
> > > the MTU from 9kB->1500B there will be some skbs in-flight with the larger
> > > length and some with the shorter. If the drivers panic/fault or otherwise
> > > does something else horrible thats not going to be friendly in general case
> > > regardless of what BPF does. And seeing this type of config is all done
> > > async its tricky (not practical) to flush any skbs in-flight.
> > >
> > > I've spent many hours debugging these types of feature flag, mtu
> > > change bugs on the driver side I'm not sure it can be resolved by
> > > the stack easily. Better to just build drivers that can handle it IMO.
> > >
> > > Do we know if sending >MTU size skbs to drivers causes problems in real
> > > cases? I haven't tried on the NICs I have here, but I expect they should
> > > be fine. Fine here being system keeps running as expected. Dropping the
> > > skb either on TX or RX side is expected. Even with this change though
> > > its possible for the skb to slip through if I configure MTU on a live
> > > system.  
> > 
> > I wholeheartedly agree with the above.
> > 
> > Ideally the only >mtu check should happen at driver admittance.
> > But again ideally it should happen in some core stack location not in
> > the driver itself.  
> 
> Ideally maybe, but IMO we should just let the skb go to the driver
> and let the driver sort it out. Even if this means pushing the packet
> onto the wire then the switch will drop it or the receiver, etc. A
> BPF program can do lots of horrible things that should never be
> on the wire otherwise. MTU is just one of them, but sending corrupted
> payloads, adding bogus headers, checksums etc. so I don't think we can
> reasonable protect against all of them.

That is a good point.

> Of course if the driver is going to hang/panic then something needs
> to be done. Perhaps a needs_mtu_check feature flag, although thats
> not so nice either so perhaps drivers just need to handle it themselves.
> Also even today the case could happen without BPF as best I can tell
> so the drivers should be prepared for it.

Yes, borderline cases do exist already today (like your reconf with
inflight packets), so I guess drivers should at-least not hang/panic
and be robust enough that we can drop this check.

I think you have convinced me that we can drop this check.  My only
concern is how people can troubleshoot this, but its not different from
current state.


> > However, due to both gso and vlan offload, even this is not trivial to do...
> > The mtu is L3, but drivers/hardware/the wire usually care about L2...

If net_device->mtu is L3 (1500) and XDP (and TC, right?) operate at L2,
that likely means that the "strict" bpf_mtu_check (in my BPF-helper) is
wrong, as XDP (and TC) length at this point include the 14 bytes
Ethernet header.  I will check and fix.

Is this accounted for via net_device->hard_header_len ?

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer