Re: [PATCHv3 bpf-next 2/4] xdp: extend xdp_redirect_map with broadcast support

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Hangbin Liu <liuhangbin@gmail.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