[PATCH bpf-next 0/3] bpf: add dumping and disassembler for non-host JITs

[PATCH bpf-next 0/3] bpf: add dumping and disassembler for non-host JITs

[Jakub Kicinski]

Hi,

Currently bpftool could disassemble host jited image, for example x86_64,
using libbfd. However it couldn't disassemble offload jited image.

There are two reasons:

  1. bpf_obj_get_info_by_fd/struct bpf_prog_info couldn't get the address
     of jited image and image's length.

  2. Even after issue 1 resolved, bpftool couldn't figure out what is the
     offload arch from bpf_prog_info, therefore can't drive libbfd
     disassembler correctly.

  This patch set resolve issue 1 by introducing two new fields "jited_len"
and "jited_image" in bpf_dev_offload. These two fields serve as the generic
interface to communicate the jited image address and length for all offload
targets to higher level caller. For example, bpf_obj_get_info_by_fd could
use them to fill the userspace visible fields jited_prog_len and
jited_prog_insns.

  This patch set resolve issue 2 by getting bfd backend name through
"ifindex", i.e network interface index.

v1:
 - Deduct bfd arch name through ifindex, i.e network interface index.
   First, map ifindex to devname through ifindex_to_name_ns, then get
   pci id through /sys/class/dev/DEVNAME/device/vendor. (Daniel, Alexei)
 
Jiong Wang (3):
  bpf: add new jited info fields in bpf_dev_offload and bpf_prog_info
  nfp: bpf: set new jit info fields
  tools: bpftool: improve architecture detection by using ifindex

 drivers/net/ethernet/netronome/nfp/bpf/offload.c | 10 +++-
 include/linux/bpf.h                              |  2 +
 kernel/bpf/offload.c                             | 23 ++++++++
 kernel/bpf/syscall.c                             | 31 +++++-----
 tools/bpf/bpftool/common.c                       | 72 ++++++++++++++++++++++++
 tools/bpf/bpftool/jit_disasm.c                   | 16 +++++-
 tools/bpf/bpftool/main.h                         |  5 +-
 tools/bpf/bpftool/prog.c                         | 12 +++-
 8 files changed, 154 insertions(+), 17 deletions(-)

-- 
2.15.1

[PATCH bpf-next 1/3] bpf: add new jited info fields in bpf_dev_offload and bpf_prog_info

[Jakub Kicinski]

From: Jiong Wang <jiong.wang@netronome.com>

For host JIT, there are "jited_len"/"bpf_func" fields in struct bpf_prog
used by all host JIT targets to get jited image and it's length. While for
offload, targets are likely to have different offload mechanisms that these
info are kept in device private data fields.

Therefore, BPF_OBJ_GET_INFO_BY_FD syscall needs an unified way to get JIT
length and contents info for offload targets.

One way is to introduce new callback to parse device private data then fill
those fields in bpf_prog_info. This might be a little heavy, the other way
is to add generic fields which will be initialized by all offload targets.

