[v2,0/2] tools: bpftool: add net attach/detach command to attach XDP prog

[v2,0/2] tools: bpftool: add net attach/detach command to attach XDP prog

[Daniel T. Lee]

Currently, bpftool net only supports dumping progs attached on the
interface. To attach XDP prog on interface, user must use other tool
(eg. iproute2). By this patch, with `bpftool net attach/detach`, user
can attach/detach XDP prog on interface.

    $ ./bpftool prog
    ...
    208: xdp  name xdp_prog1  tag ad822e38b629553f  gpl
      loaded_at 2019-07-28T18:03:11+0900  uid 0
    ...
    $ ./bpftool net attach id 208 xdpdrv enp6s0np1
    $ ./bpftool net
    xdp:
    enp6s0np1(5) driver id 208
    ...
    $ ./bpftool net detach xdpdrv enp6s0np1
    $ ./bpftool net
    xdp:
    ...

While this patch only contains support for XDP, through `net
attach/detach`, bpftool can further support other prog attach types.

XDP attach/detach tested on Mellanox ConnectX-4 and Netronome Agilio.

---
Changes in v2:
  - command 'load/unload' changed to 'attach/detach' for the consistency

Daniel T. Lee (2):
  tools: bpftool: add net attach command to attach XDP on interface
  tools: bpftool: add net detach command to detach XDP on interface

 tools/bpf/bpftool/net.c | 160 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 159 insertions(+), 1 deletion(-)

-- 
2.20.1

[v2,1/2] tools: bpftool: add net attach command to attach XDP on interface

[Daniel T. Lee]

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@gmail.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,
+};
+
+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;
+	}
+
+	NEXT_ARG();
+	if (!REQ_ARGS(1))
+		return -EINVAL;
+
+	*ifindex = if_nametoindex(*argv);
+	if (!*ifindex) {
+		p_err("Invalid ifname");
+		return -EINVAL;
+	}
+
+	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;
+	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;
+}
+
+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;
+
+	if (is_prefix("xdp", attach_type_strings[attach_type]))
+		err = do_attach_detach_xdp(&progfd, &attach_type, &ifindex);
+
+	if (err < 0) {
+		p_err("link set %s failed", attach_type_strings[attach_type]);
+		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"
 		"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 }
 };
-- 
2.20.1

[v2,2/2] tools: bpftool: add net detach command to detach XDP on interface

[Daniel T. Lee]

By this commit, using `bpftool net detach`, the attached XDP prog can
be detached. Detaching the BPF prog will be done through libbpf
'bpf_set_link_xdp_fd' with the progfd set to -1.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
Changes in v2:
  - command 'unload' changed to 'detach' for the consistency

 tools/bpf/bpftool/net.c | 55 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index f3b57660b303..2ae9a613b05c 100644
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -281,6 +281,31 @@ static int parse_attach_args(int argc, char **argv, int *progfd,
 	return 0;
 }
 
+static int parse_detach_args(int argc, char **argv,
+			     enum net_attach_type *attach_type, int *ifindex)
+{
+	if (!REQ_ARGS(2))
+		return -EINVAL;
+
+	*attach_type = parse_attach_type(*argv);
+	if (*attach_type == __MAX_NET_ATTACH_TYPE) {
+		p_err("invalid net attach/detach type");
+		return -EINVAL;
+	}
+
+	NEXT_ARG();
+	if (!REQ_ARGS(1))
+		return -EINVAL;
+
+	*ifindex = if_nametoindex(*argv);
+	if (!*ifindex) {
+		p_err("Invalid ifname");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int do_attach_detach_xdp(int *progfd, enum net_attach_type *attach_type,
 				int *ifindex)
 {
@@ -323,6 +348,31 @@ static int do_attach(int argc, char **argv)
 	return 0;
 }
 
+static int do_detach(int argc, char **argv)
+{
+	enum net_attach_type attach_type;
+	int err, progfd, ifindex;
+
+	err = parse_detach_args(argc, argv, &attach_type, &ifindex);
+	if (err)
+		return err;
+
+	/* to detach xdp prog */
+	progfd = -1;
+	if (is_prefix("xdp", attach_type_strings[attach_type]))
+		err = do_attach_detach_xdp(&progfd, &attach_type, &ifindex);
+
+	if (err < 0) {
+		p_err("link set %s failed", attach_type_strings[attach_type]);
+		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 = {};
@@ -406,6 +456,7 @@ 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 detach LOAD_TYPE <devname>\n"
 		"       %s %s help\n"
 		"\n"
 		"       " HELP_SPEC_PROGRAM "\n"
@@ -415,7 +466,8 @@ static int do_help(int argc, char **argv)
 		"      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], bin_name, argv[-2],
+		bin_name, argv[-2]);
 
 	return 0;
 }
@@ -424,6 +476,7 @@ static const struct cmd cmds[] = {
 	{ "show",	do_show },
 	{ "list",	do_show },
 	{ "attach",	do_attach },
+	{ "detach",	do_detach },
 	{ "help",	do_help },
 	{ 0 }
 };
-- 
2.20.1

Re: [v2,0/2] tools: bpftool: add net attach/detach command to attach XDP prog

[Jakub Kicinski]

On Thu,  1 Aug 2019 17:11:31 +0900, Daniel T. Lee wrote:
> Currently, bpftool net only supports dumping progs attached on the
> interface. To attach XDP prog on interface, user must use other tool
> (eg. iproute2). By this patch, with `bpftool net attach/detach`, user
> can attach/detach XDP prog on interface.
> 
>     $ ./bpftool prog
>     ...
>     208: xdp  name xdp_prog1  tag ad822e38b629553f  gpl
>       loaded_at 2019-07-28T18:03:11+0900  uid 0
>     ...
>     $ ./bpftool net attach id 208 xdpdrv enp6s0np1
>     $ ./bpftool net
>     xdp:
>     enp6s0np1(5) driver id 208
>     ...
>     $ ./bpftool net detach xdpdrv enp6s0np1
>     $ ./bpftool net
>     xdp:
>     ...
> 
> While this patch only contains support for XDP, through `net
> attach/detach`, bpftool can further support other prog attach types.
> 
> XDP attach/detach tested on Mellanox ConnectX-4 and Netronome Agilio.

Please provide documentation for man pages, and bash completions.

Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP on interface

[Jakub Kicinski]

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@gmail.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 }
>  };

Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP on interface

