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>
On Sat, 30 Jan 2021 01:08:17 +0100
Daniel Borkmann [off-list ref] wrote:
On 1/29/21 4:50 PM, John Fastabend wrote:
quoted
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.
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