netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To: Andrii Nakryiko <andriin@fb.com>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org, ast@fb.com,
	daniel@iogearbox.net, dsahern@gmail.com,
	andrii.nakryiko@gmail.com, kernel-team@fb.com
Subject: Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic
Date: Wed, 22 Jul 2020 21:13:20 +0200	[thread overview]
Message-ID: <20200722191320.GC8874@ranger.igk.intel.com> (raw)
In-Reply-To: <20200722064603.3350758-4-andriin@fb.com>

On Tue, Jul 21, 2020 at 11:45:56PM -0700, Andrii Nakryiko wrote:
> Further refactor XDP attachment code. dev_change_xdp_fd() is split into two
> parts: getting bpf_progs from FDs and attachment logic, working with
> bpf_progs. This makes attachment  logic a bit more straightforward and
> prepares code for bpf_xdp_link inclusion, which will share the common logic.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  net/core/dev.c | 165 +++++++++++++++++++++++++++----------------------
>  1 file changed, 91 insertions(+), 74 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7e753e248cef..abf573b2dcf4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8815,111 +8815,128 @@ static void dev_xdp_uninstall(struct net_device *dev)
>  	}
>  }
>  
> -/**
> - *	dev_change_xdp_fd - set or clear a bpf program for a device rx path
> - *	@dev: device
> - *	@extack: netlink extended ack
> - *	@fd: new program fd or negative value to clear
> - *	@expected_fd: old program fd that userspace expects to replace or clear
> - *	@flags: xdp-related flags
> - *
> - *	Set or clear a bpf program for a device
> - */
> -int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> -		      int fd, int expected_fd, u32 flags)
> +static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack,
> +			  struct bpf_prog *new_prog, struct bpf_prog *old_prog,
> +			  u32 flags)
>  {
> -	const struct net_device_ops *ops = dev->netdev_ops;
> -	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> -	bool offload = mode == XDP_MODE_HW;
> -	u32 prog_id, expected_id = 0;
> -	struct bpf_prog *prog;
> +	struct bpf_prog *cur_prog;
> +	enum bpf_xdp_mode mode;
>  	bpf_op_t bpf_op;
>  	int err;
>  
>  	ASSERT_RTNL();

couldn't we rely on caller's rtnl assertion? dev_change_xdp_fd() already
has one.

>  
> -	bpf_op = dev_xdp_bpf_op(dev, mode);
> -	if (!bpf_op) {
> -		NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
> -		return -EOPNOTSUPP;
> +	/* just one XDP mode bit should be set, zero defaults to SKB mode */
> +	if (hweight32(flags & XDP_FLAGS_MODES) > 1) {
> +		NL_SET_ERR_MSG(extack, "Only one XDP mode flag can be set");
> +		return -EINVAL;
> +	}
> +	/* old_prog != NULL implies XDP_FLAGS_REPLACE is set */
> +	if (old_prog && !(flags & XDP_FLAGS_REPLACE)) {
> +		NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not specified");
> +		return -EINVAL;
>  	}
>  
> -	prog_id = dev_xdp_prog_id(dev, mode);
> -	if (flags & XDP_FLAGS_REPLACE) {
> -		if (expected_fd >= 0) {
> -			prog = bpf_prog_get_type_dev(expected_fd,
> -						     BPF_PROG_TYPE_XDP,
> -						     bpf_op == ops->ndo_bpf);
> -			if (IS_ERR(prog))
> -				return PTR_ERR(prog);
> -			expected_id = prog->aux->id;
> -			bpf_prog_put(prog);
> -		}
> -
> -		if (prog_id != expected_id) {
> -			NL_SET_ERR_MSG(extack, "Active program does not match expected");
> -			return -EEXIST;
> -		}
> +	mode = dev_xdp_mode(flags);
> +	cur_prog = dev_xdp_prog(dev, mode);
> +	if ((flags & XDP_FLAGS_REPLACE) && cur_prog != old_prog) {
> +		NL_SET_ERR_MSG(extack, "Active program does not match expected");
> +		return -EEXIST;
>  	}
> -	if (fd >= 0) {
> +	if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && cur_prog) {
> +		NL_SET_ERR_MSG(extack, "XDP program already attached");
> +		return -EBUSY;
> +	}
> +
> +	if (new_prog) {
> +		bool offload = mode == XDP_MODE_HW;
>  		enum bpf_xdp_mode other_mode = mode == XDP_MODE_SKB
>  					       ? XDP_MODE_DRV : XDP_MODE_SKB;
>  
> -		if (!offload && dev_xdp_prog_id(dev, other_mode)) {
> +		if (!offload && dev_xdp_prog(dev, other_mode)) {
>  			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) && prog_id) {
> -			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);
> -		if (IS_ERR(prog))
> -			return PTR_ERR(prog);
> -
> -		if (!offload && bpf_prog_is_dev_bound(prog->aux)) {
> +		if (!offload && bpf_prog_is_dev_bound(new_prog->aux)) {
>  			NL_SET_ERR_MSG(extack, "Using device-bound program without HW_MODE flag is not supported");
> -			bpf_prog_put(prog);
>  			return -EINVAL;
>  		}
> -
> -		if (prog->expected_attach_type == BPF_XDP_DEVMAP) {
> +		if (new_prog->expected_attach_type == BPF_XDP_DEVMAP) {
>  			NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP programs can not be attached to a device");
> -			bpf_prog_put(prog);
>  			return -EINVAL;
>  		}
> -
> -		if (prog->expected_attach_type == BPF_XDP_CPUMAP) {
> -			NL_SET_ERR_MSG(extack,
> -				       "BPF_XDP_CPUMAP programs can not be attached to a device");
> -			bpf_prog_put(prog);
> +		if (new_prog->expected_attach_type == BPF_XDP_CPUMAP) {
> +			NL_SET_ERR_MSG(extack, "BPF_XDP_CPUMAP programs can not be attached to a device");

bpf_prog_put() missing?

>  			return -EINVAL;
>  		}
> +	}
>  
> -		/* prog->aux->id may be 0 for orphaned device-bound progs */
> -		if (prog->aux->id && prog->aux->id == prog_id) {
> -			bpf_prog_put(prog);
> -			return 0;
> +	/* don't call drivers if the effective program didn't change */
> +	if (new_prog != cur_prog) {
> +		bpf_op = dev_xdp_bpf_op(dev, mode);
> +		if (!bpf_op) {
> +			NL_SET_ERR_MSG(extack, "Underlying driver does not support XDP in native mode");
> +			return -EOPNOTSUPP;
>  		}
> -	} else {
> -		if (!prog_id)
> -			return 0;
> -		prog = NULL;
> -	}
>  
> -	err = dev_xdp_install(dev, mode, bpf_op, extack, flags, prog);
> -	if (err < 0 && prog) {
> -		bpf_prog_put(prog);
> -		return err;
> +		err = dev_xdp_install(dev, mode, bpf_op, extack, flags, new_prog);
> +		if (err)
> +			return err;
>  	}
> -	dev_xdp_set_prog(dev, mode, prog);
> +
> +	dev_xdp_set_prog(dev, mode, new_prog);
> +	if (cur_prog)
> +		bpf_prog_put(cur_prog);
>  
>  	return 0;
>  }
>  
> +/**
> + *	dev_change_xdp_fd - set or clear a bpf program for a device rx path
> + *	@dev: device
> + *	@extack: netlink extended ack
> + *	@fd: new program fd or negative value to clear
> + *	@expected_fd: old program fd that userspace expects to replace or clear
> + *	@flags: xdp-related flags
> + *
> + *	Set or clear a bpf program for a device
> + */
> +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);
> +	struct bpf_prog *new_prog = NULL, *old_prog = NULL;
> +	int err;
> +
> +	ASSERT_RTNL();
> +
> +	if (fd >= 0) {
> +		new_prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP,
> +						 mode != XDP_MODE_SKB);
> +		if (IS_ERR(new_prog))
> +			return PTR_ERR(new_prog);
> +	}
> +
> +	if (expected_fd >= 0) {
> +		old_prog = bpf_prog_get_type_dev(expected_fd, BPF_PROG_TYPE_XDP,
> +						 mode != XDP_MODE_SKB);
> +		if (IS_ERR(old_prog)) {
> +			err = PTR_ERR(old_prog);
> +			old_prog = NULL;
> +			goto err_out;
> +		}
> +	}
> +
> +	err = dev_xdp_attach(dev, extack, new_prog, old_prog, flags);
> +
> +err_out:
> +	if (err && new_prog)
> +		bpf_prog_put(new_prog);
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +	return err;
> +}
> +
>  /**
>   *	dev_new_index	-	allocate an ifindex
>   *	@net: the applicable net namespace
> -- 
> 2.24.1
> 

  reply	other threads:[~2020-07-22 19:18 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-22  6:45 [PATCH v4 bpf-next 0/9] BPF XDP link Andrii Nakryiko
2020-07-22  6:45 ` [PATCH v4 bpf-next 1/9] bpf: make bpf_link API available indepently of CONFIG_BPF_SYSCALL Andrii Nakryiko
2020-07-22  6:45 ` [PATCH v4 bpf-next 2/9] bpf, xdp: maintain info on attached XDP BPF programs in net_device Andrii Nakryiko
2020-07-22 15:26   ` Maciej Fijalkowski
2020-07-22 19:35     ` Andrii Nakryiko
2020-07-22 19:56       ` Maciej Fijalkowski
2020-07-22  6:45 ` [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic Andrii Nakryiko
2020-07-22 19:13   ` Maciej Fijalkowski [this message]
2020-07-22 19:29     ` Andrii Nakryiko
2020-07-27 12:07   ` Shay Agroskin
2020-07-27 18:51     ` Andrii Nakryiko
2020-08-11 18:14   ` sdf
2020-08-12  2:19     ` Andrii Nakryiko
2020-07-22  6:45 ` [PATCH v4 bpf-next 4/9] bpf, xdp: add bpf_link-based XDP attachment API Andrii Nakryiko
2020-07-22  6:45 ` [PATCH v4 bpf-next 5/9] bpf, xdp: implement LINK_UPDATE for BPF XDP link Andrii Nakryiko
2020-07-22  6:45 ` [PATCH v4 bpf-next 6/9] bpf: implement BPF XDP link-specific introspection APIs Andrii Nakryiko
2020-07-22  6:46 ` [PATCH v4 bpf-next 7/9] libbpf: add support for BPF XDP link Andrii Nakryiko
2020-07-22  6:46 ` [PATCH v4 bpf-next 8/9] selftests/bpf: add BPF XDP link selftests Andrii Nakryiko
2020-07-22  6:46 ` [PATCH v4 bpf-next 9/9] bpf, xdp: remove XDP_QUERY_PROG and XDP_QUERY_PROG_HW XDP commands Andrii Nakryiko
2020-07-26  3:46 ` [PATCH v4 bpf-next 0/9] BPF XDP link Alexei Starovoitov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200722191320.GC8874@ranger.igk.intel.com \
    --to=maciej.fijalkowski@intel.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=dsahern@gmail.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).