[Daniel T. Lee]

On Fri, Aug 2, 2019 at 8:36 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> 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@gmail.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?

Yes, ARRAY_SIZE is fine though. But I was just trying to make below
'parse_attach_type' consistent with 'parse_attach_type' from the 'prog.c'.
At 'prog.c', It has same terminator at 'attach_type_strings'.

Should I change it or keep it?

> > +};
> > +
> > +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.

I will add 'close(*progfd);' at next version of patch.

>
> > +     }
>
> 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

I think it is more reasonable than current format.
I'll change the argument order as attach type comes in first.

> > +     NEXT_ARG();
> > +     if (!REQ_ARGS(1))
> > +             return -EINVAL;
>
> Error message needed here.
>

Actually it provides error message like:
Error: 'xdp' needs at least 1 arguments, 0 found

are you suggesting that any additional error message is necessary?

> > +     *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.

I will change it to a 'devname', since the word is mentioned in help.

> > +             return -EINVAL;
> > +     }
>
> Please require the dev keyword before the interface name.
> That'll make it feel closer to prog load syntax.

If adding the dev keyword before interface name, will it be too long to type in?
and also `bpftool prog` use extra keyword (such as dev) when it is
optional keyword.

       bpftool prog dump jited  PROG [{ file FILE | opcodes | linum }]
       bpftool prog pin   PROG FILE
       bpftool prog { load | loadall } OBJ  PATH \

as you can see here, FILE uses optional keyword 'file' when the
argument is optional.

       bpftool prog { load | loadall } OBJ  PATH \
                         [type TYPE] [dev NAME] \
                         [map { idx IDX | name NAME } MAP]\
                         [pinmaps MAP_DIR]

Yes, bpftool prog load has dev keyword with it,

but first, like previous, the argument is optional so i think it is
unnecessary to use optional keyword 'dev'.
and secondly, 'bpftool net attach' isn't really related to 'bpftool prog load'.

At previous version patch, I was using word 'load' instead of
'attach', since XDP program is not
considered as 'BPF_PROG_ATTACH', so it might give a confusion. However
by the last patch discussion,
word 'load' has been replaced to 'attach'.

Keeping the consistency is very important, but I was just wandering
about making command
similar to 'bpftool 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.

Adding force flag to bpftool seems necessary.
I will add an optional argument for this.

> > +     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.

My apologies, but I'm not sure why err variable isn't needed at here.
AFAIK, 'bpf_set_link_xdp_fd' from libbpf returns the netlink_recv result,
and in order to propagate error, err variable is necessary, I guess?

> > +}
> > +
> > +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.

Again, just trying to make consistent with 'prog.c'.

But clearly it has differences with do_attach/detach from 'prog.c'.
From it, it uses the same parse logic 'parse_attach_detach_args' since
the two command 'bpftool prog attach/detach' uses the same argument format.

However, in here, 'bpftool net' attach and detach requires different number of
argument, so function for parse argument has been defined separately.
The situation is little bit different, but keeping argument parse logic as an
helper, I think it's better in terms of consistency.


About the moving parse logic to a helper, I was trying to keep command
entry (do_attach)
as simple as possible. Parse all the argument in command entry will
make function longer
and might make harder to understand what it does.

And I'm not pretty sure that argument parse logic will stays the same
after other attachment
type comes in. What I mean is, the argument count or type might be
added and to fulfill
all that specific cases, the code might grow larger.

So for the consistency, simplicity and extensibility, I prefer to keep
it as 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?

