Re: [PATCH bpf] bpf: xdp: fix XDP mode when no mode flags specified

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3601 bytes --]

> 7f0a838254bd ("bpf, xdp: Maintain info on attached XDP BPF programs in net_device")
> inadvertently changed which XDP mode is assumed when no mode flags are
> specified explicitly. Previously, driver mode was preferred, if driver
> supported it. If not, generic SKB mode was chosen. That commit changed default
> to SKB mode always. This patch fixes the issue and restores the original
> logic.
> 
> Reported-by: Lorenzo Bianconi <lorenzo@kernel.org>
> Fixes: 7f0a838254bd ("bpf, xdp: Maintain info on attached XDP BPF programs in net_device")
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Hi Andrii,

Regarding this patch:

Tested-by: Lorenzo Bianconi <lorenzo@kernel.org>

I found another similar issue (not sure if it is related yet), the program removal is failing:

$ip link show dev eth0
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 xdp qdisc mq state UP mode DEFAULT group default qlen 1024
    link/ether f0:ad:4e:09:6b:57 brd ff:ff:ff:ff:ff:ff
    prog/xdp id 1 tag 3b185187f1855c4c jited 

$ip link set dev eth0 xdp off                                                                                                                                                                                            
Error: XDP program already attached.

Regards,
Lorenzo

> ---
>  net/core/dev.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b5d1129d8310..d42c9ea0c3c0 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8742,13 +8742,15 @@ struct bpf_xdp_link {
>  	int flags;
>  };
>  
> -static enum bpf_xdp_mode dev_xdp_mode(u32 flags)
> +static enum bpf_xdp_mode dev_xdp_mode(struct net_device *dev, u32 flags)
>  {
>  	if (flags & XDP_FLAGS_HW_MODE)
>  		return XDP_MODE_HW;
>  	if (flags & XDP_FLAGS_DRV_MODE)
>  		return XDP_MODE_DRV;
> -	return XDP_MODE_SKB;
> +	if (flags & XDP_FLAGS_SKB_MODE)
> +		return XDP_MODE_SKB;
> +	return dev->netdev_ops->ndo_bpf ? XDP_MODE_DRV : XDP_MODE_SKB;
>  }
>  
>  static bpf_op_t dev_xdp_bpf_op(struct net_device *dev, enum bpf_xdp_mode mode)
> @@ -8896,7 +8898,7 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
>  		return -EINVAL;
>  	}
>  
> -	mode = dev_xdp_mode(flags);
> +	mode = dev_xdp_mode(dev, flags);
>  	/* can't replace attached link */
>  	if (dev_xdp_link(dev, mode)) {
>  		NL_SET_ERR_MSG(extack, "Can't replace active BPF XDP link");
> @@ -8984,7 +8986,7 @@ static int dev_xdp_detach_link(struct net_device *dev,
>  
>  	ASSERT_RTNL();
>  
> -	mode = dev_xdp_mode(link->flags);
> +	mode = dev_xdp_mode(dev, link->flags);
>  	if (dev_xdp_link(dev, mode) != link)
>  		return -EINVAL;
>  
> @@ -9080,7 +9082,7 @@ static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
>  		goto out_unlock;
>  	}
>  
> -	mode = dev_xdp_mode(xdp_link->flags);
> +	mode = dev_xdp_mode(xdp_link->dev, xdp_link->flags);
>  	bpf_op = dev_xdp_bpf_op(xdp_link->dev, mode);
>  	err = dev_xdp_install(xdp_link->dev, mode, bpf_op, NULL,
>  			      xdp_link->flags, new_prog);
> @@ -9164,7 +9166,7 @@ int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>  int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>  		      int fd, int expected_fd, u32 flags)
>  {
> -	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> +	enum bpf_xdp_mode mode = dev_xdp_mode(dev, flags);
>  	struct bpf_prog *new_prog = NULL, *old_prog = NULL;
>  	int err;
>  
> -- 
> 2.24.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]