[PATCHv3 0/9] bpf: Add d_path helper

[PATCHv3 0/9] bpf: Add d_path helper

[Jiri Olsa]

hi,
adding d_path helper to return full path for 'path' object.

I originally added and used 'file_path' helper, which did the same,
but used 'struct file' object. Then realized that file_path is just
a wrapper for d_path, so we'd cover more calling sites if we add
d_path helper and allowed resolving BTF object within another object,
so we could call d_path also with file pointer, like:

  bpf_d_path(&file->f_path, buf, size);

This feature is mainly to be able to add dpath (filepath originally)
function to bpftrace:

  # bpftrace -e 'kfunc:vfs_open { printf("%s\n", dpath(args->path)); }'

v3 changes:
  - changed tests to use seleton and vmlinux.h [Andrii]
  - refactored to define ID lists in C object [Andrii]
  - changed btf_struct_access for nested ID check,
    instead of adding new function for that [Andrii]
  - fail build with CONFIG_DEBUG_INFO_BTF if libelf is not detected [Andrii]

Also available at:
  https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  bpf/d_path

thanks,
jirka


---
Jiri Olsa (11):
      bpf: Add btfid tool to resolve BTF IDs in ELF object
      bpf: Compile btfid tool at kernel compilation start
      bpf: Add btf_ids object
      bpf: Resolve BTF IDs in vmlinux image
      bpf: Remove btf_id helpers resolving
      bpf: Do not pass enum bpf_access_type to btf_struct_access
      bpf: Allow nested BTF object to be refferenced by BTF object + offset
      bpf: Add BTF whitelist support
      bpf: Add d_path helper
      selftests/bpf: Add verifier test for d_path helper
      selftests/bpf: Add test for d_path helper

 Makefile                                        |  25 ++++-
 include/asm-generic/vmlinux.lds.h               |   4 +
 include/linux/bpf.h                             |  16 ++-
 include/uapi/linux/bpf.h                        |  14 ++-
 kernel/bpf/Makefile                             |   2 +-
 kernel/bpf/btf.c                                | 149 +++++++++++--------------
 kernel/bpf/btf_ids.c                            |  26 +++++
 kernel/bpf/btf_ids.h                            | 108 ++++++++++++++++++
 kernel/bpf/verifier.c                           |  39 +++++--
 kernel/trace/bpf_trace.c                        |  40 ++++++-
 net/core/filter.c                               |   2 -
 net/ipv4/bpf_tcp_ca.c                           |   2 +-
 scripts/bpf_helpers_doc.py                      |   2 +
 scripts/link-vmlinux.sh                         |   6 +
 tools/Makefile                                  |   3 +
 tools/bpf/Makefile                              |   5 +-
 tools/bpf/btfid/Build                           |  26 +++++
 tools/bpf/btfid/Makefile                        |  71 ++++++++++++
 tools/bpf/btfid/btfid.c                         | 627 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h                  |  14 ++-
 tools/testing/selftests/bpf/prog_tests/d_path.c | 153 +++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/test_d_path.c |  55 +++++++++
 tools/testing/selftests/bpf/test_verifier.c     |  13 ++-
 tools/testing/selftests/bpf/verifier/d_path.c   |  38 +++++++
 24 files changed, 1329 insertions(+), 111 deletions(-)
 create mode 100644 kernel/bpf/btf_ids.c
 create mode 100644 kernel/bpf/btf_ids.h
 create mode 100644 tools/bpf/btfid/Build
 create mode 100644 tools/bpf/btfid/Makefile
 create mode 100644 tools/bpf/btfid/btfid.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_d_path.c
 create mode 100644 tools/testing/selftests/bpf/verifier/d_path.c

[PATCH 01/11] bpf: Add btfid tool to resolve BTF IDs in ELF object

[Jiri Olsa]

The btfid tool scans Elf object for .BTF_ids section and
resolves its symbols with BTF IDs.

It will be used to during linking time to resolve arrays
of BTF IDs used in verifier, so these IDs do not need to
be resolved in runtime.

The expected layout of .BTF_ids section is described
in btfid.c header. Related kernel changes are coming in
following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/bpf/btfid/Build    |  26 ++
 tools/bpf/btfid/Makefile |  71 +++++
 tools/bpf/btfid/btfid.c  | 627 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 724 insertions(+)
 create mode 100644 tools/bpf/btfid/Build
 create mode 100644 tools/bpf/btfid/Makefile
 create mode 100644 tools/bpf/btfid/btfid.c

diff --git a/tools/bpf/btfid/Build b/tools/bpf/btfid/Build
new file mode 100644
index 000000000000..12d43396d2a0
--- /dev/null
+++ b/tools/bpf/btfid/Build
@@ -0,0 +1,26 @@
+btfid-y += btfid.o
+btfid-y += rbtree.o
+btfid-y += zalloc.o
+btfid-y += string.o
+btfid-y += ctype.o
+btfid-y += str_error_r.o
+
+$(OUTPUT)rbtree.o: ../../lib/rbtree.c FORCE
+	$(call rule_mkdir)
+	$(call if_changed_dep,cc_o_c)
+
+$(OUTPUT)zalloc.o: ../../lib/zalloc.c FORCE
+	$(call rule_mkdir)
+	$(call if_changed_dep,cc_o_c)
+
+$(OUTPUT)string.o: ../../lib/string.c FORCE
+	$(call rule_mkdir)
+	$(call if_changed_dep,cc_o_c)
+
+$(OUTPUT)ctype.o: ../../lib/ctype.c FORCE
+	$(call rule_mkdir)
+	$(call if_changed_dep,cc_o_c)
+
+$(OUTPUT)str_error_r.o: ../../lib/str_error_r.c FORCE
+	$(call rule_mkdir)
+	$(call if_changed_dep,cc_o_c)
diff --git a/tools/bpf/btfid/Makefile b/tools/bpf/btfid/Makefile
new file mode 100644
index 000000000000..30b721cf0a21
--- /dev/null
+++ b/tools/bpf/btfid/Makefile
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: GPL-2.0-only
+include ../../scripts/Makefile.include
+
+MAKEFLAGS=--no-print-directory
+
+ifeq ($(srctree),)
+srctree := $(patsubst %/,%,$(dir $(CURDIR)))
+srctree := $(patsubst %/,%,$(dir $(srctree)))
+srctree := $(patsubst %/,%,$(dir $(srctree)))
+endif
+
+ifeq ($(V),1)
+  Q =
+else
+  Q = @
+endif
+
+BPF_DIR       = $(srctree)/tools/lib/bpf/
+SUBCMD_DIR    = $(srctree)/tools/lib/subcmd/
+SUBCMD_OUTPUT = $(if $(OUTPUT),$(OUTPUT),$(CURDIR)/)
+
+ifneq ($(OUTPUT),)
+  LIBBPF_PATH = $(OUTPUT)/libbpf/
+  SUBCMD_PATH = $(OUTPUT)/subcmd/
+else
+  LIBBPF_PATH = $(BPF_DIR)
+  SUBCMD_PATH = $(SUBCMD_DIR)
+endif
+
+LIBSUBCMD = $(SUBCMD_OUTPUT)libsubcmd.a
+LIBBPF    = $(LIBBPF_PATH)libbpf.a
+BPFWL     = $(OUTPUT)btfid
+BPFWL_IN  = $(BPFWL)-in.o
+
+all: $(OUTPUT)btfid
+
+$(LIBSUBCMD): fixdep FORCE
+	$(Q)$(MAKE) -C $(SUBCMD_DIR) OUTPUT=$(SUBCMD_OUTPUT)
+
+$(LIBSUBCMD)-clean:
+	$(Q)$(MAKE) -C $(SUBCMD_DIR) O=$(OUTPUT) clean
+
+$(LIBBPF): FORCE
+	$(if $(LIBBPF_PATH),@mkdir -p $(LIBBPF_PATH))
+	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_PATH) $(LIBBPF_PATH)libbpf.a
+
+$(LIBBPF)-clean:
+	$(call QUIET_CLEAN, libbpf)
+	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_PATH) clean >/dev/null
+
+CFLAGS := -g -I$(srctree)/tools/include -I$(BPF_DIR) -I$(SUBCMD_DIR)
+
+LIBS = -lelf -lz
+
+export srctree OUTPUT CFLAGS
+include $(srctree)/tools/build/Makefile.include
+
+$(BPFWL_IN): fixdep FORCE
+	$(Q)$(MAKE) $(build)=btfid
+
+$(BPFWL): $(LIBBPF) $(LIBSUBCMD) $(BPFWL_IN)
+	$(QUIET_LINK)$(CC) $(BPFWL_IN) $(LDFLAGS) -o $@ $(LIBBPF) $(LIBSUBCMD) $(LIBS)
+
+clean: $(LIBBPF)-clean $(LIBSUBCMD)-clean
+	$(call QUIET_CLEAN, btfid)
+	$(Q)$(RM) -f $(BPFWL)
+	$(Q)find $(if $(OUTPUT),$(OUTPUT),.) -name '*.o' -delete -o -name '\.*.cmd' -delete -o -name '\.*.d' -delete
+
+FORCE:
+
+.PHONY: all FORCE clean
diff --git a/tools/bpf/btfid/btfid.c b/tools/bpf/btfid/btfid.c
new file mode 100644
index 000000000000..7cdf39bfb150
--- /dev/null
+++ b/tools/bpf/btfid/btfid.c
@@ -0,0 +1,627 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+#define  _GNU_SOURCE
+
+/*
+ * btfid scans Elf object for .BTF_ids section and resolves
+ * its symbols with BTF IDs.
+ *
+ * Each symbol points to 4 bytes data and is expected to have
+ * following name syntax:
+ *
+ * __BTF_ID__<type>__<symbol>[__<id>]
+ *
+ * type is:
+ *
+ *   func   - lookup BTF_KIND_FUNC symbol with <symbol> name
+ *            and put its ID into its data
+ *
+ *             __BTF_ID__func__vfs_close__1:
+ *             .zero 4
+ *
+ *   struct - lookup BTF_KIND_STRUCT symbol with <symbol> name
+ *            and put its ID into its data
+ *
+ *             __BTF_ID__struct__sk_buff__1:
+ *             .zero 4
+ *
+ *   sort   - put symbol size into data area and sort following
+ *            ID list
+ *
+ *             __BTF_ID__sort__list:
+ *             list_cnt:
+ *             .zero 4
+ *             list:
+ *             __BTF_ID__func__vfs_getattr__3:
+ *             .zero 4
+ *             __BTF_ID__func__vfs_fallocate__4:
+ *             .zero 4
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <libelf.h>
+#include <gelf.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <linux/rbtree.h>
+#include <linux/zalloc.h>
+#include <btf.h>
+#include <libbpf.h>
+#include <parse-options.h>
+
+#define ADDR_CNT	100
+#define SECTION		".BTF_ids"
+#define BTF_STRUCT	"struct"
+#define BTF_FUNC	"func"
+#define BTF_SORT	"sort"
+#define BTF_ID		"__BTF_ID__"
+
+struct btf_id {
+	struct rb_node	 rb_node;
+	char		*name;
+	union {
+		int	 id;
+		int	 cnt;
+	};
+	int		 addr_cnt;
+	Elf64_Addr	 addr[ADDR_CNT];
+};
+
+struct object {
+	const char *path;
+
+	struct {
+		int		 fd;
+		Elf		*elf;
+		Elf_Data	*symbols;
+		Elf_Data	*idlist;
+		int		 symbols_shndx;
+		int		 idlist_shndx;
+		size_t		 strtabidx;
+		unsigned long	 idlist_addr;
+	} efile;
+
+	struct rb_root	sorts;
+	struct rb_root	funcs;
+	struct rb_root	structs;
+
+	int nr_funcs;
+	int nr_structs;
+};
+
+static int verbose;
+
+int eprintf(int level, int var, const char *fmt, ...)
+{
+	va_list args;
+	int ret;
+
+	if (var >= level) {
+		va_start(args, fmt);
+		ret = vfprintf(stderr, fmt, args);
+		va_end(args);
+	}
+	return ret;
+}
+
+#ifndef pr_fmt
+#define pr_fmt(fmt) fmt
+#endif
+
+#define pr_debug(fmt, ...) \
+	eprintf(1, verbose, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_debugN(n, fmt, ...) \
+	eprintf(n, verbose, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_debug2(fmt, ...) pr_debugN(2, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_err(fmt, ...) \
+	eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
+
+static bool is_btf_id(const char *name)
+{
+	return name && !strncmp(name, BTF_ID, sizeof(BTF_ID) - 1);
+}
+
+static struct btf_id *btf_id__find(struct rb_root *root, const char *name)
+{
+	struct rb_node *p = root->rb_node;
+	struct btf_id *id;
+	int cmp;
+
+	while (p) {
+		id = rb_entry(p, struct btf_id, rb_node);
+		cmp = strcmp(id->name, name);
+		if (cmp < 0)
+			p = p->rb_left;
+		else if (cmp > 0)
+			p = p->rb_right;
+		else
+			return id;
+	}
+	return NULL;
+}
+
+static struct btf_id*
+btf_id__add(struct rb_root *root, char *name, bool unique)
+{
+	struct rb_node **p = &root->rb_node;
+	struct rb_node *parent = NULL;
+	struct btf_id *id;
+	int cmp;
+
+	while (*p != NULL) {
+		parent = *p;
+		id = rb_entry(parent, struct btf_id, rb_node);
+		cmp = strcmp(id->name, name);
+		if (cmp < 0)
+			p = &(*p)->rb_left;
+		else if (cmp > 0)
+			p = &(*p)->rb_right;
+		else
+			return unique ? NULL : id;
+	}
+
+	id = zalloc(sizeof(*id));
+	if (id) {
+		pr_debug("adding symbol %s\n", name);
+		id->name = name;
+		rb_link_node(&id->rb_node, parent, p);
+		rb_insert_color(&id->rb_node, root);
+	}
+	return id;
+}
+
+static char *get_id(const char *prefix_end)
+{
+	/*
+	 * __BTF_ID__func__vfs_truncate__0
+	 * prefix_end =  ^
+	 */
+	char *p, *id = strdup(prefix_end + sizeof("__") - 1);
+
+	if (id) {
+		/*
+		 * __BTF_ID__func__vfs_truncate__0
+		 * id =            ^
+		 *
+		 * cut the unique id part
+		 */
+		p = strrchr(id, '_');
+		p--;
+		if (*p != '_') {
+			free(id);
+			return NULL;
+		}
+		*p = '\0';
+	}
+	return id;
+}
+
+static struct btf_id *add_sort(struct object *obj, char *name)
+{
+	char *id;
+
+	id = strdup(name + sizeof(BTF_SORT) + sizeof("__") - 2);
+	if (!id) {
+		pr_err("FAILED to parse cnt name: %s\n", name);
+		return NULL;
+	}
+
+	return btf_id__add(&obj->sorts, id, true);
+}
+
+static struct btf_id *add_func(struct object *obj, char *name)
+{
+	char *id;
+
+	id = get_id(name + sizeof(BTF_FUNC) - 1);
+	if (!id) {
+		pr_err("FAILED to parse func name: %s\n", name);
+		return NULL;
+	}
+
+	obj->nr_funcs++;
+	return btf_id__add(&obj->funcs, id, false);
+}
+
+static struct btf_id *add_struct(struct object *obj, char *name)
+{
+	char *id;
+
+	id = get_id(name + sizeof(BTF_STRUCT) - 1);
+	if (!id) {
+		pr_err("FAILED to parse struct name: %s\n", name);
+		return NULL;
+	}
+
+	obj->nr_structs++;
+	return btf_id__add(&obj->structs, id, false);
+}
+
+static int elf_collect(struct object *obj)
+{
+	Elf_Scn *scn = NULL;
+	size_t shdrstrndx;
+	int idx = 0;
+	Elf *elf;
+	int fd;
+
+	fd = open(obj->path, O_RDWR, 0666);
+	if (fd == -1) {
+		pr_err("FAILED cannot open %s: %s\n",
+			obj->path, strerror(errno));
+		return -1;
+	}
+
+	elf_version(EV_CURRENT);
+
+	elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL);
+	if (!elf) {
+		pr_err("FAILED cannot create ELF descriptor: %s\n",
+			elf_errmsg(-1));
+		return -1;
+	}
+
+	obj->efile.fd  = fd;
+	obj->efile.elf = elf;
+
+	elf_flagelf(elf, ELF_C_SET, ELF_F_LAYOUT);
+
+	if (elf_getshdrstrndx(elf, &shdrstrndx) != 0) {
+		pr_err("FAILED cannot get shdr str ndx\n");
+		return -1;
+	}
+
+	/*
+	 * Scan all the elf sections and look for save data
+	 * from .BTF_ids section and symbols.
+	 */
+	while ((scn = elf_nextscn(elf, scn)) != NULL) {
+		Elf_Data *data;
+		GElf_Shdr sh;
+		char *name;
+
+		idx++;
+		if (gelf_getshdr(scn, &sh) != &sh) {
+			pr_err("FAILED get section(%d) header\n", idx);
+			return -1;
+		}
+
+		name = elf_strptr(elf, shdrstrndx, sh.sh_name);
+		if (!name) {
+			pr_err("FAILED get section(%d) name\n", idx);
+			return -1;
+		}
+
+		data = elf_getdata(scn, 0);
+		if (!data) {
+			pr_err("failed to get section(%d) data from %s\n",
+				idx, name);
+			return -1;
+		}
+
+		pr_debug2("section(%d) %s, size %ld, link %d, flags %lx, type=%d\n",
+			  idx, name, (unsigned long) data->d_size,
+			  (int) sh.sh_link, (unsigned long) sh.sh_flags,
+			  (int) sh.sh_type);
+
+		if (sh.sh_type == SHT_SYMTAB) {
+			obj->efile.symbols       = data;
+			obj->efile.symbols_shndx = idx;
+			obj->efile.strtabidx     = sh.sh_link;
+		} else if (!strcmp(name, SECTION)) {
+			obj->efile.idlist       = data;
+			obj->efile.idlist_shndx = idx;
+			obj->efile.idlist_addr  = sh.sh_addr;
+		}
+	}
+
+	/*
+	 * We did not find .BTF_ids section or
+	 * symbols section, nothing to do..
+	 */
+	if (obj->efile.idlist_shndx == -1 ||
+	    obj->efile.symbols_shndx == -1) {
+		pr_err("FAILED to find needed sections\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+static int symbols_collect(struct object *obj)
+{
+	Elf_Scn *scn = NULL;
+	int n, i, err = 0;
+	GElf_Shdr sh;
+	char *name;
+
+	scn = elf_getscn(obj->efile.elf, obj->efile.symbols_shndx);
+	if (!scn)
+		return -1;
+
+	if (gelf_getshdr(scn, &sh) != &sh)
+		return -1;
+
+	n = sh.sh_size / sh.sh_entsize;
+
+	/*
+	 * Scan symbols and look for the ones starting with
+	 * __BTF_ID__* over .BTF_ids section.
+	 */
+	for (i = 0; !err && i < n; i++) {
+		char *tmp, *prefix;
+		struct btf_id *id;
+		GElf_Sym sym;
+		int err = -1;
+
+		if (!gelf_getsym(obj->efile.symbols, i, &sym))
+			return -1;
+
+		if (sym.st_shndx != obj->efile.idlist_shndx)
+			continue;
+
+		name = elf_strptr(obj->efile.elf, obj->efile.strtabidx,
+				  sym.st_name);
+
+		if (!is_btf_id(name))
+			continue;
+
+		/*
+		 * __BTF_ID__TYPE__vfs_truncate__0
+		 * prefix =  ^
+		 */
+		prefix = name + sizeof(BTF_ID) - 1;
+
+		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
+			id = add_struct(obj, prefix);
+		} else if (!strncmp(prefix, BTF_FUNC, sizeof(BTF_FUNC) - 1)) {
+			id = add_func(obj, prefix);
+		} else if (!strncmp(prefix, BTF_SORT, sizeof(BTF_SORT) - 1)) {
+			id = add_sort(obj, prefix);
+
+			/*
+			 * SORT objects store list's count, which is encoded
+			 * in symbol's size.
+			 */
+			if (id)
+				id->cnt = sym.st_size / sizeof(int);
+		} else {
+			pr_err("FAILED unsupported prefix %s\n", prefix);
+			return -1;
+		}
+
+		if (!id)
+			return -ENOMEM;
+
+		if (id->addr_cnt >= ADDR_CNT) {
+			pr_err("FAILED symbol %s crossed the number of allowed lists",
+				id->name);
+			return -1;
+		}
+		id->addr[id->addr_cnt++] = sym.st_value;
+	}
+
+	return 0;
+}
+
+static int symbols_resolve(struct object *obj)
+{
+	int nr_structs = obj->nr_structs;
+	int nr_funcs   = obj->nr_funcs;
+	struct btf *btf;
+	int err, type_id;
+	__u32 nr;
+
+	btf = btf__parse_elf(obj->path, NULL);
+	err = libbpf_get_error(btf);
+	if (err) {
+		pr_err("FAILED: load BTF from %s: %s",
+			obj->path, strerror(err));
+		return -1;
+	}
+
+	nr = btf__get_nr_types(btf);
+
+	/*
+	 * Iterate all the BTF types and search for collected symbol IDs.
+	 */
+	for (type_id = 0; type_id < nr; type_id++) {
+		const struct btf_type *type;
+		struct rb_root *root = NULL;
+		struct btf_id *id;
+		const char *str;
+		int *nr;
+
+		type = btf__type_by_id(btf, type_id);
+		if (!type)
+			continue;
+
+		/* We support func/struct types. */
+		if (BTF_INFO_KIND(type->info) == BTF_KIND_FUNC && nr_funcs) {
+			root = &obj->funcs;
+			nr = &nr_funcs;
+		} else if (BTF_INFO_KIND(type->info) == BTF_KIND_STRUCT && nr_structs) {
+			root = &obj->structs;
+			nr = &nr_structs;
+		} else {
+			continue;
+		}
+
+		str = btf__name_by_offset(btf, type->name_off);
+		if (!str)
+			continue;
+
+		id = btf_id__find(root, str);
+		if (id) {
+			id->id = type_id;
+			(*nr)--;
+		}
+	}
+
+	return 0;
+}
+
+static int id_patch(struct object *obj, struct btf_id *id)
+{
+	Elf_Data *data = obj->efile.idlist;
+	int *ptr = data->d_buf;
+	int i;
+
+	if (!id->id) {
+		pr_err("FAILED unresolved symbol %s\n", id->name);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < id->addr_cnt; i++) {
+		unsigned long addr = id->addr[i];
+		unsigned long idx = addr - obj->efile.idlist_addr;
+
+		pr_debug("patching addr %5lu: ID %7d [%s]\n", idx, id->id, id->name);
+
+		if (idx >= data->d_size) {
+			pr_err("FAILED patching index %lu out of bounds %lu\n",
+				idx, data->d_size);
+			return -1;
+		}
+
+		idx = idx / sizeof(int);
+		ptr[idx] = id->id;
+	}
+
+	return 0;
+}
+
+static int __symbols_patch(struct object *obj, struct rb_root *root)
+{
+	struct rb_node *next;
+	struct btf_id *id;
+
+	next = rb_first(root);
+	while (next) {
+		id = rb_entry(next, struct btf_id, rb_node);
+
+		if (id_patch(obj, id))
+			return -1;
+
+		next = rb_next(next);
+	}
+	return 0;
+}
+
+static int cmp_id(const void *pa, const void *pb)
+{
+	const int *a = pa, *b = pb;
+
+	return *a - *b;
+}
+
+static int sorts_patch(struct object *obj)
+{
+	Elf_Data *data = obj->efile.idlist;
+	int *ptr = data->d_buf;
+	struct rb_node *next;
+	struct btf_id *id;
+
+	next = rb_first(&obj->sorts);
+	while (next) {
+		unsigned long addr = id->addr[0];
+		unsigned long idx = addr - obj->efile.idlist_addr;
+		int *base;
+		int cnt;
+
+		id = rb_entry(next, struct btf_id, rb_node);
+
+		if (id->addr_cnt != 1)
+			return -1;
+
+		idx = idx / sizeof(int);
+		base = &ptr[idx] + 1;
+		cnt = ptr[idx];
+
+		pr_debug("sorting  addr %5lu: cnt %6d [%s]\n",
+			 (idx + 1) * sizeof(int), cnt, id->name);
+
+		qsort(base, cnt, sizeof(int), cmp_id);
+
+		next = rb_next(next);
+	}
+}
+
+static int symbols_patch(struct object *obj)
+{
+	int err;
+
+	if (__symbols_patch(obj, &obj->funcs) ||
+	    __symbols_patch(obj, &obj->structs) ||
+	    __symbols_patch(obj, &obj->sorts))
+		return -1;
+
+	if (sorts_patch(obj))
+		return -1;
+
+	elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY);
+
+	err = elf_update(obj->efile.elf, ELF_C_WRITE);
+	if (err < 0) {
+		pr_err("FAILED elf_update(WRITE): %s\n",
+			elf_errmsg(-1));
+	}
+
+	pr_debug("update %s for %s\n",
+		 err >= 0 ? "ok" : "failed", obj->path);
+	return err < 0 ? -1 : 0;
+}
+
+static const char * const btfid_usage[] = {
+	"btfid [<options>] <ELF object>",
+	NULL
+};
+
+static struct option btfid_options[] = {
+	OPT_INCR('v', "verbose", &verbose,
+		 "be more verbose (show errors, etc)"),
+	OPT_END()
+};
+
+int main(int argc, const char **argv)
+{
+	struct object obj = {
+		.efile = {
+			.idlist_shndx  = -1,
+			.symbols_shndx = -1,
+		},
+		.funcs   = RB_ROOT,
+		.structs = RB_ROOT,
+		.sorts   = RB_ROOT,
+	};
+
+	argc = parse_options(argc, argv, btfid_options, btfid_usage,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
+	if (argc != 1)
+		usage_with_options(btfid_usage, btfid_options);
+
+	obj.path = argv[0];
+
+	/*
+	 * We do proper cleanup and file close
+	 * intentionally only on success.
+	 */
+	if (elf_collect(&obj))
+		return -1;
+
+	if (symbols_collect(&obj))
+		return -1;
+
+	if (symbols_resolve(&obj))
+		return -1;
+
+	if (symbols_patch(&obj))
+		return -1;
+
+	elf_end(obj.efile.elf);
+	close(obj.efile.fd);
+	return 0;
+}
-- 
2.25.4

Re: [PATCH 01/11] bpf: Add btfid tool to resolve BTF IDs in ELF object

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The btfid tool scans Elf object for .BTF_ids section and
> resolves its symbols with BTF IDs.

naming is hard and subjective, I know. But given this actively
modifies ELF file it probably should indicate this in the name. So
something like patch_btfids or resolve_btfids would be a bit more
accurate and for people not in the know will still trigger the
"warning, tool can modify something" flag, if there are any problems.

>
> It will be used to during linking time to resolve arrays
> of BTF IDs used in verifier, so these IDs do not need to
> be resolved in runtime.
>
> The expected layout of .BTF_ids section is described
> in btfid.c header. Related kernel changes are coming in
> following changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/bpf/btfid/Build    |  26 ++
>  tools/bpf/btfid/Makefile |  71 +++++
>  tools/bpf/btfid/btfid.c  | 627 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 724 insertions(+)
>  create mode 100644 tools/bpf/btfid/Build
>  create mode 100644 tools/bpf/btfid/Makefile
>  create mode 100644 tools/bpf/btfid/btfid.c
>

[...]

> diff --git a/tools/bpf/btfid/btfid.c b/tools/bpf/btfid/btfid.c
> new file mode 100644
> index 000000000000..7cdf39bfb150
> --- /dev/null
> +++ b/tools/bpf/btfid/btfid.c
> @@ -0,0 +1,627 @@
> +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> +#define  _GNU_SOURCE
> +
> +/*
> + * btfid scans Elf object for .BTF_ids section and resolves
> + * its symbols with BTF IDs.
> + *
> + * Each symbol points to 4 bytes data and is expected to have
> + * following name syntax:
> + *
> + * __BTF_ID__<type>__<symbol>[__<id>]

This ___<id> thingy is just disambiguation between multiple places in
the code that could have BTF_ID macro, right? Or it has extra meaning?

> + *
> + * type is:
> + *
> + *   func   - lookup BTF_KIND_FUNC symbol with <symbol> name
> + *            and put its ID into its data
> + *
> + *             __BTF_ID__func__vfs_close__1:
> + *             .zero 4
> + *
> + *   struct - lookup BTF_KIND_STRUCT symbol with <symbol> name
> + *            and put its ID into its data
> + *
> + *             __BTF_ID__struct__sk_buff__1:
> + *             .zero 4
> + *
> + *   sort   - put symbol size into data area and sort following

Oh, I finally got what "put symbol size" means :) It's quite unclear,
to be honest. Also, is this size in bytes or number of IDs? Clarifying
would be helpful (I'll probably get this from reading further down the
code, but still..)

> + *            ID list
> + *
> + *             __BTF_ID__sort__list:
> + *             list_cnt:
> + *             .zero 4
> + *             list:
> + *             __BTF_ID__func__vfs_getattr__3:
> + *             .zero 4
> + *             __BTF_ID__func__vfs_fallocate__4:
> + *             .zero 4
> + */
> +

[...]

> +
> +static int symbols_collect(struct object *obj)
> +{
> +       Elf_Scn *scn = NULL;
> +       int n, i, err = 0;
> +       GElf_Shdr sh;
> +       char *name;
> +
> +       scn = elf_getscn(obj->efile.elf, obj->efile.symbols_shndx);
> +       if (!scn)
> +               return -1;
> +
> +       if (gelf_getshdr(scn, &sh) != &sh)
> +               return -1;
> +
> +       n = sh.sh_size / sh.sh_entsize;
> +
> +       /*
> +        * Scan symbols and look for the ones starting with
> +        * __BTF_ID__* over .BTF_ids section.
> +        */
> +       for (i = 0; !err && i < n; i++) {
> +               char *tmp, *prefix;
> +               struct btf_id *id;
> +               GElf_Sym sym;
> +               int err = -1;
> +
> +               if (!gelf_getsym(obj->efile.symbols, i, &sym))
> +                       return -1;
> +
> +               if (sym.st_shndx != obj->efile.idlist_shndx)
> +                       continue;
> +
> +               name = elf_strptr(obj->efile.elf, obj->efile.strtabidx,
> +                                 sym.st_name);
> +
> +               if (!is_btf_id(name))
> +                       continue;
> +
> +               /*
> +                * __BTF_ID__TYPE__vfs_truncate__0
> +                * prefix =  ^
> +                */
> +               prefix = name + sizeof(BTF_ID) - 1;
> +
> +               if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
> +                       id = add_struct(obj, prefix);
> +               } else if (!strncmp(prefix, BTF_FUNC, sizeof(BTF_FUNC) - 1)) {
> +                       id = add_func(obj, prefix);
> +               } else if (!strncmp(prefix, BTF_SORT, sizeof(BTF_SORT) - 1)) {
> +                       id = add_sort(obj, prefix);
> +
> +                       /*
> +                        * SORT objects store list's count, which is encoded
> +                        * in symbol's size.
> +                        */
> +                       if (id)
> +                               id->cnt = sym.st_size / sizeof(int);

doesn't sym.st_size also include extra 4 bytes for length prefix?

> +               } else {
> +                       pr_err("FAILED unsupported prefix %s\n", prefix);
> +                       return -1;
> +               }
> +
> +               if (!id)
> +                       return -ENOMEM;
> +
> +               if (id->addr_cnt >= ADDR_CNT) {
> +                       pr_err("FAILED symbol %s crossed the number of allowed lists",
> +                               id->name);
> +                       return -1;
> +               }
> +               id->addr[id->addr_cnt++] = sym.st_value;
> +       }
> +
> +       return 0;
> +}
> +
> +static int symbols_resolve(struct object *obj)
> +{
> +       int nr_structs = obj->nr_structs;
> +       int nr_funcs   = obj->nr_funcs;
> +       struct btf *btf;
> +       int err, type_id;
> +       __u32 nr;
> +
> +       btf = btf__parse_elf(obj->path, NULL);
> +       err = libbpf_get_error(btf);
> +       if (err) {
> +               pr_err("FAILED: load BTF from %s: %s",
> +                       obj->path, strerror(err));
> +               return -1;
> +       }
> +
> +       nr = btf__get_nr_types(btf);
> +
> +       /*
> +        * Iterate all the BTF types and search for collected symbol IDs.
> +        */
> +       for (type_id = 0; type_id < nr; type_id++) {

common gotcha: type_id <= nr, you can also skip type_id == 0 (always VOID)

> +               const struct btf_type *type;
> +               struct rb_root *root = NULL;
> +               struct btf_id *id;
> +               const char *str;
> +               int *nr;
> +
> +               type = btf__type_by_id(btf, type_id);
> +               if (!type)
> +                       continue;

This ought to be an error...

> +
> +               /* We support func/struct types. */
> +               if (BTF_INFO_KIND(type->info) == BTF_KIND_FUNC && nr_funcs) {

see libbpf's btf.h: btf_is_func(type)

> +                       root = &obj->funcs;
> +                       nr = &nr_funcs;
> +               } else if (BTF_INFO_KIND(type->info) == BTF_KIND_STRUCT && nr_structs) {

same as above: btf_is_struct

But I think you also need to support unions?

Also what about typedefs? A lot of types are typedefs to struct/func_proto/etc.

> +                       root = &obj->structs;
> +                       nr = &nr_structs;
> +               } else {
> +                       continue;
> +               }
> +
> +               str = btf__name_by_offset(btf, type->name_off);
> +               if (!str)
> +                       continue;

error, shouldn't happen

> +
> +               id = btf_id__find(root, str);
> +               if (id) {

isn't it an error, if not found?

> +                       id->id = type_id;
> +                       (*nr)--;
> +               }
> +       }
> +
> +       return 0;
> +}
> +