Well, if the attach_type isn't xdp, the error will be occurred from
the argument parse,
Will it be necessary to reinforce with error logic to make it more secure?

> > +     if (err < 0) {
> > +             p_err("link set %s failed", attach_type_strings[attach_type]);
>
> "link set"?  So you are familiar with iproute2 syntax! :)

Well at first, to avoid confusion about using a command for end-user,
I referenced iproute2 and tried to keep similar message form with it.

But through the discussion about iproute2 and bpftool, it would be better not to
use word such as 'link set'. Maybe "interface %s attach failed" would be better
for clear understanding.

I will fix it at next version of patch.

> > +             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"?

My bad.
Will change it right away.

> >               "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 }
> >  };
>

Thanks for the detailed review! :)

Re: [v2,0/2] tools: bpftool: add net attach/detach command to attach XDP prog

[Daniel T. Lee]

Thank you for letting me know.
I will add to next version of patch.

And, thank you for the detailed review. :)

On Fri, Aug 2, 2019 at 8:21 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu,  1 Aug 2019 17:11:31 +0900, Daniel T. Lee wrote:
> > Currently, bpftool net only supports dumping progs attached on the
> > interface. To attach XDP prog on interface, user must use other tool
> > (eg. iproute2). By this patch, with `bpftool net attach/detach`, user
> > can attach/detach XDP prog on interface.
> >
> >     $ ./bpftool prog
> >     ...
> >     208: xdp  name xdp_prog1  tag ad822e38b629553f  gpl
> >       loaded_at 2019-07-28T18:03:11+0900  uid 0
> >     ...
> >     $ ./bpftool net attach id 208 xdpdrv enp6s0np1
> >     $ ./bpftool net
> >     xdp:
> >     enp6s0np1(5) driver id 208
> >     ...
> >     $ ./bpftool net detach xdpdrv enp6s0np1
> >     $ ./bpftool net
> >     xdp:
> >     ...
> >
> > While this patch only contains support for XDP, through `net
> > attach/detach`, bpftool can further support other prog attach types.
> >
> > XDP attach/detach tested on Mellanox ConnectX-4 and Netronome Agilio.
>
> Please provide documentation for man pages, and bash completions.

Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP on interface

[Y Song]

On Thu, Aug 1, 2019 at 2:04 AM Daniel T. Lee <danieltimlee@gmail.com> 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@gmail.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,
> +};
> +
> +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;
> +       }
> +
> +       NEXT_ARG();
> +       if (!REQ_ARGS(1))
> +               return -EINVAL;
> +
> +       *ifindex = if_nametoindex(*argv);
> +       if (!*ifindex) {
> +               p_err("Invalid ifname");

Do you want to use the full function name "invalid if_nametoindex" here?

> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int do_attach_detach_xdp(int *progfd, enum net_attach_type *attach_type,
> +                               int *ifindex)

You can just use plain int as the argument type here.

> +{
> +       __u32 flags;
> +       int err;
> +
> +       flags = XDP_FLAGS_UPDATE_IF_NOEXIST;
> +       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;

Just do "return bpf_set_link_xdp_fd(...)" and you do not need variable err.

> +}
> +
> +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;
> +
> +       if (is_prefix("xdp", attach_type_strings[attach_type]))
> +               err = do_attach_detach_xdp(&progfd, &attach_type, &ifindex);
> +
> +       if (err < 0) {
> +               p_err("link set %s failed", attach_type_strings[attach_type]);
> +               return -1;
> +       }

The above "if (err < 0)" can be true only under the above "if (is_prefix(..))"
conditions. But compiler may optimize this. So I think current form is okay.
But could you change the return value to "return err" instead of "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"
>                 "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 }
>  };
> --
> 2.20.1
>

Re: [v2,2/2] tools: bpftool: add net detach command to detach XDP on interface

[Y Song]