This patch follow the second approach to introduce two new fields in
struct bpf_dev_offload and teach bpf_prog_get_info_by_fd about them to fill
correct jited_prog_len and jited_prog_insns in bpf_prog_info.

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
---
 include/linux/bpf.h  |  2 ++
 kernel/bpf/offload.c | 23 +++++++++++++++++++++++
 kernel/bpf/syscall.c | 31 ++++++++++++++++++-------------
 3 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5c2c104dc2c5..025b1c2f8053 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -234,6 +234,8 @@ struct bpf_prog_offload {
 	struct list_head	offloads;
 	bool			dev_state;
 	const struct bpf_prog_offload_ops *dev_ops;
+	void			*jited_image;
+	u32			jited_len;
 };
 
 struct bpf_prog_aux {
diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index a88cebf368bf..6c0baa1cf8f8 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -230,9 +230,12 @@ int bpf_prog_offload_info_fill(struct bpf_prog_info *info,
 		.prog	= prog,
 		.info	= info,
 	};
+	struct bpf_prog_aux *aux = prog->aux;
 	struct inode *ns_inode;
 	struct path ns_path;
+	char __user *uinsns;
 	void *res;
+	u32 ulen;
 
 	res = ns_get_path_cb(&ns_path, bpf_prog_offload_info_fill_ns, &args);
 	if (IS_ERR(res)) {
@@ -241,6 +244,26 @@ int bpf_prog_offload_info_fill(struct bpf_prog_info *info,
 		return PTR_ERR(res);
 	}
 
+	down_read(&bpf_devs_lock);
+
+	if (!aux->offload) {
+		up_read(&bpf_devs_lock);
+		return -ENODEV;
+	}
+
+	ulen = info->jited_prog_len;
+	info->jited_prog_len = aux->offload->jited_len;
+	if (info->jited_prog_len & ulen) {
+		uinsns = u64_to_user_ptr(info->jited_prog_insns);
+		ulen = min_t(u32, info->jited_prog_len, ulen);
+		if (copy_to_user(uinsns, aux->offload->jited_image, ulen)) {
+			up_read(&bpf_devs_lock);
+			return -EFAULT;
+		}
+	}
+
+	up_read(&bpf_devs_lock);
+
 	ns_inode = ns_path.dentry->d_inode;
 	info->netns_dev = new_encode_dev(ns_inode->i_sb->s_dev);
 	info->netns_ino = ns_inode->i_ino;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c691b9e972e3..c28524483bf4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1724,19 +1724,6 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
 		goto done;
 	}
 
-	ulen = info.jited_prog_len;
-	info.jited_prog_len = prog->jited_len;
-	if (info.jited_prog_len && ulen) {
-		if (bpf_dump_raw_ok()) {
-			uinsns = u64_to_user_ptr(info.jited_prog_insns);
-			ulen = min_t(u32, info.jited_prog_len, ulen);
-			if (copy_to_user(uinsns, prog->bpf_func, ulen))
-				return -EFAULT;
-		} else {
-			info.jited_prog_insns = 0;
-		}
-	}
-
 	ulen = info.xlated_prog_len;
 	info.xlated_prog_len = bpf_prog_insn_size(prog);
 	if (info.xlated_prog_len && ulen) {
@@ -1762,6 +1749,24 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
 		err = bpf_prog_offload_info_fill(&info, prog);
 		if (err)
 			return err;
+		goto done;
+	}
+
+	/* NOTE: the following code is supposed to be skipped for offload.
+	 * bpf_prog_offload_info_fill() is the place to fill similar fields
+	 * for offload.
+	 */
+	ulen = info.jited_prog_len;
+	info.jited_prog_len = prog->jited_len;
+	if (info.jited_prog_len && ulen) {
+		if (bpf_dump_raw_ok()) {
+			uinsns = u64_to_user_ptr(info.jited_prog_insns);
+			ulen = min_t(u32, info.jited_prog_len, ulen);
+			if (copy_to_user(uinsns, prog->bpf_func, ulen))
+				return -EFAULT;
+		} else {
+			info.jited_prog_insns = 0;
+		}
 	}
 
 done:
-- 
2.15.1

[PATCH bpf-next 2/3] nfp: bpf: set new jit info fields

[Jakub Kicinski]

From: Jiong Wang <jiong.wang@netronome.com>

This patch set those new jit info fields introduced in this patch set.

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/offload.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index 9c78a09cda24..4c1cea68f19e 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -127,6 +127,7 @@ static int nfp_bpf_translate(struct nfp_net *nn, struct bpf_prog *prog)
 	struct nfp_prog *nfp_prog = prog->aux->offload->dev_priv;
 	unsigned int stack_size;
 	unsigned int max_instr;
+	int err;
 
 	stack_size = nn_readb(nn, NFP_NET_CFG_BPF_STACK_SZ) * 64;
 	if (prog->aux->stack_depth > stack_size) {
@@ -143,7 +144,14 @@ static int nfp_bpf_translate(struct nfp_net *nn, struct bpf_prog *prog)
 	if (!nfp_prog->prog)
 		return -ENOMEM;
 
-	return nfp_bpf_jit(nfp_prog);
+	err = nfp_bpf_jit(nfp_prog);
+	if (err)
+		return err;
+
+	prog->aux->offload->jited_len = nfp_prog->prog_len * sizeof(u64);
+	prog->aux->offload->jited_image = nfp_prog->prog;
+
+	return 0;
 }
 
 static int nfp_bpf_destroy(struct nfp_net *nn, struct bpf_prog *prog)
-- 
2.15.1

[PATCH bpf-next 3/3] tools: bpftool: improve architecture detection by using ifindex

[Jakub Kicinski]

From: Jiong Wang <jiong.wang@netronome.com>

The current architecture detection method in bpftool is designed for host
case.

For offload case, we can't use the architecture of "bpftool" itself.
Instead, we could call the existing "ifindex_to_name_ns" to get DEVNAME,
then read pci id from /sys/class/dev/DEVNAME/device/vendor, finally we map
vendor id to bfd arch name which will finally be used to select bfd backend
for the disassembler.

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
---
 tools/bpf/bpftool/common.c     | 72 ++++++++++++++++++++++++++++++++++++++++++
 tools/bpf/bpftool/jit_disasm.c | 16 +++++++++-
 tools/bpf/bpftool/main.h       |  5 ++-
 tools/bpf/bpftool/prog.c       | 12 ++++++-
 4 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 6601c95a9258..0b482c0070e0 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -34,6 +34,7 @@
 /* Author: Jakub Kicinski <kubakici@wp.pl> */
 
 #include <errno.h>
+#include <fcntl.h>
 #include <fts.h>
 #include <libgen.h>
 #include <mntent.h>
@@ -433,6 +434,77 @@ ifindex_to_name_ns(__u32 ifindex, __u32 ns_dev, __u32 ns_ino, char *buf)
 	return if_indextoname(ifindex, buf);
 }
 