[...]

> +
> +       /*
> +        * We do proper cleanup and file close
> +        * intentionally only on success.
> +        */
> +       if (elf_collect(&obj))
> +               return -1;
> +
> +       if (symbols_collect(&obj))
> +               return -1;
> +
> +       if (symbols_resolve(&obj))
> +               return -1;
> +
> +       if (symbols_patch(&obj))
> +               return -1;

nit: should these elf_end/close properly on error?


> +
> +       elf_end(obj.efile.elf);
> +       close(obj.efile.fd);
> +       return 0;
> +}
> --
> 2.25.4
>

Re: [PATCH 01/11] bpf: Add btfid tool to resolve BTF IDs in ELF object

[Jiri Olsa]

On Thu, Jun 18, 2020 at 05:38:03PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The btfid tool scans Elf object for .BTF_ids section and
> > resolves its symbols with BTF IDs.
> 
> naming is hard and subjective, I know. But given this actively
> modifies ELF file it probably should indicate this in the name. So
> something like patch_btfids or resolve_btfids would be a bit more
> accurate and for people not in the know will still trigger the
> "warning, tool can modify something" flag, if there are any problems.

resolve_btfids sounds good to me

> 
> >
> > It will be used to during linking time to resolve arrays
> > of BTF IDs used in verifier, so these IDs do not need to
> > be resolved in runtime.
> >
> > The expected layout of .BTF_ids section is described
> > in btfid.c header. Related kernel changes are coming in
> > following changes.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/bpf/btfid/Build    |  26 ++
> >  tools/bpf/btfid/Makefile |  71 +++++
> >  tools/bpf/btfid/btfid.c  | 627 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 724 insertions(+)
> >  create mode 100644 tools/bpf/btfid/Build
> >  create mode 100644 tools/bpf/btfid/Makefile
> >  create mode 100644 tools/bpf/btfid/btfid.c
> >
> 
> [...]
> 
> > diff --git a/tools/bpf/btfid/btfid.c b/tools/bpf/btfid/btfid.c
> > new file mode 100644
> > index 000000000000..7cdf39bfb150
> > --- /dev/null
> > +++ b/tools/bpf/btfid/btfid.c
> > @@ -0,0 +1,627 @@
> > +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> > +#define  _GNU_SOURCE
> > +
> > +/*
> > + * btfid scans Elf object for .BTF_ids section and resolves
> > + * its symbols with BTF IDs.
> > + *
> > + * Each symbol points to 4 bytes data and is expected to have
> > + * following name syntax:
> > + *
> > + * __BTF_ID__<type>__<symbol>[__<id>]
> 
> This ___<id> thingy is just disambiguation between multiple places in
> the code that could have BTF_ID macro, right? Or it has extra meaning?

it's there so you could multiple BTF_ID instances with the same
symbol name

> 
> > + *
> > + * type is:
> > + *
> > + *   func   - lookup BTF_KIND_FUNC symbol with <symbol> name
> > + *            and put its ID into its data
> > + *
> > + *             __BTF_ID__func__vfs_close__1:
> > + *             .zero 4
> > + *
> > + *   struct - lookup BTF_KIND_STRUCT symbol with <symbol> name
> > + *            and put its ID into its data
> > + *
> > + *             __BTF_ID__struct__sk_buff__1:
> > + *             .zero 4
> > + *
> > + *   sort   - put symbol size into data area and sort following
> 
> Oh, I finally got what "put symbol size" means :) It's quite unclear,
> to be honest. Also, is this size in bytes or number of IDs? Clarifying
> would be helpful (I'll probably get this from reading further down the
> code, but still..)

I 'put' ;-) the documentation mainly to kernel/bpf/btf_ids.h,

so there are 2 types of lists, first one defines
just IDs as they go:

 BTF_ID_LIST(list1)
 BTF_ID(type1, name1)
 BTF_ID(type2, name2)

and it's used for helpers btf_id array

2nd one provides count and is sorted:

 BTF_WHITELIST_ENTRY(list2)
 BTF_ID(type1, name1)
 BTF_ID(type2, name2)
 BTF_WHITELIST_END(list)

and it's used for d_path whitelist so far

SNIP

> > +               if (sym.st_shndx != obj->efile.idlist_shndx)
> > +                       continue;
> > +
> > +               name = elf_strptr(obj->efile.elf, obj->efile.strtabidx,
> > +                                 sym.st_name);
> > +
> > +               if (!is_btf_id(name))
> > +                       continue;
> > +
> > +               /*
> > +                * __BTF_ID__TYPE__vfs_truncate__0
> > +                * prefix =  ^
> > +                */
> > +               prefix = name + sizeof(BTF_ID) - 1;
> > +
> > +               if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
> > +                       id = add_struct(obj, prefix);
> > +               } else if (!strncmp(prefix, BTF_FUNC, sizeof(BTF_FUNC) - 1)) {
> > +                       id = add_func(obj, prefix);
> > +               } else if (!strncmp(prefix, BTF_SORT, sizeof(BTF_SORT) - 1)) {
> > +                       id = add_sort(obj, prefix);
> > +
> > +                       /*
> > +                        * SORT objects store list's count, which is encoded
> > +                        * in symbol's size.
> > +                        */
> > +                       if (id)
> > +                               id->cnt = sym.st_size / sizeof(int);
> 
> doesn't sym.st_size also include extra 4 bytes for length prefix?

no, count is excluded from the size

SNIP

> > +       btf = btf__parse_elf(obj->path, NULL);
> > +       err = libbpf_get_error(btf);
> > +       if (err) {
> > +               pr_err("FAILED: load BTF from %s: %s",
> > +                       obj->path, strerror(err));
> > +               return -1;
> > +       }
> > +
> > +       nr = btf__get_nr_types(btf);
> > +
> > +       /*
> > +        * Iterate all the BTF types and search for collected symbol IDs.
> > +        */
> > +       for (type_id = 0; type_id < nr; type_id++) {
> 
> common gotcha: type_id <= nr, you can also skip type_id == 0 (always VOID)

ugh, yep.. thanks ;-)

> 
> > +               const struct btf_type *type;
> > +               struct rb_root *root = NULL;
> > +               struct btf_id *id;
> > +               const char *str;
> > +               int *nr;
> > +
> > +               type = btf__type_by_id(btf, type_id);
> > +               if (!type)
> > +                       continue;
> 
> This ought to be an error...

ok, something like "BTF malformed error"

> 
> > +
> > +               /* We support func/struct types. */
> > +               if (BTF_INFO_KIND(type->info) == BTF_KIND_FUNC && nr_funcs) {
> 
> see libbpf's btf.h: btf_is_func(type)

ok 

> 
> > +                       root = &obj->funcs;
> > +                       nr = &nr_funcs;
> > +               } else if (BTF_INFO_KIND(type->info) == BTF_KIND_STRUCT && nr_structs) {
> 
> same as above: btf_is_struct
> 
> But I think you also need to support unions?
> 
> Also what about typedefs? A lot of types are typedefs to struct/func_proto/etc.

I added only types which are needed at the moment, but maybe
we can add the basic types now, so we don't need to bother later,
when we forget how this all work ;-)

> 
> > +                       root = &obj->structs;
> > +                       nr = &nr_structs;
> > +               } else {
> > +                       continue;
> > +               }
> > +
> > +               str = btf__name_by_offset(btf, type->name_off);
> > +               if (!str)
> > +                       continue;
> 
> error, shouldn't happen

ok

> 
> > +
> > +               id = btf_id__find(root, str);
> > +               if (id) {
> 
> isn't it an error, if not found?

no, at this point we are checking if this BTF type was collected
as a symbol for struct/func in some list.. if not, we continue the
iteration to next BTF type

> 
> > +                       id->id = type_id;
> > +                       (*nr)--;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> 
> [...]
> 
> > +
> > +       /*
> > +        * We do proper cleanup and file close
> > +        * intentionally only on success.
> > +        */
> > +       if (elf_collect(&obj))
> > +               return -1;
> > +
> > +       if (symbols_collect(&obj))
> > +               return -1;
> > +
> > +       if (symbols_resolve(&obj))
> > +               return -1;
> > +
> > +       if (symbols_patch(&obj))
> > +               return -1;
> 
> nit: should these elf_end/close properly on error?

I wrote in the comment above that I intentionaly do not cleanup
on error path.. I wanted to save some time, but actualy I think
that would not be so expensive, I can add it

thanks,
jirka

> 
> 
> > +
> > +       elf_end(obj.efile.elf);
> > +       close(obj.efile.fd);
> > +       return 0;
> > +}
> > --
> > 2.25.4
> >
> 

Re: [PATCH 01/11] bpf: Add btfid tool to resolve BTF IDs in ELF object

[Andrii Nakryiko]

