Re: [PATCH net-next v4 1/6] net: xdp: refactor XDP attach

[Daniel Borkmann <daniel@iogearbox.net>]

On 04/08/2019 07:05 PM, Toke Høiland-Jørgensen wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
> 
> Generic XDP and driver XDP cannot be enabled at the same time. However,
> they don't share any state; let's fix that. Here, dev->xdp_prog, is
> used for both driver and generic mode. This removes the need for the
> XDP_QUERY_PROG command to ndo_bpf.
> 
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  include/linux/netdevice.h |    8 +-
>  net/core/dev.c            |  148 ++++++++++++++++++++++++++-------------------
>  net/core/rtnetlink.c      |   14 +---
>  3 files changed, 93 insertions(+), 77 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index eb9f05e0863d..68d4bbc44c63 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1957,7 +1957,7 @@ struct net_device {
>  	struct cpu_rmap		*rx_cpu_rmap;
>  #endif
>  	struct hlist_node	index_hlist;
> -
> +	unsigned int		xdp_target;
>  /*
>   * Cache lines mostly used on transmit path
>   */
> @@ -2063,7 +2063,8 @@ struct net_device {
>  
>  static inline bool netif_elide_gro(const struct net_device *dev)
>  {
> -	if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
> +	if (!(dev->features & NETIF_F_GRO) ||
> +	    dev->xdp_target == XDP_FLAGS_SKB_MODE)
>  		return true;
>  	return false;
>  }
> @@ -3702,8 +3703,7 @@ struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>  typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf *bpf);
>  int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>  		      int fd, u32 flags);
> -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
> -		    enum bpf_netdev_command cmd);
> +u32 __dev_xdp_query(struct net_device *dev, unsigned int target);
>  int xdp_umem_query(struct net_device *dev, u16 queue_id);
>  
>  int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d5b1315218d3..feafc3580350 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5121,35 +5121,25 @@ static void __netif_receive_skb_list(struct list_head *head)
>  
>  static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
>  {
> -	struct bpf_prog *old = rtnl_dereference(dev->xdp_prog);
>  	struct bpf_prog *new = xdp->prog;
> -	int ret = 0;
> -
> -	switch (xdp->command) {
> -	case XDP_SETUP_PROG:
> -		rcu_assign_pointer(dev->xdp_prog, new);
> -		if (old)
> -			bpf_prog_put(old);
>  
> -		if (old && !new) {
> +	if (xdp->command == XDP_SETUP_PROG) {
> +		if (static_key_enabled(&generic_xdp_needed_key) && !new) {
>  			static_branch_dec(&generic_xdp_needed_key);
> -		} else if (new && !old) {
> +		} else if (new &&
> +			   !static_key_enabled(&generic_xdp_needed_key)) {
>  			static_branch_inc(&generic_xdp_needed_key);
>  			dev_disable_lro(dev);
>  			dev_disable_gro_hw(dev);
>  		}
> -		break;
>  
> -	case XDP_QUERY_PROG:
> -		xdp->prog_id = old ? old->aux->id : 0;
> -		break;
> +		if (new)
> +			bpf_prog_put(new);

Hmm, why you drop ref on the new program that you're installing here?

>  
> -	default:
> -		ret = -EINVAL;
> -		break;
> +		return 0;
>  	}
>  
> -	return ret;
> +	return -EINVAL;
>  }
>  
>  static int netif_receive_skb_internal(struct sk_buff *skb)
> @@ -7981,30 +7971,50 @@ int dev_change_proto_down_generic(struct net_device *dev, bool proto_down)
>  }
>  EXPORT_SYMBOL(dev_change_proto_down_generic);
>  
> -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t bpf_op,
> -		    enum bpf_netdev_command cmd)
> +u32 __dev_xdp_query(struct net_device *dev, unsigned int target)
>  {
> -	struct netdev_bpf xdp;
> +	if (target == XDP_FLAGS_DRV_MODE || target == XDP_FLAGS_SKB_MODE) {
> +		struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
>  
> -	if (!bpf_op)
> -		return 0;
> +		return prog && (dev->xdp_target == target) ? prog->aux->id : 0;
> +	}
>  
> -	memset(&xdp, 0, sizeof(xdp));
> -	xdp.command = cmd;
> +	if (target == XDP_FLAGS_HW_MODE) {
> +		struct netdev_bpf xdp = { .command = XDP_QUERY_PROG_HW };
>  
> -	/* Query must always succeed. */
> -	WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> +		if (!dev->netdev_ops->ndo_bpf)
> +			return 0;
> +		dev->netdev_ops->ndo_bpf(dev, &xdp);
> +		return xdp.prog_id;
> +	}
>  
> -	return xdp.prog_id;
> +	WARN_ON(1);
> +	return 0;
>  }
>  
> -static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> +static int dev_xdp_install(struct net_device *dev, unsigned int target,
>  			   struct netlink_ext_ack *extack, u32 flags,
>  			   struct bpf_prog *prog)
>  {
> -	struct netdev_bpf xdp;
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct bpf_prog *old = NULL;
> +	struct netdev_bpf xdp = {};
> +	int ret;
> +
> +	if (target != XDP_FLAGS_HW_MODE) {
> +		if (prog) {
> +			prog = bpf_prog_inc(prog);
> +			if (IS_ERR(prog))
> +				return PTR_ERR(prog);
> +		}
> +
> +		old = rtnl_dereference(dev->xdp_prog);
> +		if (!old && !prog)
> +			return 0;
> +		rcu_assign_pointer(dev->xdp_prog, prog);
> +		dev->xdp_target = prog ? target : 0;
> +	}
>  
> -	memset(&xdp, 0, sizeof(xdp));
>  	if (flags & XDP_FLAGS_HW_MODE)
>  		xdp.command = XDP_SETUP_PROG_HW;
>  	else
> @@ -8013,7 +8023,24 @@ static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
>  	xdp.flags = flags;
>  	xdp.prog = prog;
>  
> -	return bpf_op(dev, &xdp);
> +	if (target == XDP_FLAGS_SKB_MODE)
> +		ret = generic_xdp_install(dev, &xdp);
> +	else
> +		ret = ops->ndo_bpf(dev, &xdp);
> +
> +	if (target != XDP_FLAGS_HW_MODE) {
> +		if (ret) {
> +			if (prog)
> +				bpf_prog_put(prog);
> +			rcu_assign_pointer(dev->xdp_prog, old);
> +			dev->xdp_target = old ? target : 0;

Hmm, please correct me if I'm wrong, but from reading this patch (with following
generic XDP as a simple example), it means: above we inc ref on prog, fetch old
prog, assign new prog to dev->xdp_prog, call generic_xdp_install(). If one would
fail there, we drop the ref of the prog that is currently attached to dev->xdp_prog
(effectively a use-after-free?), and assign the old prog back. So basically we
would shortly flake wrt prog assignment. I don't think this is a good design, the
update should only really happen once everything else succeeded and there is nothing
that can fail anymore. The existing code is doing exactly the latter, why diverge
from this practice?! (Also, why special casing XDP_FLAGS_HW_MODE? Feels a bit like
a hack tbh.)

> +		} else {
> +			if (old)
> +				bpf_prog_put(old);
> +		}
> +	}
> +
> +	return ret;
>  }
>  
>  static void dev_xdp_uninstall(struct net_device *dev)
> @@ -8021,27 +8048,28 @@ static void dev_xdp_uninstall(struct net_device *dev)
>  	struct netdev_bpf xdp;
>  	bpf_op_t ndo_bpf;
>  
> -	/* Remove generic XDP */
> -	WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
> +	/* Remove generic/native XDP */
> +	WARN_ON(dev_xdp_install(dev, dev->xdp_target, NULL, 0, NULL));
>  
>  	/* Remove from the driver */
>  	ndo_bpf = dev->netdev_ops->ndo_bpf;
>  	if (!ndo_bpf)
>  		return;
>  
> -	memset(&xdp, 0, sizeof(xdp));
> -	xdp.command = XDP_QUERY_PROG;
> -	WARN_ON(ndo_bpf(dev, &xdp));
> -	if (xdp.prog_id)
> -		WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> -					NULL));
> -
> -	/* Remove HW offload */
>  	memset(&xdp, 0, sizeof(xdp));
>  	xdp.command = XDP_QUERY_PROG_HW;
>  	if (!ndo_bpf(dev, &xdp) && xdp.prog_id)
> -		WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> -					NULL));
> +		WARN_ON(dev_xdp_install(dev, XDP_FLAGS_HW_MODE, NULL,
> +					xdp.prog_flags, NULL));
> +}
> +
> +static bool dev_validate_active_xdp(struct net_device *dev, unsigned int target)
> +{
> +	if (target == XDP_FLAGS_DRV_MODE)
> +		return dev->xdp_target != XDP_FLAGS_SKB_MODE;
> +	if (target == XDP_FLAGS_SKB_MODE)
> +		return dev->xdp_target != XDP_FLAGS_DRV_MODE;
> +	return true;
>  }
>  
>  /**
> @@ -8057,40 +8085,36 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>  		      int fd, u32 flags)
>  {
>  	const struct net_device_ops *ops = dev->netdev_ops;
> -	enum bpf_netdev_command query;
> +	bool offload, drv = !!ops->ndo_bpf;
>  	struct bpf_prog *prog = NULL;
> -	bpf_op_t bpf_op, bpf_chk;
> -	bool offload;
> +	unsigned int target;
>  	int err;
>  
>  	ASSERT_RTNL();
>  
>  	offload = flags & XDP_FLAGS_HW_MODE;
> -	query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> +	target = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
>  
> -	bpf_op = bpf_chk = ops->ndo_bpf;
> -	if (!bpf_op && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE))) {
> +	if (!drv && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE))) {
>  		NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
>  		return -EOPNOTSUPP;
>  	}
> -	if (!bpf_op || (flags & XDP_FLAGS_SKB_MODE))
> -		bpf_op = generic_xdp_install;
> -	if (bpf_op == bpf_chk)
> -		bpf_chk = generic_xdp_install;
> +	if (!drv || (flags & XDP_FLAGS_SKB_MODE))
> +		target = XDP_FLAGS_SKB_MODE;
> +
> +	if (!dev_validate_active_xdp(dev, target)) {
> +		NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time");
> +		return -EEXIST;
> +	}
>  
>  	if (fd >= 0) {
> -		if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
> -			NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time");
> -			return -EEXIST;
> -		}
>  		if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) &&
> -		    __dev_xdp_query(dev, bpf_op, query)) {
> +		    __dev_xdp_query(dev, target)) {
>  			NL_SET_ERR_MSG(extack, "XDP program already attached");
>  			return -EBUSY;
>  		}
>  
> -		prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP,
> -					     bpf_op == ops->ndo_bpf);
> +		prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, drv);
>  		if (IS_ERR(prog))
>  			return PTR_ERR(prog);
>  
> @@ -8101,7 +8125,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>  		}
>  	}
>  
> -	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> +	err = dev_xdp_install(dev, target, extack, flags, prog);
>  	if (err < 0 && prog)
>  		bpf_prog_put(prog);
>  
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f9b964fd4e4d..d9c7fe34c3a1 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1362,25 +1362,17 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
>  
>  static u32 rtnl_xdp_prog_skb(struct net_device *dev)
>  {
> -	const struct bpf_prog *generic_xdp_prog;
> -
> -	ASSERT_RTNL();
> -
> -	generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> -	if (!generic_xdp_prog)
> -		return 0;
> -	return generic_xdp_prog->aux->id;
> +	return __dev_xdp_query(dev, XDP_FLAGS_SKB_MODE);
>  }
>  
>  static u32 rtnl_xdp_prog_drv(struct net_device *dev)
>  {
> -	return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf, XDP_QUERY_PROG);
> +	return __dev_xdp_query(dev, XDP_FLAGS_DRV_MODE);
>  }
>  
>  static u32 rtnl_xdp_prog_hw(struct net_device *dev)
>  {
> -	return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> -			       XDP_QUERY_PROG_HW);
> +	return __dev_xdp_query(dev, XDP_FLAGS_HW_MODE);
>  }
>  
>  static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
>