On Thu, Aug 1, 2019 at 2:04 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> By this commit, using `bpftool net detach`, the attached XDP prog can
> be detached. Detaching the BPF prog will be done through libbpf
> 'bpf_set_link_xdp_fd' with the progfd set to -1.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
> Changes in v2:
>   - command 'unload' changed to 'detach' for the consistency
>
>  tools/bpf/bpftool/net.c | 55 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index f3b57660b303..2ae9a613b05c 100644
> --- a/tools/bpf/bpftool/net.c
> +++ b/tools/bpf/bpftool/net.c
> @@ -281,6 +281,31 @@ static int parse_attach_args(int argc, char **argv, int *progfd,
>         return 0;
>  }
>
> +static int parse_detach_args(int argc, char **argv,
> +                            enum net_attach_type *attach_type, int *ifindex)
> +{
> +       if (!REQ_ARGS(2))
> +               return -EINVAL;
> +
> +       *attach_type = parse_attach_type(*argv);
> +       if (*attach_type == __MAX_NET_ATTACH_TYPE) {
> +               p_err("invalid net attach/detach type");
> +               return -EINVAL;
> +       }
> +
> +       NEXT_ARG();
> +       if (!REQ_ARGS(1))
> +               return -EINVAL;
> +
> +       *ifindex = if_nametoindex(*argv);
> +       if (!*ifindex) {
> +               p_err("Invalid ifname");
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  static int do_attach_detach_xdp(int *progfd, enum net_attach_type *attach_type,
>                                 int *ifindex)
>  {
> @@ -323,6 +348,31 @@ static int do_attach(int argc, char **argv)
>         return 0;
>  }
>
> +static int do_detach(int argc, char **argv)
> +{
> +       enum net_attach_type attach_type;
> +       int err, progfd, ifindex;
> +
> +       err = parse_detach_args(argc, argv, &attach_type, &ifindex);
> +       if (err)
> +               return err;
> +
> +       /* to detach xdp prog */
> +       progfd = -1;
> +       if (is_prefix("xdp", attach_type_strings[attach_type]))
> +               err = do_attach_detach_xdp(&progfd, &attach_type, &ifindex);

Similar to previous patch, parameters no need to be pointer.

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

Maybe "return err"?

> +       }
> +
> +       if (json_output)
> +               jsonw_null(json_wtr);
> +
> +       return 0;
> +}
> +
>  static int do_show(int argc, char **argv)
>  {
>         struct bpf_attach_info attach_info = {};
> @@ -406,6 +456,7 @@ 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 detach LOAD_TYPE <devname>\n"
>                 "       %s %s help\n"
>                 "\n"
>                 "       " HELP_SPEC_PROGRAM "\n"
> @@ -415,7 +466,8 @@ static int do_help(int argc, char **argv)
>                 "      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], bin_name, argv[-2],
> +               bin_name, argv[-2]);
>
>         return 0;
>  }
> @@ -424,6 +476,7 @@ static const struct cmd cmds[] = {
>         { "show",       do_show },
>         { "list",       do_show },
>         { "attach",     do_attach },
> +       { "detach",     do_detach },
>         { "help",       do_help },
>         { 0 }
>  };
> --
> 2.20.1
>

Re: [v2,0/2] tools: bpftool: add net attach/detach command to attach XDP prog

[Y Song]

On Thu, Aug 1, 2019 at 1:33 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> Currently, bpftool net only supports dumping progs attached on the
> interface. To attach XDP prog on interface, user must use other tool
> (eg. iproute2). By this patch, with `bpftool net attach/detach`, user
> can attach/detach XDP prog on interface.
>
>     $ ./bpftool prog
>     ...
>     208: xdp  name xdp_prog1  tag ad822e38b629553f  gpl
>       loaded_at 2019-07-28T18:03:11+0900  uid 0
>     ...
>     $ ./bpftool net attach id 208 xdpdrv enp6s0np1
>     $ ./bpftool net
>     xdp:
>     enp6s0np1(5) driver id 208
>     ...
>     $ ./bpftool net detach xdpdrv enp6s0np1
>     $ ./bpftool net
>     xdp:
>     ...
>
> While this patch only contains support for XDP, through `net
> attach/detach`, bpftool can further support other prog attach types.

new bpftool net subcommands are added. I guess doc and bash
completion needs update as well.

>
> XDP attach/detach tested on Mellanox ConnectX-4 and Netronome Agilio.
>
> ---
> Changes in v2:
>   - command 'load/unload' changed to 'attach/detach' for the consistency
>
> Daniel T. Lee (2):
>   tools: bpftool: add net attach command to attach XDP on interface
>   tools: bpftool: add net detach command to detach XDP on interface
>
>  tools/bpf/bpftool/net.c | 160 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 159 insertions(+), 1 deletion(-)
>
> --
> 2.20.1
>

Re: [v2,2/2] tools: bpftool: add net detach command to detach XDP on interface

[Daniel T. Lee]