On Fri, Jun 19, 2020 at 6:04 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Jun 18, 2020 at 05:38:03PM -0700, Andrii Nakryiko wrote:
> > On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > The btfid tool scans Elf object for .BTF_ids section and
> > > resolves its symbols with BTF IDs.
> >
> > naming is hard and subjective, I know. But given this actively
> > modifies ELF file it probably should indicate this in the name. So
> > something like patch_btfids or resolve_btfids would be a bit more
> > accurate and for people not in the know will still trigger the
> > "warning, tool can modify something" flag, if there are any problems.
>
> resolve_btfids sounds good to me
>
> >
> > >
> > > It will be used to during linking time to resolve arrays
> > > of BTF IDs used in verifier, so these IDs do not need to
> > > be resolved in runtime.
> > >
> > > The expected layout of .BTF_ids section is described
> > > in btfid.c header. Related kernel changes are coming in
> > > following changes.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/bpf/btfid/Build    |  26 ++
> > >  tools/bpf/btfid/Makefile |  71 +++++
> > >  tools/bpf/btfid/btfid.c  | 627 +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 724 insertions(+)
> > >  create mode 100644 tools/bpf/btfid/Build
> > >  create mode 100644 tools/bpf/btfid/Makefile
> > >  create mode 100644 tools/bpf/btfid/btfid.c
> > >
> >
> > [...]
> >
> > > diff --git a/tools/bpf/btfid/btfid.c b/tools/bpf/btfid/btfid.c
> > > new file mode 100644
> > > index 000000000000..7cdf39bfb150
> > > --- /dev/null
> > > +++ b/tools/bpf/btfid/btfid.c
> > > @@ -0,0 +1,627 @@
> > > +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> > > +#define  _GNU_SOURCE
> > > +
> > > +/*
> > > + * btfid scans Elf object for .BTF_ids section and resolves
> > > + * its symbols with BTF IDs.
> > > + *
> > > + * Each symbol points to 4 bytes data and is expected to have
> > > + * following name syntax:
> > > + *
> > > + * __BTF_ID__<type>__<symbol>[__<id>]
> >
> > This ___<id> thingy is just disambiguation between multiple places in
> > the code that could have BTF_ID macro, right? Or it has extra meaning?
>
> it's there so you could multiple BTF_ID instances with the same
> symbol name

yeah, reading subsequent patches clarified that for me :)

>
> >
> > > + *
> > > + * type is:
> > > + *
> > > + *   func   - lookup BTF_KIND_FUNC symbol with <symbol> name
> > > + *            and put its ID into its data
> > > + *
> > > + *             __BTF_ID__func__vfs_close__1:
> > > + *             .zero 4
> > > + *
> > > + *   struct - lookup BTF_KIND_STRUCT symbol with <symbol> name
> > > + *            and put its ID into its data
> > > + *
> > > + *             __BTF_ID__struct__sk_buff__1:
> > > + *             .zero 4
> > > + *
> > > + *   sort   - put symbol size into data area and sort following
> >
> > Oh, I finally got what "put symbol size" means :) It's quite unclear,
> > to be honest. Also, is this size in bytes or number of IDs? Clarifying
> > would be helpful (I'll probably get this from reading further down the
> > code, but still..)
>
> I 'put' ;-) the documentation mainly to kernel/bpf/btf_ids.h,
>
> so there are 2 types of lists, first one defines
> just IDs as they go:
>
>  BTF_ID_LIST(list1)
>  BTF_ID(type1, name1)
>  BTF_ID(type2, name2)
>
> and it's used for helpers btf_id array
>
> 2nd one provides count and is sorted:
>
>  BTF_WHITELIST_ENTRY(list2)
>  BTF_ID(type1, name1)
>  BTF_ID(type2, name2)
>  BTF_WHITELIST_END(list)
>
> and it's used for d_path whitelist so far


yeah, because BTF_ID_LIST and BTF_WHITELIST stuff was in two different
patches, I initially was confused by BTF_ID_LIST and thought it's
actually what BTF_WHITELIST is. Sorry for too many questions!


>
> SNIP
>
> > > +               if (sym.st_shndx != obj->efile.idlist_shndx)
> > > +                       continue;
> > > +
> > > +               name = elf_strptr(obj->efile.elf, obj->efile.strtabidx,
> > > +                                 sym.st_name);
> > > +
> > > +               if (!is_btf_id(name))
> > > +                       continue;
> > > +
> > > +               /*
> > > +                * __BTF_ID__TYPE__vfs_truncate__0
> > > +                * prefix =  ^
> > > +                */
> > > +               prefix = name + sizeof(BTF_ID) - 1;
> > > +
> > > +               if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
> > > +                       id = add_struct(obj, prefix);
> > > +               } else if (!strncmp(prefix, BTF_FUNC, sizeof(BTF_FUNC) - 1)) {
> > > +                       id = add_func(obj, prefix);
> > > +               } else if (!strncmp(prefix, BTF_SORT, sizeof(BTF_SORT) - 1)) {
> > > +                       id = add_sort(obj, prefix);
> > > +
> > > +                       /*
> > > +                        * SORT objects store list's count, which is encoded
> > > +                        * in symbol's size.
> > > +                        */
> > > +                       if (id)
> > > +                               id->cnt = sym.st_size / sizeof(int);
> >
> > doesn't sym.st_size also include extra 4 bytes for length prefix?
>
> no, count is excluded from the size
>
> SNIP
>
> > > +       btf = btf__parse_elf(obj->path, NULL);
> > > +       err = libbpf_get_error(btf);
> > > +       if (err) {
> > > +               pr_err("FAILED: load BTF from %s: %s",
> > > +                       obj->path, strerror(err));
> > > +               return -1;
> > > +       }
> > > +
> > > +       nr = btf__get_nr_types(btf);
> > > +
> > > +       /*
> > > +        * Iterate all the BTF types and search for collected symbol IDs.
> > > +        */
> > > +       for (type_id = 0; type_id < nr; type_id++) {
> >
> > common gotcha: type_id <= nr, you can also skip type_id == 0 (always VOID)
>
> ugh, yep.. thanks ;-)
>
> >
> > > +               const struct btf_type *type;
> > > +               struct rb_root *root = NULL;
> > > +               struct btf_id *id;
> > > +               const char *str;
> > > +               int *nr;
> > > +
> > > +               type = btf__type_by_id(btf, type_id);
> > > +               if (!type)
> > > +                       continue;
> >
> > This ought to be an error...
>
> ok, something like "BTF malformed error"
>
> >
> > > +
> > > +               /* We support func/struct types. */
> > > +               if (BTF_INFO_KIND(type->info) == BTF_KIND_FUNC && nr_funcs) {
> >
> > see libbpf's btf.h: btf_is_func(type)
>
> ok
>
> >
> > > +                       root = &obj->funcs;
> > > +                       nr = &nr_funcs;
> > > +               } else if (BTF_INFO_KIND(type->info) == BTF_KIND_STRUCT && nr_structs) {
> >
> > same as above: btf_is_struct
> >
> > But I think you also need to support unions?
> >
> > Also what about typedefs? A lot of types are typedefs to struct/func_proto/etc.
>
> I added only types which are needed at the moment, but maybe
> we can add the basic types now, so we don't need to bother later,
> when we forget how this all work ;-)

yeah, exactly. Once this works, no one will want to go and revisit it,
so I'd rather make it generic from the get go, especially that it's
really easy in this case, right?

>
> >
> > > +                       root = &obj->structs;
> > > +                       nr = &nr_structs;
> > > +               } else {
> > > +                       continue;
> > > +               }
> > > +
> > > +               str = btf__name_by_offset(btf, type->name_off);
> > > +               if (!str)
> > > +                       continue;
> >
> > error, shouldn't happen
>
> ok
>
> >
> > > +
> > > +               id = btf_id__find(root, str);
> > > +               if (id) {
> >
> > isn't it an error, if not found?
>
> no, at this point we are checking if this BTF type was collected
> as a symbol for struct/func in some list.. if not, we continue the
> iteration to next BTF type

ah, ok

>
> >
> > > +                       id->id = type_id;
> > > +                       (*nr)--;
> > > +               }
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> >
> > [...]
> >
> > > +
> > > +       /*
> > > +        * We do proper cleanup and file close
> > > +        * intentionally only on success.
> > > +        */
> > > +       if (elf_collect(&obj))
> > > +               return -1;
> > > +
> > > +       if (symbols_collect(&obj))
> > > +               return -1;
> > > +
> > > +       if (symbols_resolve(&obj))
> > > +               return -1;
> > > +
> > > +       if (symbols_patch(&obj))
> > > +               return -1;
> >
> > nit: should these elf_end/close properly on error?
>
> I wrote in the comment above that I intentionaly do not cleanup
> on error path.. I wanted to save some time, but actualy I think
> that would not be so expensive, I can add it

as in save CPU time in error case? Who cares about that? If saving
developer time, well... `goto cleanup` is common and simple pattern ;)

>
> thanks,
> jirka
>
> >
> >
> > > +
> > > +       elf_end(obj.efile.elf);
> > > +       close(obj.efile.fd);
> > > +       return 0;
> > > +}
> > > --
> > > 2.25.4
> > >
> >
>

Re: [PATCH 01/11] bpf: Add btfid tool to resolve BTF IDs in ELF object

[Jiri Olsa]

On Fri, Jun 19, 2020 at 11:12:29AM -0700, Andrii Nakryiko wrote:

SNIP

> >
> > ok
> >
> > >
> > > > +                       root = &obj->funcs;
> > > > +                       nr = &nr_funcs;
> > > > +               } else if (BTF_INFO_KIND(type->info) == BTF_KIND_STRUCT && nr_structs) {
> > >
> > > same as above: btf_is_struct
> > >
> > > But I think you also need to support unions?
> > >
> > > Also what about typedefs? A lot of types are typedefs to struct/func_proto/etc.
> >
> > I added only types which are needed at the moment, but maybe
> > we can add the basic types now, so we don't need to bother later,
> > when we forget how this all work ;-)
> 
> yeah, exactly. Once this works, no one will want to go and revisit it,
> so I'd rather make it generic from the get go, especially that it's
> really easy in this case, right?

right, I'll add the basic types

SNIP

> > >
> > > nit: should these elf_end/close properly on error?
> >
> > I wrote in the comment above that I intentionaly do not cleanup
> > on error path.. I wanted to save some time, but actualy I think
> > that would not be so expensive, I can add it
> 
> as in save CPU time in error case? Who cares about that? If saving
> developer time, well... `goto cleanup` is common and simple pattern ;)

the sooner you see the error the better ;-) ok

jirka

[PATCH 02/11] bpf: Compile btfid tool at kernel compilation start

[Jiri Olsa]

The btfid tool will be used during the vmlinux linking,
so it's necessary it's ready for it.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 Makefile           | 22 ++++++++++++++++++----
 tools/Makefile     |  3 +++
 tools/bpf/Makefile |  5 ++++-
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 839f9fee22cb..b190d502d7d7 100644
--- a/Makefile
+++ b/Makefile
@@ -1066,9 +1066,10 @@ export mod_sign_cmd
 
 HOST_LIBELF_LIBS = $(shell pkg-config libelf --libs 2>/dev/null || echo -lelf)
 
+has_libelf = $(call try-run,\
+               echo "int main() {}" | $(HOSTCC) -xc -o /dev/null $(HOST_LIBELF_LIBS) -,1,0)
+
 ifdef CONFIG_STACK_VALIDATION
-  has_libelf := $(call try-run,\
-		echo "int main() {}" | $(HOSTCC) -xc -o /dev/null $(HOST_LIBELF_LIBS) -,1,0)
   ifeq ($(has_libelf),1)
     objtool_target := tools/objtool FORCE
   else
@@ -1077,6 +1078,14 @@ ifdef CONFIG_STACK_VALIDATION
   endif
 endif
 
+ifdef CONFIG_DEBUG_INFO_BTF
+  ifeq ($(has_libelf),1)
+    btfid_target := tools/bpf/btfid FORCE
+  else
+    ERROR_BTF_IDS_RESOLVE := 1
+  endif
+endif
+
 PHONY += prepare0
 
 export MODORDER := $(extmod-prefix)modules.order
@@ -1188,7 +1197,7 @@ prepare0: archprepare
 	$(Q)$(MAKE) $(build)=.
 
 # All the preparing..
-prepare: prepare0 prepare-objtool
+prepare: prepare0 prepare-objtool prepare-btfid
 
 # Support for using generic headers in asm-generic
 asm-generic := -f $(srctree)/scripts/Makefile.asm-generic obj
@@ -1201,7 +1210,7 @@ uapi-asm-generic:
 	$(Q)$(MAKE) $(asm-generic)=arch/$(SRCARCH)/include/generated/uapi/asm \
 	generic=include/uapi/asm-generic
 
-PHONY += prepare-objtool
+PHONY += prepare-objtool prepare-btfid
 prepare-objtool: $(objtool_target)
 ifeq ($(SKIP_STACK_VALIDATION),1)
 ifdef CONFIG_UNWINDER_ORC
@@ -1212,6 +1221,11 @@ else
 endif
 endif
 
+prepare-btfid: $(btfid_target)
+ifeq ($(ERROR_BTF_IDS_RESOLVE),1)
+	@echo "error: Cannot resolve BTF IDs for CONFIG_DEBUG_INFO_BTF, please install libelf-dev, libelf-devel or elfutils-libelf-devel" >&2
+	@false
+endif
 # Generate some files
 # ---------------------------------------------------------------------------
 
diff --git a/tools/Makefile b/tools/Makefile
index bd778812e915..85af6ebbce91 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -67,6 +67,9 @@ cpupower: FORCE
 cgroup firewire hv guest bootconfig spi usb virtio vm bpf iio gpio objtool leds wmi pci firmware debugging: FORCE
 	$(call descend,$@)
 
+bpf/%: FORCE
+	$(call descend,$@)
+
 liblockdep: FORCE
 	$(call descend,lib/lockdep)
 
diff --git a/tools/bpf/Makefile b/tools/bpf/Makefile
index 77472e28c8fd..d8bbe7ef264f 100644
--- a/tools/bpf/Makefile
+++ b/tools/bpf/Makefile
@@ -124,5 +124,8 @@ runqslower_install:
 runqslower_clean:
 	$(call descend,runqslower,clean)
 
+btfid:
+	$(call descend,btfid)
+
 .PHONY: all install clean bpftool bpftool_install bpftool_clean \
-	runqslower runqslower_install runqslower_clean
+	runqslower runqslower_install runqslower_clean btfid
-- 
2.25.4

RE: [PATCH 02/11] bpf: Compile btfid tool at kernel compilation start

[John Fastabend]

Jiri Olsa wrote:
> The btfid tool will be used during the vmlinux linking,
> so it's necessary it's ready for it.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  Makefile           | 22 ++++++++++++++++++----
>  tools/Makefile     |  3 +++
>  tools/bpf/Makefile |  5 ++++-
>  3 files changed, 25 insertions(+), 5 deletions(-)

This breaks the build for me. I fixed it with this but then I get warnings,

diff --git a/tools/bpf/btfid/btfid.c b/tools/bpf/btfid/btfid.c
index 7cdf39bfb150..3697e8ae9efa 100644
--- a/tools/bpf/btfid/btfid.c
+++ b/tools/bpf/btfid/btfid.c
@@ -48,7 +48,7 @@
 #include <errno.h>
 #include <linux/rbtree.h>
 #include <linux/zalloc.h>
-#include <btf.h>
+#include <linux/btf.h>
 #include <libbpf.h>
 #include <parse-options.h>

Here is the error. Is it something about my setup? bpftool/btf.c uses
<btf.h>. Because this in top-level Makefile we probably don't want to
push extra setup onto folks.

In file included from btfid.c:51:
/home/john/git/bpf-next/tools/lib/bpf/btf.h: In function ‘btf_is_var’:
/home/john/git/bpf-next/tools/lib/bpf/btf.h:254:24: error: ‘BTF_KIND_VAR’ undeclared (first use in this function); did you mean ‘BTF_KIND_PTR’?
  return btf_kind(t) == BTF_KIND_VAR;
                        ^~~~~~~~~~~~
                        BTF_KIND_PTR
/home/john/git/bpf-next/tools/lib/bpf/btf.h:254:24: note: each undeclared identifier is reported only once for each function it appears in
/home/john/git/bpf-next/tools/lib/bpf/btf.h: In function ‘btf_is_datasec’:
/home/john/git/bpf-next/tools/lib/bpf/btf.h:259:24: error: ‘BTF_KIND_DATASEC’ undeclared (first use in this function); did you mean ‘BTF_KIND_PTR’?
  return btf_kind(t) == BTF_KIND_DATASEC;
                        ^~~~~~~~~~~~~~~~
                        BTF_KIND_PTR
mv: cannot stat '/home/john/git/bpf-next/tools/bpf/btfid/.btfid.o.tmp': No such file or directory
make[3]: *** [/home/john/git/bpf-next/tools/build/Makefile.build:97: /home/john/git/bpf-next/tools/bpf/btfid/btfid.o] Error 1
make[2]: *** [Makefile:59: /home/john/git/bpf-next/tools/bpf/btfid/btfid-in.o] Error 2
make[1]: *** [Makefile:71: bpf/btfid] Error 2
make: *** [Makefile:1894: tools/bpf/btfid] Error 2

RE: [PATCH 02/11] bpf: Compile btfid tool at kernel compilation start

[John Fastabend]

John Fastabend wrote:
> Jiri Olsa wrote:
> > The btfid tool will be used during the vmlinux linking,
> > so it's necessary it's ready for it.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  Makefile           | 22 ++++++++++++++++++----
> >  tools/Makefile     |  3 +++
> >  tools/bpf/Makefile |  5 ++++-
> >  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> This breaks the build for me. I fixed it with this but then I get warnings,

Also maybe fix below is not good because now I get a segfault in btfid.

> 
> diff --git a/tools/bpf/btfid/btfid.c b/tools/bpf/btfid/btfid.c
> index 7cdf39bfb150..3697e8ae9efa 100644
> --- a/tools/bpf/btfid/btfid.c
> +++ b/tools/bpf/btfid/btfid.c
> @@ -48,7 +48,7 @@
>  #include <errno.h>
>  #include <linux/rbtree.h>
>  #include <linux/zalloc.h>
> -#include <btf.h>
> +#include <linux/btf.h>
>  #include <libbpf.h>
>  #include <parse-options.h>
> 
> Here is the error. Is it something about my setup? bpftool/btf.c uses
> <btf.h>. Because this in top-level Makefile we probably don't want to
> push extra setup onto folks.
> 
> In file included from btfid.c:51:
> /home/john/git/bpf-next/tools/lib/bpf/btf.h: In function ‘btf_is_var’:
> /home/john/git/bpf-next/tools/lib/bpf/btf.h:254:24: error: ‘BTF_KIND_VAR’ undeclared (first use in this function); did you mean ‘BTF_KIND_PTR’?
>   return btf_kind(t) == BTF_KIND_VAR;
>                         ^~~~~~~~~~~~
>                         BTF_KIND_PTR
> /home/john/git/bpf-next/tools/lib/bpf/btf.h:254:24: note: each undeclared identifier is reported only once for each function it appears in
> /home/john/git/bpf-next/tools/lib/bpf/btf.h: In function ‘btf_is_datasec’:
> /home/john/git/bpf-next/tools/lib/bpf/btf.h:259:24: error: ‘BTF_KIND_DATASEC’ undeclared (first use in this function); did you mean ‘BTF_KIND_PTR’?
>   return btf_kind(t) == BTF_KIND_DATASEC;
>                         ^~~~~~~~~~~~~~~~
>                         BTF_KIND_PTR
> mv: cannot stat '/home/john/git/bpf-next/tools/bpf/btfid/.btfid.o.tmp': No such file or directory
> make[3]: *** [/home/john/git/bpf-next/tools/build/Makefile.build:97: /home/john/git/bpf-next/tools/bpf/btfid/btfid.o] Error 1
> make[2]: *** [Makefile:59: /home/john/git/bpf-next/tools/bpf/btfid/btfid-in.o] Error 2
> make[1]: *** [Makefile:71: bpf/btfid] Error 2
> make: *** [Makefile:1894: tools/bpf/btfid] Error 2

Re: [PATCH 02/11] bpf: Compile btfid tool at kernel compilation start

[Jiri Olsa]

On Thu, Jun 18, 2020 at 01:40:40PM -0700, John Fastabend wrote:
> Jiri Olsa wrote:
> > The btfid tool will be used during the vmlinux linking,
> > so it's necessary it's ready for it.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  Makefile           | 22 ++++++++++++++++++----
> >  tools/Makefile     |  3 +++
> >  tools/bpf/Makefile |  5 ++++-
> >  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> This breaks the build for me. I fixed it with this but then I get warnings,
> 
> diff --git a/tools/bpf/btfid/btfid.c b/tools/bpf/btfid/btfid.c
> index 7cdf39bfb150..3697e8ae9efa 100644
> --- a/tools/bpf/btfid/btfid.c
> +++ b/tools/bpf/btfid/btfid.c
> @@ -48,7 +48,7 @@
>  #include <errno.h>
>  #include <linux/rbtree.h>
>  #include <linux/zalloc.h>
> -#include <btf.h>
> +#include <linux/btf.h>
>  #include <libbpf.h>
>  #include <parse-options.h>
> 
> Here is the error. Is it something about my setup? bpftool/btf.c uses
> <btf.h>. Because this in top-level Makefile we probably don't want to
> push extra setup onto folks.

ouch, I wonder it's because I have libbpf installed and the
setup got mixed up.. I'll erase and try to reproduce

thanks,
jirka

> 
> In file included from btfid.c:51:
> /home/john/git/bpf-next/tools/lib/bpf/btf.h: In function ‘btf_is_var’:
> /home/john/git/bpf-next/tools/lib/bpf/btf.h:254:24: error: ‘BTF_KIND_VAR’ undeclared (first use in this function); did you mean ‘BTF_KIND_PTR’?
>   return btf_kind(t) == BTF_KIND_VAR;
>                         ^~~~~~~~~~~~
>                         BTF_KIND_PTR
> /home/john/git/bpf-next/tools/lib/bpf/btf.h:254:24: note: each undeclared identifier is reported only once for each function it appears in
> /home/john/git/bpf-next/tools/lib/bpf/btf.h: In function ‘btf_is_datasec’:
> /home/john/git/bpf-next/tools/lib/bpf/btf.h:259:24: error: ‘BTF_KIND_DATASEC’ undeclared (first use in this function); did you mean ‘BTF_KIND_PTR’?
>   return btf_kind(t) == BTF_KIND_DATASEC;
>                         ^~~~~~~~~~~~~~~~
>                         BTF_KIND_PTR
> mv: cannot stat '/home/john/git/bpf-next/tools/bpf/btfid/.btfid.o.tmp': No such file or directory
> make[3]: *** [/home/john/git/bpf-next/tools/build/Makefile.build:97: /home/john/git/bpf-next/tools/bpf/btfid/btfid.o] Error 1
> make[2]: *** [Makefile:59: /home/john/git/bpf-next/tools/bpf/btfid/btfid-in.o] Error 2
> make[1]: *** [Makefile:71: bpf/btfid] Error 2
> make: *** [Makefile:1894: tools/bpf/btfid] Error 2
> 

Re: [PATCH 02/11] bpf: Compile btfid tool at kernel compilation start

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The btfid tool will be used during the vmlinux linking,
> so it's necessary it's ready for it.
>

Seeing troubles John runs into, I wonder if it maybe would be better
to add it to pahole instead? It's already a dependency for anything
BTF-related in the kernel. It has libelf, libbpf linked and set up.
WDYT? I've cc'ed Arnaldo as well for an opinion.

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  Makefile           | 22 ++++++++++++++++++----
>  tools/Makefile     |  3 +++
>  tools/bpf/Makefile |  5 ++++-
>  3 files changed, 25 insertions(+), 5 deletions(-)
>

[...]

Re: [PATCH 02/11] bpf: Compile btfid tool at kernel compilation start

[Arnaldo Carvalho de Melo]

On June 18, 2020 9:40:32 PM GMT-03:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>>
>> The btfid tool will be used during the vmlinux linking,
>> so it's necessary it's ready for it.
>>
>
>Seeing troubles John runs into, I wonder if it maybe would be better
>to add it to pahole instead? It's already a dependency for anything
>BTF-related in the kernel. It has libelf, libbpf linked and set up.
>WDYT? I've cc'ed Arnaldo as well for an opinion.

I was reading this thread with a low prio, but my gut feeling was that yeah, since pahole is already there, why not have it do this?

I'll try to look at this tomorrow and see if this is more than just a hunch.

- Arnaldo


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 02/11] bpf: Compile btfid tool at kernel compilation start

[Alexei Starovoitov]

On Thu, Jun 18, 2020 at 5:47 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
>
>
> On June 18, 2020 9:40:32 PM GMT-03:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >>
> >> The btfid tool will be used during the vmlinux linking,
> >> so it's necessary it's ready for it.
> >>
> >
> >Seeing troubles John runs into, I wonder if it maybe would be better
> >to add it to pahole instead? It's already a dependency for anything
> >BTF-related in the kernel. It has libelf, libbpf linked and set up.
> >WDYT? I've cc'ed Arnaldo as well for an opinion.
>
> I was reading this thread with a low prio, but my gut feeling was that yeah, since pahole is already there, why not have it do this?
>
> I'll try to look at this tomorrow and see if this is more than just a hunch.

I think it's better to keep it separate like Jiri did.
It is really vmlinux specific as far as I can see and can change in the future.

Re: [PATCH 02/11] bpf: Compile btfid tool at kernel compilation start

[Andrii Nakryiko]

On Thu, Jun 18, 2020 at 7:08 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jun 18, 2020 at 5:47 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> >
> >
> > On June 18, 2020 9:40:32 PM GMT-03:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > >On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >>
> > >> The btfid tool will be used during the vmlinux linking,
> > >> so it's necessary it's ready for it.
> > >>
> > >
> > >Seeing troubles John runs into, I wonder if it maybe would be better
> > >to add it to pahole instead? It's already a dependency for anything
> > >BTF-related in the kernel. It has libelf, libbpf linked and set up.
> > >WDYT? I've cc'ed Arnaldo as well for an opinion.
> >
> > I was reading this thread with a low prio, but my gut feeling was that yeah, since pahole is already there, why not have it do this?
> >
> > I'll try to look at this tomorrow and see if this is more than just a hunch.
>
> I think it's better to keep it separate like Jiri did.
> It is really vmlinux specific as far as I can see and can change in the future.

Yeah, I actually agree, on the second though, no real reason to put it
in pahole.

[PATCH 03/11] bpf: Add btf_ids object

[Jiri Olsa]

Adding support to generate .BTF_ids section that would
hold various BTF IDs list for verifier.

