Re: [PATCH] net-xdp: netdev attribute to control xdpgeneric skb linearization

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Luigi Rizzo <lrizzo@google.com> writes:

> On Fri, Jan 24, 2020 at 1:57 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Daniel Borkmann <daniel@iogearbox.net> writes:
>>
>> > On 1/23/20 7:06 PM, Luigi Rizzo wrote:
>> >> On Thu, Jan 23, 2020 at 10:01 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>> Luigi Rizzo <lrizzo@google.com> writes:
>> >>>> On Thu, Jan 23, 2020 at 8:14 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>>>> Daniel Borkmann <daniel@iogearbox.net> writes:
>> >>>>>> On 1/23/20 10:53 AM, Toke Høiland-Jørgensen wrote:
>> >>>>>>> Luigi Rizzo <lrizzo@google.com> writes:
>> >>>>>>>
>> >>>>>>>> Add a netdevice flag to control skb linearization in generic xdp mode.
>> >>>>>>>> Among the various mechanism to control the flag, the sysfs
>> >>>>>>>> interface seems sufficiently simple and self-contained.
>> >>>>>>>> The attribute can be modified through
>> >>>>>>>>      /sys/class/net/<DEVICE>/xdp_linearize
>> >>>>>>>> The default is 1 (on)
>> >>>>>>
>> >>>>>> Needs documentation in Documentation/ABI/testing/sysfs-class-net.
>> >>>>>>
>> >>>>>>> Erm, won't turning off linearization break the XDP program's ability to
>> >>>>>>> do direct packet access?
>> >>>>>>
>> >>>>>> Yes, in the worst case you only have eth header pulled into linear
>> >>>>>> section. :/
>> >>>>>
>> >>>>> In which case an eBPF program could read/write out of bounds since the
>> >>>>> verifier only verifies checks against xdp->data_end. Right?
>> >>>>
>> >>>> Why out of bounds? Without linearization we construct xdp_buff as follows:
>> >>>>
>> >>>> mac_len = skb->data - skb_mac_header(skb);
>> >>>> hlen = skb_headlen(skb) + mac_len;
>> >>>> xdp->data = skb->data - mac_len;
>> >>>> xdp->data_end = xdp->data + hlen;
>> >>>> xdp->data_hard_start = skb->data - skb_headroom(skb);
>> >>>>
>> >>>> so we shouldn't go out of bounds.
>> >>>
>> >>> Hmm, right, as long as it's guaranteed that the bit up to hlen is
>> >>> already linear; is it? :)
>> >>
>> >> honest question: that would be skb->len - skb->data_len, isn't that
>> >> the linear part by definition ?
>> >
>> > Yep, that's the linear part by definition. Generic XDP with ->data/->data_end is in
>> > this aspect no different from tc/BPF where we operate on skb context. Only linear part
>> > can be covered from skb (unless you pull in more via helper for the
>> > latter).
>>
>> OK, but then why are we linearising in the first place? Just to get
>> sufficient headroom?
>
> Looking at the condition in the if() it is both to make sufficient
> headroom available and have linear data so the bpf code can access all
> the packet data.

Ohhh, didn't realise that linearising also changes skb_headlen() - makes
so much more sense now :)

> My motivation for this change is that enforcing those guarantees has
> significant cost (even for native xdp in the cases I mentioned - mtu >
> 1 page, hw LRO, header split), and this is an interim solution to make
> generic skb usable without too much penalty.

Sure, that part I understand; I just don't like that this "interim"
solution makes generic and native XDP diverge further in their
semantics...

> In the long term I think it would be good if the xdp program could
> express its requirements at load time ("i just need header, I need at
> least 18 bytes of headroom..") and have the netdev or nic driver
> reconfigure as appropriate.

This may be interesting to include in the XDP feature detection
capabilities we've been discussing for some time. Our current thinking
is that the verifier should detect what a program does, rather than the
program having to explicitly declare what features it needs. See
https://github.com/xdp-project/xdp-project/blob/master/xdp-project.org#notes-implementation-plan
for some notes on this :)

-Toke