On Fri, Aug 2, 2019 at 3:26 PM Y Song <ys114321@gmail.com> wrote:
>
> On Thu, Aug 1, 2019 at 2:04 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > By this commit, using `bpftool net detach`, the attached XDP prog can
> > be detached. Detaching the BPF prog will be done through libbpf
> > 'bpf_set_link_xdp_fd' with the progfd set to -1.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
> > Changes in v2:
> >   - command 'unload' changed to 'detach' for the consistency
> >
> >  tools/bpf/bpftool/net.c | 55 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 54 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> > index f3b57660b303..2ae9a613b05c 100644
> > --- a/tools/bpf/bpftool/net.c
> > +++ b/tools/bpf/bpftool/net.c
> > @@ -281,6 +281,31 @@ static int parse_attach_args(int argc, char **argv, int *progfd,
> >         return 0;
> >  }
> >
> > +static int parse_detach_args(int argc, char **argv,
> > +                            enum net_attach_type *attach_type, int *ifindex)
> > +{
> > +       if (!REQ_ARGS(2))
> > +               return -EINVAL;
> > +
> > +       *attach_type = parse_attach_type(*argv);
> > +       if (*attach_type == __MAX_NET_ATTACH_TYPE) {
> > +               p_err("invalid net attach/detach type");
> > +               return -EINVAL;
> > +       }
> > +
> > +       NEXT_ARG();
> > +       if (!REQ_ARGS(1))
> > +               return -EINVAL;
> > +
> > +       *ifindex = if_nametoindex(*argv);
> > +       if (!*ifindex) {
> > +               p_err("Invalid ifname");
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int do_attach_detach_xdp(int *progfd, enum net_attach_type *attach_type,
> >                                 int *ifindex)
> >  {
> > @@ -323,6 +348,31 @@ static int do_attach(int argc, char **argv)
> >         return 0;
> >  }
> >
> > +static int do_detach(int argc, char **argv)
> > +{
> > +       enum net_attach_type attach_type;
> > +       int err, progfd, ifindex;
> > +
> > +       err = parse_detach_args(argc, argv, &attach_type, &ifindex);
> > +       if (err)
> > +               return err;
> > +
> > +       /* to detach xdp prog */
> > +       progfd = -1;
> > +       if (is_prefix("xdp", attach_type_strings[attach_type]))
> > +               err = do_attach_detach_xdp(&progfd, &attach_type, &ifindex);
>
> Similar to previous patch, parameters no need to be pointer.
>

I will change the parameter as pass by value.

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

Hadn't thought of that.
I will change to it!

> > +       }
> > +
> > +       if (json_output)
> > +               jsonw_null(json_wtr);
> > +
> > +       return 0;
> > +}
> > +
> >  static int do_show(int argc, char **argv)
> >  {
> >         struct bpf_attach_info attach_info = {};
> > @@ -406,6 +456,7 @@ 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 detach LOAD_TYPE <devname>\n"
> >                 "       %s %s help\n"
> >                 "\n"
> >                 "       " HELP_SPEC_PROGRAM "\n"
> > @@ -415,7 +466,8 @@ static int do_help(int argc, char **argv)
> >                 "      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], bin_name, argv[-2],
> > +               bin_name, argv[-2]);
> >
> >         return 0;
> >  }
> > @@ -424,6 +476,7 @@ static const struct cmd cmds[] = {
> >         { "show",       do_show },
> >         { "list",       do_show },
> >         { "attach",     do_attach },
> > +       { "detach",     do_detach },
> >         { "help",       do_help },
> >         { 0 }
> >  };
> > --
> > 2.20.1
> >

Thanks for the review!

Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP on interface

[Daniel T. Lee]

On Fri, Aug 2, 2019 at 3:23 PM Y Song <ys114321@gmail.com> wrote:
>
> On Thu, Aug 1, 2019 at 2:04 AM Daniel T. Lee <danieltimlee@gmail.com> 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@gmail.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,
> > +};
> > +
> > +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;
> > +       }
> > +
> > +       NEXT_ARG();
> > +       if (!REQ_ARGS(1))
> > +               return -EINVAL;
> > +
> > +       *ifindex = if_nametoindex(*argv);
> > +       if (!*ifindex) {
> > +               p_err("Invalid ifname");
>
> Do you want to use the full function name "invalid if_nametoindex" here?
>

No. I was trying to fix the message as "Invalid devname", since the
word "devanme"
is mentioned in 'bpftool net help'.

> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int do_attach_detach_xdp(int *progfd, enum net_attach_type *attach_type,
> > +                               int *ifindex)
>
> You can just use plain int as the argument type here.
>

I will change the parameter as pass by value.

> > +{
> > +       __u32 flags;
> > +       int err;
> > +
> > +       flags = XDP_FLAGS_UPDATE_IF_NOEXIST;
> > +       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;
>
> Just do "return bpf_set_link_xdp_fd(...)" and you do not need variable err.
>

Oh. I've misunderstood why variable err won't be needed.
I'll remove the variable err and update to it.

> > +}
> > +
> > +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;
> > +
> > +       if (is_prefix("xdp", attach_type_strings[attach_type]))
> > +               err = do_attach_detach_xdp(&progfd, &attach_type, &ifindex);
> > +
> > +       if (err < 0) {
> > +               p_err("link set %s failed", attach_type_strings[attach_type]);
> > +               return -1;
> > +       }
>
> The above "if (err < 0)" can be true only under the above "if (is_prefix(..))"
> conditions. But compiler may optimize this. So I think current form is okay.
> But could you change the return value to "return err" instead of "return -1"?
>

Okay. I'll update the return value to "return err".

> > +
> > +       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"
> >                 "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 }
> >  };
> > --
> > 2.20.1
> >

Thank you for the review :)

Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP on interface

[Jakub Kicinski]

On Fri, 2 Aug 2019 14:02:29 +0900, Daniel T. Lee wrote:
> On Fri, Aug 2, 2019 at 8:36 AM Jakub Kicinski  wrote:
> > 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@gmail.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?  
> 
> Yes, ARRAY_SIZE is fine though. But I was just trying to make below
> 'parse_attach_type' consistent with 'parse_attach_type' from the 'prog.c'.
> At 'prog.c', It has same terminator at 'attach_type_strings'.
> 
> Should I change it or keep it?

