netdev - Re: [PATCHv3 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] [day] [month] [year] [list]
Date:   Thu, 01 Apr 2021 13:10:29 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     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>,
        John Fastabend <john.fastabend@...il.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Björn Töpel 
        <bjorn.topel@...il.com>
Subject: Re: [PATCHv3 bpf-next 2/4] xdp: extend xdp_redirect_map with
 broadcast support

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

> On Wed, Mar 31, 2021 at 03:41:17PM +0200, Toke Høiland-Jørgensen wrote:
>> > @@ -1491,13 +1492,20 @@ 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 ((map->map_type == BPF_MAP_TYPE_DEVMAP ||
>> > +	     map->map_type == BPF_MAP_TYPE_DEVMAP_HASH) &&
>> > +	    (flags & BPF_F_BROADCAST)) {
>> > +		ri->flags = flags;
>> 
>> This, combined with this:
>> 
>> [...]
>> 
>> > +	if (ri->flags & BPF_F_BROADCAST) {
>> > +		map = READ_ONCE(ri->map);
>> > +		WRITE_ONCE(ri->map, NULL);
>> > +	}
>> > +
>> >  	switch (map_type) {
>> >  	case BPF_MAP_TYPE_DEVMAP:
>> >  		fallthrough;
>> >  	case BPF_MAP_TYPE_DEVMAP_HASH:
>> > -		err = dev_map_enqueue(fwd, xdp, dev);
>> > +		if (ri->flags & BPF_F_BROADCAST)
>> > +			err = dev_map_enqueue_multi(xdp, dev, map,
>> > +						    ri->flags & BPF_F_EXCLUDE_INGRESS);
>> > +		else
>> > +			err = dev_map_enqueue(fwd, xdp, dev);
>> 
>> Means that (since the flags value is never cleared again) once someone
>> has done a broadcast redirect, that's all they'll ever get until the
>> next reboot ;)
>
> How about just get the ri->flags first and clean it directly. e.g.
>
> flags = ri->flags;
> ri->flags = 0;

That would fix the "until next reboot" issue, but would still render
bpf_clear_redirect_map() useless. So you still need to check ri->map and
if you do that you don't actually need to clear the flag field as long
as it is set correctly whenever the map pointer is.

> With this we don't need to add an extra field ri->exclude_ingress as you
> mentioned later.

The ri->exclude_ingress would be *instead* of the flags field. You could
of course also just keep the flags field, but turning it into a bool
makes it obvious that only one of the bits is actually used (and thus
easier to see that it's correct to not clear it).

> People may also need the flags field in future.

In which case they can add it back at that time :)

>> Also, the bpf_clear_redirect_map() call has no effect since the call to
>> dev_map_enqueue_multi() only checks the flags and not the value of the
>> map pointer before deciding which enqueue function to call.
>> 
>> To fix both of these, how about changing the logic so that:
>> 
>> - __bpf_xdp_redirect_map() sets the map pointer if the broadcast flag is
>>   set (and clears it if the flag isn't set!)
>
> OK
>> 
>> - xdp_do_redirect() does the READ_ONCE/WRITE_ONCE on ri->map
>>   unconditionally and then dispatches to dev_map_enqueue_multi() if the
>>   read resulted in a non-NULL pointer
>> 
>> Also, it should be invalid to set the broadcast flag with a map type
>> other than a devmap; __bpf_xdp_redirect_map() should check that.
>
> The current code only do if (unlikely(flags > XDP_TX)) and didn't check the
> map type. I also only set the map when there has devmap + broadcast flag.
> Do you mean we need a more strict check? like
>
> if (unlikely((flags & ~(BPF_F_ACTION_MASK | BPF_F_REDIR_MASK)) ||
> 	      (map->map_type != BPF_MAP_TYPE_DEVMAP &&
> 	       map->map_type != BPF_MAP_TYPE_DEVMAP_HASH &&
> 	       flags & BPF_F_REDIR_MASK)))

Yeah, that's what I meant, but when you type it out that does seem like
a bit too many checks.

However, I think we can do something different: Since Björn has
helpfully split out the helper functions for the different map types, we
can add another argument to __bpf_xdp_redirect_map() which is the mask
of valid flag values. With this, dev_{hash_,}map_redirect() can include
BPF_F_REDIR_MASK in the valid flags, and {xsk,cpu}_map_redirect() can
leave them out. That makes the code do the right thing without actually
adding any more checks in the fast path :)

(You'd still need to AND the return value with BPF_F_ACTION_MASK when
returning, of course).

-Toke