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

[v3,0/4] 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
	16: xdp  name xdp_prog1  tag 539ec6ce11b52f98  gpl
        loaded_at 2019-08-07T08:30:17+0900  uid 0
    ...
	20: xdp  name xdp_fwd_prog  tag b9cb69f121e4a274  gpl
        loaded_at 2019-08-07T08:30:17+0900  uid 0
    
	# bpftool net attach xdpdrv id 16 dev enp6s0np0
    # bpftool net
    xdp:
	enp6s0np0(4) driver id 16
    
	# bpftool net attach xdpdrv id 20 dev enp6s0np0 overwrite
    # bpftool net
    xdp:
	enp6s0np0(4) driver id 20

	# bpftool net detach xdpdrv dev enp6s0np0
    # 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 v3:
  - added 'overwrite' option for replacing previously attached XDP prog
  - command argument order has been changed ('ATTACH_TYPE' comes first)
  - add 'dev' keyword in front of <devname>
  - added bash-completion and documentation

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

Daniel T. Lee (4):
  tools: bpftool: add net attach command to attach XDP on interface
  tools: bpftool: add net detach command to detach XDP on interface
  tools: bpftool: add bash-completion for net attach/detach
  tools: bpftool: add documentation for net attach/detach

 .../bpf/bpftool/Documentation/bpftool-net.rst |  51 ++++-
 tools/bpf/bpftool/bash-completion/bpftool     |  64 ++++++-
 tools/bpf/bpftool/net.c                       | 181 ++++++++++++++++--
 3 files changed, 273 insertions(+), 23 deletions(-)

-- 
2.20.1

[v3,1/4] 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.

With 'overwrite' option at argument, attached XDP program could be replaced.
Added new helper 'net_parse_dev' to parse the network device at argument.

BPF prog will be attached through libbpf 'bpf_set_link_xdp_fd'.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 tools/bpf/bpftool/net.c | 141 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 130 insertions(+), 11 deletions(-)

diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index 67e99c56bc88..c05a3fac5cac 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,
+};
+
+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",
+};
+
+const size_t max_net_attach_type = ARRAY_SIZE(attach_type_strings);
+
+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,97 @@ static int query_flow_dissector(struct bpf_attach_info *attach_info)
 	return 0;
 }
 
+static int net_parse_dev(int *argc, char ***argv)
+{
+	int ifindex;
+
+	if (is_prefix(**argv, "dev")) {
+		NEXT_ARGP();
+
+		ifindex = if_nametoindex(**argv);
+		if (!ifindex)
+			p_err("invalid devname %s", **argv);
+
+		NEXT_ARGP();
+	} else {
+		p_err("expected 'dev', got: '%s'?", **argv);
+		return -1;
+	}
+
+	return ifindex;
+}
+
+static int do_attach_detach_xdp(int progfd, enum net_attach_type attach_type,
+				int ifindex, bool overwrite)
+{
+	__u32 flags = 0;
+
+	if (!overwrite)
+		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;
+
+	return bpf_set_link_xdp_fd(ifindex, progfd, flags);
+}
+
+static int do_attach(int argc, char **argv)
+{
+	enum net_attach_type attach_type;
+	int progfd, ifindex, err = 0;
+	bool overwrite = false;
+
+	/* parse attach args */
+	if (!REQ_ARGS(5))
+		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();
+	progfd = prog_parse_fd(&argc, &argv);
+	if (progfd < 0)
+		return -EINVAL;
+
+	ifindex = net_parse_dev(&argc, &argv);
+	if (ifindex < 1) {
+		close(progfd);
+		return -EINVAL;
+	}
+
+	if (argc) {
+		if (is_prefix(*argv, "overwrite")) {
+			overwrite = true;
+		} else {
+			p_err("expected 'overwrite', got: '%s'?", *argv);
+			close(progfd);
+			return -EINVAL;
+		}
+	}
+
+	/* attach xdp prog */
+	if (is_prefix("xdp", attach_type_strings[attach_type]))
+		err = do_attach_detach_xdp(progfd, attach_type, ifindex,
+					   overwrite);
+
+	if (err < 0) {
+		p_err("interface %s attach failed",
+		      attach_type_strings[attach_type]);
+		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 = {};
@@ -231,17 +351,10 @@ static int do_show(int argc, char **argv)
 	unsigned int nl_pid;
 	char err_buf[256];
 
-	if (argc == 2) {
-		if (strcmp(argv[0], "dev") != 0)
-			usage();
-		filter_idx = if_nametoindex(argv[1]);
-		if (filter_idx == 0) {
-			fprintf(stderr, "invalid dev name %s\n", argv[1]);
-			return -1;
-		}
-	} else if (argc != 0) {
+	if (argc == 2)
+		filter_idx = net_parse_dev(&argc, &argv);
+	else if (argc != 0)
 		usage();
-	}
 
 	ret = query_flow_dissector(&attach_info);
 	if (ret)
@@ -305,13 +418,18 @@ static int do_help(int argc, char **argv)
 
 	fprintf(stderr,
 		"Usage: %s %s { show | list } [dev <devname>]\n"
+		"       %s %s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
 		"       %s %s help\n"
+		"\n"
+		"       " HELP_SPEC_PROGRAM "\n"
+		"       ATTACH_TYPE := { xdp | xdpgeneric | xdpdrv | xdpoffload }\n"
+		"\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 +437,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

[v3,2/4] 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>
---
 tools/bpf/bpftool/net.c | 42 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index c05a3fac5cac..7be96acb08e0 100644
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -343,6 +343,43 @@ 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 progfd, ifindex, err = 0;
+
+	/* parse detach args */
+	if (!REQ_ARGS(3))
+		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();
+	ifindex = net_parse_dev(&argc, &argv);
+	if (ifindex < 1)
+		return -EINVAL;
+
+	/* detach xdp prog */
+	progfd = -1;
+	if (is_prefix("xdp", attach_type_strings[attach_type]))
+		err = do_attach_detach_xdp(progfd, attach_type, ifindex, NULL);
+
+	if (err < 0) {
+		p_err("interface %s detach failed",
+		      attach_type_strings[attach_type]);
+		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 = {};
@@ -419,6 +456,7 @@ static int do_help(int argc, char **argv)
 	fprintf(stderr,
 		"Usage: %s %s { show | list } [dev <devname>]\n"
 		"       %s %s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
+		"       %s %s detach ATTACH_TYPE dev <devname>\n"
 		"       %s %s help\n"
 		"\n"
 		"       " HELP_SPEC_PROGRAM "\n"
@@ -429,7 +467,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;
 }
@@ -438,6 +477,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

[v3,3/4] tools: bpftool: add bash-completion for net attach/detach

[Daniel T. Lee]

This commit adds bash-completion for new "net attach/detach"
subcommand for attaching XDP program on interface.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 tools/bpf/bpftool/bash-completion/bpftool | 64 +++++++++++++++++++----
 1 file changed, 55 insertions(+), 9 deletions(-)

diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index c8f42e1fcbc9..1d81cb09d478 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -201,6 +201,10 @@ _bpftool()
             _bpftool_get_prog_tags
             return 0
             ;;
+        dev)
+            _sysfs_get_netdevs
+            return 0
+            ;;
         file|pinned)
             _filedir
             return 0
@@ -399,10 +403,6 @@ _bpftool()
                             _filedir
                             return 0
                             ;;
-                        dev)
-                            _sysfs_get_netdevs
-                            return 0
-                            ;;
                         *)
                             COMPREPLY=( $( compgen -W "map" -- "$cur" ) )
                             _bpftool_once_attr 'type'