Adding macros help to define lists of BTF IDs placed in
.BTF_ids section. They are initially filled with zeros
(during compilation) and resolved later during the
linking phase by btfid tool.

Following defines list of one BTF ID that is accessible
within kernel code as bpf_skb_output_btf_ids array.

  extern int bpf_skb_output_btf_ids[];

  BTF_ID_LIST(bpf_skb_output_btf_ids)
  BTF_ID(struct, sk_buff)

Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/asm-generic/vmlinux.lds.h |  4 ++
 kernel/bpf/Makefile               |  2 +-
 kernel/bpf/btf_ids.c              |  3 ++
 kernel/bpf/btf_ids.h              | 70 +++++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 kernel/bpf/btf_ids.c
 create mode 100644 kernel/bpf/btf_ids.h

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index db600ef218d7..0be2ee265931 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -641,6 +641,10 @@
 		__start_BTF = .;					\
 		*(.BTF)							\
 		__stop_BTF = .;						\
+	}								\
+	. = ALIGN(4);							\
+	.BTF_ids : AT(ADDR(.BTF_ids) - LOAD_OFFSET) {			\
+		*(.BTF_ids)						\
 	}
 #else
 #define BTF
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index 1131a921e1a6..21e4fc7c25ab 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -7,7 +7,7 @@ obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list
 obj-$(CONFIG_BPF_SYSCALL) += local_storage.o queue_stack_maps.o ringbuf.o
 obj-$(CONFIG_BPF_SYSCALL) += disasm.o
 obj-$(CONFIG_BPF_JIT) += trampoline.o
-obj-$(CONFIG_BPF_SYSCALL) += btf.o
+obj-$(CONFIG_BPF_SYSCALL) += btf.o btf_ids.o
 obj-$(CONFIG_BPF_JIT) += dispatcher.o
 ifeq ($(CONFIG_NET),y)
 obj-$(CONFIG_BPF_SYSCALL) += devmap.o
diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
new file mode 100644
index 000000000000..e7f9d94ad293
--- /dev/null
+++ b/kernel/bpf/btf_ids.c
@@ -0,0 +1,3 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "btf_ids.h"
diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h
new file mode 100644
index 000000000000..68aa5c38a37f
--- /dev/null
+++ b/kernel/bpf/btf_ids.h
@@ -0,0 +1,70 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __BTF_IDS_H__
+#define __BTF_IDS_H__
+
+#include <linux/stringify.h>
+#include <linux/compiler.h>
+#include <linux/linkage.h>
+
+
+/*
+ * Following macros help to define lists of BTF IDs placed
+ * in .BTF_ids section. They are initially filled with zeros
+ * (during compilation) and resolved later during the
+ * linking phase by btfid tool.
+ *
+ * Any change in list layout must be reflected in btfid
+ * tool logic.
+ */
+
+#define SECTION ".BTF_ids"
+
+#define ____BTF_ID(symbol)				\
+asm(							\
+".pushsection " SECTION ",\"a\";               \n"	\
+".local " #symbol " ;                          \n"	\
+".type  " #symbol ", @object;                  \n"	\
+".size  " #symbol ", 4;                        \n"	\
+#symbol ":                                     \n"	\
+".zero 4                                       \n"	\
+".popsection;                                  \n");
+
+#define __BTF_ID(...) \
+	____BTF_ID(__VA_ARGS__)
+
+#define __ID(prefix) \
+	__PASTE(prefix, __COUNTER__)
+
+
+/*
+ * The BTF_ID defines unique symbol for each ID pointing
+ * to 4 zero bytes.
+ */
+#define BTF_ID(prefix, name) \
+	__BTF_ID(__ID(__BTF_ID__##prefix##__##name##__))
+
+
+/*
+ * The BTF_ID_LIST macro defines pure (unsorted) list
+ * of BTF IDs, with following layout:
+ *
+ * BTF_ID_LIST(list1)
+ * BTF_ID(type1, name1)
+ * BTF_ID(type2, name2)
+ *
+ * list1:
+ * __BTF_ID__type1__name1__1:
+ * .zero 4
+ * __BTF_ID__type2__name2__2:
+ * .zero 4
+ *
+ */
+#define BTF_ID_LIST(name)				\
+asm(							\
+".pushsection " SECTION ",\"a\";               \n"	\
+".global " #name ";                            \n"	\
+#name ":;                                      \n"	\
+".popsection;                                  \n");
+
+#endif
-- 
2.25.4

Re: [PATCH 03/11] bpf: Add btf_ids object

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to generate .BTF_ids section that would
> hold various BTF IDs list for verifier.
>
> Adding macros help to define lists of BTF IDs placed in
> .BTF_ids section. They are initially filled with zeros
> (during compilation) and resolved later during the
> linking phase by btfid tool.
>
> Following defines list of one BTF ID that is accessible
> within kernel code as bpf_skb_output_btf_ids array.
>
>   extern int bpf_skb_output_btf_ids[];
>
>   BTF_ID_LIST(bpf_skb_output_btf_ids)
>   BTF_ID(struct, sk_buff)
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/asm-generic/vmlinux.lds.h |  4 ++
>  kernel/bpf/Makefile               |  2 +-
>  kernel/bpf/btf_ids.c              |  3 ++
>  kernel/bpf/btf_ids.h              | 70 +++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 1 deletion(-)
>  create mode 100644 kernel/bpf/btf_ids.c
>  create mode 100644 kernel/bpf/btf_ids.h
>

[...]

> +/*
> + * Following macros help to define lists of BTF IDs placed
> + * in .BTF_ids section. They are initially filled with zeros
> + * (during compilation) and resolved later during the
> + * linking phase by btfid tool.
> + *
> + * Any change in list layout must be reflected in btfid
> + * tool logic.
> + */
> +
> +#define SECTION ".BTF_ids"

nit: SECTION is super generic and non-greppable. BTF_IDS_SECTION?

> +
> +#define ____BTF_ID(symbol)                             \
> +asm(                                                   \
> +".pushsection " SECTION ",\"a\";               \n"     \

section should be also read-only? Either immediately here, of btfid
tool should mark it? Unless I missed that it's already doing it :)

> +".local " #symbol " ;                          \n"     \
> +".type  " #symbol ", @object;                  \n"     \
> +".size  " #symbol ", 4;                        \n"     \
> +#symbol ":                                     \n"     \
> +".zero 4                                       \n"     \
> +".popsection;                                  \n");
> +
> +#define __BTF_ID(...) \
> +       ____BTF_ID(__VA_ARGS__)

why varargs, if it's always a single argument? Or it's one of those
macro black magic things were it works only in this particular case,
but not others?


> +
> +#define __ID(prefix) \
> +       __PASTE(prefix, __COUNTER__)
> +
> +
> +/*
> + * The BTF_ID defines unique symbol for each ID pointing
> + * to 4 zero bytes.
> + */
> +#define BTF_ID(prefix, name) \
> +       __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__))
> +
> +
> +/*
> + * The BTF_ID_LIST macro defines pure (unsorted) list
> + * of BTF IDs, with following layout:
> + *
> + * BTF_ID_LIST(list1)
> + * BTF_ID(type1, name1)
> + * BTF_ID(type2, name2)
> + *
> + * list1:
> + * __BTF_ID__type1__name1__1:
> + * .zero 4
> + * __BTF_ID__type2__name2__2:
> + * .zero 4
> + *
> + */
> +#define BTF_ID_LIST(name)                              \

nit: btw, you call it a list here, but btfids tool talks about
"sorts". Maybe stick to consistent naming. Either "list" or "set"
seems to be appropriate. Set implies a sorted aspect a bit more, IMO.

> +asm(                                                   \
> +".pushsection " SECTION ",\"a\";               \n"     \
> +".global " #name ";                            \n"     \

I was expecting to see reserved 4 bytes for list size? I also couldn't
find where btfids tool prepends it. From what I could understand, it
just assumed the first 4 bytes are the length prefix? Sorry if I'm
slow...


> +#name ":;                                      \n"     \
> +".popsection;                                  \n");
> +
> +#endif
> --
> 2.25.4
>

Re: [PATCH 03/11] bpf: Add btf_ids object

[Andrii Nakryiko]

On Thu, Jun 18, 2020 at 5:56 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jun 16, 2020 at 3:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to generate .BTF_ids section that would
> > hold various BTF IDs list for verifier.
> >
> > Adding macros help to define lists of BTF IDs placed in
> > .BTF_ids section. They are initially filled with zeros
> > (during compilation) and resolved later during the
> > linking phase by btfid tool.
> >
> > Following defines list of one BTF ID that is accessible
> > within kernel code as bpf_skb_output_btf_ids array.
> >
> >   extern int bpf_skb_output_btf_ids[];
> >
> >   BTF_ID_LIST(bpf_skb_output_btf_ids)
> >   BTF_ID(struct, sk_buff)
> >
> > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/asm-generic/vmlinux.lds.h |  4 ++
> >  kernel/bpf/Makefile               |  2 +-
> >  kernel/bpf/btf_ids.c              |  3 ++
> >  kernel/bpf/btf_ids.h              | 70 +++++++++++++++++++++++++++++++
> >  4 files changed, 78 insertions(+), 1 deletion(-)
> >  create mode 100644 kernel/bpf/btf_ids.c
> >  create mode 100644 kernel/bpf/btf_ids.h
> >
>
> [...]
>
> > +/*
> > + * Following macros help to define lists of BTF IDs placed
> > + * in .BTF_ids section. They are initially filled with zeros
> > + * (during compilation) and resolved later during the
> > + * linking phase by btfid tool.
> > + *
> > + * Any change in list layout must be reflected in btfid
> > + * tool logic.
> > + */
> > +
> > +#define SECTION ".BTF_ids"
>
> nit: SECTION is super generic and non-greppable. BTF_IDS_SECTION?
>
> > +
> > +#define ____BTF_ID(symbol)                             \
> > +asm(                                                   \
> > +".pushsection " SECTION ",\"a\";               \n"     \
>
> section should be also read-only? Either immediately here, of btfid
> tool should mark it? Unless I missed that it's already doing it :)
>
> > +".local " #symbol " ;                          \n"     \
> > +".type  " #symbol ", @object;                  \n"     \
> > +".size  " #symbol ", 4;                        \n"     \
> > +#symbol ":                                     \n"     \
> > +".zero 4                                       \n"     \
> > +".popsection;                                  \n");
> > +
> > +#define __BTF_ID(...) \
> > +       ____BTF_ID(__VA_ARGS__)
>
> why varargs, if it's always a single argument? Or it's one of those
> macro black magic things were it works only in this particular case,
> but not others?
>
>
> > +
> > +#define __ID(prefix) \
> > +       __PASTE(prefix, __COUNTER__)
> > +
> > +
> > +/*
> > + * The BTF_ID defines unique symbol for each ID pointing
> > + * to 4 zero bytes.
> > + */
> > +#define BTF_ID(prefix, name) \
> > +       __BTF_ID(__ID(__BTF_ID__##prefix##__##name##__))
> > +
> > +
> > +/*
> > + * The BTF_ID_LIST macro defines pure (unsorted) list
> > + * of BTF IDs, with following layout:
> > + *
> > + * BTF_ID_LIST(list1)
> > + * BTF_ID(type1, name1)
> > + * BTF_ID(type2, name2)
> > + *
> > + * list1:
> > + * __BTF_ID__type1__name1__1:
> > + * .zero 4
> > + * __BTF_ID__type2__name2__2:
> > + * .zero 4
> > + *
> > + */
> > +#define BTF_ID_LIST(name)                              \
>
> nit: btw, you call it a list here, but btfids tool talks about
> "sorts". Maybe stick to consistent naming. Either "list" or "set"
> seems to be appropriate. Set implies a sorted aspect a bit more, IMO.
>
> > +asm(                                                   \
> > +".pushsection " SECTION ",\"a\";               \n"     \
> > +".global " #name ";                            \n"     \
>
> I was expecting to see reserved 4 bytes for list size? I also couldn't
> find where btfids tool prepends it. From what I could understand, it
> just assumed the first 4 bytes are the length prefix? Sorry if I'm
> slow...

Never mind, this is different from whitelisting you do in patch #8.
But now I'm curious how this list symbol gets its size correctly
calculated?..

>
>
> > +#name ":;                                      \n"     \
> > +".popsection;                                  \n");
> > +
> > +#endif
> > --
> > 2.25.4
> >

Re: [PATCH 03/11] bpf: Add btf_ids object

[Jiri Olsa]

On Thu, Jun 18, 2020 at 06:06:49PM -0700, Andrii Nakryiko wrote:

SNIP

> > > +/*
> > > + * The BTF_ID_LIST macro defines pure (unsorted) list
> > > + * of BTF IDs, with following layout:
> > > + *
> > > + * BTF_ID_LIST(list1)
> > > + * BTF_ID(type1, name1)
> > > + * BTF_ID(type2, name2)
> > > + *
> > > + * list1:
> > > + * __BTF_ID__type1__name1__1:
> > > + * .zero 4
> > > + * __BTF_ID__type2__name2__2:
> > > + * .zero 4
> > > + *
> > > + */
> > > +#define BTF_ID_LIST(name)                              \
> >
> > nit: btw, you call it a list here, but btfids tool talks about
> > "sorts". Maybe stick to consistent naming. Either "list" or "set"
> > seems to be appropriate. Set implies a sorted aspect a bit more, IMO.

so how about we keep BTF_ID_LIST as it is and rename
BTF_WHITELIST_* to BTF_SET_*

> >
> > > +asm(                                                   \
> > > +".pushsection " SECTION ",\"a\";               \n"     \
> > > +".global " #name ";                            \n"     \
> >
> > I was expecting to see reserved 4 bytes for list size? I also couldn't
> > find where btfids tool prepends it. From what I could understand, it
> > just assumed the first 4 bytes are the length prefix? Sorry if I'm
> > slow...
> 
> Never mind, this is different from whitelisting you do in patch #8.
> But now I'm curious how this list symbol gets its size correctly
> calculated?..

so the BTF_ID_LIST list does not care about the size,
each symbol in the 'list' gets resolved based on its
__BTF_ID__XX__symbol__XX symbol

the count is kept in BTF_WHITELIST_* list because we
need it to sort it and search in it

thanks,
jirka

Re: [PATCH 03/11] bpf: Add btf_ids object

[Jiri Olsa]

On Thu, Jun 18, 2020 at 05:56:38PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 16, 2020 at 3:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to generate .BTF_ids section that would
> > hold various BTF IDs list for verifier.
> >
> > Adding macros help to define lists of BTF IDs placed in
> > .BTF_ids section. They are initially filled with zeros
> > (during compilation) and resolved later during the
> > linking phase by btfid tool.
> >
> > Following defines list of one BTF ID that is accessible
> > within kernel code as bpf_skb_output_btf_ids array.
> >
> >   extern int bpf_skb_output_btf_ids[];
> >
> >   BTF_ID_LIST(bpf_skb_output_btf_ids)
> >   BTF_ID(struct, sk_buff)
> >
> > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/asm-generic/vmlinux.lds.h |  4 ++
> >  kernel/bpf/Makefile               |  2 +-
> >  kernel/bpf/btf_ids.c              |  3 ++
> >  kernel/bpf/btf_ids.h              | 70 +++++++++++++++++++++++++++++++
> >  4 files changed, 78 insertions(+), 1 deletion(-)
> >  create mode 100644 kernel/bpf/btf_ids.c
> >  create mode 100644 kernel/bpf/btf_ids.h
> >
> 
> [...]
> 
> > +/*
> > + * Following macros help to define lists of BTF IDs placed
> > + * in .BTF_ids section. They are initially filled with zeros
> > + * (during compilation) and resolved later during the
> > + * linking phase by btfid tool.
> > + *
> > + * Any change in list layout must be reflected in btfid
> > + * tool logic.
> > + */
> > +
> > +#define SECTION ".BTF_ids"
> 
> nit: SECTION is super generic and non-greppable. BTF_IDS_SECTION?

ok

> 
> > +
> > +#define ____BTF_ID(symbol)                             \
> > +asm(                                                   \
> > +".pushsection " SECTION ",\"a\";               \n"     \
> 
> section should be also read-only? Either immediately here, of btfid
> tool should mark it? Unless I missed that it's already doing it :)

hm, it's there next to the .BTF section within RO_DATA macro,
so I thought that was enough.. I'll double check

> 
> > +".local " #symbol " ;                          \n"     \
> > +".type  " #symbol ", @object;                  \n"     \
> > +".size  " #symbol ", 4;                        \n"     \
> > +#symbol ":                                     \n"     \
> > +".zero 4                                       \n"     \
> > +".popsection;                                  \n");
> > +
> > +#define __BTF_ID(...) \
> > +       ____BTF_ID(__VA_ARGS__)
> 
> why varargs, if it's always a single argument? Or it's one of those
> macro black magic things were it works only in this particular case,
> but not others?

yea, I kind of struggled in here, because any other would not
expand the name concat together with the unique ID bit,
__VA_ARGS__ did it nicely ;-) I'll revisit this

thanks,
jirka

Re: [PATCH 03/11] bpf: Add btf_ids object

[Andrii Nakryiko]

On Fri, Jun 19, 2020 at 6:13 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Jun 18, 2020 at 05:56:38PM -0700, Andrii Nakryiko wrote:
> > On Tue, Jun 16, 2020 at 3:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding support to generate .BTF_ids section that would
> > > hold various BTF IDs list for verifier.
> > >
> > > Adding macros help to define lists of BTF IDs placed in
> > > .BTF_ids section. They are initially filled with zeros
> > > (during compilation) and resolved later during the
> > > linking phase by btfid tool.
> > >
> > > Following defines list of one BTF ID that is accessible
> > > within kernel code as bpf_skb_output_btf_ids array.
> > >
> > >   extern int bpf_skb_output_btf_ids[];
> > >
> > >   BTF_ID_LIST(bpf_skb_output_btf_ids)
> > >   BTF_ID(struct, sk_buff)
> > >
> > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  include/asm-generic/vmlinux.lds.h |  4 ++
> > >  kernel/bpf/Makefile               |  2 +-
> > >  kernel/bpf/btf_ids.c              |  3 ++
> > >  kernel/bpf/btf_ids.h              | 70 +++++++++++++++++++++++++++++++
> > >  4 files changed, 78 insertions(+), 1 deletion(-)
> > >  create mode 100644 kernel/bpf/btf_ids.c
> > >  create mode 100644 kernel/bpf/btf_ids.h
> > >
> >
> > [...]
> >
> > > +/*
> > > + * Following macros help to define lists of BTF IDs placed
> > > + * in .BTF_ids section. They are initially filled with zeros
> > > + * (during compilation) and resolved later during the
> > > + * linking phase by btfid tool.
> > > + *
> > > + * Any change in list layout must be reflected in btfid
> > > + * tool logic.
> > > + */
> > > +
> > > +#define SECTION ".BTF_ids"
> >
> > nit: SECTION is super generic and non-greppable. BTF_IDS_SECTION?
>
> ok
>
> >
> > > +
> > > +#define ____BTF_ID(symbol)                             \
> > > +asm(                                                   \
> > > +".pushsection " SECTION ",\"a\";               \n"     \
> >
> > section should be also read-only? Either immediately here, of btfid
> > tool should mark it? Unless I missed that it's already doing it :)
>
> hm, it's there next to the .BTF section within RO_DATA macro,
> so I thought that was enough.. I'll double check

ah, linker script magic, got it

>
> >
> > > +".local " #symbol " ;                          \n"     \
> > > +".type  " #symbol ", @object;                  \n"     \
> > > +".size  " #symbol ", 4;                        \n"     \
> > > +#symbol ":                                     \n"     \
> > > +".zero 4                                       \n"     \
> > > +".popsection;                                  \n");
> > > +
> > > +#define __BTF_ID(...) \
> > > +       ____BTF_ID(__VA_ARGS__)
> >
> > why varargs, if it's always a single argument? Or it's one of those
> > macro black magic things were it works only in this particular case,
> > but not others?
>
> yea, I kind of struggled in here, because any other would not
> expand the name concat together with the unique ID bit,
> __VA_ARGS__ did it nicely ;-) I'll revisit this

it's probably not varargs, but rather nested macro call. Macros are
weird and tricky...

>
> thanks,
> jirka
>

Re: [PATCH 03/11] bpf: Add btf_ids object

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:05 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to generate .BTF_ids section that would
> hold various BTF IDs list for verifier.
>
> Adding macros help to define lists of BTF IDs placed in
> .BTF_ids section. They are initially filled with zeros
> (during compilation) and resolved later during the
> linking phase by btfid tool.
>
> Following defines list of one BTF ID that is accessible
> within kernel code as bpf_skb_output_btf_ids array.
>
>   extern int bpf_skb_output_btf_ids[];
>
>   BTF_ID_LIST(bpf_skb_output_btf_ids)
>   BTF_ID(struct, sk_buff)
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/asm-generic/vmlinux.lds.h |  4 ++
>  kernel/bpf/Makefile               |  2 +-
>  kernel/bpf/btf_ids.c              |  3 ++
>  kernel/bpf/btf_ids.h              | 70 +++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 1 deletion(-)
>  create mode 100644 kernel/bpf/btf_ids.c
>  create mode 100644 kernel/bpf/btf_ids.h
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index db600ef218d7..0be2ee265931 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -641,6 +641,10 @@
>                 __start_BTF = .;                                        \
>                 *(.BTF)                                                 \
>                 __stop_BTF = .;                                         \
> +       }                                                               \
> +       . = ALIGN(4);                                                   \
> +       .BTF_ids : AT(ADDR(.BTF_ids) - LOAD_OFFSET) {                   \
> +               *(.BTF_ids)                                             \
>         }
>  #else
>  #define BTF
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 1131a921e1a6..21e4fc7c25ab 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -7,7 +7,7 @@ obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list
>  obj-$(CONFIG_BPF_SYSCALL) += local_storage.o queue_stack_maps.o ringbuf.o
>  obj-$(CONFIG_BPF_SYSCALL) += disasm.o
>  obj-$(CONFIG_BPF_JIT) += trampoline.o
> -obj-$(CONFIG_BPF_SYSCALL) += btf.o
> +obj-$(CONFIG_BPF_SYSCALL) += btf.o btf_ids.o
>  obj-$(CONFIG_BPF_JIT) += dispatcher.o
>  ifeq ($(CONFIG_NET),y)
>  obj-$(CONFIG_BPF_SYSCALL) += devmap.o
> diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> new file mode 100644
> index 000000000000..e7f9d94ad293
> --- /dev/null
> +++ b/kernel/bpf/btf_ids.c
> @@ -0,0 +1,3 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include "btf_ids.h"

hm... what's the purpose of this btf_ids.c file?

> diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h
> new file mode 100644
> index 000000000000..68aa5c38a37f
> --- /dev/null
> +++ b/kernel/bpf/btf_ids.h

[...]

Re: [PATCH 03/11] bpf: Add btf_ids object

[Jiri Olsa]

On Thu, Jun 18, 2020 at 06:02:48PM -0700, Andrii Nakryiko wrote:

SNIP

> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > index db600ef218d7..0be2ee265931 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -641,6 +641,10 @@
> >                 __start_BTF = .;                                        \
> >                 *(.BTF)                                                 \
> >                 __stop_BTF = .;                                         \
> > +       }                                                               \
> > +       . = ALIGN(4);                                                   \
> > +       .BTF_ids : AT(ADDR(.BTF_ids) - LOAD_OFFSET) {                   \
> > +               *(.BTF_ids)                                             \
> >         }
> >  #else
> >  #define BTF
> > diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> > index 1131a921e1a6..21e4fc7c25ab 100644
> > --- a/kernel/bpf/Makefile
> > +++ b/kernel/bpf/Makefile
> > @@ -7,7 +7,7 @@ obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list
> >  obj-$(CONFIG_BPF_SYSCALL) += local_storage.o queue_stack_maps.o ringbuf.o
> >  obj-$(CONFIG_BPF_SYSCALL) += disasm.o
> >  obj-$(CONFIG_BPF_JIT) += trampoline.o
> > -obj-$(CONFIG_BPF_SYSCALL) += btf.o
> > +obj-$(CONFIG_BPF_SYSCALL) += btf.o btf_ids.o
> >  obj-$(CONFIG_BPF_JIT) += dispatcher.o
> >  ifeq ($(CONFIG_NET),y)
> >  obj-$(CONFIG_BPF_SYSCALL) += devmap.o
> > diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> > new file mode 100644
> > index 000000000000..e7f9d94ad293
> > --- /dev/null
> > +++ b/kernel/bpf/btf_ids.c
> > @@ -0,0 +1,3 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include "btf_ids.h"
> 
> hm... what's the purpose of this btf_ids.c file?

I put all the lists in here.. I can add it in that patch later on

jirka

> 
> > diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h
> > new file mode 100644
> > index 000000000000..68aa5c38a37f
> > --- /dev/null
> > +++ b/kernel/bpf/btf_ids.h
> 
> [...]
> 

[PATCH 04/11] bpf: Resolve BTF IDs in vmlinux image

[Jiri Olsa]

Run btfid on vmlinux object during linking, so the
.BTF_ids section is processed and IDs are resolved.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 Makefile                 |  3 ++-
 include/linux/bpf.h      |  5 +++++
 kernel/bpf/btf_ids.c     | 12 ++++++++++++
 kernel/trace/bpf_trace.c |  2 --
 net/core/filter.c        |  2 --
 scripts/link-vmlinux.sh  |  6 ++++++
 6 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index b190d502d7d7..889d909fd71a 100644
--- a/Makefile
+++ b/Makefile
@@ -448,6 +448,7 @@ OBJSIZE		= $(CROSS_COMPILE)size
 STRIP		= $(CROSS_COMPILE)strip
 endif
 PAHOLE		= pahole
+BTFID		= $(srctree)/tools/bpf/btfid/btfid
 LEX		= flex
 YACC		= bison
 AWK		= awk
@@ -524,7 +525,7 @@ GCC_PLUGINS_CFLAGS :=
 CLANG_FLAGS :=
 
 export ARCH SRCARCH CONFIG_SHELL BASH HOSTCC KBUILD_HOSTCFLAGS CROSS_COMPILE LD CC
-export CPP AR NM STRIP OBJCOPY OBJDUMP OBJSIZE READELF PAHOLE LEX YACC AWK INSTALLKERNEL
+export CPP AR NM STRIP OBJCOPY OBJDUMP OBJSIZE READELF PAHOLE BTFID LEX YACC AWK INSTALLKERNEL
 export PERL PYTHON PYTHON3 CHECK CHECKFLAGS MAKE UTS_MACHINE HOSTCXX
 export _GZIP _BZIP2 _LZOP LZMA LZ4 XZ
 export KBUILD_HOSTCXXFLAGS KBUILD_HOSTLDFLAGS KBUILD_HOSTLDLIBS LDFLAGS_MODULE
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 07052d44bca1..f18c23dcc858 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1743,4 +1743,9 @@ enum bpf_text_poke_type {
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
 		       void *addr1, void *addr2);
 
+extern int bpf_skb_output_btf_ids[];
+extern int bpf_seq_printf_btf_ids[];
+extern int bpf_seq_write_btf_ids[];
+extern int bpf_xdp_output_btf_ids[];
+
 #endif /* _LINUX_BPF_H */
diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
index e7f9d94ad293..d8d0df162f04 100644
--- a/kernel/bpf/btf_ids.c
+++ b/kernel/bpf/btf_ids.c
@@ -1,3 +1,15 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #include "btf_ids.h"
+
+BTF_ID_LIST(bpf_skb_output_btf_ids)
+BTF_ID(struct, sk_buff)
+
+BTF_ID_LIST(bpf_seq_printf_btf_ids)
+BTF_ID(struct, seq_file)
+
+BTF_ID_LIST(bpf_seq_write_btf_ids)
+BTF_ID(struct, seq_file)
+
+BTF_ID_LIST(bpf_xdp_output_btf_ids)
+BTF_ID(struct, xdp_buff)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 3744372a24e2..c1866d76041f 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -667,7 +667,6 @@ BPF_CALL_5(bpf_seq_printf, struct seq_file *, m, char *, fmt, u32, fmt_size,
 	return err;
 }
 
-static int bpf_seq_printf_btf_ids[5];
 static const struct bpf_func_proto bpf_seq_printf_proto = {
 	.func		= bpf_seq_printf,
 	.gpl_only	= true,
@@ -685,7 +684,6 @@ BPF_CALL_3(bpf_seq_write, struct seq_file *, m, const void *, data, u32, len)
 	return seq_write(m, data, len) ? -EOVERFLOW : 0;
 }
 
-static int bpf_seq_write_btf_ids[5];
 static const struct bpf_func_proto bpf_seq_write_proto = {
 	.func		= bpf_seq_write,
 	.gpl_only	= true,
diff --git a/net/core/filter.c b/net/core/filter.c
index 209482a4eaa2..440e52061be8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3775,7 +3775,6 @@ static const struct bpf_func_proto bpf_skb_event_output_proto = {
 	.arg5_type	= ARG_CONST_SIZE_OR_ZERO,
 };
 
-static int bpf_skb_output_btf_ids[5];
 const struct bpf_func_proto bpf_skb_output_proto = {
 	.func		= bpf_skb_event_output,
 	.gpl_only	= true,
@@ -4169,7 +4168,6 @@ static const struct bpf_func_proto bpf_xdp_event_output_proto = {
 	.arg5_type	= ARG_CONST_SIZE_OR_ZERO,
 };
 
-static int bpf_xdp_output_btf_ids[5];
 const struct bpf_func_proto bpf_xdp_output_proto = {
 	.func		= bpf_xdp_event_output,
 	.gpl_only	= true,
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 57cb14bd8925..99a3f8c65e84 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -336,6 +336,12 @@ fi
 
 vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o}
 
+# fill in BTF IDs
+if [ -n "${CONFIG_DEBUG_INFO_BTF}" ]; then
+info BTFID vmlinux
+${BTFID} vmlinux
+fi
+
 if [ -n "${CONFIG_BUILDTIME_TABLE_SORT}" ]; then
 	info SORTTAB vmlinux
 	if ! sorttable vmlinux; then
-- 
2.25.4

[PATCH 05/11] bpf: Remove btf_id helpers resolving

[Jiri Olsa]

Now when we moved the helpers btf_id into .BTF_ids section,
we can remove the code that resolve those IDs in runtime.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/btf.c | 88 +++---------------------------------------------
 1 file changed, 4 insertions(+), 84 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 58c9af1d4808..aea7b2cc8d26 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4049,96 +4049,16 @@ int btf_struct_access(struct bpf_verifier_log *log,
 	return -EINVAL;
 }
 
-static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
-				   int arg)
-{
-	char fnname[KSYM_SYMBOL_LEN + 4] = "btf_";
-	const struct btf_param *args;
-	const struct btf_type *t;
-	const char *tname, *sym;
-	u32 btf_id, i;
-
-	if (IS_ERR(btf_vmlinux)) {
-		bpf_log(log, "btf_vmlinux is malformed\n");
-		return -EINVAL;
-	}
-
-	sym = kallsyms_lookup((long)fn, NULL, NULL, NULL, fnname + 4);
-	if (!sym) {
-		bpf_log(log, "kernel doesn't have kallsyms\n");
-		return -EFAULT;
-	}
-
-	for (i = 1; i <= btf_vmlinux->nr_types; i++) {
-		t = btf_type_by_id(btf_vmlinux, i);
-		if (BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF)
-			continue;
-		tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
-		if (!strcmp(tname, fnname))
-			break;
-	}
-	if (i > btf_vmlinux->nr_types) {
-		bpf_log(log, "helper %s type is not found\n", fnname);
-		return -ENOENT;
-	}
-
-	t = btf_type_by_id(btf_vmlinux, t->type);
-	if (!btf_type_is_ptr(t))
-		return -EFAULT;
-	t = btf_type_by_id(btf_vmlinux, t->type);
-	if (!btf_type_is_func_proto(t))
-		return -EFAULT;
-
-	args = (const struct btf_param *)(t + 1);
-	if (arg >= btf_type_vlen(t)) {
-		bpf_log(log, "bpf helper %s doesn't have %d-th argument\n",
-			fnname, arg);
-		return -EINVAL;
-	}
-
-	t = btf_type_by_id(btf_vmlinux, args[arg].type);
-	if (!btf_type_is_ptr(t) || !t->type) {
-		/* anything but the pointer to struct is a helper config bug */
-		bpf_log(log, "ARG_PTR_TO_BTF is misconfigured\n");
-		return -EFAULT;
-	}
-	btf_id = t->type;
-	t = btf_type_by_id(btf_vmlinux, t->type);
-	/* skip modifiers */
-	while (btf_type_is_modifier(t)) {
-		btf_id = t->type;
-		t = btf_type_by_id(btf_vmlinux, t->type);
-	}
-	if (!btf_type_is_struct(t)) {
-		bpf_log(log, "ARG_PTR_TO_BTF is not a struct\n");
-		return -EFAULT;
-	}
-	bpf_log(log, "helper %s arg%d has btf_id %d struct %s\n", fnname + 4,
-		arg, btf_id, __btf_name_by_offset(btf_vmlinux, t->name_off));
-	return btf_id;
-}
-
 int btf_resolve_helper_id(struct bpf_verifier_log *log,
 			  const struct bpf_func_proto *fn, int arg)
 {
-	int *btf_id = &fn->btf_id[arg];
-	int ret;
-
 	if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
 		return -EINVAL;
 
-	ret = READ_ONCE(*btf_id);
-	if (ret)
-		return ret;
-	/* ok to race the search. The result is the same */
-	ret = __btf_resolve_helper_id(log, fn->func, arg);
-	if (!ret) {
-		/* Function argument cannot be type 'void' */
-		bpf_log(log, "BTF resolution bug\n");
-		return -EFAULT;
-	}
-	WRITE_ONCE(*btf_id, ret);
-	return ret;
+	if (WARN_ON_ONCE(!fn->btf_id))
+		return -EINVAL;
+
+	return fn->btf_id[arg];
 }
 
 static int __get_type_size(struct btf *btf, u32 btf_id,
-- 
2.25.4

Re: [PATCH 05/11] bpf: Remove btf_id helpers resolving

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Now when we moved the helpers btf_id into .BTF_ids section,
> we can remove the code that resolve those IDs in runtime.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Nice! :)

BTW, have you looked at bpf_ctx_convert stuff? Would we be able to
replace it with your btfids thing as well?


>  kernel/bpf/btf.c | 88 +++---------------------------------------------
>  1 file changed, 4 insertions(+), 84 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 58c9af1d4808..aea7b2cc8d26 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4049,96 +4049,16 @@ int btf_struct_access(struct bpf_verifier_log *log,
>         return -EINVAL;
>  }
>

[...]

>  int btf_resolve_helper_id(struct bpf_verifier_log *log,
>                           const struct bpf_func_proto *fn, int arg)
>  {
> -       int *btf_id = &fn->btf_id[arg];
> -       int ret;
> -
>         if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
>                 return -EINVAL;
>
> -       ret = READ_ONCE(*btf_id);
> -       if (ret)
> -               return ret;
> -       /* ok to race the search. The result is the same */
> -       ret = __btf_resolve_helper_id(log, fn->func, arg);
> -       if (!ret) {
> -               /* Function argument cannot be type 'void' */
> -               bpf_log(log, "BTF resolution bug\n");
> -               return -EFAULT;
> -       }
> -       WRITE_ONCE(*btf_id, ret);
> -       return ret;
> +       if (WARN_ON_ONCE(!fn->btf_id))
> +               return -EINVAL;
> +
> +       return fn->btf_id[arg];

It probably would be a good idea to add some sanity checking here,
making sure that btf_id is >0 (void is never a right type) and <=
nr_types in vmlinux_btf?

>  }
>
>  static int __get_type_size(struct btf *btf, u32 btf_id,
> --
> 2.25.4
>

Re: [PATCH 05/11] bpf: Remove btf_id helpers resolving

[Jiri Olsa]

On Thu, Jun 18, 2020 at 06:10:29PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Now when we moved the helpers btf_id into .BTF_ids section,
> > we can remove the code that resolve those IDs in runtime.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> Nice! :)
> 
> BTW, have you looked at bpf_ctx_convert stuff? Would we be able to
> replace it with your btfids thing as well?

good, another usage ;-) I'll check

