netdev - Re: [PATCH bpf-next V3 4/6] bpf: make it possible to identify BPF redirected SKBs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 12 Oct 2020 14:04:08 -0700
From:   Maciej Żenczykowski <maze@...gle.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>, bpf <bpf@...r.kernel.org>,
        Linux NetDev <netdev@...r.kernel.org>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Shaun Crampton <shaun@...era.io>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Marek Majkowski <marek@...udflare.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eyal Birger <eyal.birger@...il.com>
Subject: Re: [PATCH bpf-next V3 4/6] bpf: make it possible to identify BPF
 redirected SKBs

On Sat, Oct 10, 2020 at 4:09 AM Jesper Dangaard Brouer
<brouer@...hat.com> wrote:
>
> On Fri, 9 Oct 2020 11:33:33 -0700
> Maciej Żenczykowski <maze@...gle.com> wrote:
>
> > > > This change makes it possible to identify SKBs that have been redirected
> > > > by TC-BPF (cls_act). This is needed for a number of cases.
> > > >
> > > > (1) For collaborating with driver ifb net_devices.
> > > > (2) For avoiding starting generic-XDP prog on TC ingress redirect.
> > > >
> > > > Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> > >
> > > Not sure if anyone actually cares about ifb devices, but my worry is that the
> > > generic XDP vs tc interaction has been as-is for quite some time so this change
> > > in behavior could break in the wild.
>
> No, I believe this happened as recent at kernel v5.2, when Stephen
> Hemminger changed this in commit 458bf2f224f0 ("net: core: support XDP
> generic on stacked devices.").  And for the record I think that
> patch/change was a mistake, as people should not use generic-XDP for
> these kind of stacked devices (they should really use TC-BPF as that is
> the right tool for the job).
>
>
> > I'm not at all sure of the interactions/implications here.
> > But I do have a request to enable ifb on Android for ingress rate
> > limiting and separately we're trying to make XDP work...
> > So we might at some point end up with cellular interfaces with xdp
> > ebpf (redirect for forwarding/nat/tethering) + ifb + tc ebpf (for
> > device local stuff).
>
> To me I was very surprised when I discovered tc-redirect didn't work
> with ifb driver.  And it sounds like you have an actual use-case for
> this on Android.
>
> > But this is still all very vague and 'ideas only' level.
> > (and in general I think I'd like to get rid of the redirect in tc
> > ebpf, and leave only xlat64 translation for to-the-device traffic in
> > there, so maybe there's no problem anyway??)
>
> I know it sounds strange coming from me "Mr.XDP", but I actaully think
> that in many cases you will be better off with using TC-BPF.
> Especially on Android, as it will be very hard to get native-XDP
> implemented in all these different drivers. (And you don't want to use
> generic-XDP, because there is a high chance it causes a reallocation of
> the SKB, which is a huge performance hit).

We want the benefits of not allocating/zeroing skb metadata.
We probably can't (always) do zerocopy...

But let's list what we have on at least 1 sample device:
(a) cellular interface receives, no LRO, into skb, no build_skb
so each packet is <= mtu and requires meta alloc, meta zero, payload alloc
on some devices, payload is copied because nic does not receive into
all of system RAM, just SWMMIO style into a small ~60MB buffer.
(b) GRO happens
(c) TC BPF with redirect or routing/forwarding/iptables
(d) GSO happens, cause no TSO at NCM usb driver
(e) NCM driver copies payload, discards skb.
[and it allocates around 1 more skb per 16KB]

so we basically have at least 2 allocs and 2 payload copies per <=1500 packet
(and cellular mtus are likely closer to 1280 then 1500)

Lots of room for improvement - GRO/GSO are probably a net loss
(unclear) and all that allocation/copies.
If I can get XDP to eliminate the skb meta allocation and the fast
path payload copy in the cellular driver.
(so we only have copy from xdp frame into skb), then it's already a
huge win - we're down to 1 copy in NCM driver.
NCM could technically not require a copy with USB controller SG, but
current demo patches for that are not a win.
(Most likely usb controller is crappy, but lots of work left)
If forwarding/tethering is through XDP Redirect, then I also win due
to no GRO/GSO on that path.
(at least I think so)