@@ -498,10 +498,6 @@ _bpftool()
                         key|value|flags|name|entries)
                             return 0
                             ;;
-                        dev)
-                            _sysfs_get_netdevs
-                            return 0
-                            ;;
                         *)
                             _bpftool_once_attr 'type'
                             _bpftool_once_attr 'key'
@@ -775,11 +771,61 @@ _bpftool()
             esac
             ;;
         net)
+            local PROG_TYPE='id pinned tag'
+            local ATTACH_TYPES='xdp xdpgeneric xdpdrv xdpoffload'
             case $command in
+                show|list)
+                    [[ $prev != "$command" ]] && return 0
+                    COMPREPLY=( $( compgen -W 'dev' -- "$cur" ) )
+                    return 0
+                    ;;
+                attach)
+                    case $cword in
+                        3)
+                            COMPREPLY=( $( compgen -W "$ATTACH_TYPES" -- "$cur" ) )
+                            return 0
+                            ;;
+                        4)
+                            COMPREPLY=( $( compgen -W "$PROG_TYPE" -- "$cur" ) )
+                            return 0
+                            ;;
+                        5)
+                            case $prev in
+                                id)
+                                    _bpftool_get_prog_ids
+                                    ;;
+                                pinned)
+                                    _filedir
+                                    ;;
+                            esac
+                            return 0
+                            ;;
+                        6)
+                            COMPREPLY=( $( compgen -W 'dev' -- "$cur" ) )
+                            return 0
+                            ;;
+                        8)
+                            _bpftool_once_attr 'overwrite'
+                            return 0
+                            ;;
+                    esac
+                    ;;
+                detach)
+                    case $cword in
+                        3)
+                            COMPREPLY=( $( compgen -W "$ATTACH_TYPES" -- "$cur" ) )
+                            return 0
+                            ;;
+                        4)
+                            COMPREPLY=( $( compgen -W 'dev' -- "$cur" ) )
+                            return 0
+                            ;;
+                    esac
+                    ;;
                 *)
                     [[ $prev == $object ]] && \
                         COMPREPLY=( $( compgen -W 'help \
-                            show list' -- "$cur" ) )
+                            show list attach detach' -- "$cur" ) )
                     ;;
             esac
             ;;
-- 
2.20.1

[v3,4/4] tools: bpftool: add documentation for net attach/detach

[Daniel T. Lee]

Since, new sub-command 'net attach/detach' has been added for
attaching XDP program on interface,
this commit documents usage and sample output of `net attach/detach`.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 .../bpf/bpftool/Documentation/bpftool-net.rst | 51 +++++++++++++++++--
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-net.rst b/tools/bpf/bpftool/Documentation/bpftool-net.rst
index d8e5237a2085..4ad1a380e186 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-net.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-net.rst
@@ -15,17 +15,22 @@ SYNOPSIS
 	*OPTIONS* := { [{ **-j** | **--json** }] [{ **-p** | **--pretty** }] }
 
 	*COMMANDS* :=
-	{ **show** | **list** } [ **dev** name ] | **help**
+	{ **show** | **list** | **attach** | **detach** | **help** }
 
 NET COMMANDS
 ============
 
-|	**bpftool** **net { show | list } [ dev name ]**
+|	**bpftool** **net { show | list }** [ **dev** *name* ]
+|	**bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *name* [ **overwrite** ]
+|	**bpftool** **net detach** *ATTACH_TYPE* **dev** *name*
 |	**bpftool** **net help**
+|
+|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
+|	*ATTACH_TYPE* := { **xdp** | **xdpgeneric** | **xdpdrv** | **xdpoffload** }
 
 DESCRIPTION
 ===========
-	**bpftool net { show | list } [ dev name ]**
+	**bpftool net { show | list }** [ **dev** *name* ]
                   List bpf program attachments in the kernel networking subsystem.
 
                   Currently, only device driver xdp attachments and tc filter
@@ -47,6 +52,18 @@ DESCRIPTION
                   all bpf programs attached to non clsact qdiscs, and finally all
                   bpf programs attached to root and clsact qdisc.
 
+	**bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *name* [ **overwrite** ]
+                  Attach bpf program *PROG* to network interface *name* with
+                  type specified by *ATTACH_TYPE*. Previously attached bpf program
+                  can be replaced by the command used with **overwrite** option.
+                  Currently, *ATTACH_TYPE* only contains XDP programs.
+
+	**bpftool** **net detach** *ATTACH_TYPE* **dev** *name*
+                  Detach bpf program attached to network interface *name* with
+                  type specified by *ATTACH_TYPE*. To detach bpf program, same
+                  *ATTACH_TYPE* previously used for attach must be specified.
+                  Currently, *ATTACH_TYPE* only contains XDP programs.
+
 	**bpftool net help**
 		  Print short help message.
 
@@ -137,6 +154,34 @@ EXAMPLES
         }
     ]
 
+|
+| **# bpftool net attach xdpdrv id 16 dev enp6s0np0**
+| **# bpftool net**
+
+::
+
+      xdp:
+      enp6s0np0(4) driver id 16
+
+|
+| **# bpftool net attach xdpdrv id 16 dev enp6s0np0**
+| **# bpftool net attach xdpdrv id 20 dev enp6s0np0 overwrite**
+| **# bpftool net**
+
+::
+
+      xdp:
+      enp6s0np0(4) driver id 20
+
+|
+| **# bpftool net attach xdpdrv id 16 dev enp6s0np0**
+| **# bpftool net detach xdpdrv dev enp6s0np0**
+| **# bpftool net**
+
+::
+
+      xdp:
+
 
 SEE ALSO
 ========
-- 
2.20.1

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

[Y Song]

On Tue, Aug 6, 2019 at 7:25 PM 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.
>
> With 'overwrite' option at argument, attached XDP program could be replaced.
> Added new helper 'net_parse_dev' to parse the network device at argument.
>
> BPF prog will be attached through libbpf 'bpf_set_link_xdp_fd'.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>

Acked-by: Yonghong Song <yhs@fb.com>

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

[Y Song]

On Tue, Aug 6, 2019 at 7:25 PM 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>
> ---
>  tools/bpf/bpftool/net.c | 42 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index c05a3fac5cac..7be96acb08e0 100644
> --- a/tools/bpf/bpftool/net.c
> +++ b/tools/bpf/bpftool/net.c
> @@ -343,6 +343,43 @@ 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 progfd, ifindex, err = 0;
> +
> +       /* parse detach args */
> +       if (!REQ_ARGS(3))
> +               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();
> +       ifindex = net_parse_dev(&argc, &argv);
> +       if (ifindex < 1)
> +               return -EINVAL;
> +
> +       /* detach xdp prog */
> +       progfd = -1;
> +       if (is_prefix("xdp", attach_type_strings[attach_type]))
> +               err = do_attach_detach_xdp(progfd, attach_type, ifindex, NULL);

