Re: BPF redirect API design issue for BPF-prog MTU feedback?

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

On Mon, 21 Sep 2020 11:04:09 -0700
John Fastabend <john.fastabend@gmail.com> wrote:

> Daniel Borkmann wrote:
> > On 9/21/20 2:49 PM, Jesper Dangaard Brouer wrote:  
> > > On Mon, 21 Sep 2020 11:37:18 +0100
> > > Lorenz Bauer <lmb@cloudflare.com> wrote:  
> > >> On Sat, 19 Sep 2020 at 00:06, Maciej Żenczykowski <maze@google.com> wrote:  
> > >>>     
> > >>>> This is a good point.  As bpf_skb_adjust_room() can just be run after
> > >>>> bpf_redirect() call, then a MTU check in bpf_redirect() actually
> > >>>> doesn't make much sense.  As clever/bad BPF program can then avoid the
> > >>>> MTU check anyhow.  This basically means that we have to do the MTU
> > >>>> check (again) on kernel side anyhow to catch such clever/bad BPF
> > >>>> programs.  (And I don't like wasting cycles on doing the same check two
> > >>>> times).  
> > >>>
> > >>> If you get rid of the check in bpf_redirect() you might as well get
> > >>> rid of *all* the checks for excessive mtu in all the helpers that
> > >>> adjust packet size one way or another way.  They *all* then become
> > >>> useless overhead.
> > >>>
[...]
> > 
> > Sorry for jumping in late here... one thing that is not clear to me is that if
> > we are fully sure that skb is dropped by stack anyway due to invalid MTU (redirect
> > to ingress does this via dev_forward_skb(), it's not fully clear to me whether it's
> > also the case for the dev_queue_xmiy()), then why not dropping all the MTU checks
> > aside from SKB_MAX_ALLOC sanity check for BPF helpers and have something like a
> > device object (similar to e.g. TCP sockets) exposed to BPF prog where we can retrieve
> > the object and read dev->mtu from the prog, so the BPF program could then do the
> > "exception" handling internally w/o extra prog needed (we also already expose whether
> > skb is GSO or not).
> > 
> > Thanks,
> > Daniel  
> 
> My $.02 is MTU should only apply to transmitted packets so redirect to
> ingress should be OK. Then on transmit shouldn't the user know the MTU
> on their devices?

I like the point that "MTU should only apply to transmitted packets". 
 
> I'm for dropping all the MTU checks and if a driver tosses a packet then
> the user should be more careful. Having a bpf helper to check MTU of a
> dev seems useful although the workaround would be a map the user could
> put the max MTU in. Of course that would be a bit fragile if the BPF program
> and person managing MTU are not in-sync.

I'm coding this up. Dropping all the MTU checks in helpers, but adding
helper to lookup/check the MTU.  I've also extended the bpf_fib_lookup
to return MTU value (it already does MTU check), as it can be more
specific.

The problematic code path seems to be when TC-ingress redirect packet
to egress on another netdev, then the normal netstack MTU checks are
skipped and driver level will not catch any MTU violation (only checked
ixgbe code path).

First I looked at adding MTU check in the egress code path of
skb_do_redirect() prior to calling dev_queue_xmit(), but I found this
to be the wrong approach.  This is because it is still possible to run
another BPF egress program that will shrink/consume headers, which will
make packet comply with netdev MTU. This use-case might already be in
production use (allowed if ingress MTU is larger than egress MTU).

Instead I'm currently coding up doing the MTU check after
sch_handle_egress() step, for the cases that require this.

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