netdev - Re: [PATCHv4 bpf-next 2/4] xdp: extend xdp_redirect

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: <606f922584d89_c8b92089a@john-XPS-13-9370.notmuch>
Date:   Thu, 08 Apr 2021 16:30:45 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Hangbin Liu <liuhangbin@...il.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Jiri Benc <jbenc@...hat.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>, ast@...nel.org,
        Daniel Borkmann <daniel@...earbox.net>,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        David Ahern <dsahern@...il.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Björn Töpel <bjorn.topel@...il.com>
Subject: Re: [PATCHv4 bpf-next 2/4] xdp: extend xdp_redirect_map with
 broadcast support

Toke Høiland-Jørgensen wrote:
> John Fastabend <john.fastabend@...il.com> writes:
> 
> > Toke Høiland-Jørgensen wrote:
> >> John Fastabend <john.fastabend@...il.com> writes:
> >> 
> >> > Toke Høiland-Jørgensen wrote:
> >> >> Hangbin Liu <liuhangbin@...il.com> writes:
> >> >> 
> >> >> > On Mon, Apr 05, 2021 at 05:24:48PM -0700, John Fastabend wrote:
> >> >> >> Hangbin Liu wrote:
> >> >> >> > This patch add two flags BPF_F_BROADCAST and BPF_F_EXCLUDE_INGRESS to extend
> >> >> >> > xdp_redirect_map for broadcast support.

[...]

> >> > Have we consider doing something like the batch lookup ops over hashtab?
> >> > I don't mind "missing" values so if we just walk the list?
> >> >
> >> >      head = dev_map_index_hash(dtab, i)
> >> >      // collect all my devs and get ready to send multicast
> >> >      hlist_nulls_for_each_entry_safe(dev, next, head, index_hlist) {
> >> > 		enqueue(dev, skb)
> >> >      }
> >> >      // submit the queue of entries and do all the work to actually xmit
> >> >      submit_enqueued();
> >> >
> >> > We don't have to care about keys just walk the hash list?
> >> 
> >> So you'd wrap that in a loop like:
> >> 
> >> for (i = 0; i < dtab->n_buckets; i++) {
> >> 	head = dev_map_index_hash(dtab, i);
> >> 	hlist_nulls_for_each_entry_safe(dev, next, head, index_hlist) {
> >> 		bq_enqueue(dev, xdpf, dev_rx, obj->xdp_prog);
> >> 	}
> >> }
> >> 
> >> or? Yeah, I guess that would work!
> >
> > Nice. Thanks for sticking with this Hangbin its taking us a bit, but
> > I think above works on my side at least.
> >
> >> 
> >> It would mean that dev_map_enqueue_multi() would need more in-depth
> >> knowledge into the map type, so would likely need to be two different
> >> functions for the two different map types, living in devmap.c - but
> >> that's probably acceptable.
> >
> > Yeah, I think thats fine.
> >
> >> 
> >> And while we're doing that, the array-map version can also loop over all
> >> indexes up to max_entries, instead of stopping at the first index that
> >> doesn't have an entry like it does now (right now, it looks like if you
> >> populate entries 0 and 2 in an array-map only one copy of the packet
> >> will be sent, to index 0).
> >
> > Right, this is likely needed anyways. At least when I was doing prototypes
> > of using array maps I often ended up with holes in the map. Just imagine
> > adding a set of devs and then removing one, its not likely to be the
> > last one you insert.
> 
> Yeah, totally. Would have pointed it out if I'd noticed before, but I
> was too trusting in the abstraction of get_next_key() etc :)
 
Hangbin,

If possible please try to capture some of the design discussion in
the commit message on the next rev along with the tradeoffs we are making
so we don't lose these important details. Some of these points are fairly
subtle calling them out will surely save (for me at least) some thinking
when I pick this up when it lands in a released kernel.

Thanks!