> 
> 
> >  kernel/bpf/btf.c | 88 +++---------------------------------------------
> >  1 file changed, 4 insertions(+), 84 deletions(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 58c9af1d4808..aea7b2cc8d26 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -4049,96 +4049,16 @@ int btf_struct_access(struct bpf_verifier_log *log,
> >         return -EINVAL;
> >  }
> >
> 
> [...]
> 
> >  int btf_resolve_helper_id(struct bpf_verifier_log *log,
> >                           const struct bpf_func_proto *fn, int arg)
> >  {
> > -       int *btf_id = &fn->btf_id[arg];
> > -       int ret;
> > -
> >         if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
> >                 return -EINVAL;
> >
> > -       ret = READ_ONCE(*btf_id);
> > -       if (ret)
> > -               return ret;
> > -       /* ok to race the search. The result is the same */
> > -       ret = __btf_resolve_helper_id(log, fn->func, arg);
> > -       if (!ret) {
> > -               /* Function argument cannot be type 'void' */
> > -               bpf_log(log, "BTF resolution bug\n");
> > -               return -EFAULT;
> > -       }
> > -       WRITE_ONCE(*btf_id, ret);
> > -       return ret;
> > +       if (WARN_ON_ONCE(!fn->btf_id))
> > +               return -EINVAL;
> > +
> > +       return fn->btf_id[arg];
> 
> It probably would be a good idea to add some sanity checking here,
> making sure that btf_id is >0 (void is never a right type) and <=
> nr_types in vmlinux_btf?

yep, will add it ;-)

jirka

[PATCH 06/11] bpf: Do not pass enum bpf_access_type to btf_struct_access

[Jiri Olsa]

There's no need for it.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h   | 1 -
 kernel/bpf/btf.c      | 3 +--
 kernel/bpf/verifier.c | 2 +-
 net/ipv4/bpf_tcp_ca.c | 2 +-
 4 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f18c23dcc858..b7d3b5f3dc09 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1282,7 +1282,6 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		    struct bpf_insn_access_aux *info);
 int btf_struct_access(struct bpf_verifier_log *log,
 		      const struct btf_type *t, int off, int size,
-		      enum bpf_access_type atype,
 		      u32 *next_btf_id);
 int btf_resolve_helper_id(struct bpf_verifier_log *log,
 			  const struct bpf_func_proto *fn, int);
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index aea7b2cc8d26..304369a4c2e2 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3831,7 +3831,6 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 
 int btf_struct_access(struct bpf_verifier_log *log,
 		      const struct btf_type *t, int off, int size,
-		      enum bpf_access_type atype,
 		      u32 *next_btf_id)
 {
 	u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
@@ -3880,7 +3879,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 			goto error;
 
 		off = (off - moff) % elem_type->size;
-		return btf_struct_access(log, elem_type, off, size, atype,
+		return btf_struct_access(log, elem_type, off, size,
 					 next_btf_id);
 
 error:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5c7bbaac81ef..b553e4523bd3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3175,7 +3175,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 			return -EACCES;
 		}
 
-		ret = btf_struct_access(&env->log, t, off, size, atype,
+		ret = btf_struct_access(&env->log, t, off, size,
 					&btf_id);
 	}
 
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index e3939f76b024..c6aab9389ac4 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -130,7 +130,7 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
 	size_t end;
 
 	if (atype == BPF_READ)
-		return btf_struct_access(log, t, off, size, atype, next_btf_id);
+		return btf_struct_access(log, t, off, size, next_btf_id);
 
 	if (t != tcp_sock_type) {
 		bpf_log(log, "only read is supported\n");
-- 
2.25.4

Re: [PATCH 06/11] bpf: Do not pass enum bpf_access_type to btf_struct_access

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> There's no need for it.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

It matches bpf_verifier_ops->btf_struct_access, though, which, I
think, actually allows write access for some special cases. So I think
we should keep it.

>  include/linux/bpf.h   | 1 -
>  kernel/bpf/btf.c      | 3 +--
>  kernel/bpf/verifier.c | 2 +-
>  net/ipv4/bpf_tcp_ca.c | 2 +-
>  4 files changed, 3 insertions(+), 5 deletions(-)
>

[...]

Re: [PATCH 06/11] bpf: Do not pass enum bpf_access_type to btf_struct_access

[Jiri Olsa]

On Thu, Jun 18, 2020 at 08:58:06PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > There's no need for it.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> It matches bpf_verifier_ops->btf_struct_access, though, which, I
> think, actually allows write access for some special cases. So I think
> we should keep it.

ok, will keep it

jirka

> 
> >  include/linux/bpf.h   | 1 -
> >  kernel/bpf/btf.c      | 3 +--
> >  kernel/bpf/verifier.c | 2 +-
> >  net/ipv4/bpf_tcp_ca.c | 2 +-
> >  4 files changed, 3 insertions(+), 5 deletions(-)
> >
> 
> [...]
> 

[PATCH 07/11] bpf: Allow nested BTF object to be refferenced by BTF object + offset

[Jiri Olsa]

Adding btf_struct_address function that takes 2 BTF objects
and offset as arguments and checks whether object A is nested
in object B on given offset.

This function is be used when checking the helper function
PTR_TO_BTF_ID arguments. If the argument has an offset value,
the btf_struct_address will check if the final address is
the expected BTF ID.

This way we can access nested BTF objects under PTR_TO_BTF_ID
pointer type and pass them to helpers, while they still point
to valid kernel BTF objects.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h   |  3 +++
 kernel/bpf/btf.c      | 63 ++++++++++++++++++++++++++++++++++++++-----
 kernel/bpf/verifier.c | 32 ++++++++++++++--------
 3 files changed, 81 insertions(+), 17 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b7d3b5f3dc09..e98c113a5d27 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1283,6 +1283,9 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 int btf_struct_access(struct bpf_verifier_log *log,
 		      const struct btf_type *t, int off, int size,
 		      u32 *next_btf_id);
+int btf_struct_address(struct bpf_verifier_log *log,
+		     const struct btf_type *t,
+		     u32 off, u32 id);
 int btf_resolve_helper_id(struct bpf_verifier_log *log,
 			  const struct bpf_func_proto *fn, int);
 
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 304369a4c2e2..6924180a19c4 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3829,9 +3829,22 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	return true;
 }
 
-int btf_struct_access(struct bpf_verifier_log *log,
-		      const struct btf_type *t, int off, int size,
-		      u32 *next_btf_id)
+enum access_op {
+	ACCESS_NEXT,
+	ACCESS_EXPECT,
+};
+
+struct access_data {
+	enum access_op op;
+	union {
+		u32 *next_btf_id;
+		const struct btf_type *exp_type;
+	};
+};
+
+static int struct_access(struct bpf_verifier_log *log,
+			 const struct btf_type *t, int off, int size,
+			 struct access_data *data)
 {
 	u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
 	const struct btf_type *mtype, *elem_type = NULL;
@@ -3879,8 +3892,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 			goto error;
 
 		off = (off - moff) % elem_type->size;
-		return btf_struct_access(log, elem_type, off, size,
-					 next_btf_id);
+		return struct_access(log, elem_type, off, size, data);
 
 error:
 		bpf_log(log, "access beyond struct %s at off %u size %u\n",
@@ -4008,9 +4020,21 @@ int btf_struct_access(struct bpf_verifier_log *log,
 
 			/* adjust offset we're looking for */
 			off -= moff;
+
+			/* We are nexting into another struct,
+			 * check if we are crossing expected ID.
+			 */
+			if (data->op == ACCESS_EXPECT && !off && t == data->exp_type)
+				return 0;
 			goto again;
 		}
 
+		/* We are interested only in structs for expected ID,
+		 * bail out.
+		 */
+		if (data->op == ACCESS_EXPECT)
+			return -EINVAL;
+
 		if (btf_type_is_ptr(mtype)) {
 			const struct btf_type *stype;
 			u32 id;
@@ -4024,7 +4048,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 
 			stype = btf_type_skip_modifiers(btf_vmlinux, mtype->type, &id);
 			if (btf_type_is_struct(stype)) {
-				*next_btf_id = id;
+				*data->next_btf_id = id;
 				return PTR_TO_BTF_ID;
 			}
 		}
@@ -4048,6 +4072,33 @@ int btf_struct_access(struct bpf_verifier_log *log,
 	return -EINVAL;
 }
 
+int btf_struct_access(struct bpf_verifier_log *log,
+		      const struct btf_type *t, int off, int size,
+		      u32 *next_btf_id)
+{
+	struct access_data data = {
+		.op = ACCESS_NEXT,
+		.next_btf_id = next_btf_id,
+	};
+
+	return struct_access(log, t, off, size, &data);
+}
+
+int btf_struct_address(struct bpf_verifier_log *log,
+		       const struct btf_type *t,
+		       u32 off, u32 id)
+{
+	struct access_data data = { .op = ACCESS_EXPECT };
+	const struct btf_type *type;
+
+	type = btf_type_by_id(btf_vmlinux, id);
+	if (!type)
+		return -EINVAL;
+
+	data.exp_type = type;
+	return struct_access(log, t, off, 1, &data);
+}
+
 int btf_resolve_helper_id(struct bpf_verifier_log *log,
 			  const struct bpf_func_proto *fn, int arg)
 {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b553e4523bd3..bee3da2cd945 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3741,6 +3741,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
 {
 	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
 	enum bpf_reg_type expected_type, type = reg->type;
+	const struct btf_type *btf_type;
 	int err = 0;
 
 	if (arg_type == ARG_DONTCARE)
@@ -3820,17 +3821,26 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
 		expected_type = PTR_TO_BTF_ID;
 		if (type != expected_type)
 			goto err_type;
-		if (reg->btf_id != meta->btf_id) {
-			verbose(env, "Helper has type %s got %s in R%d\n",
-				kernel_type_name(meta->btf_id),
-				kernel_type_name(reg->btf_id), regno);
-
-			return -EACCES;
-		}
-		if (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off) {
-			verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
-				regno);
-			return -EACCES;
+		if (reg->off) {
+			btf_type = btf_type_by_id(btf_vmlinux, reg->btf_id);
+			if (btf_struct_address(&env->log, btf_type, reg->off, meta->btf_id)) {
+				verbose(env, "Helper has type %s got %s in R%d, off %d\n",
+					kernel_type_name(meta->btf_id),
+					kernel_type_name(reg->btf_id), regno, reg->off);
+				return -EACCES;
+			}
+		} else {
+			if (reg->btf_id != meta->btf_id) {
+				verbose(env, "Helper has type %s got %s in R%d\n",
+					kernel_type_name(meta->btf_id),
+					kernel_type_name(reg->btf_id), regno);
+				return -EACCES;
+			}
+			if (!tnum_is_const(reg->var_off) || reg->var_off.value) {
+				verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
+					regno);
+				return -EACCES;
+			}
 		}
 	} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
 		if (meta->func_id == BPF_FUNC_spin_lock) {
-- 
2.25.4

[PATCH 08/11] bpf: Add BTF whitelist support

[Jiri Olsa]

Adding support to define 'whitelist' of BTF IDs, which is
also sorted.

Following defines sorted list of BTF IDs that is accessible
within kernel code as btf_whitelist_d_path and its count is
in btf_whitelist_d_path_cnt variable.

  extern int btf_whitelist_d_path[];
  extern int btf_whitelist_d_path_cnt;

  BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
  BTF_ID(func, vfs_truncate)
  BTF_ID(func, vfs_fallocate)
  BTF_ID(func, dentry_open)
  BTF_ID(func, vfs_getattr)
  BTF_ID(func, filp_close)
  BTF_WHITELIST_END(btf_whitelist_d_path)

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h   |  3 +++
 kernel/bpf/btf.c      | 13 +++++++++++++
 kernel/bpf/btf_ids.h  | 38 ++++++++++++++++++++++++++++++++++++++
 kernel/bpf/verifier.c |  5 +++++
 4 files changed, 59 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e98c113a5d27..a94e85c2ec50 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -283,6 +283,7 @@ struct bpf_func_proto {
 		enum bpf_arg_type arg_type[5];
 	};
 	int *btf_id; /* BTF ids of arguments */
+	bool (*allowed)(const struct bpf_prog *prog);
 };
 
 /* bpf_context is intentionally undefined structure. Pointer to bpf_context is
@@ -1745,6 +1746,8 @@ enum bpf_text_poke_type {
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
 		       void *addr1, void *addr2);
 
+bool btf_whitelist_search(int id, int list[], int cnt);
+
 extern int bpf_skb_output_btf_ids[];
 extern int bpf_seq_printf_btf_ids[];
 extern int bpf_seq_write_btf_ids[];
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6924180a19c4..feda74d232c5 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -20,6 +20,7 @@
 #include <linux/btf.h>
 #include <linux/skmsg.h>
 #include <linux/perf_event.h>
+#include <linux/bsearch.h>
 #include <net/sock.h>
 
 /* BTF (BPF Type Format) is the meta data format which describes
@@ -4669,3 +4670,15 @@ u32 btf_id(const struct btf *btf)
 {
 	return btf->id;
 }
+
+static int btf_id_cmp_func(const void *a, const void *b)
+{
+	const int *pa = a, *pb = b;
+
+	return *pa - *pb;
+}
+
+bool btf_whitelist_search(int id, int list[], int cnt)
+{
+	return bsearch(&id, list, cnt, sizeof(int), btf_id_cmp_func) != NULL;
+}
diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h
index 68aa5c38a37f..a90c09faa515 100644
--- a/kernel/bpf/btf_ids.h
+++ b/kernel/bpf/btf_ids.h
@@ -67,4 +67,42 @@ asm(							\
 #name ":;                                      \n"	\
 ".popsection;                                  \n");
 
+
+/*
+ * The BTF_WHITELIST_ENTRY/END macros pair defines sorted
+ * list of BTF IDs plus its members count, with following
+ * layout:
+ *
+ * BTF_WHITELIST_ENTRY(list2)
+ * BTF_ID(type1, name1)
+ * BTF_ID(type2, name2)
+ * BTF_WHITELIST_END(list)
+ *
+ * __BTF_ID__sort__list:
+ * list2_cnt:
+ * .zero 4
+ * list2:
+ * __BTF_ID__type1__name1__3:
+ * .zero 4
+ * __BTF_ID__type2__name2__4:
+ * .zero 4
+ *
+ */
+#define BTF_WHITELIST_ENTRY(name)			\
+asm(							\
+".pushsection " SECTION ",\"a\";               \n"	\
+".global __BTF_ID__sort__" #name ";            \n"	\
+"__BTF_ID__sort__" #name ":;                   \n"	\
+".global " #name "_cnt;                        \n"	\
+#name "_cnt:;                                  \n"	\
+".zero 4                                       \n"	\
+".popsection;                                  \n");	\
+BTF_ID_LIST(name)
+
+#define BTF_WHITELIST_END(name)				\
+asm(							\
+".pushsection " SECTION ",\"a\";              \n"	\
+".size __BTF_ID__sort__" #name ", .-" #name " \n"	\
+".popsection;                                 \n");
+
 #endif
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index bee3da2cd945..5a9a6fd72907 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4633,6 +4633,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
 		return -EINVAL;
 	}
 
+	if (fn->allowed && !fn->allowed(env->prog)) {
+		verbose(env, "helper call is not allowed in probe\n");
+		return -EINVAL;
+	}
+
 	/* With LD_ABS/IND some JITs save/restore skb from r1. */
 	changes_data = bpf_helper_changes_pkt_data(fn->func);
 	if (changes_data && fn->arg1_type != ARG_PTR_TO_CTX) {
-- 
2.25.4

Re: [PATCH 08/11] bpf: Add BTF whitelist support

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to define 'whitelist' of BTF IDs, which is
> also sorted.
>
> Following defines sorted list of BTF IDs that is accessible
> within kernel code as btf_whitelist_d_path and its count is
> in btf_whitelist_d_path_cnt variable.
>
>   extern int btf_whitelist_d_path[];
>   extern int btf_whitelist_d_path_cnt;
>
>   BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
>   BTF_ID(func, vfs_truncate)
>   BTF_ID(func, vfs_fallocate)
>   BTF_ID(func, dentry_open)
>   BTF_ID(func, vfs_getattr)
>   BTF_ID(func, filp_close)
>   BTF_WHITELIST_END(btf_whitelist_d_path)
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h   |  3 +++
>  kernel/bpf/btf.c      | 13 +++++++++++++
>  kernel/bpf/btf_ids.h  | 38 ++++++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c |  5 +++++
>  4 files changed, 59 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index e98c113a5d27..a94e85c2ec50 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -283,6 +283,7 @@ struct bpf_func_proto {
>                 enum bpf_arg_type arg_type[5];
>         };
>         int *btf_id; /* BTF ids of arguments */
> +       bool (*allowed)(const struct bpf_prog *prog);
>  };
>
>  /* bpf_context is intentionally undefined structure. Pointer to bpf_context is
> @@ -1745,6 +1746,8 @@ enum bpf_text_poke_type {
>  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
>                        void *addr1, void *addr2);
>
> +bool btf_whitelist_search(int id, int list[], int cnt);
> +
>  extern int bpf_skb_output_btf_ids[];
>  extern int bpf_seq_printf_btf_ids[];
>  extern int bpf_seq_write_btf_ids[];
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 6924180a19c4..feda74d232c5 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -20,6 +20,7 @@
>  #include <linux/btf.h>
>  #include <linux/skmsg.h>
>  #include <linux/perf_event.h>
> +#include <linux/bsearch.h>
>  #include <net/sock.h>
>
>  /* BTF (BPF Type Format) is the meta data format which describes
> @@ -4669,3 +4670,15 @@ u32 btf_id(const struct btf *btf)
>  {
>         return btf->id;
>  }
> +
> +static int btf_id_cmp_func(const void *a, const void *b)
> +{
> +       const int *pa = a, *pb = b;
> +
> +       return *pa - *pb;
> +}
> +
> +bool btf_whitelist_search(int id, int list[], int cnt)

whitelist is a bit too specific, this functionality can be used for
blacklisting as well, no?

How about instead of "open coding" separately int list[] + int cnt, we
define a struct:

struct btf_id_set {
    u32 cnt;
    u32 ids[];
};

and pass that around?

This function then can be generic

bool btf_id_set_contains(struct btf_id_set *set, u32 id);

Then it's usable for both whitelist and blacklist? _contains also
clearly implies what's the return result, while _search isn't so clear
in that regard.


> +{
> +       return bsearch(&id, list, cnt, sizeof(int), btf_id_cmp_func) != NULL;
> +}
> diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h
> index 68aa5c38a37f..a90c09faa515 100644
> --- a/kernel/bpf/btf_ids.h
> +++ b/kernel/bpf/btf_ids.h
> @@ -67,4 +67,42 @@ asm(                                                 \
>  #name ":;                                      \n"     \
>  ".popsection;                                  \n");
>
> +
> +/*
> + * The BTF_WHITELIST_ENTRY/END macros pair defines sorted
> + * list of BTF IDs plus its members count, with following
> + * layout:
> + *
> + * BTF_WHITELIST_ENTRY(list2)
> + * BTF_ID(type1, name1)
> + * BTF_ID(type2, name2)
> + * BTF_WHITELIST_END(list)

It kind of sucks you need two separate ENTRY/END macro (btw, START/END
or BEGIN/END would be a bit more "paired"), and your example clearly
shows why: it is not self-consistent (list2 on start, list on end ;).
But doing variadic macro like this would be a nightmare as well,
unfortunately. :(

> + *
> + * __BTF_ID__sort__list:
> + * list2_cnt:
> + * .zero 4
> + * list2:
> + * __BTF_ID__type1__name1__3:
> + * .zero 4
> + * __BTF_ID__type2__name2__4:
> + * .zero 4
> + *
> + */
> +#define BTF_WHITELIST_ENTRY(name)                      \
> +asm(                                                   \
> +".pushsection " SECTION ",\"a\";               \n"     \
> +".global __BTF_ID__sort__" #name ";            \n"     \
> +"__BTF_ID__sort__" #name ":;                   \n"     \

I mentioned in the previous patch already, I think "sort" is a bad
name, consider "set" (or "list", but you used list name already for a
slightly different macro).

> +".global " #name "_cnt;                        \n"     \
> +#name "_cnt:;                                  \n"     \

This label/symbol isn't necessary, why polluting the symbol table?

> +".zero 4                                       \n"     \
> +".popsection;                                  \n");   \
> +BTF_ID_LIST(name)
> +
> +#define BTF_WHITELIST_END(name)                                \
> +asm(                                                   \
> +".pushsection " SECTION ",\"a\";              \n"      \
> +".size __BTF_ID__sort__" #name ", .-" #name " \n"      \
> +".popsection;                                 \n");
> +
>  #endif
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index bee3da2cd945..5a9a6fd72907 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4633,6 +4633,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>                 return -EINVAL;
>         }
>
> +       if (fn->allowed && !fn->allowed(env->prog)) {
> +               verbose(env, "helper call is not allowed in probe\n");

nit: probe -> program, or just drop "in probe" part altogether

> +               return -EINVAL;
> +       }
> +
>         /* With LD_ABS/IND some JITs save/restore skb from r1. */
>         changes_data = bpf_helper_changes_pkt_data(fn->func);
>         if (changes_data && fn->arg1_type != ARG_PTR_TO_CTX) {
> --
> 2.25.4
>

Re: [PATCH 08/11] bpf: Add BTF whitelist support

[Jiri Olsa]

On Thu, Jun 18, 2020 at 09:29:58PM -0700, Andrii Nakryiko wrote:

SNIP

> > @@ -4669,3 +4670,15 @@ u32 btf_id(const struct btf *btf)
> >  {
> >         return btf->id;
> >  }
> > +
> > +static int btf_id_cmp_func(const void *a, const void *b)
> > +{
> > +       const int *pa = a, *pb = b;
> > +
> > +       return *pa - *pb;
> > +}
> > +
> > +bool btf_whitelist_search(int id, int list[], int cnt)
> 
> whitelist is a bit too specific, this functionality can be used for
> blacklisting as well, no?
> 
> How about instead of "open coding" separately int list[] + int cnt, we
> define a struct:
> 
> struct btf_id_set {
>     u32 cnt;
>     u32 ids[];
> };
> 
> and pass that around?
> 
> This function then can be generic
> 
> bool btf_id_set_contains(struct btf_id_set *set, u32 id);
> 
> Then it's usable for both whitelist and blacklist? _contains also
> clearly implies what's the return result, while _search isn't so clear
> in that regard.

yep, looks better this way, will change

> 
> 
> > +{
> > +       return bsearch(&id, list, cnt, sizeof(int), btf_id_cmp_func) != NULL;
> > +}
> > diff --git a/kernel/bpf/btf_ids.h b/kernel/bpf/btf_ids.h
> > index 68aa5c38a37f..a90c09faa515 100644
> > --- a/kernel/bpf/btf_ids.h
> > +++ b/kernel/bpf/btf_ids.h
> > @@ -67,4 +67,42 @@ asm(                                                 \
> >  #name ":;                                      \n"     \
> >  ".popsection;                                  \n");
> >
> > +
> > +/*
> > + * The BTF_WHITELIST_ENTRY/END macros pair defines sorted
> > + * list of BTF IDs plus its members count, with following
> > + * layout:
> > + *
> > + * BTF_WHITELIST_ENTRY(list2)
> > + * BTF_ID(type1, name1)
> > + * BTF_ID(type2, name2)
> > + * BTF_WHITELIST_END(list)
> 
> It kind of sucks you need two separate ENTRY/END macro (btw, START/END
> or BEGIN/END would be a bit more "paired"), and your example clearly

ok, START/END it is

> shows why: it is not self-consistent (list2 on start, list on end ;).

ugh ;-)

> But doing variadic macro like this would be a nightmare as well,
> unfortunately. :(
> 
> > + *
> > + * __BTF_ID__sort__list:
> > + * list2_cnt:
> > + * .zero 4
> > + * list2:
> > + * __BTF_ID__type1__name1__3:
> > + * .zero 4
> > + * __BTF_ID__type2__name2__4:
> > + * .zero 4
> > + *
> > + */
> > +#define BTF_WHITELIST_ENTRY(name)                      \
> > +asm(                                                   \
> > +".pushsection " SECTION ",\"a\";               \n"     \
> > +".global __BTF_ID__sort__" #name ";            \n"     \
> > +"__BTF_ID__sort__" #name ":;                   \n"     \
> 
> I mentioned in the previous patch already, I think "sort" is a bad
> name, consider "set" (or "list", but you used list name already for a
> slightly different macro).

yes, I replied to this in another email

> 
> > +".global " #name "_cnt;                        \n"     \
> > +#name "_cnt:;                                  \n"     \
> 
> This label/symbol isn't necessary, why polluting the symbol table?

XXX_cnt variable is used in search function, but isn't needed
if we use that 'struct btf_id_set' you proposed

> 
> > +".zero 4                                       \n"     \
> > +".popsection;                                  \n");   \
> > +BTF_ID_LIST(name)
> > +
> > +#define BTF_WHITELIST_END(name)                                \
> > +asm(                                                   \
> > +".pushsection " SECTION ",\"a\";              \n"      \
> > +".size __BTF_ID__sort__" #name ", .-" #name " \n"      \
> > +".popsection;                                 \n");
> > +
> >  #endif
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index bee3da2cd945..5a9a6fd72907 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4633,6 +4633,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> >                 return -EINVAL;
> >         }
> >
> > +       if (fn->allowed && !fn->allowed(env->prog)) {
> > +               verbose(env, "helper call is not allowed in probe\n");
> 
> nit: probe -> program, or just drop "in probe" part altogether

ok

thnaks,
jirka

[PATCH 09/11] bpf: Add d_path helper

[Jiri Olsa]

Adding d_path helper function that returns full path
for give 'struct path' object, which needs to be the
kernel BTF 'path' object.

The helper calls directly d_path function.

Updating also bpf.h tools uapi header and adding
'path' to bpf_helpers_doc.py script.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h            |  4 ++++
 include/uapi/linux/bpf.h       | 14 ++++++++++++-
 kernel/bpf/btf_ids.c           | 11 ++++++++++
 kernel/trace/bpf_trace.c       | 38 ++++++++++++++++++++++++++++++++++
 scripts/bpf_helpers_doc.py     |  2 ++
 tools/include/uapi/linux/bpf.h | 14 ++++++++++++-
 6 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a94e85c2ec50..d35265b6c574 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1752,5 +1752,9 @@ extern int bpf_skb_output_btf_ids[];
 extern int bpf_seq_printf_btf_ids[];
 extern int bpf_seq_write_btf_ids[];
 extern int bpf_xdp_output_btf_ids[];
+extern int bpf_d_path_btf_ids[];
+
+extern int btf_whitelist_d_path[];
+extern int btf_whitelist_d_path_cnt;
 
 #endif /* _LINUX_BPF_H */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c65b374a5090..e308746b9344 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3252,6 +3252,17 @@ union bpf_attr {
  * 		case of **BPF_CSUM_LEVEL_QUERY**, the current skb->csum_level
  * 		is returned or the error code -EACCES in case the skb is not
  * 		subject to CHECKSUM_UNNECESSARY.
+ *
+ * int bpf_d_path(struct path *path, char *buf, u32 sz)
+ *	Description
+ *		Return full path for given 'struct path' object, which
+ *		needs to be the kernel BTF 'path' object. The path is
+ *		returned in buffer provided 'buf' of size 'sz'.
+ *
+ *	Return
+ *		length of returned string on success, or a negative
+ *		error in case of failure
+ *
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3389,7 +3400,8 @@ union bpf_attr {
 	FN(ringbuf_submit),		\
 	FN(ringbuf_discard),		\
 	FN(ringbuf_query),		\
-	FN(csum_level),
+	FN(csum_level),			\
+	FN(d_path),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
index d8d0df162f04..853c8fd59b06 100644
--- a/kernel/bpf/btf_ids.c
+++ b/kernel/bpf/btf_ids.c
@@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
 
 BTF_ID_LIST(bpf_xdp_output_btf_ids)
 BTF_ID(struct, xdp_buff)
+
+BTF_ID_LIST(bpf_d_path_btf_ids)
+BTF_ID(struct, path)
+
+BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
+BTF_ID(func, vfs_truncate)
+BTF_ID(func, vfs_fallocate)
+BTF_ID(func, dentry_open)
+BTF_ID(func, vfs_getattr)
+BTF_ID(func, filp_close)
+BTF_WHITELIST_END(btf_whitelist_d_path)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index c1866d76041f..0ff5d8434d40 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1016,6 +1016,42 @@ static const struct bpf_func_proto bpf_send_signal_thread_proto = {
 	.arg1_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
+{
+	char *p = d_path(path, buf, sz - 1);
+	int len;
+
+	if (IS_ERR(p)) {
+		len = PTR_ERR(p);
+	} else {
+		len = strlen(p);
+		if (len && p != buf) {
+			memmove(buf, p, len);
+			buf[len] = 0;
+		}
+	}
+
+	return len;
+}
+
+static bool bpf_d_path_allowed(const struct bpf_prog *prog)
+{
+	return btf_whitelist_search(prog->aux->attach_btf_id,
+				    btf_whitelist_d_path,
+				    btf_whitelist_d_path_cnt);
+}
+
+static const struct bpf_func_proto bpf_d_path_proto = {
+	.func		= bpf_d_path,
+	.gpl_only	= true,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_type	= ARG_PTR_TO_MEM,
+	.arg3_type	= ARG_CONST_SIZE,
+	.btf_id		= bpf_d_path_btf_ids,
+	.allowed	= bpf_d_path_allowed,
+};
+
 const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1483,6 +1519,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return prog->expected_attach_type == BPF_TRACE_ITER ?
 		       &bpf_seq_write_proto :
 		       NULL;
+	case BPF_FUNC_d_path:
+		return &bpf_d_path_proto;
 	default:
 		return raw_tp_prog_func_proto(func_id, prog);
 	}
diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index 91fa668fa860..3161bf4ccee4 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -425,6 +425,7 @@ class PrinterHelpers(Printer):
             'struct __sk_buff',
             'struct sk_msg_md',
             'struct xdp_md',
+            'struct path',
     ]
     known_types = {
             '...',
@@ -458,6 +459,7 @@ class PrinterHelpers(Printer):
             'struct sockaddr',
             'struct tcphdr',
             'struct seq_file',
+            'struct path',
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index c65b374a5090..e308746b9344 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3252,6 +3252,17 @@ union bpf_attr {
  * 		case of **BPF_CSUM_LEVEL_QUERY**, the current skb->csum_level
  * 		is returned or the error code -EACCES in case the skb is not
  * 		subject to CHECKSUM_UNNECESSARY.
+ *
+ * int bpf_d_path(struct path *path, char *buf, u32 sz)
+ *	Description
+ *		Return full path for given 'struct path' object, which
+ *		needs to be the kernel BTF 'path' object. The path is
+ *		returned in buffer provided 'buf' of size 'sz'.
+ *
+ *	Return
+ *		length of returned string on success, or a negative
+ *		error in case of failure
+ *
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3389,7 +3400,8 @@ union bpf_attr {
 	FN(ringbuf_submit),		\
 	FN(ringbuf_discard),		\
 	FN(ringbuf_query),		\
-	FN(csum_level),
+	FN(csum_level),			\
+	FN(d_path),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
-- 
2.25.4

Re: [PATCH 09/11] bpf: Add d_path helper

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding d_path helper function that returns full path
> for give 'struct path' object, which needs to be the
> kernel BTF 'path' object.
>
> The helper calls directly d_path function.
>
> Updating also bpf.h tools uapi header and adding
> 'path' to bpf_helpers_doc.py script.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h            |  4 ++++
>  include/uapi/linux/bpf.h       | 14 ++++++++++++-
>  kernel/bpf/btf_ids.c           | 11 ++++++++++
>  kernel/trace/bpf_trace.c       | 38 ++++++++++++++++++++++++++++++++++
>  scripts/bpf_helpers_doc.py     |  2 ++
>  tools/include/uapi/linux/bpf.h | 14 ++++++++++++-
>  6 files changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a94e85c2ec50..d35265b6c574 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1752,5 +1752,9 @@ extern int bpf_skb_output_btf_ids[];
>  extern int bpf_seq_printf_btf_ids[];
>  extern int bpf_seq_write_btf_ids[];
>  extern int bpf_xdp_output_btf_ids[];
> +extern int bpf_d_path_btf_ids[];
> +
> +extern int btf_whitelist_d_path[];
> +extern int btf_whitelist_d_path_cnt;

So with suggestion from previous patch, this would be declared as:

extern const struct btf_id_set btf_whitelist_d_path;

>
>  #endif /* _LINUX_BPF_H */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index c65b374a5090..e308746b9344 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3252,6 +3252,17 @@ union bpf_attr {
>   *             case of **BPF_CSUM_LEVEL_QUERY**, the current skb->csum_level
>   *             is returned or the error code -EACCES in case the skb is not
>   *             subject to CHECKSUM_UNNECESSARY.
> + *
> + * int bpf_d_path(struct path *path, char *buf, u32 sz)
> + *     Description
> + *             Return full path for given 'struct path' object, which
> + *             needs to be the kernel BTF 'path' object. The path is
> + *             returned in buffer provided 'buf' of size 'sz'.
> + *
> + *     Return
> + *             length of returned string on success, or a negative
> + *             error in case of failure
> + *
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -3389,7 +3400,8 @@ union bpf_attr {
>         FN(ringbuf_submit),             \
>         FN(ringbuf_discard),            \
>         FN(ringbuf_query),              \
> -       FN(csum_level),
> +       FN(csum_level),                 \
> +       FN(d_path),
>
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>   * function eBPF program intends to call
> diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> index d8d0df162f04..853c8fd59b06 100644
> --- a/kernel/bpf/btf_ids.c
> +++ b/kernel/bpf/btf_ids.c
> @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
>
>  BTF_ID_LIST(bpf_xdp_output_btf_ids)
>  BTF_ID(struct, xdp_buff)
> +
> +BTF_ID_LIST(bpf_d_path_btf_ids)
> +BTF_ID(struct, path)
> +
> +BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
> +BTF_ID(func, vfs_truncate)
> +BTF_ID(func, vfs_fallocate)
> +BTF_ID(func, dentry_open)
> +BTF_ID(func, vfs_getattr)
> +BTF_ID(func, filp_close)
> +BTF_WHITELIST_END(btf_whitelist_d_path)

Oh, so that's why you added btf_ids.c. Do you think centralizing all
those BTF ID lists in one file is going to be more convenient? I lean
towards keeping them closer to where they are used, as it was with all
those helper BTF IDS. But I wonder what others think...

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index c1866d76041f..0ff5d8434d40 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1016,6 +1016,42 @@ static const struct bpf_func_proto bpf_send_signal_thread_proto = {
>         .arg1_type      = ARG_ANYTHING,
>  };
>

[...]

Re: [PATCH 09/11] bpf: Add d_path helper

[Jiri Olsa]

On Thu, Jun 18, 2020 at 09:35:10PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 16, 2020 at 3:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding d_path helper function that returns full path
> > for give 'struct path' object, which needs to be the
> > kernel BTF 'path' object.
> >
> > The helper calls directly d_path function.
> >
> > Updating also bpf.h tools uapi header and adding
> > 'path' to bpf_helpers_doc.py script.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/bpf.h            |  4 ++++
> >  include/uapi/linux/bpf.h       | 14 ++++++++++++-
> >  kernel/bpf/btf_ids.c           | 11 ++++++++++
> >  kernel/trace/bpf_trace.c       | 38 ++++++++++++++++++++++++++++++++++
> >  scripts/bpf_helpers_doc.py     |  2 ++
> >  tools/include/uapi/linux/bpf.h | 14 ++++++++++++-
> >  6 files changed, 81 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index a94e85c2ec50..d35265b6c574 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1752,5 +1752,9 @@ extern int bpf_skb_output_btf_ids[];
> >  extern int bpf_seq_printf_btf_ids[];
> >  extern int bpf_seq_write_btf_ids[];
> >  extern int bpf_xdp_output_btf_ids[];
> > +extern int bpf_d_path_btf_ids[];
> > +
> > +extern int btf_whitelist_d_path[];
> > +extern int btf_whitelist_d_path_cnt;
> 
> So with suggestion from previous patch, this would be declared as:
> 
> extern const struct btf_id_set btf_whitelist_d_path;

yes

SNIP

> >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >   * function eBPF program intends to call
> > diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> > index d8d0df162f04..853c8fd59b06 100644
> > --- a/kernel/bpf/btf_ids.c
> > +++ b/kernel/bpf/btf_ids.c
> > @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
> >
> >  BTF_ID_LIST(bpf_xdp_output_btf_ids)
> >  BTF_ID(struct, xdp_buff)
> > +
> > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > +BTF_ID(struct, path)
> > +
> > +BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
> > +BTF_ID(func, vfs_truncate)
> > +BTF_ID(func, vfs_fallocate)
> > +BTF_ID(func, dentry_open)
> > +BTF_ID(func, vfs_getattr)
> > +BTF_ID(func, filp_close)
> > +BTF_WHITELIST_END(btf_whitelist_d_path)
> 
> Oh, so that's why you added btf_ids.c. Do you think centralizing all
> those BTF ID lists in one file is going to be more convenient? I lean
> towards keeping them closer to where they are used, as it was with all
> those helper BTF IDS. But I wonder what others think...

either way works for me, but then BTF_ID_* macros needs to go
to include/linux/btf_ids.h header right?

jirka

> 
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index c1866d76041f..0ff5d8434d40 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1016,6 +1016,42 @@ static const struct bpf_func_proto bpf_send_signal_thread_proto = {
> >         .arg1_type      = ARG_ANYTHING,
> >  };
> >
> 
> [...]
> 

Re: [PATCH 09/11] bpf: Add d_path helper

[Andrii Nakryiko]

On Fri, Jun 19, 2020 at 6:31 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Jun 18, 2020 at 09:35:10PM -0700, Andrii Nakryiko wrote:
> > On Tue, Jun 16, 2020 at 3:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding d_path helper function that returns full path
> > > for give 'struct path' object, which needs to be the
> > > kernel BTF 'path' object.
> > >
> > > The helper calls directly d_path function.
> > >
> > > Updating also bpf.h tools uapi header and adding
> > > 'path' to bpf_helpers_doc.py script.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  include/linux/bpf.h            |  4 ++++
> > >  include/uapi/linux/bpf.h       | 14 ++++++++++++-
> > >  kernel/bpf/btf_ids.c           | 11 ++++++++++
> > >  kernel/trace/bpf_trace.c       | 38 ++++++++++++++++++++++++++++++++++
> > >  scripts/bpf_helpers_doc.py     |  2 ++
> > >  tools/include/uapi/linux/bpf.h | 14 ++++++++++++-
> > >  6 files changed, 81 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index a94e85c2ec50..d35265b6c574 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -1752,5 +1752,9 @@ extern int bpf_skb_output_btf_ids[];
> > >  extern int bpf_seq_printf_btf_ids[];
> > >  extern int bpf_seq_write_btf_ids[];
> > >  extern int bpf_xdp_output_btf_ids[];
> > > +extern int bpf_d_path_btf_ids[];
> > > +
> > > +extern int btf_whitelist_d_path[];
> > > +extern int btf_whitelist_d_path_cnt;
> >
> > So with suggestion from previous patch, this would be declared as:
> >
> > extern const struct btf_id_set btf_whitelist_d_path;
>
> yes
>
> SNIP
>
> > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > >   * function eBPF program intends to call
> > > diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> > > index d8d0df162f04..853c8fd59b06 100644
> > > --- a/kernel/bpf/btf_ids.c
> > > +++ b/kernel/bpf/btf_ids.c
> > > @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
> > >
> > >  BTF_ID_LIST(bpf_xdp_output_btf_ids)
> > >  BTF_ID(struct, xdp_buff)
> > > +
> > > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > > +BTF_ID(struct, path)
> > > +
> > > +BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
> > > +BTF_ID(func, vfs_truncate)
> > > +BTF_ID(func, vfs_fallocate)
> > > +BTF_ID(func, dentry_open)
> > > +BTF_ID(func, vfs_getattr)
> > > +BTF_ID(func, filp_close)
> > > +BTF_WHITELIST_END(btf_whitelist_d_path)
> >
> > Oh, so that's why you added btf_ids.c. Do you think centralizing all
> > those BTF ID lists in one file is going to be more convenient? I lean
> > towards keeping them closer to where they are used, as it was with all
> > those helper BTF IDS. But I wonder what others think...
>
> either way works for me, but then BTF_ID_* macros needs to go
> to include/linux/btf_ids.h header right?
>

given it's internal API, I'd probably just put it in
include/linux/btf.h or include/linux/bpf.h, don't think we need extra
header just for these


> jirka
>
> >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index c1866d76041f..0ff5d8434d40 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -1016,6 +1016,42 @@ static const struct bpf_func_proto bpf_send_signal_thread_proto = {
> > >         .arg1_type      = ARG_ANYTHING,
> > >  };
> > >
> >
> > [...]
> >
>

Re: [PATCH 09/11] bpf: Add d_path helper

[Jiri Olsa]

On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote:

SNIP

> > > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > >   * function eBPF program intends to call
> > > > diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> > > > index d8d0df162f04..853c8fd59b06 100644
> > > > --- a/kernel/bpf/btf_ids.c
> > > > +++ b/kernel/bpf/btf_ids.c
> > > > @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
> > > >
> > > >  BTF_ID_LIST(bpf_xdp_output_btf_ids)
> > > >  BTF_ID(struct, xdp_buff)
> > > > +
> > > > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > > > +BTF_ID(struct, path)
> > > > +
> > > > +BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
> > > > +BTF_ID(func, vfs_truncate)
> > > > +BTF_ID(func, vfs_fallocate)
> > > > +BTF_ID(func, dentry_open)
> > > > +BTF_ID(func, vfs_getattr)
> > > > +BTF_ID(func, filp_close)
> > > > +BTF_WHITELIST_END(btf_whitelist_d_path)
> > >
> > > Oh, so that's why you added btf_ids.c. Do you think centralizing all
> > > those BTF ID lists in one file is going to be more convenient? I lean
> > > towards keeping them closer to where they are used, as it was with all
> > > those helper BTF IDS. But I wonder what others think...
> >
> > either way works for me, but then BTF_ID_* macros needs to go
> > to include/linux/btf_ids.h header right?
> >
> 
> given it's internal API, I'd probably just put it in
> include/linux/btf.h or include/linux/bpf.h, don't think we need extra
> header just for these

actually, I might end up with extra header, so it's possible
to add selftest for this

jirka

Re: [PATCH 09/11] bpf: Add d_path helper

[Andrii Nakryiko]

On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > > > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > > >   * function eBPF program intends to call
> > > > > diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> > > > > index d8d0df162f04..853c8fd59b06 100644
> > > > > --- a/kernel/bpf/btf_ids.c
> > > > > +++ b/kernel/bpf/btf_ids.c
> > > > > @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
> > > > >
> > > > >  BTF_ID_LIST(bpf_xdp_output_btf_ids)
> > > > >  BTF_ID(struct, xdp_buff)
> > > > > +
> > > > > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > > > > +BTF_ID(struct, path)
> > > > > +
> > > > > +BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
> > > > > +BTF_ID(func, vfs_truncate)
> > > > > +BTF_ID(func, vfs_fallocate)
> > > > > +BTF_ID(func, dentry_open)
> > > > > +BTF_ID(func, vfs_getattr)
> > > > > +BTF_ID(func, filp_close)
> > > > > +BTF_WHITELIST_END(btf_whitelist_d_path)
> > > >
> > > > Oh, so that's why you added btf_ids.c. Do you think centralizing all
> > > > those BTF ID lists in one file is going to be more convenient? I lean
> > > > towards keeping them closer to where they are used, as it was with all
> > > > those helper BTF IDS. But I wonder what others think...
> > >
> > > either way works for me, but then BTF_ID_* macros needs to go
> > > to include/linux/btf_ids.h header right?
> > >
> >
> > given it's internal API, I'd probably just put it in
> > include/linux/btf.h or include/linux/bpf.h, don't think we need extra
> > header just for these
>
> actually, I might end up with extra header, so it's possible
> to add selftest for this
>

How does extra header help with selftest?

> jirka
>

Re: [PATCH 09/11] bpf: Add d_path helper

[Jiri Olsa]

On Mon, Jun 22, 2020 at 12:18:19PM -0700, Andrii Nakryiko wrote:
> On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > > > > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > > > >   * function eBPF program intends to call
> > > > > > diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> > > > > > index d8d0df162f04..853c8fd59b06 100644
> > > > > > --- a/kernel/bpf/btf_ids.c
> > > > > > +++ b/kernel/bpf/btf_ids.c
> > > > > > @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
> > > > > >
> > > > > >  BTF_ID_LIST(bpf_xdp_output_btf_ids)
> > > > > >  BTF_ID(struct, xdp_buff)
> > > > > > +
> > > > > > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > > > > > +BTF_ID(struct, path)
> > > > > > +
> > > > > > +BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
> > > > > > +BTF_ID(func, vfs_truncate)
> > > > > > +BTF_ID(func, vfs_fallocate)
> > > > > > +BTF_ID(func, dentry_open)
> > > > > > +BTF_ID(func, vfs_getattr)
> > > > > > +BTF_ID(func, filp_close)
> > > > > > +BTF_WHITELIST_END(btf_whitelist_d_path)
> > > > >
> > > > > Oh, so that's why you added btf_ids.c. Do you think centralizing all
> > > > > those BTF ID lists in one file is going to be more convenient? I lean
> > > > > towards keeping them closer to where they are used, as it was with all
> > > > > those helper BTF IDS. But I wonder what others think...
> > > >
> > > > either way works for me, but then BTF_ID_* macros needs to go
> > > > to include/linux/btf_ids.h header right?
> > > >
> > >
> > > given it's internal API, I'd probably just put it in
> > > include/linux/btf.h or include/linux/bpf.h, don't think we need extra
> > > header just for these
> >
> > actually, I might end up with extra header, so it's possible
> > to add selftest for this
> >
> 
> How does extra header help with selftest?

to create binary with various lists defined like we do in kernel
using the same macros..  and check they are properly made/sorted

jirka

Re: [PATCH 09/11] bpf: Add d_path helper

[Andrii Nakryiko]

On Tue, Jun 23, 2020 at 3:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Jun 22, 2020 at 12:18:19PM -0700, Andrii Nakryiko wrote:
> > On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote:
> > >
> > > SNIP
> > >
> > > > > > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > > > > >   * function eBPF program intends to call
> > > > > > > diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> > > > > > > index d8d0df162f04..853c8fd59b06 100644
> > > > > > > --- a/kernel/bpf/btf_ids.c
> > > > > > > +++ b/kernel/bpf/btf_ids.c
> > > > > > > @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
> > > > > > >
> > > > > > >  BTF_ID_LIST(bpf_xdp_output_btf_ids)
> > > > > > >  BTF_ID(struct, xdp_buff)
> > > > > > > +
> > > > > > > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > > > > > > +BTF_ID(struct, path)
> > > > > > > +
> > > > > > > +BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
> > > > > > > +BTF_ID(func, vfs_truncate)
> > > > > > > +BTF_ID(func, vfs_fallocate)
> > > > > > > +BTF_ID(func, dentry_open)
> > > > > > > +BTF_ID(func, vfs_getattr)
> > > > > > > +BTF_ID(func, filp_close)
> > > > > > > +BTF_WHITELIST_END(btf_whitelist_d_path)
> > > > > >
> > > > > > Oh, so that's why you added btf_ids.c. Do you think centralizing all
> > > > > > those BTF ID lists in one file is going to be more convenient? I lean
> > > > > > towards keeping them closer to where they are used, as it was with all
> > > > > > those helper BTF IDS. But I wonder what others think...
> > > > >
> > > > > either way works for me, but then BTF_ID_* macros needs to go
> > > > > to include/linux/btf_ids.h header right?
> > > > >
> > > >
> > > > given it's internal API, I'd probably just put it in
> > > > include/linux/btf.h or include/linux/bpf.h, don't think we need extra
> > > > header just for these
> > >
> > > actually, I might end up with extra header, so it's possible
> > > to add selftest for this
> > >
> >
> > How does extra header help with selftest?
>
> to create binary with various lists defined like we do in kernel
> using the same macros..  and check they are properly made/sorted
>

So the problem here is that selftests don't have access to internal
(non-UAPI) linux/bpf.h header, right? Ok, that's a fair point.

> jirka
>

Re: [PATCH 09/11] bpf: Add d_path helper

[Jiri Olsa]

On Tue, Jun 23, 2020 at 11:58:33AM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 23, 2020 at 3:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Mon, Jun 22, 2020 at 12:18:19PM -0700, Andrii Nakryiko wrote:
> > > On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote:
> > > >
> > > > SNIP
> > > >
> > > > > > > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > > > > > >   * function eBPF program intends to call
> > > > > > > > diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> > > > > > > > index d8d0df162f04..853c8fd59b06 100644
> > > > > > > > --- a/kernel/bpf/btf_ids.c
> > > > > > > > +++ b/kernel/bpf/btf_ids.c
> > > > > > > > @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
> > > > > > > >
> > > > > > > >  BTF_ID_LIST(bpf_xdp_output_btf_ids)
> > > > > > > >  BTF_ID(struct, xdp_buff)
> > > > > > > > +
> > > > > > > > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > > > > > > > +BTF_ID(struct, path)
> > > > > > > > +
> > > > > > > > +BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
> > > > > > > > +BTF_ID(func, vfs_truncate)
> > > > > > > > +BTF_ID(func, vfs_fallocate)
> > > > > > > > +BTF_ID(func, dentry_open)
> > > > > > > > +BTF_ID(func, vfs_getattr)
> > > > > > > > +BTF_ID(func, filp_close)
> > > > > > > > +BTF_WHITELIST_END(btf_whitelist_d_path)
> > > > > > >
> > > > > > > Oh, so that's why you added btf_ids.c. Do you think centralizing all
> > > > > > > those BTF ID lists in one file is going to be more convenient? I lean
> > > > > > > towards keeping them closer to where they are used, as it was with all
> > > > > > > those helper BTF IDS. But I wonder what others think...
> > > > > >
> > > > > > either way works for me, but then BTF_ID_* macros needs to go
> > > > > > to include/linux/btf_ids.h header right?
> > > > > >
> > > > >
> > > > > given it's internal API, I'd probably just put it in
> > > > > include/linux/btf.h or include/linux/bpf.h, don't think we need extra
> > > > > header just for these
> > > >
> > > > actually, I might end up with extra header, so it's possible
> > > > to add selftest for this
> > > >
> > >
> > > How does extra header help with selftest?
> >
> > to create binary with various lists defined like we do in kernel
> > using the same macros..  and check they are properly made/sorted
> >
> 
> So the problem here is that selftests don't have access to internal
> (non-UAPI) linux/bpf.h header, right? Ok, that's a fair point.

hm, how about we keep tools/include/linux/btf_ids.h copy
like we do for other kernel headers

jirka

Re: [PATCH 09/11] bpf: Add d_path helper

[Andrii Nakryiko]

On Tue, Jun 23, 2020 at 1:14 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Jun 23, 2020 at 11:58:33AM -0700, Andrii Nakryiko wrote:
> > On Tue, Jun 23, 2020 at 3:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Mon, Jun 22, 2020 at 12:18:19PM -0700, Andrii Nakryiko wrote:
> > > > On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > >
> > > > > On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote:
> > > > >
> > > > > SNIP
> > > > >
> > > > > > > > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > > > > > > >   * function eBPF program intends to call
> > > > > > > > > diff --git a/kernel/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> > > > > > > > > index d8d0df162f04..853c8fd59b06 100644
> > > > > > > > > --- a/kernel/bpf/btf_ids.c
> > > > > > > > > +++ b/kernel/bpf/btf_ids.c
> > > > > > > > > @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
> > > > > > > > >
> > > > > > > > >  BTF_ID_LIST(bpf_xdp_output_btf_ids)
> > > > > > > > >  BTF_ID(struct, xdp_buff)
> > > > > > > > > +
> > > > > > > > > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > > > > > > > > +BTF_ID(struct, path)
> > > > > > > > > +
> > > > > > > > > +BTF_WHITELIST_ENTRY(btf_whitelist_d_path)
> > > > > > > > > +BTF_ID(func, vfs_truncate)
> > > > > > > > > +BTF_ID(func, vfs_fallocate)
> > > > > > > > > +BTF_ID(func, dentry_open)
> > > > > > > > > +BTF_ID(func, vfs_getattr)
> > > > > > > > > +BTF_ID(func, filp_close)
> > > > > > > > > +BTF_WHITELIST_END(btf_whitelist_d_path)
> > > > > > > >
> > > > > > > > Oh, so that's why you added btf_ids.c. Do you think centralizing all
> > > > > > > > those BTF ID lists in one file is going to be more convenient? I lean
> > > > > > > > towards keeping them closer to where they are used, as it was with all
> > > > > > > > those helper BTF IDS. But I wonder what others think...
> > > > > > >
> > > > > > > either way works for me, but then BTF_ID_* macros needs to go
> > > > > > > to include/linux/btf_ids.h header right?
> > > > > > >
> > > > > >
> > > > > > given it's internal API, I'd probably just put it in
> > > > > > include/linux/btf.h or include/linux/bpf.h, don't think we need extra
> > > > > > header just for these
> > > > >
> > > > > actually, I might end up with extra header, so it's possible
> > > > > to add selftest for this
> > > > >
> > > >
> > > > How does extra header help with selftest?
> > >
> > > to create binary with various lists defined like we do in kernel
> > > using the same macros..  and check they are properly made/sorted
> > >
> >
> > So the problem here is that selftests don't have access to internal
> > (non-UAPI) linux/bpf.h header, right? Ok, that's a fair point.
>
> hm, how about we keep tools/include/linux/btf_ids.h copy
> like we do for other kernel headers

yes, I assumed you are going to do that

>
> jirka
>

[PATCH 10/11] selftests/bpf: Add verifier test for d_path helper

[Jiri Olsa]

Adding verifier test for attaching tracing program and
calling d_path helper from within and testing that it's
allowed for dentry_open function and denied for 'd_path'
function with appropriate error.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/test_verifier.c   | 13 ++++++-
 tools/testing/selftests/bpf/verifier/d_path.c | 38 +++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/verifier/d_path.c

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 78a6bae56ea6..3cce3dc766a2 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -114,6 +114,7 @@ struct bpf_test {
 		bpf_testdata_struct_t retvals[MAX_TEST_RUNS];
 	};
 	enum bpf_attach_type expected_attach_type;
+	const char *kfunc;
 };
 
 /* Note we want this to be 64 bit aligned so that the end of our array is
@@ -984,8 +985,18 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 		attr.log_level = 4;
 	attr.prog_flags = pflags;
 
+	if (prog_type == BPF_PROG_TYPE_TRACING && test->kfunc) {
+		attr.attach_btf_id = libbpf_find_vmlinux_btf_id(test->kfunc,
+						attr.expected_attach_type);
+	}
+
 	fd_prog = bpf_load_program_xattr(&attr, bpf_vlog, sizeof(bpf_vlog));
-	if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
+
+	/* BPF_PROG_TYPE_TRACING requires more setup and
+	 * bpf_probe_prog_type won't give correct answer
+	 */
+	if (fd_prog < 0 && (prog_type != BPF_PROG_TYPE_TRACING) &&
+	    !bpf_probe_prog_type(prog_type, 0)) {
 		printf("SKIP (unsupported program type %d)\n", prog_type);
 		skips++;
 		goto close_fds;
diff --git a/tools/testing/selftests/bpf/verifier/d_path.c b/tools/testing/selftests/bpf/verifier/d_path.c
new file mode 100644
index 000000000000..e08181abc056
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/d_path.c
@@ -0,0 +1,38 @@
+{
+	"d_path accept",
+	.insns = {
+	BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0),
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+	BPF_MOV64_IMM(BPF_REG_6, 0),
+	BPF_STX_MEM(BPF_DW, BPF_REG_2, BPF_REG_6, 0),
+	BPF_LD_IMM64(BPF_REG_3, 8),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_d_path),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.errstr = "R0 max value is outside of the array range",
+	.result = ACCEPT,
+	.prog_type = BPF_PROG_TYPE_TRACING,
+	.expected_attach_type = BPF_TRACE_FENTRY,
+	.kfunc = "dentry_open",
+},
+{
+	"d_path reject",
+	.insns = {
+	BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0),
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+	BPF_MOV64_IMM(BPF_REG_6, 0),
+	BPF_STX_MEM(BPF_DW, BPF_REG_2, BPF_REG_6, 0),
+	BPF_LD_IMM64(BPF_REG_3, 8),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_d_path),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.errstr = "helper call is not allowed in probe",
+	.result = REJECT,
+	.prog_type = BPF_PROG_TYPE_TRACING,
+	.expected_attach_type = BPF_TRACE_FENTRY,
+	.kfunc = "d_path",
+},
-- 
2.25.4

Re: [PATCH 10/11] selftests/bpf: Add verifier test for d_path helper

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding verifier test for attaching tracing program and
> calling d_path helper from within and testing that it's
> allowed for dentry_open function and denied for 'd_path'
> function with appropriate error.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/testing/selftests/bpf/test_verifier.c   | 13 ++++++-
>  tools/testing/selftests/bpf/verifier/d_path.c | 38 +++++++++++++++++++
>  2 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/verifier/d_path.c
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 78a6bae56ea6..3cce3dc766a2 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -114,6 +114,7 @@ struct bpf_test {
>                 bpf_testdata_struct_t retvals[MAX_TEST_RUNS];
>         };
>         enum bpf_attach_type expected_attach_type;
> +       const char *kfunc;
>  };
>
>  /* Note we want this to be 64 bit aligned so that the end of our array is
> @@ -984,8 +985,18 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>                 attr.log_level = 4;
>         attr.prog_flags = pflags;
>
> +       if (prog_type == BPF_PROG_TYPE_TRACING && test->kfunc) {
> +               attr.attach_btf_id = libbpf_find_vmlinux_btf_id(test->kfunc,
> +                                               attr.expected_attach_type);

if (!attr.attach_btf_id)
  emit more meaningful error, than later during load?

> +       }
> +
>         fd_prog = bpf_load_program_xattr(&attr, bpf_vlog, sizeof(bpf_vlog));
> -       if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
> +
> +       /* BPF_PROG_TYPE_TRACING requires more setup and
> +        * bpf_probe_prog_type won't give correct answer
> +        */
> +       if (fd_prog < 0 && (prog_type != BPF_PROG_TYPE_TRACING) &&

nit: () are redundant

> +           !bpf_probe_prog_type(prog_type, 0)) {
>                 printf("SKIP (unsupported program type %d)\n", prog_type);
>                 skips++;
>                 goto close_fds;
> diff --git a/tools/testing/selftests/bpf/verifier/d_path.c b/tools/testing/selftests/bpf/verifier/d_path.c
> new file mode 100644
> index 000000000000..e08181abc056
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/d_path.c
> @@ -0,0 +1,38 @@
> +{
> +       "d_path accept",
> +       .insns = {
> +       BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0),
> +       BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +       BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +       BPF_MOV64_IMM(BPF_REG_6, 0),
> +       BPF_STX_MEM(BPF_DW, BPF_REG_2, BPF_REG_6, 0),
> +       BPF_LD_IMM64(BPF_REG_3, 8),
> +       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_d_path),
> +       BPF_MOV64_IMM(BPF_REG_0, 0),
> +       BPF_EXIT_INSN(),
> +       },
> +       .errstr = "R0 max value is outside of the array range",
> +       .result = ACCEPT,

accept with error string expected?


> +       .prog_type = BPF_PROG_TYPE_TRACING,
> +       .expected_attach_type = BPF_TRACE_FENTRY,
> +       .kfunc = "dentry_open",
> +},
> +{
> +       "d_path reject",
> +       .insns = {
> +       BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0),
> +       BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +       BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +       BPF_MOV64_IMM(BPF_REG_6, 0),
> +       BPF_STX_MEM(BPF_DW, BPF_REG_2, BPF_REG_6, 0),
> +       BPF_LD_IMM64(BPF_REG_3, 8),
> +       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_d_path),
> +       BPF_MOV64_IMM(BPF_REG_0, 0),
> +       BPF_EXIT_INSN(),
> +       },
> +       .errstr = "helper call is not allowed in probe",
> +       .result = REJECT,
> +       .prog_type = BPF_PROG_TYPE_TRACING,
> +       .expected_attach_type = BPF_TRACE_FENTRY,
> +       .kfunc = "d_path",
> +},
> --
> 2.25.4
>

