Date:   Thu, 1 Aug 2019 16:36:38 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     "Daniel T. Lee" <danieltimlee@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org
Subject: Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP
 on interface

On Thu,  1 Aug 2019 17:11:32 +0900, Daniel T. Lee wrote:
> By this commit, using `bpftool net attach`, user can attach XDP prog on
> interface. New type of enum 'net_attach_type' has been made, as stated at
> cover-letter, the meaning of 'attach' is, prog will be attached on interface.
> 
> BPF prog will be attached through libbpf 'bpf_set_link_xdp_fd'.
> 
> Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>
> ---
> Changes in v2:
>   - command 'load' changed to 'attach' for the consistency
>   - 'NET_ATTACH_TYPE_XDP_DRIVE' changed to 'NET_ATTACH_TYPE_XDP_DRIVER'
> 
>  tools/bpf/bpftool/net.c | 107 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index 67e99c56bc88..f3b57660b303 100644
> --- a/tools/bpf/bpftool/net.c
> +++ b/tools/bpf/bpftool/net.c
> @@ -55,6 +55,35 @@ struct bpf_attach_info {
>  	__u32 flow_dissector_id;
>  };
>  
> +enum net_attach_type {
> +	NET_ATTACH_TYPE_XDP,
> +	NET_ATTACH_TYPE_XDP_GENERIC,
> +	NET_ATTACH_TYPE_XDP_DRIVER,
> +	NET_ATTACH_TYPE_XDP_OFFLOAD,
> +	__MAX_NET_ATTACH_TYPE
> +};
> +
> +static const char * const attach_type_strings[] = {
> +	[NET_ATTACH_TYPE_XDP] = "xdp",
> +	[NET_ATTACH_TYPE_XDP_GENERIC] = "xdpgeneric",
> +	[NET_ATTACH_TYPE_XDP_DRIVER] = "xdpdrv",
> +	[NET_ATTACH_TYPE_XDP_OFFLOAD] = "xdpoffload",
> +	[__MAX_NET_ATTACH_TYPE] = NULL,

Not sure if the terminator is necessary,
ARRAY_SIZE(attach_type_strings) should suffice?

> +};
> +
> +static enum net_attach_type parse_attach_type(const char *str)
> +{
> +	enum net_attach_type type;
> +
> +	for (type = 0; type < __MAX_NET_ATTACH_TYPE; type++) {
> +		if (attach_type_strings[type] &&
> +		   is_prefix(str, attach_type_strings[type]))
> +			return type;
> +	}
> +
> +	return __MAX_NET_ATTACH_TYPE;
> +}
> +
>  static int dump_link_nlmsg(void *cookie, void *msg, struct nlattr **tb)
>  {
>  	struct bpf_netdev_t *netinfo = cookie;
> @@ -223,6 +252,77 @@ static int query_flow_dissector(struct bpf_attach_info *attach_info)
>  	return 0;
>  }
>  
> +static int parse_attach_args(int argc, char **argv, int *progfd,
> +			     enum net_attach_type *attach_type, int *ifindex)
> +{
> +	if (!REQ_ARGS(3))
> +		return -EINVAL;
> +
> +	*progfd = prog_parse_fd(&argc, &argv);
> +	if (*progfd < 0)
> +		return *progfd;
> +
> +	*attach_type = parse_attach_type(*argv);
> +	if (*attach_type == __MAX_NET_ATTACH_TYPE) {
> +		p_err("invalid net attach/detach type");
> +		return -EINVAL;

You should close the progfd on error paths.

> +	}

Hm. I'm not too sure about the ordering of arguments, type should
probably be right after attach.

If we ever add tc attach support or some other hook, that's more
fundamental part of the command than the program. So I think:

bpftool net attach xdp id xyz dev ethN

> +	NEXT_ARG();
> +	if (!REQ_ARGS(1))
> +		return -EINVAL;

Error message needed here.

> +	*ifindex = if_nametoindex(*argv);
> +	if (!*ifindex) {
> +		p_err("Invalid ifname");

"ifname" is not mentioned in help, it'd be best to keep this error
message consistent with bpftool prog load.

> +		return -EINVAL;
> +	}

Please require the dev keyword before the interface name.
That'll make it feel closer to prog load syntax.

> +	return 0;
> +}
> +
> +static int do_attach_detach_xdp(int *progfd, enum net_attach_type *attach_type,
> +				int *ifindex)
> +{
> +	__u32 flags;
> +	int err;
> +
> +	flags = XDP_FLAGS_UPDATE_IF_NOEXIST;

Please add this as an option so that user can decide whether overwrite
is allowed or not.

> +	if (*attach_type == NET_ATTACH_TYPE_XDP_GENERIC)
> +		flags |= XDP_FLAGS_SKB_MODE;
> +	if (*attach_type == NET_ATTACH_TYPE_XDP_DRIVER)
> +		flags |= XDP_FLAGS_DRV_MODE;
> +	if (*attach_type == NET_ATTACH_TYPE_XDP_OFFLOAD)
> +		flags |= XDP_FLAGS_HW_MODE;
> +
> +	err = bpf_set_link_xdp_fd(*ifindex, *progfd, flags);
> +
> +	return err;

no need for the err variable here.

> +}
> +
> +static int do_attach(int argc, char **argv)
> +{
> +	enum net_attach_type attach_type;
> +	int err, progfd, ifindex;
> +
> +	err = parse_attach_args(argc, argv, &progfd, &attach_type, &ifindex);
> +	if (err)
> +		return err;

Probably not the best idea to move this out into a helper.

> +	if (is_prefix("xdp", attach_type_strings[attach_type]))
> +		err = do_attach_detach_xdp(&progfd, &attach_type, &ifindex);

Hm. We either need an error to be reported if it's not xdp or since we
only accept XDP now perhaps the if() is superfluous?

> +	if (err < 0) {
> +		p_err("link set %s failed", attach_type_strings[attach_type]);

"link set"?  So you are familiar with iproute2 syntax! :)

> +		return -1;
> +	}
> +
> +	if (json_output)
> +		jsonw_null(json_wtr);
> +
> +	return 0;
> +}
> +
>  static int do_show(int argc, char **argv)
>  {
>  	struct bpf_attach_info attach_info = {};
> @@ -305,13 +405,17 @@ static int do_help(int argc, char **argv)
>  
>  	fprintf(stderr,
>  		"Usage: %s %s { show | list } [dev <devname>]\n"
> +		"       %s %s attach PROG LOAD_TYPE <devname>\n"
>  		"       %s %s help\n"
> +		"\n"
> +		"       " HELP_SPEC_PROGRAM "\n"
> +		"       LOAD_TYPE := { xdp | xdpgeneric | xdpdrv | xdpoffload }\n"

ATTACH_TYPE now?

Perhaps a new line before the "Note"?

>  		"Note: Only xdp and tc attachments are supported now.\n"
>  		"      For progs attached to cgroups, use \"bpftool cgroup\"\n"
>  		"      to dump program attachments. For program types\n"
>  		"      sk_{filter,skb,msg,reuseport} and lwt/seg6, please\n"
>  		"      consult iproute2.\n",
> -		bin_name, argv[-2], bin_name, argv[-2]);
> +		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
>  
>  	return 0;
>  }
> @@ -319,6 +423,7 @@ static int do_help(int argc, char **argv)
>  static const struct cmd cmds[] = {
>  	{ "show",	do_show },
>  	{ "list",	do_show },
> +	{ "attach",	do_attach },
>  	{ "help",	do_help },
>  	{ 0 }
>  };

Powered by blists - more mailing lists