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

Hangbin Liu [off-list ref] writes:

On Wed, Mar 31, 2021 at 03:41:17PM +0200, Toke Høiland-Jørgensen wrote:

quoted

quoted

@@ -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:

[...]

quoted

+	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 :)

quoted

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

quoted

- 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