I found an issue here. This is probably related to do_attach_detach_xdp.

-bash-4.4$ sudo ./bpftool net attach x pinned /sys/fs/bpf/xdp_example dev v1
-bash-4.4$ sudo ./bpftool net
xdp:
v1(4) driver id 1172

tc:
eth0(2) clsact/ingress fbflow_icmp id 29 act []
eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
eth0(2) clsact/egress fbflow_egress id 28
eth0(2) clsact/egress fbflow_sslwall_egress id 35

flow_dissector:

-bash-4.4$ sudo ./bpftool net detach x dev v2
-bash-4.4$ sudo ./bpftool net
xdp:
v1(4) driver id 1172

tc:
eth0(2) clsact/ingress fbflow_icmp id 29 act []
eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
eth0(2) clsact/egress fbflow_egress id 28
eth0(2) clsact/egress fbflow_sslwall_egress id 35

flow_dissector:

-bash-4.4$

Basically detaching may fail due to wrong dev name or wrong type, etc.
But the tool did not return an error. Is this expected?
This may be related to this funciton "bpf_set_link_xdp_fd()".
So this patch itself should be okay.

> +
> +       if (err < 0) {
> +               p_err("interface %s detach failed",
> +                     attach_type_strings[attach_type]);
> +               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 = {};
> @@ -419,6 +456,7 @@ static int do_help(int argc, char **argv)
>         fprintf(stderr,
>                 "Usage: %s %s { show | list } [dev <devname>]\n"
>                 "       %s %s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
> +               "       %s %s detach ATTACH_TYPE dev <devname>\n"
>                 "       %s %s help\n"
>                 "\n"
>                 "       " HELP_SPEC_PROGRAM "\n"
> @@ -429,7 +467,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;
>  }
> @@ -438,6 +477,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: [v3,2/4] tools: bpftool: add net detach command to detach XDP on interface

[Maciej Fijalkowski]

On Wed, 7 Aug 2019 10:02:04 -0700
Y Song <ys114321@gmail.com> wrote:

> On Tue, Aug 6, 2019 at 7:25 PM 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>
> > ---
> >  tools/bpf/bpftool/net.c | 42 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> > index c05a3fac5cac..7be96acb08e0 100644
> > --- a/tools/bpf/bpftool/net.c
> > +++ b/tools/bpf/bpftool/net.c
> > @@ -343,6 +343,43 @@ 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 progfd, ifindex, err = 0;
> > +
> > +       /* parse detach args */
> > +       if (!REQ_ARGS(3))
> > +               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();
> > +       ifindex = net_parse_dev(&argc, &argv);
> > +       if (ifindex < 1)
> > +               return -EINVAL;
> > +
> > +       /* detach xdp prog */
> > +       progfd = -1;
> > +       if (is_prefix("xdp", attach_type_strings[attach_type]))
> > +               err = do_attach_detach_xdp(progfd, attach_type, ifindex, NULL);  
> 
> I found an issue here. This is probably related to do_attach_detach_xdp.
> 
> -bash-4.4$ sudo ./bpftool net attach x pinned /sys/fs/bpf/xdp_example dev v1
> -bash-4.4$ sudo ./bpftool net
> xdp:
> v1(4) driver id 1172
> 
> tc:
> eth0(2) clsact/ingress fbflow_icmp id 29 act []
> eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> eth0(2) clsact/egress fbflow_egress id 28
> eth0(2) clsact/egress fbflow_sslwall_egress id 35
> 
> flow_dissector:
> 
> -bash-4.4$ sudo ./bpftool net detach x dev v2

Shouldn't this be v1 as dev?

> -bash-4.4$ sudo ./bpftool net
> xdp:
> v1(4) driver id 1172
> 
> tc:
> eth0(2) clsact/ingress fbflow_icmp id 29 act []
> eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> eth0(2) clsact/egress fbflow_egress id 28
> eth0(2) clsact/egress fbflow_sslwall_egress id 35
> 
> flow_dissector:
> 
> -bash-4.4$
> 
> Basically detaching may fail due to wrong dev name or wrong type, etc.
> But the tool did not return an error. Is this expected?
> This may be related to this funciton "bpf_set_link_xdp_fd()".
> So this patch itself should be okay.
> 
> > +
> > +       if (err < 0) {
> > +               p_err("interface %s detach failed",
> > +                     attach_type_strings[attach_type]);
> > +               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 = {};
> > @@ -419,6 +456,7 @@ static int do_help(int argc, char **argv)
> >         fprintf(stderr,
> >                 "Usage: %s %s { show | list } [dev <devname>]\n"
> >                 "       %s %s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
> > +               "       %s %s detach ATTACH_TYPE dev <devname>\n"
> >                 "       %s %s help\n"
> >                 "\n"
> >                 "       " HELP_SPEC_PROGRAM "\n"
> > @@ -429,7 +467,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;
> >  }
> > @@ -438,6 +477,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: [v3,2/4] tools: bpftool: add net detach command to detach XDP on interface

[Jakub Kicinski]

On Wed, 7 Aug 2019 10:02:04 -0700, Y Song wrote:
> -bash-4.4$ sudo ./bpftool net detach x dev v2
> -bash-4.4$ sudo ./bpftool net
> xdp:
> v1(4) driver id 1172
> 
> tc:
> eth0(2) clsact/ingress fbflow_icmp id 29 act []
> eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> eth0(2) clsact/egress fbflow_egress id 28
> eth0(2) clsact/egress fbflow_sslwall_egress id 35
> 
> flow_dissector:
> 
> -bash-4.4$
> 
> Basically detaching may fail due to wrong dev name or wrong type, etc.
> But the tool did not return an error. Is this expected?
> This may be related to this funciton "bpf_set_link_xdp_fd()".
> So this patch itself should be okay.

Yup, I'm pretty sure kernel doesn't return errors on unset if there 
is no program on the interface.

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

[Y Song]

On Wed, Aug 7, 2019 at 11:30 AM Maciej Fijalkowski
<maciejromanfijalkowski@gmail.com> wrote:
>
> On Wed, 7 Aug 2019 10:02:04 -0700
> Y Song <ys114321@gmail.com> wrote:
>
> > On Tue, Aug 6, 2019 at 7:25 PM 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>
> > > ---
> > >  tools/bpf/bpftool/net.c | 42 ++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> > > index c05a3fac5cac..7be96acb08e0 100644
> > > --- a/tools/bpf/bpftool/net.c
> > > +++ b/tools/bpf/bpftool/net.c
> > > @@ -343,6 +343,43 @@ 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 progfd, ifindex, err = 0;
> > > +
> > > +       /* parse detach args */
> > > +       if (!REQ_ARGS(3))
> > > +               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();
> > > +       ifindex = net_parse_dev(&argc, &argv);
> > > +       if (ifindex < 1)
> > > +               return -EINVAL;
> > > +
> > > +       /* detach xdp prog */
> > > +       progfd = -1;
> > > +       if (is_prefix("xdp", attach_type_strings[attach_type]))
> > > +               err = do_attach_detach_xdp(progfd, attach_type, ifindex, NULL);
> >
> > I found an issue here. This is probably related to do_attach_detach_xdp.
> >
> > -bash-4.4$ sudo ./bpftool net attach x pinned /sys/fs/bpf/xdp_example dev v1
> > -bash-4.4$ sudo ./bpftool net
> > xdp:
> > v1(4) driver id 1172
> >
> > tc:
> > eth0(2) clsact/ingress fbflow_icmp id 29 act []
> > eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> > eth0(2) clsact/egress fbflow_egress id 28
> > eth0(2) clsact/egress fbflow_sslwall_egress id 35
> >
> > flow_dissector:
> >
> > -bash-4.4$ sudo ./bpftool net detach x dev v2
>
> Shouldn't this be v1 as dev?

I am testing a scenario where with wrong devname
we did not return an error.

Yes, if dev "v1", it works as expected.

>
> > -bash-4.4$ sudo ./bpftool net
> > xdp:
> > v1(4) driver id 1172
> >
> > tc:
> > eth0(2) clsact/ingress fbflow_icmp id 29 act []
> > eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> > eth0(2) clsact/egress fbflow_egress id 28
> > eth0(2) clsact/egress fbflow_sslwall_egress id 35
> >
> > flow_dissector:
> >
> > -bash-4.4$
> >
> > Basically detaching may fail due to wrong dev name or wrong type, etc.
> > But the tool did not return an error. Is this expected?
> > This may be related to this funciton "bpf_set_link_xdp_fd()".
> > So this patch itself should be okay.
> >
> > > +
> > > +       if (err < 0) {
> > > +               p_err("interface %s detach failed",
> > > +                     attach_type_strings[attach_type]);
> > > +               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 = {};
> > > @@ -419,6 +456,7 @@ static int do_help(int argc, char **argv)
> > >         fprintf(stderr,
> > >                 "Usage: %s %s { show | list } [dev <devname>]\n"
> > >                 "       %s %s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
> > > +               "       %s %s detach ATTACH_TYPE dev <devname>\n"
> > >                 "       %s %s help\n"
> > >                 "\n"
> > >                 "       " HELP_SPEC_PROGRAM "\n"
> > > @@ -429,7 +467,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;
> > >  }
> > > @@ -438,6 +477,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: [v3,2/4] tools: bpftool: add net detach command to detach XDP on interface

[Maciej Fijalkowski]

On Wed, 7 Aug 2019 13:12:17 -0700
Y Song <ys114321@gmail.com> wrote:

> On Wed, Aug 7, 2019 at 11:30 AM Maciej Fijalkowski
> <maciejromanfijalkowski@gmail.com> wrote:
> >
> > On Wed, 7 Aug 2019 10:02:04 -0700
> > Y Song <ys114321@gmail.com> wrote:
> >  
> > > On Tue, Aug 6, 2019 at 7:25 PM 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>
> > > > ---
> > > >  tools/bpf/bpftool/net.c | 42 ++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> > > > index c05a3fac5cac..7be96acb08e0 100644
> > > > --- a/tools/bpf/bpftool/net.c
> > > > +++ b/tools/bpf/bpftool/net.c
> > > > @@ -343,6 +343,43 @@ 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 progfd, ifindex, err = 0;
> > > > +
> > > > +       /* parse detach args */
> > > > +       if (!REQ_ARGS(3))
> > > > +               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();
> > > > +       ifindex = net_parse_dev(&argc, &argv);
> > > > +       if (ifindex < 1)
> > > > +               return -EINVAL;
> > > > +
> > > > +       /* detach xdp prog */
> > > > +       progfd = -1;
> > > > +       if (is_prefix("xdp", attach_type_strings[attach_type]))
> > > > +               err = do_attach_detach_xdp(progfd, attach_type, ifindex, NULL);  
> > >
> > > I found an issue here. This is probably related to do_attach_detach_xdp.
> > >
> > > -bash-4.4$ sudo ./bpftool net attach x pinned /sys/fs/bpf/xdp_example dev v1
> > > -bash-4.4$ sudo ./bpftool net
> > > xdp:
> > > v1(4) driver id 1172
> > >
> > > tc:
> > > eth0(2) clsact/ingress fbflow_icmp id 29 act []
> > > eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> > > eth0(2) clsact/egress fbflow_egress id 28
> > > eth0(2) clsact/egress fbflow_sslwall_egress id 35
> > >
> > > flow_dissector:
> > >
> > > -bash-4.4$ sudo ./bpftool net detach x dev v2  
> >
> > Shouldn't this be v1 as dev?  
> 
> I am testing a scenario where with wrong devname
> we did not return an error.

Ah ok. In this scenario if driver has a native xdp support we would be invoking
its ndo_bpf even if there's no prog currently attached and it wouldn't return
error value.

Looking at dev_xdp_uninstall, setting driver's prog to NULL is being done only
when prog is attached. Maybe we should consider querying the driver in
dev_change_xdp_fd regardless of passed fd value? E.g. don't query only when
prog >= 0.

I don't recall whether this was brought up previously.

CCing Jakub so we have one thread.

Maciej

> 
> Yes, if dev "v1", it works as expected.
> 
> >  
> > > -bash-4.4$ sudo ./bpftool net
> > > xdp:
> > > v1(4) driver id 1172
> > >
> > > tc:
> > > eth0(2) clsact/ingress fbflow_icmp id 29 act []
> > > eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> > > eth0(2) clsact/egress fbflow_egress id 28
> > > eth0(2) clsact/egress fbflow_sslwall_egress id 35
> > >
> > > flow_dissector:
> > >
> > > -bash-4.4$
> > >
> > > Basically detaching may fail due to wrong dev name or wrong type, etc.
> > > But the tool did not return an error. Is this expected?
> > > This may be related to this funciton "bpf_set_link_xdp_fd()".
> > > So this patch itself should be okay.
> > >  
> > > > +
> > > > +       if (err < 0) {
> > > > +               p_err("interface %s detach failed",
> > > > +                     attach_type_strings[attach_type]);
> > > > +               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 = {};
> > > > @@ -419,6 +456,7 @@ static int do_help(int argc, char **argv)
> > > >         fprintf(stderr,
> > > >                 "Usage: %s %s { show | list } [dev <devname>]\n"
> > > >                 "       %s %s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
> > > > +               "       %s %s detach ATTACH_TYPE dev <devname>\n"
> > > >                 "       %s %s help\n"
> > > >                 "\n"
> > > >                 "       " HELP_SPEC_PROGRAM "\n"
> > > > @@ -429,7 +467,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;
> > > >  }
> > > > @@ -438,6 +477,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: [v3,1/4] tools: bpftool: add net attach command to attach XDP on interface

[Jakub Kicinski]

On Wed,  7 Aug 2019 11:25:06 +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.
> 
> With 'overwrite' option at argument, attached XDP program could be replaced.
> Added new helper 'net_parse_dev' to parse the network device at argument.
> 
> BPF prog will be attached through libbpf 'bpf_set_link_xdp_fd'.
> 
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
>  tools/bpf/bpftool/net.c | 141 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 130 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index 67e99c56bc88..c05a3fac5cac 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,
> +};
> +
> +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",
> +};
> +
> +const size_t max_net_attach_type = ARRAY_SIZE(attach_type_strings);

Nit: in practice max_.._type is num_types - 1, so perhaps rename this
to num_.. or such?

> +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]))

                   ^
