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

[Luigi Rizzo <lrizzo@google.com>]

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.

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.

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.

cheers
luigi