+static int read_sysfs_hex_int(char *path)
+{
+	char vendor_id_buf[8];
+	int len;
+	int fd;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0) {
+		p_err("Can't open %s: %s", path, strerror(errno));
+		return -1;
+	}
+
+	len = read(fd, vendor_id_buf, sizeof(vendor_id_buf));
+	close(fd);
+	if (len < 0) {
+		p_err("Can't read %s: %s", path, strerror(errno));
+		return -1;
+	}
+	if (len >= (int)sizeof(vendor_id_buf)) {
+		p_err("Value in %s too long", path);
+		return -1;
+	}
+
+	vendor_id_buf[len] = 0;
+
+	return strtol(vendor_id_buf, NULL, 0);
+}
+
+static int read_sysfs_netdev_hex_int(char *devname, const char *entry_name)
+{
+	char full_path[64];
+
+	snprintf(full_path, sizeof(full_path), "/sys/class/net/%s/device/%s",
+		 devname, entry_name);
+
+	return read_sysfs_hex_int(full_path);
+}
+
+const char *ifindex_to_bfd_name_ns(__u32 ifindex, __u64 ns_dev, __u64 ns_ino)
+{
+	char devname[IF_NAMESIZE];
+	int vendor_id;
+	int device_id;
+
+	if (!ifindex_to_name_ns(ifindex, ns_dev, ns_ino, devname)) {
+		p_err("Can't get net device name for ifindex %d: %s", ifindex,
+		      strerror(errno));
+		return NULL;
+	}
+
+	vendor_id = read_sysfs_netdev_hex_int(devname, "vendor");
+	if (vendor_id < 0) {
+		p_err("Can't get device vendor id for %s", devname);
+		return NULL;
+	}
+
+	switch (vendor_id) {
+	case 0x19ee:
+		device_id = read_sysfs_netdev_hex_int(devname, "device");
+		if (device_id != 0x4000 &&
+		    device_id != 0x6000 &&
+		    device_id != 0x6003)
+			p_info("Unknown NFP device ID, assuming it is NFP-6xxx arch");
+		return "NFP-6xxx";
+	default:
+		p_err("Can't get bfd arch name for device vendor id 0x%04x",
+		      vendor_id);
+		return NULL;
+	}
+}
+
 void print_dev_plain(__u32 ifindex, __u64 ns_dev, __u64 ns_inode)
 {
 	char name[IF_NAMESIZE];
diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c
index 57d32e8a1391..87439320ef70 100644
--- a/tools/bpf/bpftool/jit_disasm.c
+++ b/tools/bpf/bpftool/jit_disasm.c
@@ -76,7 +76,8 @@ static int fprintf_json(void *out, const char *fmt, ...)
 	return 0;
 }
 