this is misaligned by one space

Please try checkpatch with the --strict option to catch these.

> +			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,97 @@ static int query_flow_dissector(struct bpf_attach_info *attach_info)
>  	return 0;
>  }
>  
> +static int net_parse_dev(int *argc, char ***argv)
> +{
> +	int ifindex;
> +
> +	if (is_prefix(**argv, "dev")) {
> +		NEXT_ARGP();
> +
> +		ifindex = if_nametoindex(**argv);
> +		if (!ifindex)
> +			p_err("invalid devname %s", **argv);
> +
> +		NEXT_ARGP();
> +	} else {
> +		p_err("expected 'dev', got: '%s'?", **argv);
> +		return -1;
> +	}
> +
> +	return ifindex;
> +}
> +
> +static int do_attach_detach_xdp(int progfd, enum net_attach_type attach_type,
> +				int ifindex, bool overwrite)
> +{
> +	__u32 flags = 0;
> +
> +	if (!overwrite)
> +		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;
> +
> +	return bpf_set_link_xdp_fd(ifindex, progfd, flags);
> +}
> +
> +static int do_attach(int argc, char **argv)
> +{
> +	enum net_attach_type attach_type;
> +	int progfd, ifindex, err = 0;
> +	bool overwrite = false;
> +
> +	/* parse attach args */
> +	if (!REQ_ARGS(5))
> +		return -EINVAL;
> +
> +	attach_type = parse_attach_type(*argv);
> +	if (attach_type == max_net_attach_type) {
> +		p_err("invalid net attach/detach type");

worth adding the type to the error message so that user know which part
of command line was wrong:

	p_err("invalid net attach/detach type '%s'", *argv);

> +		return -EINVAL;
> +	}
> +
> +	NEXT_ARG();

nit: the new line should be before NEXT_ARG(), IOV NEXT_ARG() belongs
to the code which consumed the argument

> +	progfd = prog_parse_fd(&argc, &argv);
> +	if (progfd < 0)
> +		return -EINVAL;
> +
> +	ifindex = net_parse_dev(&argc, &argv);
> +	if (ifindex < 1) {
> +		close(progfd);
> +		return -EINVAL;
> +	}
> +
> +	if (argc) {
> +		if (is_prefix(*argv, "overwrite")) {
> +			overwrite = true;
> +		} else {
> +			p_err("expected 'overwrite', got: '%s'?", *argv);
> +			close(progfd);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* attach xdp prog */
> +	if (is_prefix("xdp", attach_type_strings[attach_type]))

I'm still unclear on why this if is needed

> +		err = do_attach_detach_xdp(progfd, attach_type, ifindex,
> +					   overwrite);
> +
> +	if (err < 0) {
> +		p_err("interface %s attach failed",
> +		      attach_type_strings[attach_type]);

Please add the error string, like:

		p_err("interface %s attach failed: %s",
		      attach_type_strings[attach_type], strerror(errno));


> +		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 = {};
> @@ -231,17 +351,10 @@ static int do_show(int argc, char **argv)
>  	unsigned int nl_pid;
>  	char err_buf[256];
>  
> -	if (argc == 2) {
> -		if (strcmp(argv[0], "dev") != 0)
> -			usage();
> -		filter_idx = if_nametoindex(argv[1]);
> -		if (filter_idx == 0) {
> -			fprintf(stderr, "invalid dev name %s\n", argv[1]);
> -			return -1;
> -		}
> -	} else if (argc != 0) {
> +	if (argc == 2)
> +		filter_idx = net_parse_dev(&argc, &argv);

You should check filter_idx is not negative here, no?

> +	else if (argc != 0)
>  		usage();
> -	}
>  
>  	ret = query_flow_dissector(&attach_info);
>  	if (ret)

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

[Daniel T. Lee]

On Thu, Aug 8, 2019 at 5:42 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed,  7 Aug 2019 11:25:06 +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.
> >
> > With 'overwrite' option at argument, attached XDP program could be replaced.
> > Added new helper 'net_parse_dev' to parse the network device at argument.
> >
> > BPF prog will be attached through libbpf 'bpf_set_link_xdp_fd'.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
> >  tools/bpf/bpftool/net.c | 141 ++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 130 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> > index 67e99c56bc88..c05a3fac5cac 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,
> > +};
> > +
> > +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",
> > +};
> > +
> > +const size_t max_net_attach_type = ARRAY_SIZE(attach_type_strings);
>
> Nit: in practice max_.._type is num_types - 1, so perhaps rename this
> to num_.. or such?
>

I can see at 'map.c', it declares ARRAY_SIZE with '_size' suffix.
         const size_t map_type_name_size = ARRAY_SIZE(map_type_name);

I'll change this variable name 'max_net_attach_type' to 'net_attach_type_size'.

> > +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]))
>
>                    ^
> this is misaligned by one space
>
> Please try checkpatch with the --strict option to catch these.
>

