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

On 1/29/21 4:50 PM, John Fastabend wrote:

Jesper Dangaard Brouer wrote:

quoted

On Thu, 28 Jan 2021 22:51:23 -0800
John Fastabend [off-list ref] wrote:

quoted

Jesper Dangaard Brouer wrote:

quoted

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.

For the patch,

Acked-by: John Fastabend <john.fastabend@gmail.com>