Re: [PATCH 10/11] selftests/bpf: Add verifier test for d_path helper

[Jiri Olsa]

On Thu, Jun 18, 2020 at 09:38:56PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 16, 2020 at 3:06 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding verifier test for attaching tracing program and
> > calling d_path helper from within and testing that it's
> > allowed for dentry_open function and denied for 'd_path'
> > function with appropriate error.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/test_verifier.c   | 13 ++++++-
> >  tools/testing/selftests/bpf/verifier/d_path.c | 38 +++++++++++++++++++
> >  2 files changed, 50 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/bpf/verifier/d_path.c
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 78a6bae56ea6..3cce3dc766a2 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -114,6 +114,7 @@ struct bpf_test {
> >                 bpf_testdata_struct_t retvals[MAX_TEST_RUNS];
> >         };
> >         enum bpf_attach_type expected_attach_type;
> > +       const char *kfunc;
> >  };
> >
> >  /* Note we want this to be 64 bit aligned so that the end of our array is
> > @@ -984,8 +985,18 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >                 attr.log_level = 4;
> >         attr.prog_flags = pflags;
> >
> > +       if (prog_type == BPF_PROG_TYPE_TRACING && test->kfunc) {
> > +               attr.attach_btf_id = libbpf_find_vmlinux_btf_id(test->kfunc,
> > +                                               attr.expected_attach_type);
> 
> if (!attr.attach_btf_id)
>   emit more meaningful error, than later during load?

ok

> 
> > +       }
> > +
> >         fd_prog = bpf_load_program_xattr(&attr, bpf_vlog, sizeof(bpf_vlog));
> > -       if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
> > +
> > +       /* BPF_PROG_TYPE_TRACING requires more setup and
> > +        * bpf_probe_prog_type won't give correct answer
> > +        */
> > +       if (fd_prog < 0 && (prog_type != BPF_PROG_TYPE_TRACING) &&
> 
> nit: () are redundant

ok

> 
> > +           !bpf_probe_prog_type(prog_type, 0)) {
> >                 printf("SKIP (unsupported program type %d)\n", prog_type);
> >                 skips++;
> >                 goto close_fds;
> > diff --git a/tools/testing/selftests/bpf/verifier/d_path.c b/tools/testing/selftests/bpf/verifier/d_path.c
> > new file mode 100644
> > index 000000000000..e08181abc056
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/verifier/d_path.c
> > @@ -0,0 +1,38 @@
> > +{
> > +       "d_path accept",
> > +       .insns = {
> > +       BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0),
> > +       BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > +       BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > +       BPF_MOV64_IMM(BPF_REG_6, 0),
> > +       BPF_STX_MEM(BPF_DW, BPF_REG_2, BPF_REG_6, 0),
> > +       BPF_LD_IMM64(BPF_REG_3, 8),
> > +       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_d_path),
> > +       BPF_MOV64_IMM(BPF_REG_0, 0),
> > +       BPF_EXIT_INSN(),
> > +       },
> > +       .errstr = "R0 max value is outside of the array range",
> > +       .result = ACCEPT,
> 
> accept with error string expected?