-void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes)
+void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes,
+		       const char *arch)
 {
 	disassembler_ftype disassemble;
 	struct disassemble_info info;
@@ -100,6 +101,19 @@ void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes)
 	else
 		init_disassemble_info(&info, stdout,
 				      (fprintf_ftype) fprintf);
+
+	/* Update architecture info for offload. */
+	if (arch) {
+		const bfd_arch_info_type *inf = bfd_scan_arch(arch);
+
+		if (inf) {
+			bfdf->arch_info = inf;
+		} else {
+			p_err("No libfd support for %s", arch);
+			return;
+		}
+	}
+
 	info.arch = bfd_get_arch(bfdf);
 	info.mach = bfd_get_mach(bfdf);
 	info.buffer = image;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 65b526fe6e7e..b8e9584d6246 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -121,7 +121,10 @@ int do_cgroup(int argc, char **arg);
 
 int prog_parse_fd(int *argc, char ***argv);
 
-void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes);
+void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes,
+		       const char *arch);
 void print_hex_data_json(uint8_t *data, size_t len);
 
+const char *ifindex_to_bfd_name_ns(__u32 ifindex, __u64 ns_dev, __u64 ns_ino);
+
 #endif
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 099e21cf1b5c..e8e2baaf93c2 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -776,7 +776,17 @@ static int do_dump(int argc, char **argv)
 		}
 	} else {
 		if (member_len == &info.jited_prog_len) {
-			disasm_print_insn(buf, *member_len, opcodes);
+			const char *name = NULL;
+
+			if (info.ifindex) {
+				name = ifindex_to_bfd_name_ns(info.ifindex,
+							      info.netns_dev,
+							      info.netns_ino);
+				if (!name)
+					goto err_free;
+			}
+
+			disasm_print_insn(buf, *member_len, opcodes, name);
 		} else {
 			kernel_syms_load(&dd);
 			if (json_output)
-- 
2.15.1

Re: [PATCH bpf-next 0/3] bpf: add dumping and disassembler for non-host JITs

[Jakub Kicinski]

On Tue, 16 Jan 2018 16:05:18 -0800, Jakub Kicinski wrote:
> Hi,

Ah, forgot to insert "Jiong says:" here :)

> Currently bpftool could disassemble host jited image, for example x86_64,
> using libbfd. However it couldn't disassemble offload jited image.
> 
> There are two reasons:
> 
>   1. bpf_obj_get_info_by_fd/struct bpf_prog_info couldn't get the address
>      of jited image and image's length.
> 
>   2. Even after issue 1 resolved, bpftool couldn't figure out what is the
>      offload arch from bpf_prog_info, therefore can't drive libbfd
>      disassembler correctly.
> 
>   This patch set resolve issue 1 by introducing two new fields "jited_len"
> and "jited_image" in bpf_dev_offload. These two fields serve as the generic
> interface to communicate the jited image address and length for all offload
> targets to higher level caller. For example, bpf_obj_get_info_by_fd could
> use them to fill the userspace visible fields jited_prog_len and
> jited_prog_insns.
> 
>   This patch set resolve issue 2 by getting bfd backend name through
> "ifindex", i.e network interface index.
> 
> v1:
>  - Deduct bfd arch name through ifindex, i.e network interface index.
>    First, map ifindex to devname through ifindex_to_name_ns, then get
>    pci id through /sys/class/dev/DEVNAME/device/vendor. (Daniel, Alexei)

Re: [PATCH bpf-next 1/3] bpf: add new jited info fields in bpf_dev_offload and bpf_prog_info

[Alexei Starovoitov]

On Tue, Jan 16, 2018 at 04:05:19PM -0800, Jakub Kicinski wrote:
> From: Jiong Wang <jiong.wang@netronome.com>
> 
> For host JIT, there are "jited_len"/"bpf_func" fields in struct bpf_prog
> used by all host JIT targets to get jited image and it's length. While for
> offload, targets are likely to have different offload mechanisms that these
> info are kept in device private data fields.
> 
> Therefore, BPF_OBJ_GET_INFO_BY_FD syscall needs an unified way to get JIT
> length and contents info for offload targets.
> 
> One way is to introduce new callback to parse device private data then fill
> those fields in bpf_prog_info. This might be a little heavy, the other way
> is to add generic fields which will be initialized by all offload targets.
> 
> This patch follow the second approach to introduce two new fields in
> struct bpf_dev_offload and teach bpf_prog_get_info_by_fd about them to fill
> correct jited_prog_len and jited_prog_insns in bpf_prog_info.
> 
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>

Acked-by: Alexei Starovoitov <ast@kernel.org>

initially I wasn't sure that reusing jited_prog_insns field
to return offloaded prog is such a good idea, but since we
fill in ifindex at the same time the usage of the field is not ambiguous,
so I think it's a good approach.

Re: [PATCH bpf-next 3/3] tools: bpftool: improve architecture detection by using ifindex

[Alexei Starovoitov]

On Tue, Jan 16, 2018 at 04:05:21PM -0800, Jakub Kicinski wrote:
> From: Jiong Wang <jiong.wang@netronome.com>
> 
> The current architecture detection method in bpftool is designed for host
> case.
> 
> For offload case, we can't use the architecture of "bpftool" itself.
> Instead, we could call the existing "ifindex_to_name_ns" to get DEVNAME,
> then read pci id from /sys/class/dev/DEVNAME/device/vendor, finally we map
> vendor id to bfd arch name which will finally be used to select bfd backend
> for the disassembler.
> 
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>

awesome addition!
Acked-by: Alexei Starovoitov <ast@kernel.org>

> +	switch (vendor_id) {
> +	case 0x19ee:
> +		device_id = read_sysfs_netdev_hex_int(devname, "device");
> +		if (device_id != 0x4000 &&
> +		    device_id != 0x6000 &&
> +		    device_id != 0x6003)
> +			p_info("Unknown NFP device ID, assuming it is NFP-6xxx arch");
> +		return "NFP-6xxx";

is this a canonical name that bfd will understand?
a link to bfd patches?

Re: [PATCH bpf-next 3/3] tools: bpftool: improve architecture detection by using ifindex

[Jakub Kicinski]

CC: Francois

On Tue, 16 Jan 2018 19:42:15 -0800, Alexei Starovoitov wrote:
> > +	switch (vendor_id) {
> > +	case 0x19ee:
> > +		device_id = read_sysfs_netdev_hex_int(devname, "device");
> > +		if (device_id != 0x4000 &&
> > +		    device_id != 0x6000 &&
> > +		    device_id != 0x6003)
> > +			p_info("Unknown NFP device ID, assuming it is NFP-6xxx arch");
> > +		return "NFP-6xxx";  
> 
> is this a canonical name that bfd will understand?

Yes, we started with just "nfp", but tossed "6k" for good measure.

> a link to bfd patches?

Unfortunately not posted, yet.

Re: [PATCH bpf-next 0/3] bpf: add dumping and disassembler for non-host JITs

[Daniel Borkmann]

On 01/17/2018 01:05 AM, Jakub Kicinski wrote:
> Hi,
> 
> Currently bpftool could disassemble host jited image, for example x86_64,
> using libbfd. However it couldn't disassemble offload jited image.
> 
> There are two reasons:
> 
>   1. bpf_obj_get_info_by_fd/struct bpf_prog_info couldn't get the address
>      of jited image and image's length.
> 
>   2. Even after issue 1 resolved, bpftool couldn't figure out what is the
>      offload arch from bpf_prog_info, therefore can't drive libbfd
>      disassembler correctly.
> 
>   This patch set resolve issue 1 by introducing two new fields "jited_len"
> and "jited_image" in bpf_dev_offload. These two fields serve as the generic
> interface to communicate the jited image address and length for all offload
> targets to higher level caller. For example, bpf_obj_get_info_by_fd could
> use them to fill the userspace visible fields jited_prog_len and
> jited_prog_insns.
> 
>   This patch set resolve issue 2 by getting bfd backend name through
> "ifindex", i.e network interface index.
> 
> v1:
>  - Deduct bfd arch name through ifindex, i.e network interface index.
>    First, map ifindex to devname through ifindex_to_name_ns, then get
>    pci id through /sys/class/dev/DEVNAME/device/vendor. (Daniel, Alexei)

Looks good, series applied to bpf-next, thanks guys!