Oh well, I guess there is some precedent for that :S

Quick grep for const char * const reveals we have around 7 non-NULL
terminated arrays, and 2 NULL terminated. Plus the NULL-terminated
don't align the '=' sign, while most do.

it's not a big deal, my preference is for not NULL terminating here,
and aligning '='.

> > > +     NEXT_ARG();
> > > +     if (!REQ_ARGS(1))
> > > +             return -EINVAL;  
> >
> > Error message needed here.
> >  
> 
> Actually it provides error message like:
> Error: 'xdp' needs at least 1 arguments, 0 found
> 
> are you suggesting that any additional error message is necessary?

Ah, sorry, I missed REQ_ARGS() there!

> > > +             return -EINVAL;
> > > +     }  
> >
> > Please require the dev keyword before the interface name.
> > That'll make it feel closer to prog load syntax.  
> 
> If adding the dev keyword before interface name, will it be too long to type in?

I think it's probably muscle memory for most. Plus we have excellent
bash completions.

> and also `bpftool prog` use extra keyword (such as dev) when it is
> optional keyword.
> 
>        bpftool prog dump jited  PROG [{ file FILE | opcodes | linum }]
>        bpftool prog pin   PROG FILE
>        bpftool prog { load | loadall } OBJ  PATH \
> 
> as you can see here, FILE uses optional keyword 'file' when the
> argument is optional.

Not sure I follow 🤔

>        bpftool prog { load | loadall } OBJ  PATH \
>                          [type TYPE] [dev NAME] \
>                          [map { idx IDX | name NAME } MAP]\
>                          [pinmaps MAP_DIR]
> 
> Yes, bpftool prog load has dev keyword with it,
> 
> but first, like previous, the argument is optional so i think it is
> unnecessary to use optional keyword 'dev'.

The keyword should not be optional if device name is specified.
Maybe lack of coffee on my side..

> and secondly, 'bpftool net attach' isn't really related to 'bpftool prog load'.
>
> At previous version patch, I was using word 'load' instead of
> 'attach', since XDP program is not
> considered as 'BPF_PROG_ATTACH', so it might give a confusion. However
> by the last patch discussion,
> word 'load' has been replaced to 'attach'.
> 
> Keeping the consistency is very important, but I was just wandering
> about making command
> similar to 'bpftool prog load' syntax.

In case of TC the device argument is optional. You may specify it, or
you can refer to TC blocks instead. So for that reason alone I think
it'll be much cleaner to require dev before the interface name.

> > > +     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.  
> 
> Adding force flag to bpftool seems necessary.
> I will add an optional argument for this.

Right, I was wondering if we want to call it force, though? force is
sort of a reuse of iproute2 concept. But it's kind of hard to come up
with names.

Just to be sure - I mean something like:

bpftool net attach xdp id xyz dev ethN noreplace

Rather than:

bpftool -f net attach xdp id xyz dev ethN

> > > +     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.  
> 
> My apologies, but I'm not sure why err variable isn't needed at here.
> AFAIK, 'bpf_set_link_xdp_fd' from libbpf returns the netlink_recv result,
> and in order to propagate error, err variable is necessary, I guess?

	return bpf_set_link_xdp_fd(*ifindex, *progfd, flags);

Is what I meant.

> > > +}
> > > +
> > > +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.  
> 
> Again, just trying to make consistent with 'prog.c'.
> 
> But clearly it has differences with do_attach/detach from 'prog.c'.
> From it, it uses the same parse logic 'parse_attach_detach_args' since
> the two command 'bpftool prog attach/detach' uses the same argument format.
> 
> However, in here, 'bpftool net' attach and detach requires different number of
> argument, so function for parse argument has been defined separately.
> The situation is little bit different, but keeping argument parse logic as an
> helper, I think it's better in terms of consistency.