oops, probably lefover, will check

thanks,
jirka

> 
> 
> > +       .prog_type = BPF_PROG_TYPE_TRACING,
> > +       .expected_attach_type = BPF_TRACE_FENTRY,
> > +       .kfunc = "dentry_open",
> > +},
> > +{
> > +       "d_path reject",
> > +       .insns = {
> > +       BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0),
> > +       BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > +       BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > +       BPF_MOV64_IMM(BPF_REG_6, 0),
> > +       BPF_STX_MEM(BPF_DW, BPF_REG_2, BPF_REG_6, 0),
> > +       BPF_LD_IMM64(BPF_REG_3, 8),
> > +       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_d_path),
> > +       BPF_MOV64_IMM(BPF_REG_0, 0),
> > +       BPF_EXIT_INSN(),
> > +       },
> > +       .errstr = "helper call is not allowed in probe",
> > +       .result = REJECT,
> > +       .prog_type = BPF_PROG_TYPE_TRACING,
> > +       .expected_attach_type = BPF_TRACE_FENTRY,
> > +       .kfunc = "d_path",
> > +},
> > --
> > 2.25.4
> >
> 

[PATCH 11/11] selftests/bpf: Add test for d_path helper

[Jiri Olsa]

Adding test for d_path helper which is pretty much
copied from Wenbo Zhang's test for bpf_get_fd_path,
which never made it in.

I've failed so far to compile the test with <linux/fs.h>
kernel header, so for now adding 'struct file' with f_path
member that has same offset as kernel's file object.

Original-patch-by: Wenbo Zhang <ethercflow@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/d_path.c | 153 ++++++++++++++++++
 .../testing/selftests/bpf/progs/test_d_path.c |  55 +++++++
 2 files changed, 208 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_d_path.c

diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
new file mode 100644
index 000000000000..e2b7dfeb506f
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <test_progs.h>
+#include <sys/stat.h>
+#include <linux/sched.h>
+#include <sys/syscall.h>
+
+#define MAX_PATH_LEN		128
+#define MAX_FILES		7
+#define MAX_EVENT_NUM		16
+
+struct d_path_test_data {
+	pid_t pid;
+	__u32 cnt_stat;
+	__u32 cnt_close;
+	char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
+	char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
+};
+
+#include "test_d_path.skel.h"
+
+static struct {
+	__u32 cnt;
+	char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
+} src;
+
+static int set_pathname(int fd, pid_t pid)
+{
+	char buf[MAX_PATH_LEN];
+
+	snprintf(buf, MAX_PATH_LEN, "/proc/%d/fd/%d", pid, fd);
+	return readlink(buf, src.paths[src.cnt++], MAX_PATH_LEN);
+}
+
+static int trigger_fstat_events(pid_t pid)
+{
+	int sockfd = -1, procfd = -1, devfd = -1;
+	int localfd = -1, indicatorfd = -1;
+	int pipefd[2] = { -1, -1 };
+	struct stat fileStat;
+	int ret = -1;
+
+	/* unmountable pseudo-filesystems */
+	if (CHECK_FAIL(pipe(pipefd) < 0))
+		return ret;
+	/* unmountable pseudo-filesystems */
+	sockfd = socket(AF_INET, SOCK_STREAM, 0);
+	if (CHECK_FAIL(sockfd < 0))
+		goto out_close;
+	/* mountable pseudo-filesystems */
+	procfd = open("/proc/self/comm", O_RDONLY);
+	if (CHECK_FAIL(procfd < 0))
+		goto out_close;
+	devfd = open("/dev/urandom", O_RDONLY);
+	if (CHECK_FAIL(devfd < 0))
+		goto out_close;
+	localfd = open("/tmp/d_path_loadgen.txt", O_CREAT | O_RDONLY);
+	if (CHECK_FAIL(localfd < 0))
+		goto out_close;
+	/* bpf_d_path will return path with (deleted) */
+	remove("/tmp/d_path_loadgen.txt");
+	indicatorfd = open("/tmp/", O_PATH);
+	if (CHECK_FAIL(indicatorfd < 0))
+		goto out_close;
+
+	ret = set_pathname(pipefd[0], pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(pipefd[1], pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(sockfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(procfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(devfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(localfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(indicatorfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+
+	/* triggers vfs_getattr */
+	fstat(pipefd[0], &fileStat);
+	fstat(pipefd[1], &fileStat);
+	fstat(sockfd, &fileStat);
+	fstat(procfd, &fileStat);
+	fstat(devfd, &fileStat);
+	fstat(localfd, &fileStat);
+	fstat(indicatorfd, &fileStat);
+
+out_close:
+	/* triggers filp_close */
+	close(pipefd[0]);
+	close(pipefd[1]);
+	close(sockfd);
+	close(procfd);
+	close(devfd);
+	close(localfd);
+	close(indicatorfd);
+	return ret;
+}
+
+void test_d_path(void)
+{
+	struct test_d_path *skel;
+	struct d_path_test_data *dst;
+	__u32 duration = 0;
+	int err;
+
+	skel = test_d_path__open_and_load();
+	if (CHECK(!skel, "test_d_path_load", "d_path skeleton failed\n"))
+		goto cleanup;
+
+	err = test_d_path__attach(skel);
+	if (CHECK(err, "modify_return", "attach failed: %d\n", err))
+		goto cleanup;
+
+	dst = &skel->bss->data;
+	dst->pid = getpid();
+
+	err = trigger_fstat_events(skel->bss->data.pid);
+	if (CHECK_FAIL(err < 0))
+		goto cleanup;
+
+	for (int i = 0; i < MAX_FILES; i++) {
+		if (i < 3) {
+			CHECK((dst->paths_stat[i][0] == 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], dst->paths_stat[i]);
+			CHECK((dst->paths_close[i][0] == 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], dst->paths_close[i]);
+		} else {
+			CHECK(strncmp(src.paths[i], dst->paths_stat[i], MAX_PATH_LEN),
+			      "d_path",
+			      "failed to get stat path[%d]: %s vs %s\n",
+			      i, src.paths[i], dst->paths_stat[i]);
+			CHECK(strncmp(src.paths[i], dst->paths_close[i], MAX_PATH_LEN),
+			      "d_path",
+			      "failed to get close path[%d]: %s vs %s\n",
+			      i, src.paths[i], dst->paths_close[i]);
+		}
+	}
+
+cleanup:
+	test_d_path__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
new file mode 100644
index 000000000000..1b478c00ee7a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_d_path.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+#define MAX_PATH_LEN		128
+#define MAX_EVENT_NUM		16
+
+static struct d_path_test_data {
+	pid_t pid;
+	__u32 cnt_stat;
+	__u32 cnt_close;
+	char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
+	char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
+} data;
+
+struct path;
+struct kstat;
+
+SEC("fentry/vfs_getattr")
+int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
+	     __u32 request_mask, unsigned int query_flags)
+{
+	pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+	if (pid != data.pid)
+		return 0;
+
+	if (data.cnt_stat >= MAX_EVENT_NUM)
+		return 0;
+
+	bpf_d_path(path, data.paths_stat[data.cnt_stat], MAX_PATH_LEN);
+	data.cnt_stat++;
+	return 0;
+}
+
+SEC("fentry/filp_close")
+int BPF_PROG(prog_close, struct file *file, void *id)
+{
+	pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+	if (pid != data.pid)
+		return 0;
+
+	if (data.cnt_close >= MAX_EVENT_NUM)
+		return 0;
+
+	bpf_d_path((struct path *) &file->f_path,
+		   data.paths_close[data.cnt_close], MAX_PATH_LEN);
+	data.cnt_close++;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.25.4

Re: [PATCH 11/11] selftests/bpf: Add test for d_path helper

[Andrii Nakryiko]

On Tue, Jun 16, 2020 at 3:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test for d_path helper which is pretty much
> copied from Wenbo Zhang's test for bpf_get_fd_path,
> which never made it in.
>
> I've failed so far to compile the test with <linux/fs.h>
> kernel header, so for now adding 'struct file' with f_path
> member that has same offset as kernel's file object.
>
> Original-patch-by: Wenbo Zhang <ethercflow@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../testing/selftests/bpf/prog_tests/d_path.c | 153 ++++++++++++++++++
>  .../testing/selftests/bpf/progs/test_d_path.c |  55 +++++++
>  2 files changed, 208 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_d_path.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> new file mode 100644
> index 000000000000..e2b7dfeb506f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <test_progs.h>
> +#include <sys/stat.h>
> +#include <linux/sched.h>
> +#include <sys/syscall.h>
> +
> +#define MAX_PATH_LEN           128
> +#define MAX_FILES              7
> +#define MAX_EVENT_NUM          16
> +
> +struct d_path_test_data {
> +       pid_t pid;
> +       __u32 cnt_stat;
> +       __u32 cnt_close;
> +       char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
> +       char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
> +};

with skeleton there is no point in defining this container struct, and
especially duplicating it between BPF code and user-space code. Just
declare all those fields as global variables and access them from
skeleton directly.

[...]

> diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
> new file mode 100644
> index 000000000000..1b478c00ee7a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_d_path.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +#define MAX_PATH_LEN           128
> +#define MAX_EVENT_NUM          16
> +
> +static struct d_path_test_data {
> +       pid_t pid;
> +       __u32 cnt_stat;
> +       __u32 cnt_close;
> +       char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
> +       char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
> +} data;
> +
> +struct path;
> +struct kstat;

both structs are in vmlinux.h, you shouldn't need this.

[...]

>

Re: [PATCH 11/11] selftests/bpf: Add test for d_path helper

[Jiri Olsa]

On Thu, Jun 18, 2020 at 09:44:23PM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 16, 2020 at 3:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test for d_path helper which is pretty much
> > copied from Wenbo Zhang's test for bpf_get_fd_path,
> > which never made it in.
> >
> > I've failed so far to compile the test with <linux/fs.h>
> > kernel header, so for now adding 'struct file' with f_path
> > member that has same offset as kernel's file object.
> >
> > Original-patch-by: Wenbo Zhang <ethercflow@gmail.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../testing/selftests/bpf/prog_tests/d_path.c | 153 ++++++++++++++++++
> >  .../testing/selftests/bpf/progs/test_d_path.c |  55 +++++++
> >  2 files changed, 208 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_d_path.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > new file mode 100644
> > index 000000000000..e2b7dfeb506f
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > @@ -0,0 +1,153 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#define _GNU_SOURCE
> > +#include <test_progs.h>
> > +#include <sys/stat.h>
> > +#include <linux/sched.h>
> > +#include <sys/syscall.h>
> > +
> > +#define MAX_PATH_LEN           128
> > +#define MAX_FILES              7
> > +#define MAX_EVENT_NUM          16
> > +
> > +struct d_path_test_data {
> > +       pid_t pid;
> > +       __u32 cnt_stat;
> > +       __u32 cnt_close;
> > +       char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
> > +       char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
> > +};
> 
> with skeleton there is no point in defining this container struct, and
> especially duplicating it between BPF code and user-space code. Just
> declare all those fields as global variables and access them from
> skeleton directly.

ok, will check

> 
> [...]
> 
> > diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
> > new file mode 100644
> > index 000000000000..1b478c00ee7a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_d_path.c
> > @@ -0,0 +1,55 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include "vmlinux.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +#define MAX_PATH_LEN           128
> > +#define MAX_EVENT_NUM          16
> > +
> > +static struct d_path_test_data {
> > +       pid_t pid;
> > +       __u32 cnt_stat;
> > +       __u32 cnt_close;
> > +       char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
> > +       char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
> > +} data;
> > +
> > +struct path;
> > +struct kstat;
> 
> both structs are in vmlinux.h, you shouldn't need this.

leftover from earlier version, will remove

thanks,
jirka

RE: [PATCHv3 0/9] bpf: Add d_path helper

[John Fastabend]

Jiri Olsa wrote:
> hi,
> adding d_path helper to return full path for 'path' object.
> 
> I originally added and used 'file_path' helper, which did the same,
> but used 'struct file' object. Then realized that file_path is just
> a wrapper for d_path, so we'd cover more calling sites if we add
> d_path helper and allowed resolving BTF object within another object,
> so we could call d_path also with file pointer, like:
> 
>   bpf_d_path(&file->f_path, buf, size);
> 
> This feature is mainly to be able to add dpath (filepath originally)
> function to bpftrace:
> 
>   # bpftrace -e 'kfunc:vfs_open { printf("%s\n", dpath(args->path)); }'
> 
> v3 changes:
>   - changed tests to use seleton and vmlinux.h [Andrii]
>   - refactored to define ID lists in C object [Andrii]
>   - changed btf_struct_access for nested ID check,
>     instead of adding new function for that [Andrii]
>   - fail build with CONFIG_DEBUG_INFO_BTF if libelf is not detected [Andrii]
> 
> Also available at:
>   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   bpf/d_path
> 
> thanks,
> jirka

Hi Jira, Apologize for waiting until v3 to look at this series, but a
couple general requests as I review this.

In the cover letter can we get some more details. The above is really
terse/cryptic in my opinion. The bpftrace example gives good motiviation,
but nothing above mentions a new .BTF_ids section and the flow to create
and use this section.

Also if we add a BTF_ids  section adding documentation in btf.rst should
happen as well. I would like to see something in the ELF File Format
Interface section and BTF Generation sections.

I'm not going to nitpick if its in this series or a stand-alone patch
but do want to see it. So far the Documentation on BTF is fairly
good and I want to avoid these kind of gaps.

Thanks!
John

Re: [PATCHv3 0/9] bpf: Add d_path helper

[Jiri Olsa]

On Thu, Jun 18, 2020 at 01:57:19PM -0700, John Fastabend wrote:
> Jiri Olsa wrote:
> > hi,
> > adding d_path helper to return full path for 'path' object.
> > 
> > I originally added and used 'file_path' helper, which did the same,
> > but used 'struct file' object. Then realized that file_path is just
> > a wrapper for d_path, so we'd cover more calling sites if we add
> > d_path helper and allowed resolving BTF object within another object,
> > so we could call d_path also with file pointer, like:
> > 
> >   bpf_d_path(&file->f_path, buf, size);
> > 
> > This feature is mainly to be able to add dpath (filepath originally)
> > function to bpftrace:
> > 
> >   # bpftrace -e 'kfunc:vfs_open { printf("%s\n", dpath(args->path)); }'
> > 
> > v3 changes:
> >   - changed tests to use seleton and vmlinux.h [Andrii]
> >   - refactored to define ID lists in C object [Andrii]
> >   - changed btf_struct_access for nested ID check,
> >     instead of adding new function for that [Andrii]
> >   - fail build with CONFIG_DEBUG_INFO_BTF if libelf is not detected [Andrii]
> > 
> > Also available at:
> >   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >   bpf/d_path
> > 
> > thanks,
> > jirka
> 
> Hi Jira, Apologize for waiting until v3 to look at this series, but a
> couple general requests as I review this.
> 
> In the cover letter can we get some more details. The above is really
> terse/cryptic in my opinion. The bpftrace example gives good motiviation,
> but nothing above mentions a new .BTF_ids section and the flow to create
> and use this section.

ok, will add more details in next version

> 
> Also if we add a BTF_ids  section adding documentation in btf.rst should
> happen as well. I would like to see something in the ELF File Format
> Interface section and BTF Generation sections.

did not know there was bpf.rst ;-) will update

> 
> I'm not going to nitpick if its in this series or a stand-alone patch
> but do want to see it. So far the Documentation on BTF is fairly
> good and I want to avoid these kind of gaps.

sure, thanks

jirka

> 
> Thanks!
> John
>