I didn't know checkpatch has strict option.
Thanks for letting me know!

> > +                     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,97 @@ static int query_flow_dissector(struct bpf_attach_info *attach_info)
> >       return 0;
> >  }
> >
> > +static int net_parse_dev(int *argc, char ***argv)
> > +{
> > +     int ifindex;
> > +
> > +     if (is_prefix(**argv, "dev")) {
> > +             NEXT_ARGP();
> > +
> > +             ifindex = if_nametoindex(**argv);
> > +             if (!ifindex)
> > +                     p_err("invalid devname %s", **argv);
> > +
> > +             NEXT_ARGP();
> > +     } else {
> > +             p_err("expected 'dev', got: '%s'?", **argv);
> > +             return -1;
> > +     }
> > +
> > +     return ifindex;
> > +}
> > +
> > +static int do_attach_detach_xdp(int progfd, enum net_attach_type attach_type,
> > +                             int ifindex, bool overwrite)
> > +{
> > +     __u32 flags = 0;
> > +
> > +     if (!overwrite)
> > +             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;
> > +
> > +     return bpf_set_link_xdp_fd(ifindex, progfd, flags);
> > +}
> > +
> > +static int do_attach(int argc, char **argv)
> > +{
> > +     enum net_attach_type attach_type;
> > +     int progfd, ifindex, err = 0;
> > +     bool overwrite = false;
> > +
> > +     /* parse attach args */
> > +     if (!REQ_ARGS(5))
> > +             return -EINVAL;
> > +
> > +     attach_type = parse_attach_type(*argv);
> > +     if (attach_type == max_net_attach_type) {
> > +             p_err("invalid net attach/detach type");
>
> worth adding the type to the error message so that user know which part
> of command line was wrong:
>
>         p_err("invalid net attach/detach type '%s'", *argv);
>

It sounds reasonable.
I'll update the error message.


> > +             return -EINVAL;
> > +     }
> > +
> > +     NEXT_ARG();
>
> nit: the new line should be before NEXT_ARG(), IOV NEXT_ARG() belongs
> to the code which consumed the argument
>

I'm not sure I'm following.
Are you saying that, at here the newline shouldn't be necessary?

> > +     progfd = prog_parse_fd(&argc, &argv);
> > +     if (progfd < 0)
> > +             return -EINVAL;
> > +
> > +     ifindex = net_parse_dev(&argc, &argv);
> > +     if (ifindex < 1) {
> > +             close(progfd);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (argc) {
> > +             if (is_prefix(*argv, "overwrite")) {
> > +                     overwrite = true;
> > +             } else {
> > +                     p_err("expected 'overwrite', got: '%s'?", *argv);
> > +                     close(progfd);
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     /* attach xdp prog */
> > +     if (is_prefix("xdp", attach_type_strings[attach_type]))
>
> I'm still unclear on why this if is needed
>

Just an code structure that shows extensibility for other attachment types.
Well, for now there's no other type than XDP, so it's not necessary.

> > +             err = do_attach_detach_xdp(progfd, attach_type, ifindex,
> > +                                        overwrite);
> > +
> > +     if (err < 0) {
> > +             p_err("interface %s attach failed",
> > +                   attach_type_strings[attach_type]);
>
> Please add the error string, like:
>
>                 p_err("interface %s attach failed: %s",
>                       attach_type_strings[attach_type], strerror(errno));
>
>

Oh. Didn't think of propagate errno to error message.
I'll update it right away.

> > +             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 = {};
> > @@ -231,17 +351,10 @@ static int do_show(int argc, char **argv)
> >       unsigned int nl_pid;
> >       char err_buf[256];
> >
> > -     if (argc == 2) {
> > -             if (strcmp(argv[0], "dev") != 0)
> > -                     usage();
> > -             filter_idx = if_nametoindex(argv[1]);
> > -             if (filter_idx == 0) {
> > -                     fprintf(stderr, "invalid dev name %s\n", argv[1]);
> > -                     return -1;
> > -             }
> > -     } else if (argc != 0) {
> > +     if (argc == 2)
> > +             filter_idx = net_parse_dev(&argc, &argv);
>
> You should check filter_idx is not negative here, no?
>

You're right.
I'll update it.

> > +     else if (argc != 0)
> >               usage();
> > -     }
> >
> >       ret = query_flow_dissector(&attach_info);
> >       if (ret)

Thank you for your assistance.

Re: [v3,3/4] tools: bpftool: add bash-completion for net attach/detach

[Quentin Monnet]

2019-08-07 11:25 UTC+0900 ~ Daniel T. Lee <danieltimlee@gmail.com>
> This commit adds bash-completion for new "net attach/detach"
> subcommand for attaching XDP program on interface.
> 
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
>  tools/bpf/bpftool/bash-completion/bpftool | 64 +++++++++++++++++++----
>  1 file changed, 55 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index c8f42e1fcbc9..1d81cb09d478 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -201,6 +201,10 @@ _bpftool()
>              _bpftool_get_prog_tags
>              return 0
>              ;;
> +        dev)
> +            _sysfs_get_netdevs
> +            return 0
> +            ;;

Makes sense to have this for "dev", thanks! But it seems you missed one
place where it was used, for "bpftool feature probe" (We have "[[ $prev
== "dev" ]] && _sysfs_get_netdevs && return 0"). Could you also remove
that one please?

Other than this looks good, thanks:

Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>

Re: [v3,4/4] tools: bpftool: add documentation for net attach/detach

[Quentin Monnet]

2019-08-07 11:25 UTC+0900 ~ Daniel T. Lee <danieltimlee@gmail.com>
> Since, new sub-command 'net attach/detach' has been added for
> attaching XDP program on interface,
> this commit documents usage and sample output of `net attach/detach`.
> 
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
>  .../bpf/bpftool/Documentation/bpftool-net.rst | 51 +++++++++++++++++--
>  1 file changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-net.rst b/tools/bpf/bpftool/Documentation/bpftool-net.rst
> index d8e5237a2085..4ad1a380e186 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-net.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-net.rst
> @@ -15,17 +15,22 @@ SYNOPSIS
>  	*OPTIONS* := { [{ **-j** | **--json** }] [{ **-p** | **--pretty** }] }
>  
>  	*COMMANDS* :=
> -	{ **show** | **list** } [ **dev** name ] | **help**
> +	{ **show** | **list** | **attach** | **detach** | **help** }
>  
>  NET COMMANDS
>  ============
>  
> -|	**bpftool** **net { show | list } [ dev name ]**
> +|	**bpftool** **net { show | list }** [ **dev** *name* ]
> +|	**bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *name* [ **overwrite** ]
> +|	**bpftool** **net detach** *ATTACH_TYPE* **dev** *name*

Nit: Could we have "name" in capital letters (everywhere in the file),
to make this file consistent with the formatting used for
bpftool-prog.rst and bpftool-map.rst?

>  |	**bpftool** **net help**
> +|
> +|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
> +|	*ATTACH_TYPE* := { **xdp** | **xdpgeneric** | **xdpdrv** | **xdpoffload** }
>  
>  DESCRIPTION
>  ===========
> -	**bpftool net { show | list } [ dev name ]**
> +	**bpftool net { show | list }** [ **dev** *name* ]
>                    List bpf program attachments in the kernel networking subsystem.
>  
>                    Currently, only device driver xdp attachments and tc filter
> @@ -47,6 +52,18 @@ DESCRIPTION
>                    all bpf programs attached to non clsact qdiscs, and finally all
>                    bpf programs attached to root and clsact qdisc.
>  
> +	**bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *name* [ **overwrite** ]
> +                  Attach bpf program *PROG* to network interface *name* with
> +                  type specified by *ATTACH_TYPE*. Previously attached bpf program
> +                  can be replaced by the command used with **overwrite** option.
> +                  Currently, *ATTACH_TYPE* only contains XDP programs.

Other nit: "ATTACH_TYPE only contains XDP programs" sounds odd to me.
Could we maybe phrase this something like: "Currently, only XDP-related
modes are supported for ATTACH_TYPE"?

Also, could you please provide a brief description of the different
attach types? In particular, explaining what "xdp" alone stands for
might be useful.

Thanks,
Quentin

> +
> +	**bpftool** **net detach** *ATTACH_TYPE* **dev** *name*
> +                  Detach bpf program attached to network interface *name* with
> +                  type specified by *ATTACH_TYPE*. To detach bpf program, same
> +                  *ATTACH_TYPE* previously used for attach must be specified.
> +                  Currently, *ATTACH_TYPE* only contains XDP programs.

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

[Jakub Kicinski]

On Thu, 8 Aug 2019 07:15:22 +0900, Daniel T. Lee wrote:
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     NEXT_ARG();  
> >
> > nit: the new line should be before NEXT_ARG(), IOV NEXT_ARG() belongs
> > to the code which consumed the argument
> >  
> 
> I'm not sure I'm following.
> Are you saying that, at here the newline shouldn't be necessary?

I mean this is better:

	if (!is_prefix(*argv, "bla-bla"))
		return -EINVAL;
	NEXT_ARG();

	if (!is_prefix(*argv, "bla-bla"))
		return -EINVAL;
	NEXT_ARG();

Than this:

	if (!is_prefix(*argv, "bla-bla"))
		return -EINVAL;

	NEXT_ARG();
	if (!is_prefix(*argv, "bla-bla"))
		return -EINVAL;

	NEXT_ARG();

Because the NEXT_ARG() "belongs" to the code that "consumed" the option.

So instead of this:

     attach_type = parse_attach_type(*argv);
     if (attach_type == max_net_attach_type) {
             p_err("invalid net attach/detach type");  
             return -EINVAL;
     }

     NEXT_ARG();  
     progfd = prog_parse_fd(&argc, &argv);
     if (progfd < 0)
             return -EINVAL;

This seems more logical to me:

     attach_type = parse_attach_type(*argv);
     if (attach_type == max_net_attach_type) {
             p_err("invalid net attach/detach type");  
             return -EINVAL;
     }
     NEXT_ARG();  

     progfd = prog_parse_fd(&argc, &argv);
     if (progfd < 0)
             return -EINVAL;

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

[Daniel T. Lee]

On Fri, Aug 9, 2019 at 2:50 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu, 8 Aug 2019 07:15:22 +0900, Daniel T. Lee wrote:
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     NEXT_ARG();
> > >
> > > nit: the new line should be before NEXT_ARG(), IOV NEXT_ARG() belongs
> > > to the code which consumed the argument
> > >
> >
> > I'm not sure I'm following.
> > Are you saying that, at here the newline shouldn't be necessary?
>
> I mean this is better:
>
>         if (!is_prefix(*argv, "bla-bla"))
>                 return -EINVAL;
>         NEXT_ARG();
>
>         if (!is_prefix(*argv, "bla-bla"))
>                 return -EINVAL;
>         NEXT_ARG();
>
> Than this:
>
>         if (!is_prefix(*argv, "bla-bla"))
>                 return -EINVAL;
>
>         NEXT_ARG();
>         if (!is_prefix(*argv, "bla-bla"))
>                 return -EINVAL;
>
>         NEXT_ARG();
>
> Because the NEXT_ARG() "belongs" to the code that "consumed" the option.
>
> So instead of this:
>
>      attach_type = parse_attach_type(*argv);
>      if (attach_type == max_net_attach_type) {
>              p_err("invalid net attach/detach type");
>              return -EINVAL;
>      }
>
>      NEXT_ARG();
>      progfd = prog_parse_fd(&argc, &argv);
>      if (progfd < 0)
>              return -EINVAL;
>
> This seems more logical to me:
>
>      attach_type = parse_attach_type(*argv);
>      if (attach_type == max_net_attach_type) {
>              p_err("invalid net attach/detach type");
>              return -EINVAL;
>      }
>      NEXT_ARG();
>
>      progfd = prog_parse_fd(&argc, &argv);
>      if (progfd < 0)
>              return -EINVAL;

Oh. I see.
I'll update NEXT_ARG line stick to the code which "consumes" the option.

Thanks for the review! :)

Re: [v3,3/4] tools: bpftool: add bash-completion for net attach/detach

[Daniel T. Lee]

On Fri, Aug 9, 2019 at 1:48 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> 2019-08-07 11:25 UTC+0900 ~ Daniel T. Lee <danieltimlee@gmail.com>
> > This commit adds bash-completion for new "net attach/detach"
> > subcommand for attaching XDP program on interface.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
> >  tools/bpf/bpftool/bash-completion/bpftool | 64 +++++++++++++++++++----
> >  1 file changed, 55 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> > index c8f42e1fcbc9..1d81cb09d478 100644
> > --- a/tools/bpf/bpftool/bash-completion/bpftool
> > +++ b/tools/bpf/bpftool/bash-completion/bpftool
> > @@ -201,6 +201,10 @@ _bpftool()
> >              _bpftool_get_prog_tags
> >              return 0
> >              ;;
> > +        dev)
> > +            _sysfs_get_netdevs
> > +            return 0
> > +            ;;
>
> Makes sense to have this for "dev", thanks! But it seems you missed one
> place where it was used, for "bpftool feature probe" (We have "[[ $prev
> == "dev" ]] && _sysfs_get_netdevs && return 0"). Could you also remove
> that one please?
>
> Other than this looks good, thanks:
>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>

