netdev - Re: [PATCHv7 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]

Date:   Thu, 15 Apr 2021 11:29:20 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Hangbin Liu <liuhangbin@...il.com>, Martin KaFai Lau <kafai@...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>,
        John Fastabend <john.fastabend@...il.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Björn Töpel 
        <bjorn.topel@...il.com>
Subject: Re: [PATCHv7 bpf-next 2/4] xdp: extend xdp_redirect_map with
 broadcast support

Hangbin Liu <liuhangbin@...il.com> writes:

> On Wed, Apr 14, 2021 at 05:23:50PM -0700, Martin KaFai Lau wrote:
>> On Wed, Apr 14, 2021 at 08:26:08PM +0800, Hangbin Liu wrote:
>> [ ... ]
>> 
>> > +static __always_inline int __bpf_xdp_redirect_map(struct bpf_map *map, u32 ifindex,
>> > +						  u64 flags, u64 flag_mask,
>> >  						  void *lookup_elem(struct bpf_map *map, u32 key))
>> >  {
>> >  	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>> >  
>> >  	/* Lower bits of the flags are used as return code on lookup failure */
>> > -	if (unlikely(flags > XDP_TX))
>> > +	if (unlikely(flags & ~(BPF_F_ACTION_MASK | flag_mask)))
>> >  		return XDP_ABORTED;
>> >  
>> >  	ri->tgt_value = lookup_elem(map, ifindex);
>> > -	if (unlikely(!ri->tgt_value)) {
>> > +	if (unlikely(!ri->tgt_value) && !(flags & BPF_F_BROADCAST)) {
>> >  		/* If the lookup fails we want to clear out the state in the
>> >  		 * redirect_info struct completely, so that if an eBPF program
>> >  		 * performs multiple lookups, the last one always takes
>> > @@ -1482,13 +1484,21 @@ static __always_inline int __bpf_xdp_redirect_map(struct bpf_map *map, u32 ifind
>> >  		 */
>> >  		ri->map_id = INT_MAX; /* Valid map id idr range: [1,INT_MAX[ */
>> >  		ri->map_type = BPF_MAP_TYPE_UNSPEC;
>> > -		return flags;
>> > +		return flags & BPF_F_ACTION_MASK;
>> >  	}
>> >  
>> >  	ri->tgt_index = ifindex;
>> >  	ri->map_id = map->id;
>> >  	ri->map_type = map->map_type;
>> >  
>> > +	if (flags & BPF_F_BROADCAST) {
>> > +		WRITE_ONCE(ri->map, map);
>> Why only WRITE_ONCE on ri->map?  Is it needed?
>
> I think this is make sure the map pointer assigned to ri->map safely.
> which starts from commit f6069b9aa993 ("bpf: fix redirect to map under tail
> calls")

The reason WRITE_ONCE() is only on the map field is because that's the
one that could be changed by a remote CPU (in bpf_clear_redirect_map())
- everything else is only accessed on the local CPU.

As for whether it's strictly needed from a memory model PoV, I'm not
actually sure (and should we be using smp_{store_release,load_acquire}()
instead?); I view it mostly as an annotation to make it clear that the
map field is 'special' in this respect...

-Toke