Well they won't share the same arguments if you add the keyword for
controlling IF_NOEXIST :(

> About the moving parse logic to a helper, I was trying to keep command
> entry (do_attach)
> as simple as possible. Parse all the argument in command entry will
> make function longer
> and might make harder to understand what it does.
> 
> And I'm not pretty sure that argument parse logic will stays the same
> after other attachment
> type comes in. What I mean is, the argument count or type might be
> added and to fulfill
> all that specific cases, the code might grow larger.
> 
> So for the consistency, simplicity and extensibility, I prefer to keep
> it as 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?  
> 
> Well, if the attach_type isn't xdp, the error will be occurred from
> the argument parse,
> Will it be necessary to reinforce with error logic to make it more secure?

Hm. it should already be fine, no? For non-xdp parse_attach_type() will
return __MAX_NET_ATTACH_TYPE, then parsing returns EINVAL and we exit.
Not sure I follow.

Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP on interface

[Daniel T. Lee]

On Sat, Aug 3, 2019 at 3:39 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 2 Aug 2019 14:02:29 +0900, Daniel T. Lee wrote:
> > On Fri, Aug 2, 2019 at 8:36 AM Jakub Kicinski  wrote:
> > > 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@gmail.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?
> >
> > Yes, ARRAY_SIZE is fine though. But I was just trying to make below
> > 'parse_attach_type' consistent with 'parse_attach_type' from the 'prog.c'.
> > At 'prog.c', It has same terminator at 'attach_type_strings'.
> >
> > Should I change it or keep it?
>
> Oh well, I guess there is some precedent for that :S
>
> Quick grep for const char * const reveals we have around 7 non-NULL
> terminated arrays, and 2 NULL terminated. Plus the NULL-terminated
> don't align the '=' sign, while most do.
>
> it's not a big deal, my preference is for not NULL terminating here,
> and aligning '='.
>

I think following the majority, is better for this case.

Like you mentioned, those 2 NULL-terminated arrays are at 'cgroup.c'
and 'prog.c'
and the strings they are defining are 'bpf_attach_type' which is from
uapi 'bpf.h'.

And, in my guess, the reason for those 2 arrays uses NULL-terminator
is because enum from 'bpf_attach_type' has '__MAX_BPF_ATTACH_TYPE' at
the end.

Actually I was kind of uncomfortable with adding an enum since it's
not used globally and *only* used in 'net.c' context. Instead of using
hacky enum entry, stick to the majority, ARRAY_SIZE, is better to keep
consistency.

I'll update this to next version with '=' alignment.

>
> > > > +     NEXT_ARG();
> > > > +     if (!REQ_ARGS(1))
> > > > +             return -EINVAL;
> > >
> > > Error message needed here.
> > >
> >
> > Actually it provides error message like:
> > Error: 'xdp' needs at least 1 arguments, 0 found
> >
> > are you suggesting that any additional error message is necessary?
>
> Ah, sorry, I missed REQ_ARGS() there!
>
> > > > +             return -EINVAL;
> > > > +     }
> > >
> > > Please require the dev keyword before the interface name.
> > > That'll make it feel closer to prog load syntax.
> >
> > If adding the dev keyword before interface name, will it be too long to type in?
>
> I think it's probably muscle memory for most. Plus we have excellent
> bash completions.
>
> > and also `bpftool prog` use extra keyword (such as dev) when it is
> > optional keyword.
> >
> >        bpftool prog dump jited  PROG [{ file FILE | opcodes | linum }]
> >        bpftool prog pin   PROG FILE
> >        bpftool prog { load | loadall } OBJ  PATH \
> >
> > as you can see here, FILE uses optional keyword 'file' when the
> > argument is optional.
>
> Not sure I follow
>

command 'dump' has optional argument '[ file FILE ]',
and command 'pin' has required argument with 'FILE'.

I thought the preceding optional keyword 'file' with lower-case is
intended for the optional argument since it isn't preceded when the
argument is required.

> >        bpftool prog { load | loadall } OBJ  PATH \
> >                          [type TYPE] [dev NAME] \
> >                          [map { idx IDX | name NAME } MAP]\
> >                          [pinmaps MAP_DIR]
> >
> > Yes, bpftool prog load has dev keyword with it,
> >
> > but first, like previous, the argument is optional so i think it is
> > unnecessary to use optional keyword 'dev'.
>
> The keyword should not be optional if device name is specified.
> Maybe lack of coffee on my side..
>
> > and secondly, 'bpftool net attach' isn't really related to 'bpftool prog load'.
> >
> > At previous version patch, I was using word 'load' instead of
> > 'attach', since XDP program is not
> > considered as 'BPF_PROG_ATTACH', so it might give a confusion. However
> > by the last patch discussion,
> > word 'load' has been replaced to 'attach'.
> >
> > Keeping the consistency is very important, but I was just wandering
> > about making command
> > similar to 'bpftool prog load' syntax.
>
> In case of TC the device argument is optional. You may specify it, or
> you can refer to TC blocks instead. So for that reason alone I think
> it'll be much cleaner to require dev before the interface name.
>

Previously I didn't really considered TC.

Considering the extensibility, since device argument could be optional,
requiring dev before the interface name seems necessary.

Thank you for letting me know! :)

> > > > +     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.
> >
> > Adding force flag to bpftool seems necessary.
> > I will add an optional argument for this.
>
> Right, I was wondering if we want to call it force, though? force is
> sort of a reuse of iproute2 concept. But it's kind of hard to come up
> with names.
>
> Just to be sure - I mean something like:
>
> bpftool net attach xdp id xyz dev ethN noreplace
>
> Rather than:
>
> bpftool -f net attach xdp id xyz dev ethN
>

How about a word 'immutable'? 'noreplace' seems good though.
Just suggesting an option.