My bad. Thanks for letting me know.
I'll update it with the next version of patch.

Thank you for your review.
I really appreciate it.

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

[Y Song]

On Wed, Aug 7, 2019 at 1:40 PM Maciej Fijalkowski
<maciejromanfijalkowski@gmail.com> wrote:
>
> On Wed, 7 Aug 2019 13:12:17 -0700
> Y Song <ys114321@gmail.com> wrote:
>
> > On Wed, Aug 7, 2019 at 11:30 AM Maciej Fijalkowski
> > <maciejromanfijalkowski@gmail.com> wrote:
> > >
> > > On Wed, 7 Aug 2019 10:02:04 -0700
> > > Y Song <ys114321@gmail.com> wrote:
> > >
> > > > On Tue, Aug 6, 2019 at 7:25 PM 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>
> > > > > ---
> > > > >  tools/bpf/bpftool/net.c | 42 ++++++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> > > > > index c05a3fac5cac..7be96acb08e0 100644
> > > > > --- a/tools/bpf/bpftool/net.c
> > > > > +++ b/tools/bpf/bpftool/net.c
> > > > > @@ -343,6 +343,43 @@ 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 progfd, ifindex, err = 0;
> > > > > +
> > > > > +       /* parse detach args */
> > > > > +       if (!REQ_ARGS(3))
> > > > > +               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();
> > > > > +       ifindex = net_parse_dev(&argc, &argv);
> > > > > +       if (ifindex < 1)
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       /* detach xdp prog */
> > > > > +       progfd = -1;
> > > > > +       if (is_prefix("xdp", attach_type_strings[attach_type]))
> > > > > +               err = do_attach_detach_xdp(progfd, attach_type, ifindex, NULL);
> > > >
> > > > I found an issue here. This is probably related to do_attach_detach_xdp.
> > > >
> > > > -bash-4.4$ sudo ./bpftool net attach x pinned /sys/fs/bpf/xdp_example dev v1
> > > > -bash-4.4$ sudo ./bpftool net
> > > > xdp:
> > > > v1(4) driver id 1172
> > > >
> > > > tc:
> > > > eth0(2) clsact/ingress fbflow_icmp id 29 act []
> > > > eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> > > > eth0(2) clsact/egress fbflow_egress id 28
> > > > eth0(2) clsact/egress fbflow_sslwall_egress id 35
> > > >
> > > > flow_dissector:
> > > >
> > > > -bash-4.4$ sudo ./bpftool net detach x dev v2
> > >
> > > Shouldn't this be v1 as dev?
> >
> > I am testing a scenario where with wrong devname
> > we did not return an error.
>
> Ah ok. In this scenario if driver has a native xdp support we would be invoking
> its ndo_bpf even if there's no prog currently attached and it wouldn't return
> error value.
>
> Looking at dev_xdp_uninstall, setting driver's prog to NULL is being done only
> when prog is attached. Maybe we should consider querying the driver in
> dev_change_xdp_fd regardless of passed fd value? E.g. don't query only when
> prog >= 0.
>
> I don't recall whether this was brought up previously.

