Re: [PATCH bpf-next V13 4/7] bpf: add BPF-helper for MTU checking

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

On Sat, 30 Jan 2021 01:08:17 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 1/29/21 4:50 PM, John Fastabend wrote:
> > Jesper Dangaard Brouer wrote:  
> >> On Thu, 28 Jan 2021 22:51:23 -0800
> >> John Fastabend <john.fastabend@gmail.com> wrote:  
> >>> Jesper Dangaard Brouer wrote:  
> >>>> This BPF-helper bpf_check_mtu() works for both XDP and TC-BPF programs.
> >>>>
> >>>> The SKB object is complex and the skb->len value (accessible from
> >>>> BPF-prog) also include the length of any extra GRO/GSO segments, but
> >>>> without taking into account that these GRO/GSO segments get added
> >>>> transport (L4) and network (L3) headers before being transmitted. Thus,
> >>>> this BPF-helper is created such that the BPF-programmer don't need to
> >>>> handle these details in the BPF-prog.
> >>>>
> >>>> The API is designed to help the BPF-programmer, that want to do packet
> >>>> context size changes, which involves other helpers. These other helpers
> >>>> usually does a delta size adjustment. This helper also support a delta
> >>>> size (len_diff), which allow BPF-programmer to reuse arguments needed by
> >>>> these other helpers, and perform the MTU check prior to doing any actual
> >>>> size adjustment of the packet context.
> >>>>
> >>>> It is on purpose, that we allow the len adjustment to become a negative
> >>>> result, that will pass the MTU check. This might seem weird, but it's not
> >>>> this helpers responsibility to "catch" wrong len_diff adjustments. Other
> >>>> helpers will take care of these checks, if BPF-programmer chooses to do
> >>>> actual size adjustment.  
> >>
> >> The nitpick below about len adjust can become negative, is on purpose
> >> and why is described in above.  
> > 
> > following up on a nitpick :)
> > 
> > What is the use case to allow users to push a negative len_diff with
> > abs(len_diff) > skb_diff and not throw an error. I would understand if it
> > was a pain to catch the case, but below is fairly straightforward. Of
> > course if user really tries to truncate the packet like this later it
> > will also throw an error, but still missing why we don't throw an error
> > here.
> > 
> > Anyways its undefined if len_diff is truely bogus. Its not really a
> > problem I guess because garbage in (bogus len_diff) garbage out is OK I
> > think.  
> 
> What's the rationale to not sanity check for it? I just double checked
> the UAPI helper description comment ... at minimum this behavior would
> need to be documented there to avoid confusion.

The rationale is that the helper asks if the packet size adjustment
will exceed the MTU (on the given ifindex).  It is not this helpers
responsibility to catch if the packet becomes too small.  It the
responsibility of the helper function that does the size change. The
use-case for len_diff is testing prior to doing size adjustment.

The code can easily choose not to do the size adjustment.  E.g. when
parsing the header, and realizing this is not a VXLAN (50 bytes) tunnel
packet, but instead a (small 42 bytes) ARP packet.

Sure, I can spin a V14 of the patchset, where I make it more clear for
the man page that this is the behavior.

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