> > > > +     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.
> >
> > My apologies, but I'm not sure why err variable isn't needed at here.
> > AFAIK, 'bpf_set_link_xdp_fd' from libbpf returns the netlink_recv result,
> > and in order to propagate error, err variable is necessary, I guess?
>
>         return bpf_set_link_xdp_fd(*ifindex, *progfd, flags);
>
> Is what I meant.
>

Oops. I've got it wrong.
I'll update to next patch.

> > > > +}
> > > > +
> > > > +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.
> >
> > Again, just trying to make consistent with 'prog.c'.
> >
> > But clearly it has differences with do_attach/detach from 'prog.c'.
> > From it, it uses the same parse logic 'parse_attach_detach_args' since
> > the two command 'bpftool prog attach/detach' uses the same argument format.
> >
> > However, in here, 'bpftool net' attach and detach requires different number of
> > argument, so function for parse argument has been defined separately.
> > The situation is little bit different, but keeping argument parse logic as an
> > helper, I think it's better in terms of consistency.
>
> Well they won't share the same arguments if you add the keyword for
> controlling IF_NOEXIST :(
>

My apologies, but I think I'm not following with you.

Currently detach/attach isn't sharing same arguments.
And even after adding the argument for IF_NOEXIST such as  [ noreplace ],
it'll stays the same for not sharing same arguments.

I'm not sure why it is not the best idea to move arg parse logic into a helper.

> > About the moving parse logic to a helper, I was trying to keep command
> > entry (do_attach)
> > as simple as possible. Parse all the argument in command entry will
> > make function longer
> > and might make harder to understand what it does.
> >
> > And I'm not pretty sure that argument parse logic will stays the same
> > after other attachment
> > type comes in. What I mean is, the argument count or type might be
> > added and to fulfill
> > all that specific cases, the code might grow larger.
> >
> > So for the consistency, simplicity and extensibility, I prefer to keep
> > it as 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?
> >
> > Well, if the attach_type isn't xdp, the error will be occurred from
> > the argument parse,
> > Will it be necessary to reinforce with error logic to make it more secure?
>
> Hm. it should already be fine, no? For non-xdp parse_attach_type() will
> return __MAX_NET_ATTACH_TYPE, then parsing returns EINVAL and we exit.
> Not sure I follow.

Yes. That was what i meant.

Thank you for taking your time for the review.

Re: [v2,1/2] tools: bpftool: add net attach command to attach XDP on interface

[Jakub Kicinski]

On Sat, 3 Aug 2019 18:39:21 +0900, Daniel T. Lee wrote:
> On Sat, Aug 3, 2019 at 3:39 AM Jakub Kicinski wrote:
> > Right, I was wondering if we want to call it force, though? force is
> > sort of a reuse of iproute2 concept. But it's kind of hard to come up
> > with names.
> >
> > Just to be sure - I mean something like:
> >
> > bpftool net attach xdp id xyz dev ethN noreplace
> >
> > Rather than:
> >
> > bpftool -f net attach xdp id xyz dev ethN
> >  
> 
> How about a word 'immutable'? 'noreplace' seems good though.
> Just suggesting an option.

Hm. Immutable kind of has a meaning in Linux (check out immutable in
extended file attributes, and CAP_LINUX_IMMUTABLE) which is different
than here.. so I'd avoid that one.

Another option I was thinking about was using the same keywords as maps
do: "noexist" - although we don't have a way of doing "exist"
currently, which is kind of breaking the equivalence.

Or maye we should go the same route as iproute2 after all, and set
noreplace by default?  That way we don't need the negation in the 
name. We could use "overwrite", "replace"?

Naming things... :)

> > > > > +}
> > > > > +
> > > > > +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.  
> > >
> > > Again, just trying to make consistent with 'prog.c'.
> > >
> > > But clearly it has differences with do_attach/detach from 'prog.c'.
> > > From it, it uses the same parse logic 'parse_attach_detach_args' since
> > > the two command 'bpftool prog attach/detach' uses the same argument format.
> > >
> > > However, in here, 'bpftool net' attach and detach requires different number of
> > > argument, so function for parse argument has been defined separately.
> > > The situation is little bit different, but keeping argument parse logic as an
> > > helper, I think it's better in terms of consistency.  
> >
> > Well they won't share the same arguments if you add the keyword for
> > controlling IF_NOEXIST :(
> 
> My apologies, but I think I'm not following with you.
> 
> Currently detach/attach isn't sharing same arguments.
> And even after adding the argument for IF_NOEXIST such as  [ noreplace ],
> it'll stays the same for not sharing same arguments.

Ah, my apologies I misread your message.

> I'm not sure why it is not the best idea to move arg parse logic into a helper.

Output args are kind of ugly, and having to pass each parameter through
output arg seems like something that could get out of hand as the
number grows. 

Usually command handling functions are relatively small and
straightforward in bpftool so it's quite nice to have it all in one
simple procedure.

But I'm not feeling too strongly about it. Feel free to leave the
parsing in separate functions if you prefer!