netdev - Re: [PATCHv14 bpf-next 3/6] xdp: add a new helper for dev map multicast support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20210118084455.GE1421720@Leo-laptop-t470s>
Date:   Mon, 18 Jan 2021 16:44:55 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        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>
Subject: Re: [PATCHv14 bpf-next 3/6] xdp: add a new helper for dev map
 multicast support

Hi John,

Thanks for the reviewing.

On Sun, Jan 17, 2021 at 04:10:40PM -0800, John Fastabend wrote:
> > + * 		The forwarding *map* could be either BPF_MAP_TYPE_DEVMAP or
> > + * 		BPF_MAP_TYPE_DEVMAP_HASH. But the *ex_map* must be
> > + * 		BPF_MAP_TYPE_DEVMAP_HASH to get better performance.
> 
> Would be good to add a note ex_map _must_ be keyed by ifindex for the
> helper to work. Its the obvious way to key a hashmap, but not required
> iirc.

OK, I will.
> > +		if (!next_obj)
> > +			last_one = true;
> > +
> > +		if (last_one) {
> > +			bq_enqueue(obj->dev, xdpf, dev_rx, obj->xdp_prog);
> > +			return 0;
> > +		}
> 
> Just collapse above to
> 
>   if (!next_obj) {
>         bq_enqueue()
>         return
>   }
> 
> 'last_one' is a bit pointless here.

Yes, thanks.

> > @@ -3986,12 +3993,14 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> >  {
> >  	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> >  	struct bpf_map *map = READ_ONCE(ri->map);
> > +	struct bpf_map *ex_map = ri->ex_map;
> 
> READ_ONCE(ri->ex_map)?
> 
> >  	u32 index = ri->tgt_index;
> >  	void *fwd = ri->tgt_value;
> >  	int err;
> >  
> >  	ri->tgt_index = 0;
> >  	ri->tgt_value = NULL;
> > +	ri->ex_map = NULL;
> 
> WRITE_ONCE(ri->ex_map)?
> 
> >  	WRITE_ONCE(ri->map, NULL);
> 
> So we needed write_once, read_once pairs for ri->map do we also need them in
> the ex_map case?

Toke said this is no need for this read/write_once as there is already one.

https://lore.kernel.org/bpf/87r1wd2bqu.fsf@toke.dk/

Thanks
Hangbin