Thanks for explanation. I think return an error is better in
such error cases. Otherwise, people mistakenly write wrong
device name and they may think xdp is detached and it is
actually not.

But this probably should not prevent
this patch as it is more like a kernel issue.

>
> CCing Jakub so we have one thread.
>
> Maciej
>
> >
> > Yes, if dev "v1", it works as expected.
> >
> > >
> > > > -bash-4.4$ sudo ./bpftool net
> > > > xdp:
> > > > v1(4) driver id 1172
> > > >
> > > > tc:
> > > > eth0(2) clsact/ingress fbflow_icmp id 29 act []
> > > > eth0(2) clsact/egress cls_fg_dscp_section id 27 act []
> > > > eth0(2) clsact/egress fbflow_egress id 28
> > > > eth0(2) clsact/egress fbflow_sslwall_egress id 35
> > > >
> > > > flow_dissector:
> > > >
> > > > -bash-4.4$
> > > >
> > > > Basically detaching may fail due to wrong dev name or wrong type, etc.
> > > > But the tool did not return an error. Is this expected?
> > > > This may be related to this funciton "bpf_set_link_xdp_fd()".
> > > > So this patch itself should be okay.
> > > >
> > > > > +
> > > > > +       if (err < 0) {
> > > > > +               p_err("interface %s detach failed",
> > > > > +                     attach_type_strings[attach_type]);
> > > > > +               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 = {};
> > > > > @@ -419,6 +456,7 @@ static int do_help(int argc, char **argv)
> > > > >         fprintf(stderr,
> > > > >                 "Usage: %s %s { show | list } [dev <devname>]\n"
> > > > >                 "       %s %s attach ATTACH_TYPE PROG dev <devname> [ overwrite ]\n"
> > > > > +               "       %s %s detach ATTACH_TYPE dev <devname>\n"
> > > > >                 "       %s %s help\n"
> > > > >                 "\n"
> > > > >                 "       " HELP_SPEC_PROGRAM "\n"
> > > > > @@ -429,7 +467,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;
> > > > >  }
> > > > > @@ -438,6 +477,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: [v3,4/4] tools: bpftool: add documentation for net attach/detach

[Daniel T. Lee]

On Fri, Aug 9, 2019 at 1:48 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> 2019-08-07 11:25 UTC+0900 ~ Daniel T. Lee <danieltimlee@gmail.com>
> > Since, new sub-command 'net attach/detach' has been added for
> > attaching XDP program on interface,
> > this commit documents usage and sample output of `net attach/detach`.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
> >  .../bpf/bpftool/Documentation/bpftool-net.rst | 51 +++++++++++++++++--
> >  1 file changed, 48 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-net.rst b/tools/bpf/bpftool/Documentation/bpftool-net.rst
> > index d8e5237a2085..4ad1a380e186 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-net.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-net.rst
> > @@ -15,17 +15,22 @@ SYNOPSIS
> >       *OPTIONS* := { [{ **-j** | **--json** }] [{ **-p** | **--pretty** }] }
> >
> >       *COMMANDS* :=
> > -     { **show** | **list** } [ **dev** name ] | **help**
> > +     { **show** | **list** | **attach** | **detach** | **help** }
> >
> >  NET COMMANDS
> >  ============
> >
> > -|    **bpftool** **net { show | list } [ dev name ]**
> > +|    **bpftool** **net { show | list }** [ **dev** *name* ]
> > +|    **bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *name* [ **overwrite** ]
> > +|    **bpftool** **net detach** *ATTACH_TYPE* **dev** *name*
>
> Nit: Could we have "name" in capital letters (everywhere in the file),
> to make this file consistent with the formatting used for
> bpftool-prog.rst and bpftool-map.rst?
>

I'll update all "name" with capital "NAME" at next version of patch.

> >  |    **bpftool** **net help**
> > +|
> > +|    *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
> > +|    *ATTACH_TYPE* := { **xdp** | **xdpgeneric** | **xdpdrv** | **xdpoffload** }
> >
> >  DESCRIPTION
> >  ===========
> > -     **bpftool net { show | list } [ dev name ]**
> > +     **bpftool net { show | list }** [ **dev** *name* ]
> >                    List bpf program attachments in the kernel networking subsystem.
> >
> >                    Currently, only device driver xdp attachments and tc filter
> > @@ -47,6 +52,18 @@ DESCRIPTION
> >                    all bpf programs attached to non clsact qdiscs, and finally all
> >                    bpf programs attached to root and clsact qdisc.
> >
> > +     **bpftool** **net attach** *ATTACH_TYPE* *PROG* **dev** *name* [ **overwrite** ]
> > +                  Attach bpf program *PROG* to network interface *name* with
> > +                  type specified by *ATTACH_TYPE*. Previously attached bpf program
> > +                  can be replaced by the command used with **overwrite** option.
> > +                  Currently, *ATTACH_TYPE* only contains XDP programs.
>
> Other nit: "ATTACH_TYPE only contains XDP programs" sounds odd to me.
> Could we maybe phrase this something like: "Currently, only XDP-related
> modes are supported for ATTACH_TYPE"?
>
> Also, could you please provide a brief description of the different
> attach types? In particular, explaining what "xdp" alone stands for
> might be useful.
>

I'll replace the phrase and add brief description about the attach types.

> Thanks,
> Quentin
>
> > +
> > +     **bpftool** **net detach** *ATTACH_TYPE* **dev** *name*
> > +                  Detach bpf program attached to network interface *name* with
> > +                  type specified by *ATTACH_TYPE*. To detach bpf program, same
> > +                  *ATTACH_TYPE* previously used for attach must be specified.
> > +                  Currently, *ATTACH_TYPE* only contains XDP programs.

Thank you for taking your time for the review.

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

[Jakub Kicinski]

On Thu, 8 Aug 2019 12:52:11 -0700, Y Song wrote:
> > Ah ok. In this scenario if driver has a native xdp support we would be invoking
> > its ndo_bpf even if there's no prog currently attached and it wouldn't return
> > error value.
> >
> > Looking at dev_xdp_uninstall, setting driver's prog to NULL is being done only
> > when prog is attached. Maybe we should consider querying the driver in
> > dev_change_xdp_fd regardless of passed fd value? E.g. don't query only when
> > prog >= 0.
> >
> > I don't recall whether this was brought up previously.  
> 
> Thanks for explanation. I think return an error is better in
> such error cases. Otherwise, people mistakenly write wrong
> device name and they may think xdp is detached and it is
> actually not.
> 
> But this probably should not prevent
> this patch as it is more like a kernel issue.

Agreed, we'd probably need a flag here, similar to IF_NOEXIST to keep
backward compat.