netdev - Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from

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]

Message-ID: <YfFiEgNHwo7o78vq@lore-desk>
Date:   Wed, 26 Jan 2022 16:00:34 +0100
From:   Lorenzo Bianconi <lorenzo@...nel.org>
To:     Nikolay Aleksandrov <nikolay@...dia.com>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        bpf@...r.kernel.org, netdev@...r.kernel.org,
        lorenzo.bianconi@...hat.com, davem@...emloft.net, kuba@...nel.org,
        ast@...nel.org, daniel@...earbox.net, dsahern@...nel.org,
        komachi.yoshiki@...il.com, brouer@...hat.com, memxor@...il.com,
        andrii.nakryiko@...il.com, Roopa Prabhu <roopa@...dia.com>,
        "bridge@...ts.linux-foundation.org" 
        <bridge@...ts.linux-foundation.org>,
        Ido Schimmel <idosch@...sch.org>
Subject: Re: [RFC bpf-next 1/2] net: bridge: add unstable
 br_fdb_find_port_from_ifindex helper

> On 26/01/2022 14:50, Toke Høiland-Jørgensen wrote:
> > Nikolay Aleksandrov <nikolay@...dia.com> writes:
> > 
> >> On 26/01/2022 13:27, Lorenzo Bianconi wrote:
> >>>> On 24/01/2022 19:20, Lorenzo Bianconi wrote:
> >>>>> Similar to bpf_xdp_ct_lookup routine, introduce
> >>>>> br_fdb_find_port_from_ifindex unstable helper in order to accelerate
> >>>>> linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a
> >>>>> lookup in the associated bridge fdb table and it will return the
> >>>>> output ifindex if the destination address is associated to a bridge
> >>>>> port or -ENODEV for BOM traffic or if lookup fails.
> >>>>>
> >>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> >>>>> ---
> >>>>>  net/bridge/br.c         | 21 +++++++++++++
> >>>>>  net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
> >>>>>  net/bridge/br_private.h | 12 ++++++++
> >>>>>  3 files changed, 91 insertions(+), 9 deletions(-)
> >>>>>
> >>>>
> >>>> Hi Lorenzo,
> >>>
> >>> Hi Nikolay,
> >>>
> >>> thx for the review.
> >>>
> >>>> Please CC bridge maintainers for bridge-related patches, I've added Roopa and the
> >>>> bridge mailing list as well. Aside from that, the change is certainly interesting, I've been
> >>>> thinking about a similar helper for some time now, few comments below.
> >>>
> >>> yes, sorry for that. I figured it out after sending the series out.
> >>>
> >>>>
> >>>> Have you thought about the egress path and if by the current bridge state the packet would
> >>>> be allowed to egress through the found port from the lookup? I'd guess you have to keep updating
> >>>> the active ports list based on netlink events, but there's a lot of egress bridge logic that
> >>>> either have to be duplicated or somehow synced. Check should_deliver() (br_forward.c) and later
> >>>> egress stages, but I see how this is a good first step and perhaps we can build upon it.
> >>>> There are a few possible solutions, but I haven't tried anything yet, most obvious being
> >>>> yet another helper. :)
> >>>
> >>> ack, right but I am bit worried about adding too much logic and slow down xdp
> >>> performances. I guess we can investigate first the approach proposed by Alexei
> >>> and then revaluate. Agree?
> >>>
> >>
> >> Sure, that approach sounds very interesting, but my point was that
> >> bypassing the ingress and egress logic defeats most of the bridge
> >> features. You just get an fdb hash table which you can build today
> >> with ebpf without any changes to the kernel. :) You have multiple
> >> states, flags and options for each port and each vlan which can change
> >> dynamically based on external events (e.g. STP, config changes etc)
> >> and they can affect forwarding even if the fdbs remain in the table.
> > 
> > To me, leveraging all this is precisely the reason to have BPF helpers
> > instead of just replicating state in BPF maps: it's very easy to do that
> > and show a nice speedup, and then once you get all the corner cases
> > covered that the in-kernel code already deals with, you've chipped away
> > at that speedup and spent a lot of time essentially re-writing the
> > battle-tested code already in the kernel.
> > 
> > So I think figuring out how to do the state sync is the right thing to
> > do; a second helper would be fine for this, IMO, but I'm not really
> > familiar enough with the bridge code to really have a qualified opinion.
> > 
> > -Toke
> > 
> 
> Right, sounds good to me. IMO it should be required in order to get a meaningful bridge
> speedup, otherwise the solution is incomplete and you just do simple lookups that ignore
> all of the state that could impact the forwarding decision.

ack, I agree but I need to review it first since I am not so familiar
with that codebase :)
Doing so we can compare this solution with the one proposed by Alexei.

Regards,
Lorenzo

> 
> Cheers,
>  Nik

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)