On Mon, Aug 19, 2024 at 6:53 PM Jakub Kicinski [off-list ref] wrote:

On Mon, 19 Aug 2024 00:44:27 +0900 Taehee Yoo wrote:

quoted

quoted

@@ -9537,6 +9540,10 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
                        NL_SET_ERR_MSG(extack, "Native and generic XDP can't be active at the same time");
                        return -EEXIST;
                }
+               if (dev_get_max_mp_channel(dev) != -1) {
+                       NL_SET_ERR_MSG(extack, "XDP can't be installed on a netdev using memory providers");
+                       return -EINVAL;
+               }

Should we consider virtual interfaces like bonding, bridge, etc?
Virtual interfaces as an upper interface of physical interfaces can
still install XDP prog.

# ip link add bond0 type bond
# ip link set eth0 master bond0
# ip link set bond0 xdp pin /sys/fs/bpf/x/y
and
# ip link set bond0 xdpgeneric pin /sys/fs/bpf/x/y

All virtual interfaces can install generic XDP prog.
The bonding interface can install native XDP prog.

Good point. We may need some common helpers to place the checks for XDP.
They are spread all over the place now.

Took a bit of a look here. Forgive me, I'm not that familiar with XDP
and virtual interfaces, so I'm a bit unsure what to do here.

For veth, it seems, the device behind the veth is stored in
veth_priv->peer, so it seems maybe a dev_get_max_mp_channel() check on
veth_priv->peer is the way to go to disable this for veth? I think we
need to do this check on creation of the veth and on the ndo_bpf of
veth.

For bonding, it seems we need to add mp channel check in bond_xdp_set,
and bond_enslave?

There are a few other drivers that define ndo_add_slave, seems a check
in br_add_slave is needed as well.

This seems like a potentially deep rabbit hole with a few checks to
add all of the place. Is this blocking the series? AFAICT if XDP fails
with mp-bound queues with a benign error, that seems fine to me; I
don't have a use case for memory providers + xdp yet. This should only
be blocking if someone can repro a very serious error (kernel crash)
or something with this combination.

I can try to add these checks locally and propose as a follow up
series. Let me know if I'm on the right track with figuring out how to
implement this, and, if you feel like it's blocking.

-- 
Thanks,
Mina