[PATCH v4 bpf-next 00/14] bpf: Add d_path helper

[PATCH v4 bpf-next 00/14] bpf: Add d_path helper

[Jiri Olsa]

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

In a preparation for that, this patchset also adds support for BTF ID
whitelists, because d_path can't be called from any probe due to its
locks usage. The whitelists allow verifier to check if the caller is
one of the functions from the whitelist.

The whitelist is implemented in a generic way. This patchset introduces
macros that allow to define lists of BTF IDs, which are compiled in
the kernel image in a new .BTF.ids ELF section.

The generic way of BTF ID lists allows us to use them in other places
in kernel (than just for whitelists), that could use static BTF ID
values compiled in and it's also implemented in this patchset.

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)); }'

v4 changes:
  - added ID sanity checks in btf_resolve_helper_id [Andrii]
  - resolve bpf_ctx_convert via BTF_ID [Andrii]
  - keep bpf_access_type in btf_struct_access [Andrii]
  - rename whitelist to se and use struct btf_id_set [Andrii]
  - several fixes for d_path prog/verifier tests [Andrii]
  - added union and typedefs types support [Andrii]
  - rename btfid to resolve_btfids [Andrii]
  - fix segfault in resolve_btfids [John]
  - rename section from .BTF_ids .BTF.ids (following .BTF.ext example)
  - add .BTF.ids section info into btf.rst [John]
  - updated over letter with more details [John]

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

thanks,
jirka


---
Jiri Olsa (14):
      bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object
      bpf: Compile resolve_btfids tool at kernel compilation start
      bpf: Add BTF_ID_LIST/BTF_ID macros
      bpf: Resolve BTF IDs in vmlinux image
      bpf: Remove btf_id helpers resolving
      bpf: Use BTF_ID to resolve bpf_ctx_convert struct
      bpf: Allow nested BTF object to be refferenced by BTF object + offset
      bpf: Add BTF_SET_START/END macros
      bpf: Add info about .BTF.ids section to btf.rst
      bpf: Add d_path helper
      tools headers: Adopt verbatim copy of btf_ids.h from kernel sources
      selftests/bpf: Add verifier test for d_path helper
      selftests/bpf: Add test for d_path helper
      selftests/bpf: Add test for resolve_btfids

 Documentation/bpf/btf.rst                         |  53 ++++++++
 Makefile                                          |  25 +++-
 include/asm-generic/vmlinux.lds.h                 |   4 +
 include/linux/bpf.h                               |   7 +
 include/linux/btf_ids.h                           | 108 ++++++++++++++++
 include/uapi/linux/bpf.h                          |  14 +-
 kernel/bpf/btf.c                                  | 169 ++++++++++++------------
 kernel/bpf/verifier.c                             |  42 ++++--
 kernel/trace/bpf_trace.c                          |  56 +++++++-
 net/core/filter.c                                 |   9 +-
 scripts/bpf_helpers_doc.py                        |   2 +
 scripts/link-vmlinux.sh                           |   6 +
 tools/Makefile                                    |   3 +
 tools/bpf/Makefile                                |   5 +-
 tools/bpf/resolve_btfids/Build                    |  26 ++++
 tools/bpf/resolve_btfids/Makefile                 |  76 +++++++++++
 tools/bpf/resolve_btfids/main.c                   | 716 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/include/linux/btf_ids.h                     | 108 ++++++++++++++++
 tools/include/linux/compiler.h                    |   4 +
 tools/include/uapi/linux/bpf.h                    |  14 +-
 tools/testing/selftests/bpf/Makefile              |  20 ++-
 tools/testing/selftests/bpf/prog_tests/d_path.c   | 145 +++++++++++++++++++++
 tools/testing/selftests/bpf/progs/test_d_path.c   |  50 +++++++
 tools/testing/selftests/bpf/test_resolve_btfids.c | 201 +++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/test_verifier.c       |  19 ++-
 tools/testing/selftests/bpf/verifier/d_path.c     |  37 ++++++
 26 files changed, 1806 insertions(+), 113 deletions(-)
 create mode 100644 include/linux/btf_ids.h
 create mode 100644 tools/bpf/resolve_btfids/Build
 create mode 100644 tools/bpf/resolve_btfids/Makefile
 create mode 100644 tools/bpf/resolve_btfids/main.c
 create mode 100644 tools/include/linux/btf_ids.h
 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/test_resolve_btfids.c
 create mode 100644 tools/testing/selftests/bpf/verifier/d_path.c

[PATCH v4 bpf-next 01/14] bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object

[Jiri Olsa]

The resolve_btfids tool scans elf object for .BTF_ids section
and resolves its symbols with BTF ID values.

It will be used to during linking time to resolve arrays of BTF
ID values 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/resolve_btfids/Build    |  26 ++
 tools/bpf/resolve_btfids/Makefile |  76 ++++
 tools/bpf/resolve_btfids/main.c   | 716 ++++++++++++++++++++++++++++++
 3 files changed, 818 insertions(+)
 create mode 100644 tools/bpf/resolve_btfids/Build
 create mode 100644 tools/bpf/resolve_btfids/Makefile
 create mode 100644 tools/bpf/resolve_btfids/main.c

diff --git a/tools/bpf/resolve_btfids/Build b/tools/bpf/resolve_btfids/Build
new file mode 100644
index 000000000000..c7318cc55341
--- /dev/null
+++ b/tools/bpf/resolve_btfids/Build
@@ -0,0 +1,26 @@
+resolve_btfids-y += main.o
+resolve_btfids-y += rbtree.o
+resolve_btfids-y += zalloc.o
+resolve_btfids-y += string.o
+resolve_btfids-y += ctype.o
+resolve_btfids-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/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile
new file mode 100644
index 000000000000..f6502ff26573
--- /dev/null
+++ b/tools/bpf/resolve_btfids/Makefile
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+ifeq ($(srctree),)
+srctree := $(patsubst %/,%,$(dir $(CURDIR)))
+srctree := $(patsubst %/,%,$(dir $(srctree)))
+srctree := $(patsubst %/,%,$(dir $(srctree)))
+endif
+
+ifeq ($(V),1)
+  Q =
+  msg =
+else
+  Q = @
+  msg = @printf '  %-8s %s%s\n' "$(1)" "$(notdir $(2))" "$(if $(3), $(3))";
+  MAKEFLAGS=--no-print-directory
+endif
+
+OUTPUT := $(srctree)/tools/bpf/resolve_btfids/
+
+LIBBPF_SRC := $(srctree)/tools/lib/bpf/
+SUBCMD_SRC := $(srctree)/tools/lib/subcmd/
+
+BPFOBJ     := $(OUTPUT)/libbpf.a
+SUBCMDOBJ  := $(OUTPUT)/libsubcmd.a
+
+BINARY     := $(OUTPUT)/resolve_btfids
+BINARY_IN  := $(BINARY)-in.o
+
+all: $(BINARY)
+
+$(OUTPUT):
+	$(call msg,MKDIR,,$@)
+	$(Q)mkdir -p $(OUTPUT)
+
+$(SUBCMDOBJ): fixdep FORCE
+	$(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(OUTPUT)
+
+$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT)
+	$(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC)  OUTPUT=$(abspath $(dir $@))/ $(abspath $@)
+
+CFLAGS := -g \
+          -I$(srctree)/tools/include \
+          -I$(srctree)/tools/include/uapi \
+          -I$(LIBBPF_SRC) \
+          -I$(SUBCMD_SRC)
+
+LIBS = -lelf -lz
+
+export srctree OUTPUT CFLAGS Q
+include $(srctree)/tools/build/Makefile.include
+
+$(BINARY_IN): fixdep FORCE
+	$(Q)$(MAKE) $(build)=resolve_btfids
+
+$(BINARY): $(BPFOBJ) $(SUBCMDOBJ) $(BINARY_IN)
+	$(call msg,LINK,$@)
+	$(Q)$(CC) $(BINARY_IN) $(LDFLAGS) -o $@ $(BPFOBJ) $(SUBCMDOBJ) $(LIBS)
+
+libsubcmd-clean:
+	$(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(OUTPUT) clean
+
+libbpf-clean:
+	$(Q)$(MAKE) -C $(LIBBPF_SRC) OUTPUT=$(OUTPUT) clean
+
+clean: libsubcmd-clean libbpf-clean fixdep-clean
+	$(call msg,CLEAN,$(BINARY))
+	$(Q)$(RM) -f $(BINARY); \
+	find $(if $(OUTPUT),$(OUTPUT),.) -name \*.o -or -name \*.o.cmd -or -name \*.o.d | xargs $(RM)
+
+tags:
+	$(call msg,GEN,,tags)
+	$(Q)ctags -R . $(LIBBPF_SRC) $(SUBCMD_SRC)
+
+FORCE:
+
+.PHONY: all FORCE clean tags
diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
new file mode 100644
index 000000000000..d758e2bdbc9d
--- /dev/null
+++ b/tools/bpf/resolve_btfids/main.c
@@ -0,0 +1,716 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+
+/*
+ * resolve_btfids scans Elf object for .BTF_ids section and resolves
+ * its symbols with BTF ID values.
+ *
+ * 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 store its ID into the data:
+ *
+ *             __BTF_ID__func__vfs_close__1:
+ *             .zero 4
+ *
+ *   struct  - lookup BTF_KIND_STRUCT symbol with <symbol> name
+ *             and store its ID into the data:
+ *
+ *             __BTF_ID__struct__sk_buff__1:
+ *             .zero 4
+ *
+ *   union   - lookup BTF_KIND_UNION symbol with <symbol> name
+ *             and store its ID into the data:
+ *
+ *             __BTF_ID__union__thread_union__1:
+ *             .zero 4
+ *
+ *   typedef - lookup BTF_KIND_TYPEDEF symbol with <symbol> name
+ *             and store its ID into the data:
+ *
+ *             __BTF_ID__typedef__pid_t__1:
+ *             .zero 4
+ *
+ *   set     - store symbol size into first 4 bytes and sort following
+ *             ID list
+ *
+ *             __BTF_ID__set__list:
+ *             .zero 4
+ *             list:
+ *             __BTF_ID__func__vfs_getattr__3:
+ *             .zero 4
+ *             __BTF_ID__func__vfs_fallocate__4:
+ *             .zero 4
+ */
+
+#define  _GNU_SOURCE
+#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 <linux/err.h>
+#include <btf.h>
+#include <libbpf.h>
+#include <parse-options.h>
+
+#define BTF_IDS_SECTION	".BTF.ids"
+#define BTF_ID		"__BTF_ID__"
+
+#define BTF_STRUCT	"struct"
+#define BTF_UNION	"union"
+#define BTF_TYPEDEF	"typedef"
+#define BTF_FUNC	"func"
+#define BTF_SET		"set"
+
+#define ADDR_CNT	100
+
+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;
+	const char *btf;
+
+	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	sets;
+	struct rb_root	symbols;
+
+	int nr_funcs;
+	int nr_structs;
+	int nr_unions;
+	int nr_typedefs;
+};
+
+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_set(struct object *obj, char *name)
+{
+	char *id;
+
+	id = strdup(name + sizeof(BTF_SET) + sizeof("__") - 2);
+	if (!id) {
+		pr_err("FAILED to parse cnt name: %s\n", name);
+		return NULL;
+	}
+
+	return btf_id__add(&obj->sets, id, true);
+}
+
+static struct btf_id *add_symbol(struct object *obj, char *name, size_t size)
+{
+	char *id;
+
+	id = get_id(name + size);
+	if (!id) {
+		pr_err("FAILED to parse symbol name: %s\n", name);
+		return NULL;
+	}
+
+	return btf_id__add(&obj->symbols, 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, BTF_IDS_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;
+
+		/* struct */
+		if (!strncmp(prefix, BTF_STRUCT, sizeof(BTF_STRUCT) - 1)) {
+			obj->nr_structs++;
+			id = add_symbol(obj, prefix, sizeof(BTF_STRUCT) - 1);
+		/* union  */
+		} else if (!strncmp(prefix, BTF_UNION, sizeof(BTF_UNION) - 1)) {
+			obj->nr_unions++;
+			id = add_symbol(obj, prefix, sizeof(BTF_UNION) - 1);
+		/* typedef */
+		} else if (!strncmp(prefix, BTF_TYPEDEF, sizeof(BTF_TYPEDEF) - 1)) {
+			obj->nr_typedefs++;
+			id = add_symbol(obj, prefix, sizeof(BTF_TYPEDEF) - 1);
+		/* func */
+		} else if (!strncmp(prefix, BTF_FUNC, sizeof(BTF_FUNC) - 1)) {
+			obj->nr_funcs++;
+			id = add_symbol(obj, prefix, sizeof(BTF_FUNC) - 1);
+		/* set */
+		} else if (!strncmp(prefix, BTF_SET, sizeof(BTF_SET) - 1)) {
+			id = add_set(obj, prefix);
+			/*
+			 * SET objects store list's count, which is encoded
+			 * in symbol's size, together with 'cnt' field hence
+			 * that - 1.
+			 */
+			if (id)
+				id->cnt = sym.st_size / sizeof(int) - 1;
+		} 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 struct btf *btf__parse_raw(const char *file)
+{
+	struct btf *btf;
+	struct stat st;
+	__u8 *buf;
+	FILE *f;
+
+	if (stat(file, &st))
+		return NULL;
+
+	f = fopen(file, "rb");
+	if (!f)
+		return NULL;
+
+	buf = malloc(st.st_size);
+	if (!buf) {
+		btf = ERR_PTR(-ENOMEM);
+		goto exit_close;
+	}
+
+	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f)) {
+		btf = ERR_PTR(-EINVAL);
+		goto exit_free;
+	}
+
+	btf = btf__new(buf, st.st_size);
+
+exit_free:
+	free(buf);
+exit_close:
+	fclose(f);
+	return btf;
+}
+
+static bool is_btf_raw(const char *file)
+{
+	__u16 magic = 0;
+	int fd, nb_read;
+
+	fd = open(file, O_RDONLY);
+	if (fd < 0)
+		return false;
+
+	nb_read = read(fd, &magic, sizeof(magic));
+	close(fd);
+	return nb_read == sizeof(magic) && magic == BTF_MAGIC;
+}
+
+static struct btf *btf_open(const char *path)
+{
+	if (is_btf_raw(path))
+		return btf__parse_raw(path);
+	else
+		return btf__parse_elf(path, NULL);
+}
+
+static int symbols_resolve(struct object *obj)
+{
+	int nr_structs  = obj->nr_structs;
+	int nr_unions   = obj->nr_unions;
+	int nr_typedefs = obj->nr_typedefs;
+	int nr_funcs    = obj->nr_funcs;
+	struct btf *btf;
+	int err, type_id;
+	__u32 nr;
+
+	btf = btf_open(obj->btf ?: obj->path);
+	err = libbpf_get_error(btf);
+	if (err) {
+		pr_err("FAILED: load BTF from %s: %s",
+			obj->path, strerror(err));
+		return -1;
+	}
+
+	err = -1;
+	nr  = btf__get_nr_types(btf);
+
+	/*
+	 * Iterate all the BTF types and search for collected symbol IDs.
+	 */
+	for (type_id = 1; 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) {
+			pr_err("FAILED: malformed BTF, can't resolve type for ID %d\n",
+				type_id);
+			goto out;
+		}
+
+		if (btf_is_func(type) && nr_funcs)
+			nr = &nr_funcs;
+		else if (btf_is_struct(type) && nr_structs)
+			nr = &nr_structs;
+		else if (btf_is_union(type) && nr_unions)
+			nr = &nr_unions;
+		else if (btf_is_typedef(type) && nr_typedefs)
+			nr = &nr_typedefs;
+		else
+			continue;
+
+		str = btf__name_by_offset(btf, type->name_off);
+		if (!str) {
+			pr_err("FAILED: malformed BTF, can't resolve name for ID %d\n",
+				type_id);
+			goto out;
+		}
+
+		id = btf_id__find(&obj->symbols, str);
+		if (id) {
+			id->id = type_id;
+			(*nr)--;
+		}
+	}
+
+	err = 0;
+out:
+	btf__free(btf);
+	return err;
+}
+
+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 sets_patch(struct object *obj)
+{
+	Elf_Data *data = obj->efile.idlist;
+	int *ptr = data->d_buf;
+	struct rb_node *next;
+
+	next = rb_first(&obj->sets);
+	while (next) {
+		unsigned long addr, idx;
+		struct btf_id *id;
+		int *base;
+		int cnt;
+
+		id   = rb_entry(next, struct btf_id, rb_node);
+		addr = id->addr[0];
+		idx  = addr - obj->efile.idlist_addr;
+
+		/* sets are unique */
+		if (id->addr_cnt != 1) {
+			pr_err("FAILED malformed data for set '%s'\n",
+				id->name);
+			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->symbols) ||
+	    __symbols_patch(obj, &obj->sets))
+		return -1;
+
+	if (sets_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 resolve_btfids_usage[] = {
+	"resolve_btfids [<options>] <ELF object>",
+	NULL
+};
+
+int main(int argc, const char **argv)
+{
+	struct object obj = {
+		.efile = {
+			.idlist_shndx  = -1,
+			.symbols_shndx = -1,
+		},
+		.symbols = RB_ROOT,
+		.sets    = RB_ROOT,
+	};
+	struct option btfid_options[] = {
+		OPT_INCR('v', "verbose", &verbose,
+			 "be more verbose (show errors, etc)"),
+		OPT_STRING(0, "btf", &obj.btf, "BTF data",
+			   "BTF data"),
+		OPT_END()
+	};
+	int err = -1;
+
+	argc = parse_options(argc, argv, btfid_options, resolve_btfids_usage,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
+	if (argc != 1)
+		usage_with_options(resolve_btfids_usage, btfid_options);
+
+	obj.path = argv[0];
+
+	if (elf_collect(&obj))
+		goto out;
+
+	if (symbols_collect(&obj))
+		goto out;
+
+	if (symbols_resolve(&obj))
+		goto out;
+
+	if (symbols_patch(&obj))
+		goto out;
+
+	err = 0;
+out:
+	if (obj.efile.elf)
+		elf_end(obj.efile.elf);
+	close(obj.efile.fd);
+	return err;
+}
-- 
2.25.4

[PATCH v4 bpf-next 02/14] bpf: Compile resolve_btfids tool at kernel compilation start

[Jiri Olsa]

The resolve_btfids 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 ae5d8220f431..8db4fd8097e0 100644
--- a/Makefile
+++ b/Makefile
@@ -1053,9 +1053,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
@@ -1064,6 +1065,14 @@ ifdef CONFIG_STACK_VALIDATION
   endif
 endif
 
+ifdef CONFIG_DEBUG_INFO_BTF
+  ifeq ($(has_libelf),1)
+    resolve_btfids_target := tools/bpf/resolve_btfids FORCE
+  else
+    ERROR_RESOLVE_BTFIDS := 1
+  endif
+endif
+
 PHONY += prepare0
 
 export MODORDER := $(extmod-prefix)modules.order
@@ -1175,7 +1184,7 @@ prepare0: archprepare
 	$(Q)$(MAKE) $(build)=.
 
 # All the preparing..
-prepare: prepare0 prepare-objtool
+prepare: prepare0 prepare-objtool prepare-resolve_btfids
 
 # Support for using generic headers in asm-generic
 asm-generic := -f $(srctree)/scripts/Makefile.asm-generic obj
@@ -1188,7 +1197,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-resolve_btfids
 prepare-objtool: $(objtool_target)
 ifeq ($(SKIP_STACK_VALIDATION),1)
 ifdef CONFIG_UNWINDER_ORC
@@ -1199,6 +1208,11 @@ else
 endif
 endif
 
+prepare-resolve_btfids: $(resolve_btfids_target)
+ifeq ($(ERROR_RESOLVE_BTFIDS),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 6df1850f8353..89ae235b790e 100644
--- a/tools/bpf/Makefile
+++ b/tools/bpf/Makefile
@@ -123,5 +123,8 @@ runqslower_install:
 runqslower_clean:
 	$(call descend,runqslower,clean)
 
+resolve_btfids:
+	$(call descend,resolve_btfids)
+
 .PHONY: all install clean bpftool bpftool_install bpftool_clean \
-	runqslower runqslower_install runqslower_clean
+	runqslower runqslower_install runqslower_clean resolve_btfids
-- 
2.25.4

[PATCH v4 bpf-next 03/14] bpf: Add BTF_ID_LIST/BTF_ID macros

[Jiri Olsa]

Adding support to generate .BTF_ids section that will hold BTF
ID lists for verifier.

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

Following defines list of one BTF ID value:

  BTF_ID_LIST(bpf_skb_output_btf_ids)
  BTF_ID(struct, sk_buff)

It also defines following variable to access the list:

  extern int bpf_skb_output_btf_ids[];

Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/asm-generic/vmlinux.lds.h |  4 ++
 include/linux/btf_ids.h           | 69 +++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100644 include/linux/btf_ids.h

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index db600ef218d7..a9b77a4314a8 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/include/linux/btf_ids.h b/include/linux/btf_ids.h
new file mode 100644
index 000000000000..f7f9dc4d9a9f
--- /dev/null
+++ b/include/linux/btf_ids.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_BTF_IDS_H
+#define _LINUX_BTF_IDS_H 1
+
+#include <linux/compiler.h> /* for __PASTE */
+
+/*
+ * 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 BTF_IDS_SECTION ".BTF.ids"
+
+#define ____BTF_ID(symbol)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
+".local " #symbol " ;                          \n"	\
+".type  " #symbol ", @object;                  \n"	\
+".size  " #symbol ", 4;                        \n"	\
+#symbol ":                                     \n"	\
+".zero 4                                       \n"	\
+".popsection;                                  \n");
+
+#define __BTF_ID(symbol) \
+	____BTF_ID(symbol)
+
+#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 " BTF_IDS_SECTION ",\"a\";       \n"	\
+".local " #name ";                             \n"	\
+#name ":;                                      \n"	\
+".popsection;                                  \n");	\
+
+#define BTF_ID_LIST(name)				\
+__BTF_ID_LIST(name)					\
+extern int name[];
+
+#endif
-- 
2.25.4

[PATCH v4 bpf-next 04/14] bpf: Resolve BTF IDs in vmlinux image

[Jiri Olsa]

Using BTF_ID_LIST macro to define lists for several helpers
using BTF arguments.

And running resolve_btfids on vmlinux elf object during linking,
so the .BTF_ids section gets the IDs resolved.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 Makefile                 | 3 ++-
 kernel/trace/bpf_trace.c | 9 +++++++--
 net/core/filter.c        | 9 +++++++--
 scripts/link-vmlinux.sh  | 6 ++++++
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 8db4fd8097e0..def58d4f9ed7 100644
--- a/Makefile
+++ b/Makefile
@@ -448,6 +448,7 @@ OBJSIZE		= $(CROSS_COMPILE)size
 STRIP		= $(CROSS_COMPILE)strip
 endif
 PAHOLE		= pahole
+RESOLVE_BTFIDS	= $(srctree)/tools/bpf/resolve_btfids/resolve_btfids
 LEX		= flex
 YACC		= bison
 AWK		= awk
@@ -510,7 +511,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 RESOLVE_BTFIDS LEX YACC AWK INSTALLKERNEL
 export PERL PYTHON PYTHON3 CHECK CHECKFLAGS MAKE UTS_MACHINE HOSTCXX
 export KGZIP KBZIP2 KLZOP LZMA LZ4 XZ
 export KBUILD_HOSTCXXFLAGS KBUILD_HOSTLDFLAGS KBUILD_HOSTLDLIBS LDFLAGS_MODULE
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 5d59dda5f661..b124d468688c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -13,6 +13,7 @@
 #include <linux/kprobes.h>
 #include <linux/syscalls.h>
 #include <linux/error-injection.h>
+#include <linux/btf_ids.h>
 
 #include <asm/tlb.h>
 
@@ -704,7 +705,9 @@ 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];
+BTF_ID_LIST(bpf_seq_printf_btf_ids)
+BTF_ID(struct, seq_file)
+
 static const struct bpf_func_proto bpf_seq_printf_proto = {
 	.func		= bpf_seq_printf,
 	.gpl_only	= true,
@@ -722,7 +725,9 @@ 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];
+BTF_ID_LIST(bpf_seq_write_btf_ids)
+BTF_ID(struct, seq_file)
+
 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 c796e141ea8e..2f0dc67454cc 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -75,6 +75,7 @@
 #include <net/ipv6_stubs.h>
 #include <net/bpf_sk_storage.h>
 #include <net/transp_v6.h>
+#include <linux/btf_ids.h>
 
 /**
  *	sk_filter_trim_cap - run a packet through a socket filter
@@ -3779,7 +3780,9 @@ 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];
+BTF_ID_LIST(bpf_skb_output_btf_ids)
+BTF_ID(struct, sk_buff)
+
 const struct bpf_func_proto bpf_skb_output_proto = {
 	.func		= bpf_skb_event_output,
 	.gpl_only	= true,
@@ -4173,7 +4176,9 @@ 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];
+BTF_ID_LIST(bpf_xdp_output_btf_ids)
+BTF_ID(struct, xdp_buff)
+
 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 92dd745906f4..e26f02dbedee 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 BTFIDS vmlinux
+${RESOLVE_BTFIDS} vmlinux
+fi
+
 if [ -n "${CONFIG_BUILDTIME_TABLE_SORT}" ]; then
 	info SORTTAB vmlinux
 	if ! sorttable vmlinux; then
-- 
2.25.4

[PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

[Jiri Olsa]

Now when we moved the helpers btf_id arrays 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 | 90 +++++-------------------------------------------
 1 file changed, 8 insertions(+), 82 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 4c3007f428b1..4da6b0770ff9 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4079,96 +4079,22 @@ 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)
+int btf_resolve_helper_id(struct bpf_verifier_log *log,
+			  const struct bpf_func_proto *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;
+	int id;
 
-	if (IS_ERR(btf_vmlinux)) {
-		bpf_log(log, "btf_vmlinux is malformed\n");
+	if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
 		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);
+	if (WARN_ON_ONCE(!fn->btf_id))
 		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;
-}
+	id = fn->btf_id[arg];
 
-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)
+	if (!id || id > btf_vmlinux->nr_types)
 		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;
+	return id;
 }
 
 static int __get_type_size(struct btf *btf, u32 btf_id,
-- 
2.25.4

[PATCH v4 bpf-next 06/14] bpf: Use BTF_ID to resolve bpf_ctx_convert struct

[Jiri Olsa]

This way the ID is resolved during compile time,
and we can remove the runtime name search.

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

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 4da6b0770ff9..701a2cb5dfb2 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -18,6 +18,7 @@
 #include <linux/sort.h>
 #include <linux/bpf_verifier.h>
 #include <linux/btf.h>
+#include <linux/btf_ids.h>
 #include <linux/skmsg.h>
 #include <linux/perf_event.h>
 #include <net/sock.h>
@@ -3621,6 +3622,9 @@ static int btf_translate_to_vmlinux(struct bpf_verifier_log *log,
 	return kern_ctx_type->type;
 }
 
+BTF_ID_LIST(bpf_ctx_convert_btf_id)
+BTF_ID(struct, bpf_ctx_convert)
+
 struct btf *btf_parse_vmlinux(void)
 {
 	struct btf_verifier_env *env = NULL;
@@ -3659,10 +3663,10 @@ struct btf *btf_parse_vmlinux(void)
 	if (err)
 		goto errout;
 
-	/* find struct bpf_ctx_convert for type checking later */
-	btf_id = btf_find_by_name_kind(btf, "bpf_ctx_convert", BTF_KIND_STRUCT);
-	if (btf_id < 0) {
-		err = btf_id;
+	/* struct bpf_ctx_convert for type checking later */
+	btf_id = bpf_ctx_convert_btf_id[0];
+	if (btf_id <= 0) {
+		err = -EINVAL;
 		goto errout;
 	}
 	/* btf_parse_vmlinux() runs under bpf_verifier_lock */
-- 
2.25.4

[PATCH v4 bpf-next 07/14] 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 will 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.

Using btf_struct_access to implement new btf_struct_address
function, because it already walks down the given BTF object.

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

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 3d2ade703a35..c0fd1f3037dd 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1300,6 +1300,9 @@ 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_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 701a2cb5dfb2..f87e5f1dc64d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3863,10 +3863,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,
-		      enum bpf_access_type atype,
-		      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;
@@ -3914,8 +3926,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,
-					 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",
@@ -4043,9 +4054,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;
@@ -4059,7 +4082,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;
 			}
 		}
@@ -4083,6 +4106,36 @@ 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,
+		      enum bpf_access_type atype __maybe_unused,
+		      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)
+{
+	const struct btf_type *type;
+	struct access_data data = {
+		.op = ACCESS_EXPECT,
+	};
+
+	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 7de98906ddf4..da7351184295 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3808,6 +3808,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
 	enum bpf_reg_type expected_type, type = reg->type;
 	enum bpf_arg_type arg_type = fn->arg_type[arg];
+	const struct btf_type *btf_type;
 	int err = 0;
 
 	if (arg_type == ARG_DONTCARE)
@@ -3887,24 +3888,34 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		expected_type = PTR_TO_BTF_ID;
 		if (type != expected_type)
 			goto err_type;
-		if (!fn->check_btf_id) {
-			if (reg->btf_id != meta->btf_id) {
-				verbose(env, "Helper has type %s got %s in R%d\n",
+		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 (!fn->check_btf_id) {
+				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;
+				}
+			} else if (!fn->check_btf_id(reg->btf_id, arg)) {
+				verbose(env, "Helper does not support %s in R%d\n",
 					kernel_type_name(reg->btf_id), regno);
 
 				return -EACCES;
 			}
-		} else if (!fn->check_btf_id(reg->btf_id, arg)) {
-			verbose(env, "Helper does not support %s in R%d\n",
-				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 (!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;
+			}
 		}
 	} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
 		if (meta->func_id == BPF_FUNC_spin_lock) {
-- 
2.25.4

[PATCH v4 bpf-next 08/14] bpf: Add BTF_SET_START/END macros

[Jiri Olsa]

Adding support to define sorted set of BTF ID values.

Following defines sorted set of BTF ID values:

  BTF_SET_START(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_SET_END(btf_whitelist_d_path)

It defines following 'struct btf_id_set' variable to access
values and count:

  struct btf_id_set btf_whitelist_d_path;

Adding 'allowed' callback to struct bpf_func_proto, to allow
verifier the check on allowed callers.

Adding btf_id_set_contains, which will be used by allowed
callbacks to verify the caller's BTF ID value is within
allowed set.

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

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c0fd1f3037dd..9efd0c23147d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -294,6 +294,7 @@ struct bpf_func_proto {
 						    * for this argument.
 						    */
 	int *ret_btf_id; /* return value btf_id */
+	bool (*allowed)(const struct bpf_prog *prog);
 };
 
 /* bpf_context is intentionally undefined structure. Pointer to bpf_context is
@@ -1771,4 +1772,7 @@ enum bpf_text_poke_type {
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
 		       void *addr1, void *addr2);
 
+struct btf_id_set;
+bool btf_id_set_contains(struct btf_id_set *set, u32 id);
+
 #endif /* _LINUX_BPF_H */
diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index f7f9dc4d9a9f..5eb8983ee0a0 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -66,4 +66,43 @@ asm(							\
 __BTF_ID_LIST(name)					\
 extern int name[];
 
+struct btf_id_set {
+	u32 cnt;
+	u32 ids[];
+};
+
+/*
+ * The BTF_SET_START/END macros pair defines sorted list of
+ * BTF IDs plus its members count, with following layout:
+ *
+ * BTF_SET_START(list)
+ * BTF_ID(type1, name1)
+ * BTF_ID(type2, name2)
+ * BTF_SET_END(list)
+ *
+ * __BTF_ID__set__list:
+ * .zero 4
+ * list:
+ * __BTF_ID__type1__name1__3:
+ * .zero 4
+ * __BTF_ID__type2__name2__4:
+ * .zero 4
+ *
+ */
+#define BTF_SET_START(name)				\
+__BTF_ID_LIST(name)					\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
+".local __BTF_ID__set__" #name ";              \n"	\
+"__BTF_ID__set__" #name ":;                    \n"	\
+".zero 4                                       \n"	\
+".popsection;                                  \n");	\
+
+#define BTF_SET_END(name)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";      \n"	\
+".size __BTF_ID__set__" #name ", .-" #name "  \n"	\
+".popsection;                                 \n");	\
+extern struct btf_id_set name;
+
 #endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f87e5f1dc64d..201fe564bb84 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -21,6 +21,8 @@
 #include <linux/btf_ids.h>
 #include <linux/skmsg.h>
 #include <linux/perf_event.h>
+#include <linux/bsearch.h>
+#include <linux/btf_ids.h>
 #include <net/sock.h>
 
 /* BTF (BPF Type Format) is the meta data format which describes
@@ -4712,3 +4714,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_id_set_contains(struct btf_id_set *set, u32 id)
+{
+	return bsearch(&id, set->ids, set->cnt, sizeof(int), btf_id_cmp_func) != NULL;
+}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index da7351184295..b6f583e3e24f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4708,6 +4708,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

[PATCH v4 bpf-next 09/14] bpf: Add info about .BTF.ids section to btf.rst

[Jiri Olsa]

Updating btf.rst doc with info about .BTF.ids section

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 Documentation/bpf/btf.rst | 53 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
index 4d565d202ce3..60cfb3b06f34 100644
--- a/Documentation/bpf/btf.rst
+++ b/Documentation/bpf/btf.rst
@@ -691,6 +691,59 @@ kernel API, the ``insn_off`` is the instruction offset in the unit of ``struct
 bpf_insn``. For ELF API, the ``insn_off`` is the byte offset from the
 beginning of section (``btf_ext_info_sec->sec_name_off``).
 
+4.2 .BTF.ids section
+====================
+
+The .BTF.ids section encodes BTF ID values that are used within the kernel.
+
+This section is created during the kernel compilation with the help of
+macros defined in ``include/linux/btf_ids.h`` header file. Kernel code can
+use them to create lists and sets (sorted lists) of BTF ID values.
+
+The ``BTF_ID_LIST`` macro defines unsorted list of BTF ID values,
+with following syntax::
+
+  BTF_ID_LIST(list)
+  BTF_ID(type1, name1)
+  BTF_ID(type2, name2)
+
+resulting in following layout in .BTF.ids section::
+
+  __BTF_ID__type1__name1__1:
+  .zero 4
+  __BTF_ID__type2__name2__2:
+  .zero 4
+
+The ``int list[];`` variable is defined to access the list.
+
+The ``BTF_SET_START/END`` macros pair defines sorted list of BTF ID values
+and their count, with following syntax::
+
+  BTF_SET_START(set)
+  BTF_ID(type1, name1)
+  BTF_ID(type2, name2)
+  BTF_SET_END(set)
+
+resulting in following layout in .BTF.ids section::
+
+  __BTF_ID__set__set:
+  .zero 4
+  __BTF_ID__type1__name1__3:
+  .zero 4
+  __BTF_ID__type2__name2__4:
+  .zero 4
+
+The ``struct btf_id_set set;`` variable is defined to access the list.
+
+The ``typeX`` name can be one of following::
+
+   struct, union, typedef, func
+
+and is used as a filter whem resolving the BTF ID value.
+
+All the BTF ID lists and sets are compiled in the .BTF.ids section and
+resolved during the linking phase of kernel build by ``resolve_btfids`` tool.
+
 5. Using BTF
 ************
 
-- 
2.25.4

[PATCH v4 bpf-next 10/14] 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/uapi/linux/bpf.h       | 14 +++++++++-
 kernel/trace/bpf_trace.c       | 47 ++++++++++++++++++++++++++++++++++
 scripts/bpf_helpers_doc.py     |  2 ++
 tools/include/uapi/linux/bpf.h | 14 +++++++++-
 4 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 0cb8ec948816..23274c81f244 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3285,6 +3285,17 @@ union bpf_attr {
  *		Dynamically cast a *sk* pointer to a *udp6_sock* pointer.
  *	Return
  *		*sk* if casting is valid, or NULL otherwise.
+ *
+ * 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),			\
@@ -3427,7 +3438,8 @@ union bpf_attr {
 	FN(skc_to_tcp_sock),		\
 	FN(skc_to_tcp_timewait_sock),	\
 	FN(skc_to_tcp_request_sock),	\
-	FN(skc_to_udp6_sock),
+	FN(skc_to_udp6_sock),		\
+	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/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b124d468688c..6f31e21565b6 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1060,6 +1060,51 @@ 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;
+}
+
+BTF_SET_START(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_SET_END(btf_whitelist_d_path)
+
+static bool bpf_d_path_allowed(const struct bpf_prog *prog)
+{
+	return btf_id_set_contains(&btf_whitelist_d_path, prog->aux->attach_btf_id);
+}
+
+BTF_ID_LIST(bpf_d_path_btf_ids)
+BTF_ID(struct, path)
+
+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)
 {
@@ -1539,6 +1584,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 6bab40ff442e..bacc0a2856c7 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -430,6 +430,7 @@ class PrinterHelpers(Printer):
             'struct __sk_buff',
             'struct sk_msg_md',
             'struct xdp_md',
+            'struct path',
     ]
     known_types = {
             '...',
@@ -468,6 +469,7 @@ class PrinterHelpers(Printer):
             'struct tcp_timewait_sock',
             'struct tcp_request_sock',
             'struct udp6_sock',
+            'struct path',
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 0cb8ec948816..23274c81f244 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3285,6 +3285,17 @@ union bpf_attr {
  *		Dynamically cast a *sk* pointer to a *udp6_sock* pointer.
  *	Return
  *		*sk* if casting is valid, or NULL otherwise.
+ *
+ * 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),			\
@@ -3427,7 +3438,8 @@ union bpf_attr {
 	FN(skc_to_tcp_sock),		\
 	FN(skc_to_tcp_timewait_sock),	\
 	FN(skc_to_tcp_request_sock),	\
-	FN(skc_to_udp6_sock),
+	FN(skc_to_udp6_sock),		\
+	FN(d_path),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
-- 
2.25.4

[PATCH v4 bpf-next 11/14] tools headers: Adopt verbatim copy of btf_ids.h from kernel sources

[Jiri Olsa]

It will be needed by bpf selftest for resolve_btfids tool.

Also adding __PASTE macro as btf_ids.h dependency, which is
defined in:

  include/linux/compiler_types.h

but because tools/include do not have this header, I'm putting
the macro into linux/compiler.h header.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/include/linux/btf_ids.h  | 108 +++++++++++++++++++++++++++++++++
 tools/include/linux/compiler.h |   4 ++
 2 files changed, 112 insertions(+)
 create mode 100644 tools/include/linux/btf_ids.h

diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
new file mode 100644
index 000000000000..5eb8983ee0a0
--- /dev/null
+++ b/tools/include/linux/btf_ids.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_BTF_IDS_H
+#define _LINUX_BTF_IDS_H 1
+
+#include <linux/compiler.h> /* for __PASTE */
+
+/*
+ * 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 BTF_IDS_SECTION ".BTF.ids"
+
+#define ____BTF_ID(symbol)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
+".local " #symbol " ;                          \n"	\
+".type  " #symbol ", @object;                  \n"	\
+".size  " #symbol ", 4;                        \n"	\
+#symbol ":                                     \n"	\
+".zero 4                                       \n"	\
+".popsection;                                  \n");
+
+#define __BTF_ID(symbol) \
+	____BTF_ID(symbol)
+
+#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 " BTF_IDS_SECTION ",\"a\";       \n"	\
+".local " #name ";                             \n"	\
+#name ":;                                      \n"	\
+".popsection;                                  \n");	\
+
+#define BTF_ID_LIST(name)				\
+__BTF_ID_LIST(name)					\
+extern int name[];
+
+struct btf_id_set {
+	u32 cnt;
+	u32 ids[];
+};
+
+/*
+ * The BTF_SET_START/END macros pair defines sorted list of
+ * BTF IDs plus its members count, with following layout:
+ *
+ * BTF_SET_START(list)
+ * BTF_ID(type1, name1)
+ * BTF_ID(type2, name2)
+ * BTF_SET_END(list)
+ *
+ * __BTF_ID__set__list:
+ * .zero 4
+ * list:
+ * __BTF_ID__type1__name1__3:
+ * .zero 4
+ * __BTF_ID__type2__name2__4:
+ * .zero 4
+ *
+ */
+#define BTF_SET_START(name)				\
+__BTF_ID_LIST(name)					\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
+".local __BTF_ID__set__" #name ";              \n"	\
+"__BTF_ID__set__" #name ":;                    \n"	\
+".zero 4                                       \n"	\
+".popsection;                                  \n");	\
+
+#define BTF_SET_END(name)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";      \n"	\
+".size __BTF_ID__set__" #name ", .-" #name "  \n"	\
+".popsection;                                 \n");	\
+extern struct btf_id_set name;
+
+#endif
diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
index 9f9002734e19..6eac24d44e81 100644
--- a/tools/include/linux/compiler.h
+++ b/tools/include/linux/compiler.h
@@ -201,4 +201,8 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 # define __fallthrough
 #endif
 
+/* Indirect macros required for expanded argument pasting, eg. __LINE__. */
+#define ___PASTE(a, b) a##b
+#define __PASTE(a, b) ___PASTE(a, b)
+
 #endif /* _TOOLS_LINUX_COMPILER_H */
-- 
2.25.4

[PATCH v4 bpf-next 12/14] 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   | 19 +++++++++-
 tools/testing/selftests/bpf/verifier/d_path.c | 37 +++++++++++++++++++
 2 files changed, 55 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..9be395d9dc64 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,24 @@ 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 < 0) {
+			printf("FAIL\nFailed to find BTF ID for '%s'!\n",
+				test->kfunc);
+			(*errors)++;
+			return;
+		}
+	}
+
 	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..b988396379a7
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/d_path.c
@@ -0,0 +1,37 @@
+{
+	"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(),
+	},
+	.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

[PATCH v4 bpf-next 13/14] 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 | 145 ++++++++++++++++++
 .../testing/selftests/bpf/progs/test_d_path.c |  50 ++++++
 2 files changed, 195 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..c0ea45d43634
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
@@ -0,0 +1,145 @@
+// 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
+
+#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__bss *bss;
+	struct test_d_path *skel;
+	__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;
+
+	bss = skel->bss;
+	bss->my_pid = getpid();
+
+	err = trigger_fstat_events(bss->my_pid);
+	if (CHECK_FAIL(err < 0))
+		goto cleanup;
+
+	for (int i = 0; i < MAX_FILES; i++) {
+		if (i < 3) {
+			CHECK((bss->paths_stat[i][0] == 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], bss->paths_stat[i]);
+			CHECK((bss->paths_close[i][0] == 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], bss->paths_close[i]);
+		} else {
+			CHECK(strncmp(src.paths[i], bss->paths_stat[i], MAX_PATH_LEN),
+			      "d_path",
+			      "failed to get stat path[%d]: %s vs %s\n",
+			      i, src.paths[i], bss->paths_stat[i]);
+			CHECK(strncmp(src.paths[i], bss->paths_close[i], MAX_PATH_LEN),
+			      "d_path",
+			      "failed to get close path[%d]: %s vs %s\n",
+			      i, src.paths[i], bss->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..6096aef2bafc
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_d_path.c
@@ -0,0 +1,50 @@
+// 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
+
+pid_t my_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];
+
+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 != my_pid)
+		return 0;
+
+	if (cnt_stat >= MAX_EVENT_NUM)
+		return 0;
+
+	bpf_d_path(path, paths_stat[cnt_stat], MAX_PATH_LEN);
+	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 != my_pid)
+		return 0;
+
+	if (cnt_close >= MAX_EVENT_NUM)
+		return 0;
+
+	bpf_d_path((struct path *) &file->f_path,
+		   paths_close[cnt_close], MAX_PATH_LEN);
+	cnt_close++;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.25.4

[PATCH v4 bpf-next 14/14] selftests/bpf: Add test for resolve_btfids

[Jiri Olsa]

Adding test to resolve_btfids tool, that:
  - creates binary with BTF IDs list and set
  - process the binary with resolve_btfids tool
  - verifies that correct BTF ID values are in place

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/Makefile          |  20 +-
 .../selftests/bpf/test_resolve_btfids.c       | 201 ++++++++++++++++++
 2 files changed, 220 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/test_resolve_btfids.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 22aaec74ea0a..547322a5feff 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -37,7 +37,8 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
 	test_cgroup_storage \
 	test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
 	test_progs-no_alu32 \
-	test_current_pid_tgid_new_ns
+	test_current_pid_tgid_new_ns \
+	test_resolve_btfids
 
 # Also test bpf-gcc, if present
 ifneq ($(BPF_GCC),)
@@ -427,6 +428,23 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
 	$(call msg,BINARY,,$@)
 	$(CC) $(LDFLAGS) -o $@ $(filter %.a %.o,$^) $(LDLIBS)
 
+# test_resolve_btfids
+#
+$(SCRATCH_DIR)/resolve_btfids: $(BPFOBJ) FORCE
+	$(Q)$(MAKE) $(submake_extras) -C $(TOOLSDIR)/bpf/resolve_btfids	\
+		    OUTPUT=$(SCRATCH_DIR)/ BPFOBJ=$(BPFOBJ)
+
+$(OUTPUT)/test_resolve_btfids.o: test_resolve_btfids.c
+	$(call msg,CC,,$@)
+	$(CC) $(CFLAGS) -I$(TOOLSINCDIR) -D"BUILD_STR(s)=#s" -DVMLINUX_BTF="BUILD_STR($(VMLINUX_BTF))" -c -o $@ $<
+
+.PHONY: FORCE
+
+$(OUTPUT)/test_resolve_btfids: $(OUTPUT)/test_resolve_btfids.o $(SCRATCH_DIR)/resolve_btfids
+	$(call msg,BINARY,,$@)
+	$(CC) -o $@ $< $(BPFOBJ) -lelf -lz && \
+	$(SCRATCH_DIR)/resolve_btfids --btf $(VMLINUX_BTF) $@
+
 EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR)			\
 	prog_tests/tests.h map_tests/tests.h verifier/tests.h		\
 	feature								\
diff --git a/tools/testing/selftests/bpf/test_resolve_btfids.c b/tools/testing/selftests/bpf/test_resolve_btfids.c
new file mode 100644
index 000000000000..48aeda2ed881
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_resolve_btfids.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <string.h>
+#include <stdio.h>
+#include <sys/stat.h>
+#include <stdio.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <linux/err.h>
+#include <stdlib.h>
+#include <bpf/btf.h>
+#include <bpf/libbpf.h>
+#include <linux/btf.h>
+#include <linux/kernel.h>
+#include <linux/btf_ids.h>
+
+#define __CHECK(condition, format...) ({				\
+	int __ret = !!(condition);					\
+	if (__ret) {							\
+		fprintf(stderr, "%s:%d:FAIL ", __func__, __LINE__);	\
+		fprintf(stderr, format);				\
+	}								\
+	__ret;								\
+})
+
+#define CHECK(condition, format...)					\
+	do {								\
+		if (__CHECK(condition, format))				\
+			return -1;					\
+	} while (0)
+
+static struct btf *btf__parse_raw(const char *file)
+{
+	struct btf *btf;
+	struct stat st;
+	__u8 *buf;
+	FILE *f;
+
+	if (stat(file, &st))
+		return NULL;
+
+	f = fopen(file, "rb");
+	if (!f)
+		return NULL;
+
+	buf = malloc(st.st_size);
+	if (!buf) {
+		btf = ERR_PTR(-ENOMEM);
+		goto exit_close;
+	}
+
+	if ((size_t) st.st_size != fread(buf, 1, st.st_size, f)) {
+		btf = ERR_PTR(-EINVAL);
+		goto exit_free;
+	}
+
+	btf = btf__new(buf, st.st_size);
+
+exit_free:
+	free(buf);
+exit_close:
+	fclose(f);
+	return btf;
+}
+
+static bool is_btf_raw(const char *file)
+{
+	__u16 magic = 0;
+	int fd, nb_read;
+
+	fd = open(file, O_RDONLY);
+	if (fd < 0)
+		return false;
+
+	nb_read = read(fd, &magic, sizeof(magic));
+	close(fd);
+	return nb_read == sizeof(magic) && magic == BTF_MAGIC;
+}
+
+static struct btf *btf_open(const char *path)
+{
+	if (is_btf_raw(path))
+		return btf__parse_raw(path);
+	else
+		return btf__parse_elf(path, NULL);
+}
+
+BTF_ID_LIST(test_list)
+BTF_ID(typedef, pid_t)
+BTF_ID(struct,  sk_buff)
+BTF_ID(union,   thread_union)
+BTF_ID(func,    memcpy)
+
+BTF_SET_START(test_set)
+BTF_ID(typedef, pid_t)
+BTF_ID(struct,  sk_buff)
+BTF_ID(union,   thread_union)
+BTF_ID(func,    memcpy)
+BTF_SET_END(test_set)
+
+struct symbol {
+	const char	*name;
+	int		 type;
+	int		 id;
+};
+
+struct symbol test_symbols[] = {
+	{ "pid_t",        BTF_KIND_TYPEDEF, -1 },
+	{ "sk_buff",      BTF_KIND_STRUCT,  -1 },
+	{ "thread_union", BTF_KIND_UNION,   -1 },
+	{ "memcpy",       BTF_KIND_FUNC,    -1 },
+};
+
+static int
+__resolve_symbol(struct btf *btf, int type_id)
+{
+	const struct btf_type *type;
+	const char *str;
+	unsigned int i;
+
+	type = btf__type_by_id(btf, type_id);
+	CHECK(!type, "Failed to get type for ID %d\n", type_id);
+
+	for (i = 0; i < ARRAY_SIZE(test_symbols); i++) {
+		if (test_symbols[i].id != -1)
+			continue;
+
+		if (BTF_INFO_KIND(type->info) != test_symbols[i].type)
+			continue;
+
+		str = btf__name_by_offset(btf, type->name_off);
+		if (!str) {
+			fprintf(stderr, "failed to get name for BTF ID %d\n",
+				type_id);
+			continue;
+		}
+
+		if (!strcmp(str, test_symbols[i].name))
+			test_symbols[i].id = type_id;
+	}
+
+	return 0;
+}
+
+static int resolve_symbols(void)
+{
+	const char *path = VMLINUX_BTF;
+	struct btf *btf;
+	int type_id;
+	__u32 nr;
+	int err;
+
+	btf = btf_open(path);
+	CHECK(libbpf_get_error(btf), "Failed to load BTF from %s\n", path);
+
+	nr = btf__get_nr_types(btf);
+
+	for (type_id = 0; type_id < nr; type_id++) {
+		err = __resolve_symbol(btf, type_id);
+		if (__CHECK(err, "Failed to resolve symbols\n"))
+			break;
+	}
+
+	btf__free(btf);
+	return 0;
+}
+
+int main(int argc, char **argv)
+{
+	unsigned int i, j;
+
+	CHECK(resolve_symbols(), "symbol resolve failed\n");
+
+	/* Check BTF_SET_START(test_set) IDs */
+	for (i = 0; i < test_set.cnt; i++) {
+		bool found = false;
+
+		for (j = 0; j < test_set.cnt; j++) {
+			if (test_symbols[j].id != test_set.ids[i])
+				continue;
+			found = true;
+			break;
+		}
+
+		CHECK(!found, "ID %d not found in test_symbols\n",
+		      test_set.ids[i]);
+
+		if (i > 0) {
+			CHECK(test_set.ids[i - 1] > test_set.ids[i],
+			      "test_set is not sorted\n");
+		}
+	}
+
+	/* Check BTF_ID_LIST(test_list) IDs */
+	for (i = 0; i < ARRAY_SIZE(test_symbols); i++) {
+		CHECK(test_list[i] != test_symbols[i].id,
+		      "wrong ID for %s\n", test_symbols[i].name);
+	}
+
+	return 0;
+}
-- 
2.25.4

Re: [PATCH v4 bpf-next 10/14] bpf: Add d_path helper

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:49 PM 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/uapi/linux/bpf.h       | 14 +++++++++-
>  kernel/trace/bpf_trace.c       | 47 ++++++++++++++++++++++++++++++++++
>  scripts/bpf_helpers_doc.py     |  2 ++
>  tools/include/uapi/linux/bpf.h | 14 +++++++++-
>  4 files changed, 75 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 0cb8ec948816..23274c81f244 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3285,6 +3285,17 @@ union bpf_attr {
>   *             Dynamically cast a *sk* pointer to a *udp6_sock* pointer.
>   *     Return
>   *             *sk* if casting is valid, or NULL otherwise.
> + *
> + * 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

It's important to note whether string is always zero-terminated (I'm
guessing it is, right?).

> + *
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -3427,7 +3438,8 @@ union bpf_attr {
>         FN(skc_to_tcp_sock),            \
>         FN(skc_to_tcp_timewait_sock),   \
>         FN(skc_to_tcp_request_sock),    \
> -       FN(skc_to_udp6_sock),
> +       FN(skc_to_udp6_sock),           \
> +       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/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index b124d468688c..6f31e21565b6 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1060,6 +1060,51 @@ 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;

if len above is zero, you won't zero-terminate it, so probably better
to move buf[len] = 0 out of if to do unconditionally

> +               }
> +       }
> +
> +       return len;
> +}
> +
> +BTF_SET_START(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_SET_END(btf_whitelist_d_path)
> +
> +static bool bpf_d_path_allowed(const struct bpf_prog *prog)
> +{
> +       return btf_id_set_contains(&btf_whitelist_d_path, prog->aux->attach_btf_id);
> +}
> +

This looks pretty great and clean, considering what's happening under
the covers. Nice work, thanks a lot!

> +BTF_ID_LIST(bpf_d_path_btf_ids)
> +BTF_ID(struct, path)

this is a bit more confusing to read and error-prone, but I couldn't
come up with any better way to do this... Still better than
alternatives.

> +
> +static const struct bpf_func_proto bpf_d_path_proto = {
> +       .func           = bpf_d_path,
> +       .gpl_only       = true,

Does it have to be GPL-only? What's the criteria? Sorry if this was
brought up previously.

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

[...]

Re: [PATCH v4 bpf-next 01/14] bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:48 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The resolve_btfids tool scans elf object for .BTF_ids section
> and resolves its symbols with BTF ID values.
>
> It will be used to during linking time to resolve arrays of BTF
> ID values 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/resolve_btfids/Build    |  26 ++
>  tools/bpf/resolve_btfids/Makefile |  76 ++++
>  tools/bpf/resolve_btfids/main.c   | 716 ++++++++++++++++++++++++++++++
>  3 files changed, 818 insertions(+)
>  create mode 100644 tools/bpf/resolve_btfids/Build
>  create mode 100644 tools/bpf/resolve_btfids/Makefile
>  create mode 100644 tools/bpf/resolve_btfids/main.c
>

LGTM.

Acked-by: Andrii Nakryiko <andriin@fb.com>

[...]

Re: [PATCH v4 bpf-next 01/14] bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object

[Yonghong Song]

On 6/25/20 3:12 PM, Jiri Olsa wrote:
> The resolve_btfids tool scans elf object for .BTF_ids section
> and resolves its symbols with BTF ID values.
> 
> It will be used to during linking time to resolve arrays of BTF
> ID values 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/resolve_btfids/Build    |  26 ++
>   tools/bpf/resolve_btfids/Makefile |  76 ++++
>   tools/bpf/resolve_btfids/main.c   | 716 ++++++++++++++++++++++++++++++
>   3 files changed, 818 insertions(+)
>   create mode 100644 tools/bpf/resolve_btfids/Build
>   create mode 100644 tools/bpf/resolve_btfids/Makefile
>   create mode 100644 tools/bpf/resolve_btfids/main.c
> 
> diff --git a/tools/bpf/resolve_btfids/Build b/tools/bpf/resolve_btfids/Build
> new file mode 100644
> index 000000000000..c7318cc55341
> --- /dev/null
> +++ b/tools/bpf/resolve_btfids/Build
> @@ -0,0 +1,26 @@
> +resolve_btfids-y += main.o
> +resolve_btfids-y += rbtree.o
> +resolve_btfids-y += zalloc.o
> +resolve_btfids-y += string.o
> +resolve_btfids-y += ctype.o
> +resolve_btfids-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/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile
> new file mode 100644
> index 000000000000..f6502ff26573
> --- /dev/null
> +++ b/tools/bpf/resolve_btfids/Makefile
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +ifeq ($(srctree),)
> +srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +srctree := $(patsubst %/,%,$(dir $(srctree)))
> +endif
> +
> +ifeq ($(V),1)
> +  Q =
> +  msg =
> +else
> +  Q = @
> +  msg = @printf '  %-8s %s%s\n' "$(1)" "$(notdir $(2))" "$(if $(3), $(3))";
> +  MAKEFLAGS=--no-print-directory
> +endif
> +
> +OUTPUT := $(srctree)/tools/bpf/resolve_btfids/
> +
> +LIBBPF_SRC := $(srctree)/tools/lib/bpf/
> +SUBCMD_SRC := $(srctree)/tools/lib/subcmd/
> +
> +BPFOBJ     := $(OUTPUT)/libbpf.a
> +SUBCMDOBJ  := $(OUTPUT)/libsubcmd.a
> +
> +BINARY     := $(OUTPUT)/resolve_btfids
> +BINARY_IN  := $(BINARY)-in.o
> +
> +all: $(BINARY)
> +
> +$(OUTPUT):
> +	$(call msg,MKDIR,,$@)
> +	$(Q)mkdir -p $(OUTPUT)
> +
> +$(SUBCMDOBJ): fixdep FORCE
> +	$(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(OUTPUT)
> +
> +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT)
> +	$(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC)  OUTPUT=$(abspath $(dir $@))/ $(abspath $@)
> +
> +CFLAGS := -g \
> +          -I$(srctree)/tools/include \
> +          -I$(srctree)/tools/include/uapi \
> +          -I$(LIBBPF_SRC) \
> +          -I$(SUBCMD_SRC)
> +
> +LIBS = -lelf -lz
> +
> +export srctree OUTPUT CFLAGS Q
> +include $(srctree)/tools/build/Makefile.include
> +
> +$(BINARY_IN): fixdep FORCE
> +	$(Q)$(MAKE) $(build)=resolve_btfids
> +
> +$(BINARY): $(BPFOBJ) $(SUBCMDOBJ) $(BINARY_IN)
> +	$(call msg,LINK,$@)
> +	$(Q)$(CC) $(BINARY_IN) $(LDFLAGS) -o $@ $(BPFOBJ) $(SUBCMDOBJ) $(LIBS)
> +
> +libsubcmd-clean:
> +	$(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(OUTPUT) clean
> +
> +libbpf-clean:
> +	$(Q)$(MAKE) -C $(LIBBPF_SRC) OUTPUT=$(OUTPUT) clean
> +
> +clean: libsubcmd-clean libbpf-clean fixdep-clean
> +	$(call msg,CLEAN,$(BINARY))
> +	$(Q)$(RM) -f $(BINARY); \
> +	find $(if $(OUTPUT),$(OUTPUT),.) -name \*.o -or -name \*.o.cmd -or -name \*.o.d | xargs $(RM)
> +
> +tags:
> +	$(call msg,GEN,,tags)
> +	$(Q)ctags -R . $(LIBBPF_SRC) $(SUBCMD_SRC)
> +
> +FORCE:
> +
> +.PHONY: all FORCE clean tags

After applying the whole patch set to my bpf-next tree locally, I cannot 
build:

-bash-4.4$ make -j100 && make vmlinux
   GEN     Makefile
   DESCEND  objtool
   DESCEND  bpf/resolve_btfids
make[4]: *** No rule to make target 
`/data/users/yhs/work/net-next/tools/bpf/resolve_btfids/fixdep'.  Stop.
make[3]: *** [fixdep] Error 2
make[2]: *** [bpf/resolve_btfids] Error 2
make[1]: *** [tools/bpf/resolve_btfids] Error 2
make[1]: *** Waiting for unfinished jobs....
make[1]: *** wait: No child processes.  Stop.
make: *** [__sub-make] Error 2
-bash-4.4$

Any clue what is the possible issue here?

> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> new file mode 100644
> index 000000000000..d758e2bdbc9d
> --- /dev/null
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -0,0 +1,716 @@
> +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> +
> +/*
> + * resolve_btfids scans Elf object for .BTF_ids section and resolves
[...]

Re: [PATCH v4 bpf-next 02/14] bpf: Compile resolve_btfids tool at kernel compilation start

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:47 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The resolve_btfids 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>
> ---

Not sure about clean target, but otherwise looks good to me.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  Makefile           | 22 ++++++++++++++++++----
>  tools/Makefile     |  3 +++
>  tools/bpf/Makefile |  5 ++++-
>  3 files changed, 25 insertions(+), 5 deletions(-)
>

[...]

> diff --git a/tools/bpf/Makefile b/tools/bpf/Makefile
> index 6df1850f8353..89ae235b790e 100644
> --- a/tools/bpf/Makefile
> +++ b/tools/bpf/Makefile
> @@ -123,5 +123,8 @@ runqslower_install:
>  runqslower_clean:
>         $(call descend,runqslower,clean)
>
> +resolve_btfids:
> +       $(call descend,resolve_btfids)
> +

I think we talked about this. Did we decide that resolve_btfids_clean
is not necessary?

>  .PHONY: all install clean bpftool bpftool_install bpftool_clean \
> -       runqslower runqslower_install runqslower_clean
> +       runqslower runqslower_install runqslower_clean resolve_btfids
> --
> 2.25.4
>

Re: [PATCH v4 bpf-next 03/14] bpf: Add BTF_ID_LIST/BTF_ID macros

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 3:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to generate .BTF_ids section that will hold BTF
> ID lists for verifier.
>
> Adding macros that will help to define lists of BTF ID values
> placed in .BTF_ids section. They are initially filled with zeros
> (during compilation) and resolved later during the linking phase
> by resolve_btfids tool.
>
> Following defines list of one BTF ID value:
>
>   BTF_ID_LIST(bpf_skb_output_btf_ids)
>   BTF_ID(struct, sk_buff)
>
> It also defines following variable to access the list:
>
>   extern int bpf_skb_output_btf_ids[];
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Looks good, with few nits below.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  include/asm-generic/vmlinux.lds.h |  4 ++
>  include/linux/btf_ids.h           | 69 +++++++++++++++++++++++++++++++
>  2 files changed, 73 insertions(+)
>  create mode 100644 include/linux/btf_ids.h
>

[...]

> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> new file mode 100644
> index 000000000000..f7f9dc4d9a9f
> --- /dev/null
> +++ b/include/linux/btf_ids.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _LINUX_BTF_IDS_H
> +#define _LINUX_BTF_IDS_H 1

this "1", is it necessary? I think it's always just `#define HEADER_GUARD`?

> +
> +#include <linux/compiler.h> /* for __PASTE */
> +

[...]

> +#define __BTF_ID_LIST(name)                            \
> +asm(                                                   \
> +".pushsection " BTF_IDS_SECTION ",\"a\";       \n"     \
> +".local " #name ";                             \n"     \
> +#name ":;                                      \n"     \
> +".popsection;                                  \n");   \
> +
> +#define BTF_ID_LIST(name)                              \
> +__BTF_ID_LIST(name)                                    \
> +extern int name[];

nit: extern u32 (or __u32) perhaps?

> +
> +#endif
> --
> 2.25.4
>

Re: [PATCH v4 bpf-next 04/14] bpf: Resolve BTF IDs in vmlinux image

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:48 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Using BTF_ID_LIST macro to define lists for several helpers
> using BTF arguments.
>
> And running resolve_btfids on vmlinux elf object during linking,
> so the .BTF_ids section gets the IDs resolved.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  Makefile                 | 3 ++-
>  kernel/trace/bpf_trace.c | 9 +++++++--
>  net/core/filter.c        | 9 +++++++--
>  scripts/link-vmlinux.sh  | 6 ++++++
>  4 files changed, 22 insertions(+), 5 deletions(-)

[...]

Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

[Yonghong Song]

On 6/25/20 3:12 PM, Jiri Olsa wrote:
> Now when we moved the helpers btf_id arrays 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 | 90 +++++-------------------------------------------
>   1 file changed, 8 insertions(+), 82 deletions(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 4c3007f428b1..4da6b0770ff9 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4079,96 +4079,22 @@ 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)
> +int btf_resolve_helper_id(struct bpf_verifier_log *log,
> +			  const struct bpf_func_proto *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;
> +	int id;
>   
> -	if (IS_ERR(btf_vmlinux)) {
> -		bpf_log(log, "btf_vmlinux is malformed\n");
> +	if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
>   		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);
> +	if (WARN_ON_ONCE(!fn->btf_id))

The original code does not have this warning. It directly did
"ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID.

>   		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;
> -}
> +	id = fn->btf_id[arg];

The corresponding BTF_ID definition here is:
   BTF_ID_LIST(bpf_skb_output_btf_ids)
   BTF_ID(struct, sk_buff)

The bpf helper writer needs to ensure proper declarations
of BTF_IDs like the above matching helpers definition.
Support we have arg1 and arg3 as BTF_ID. then the list
definition may be

   BTF_ID_LIST(bpf_skb_output_btf_ids)
   BTF_ID(struct, sk_buff)
   BTF_ID(struct, __unused)
   BTF_ID(struct, task_struct)

This probably okay, I guess.

>   
> -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)
> +	if (!id || id > btf_vmlinux->nr_types)
>   		return -EINVAL;

id == 0 if btf_id cannot be resolved by resolve_btfids, right?
when id may be greater than btf_vmlinux->nr_types? If resolve_btfids 
application did incorrect transformation?

Anyway, this is to resolve helper meta btf_id. Even if you
return a btf_id > btf_vmlinux->nr_types, verifier will reject
since it will never be the same as the real parameter btf_id.
I would drop id > btf_vmlinux->nr_types here. This should never
happen for a correct tool. Even if it does, verifier will take
care of it.

> -
> -	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;
> +	return id;
>   }
>   
>   static int __get_type_size(struct btf *btf, u32 btf_id,
> 

Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

[Andrii Nakryiko]

On Fri, Jun 26, 2020 at 2:37 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 6/25/20 3:12 PM, Jiri Olsa wrote:
> > Now when we moved the helpers btf_id arrays 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 | 90 +++++-------------------------------------------
> >   1 file changed, 8 insertions(+), 82 deletions(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 4c3007f428b1..4da6b0770ff9 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -4079,96 +4079,22 @@ 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)
> > +int btf_resolve_helper_id(struct bpf_verifier_log *log,
> > +                       const struct bpf_func_proto *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;
> > +     int id;
> >
> > -     if (IS_ERR(btf_vmlinux)) {
> > -             bpf_log(log, "btf_vmlinux is malformed\n");
> > +     if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
> >               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);
> > +     if (WARN_ON_ONCE(!fn->btf_id))
>
> The original code does not have this warning. It directly did
> "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID.
>
> >               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;
> > -}
> > +     id = fn->btf_id[arg];
>
> The corresponding BTF_ID definition here is:
>    BTF_ID_LIST(bpf_skb_output_btf_ids)
>    BTF_ID(struct, sk_buff)
>
> The bpf helper writer needs to ensure proper declarations
> of BTF_IDs like the above matching helpers definition.
> Support we have arg1 and arg3 as BTF_ID. then the list
> definition may be
>
>    BTF_ID_LIST(bpf_skb_output_btf_ids)
>    BTF_ID(struct, sk_buff)
>    BTF_ID(struct, __unused)
>    BTF_ID(struct, task_struct)
>
> This probably okay, I guess.
>
> >
> > -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)
> > +     if (!id || id > btf_vmlinux->nr_types)
> >               return -EINVAL;
>
> id == 0 if btf_id cannot be resolved by resolve_btfids, right?
> when id may be greater than btf_vmlinux->nr_types? If resolve_btfids
> application did incorrect transformation?
>
> Anyway, this is to resolve helper meta btf_id. Even if you
> return a btf_id > btf_vmlinux->nr_types, verifier will reject
> since it will never be the same as the real parameter btf_id.
> I would drop id > btf_vmlinux->nr_types here. This should never
> happen for a correct tool. Even if it does, verifier will take
> care of it.
>

I'd love to hear Alexei's thoughts about this change as well. Jiri
removed not just BTF ID resolution, but also all the sanity checks.
This now means more trust in helper definitions to not screw up
anything. It's probably OK, but still something to consciously think
about.

> > -
> > -     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;
> > +     return id;
> >   }
> >
> >   static int __get_type_size(struct btf *btf, u32 btf_id,
> >

Re: [PATCH v4 bpf-next 06/14] bpf: Use BTF_ID to resolve bpf_ctx_convert struct

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:47 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> This way the ID is resolved during compile time,
> and we can remove the runtime name search.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

This is a good improvement, but I was thinking about actually
simplifying btf_get_prog_ctx_type() logic itself (it also does all
this BTF ID resolution). But this is a nice first step, so:

Acked-by: Andrii Nakryiko <andriin@fb.com>


[...]

Re: [PATCH v4 bpf-next 06/14] bpf: Use BTF_ID to resolve bpf_ctx_convert struct

[Yonghong Song]

On 6/25/20 3:12 PM, Jiri Olsa wrote:
> This way the ID is resolved during compile time,
> and we can remove the runtime name search.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   kernel/bpf/btf.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 4da6b0770ff9..701a2cb5dfb2 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -18,6 +18,7 @@
>   #include <linux/sort.h>
>   #include <linux/bpf_verifier.h>
>   #include <linux/btf.h>
> +#include <linux/btf_ids.h>
>   #include <linux/skmsg.h>
>   #include <linux/perf_event.h>
>   #include <net/sock.h>
> @@ -3621,6 +3622,9 @@ static int btf_translate_to_vmlinux(struct bpf_verifier_log *log,
>   	return kern_ctx_type->type;
>   }
>   
> +BTF_ID_LIST(bpf_ctx_convert_btf_id)
> +BTF_ID(struct, bpf_ctx_convert)
> +
>   struct btf *btf_parse_vmlinux(void)
>   {
>   	struct btf_verifier_env *env = NULL;
> @@ -3659,10 +3663,10 @@ struct btf *btf_parse_vmlinux(void)
>   	if (err)
>   		goto errout;
>   
> -	/* find struct bpf_ctx_convert for type checking later */
> -	btf_id = btf_find_by_name_kind(btf, "bpf_ctx_convert", BTF_KIND_STRUCT);
> -	if (btf_id < 0) {
> -		err = btf_id;
> +	/* struct bpf_ctx_convert for type checking later */
> +	btf_id = bpf_ctx_convert_btf_id[0];
> +	if (btf_id <= 0) {

Just want to double check. Is it possible btf_id < 0 since previous 
patch did not check < 0?

> +		err = -EINVAL;
>   		goto errout;
>   	}
>   	/* btf_parse_vmlinux() runs under bpf_verifier_lock */
> 

Re: [PATCH v4 bpf-next 08/14] bpf: Add BTF_SET_START/END macros

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:48 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to define sorted set of BTF ID values.
>
> Following defines sorted set of BTF ID values:
>
>   BTF_SET_START(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_SET_END(btf_whitelist_d_path)
>
> It defines following 'struct btf_id_set' variable to access
> values and count:
>
>   struct btf_id_set btf_whitelist_d_path;
>
> Adding 'allowed' callback to struct bpf_func_proto, to allow
> verifier the check on allowed callers.
>
> Adding btf_id_set_contains, which will be used by allowed
> callbacks to verify the caller's BTF ID value is within
> allowed set.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

This looks nice!

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  include/linux/bpf.h     |  4 ++++
>  include/linux/btf_ids.h | 39 +++++++++++++++++++++++++++++++++++++++
>  kernel/bpf/btf.c        | 14 ++++++++++++++
>  kernel/bpf/verifier.c   |  5 +++++
>  4 files changed, 62 insertions(+)
>

[...]

Re: [PATCH v4 bpf-next 11/14] tools headers: Adopt verbatim copy of btf_ids.h from kernel sources

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:48 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> It will be needed by bpf selftest for resolve_btfids tool.
>
> Also adding __PASTE macro as btf_ids.h dependency, which is
> defined in:
>
>   include/linux/compiler_types.h
>
> but because tools/include do not have this header, I'm putting
> the macro into linux/compiler.h header.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

I don't like `#define GUARD 1`, but besides that:

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/include/linux/btf_ids.h  | 108 +++++++++++++++++++++++++++++++++
>  tools/include/linux/compiler.h |   4 ++
>  2 files changed, 112 insertions(+)
>  create mode 100644 tools/include/linux/btf_ids.h
>

[...]

Re: [PATCH v4 bpf-next 13/14] selftests/bpf: Add test for d_path helper

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:49 PM 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 | 145 ++++++++++++++++++
>  .../testing/selftests/bpf/progs/test_d_path.c |  50 ++++++
>  2 files changed, 195 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/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
> new file mode 100644
> index 000000000000..6096aef2bafc
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_d_path.c
> @@ -0,0 +1,50 @@
> +// 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
> +
> +pid_t my_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];
> +
> +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 != my_pid)
> +               return 0;
> +
> +       if (cnt_stat >= MAX_EVENT_NUM)
> +               return 0;
> +
> +       bpf_d_path(path, paths_stat[cnt_stat], MAX_PATH_LEN);
> +       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 != my_pid)
> +               return 0;
> +
> +       if (cnt_close >= MAX_EVENT_NUM)
> +               return 0;
> +
> +       bpf_d_path((struct path *) &file->f_path,
> +                  paths_close[cnt_close], MAX_PATH_LEN);

Can you please capture the return result of bpf_d_path() (here and
above) and validate that it's correct? That will help avoid future
breakages if anyone changes this.

> +       cnt_close++;
> +       return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.25.4
>

Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

[Yonghong Song]

On 6/26/20 2:40 PM, Andrii Nakryiko wrote:
> On Fri, Jun 26, 2020 at 2:37 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 6/25/20 3:12 PM, Jiri Olsa wrote:
>>> Now when we moved the helpers btf_id arrays 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 | 90 +++++-------------------------------------------
>>>    1 file changed, 8 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>> index 4c3007f428b1..4da6b0770ff9 100644
>>> --- a/kernel/bpf/btf.c
>>> +++ b/kernel/bpf/btf.c
>>> @@ -4079,96 +4079,22 @@ 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)
>>> +int btf_resolve_helper_id(struct bpf_verifier_log *log,
>>> +                       const struct bpf_func_proto *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;
>>> +     int id;
>>>
>>> -     if (IS_ERR(btf_vmlinux)) {
>>> -             bpf_log(log, "btf_vmlinux is malformed\n");
>>> +     if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
>>>                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);
>>> +     if (WARN_ON_ONCE(!fn->btf_id))
>>
>> The original code does not have this warning. It directly did
>> "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID.
>>
>>>                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;
>>> -}
>>> +     id = fn->btf_id[arg];
>>
>> The corresponding BTF_ID definition here is:
>>     BTF_ID_LIST(bpf_skb_output_btf_ids)
>>     BTF_ID(struct, sk_buff)
>>
>> The bpf helper writer needs to ensure proper declarations
>> of BTF_IDs like the above matching helpers definition.
>> Support we have arg1 and arg3 as BTF_ID. then the list
>> definition may be
>>
>>     BTF_ID_LIST(bpf_skb_output_btf_ids)
>>     BTF_ID(struct, sk_buff)
>>     BTF_ID(struct, __unused)
>>     BTF_ID(struct, task_struct)
>>
>> This probably okay, I guess.
>>
>>>
>>> -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)
>>> +     if (!id || id > btf_vmlinux->nr_types)
>>>                return -EINVAL;
>>
>> id == 0 if btf_id cannot be resolved by resolve_btfids, right?
>> when id may be greater than btf_vmlinux->nr_types? If resolve_btfids
>> application did incorrect transformation?
>>
>> Anyway, this is to resolve helper meta btf_id. Even if you
>> return a btf_id > btf_vmlinux->nr_types, verifier will reject
>> since it will never be the same as the real parameter btf_id.
>> I would drop id > btf_vmlinux->nr_types here. This should never
>> happen for a correct tool. Even if it does, verifier will take
>> care of it.
>>
> 
> I'd love to hear Alexei's thoughts about this change as well. Jiri
> removed not just BTF ID resolution, but also all the sanity checks.
> This now means more trust in helper definitions to not screw up
> anything. It's probably OK, but still something to consciously think
> about.

The kernel will have to trust the result. Otherwise, things may go bad.
For example, if the tool incorrectly calculated the type_id to be 100
although the correct id is 50, and the bpf program happens to have
the type id 100 for the parameter. Then the helper is allowed, this
could crash the kernel... I do not know whether we have security
issue here or not.

> 
>>> -
>>> -     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;
>>> +     return id;
>>>    }
>>>
>>>    static int __get_type_size(struct btf *btf, u32 btf_id,
>>>

Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

[Alexei Starovoitov]

On Fri, Jun 26, 2020 at 04:29:23PM -0700, Yonghong Song wrote:
> > > > 
> > > > -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)
> > > > +     if (!id || id > btf_vmlinux->nr_types)
> > > >                return -EINVAL;
> > > 
> > > id == 0 if btf_id cannot be resolved by resolve_btfids, right?
> > > when id may be greater than btf_vmlinux->nr_types? If resolve_btfids
> > > application did incorrect transformation?
> > > 
> > > Anyway, this is to resolve helper meta btf_id. Even if you
> > > return a btf_id > btf_vmlinux->nr_types, verifier will reject
> > > since it will never be the same as the real parameter btf_id.
> > > I would drop id > btf_vmlinux->nr_types here. This should never
> > > happen for a correct tool. Even if it does, verifier will take
> > > care of it.
> > > 
> > 
> > I'd love to hear Alexei's thoughts about this change as well. Jiri
> > removed not just BTF ID resolution, but also all the sanity checks.
> > This now means more trust in helper definitions to not screw up
> > anything. It's probably OK, but still something to consciously think
> > about.
> 
> The kernel will have to trust the result. 

+1
I think 'if (!id || id > btf_vmlinux->nr_types)' at run-time and
other sanity checks I saw in other patches are unnecessary.
resolve_btfids should do all checks and fail vmlinux linking.
We trust gcc to generate correct assembly code in the first place
and correct dwarf. We trust pahole do correct BTF conversion from
dwarf and dedup. We should trust resolve_btfids.
It's imo the simplest tool comparing to gcc.
btf_parse_vmlinux() is doing basic sanity check of BTF mainly
to populate 'struct btf *btf_vmlinux;' for further use.
I think we can add a scan over resolved btfids to btf_parse_vmlinux()
to make sure that all ids are within the range, but I don't think
it's mandatory for this patch set. Would be a reasonable sanity
check for the future. Of course, checking for sorted set_start/end
is overkill. Basic simplest sanity is ok.

Re: [PATCH v4 bpf-next 01/14] bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object

[Alexei Starovoitov]

On Fri, Jun 26, 2020 at 02:09:53PM -0700, Yonghong Song wrote:
> 
> After applying the whole patch set to my bpf-next tree locally, I cannot
> build:
> 
> -bash-4.4$ make -j100 && make vmlinux
>   GEN     Makefile
>   DESCEND  objtool
>   DESCEND  bpf/resolve_btfids
> make[4]: *** No rule to make target
> `/data/users/yhs/work/net-next/tools/bpf/resolve_btfids/fixdep'.  Stop.
> make[3]: *** [fixdep] Error 2
> make[2]: *** [bpf/resolve_btfids] Error 2
> make[1]: *** [tools/bpf/resolve_btfids] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make[1]: *** wait: No child processes.  Stop.
> make: *** [__sub-make] Error 2
> -bash-4.4$
> 
> Any clue what is the possible issue here?

Same here. After applying patch 1 and 2 it doesn't build for me 
with the same error as above.

But if I do:
cd tools/bpf/resolve_btfids; make; cd -; make
it works.

Re: [PATCH v4 bpf-next 01/14] bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object

[Jiri Olsa]

On Sun, Jun 28, 2020 at 12:09:27PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 26, 2020 at 02:09:53PM -0700, Yonghong Song wrote:
> > 
> > After applying the whole patch set to my bpf-next tree locally, I cannot
> > build:
> > 
> > -bash-4.4$ make -j100 && make vmlinux
> >   GEN     Makefile
> >   DESCEND  objtool
> >   DESCEND  bpf/resolve_btfids
> > make[4]: *** No rule to make target
> > `/data/users/yhs/work/net-next/tools/bpf/resolve_btfids/fixdep'.  Stop.
> > make[3]: *** [fixdep] Error 2
> > make[2]: *** [bpf/resolve_btfids] Error 2
> > make[1]: *** [tools/bpf/resolve_btfids] Error 2
> > make[1]: *** Waiting for unfinished jobs....
> > make[1]: *** wait: No child processes.  Stop.
> > make: *** [__sub-make] Error 2
> > -bash-4.4$
> > 
> > Any clue what is the possible issue here?
> 
> Same here. After applying patch 1 and 2 it doesn't build for me 
> with the same error as above.
> 
> But if I do:
> cd tools/bpf/resolve_btfids; make; cd -; make
> it works.

I already got this error from 0-day bot.. I tested on fresh
Fedora installs on x86/ppx/arm/s390 archs and all's good

what distros are you guys on? I'm installing debian and
perhaps I'll try some other

thanks,
jirka

Re: [PATCH v4 bpf-next 10/14] bpf: Add d_path helper

[Jiri Olsa]

On Fri, Jun 26, 2020 at 01:38:27PM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 25, 2020 at 4:49 PM 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/uapi/linux/bpf.h       | 14 +++++++++-
> >  kernel/trace/bpf_trace.c       | 47 ++++++++++++++++++++++++++++++++++
> >  scripts/bpf_helpers_doc.py     |  2 ++
> >  tools/include/uapi/linux/bpf.h | 14 +++++++++-
> >  4 files changed, 75 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 0cb8ec948816..23274c81f244 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3285,6 +3285,17 @@ union bpf_attr {
> >   *             Dynamically cast a *sk* pointer to a *udp6_sock* pointer.
> >   *     Return
> >   *             *sk* if casting is valid, or NULL otherwise.
> > + *
> > + * 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
> 
> It's important to note whether string is always zero-terminated (I'm
> guessing it is, right?).

right, will add

> 
> > + *
> >   */
> >  #define __BPF_FUNC_MAPPER(FN)          \
> >         FN(unspec),                     \
> > @@ -3427,7 +3438,8 @@ union bpf_attr {
> >         FN(skc_to_tcp_sock),            \
> >         FN(skc_to_tcp_timewait_sock),   \
> >         FN(skc_to_tcp_request_sock),    \
> > -       FN(skc_to_udp6_sock),
> > +       FN(skc_to_udp6_sock),           \
> > +       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/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index b124d468688c..6f31e21565b6 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1060,6 +1060,51 @@ 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;
> 
> if len above is zero, you won't zero-terminate it, so probably better
> to move buf[len] = 0 out of if to do unconditionally

good catch, will change

> 
> > +               }
> > +       }
> > +
> > +       return len;
> > +}
> > +
> > +BTF_SET_START(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_SET_END(btf_whitelist_d_path)
> > +
> > +static bool bpf_d_path_allowed(const struct bpf_prog *prog)
> > +{
> > +       return btf_id_set_contains(&btf_whitelist_d_path, prog->aux->attach_btf_id);
> > +}
> > +
> 
> This looks pretty great and clean, considering what's happening under
> the covers. Nice work, thanks a lot!
> 
> > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > +BTF_ID(struct, path)
> 
> this is a bit more confusing to read and error-prone, but I couldn't
> come up with any better way to do this... Still better than
> alternatives.
> 
> > +
> > +static const struct bpf_func_proto bpf_d_path_proto = {
> > +       .func           = bpf_d_path,
> > +       .gpl_only       = true,
> 
> Does it have to be GPL-only? What's the criteria? Sorry if this was
> brought up previously.

I don't think it's needed to be gpl_only, I'll set it to false

thanks,
jirka

Re: [PATCH v4 bpf-next 02/14] bpf: Compile resolve_btfids tool at kernel compilation start

[Jiri Olsa]

On Fri, Jun 26, 2020 at 02:28:30PM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 25, 2020 at 4:47 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The resolve_btfids 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>
> > ---
> 
> Not sure about clean target, but otherwise looks good to me.
> 
> Acked-by: Andrii Nakryiko <andriin@fb.com>
> 
> >  Makefile           | 22 ++++++++++++++++++----
> >  tools/Makefile     |  3 +++
> >  tools/bpf/Makefile |  5 ++++-
> >  3 files changed, 25 insertions(+), 5 deletions(-)
> >
> 
> [...]
> 
> > diff --git a/tools/bpf/Makefile b/tools/bpf/Makefile
> > index 6df1850f8353..89ae235b790e 100644
> > --- a/tools/bpf/Makefile
> > +++ b/tools/bpf/Makefile
> > @@ -123,5 +123,8 @@ runqslower_install:
> >  runqslower_clean:
> >         $(call descend,runqslower,clean)
> >
> > +resolve_btfids:
> > +       $(call descend,resolve_btfids)
> > +
> 
> I think we talked about this. Did we decide that resolve_btfids_clean
> is not necessary?

nope, I said I'd add it and forgot ;-) will add in next version

thanks,
jirka

> 
> >  .PHONY: all install clean bpftool bpftool_install bpftool_clean \
> > -       runqslower runqslower_install runqslower_clean
> > +       runqslower runqslower_install runqslower_clean resolve_btfids
> > --
> > 2.25.4
> >
> 

Re: [PATCH v4 bpf-next 03/14] bpf: Add BTF_ID_LIST/BTF_ID macros

[Jiri Olsa]

On Fri, Jun 26, 2020 at 02:32:48PM -0700, Andrii Nakryiko wrote:

SNIP

> 
> [...]
> 
> > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > new file mode 100644
> > index 000000000000..f7f9dc4d9a9f
> > --- /dev/null
> > +++ b/include/linux/btf_ids.h
> > @@ -0,0 +1,69 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _LINUX_BTF_IDS_H
> > +#define _LINUX_BTF_IDS_H 1
> 
> this "1", is it necessary? I think it's always just `#define HEADER_GUARD`?

I followed btf.h header:

#ifndef _LINUX_BTF_H
#define _LINUX_BTF_H 1

I don't mind changing it

> 
> > +
> > +#include <linux/compiler.h> /* for __PASTE */
> > +
> 
> [...]
> 
> > +#define __BTF_ID_LIST(name)                            \
> > +asm(                                                   \
> > +".pushsection " BTF_IDS_SECTION ",\"a\";       \n"     \
> > +".local " #name ";                             \n"     \
> > +#name ":;                                      \n"     \
> > +".popsection;                                  \n");   \
> > +
> > +#define BTF_ID_LIST(name)                              \
> > +__BTF_ID_LIST(name)                                    \
> > +extern int name[];
> 
> nit: extern u32 (or __u32) perhaps?

right, should be u32 

thanks,
jirka

Re: [PATCH v4 bpf-next 06/14] bpf: Use BTF_ID to resolve bpf_ctx_convert struct

[Jiri Olsa]

On Fri, Jun 26, 2020 at 02:44:39PM -0700, Yonghong Song wrote:
> 
> 
> On 6/25/20 3:12 PM, Jiri Olsa wrote:
> > This way the ID is resolved during compile time,
> > and we can remove the runtime name search.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   kernel/bpf/btf.c | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 4da6b0770ff9..701a2cb5dfb2 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -18,6 +18,7 @@
> >   #include <linux/sort.h>
> >   #include <linux/bpf_verifier.h>
> >   #include <linux/btf.h>
> > +#include <linux/btf_ids.h>
> >   #include <linux/skmsg.h>
> >   #include <linux/perf_event.h>
> >   #include <net/sock.h>
> > @@ -3621,6 +3622,9 @@ static int btf_translate_to_vmlinux(struct bpf_verifier_log *log,
> >   	return kern_ctx_type->type;
> >   }
> > +BTF_ID_LIST(bpf_ctx_convert_btf_id)
> > +BTF_ID(struct, bpf_ctx_convert)
> > +
> >   struct btf *btf_parse_vmlinux(void)
> >   {
> >   	struct btf_verifier_env *env = NULL;
> > @@ -3659,10 +3663,10 @@ struct btf *btf_parse_vmlinux(void)
> >   	if (err)
> >   		goto errout;
> > -	/* find struct bpf_ctx_convert for type checking later */
> > -	btf_id = btf_find_by_name_kind(btf, "bpf_ctx_convert", BTF_KIND_STRUCT);
> > -	if (btf_id < 0) {
> > -		err = btf_id;
> > +	/* struct bpf_ctx_convert for type checking later */
> > +	btf_id = bpf_ctx_convert_btf_id[0];
> > +	if (btf_id <= 0) {
> 
> Just want to double check. Is it possible btf_id < 0 since previous patch
> did not check < 0?

right.. resolve_btfids would fail during linking,
so this is not necessary

thanks,
jirka

Re: [PATCH v4 bpf-next 13/14] selftests/bpf: Add test for d_path helper

[Jiri Olsa]

On Fri, Jun 26, 2020 at 02:55:34PM -0700, Andrii Nakryiko wrote:

SNIP

> > +       if (cnt_stat >= MAX_EVENT_NUM)
> > +               return 0;
> > +
> > +       bpf_d_path(path, paths_stat[cnt_stat], MAX_PATH_LEN);
> > +       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 != my_pid)
> > +               return 0;
> > +
> > +       if (cnt_close >= MAX_EVENT_NUM)
> > +               return 0;
> > +
> > +       bpf_d_path((struct path *) &file->f_path,
> > +                  paths_close[cnt_close], MAX_PATH_LEN);
> 
> Can you please capture the return result of bpf_d_path() (here and
> above) and validate that it's correct? That will help avoid future
> breakages if anyone changes this.

right, good idea, will add

thanks,
jirka

Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

[Andrii Nakryiko]

On Sun, Jun 28, 2020 at 11:50 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 04:29:23PM -0700, Yonghong Song wrote:
> > > > >
> > > > > -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)
> > > > > +     if (!id || id > btf_vmlinux->nr_types)
> > > > >                return -EINVAL;
> > > >
> > > > id == 0 if btf_id cannot be resolved by resolve_btfids, right?
> > > > when id may be greater than btf_vmlinux->nr_types? If resolve_btfids
> > > > application did incorrect transformation?
> > > >
> > > > Anyway, this is to resolve helper meta btf_id. Even if you
> > > > return a btf_id > btf_vmlinux->nr_types, verifier will reject
> > > > since it will never be the same as the real parameter btf_id.
> > > > I would drop id > btf_vmlinux->nr_types here. This should never
> > > > happen for a correct tool. Even if it does, verifier will take
> > > > care of it.
> > > >
> > >
> > > I'd love to hear Alexei's thoughts about this change as well. Jiri
> > > removed not just BTF ID resolution, but also all the sanity checks.
> > > This now means more trust in helper definitions to not screw up
> > > anything. It's probably OK, but still something to consciously think
> > > about.
> >
> > The kernel will have to trust the result.
>
> +1
> I think 'if (!id || id > btf_vmlinux->nr_types)' at run-time and
> other sanity checks I saw in other patches are unnecessary.
> resolve_btfids should do all checks and fail vmlinux linking.
> We trust gcc to generate correct assembly code in the first place
> and correct dwarf. We trust pahole do correct BTF conversion from
> dwarf and dedup. We should trust resolve_btfids.
> It's imo the simplest tool comparing to gcc.
> btf_parse_vmlinux() is doing basic sanity check of BTF mainly
> to populate 'struct btf *btf_vmlinux;' for further use.
> I think we can add a scan over resolved btfids to btf_parse_vmlinux()
> to make sure that all ids are within the range, but I don't think
> it's mandatory for this patch set. Would be a reasonable sanity
> check for the future. Of course, checking for sorted set_start/end
> is overkill. Basic simplest sanity is ok.

My point was *not* about trusting resolve_btfids tool, it was about
trusting BPF helper definitions. But it's more of help for developer
while developing new helpers, so it's not a major thing.

Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

[Jiri Olsa]

On Fri, Jun 26, 2020 at 02:36:37PM -0700, Yonghong Song wrote:

SNIP

> > -	}
> > -
> > -	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);
> > +	if (WARN_ON_ONCE(!fn->btf_id))
> 
> The original code does not have this warning. It directly did
> "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID.

not sure why I put it in there, it's probably enough guarded
by arg_type having ARG_PTR_TO_BTF_ID, will remove

> 
> >   		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;
> > -}
> > +	id = fn->btf_id[arg];
> 
> The corresponding BTF_ID definition here is:
>   BTF_ID_LIST(bpf_skb_output_btf_ids)
>   BTF_ID(struct, sk_buff)
> 
> The bpf helper writer needs to ensure proper declarations
> of BTF_IDs like the above matching helpers definition.
> Support we have arg1 and arg3 as BTF_ID. then the list
> definition may be
> 
>   BTF_ID_LIST(bpf_skb_output_btf_ids)
>   BTF_ID(struct, sk_buff)
>   BTF_ID(struct, __unused)
>   BTF_ID(struct, task_struct)
> 
> This probably okay, I guess.

right, AFAIK we don't have such case yet, but would be good
to be ready and have something like

  BTF_ID(struct, __unused)

maybe adding new type for that will be better:

  BTF_ID(none, unused)

jirka

Re: [PATCH v4 bpf-next 01/14] bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object

[Yonghong Song]

On 6/28/20 12:35 PM, Jiri Olsa wrote:
> On Sun, Jun 28, 2020 at 12:09:27PM -0700, Alexei Starovoitov wrote:
>> On Fri, Jun 26, 2020 at 02:09:53PM -0700, Yonghong Song wrote:
>>>
>>> After applying the whole patch set to my bpf-next tree locally, I cannot
>>> build:
>>>
>>> -bash-4.4$ make -j100 && make vmlinux
>>>    GEN     Makefile
>>>    DESCEND  objtool
>>>    DESCEND  bpf/resolve_btfids
>>> make[4]: *** No rule to make target
>>> `/data/users/yhs/work/net-next/tools/bpf/resolve_btfids/fixdep'.  Stop.
>>> make[3]: *** [fixdep] Error 2
>>> make[2]: *** [bpf/resolve_btfids] Error 2
>>> make[1]: *** [tools/bpf/resolve_btfids] Error 2
>>> make[1]: *** Waiting for unfinished jobs....
>>> make[1]: *** wait: No child processes.  Stop.
>>> make: *** [__sub-make] Error 2
>>> -bash-4.4$
>>>
>>> Any clue what is the possible issue here?
>>
>> Same here. After applying patch 1 and 2 it doesn't build for me
>> with the same error as above.
>>
>> But if I do:
>> cd tools/bpf/resolve_btfids; make; cd -; make
>> it works.
> 
> I already got this error from 0-day bot.. I tested on fresh
> Fedora installs on x86/ppx/arm/s390 archs and all's good
> 
> what distros are you guys on? I'm installing debian and
> perhaps I'll try some other

My system is:
  CentOS Linux release 7.8.2003 (Core)

> 
> thanks,
> jirka
> 

Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

[Yonghong Song]

On 6/28/20 1:16 PM, Jiri Olsa wrote:
> On Fri, Jun 26, 2020 at 02:36:37PM -0700, Yonghong Song wrote:
> 
> SNIP
> 
>>> -	}
>>> -
>>> -	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);
>>> +	if (WARN_ON_ONCE(!fn->btf_id))
>>
>> The original code does not have this warning. It directly did
>> "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID.
> 
> not sure why I put it in there, it's probably enough guarded
> by arg_type having ARG_PTR_TO_BTF_ID, will remove
> 
>>
>>>    		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;
>>> -}
>>> +	id = fn->btf_id[arg];
>>
>> The corresponding BTF_ID definition here is:
>>    BTF_ID_LIST(bpf_skb_output_btf_ids)
>>    BTF_ID(struct, sk_buff)
>>
>> The bpf helper writer needs to ensure proper declarations
>> of BTF_IDs like the above matching helpers definition.
>> Support we have arg1 and arg3 as BTF_ID. then the list
>> definition may be
>>
>>    BTF_ID_LIST(bpf_skb_output_btf_ids)
>>    BTF_ID(struct, sk_buff)
>>    BTF_ID(struct, __unused)
>>    BTF_ID(struct, task_struct)
>>
>> This probably okay, I guess.
> 
> right, AFAIK we don't have such case yet, but would be good
> to be ready and have something like
> 
>    BTF_ID(struct, __unused)
> 
> maybe adding new type for that will be better:
> 
>    BTF_ID(none, unused)

Maybe we can have a separate macro BTF_ID_UNUSED macro
which simply adds 4 bytes hole in the .btf_ids* section.

> 
> jirka
> 

Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

[Jiri Olsa]

On Sun, Jun 28, 2020 at 01:59:54PM -0700, Yonghong Song wrote:

SNIP

> > > 
> > > The corresponding BTF_ID definition here is:
> > >    BTF_ID_LIST(bpf_skb_output_btf_ids)
> > >    BTF_ID(struct, sk_buff)
> > > 
> > > The bpf helper writer needs to ensure proper declarations
> > > of BTF_IDs like the above matching helpers definition.
> > > Support we have arg1 and arg3 as BTF_ID. then the list
> > > definition may be
> > > 
> > >    BTF_ID_LIST(bpf_skb_output_btf_ids)
> > >    BTF_ID(struct, sk_buff)
> > >    BTF_ID(struct, __unused)
> > >    BTF_ID(struct, task_struct)
> > > 
> > > This probably okay, I guess.
> > 
> > right, AFAIK we don't have such case yet, but would be good
> > to be ready and have something like
> > 
> >    BTF_ID(struct, __unused)
> > 
> > maybe adding new type for that will be better:
> > 
> >    BTF_ID(none, unused)
> 
> Maybe we can have a separate macro BTF_ID_UNUSED macro
> which simply adds 4 bytes hole in the .btf_ids* section.

right, we don't need symbols for that

jirka

Re: [PATCH v4 bpf-next 12/14] selftests/bpf: Add verifier test for d_path helper

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:48 PM 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>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/testing/selftests/bpf/test_verifier.c   | 19 +++++++++-
>  tools/testing/selftests/bpf/verifier/d_path.c | 37 +++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/verifier/d_path.c
>

[...]

Re: [PATCH v4 bpf-next 14/14] selftests/bpf: Add test for resolve_btfids

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:48 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test to resolve_btfids tool, that:
>   - creates binary with BTF IDs list and set
>   - process the binary with resolve_btfids tool
>   - verifies that correct BTF ID values are in place
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/testing/selftests/bpf/Makefile          |  20 +-
>  .../selftests/bpf/test_resolve_btfids.c       | 201 ++++++++++++++++++
>  2 files changed, 220 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/test_resolve_btfids.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 22aaec74ea0a..547322a5feff 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -37,7 +37,8 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
>         test_cgroup_storage \
>         test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
>         test_progs-no_alu32 \
> -       test_current_pid_tgid_new_ns
> +       test_current_pid_tgid_new_ns \
> +       test_resolve_btfids
>
>  # Also test bpf-gcc, if present
>  ifneq ($(BPF_GCC),)
> @@ -427,6 +428,23 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
>         $(call msg,BINARY,,$@)
>         $(CC) $(LDFLAGS) -o $@ $(filter %.a %.o,$^) $(LDLIBS)
>
> +# test_resolve_btfids
> +#

useless comment, please drop

> +$(SCRATCH_DIR)/resolve_btfids: $(BPFOBJ) FORCE
> +       $(Q)$(MAKE) $(submake_extras) -C $(TOOLSDIR)/bpf/resolve_btfids \
> +                   OUTPUT=$(SCRATCH_DIR)/ BPFOBJ=$(BPFOBJ)

Why do you need FORCE here? To force building this tool every single
time, even if nothing changed? See what we did for bpftool rebuilds.
It's not perfect, but works fine in practice.

> +
> +$(OUTPUT)/test_resolve_btfids.o: test_resolve_btfids.c
> +       $(call msg,CC,,$@)
> +       $(CC) $(CFLAGS) -I$(TOOLSINCDIR) -D"BUILD_STR(s)=#s" -DVMLINUX_BTF="BUILD_STR($(VMLINUX_BTF))" -c -o $@ $<
> +
> +.PHONY: FORCE
> +
> +$(OUTPUT)/test_resolve_btfids: $(OUTPUT)/test_resolve_btfids.o $(SCRATCH_DIR)/resolve_btfids
> +       $(call msg,BINARY,,$@)
> +       $(CC) -o $@ $< $(BPFOBJ) -lelf -lz && \
> +       $(SCRATCH_DIR)/resolve_btfids --btf $(VMLINUX_BTF) $@
> +

Wouldn't it be better to make this just one of the tests of test_progs
and let resolve_btfids process test_progs completely? That should
still work, plus statically resolved BTF IDs against kernel would be
available for other tests immediately. And you will have all the
infrastructure of test_progs available. And this will be tested very
regularly. Win-win-win-win?

>  EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR)                     \
>         prog_tests/tests.h map_tests/tests.h verifier/tests.h           \
>         feature                                                         \
> diff --git a/tools/testing/selftests/bpf/test_resolve_btfids.c b/tools/testing/selftests/bpf/test_resolve_btfids.c
> new file mode 100644
> index 000000000000..48aeda2ed881
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_resolve_btfids.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <string.h>
> +#include <stdio.h>
> +#include <sys/stat.h>
> +#include <stdio.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <linux/err.h>
> +#include <stdlib.h>
> +#include <bpf/btf.h>
> +#include <bpf/libbpf.h>
> +#include <linux/btf.h>
> +#include <linux/kernel.h>
> +#include <linux/btf_ids.h>
> +
> +#define __CHECK(condition, format...) ({                               \
> +       int __ret = !!(condition);                                      \
> +       if (__ret) {                                                    \
> +               fprintf(stderr, "%s:%d:FAIL ", __func__, __LINE__);     \
> +               fprintf(stderr, format);                                \
> +       }                                                               \
> +       __ret;                                                          \
> +})
> +
> +#define CHECK(condition, format...)                                    \
> +       do {                                                            \
> +               if (__CHECK(condition, format))                         \
> +                       return -1;                                      \
> +       } while (0)

it's better to make CHECK return value, makes its use more flexible

> +
> +static struct btf *btf__parse_raw(const char *file)

How about adding this as a libbpf API? It's not the first time I see
this being re-implemented. While simple, libbpf already implements
this internally, so there should be no need to require users do this
all the time. Follow up patch is ok, no need to block on this.

> +{
> +       struct btf *btf;
> +       struct stat st;
> +       __u8 *buf;
> +       FILE *f;
> +
> +       if (stat(file, &st))
> +               return NULL;
> +
> +       f = fopen(file, "rb");
> +       if (!f)
> +               return NULL;
> +
> +       buf = malloc(st.st_size);
> +       if (!buf) {
> +               btf = ERR_PTR(-ENOMEM);
> +               goto exit_close;
> +       }
> +
> +       if ((size_t) st.st_size != fread(buf, 1, st.st_size, f)) {
> +               btf = ERR_PTR(-EINVAL);
> +               goto exit_free;
> +       }
> +
> +       btf = btf__new(buf, st.st_size);
> +
> +exit_free:
> +       free(buf);
> +exit_close:
> +       fclose(f);
> +       return btf;
> +}
> +

[...]

> +
> +static int
> +__resolve_symbol(struct btf *btf, int type_id)
> +{
> +       const struct btf_type *type;
> +       const char *str;
> +       unsigned int i;
> +
> +       type = btf__type_by_id(btf, type_id);
> +       CHECK(!type, "Failed to get type for ID %d\n", type_id);

return otherwise you'll get crash on few lines below; it's unpleasant
to debug crashes in VM in Travis CI

> +
> +       for (i = 0; i < ARRAY_SIZE(test_symbols); i++) {
> +               if (test_symbols[i].id != -1)
> +                       continue;
> +
> +               if (BTF_INFO_KIND(type->info) != test_symbols[i].type)
> +                       continue;
> +
> +               str = btf__name_by_offset(btf, type->name_off);
> +               if (!str) {

CHECK?

> +                       fprintf(stderr, "failed to get name for BTF ID %d\n",
> +                               type_id);
> +                       continue;
> +               }
> +
> +               if (!strcmp(str, test_symbols[i].name))
> +                       test_symbols[i].id = type_id;
> +       }
> +
> +       return 0;
> +}
> +
> +static int resolve_symbols(void)
> +{
> +       const char *path = VMLINUX_BTF;
> +       struct btf *btf;
> +       int type_id;
> +       __u32 nr;
> +       int err;
> +
> +       btf = btf_open(path);
> +       CHECK(libbpf_get_error(btf), "Failed to load BTF from %s\n", path);
> +

exit, crash othewise

> +       nr = btf__get_nr_types(btf);
> +
> +       for (type_id = 0; type_id < nr; type_id++) {

type_id = 1; type_id <= nr

> +               err = __resolve_symbol(btf, type_id);
> +               if (__CHECK(err, "Failed to resolve symbols\n"))
> +                       break;
> +       }
> +
> +       btf__free(btf);
> +       return 0;
> +}
> +

[...]

Re: [PATCH v4 bpf-next 07/14] bpf: Allow nested BTF object to be refferenced by BTF object + offset

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:49 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> 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 will 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.
>
> Using btf_struct_access to implement new btf_struct_address
> function, because it already walks down the given BTF object.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

This logic is very hard to follow. Each type I try to review it, I get
lost very fast. TBH, this access_data struct is not just not helpful,
but actually just complicates everything.

I'll get to this tomorrow morning with fresh brains and will try to do
another pass.

[...]

>  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 7de98906ddf4..da7351184295 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3808,6 +3808,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>         struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
>         enum bpf_reg_type expected_type, type = reg->type;
>         enum bpf_arg_type arg_type = fn->arg_type[arg];
> +       const struct btf_type *btf_type;
>         int err = 0;
>
>         if (arg_type == ARG_DONTCARE)
> @@ -3887,24 +3888,34 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                 expected_type = PTR_TO_BTF_ID;
>                 if (type != expected_type)
>                         goto err_type;
> -               if (!fn->check_btf_id) {
> -                       if (reg->btf_id != meta->btf_id) {
> -                               verbose(env, "Helper has type %s got %s in R%d\n",
> +               if (reg->off) {


This non-zero offset only logic looks fishy, tbh. What if the struct
you are trying to access is at offset zero? E.g., bpf_link is pretty
much always a first field of whatever specific link struct it is
contained within. The fact that we allow only non-zero offsets for
such use case looks like an arbitrary limitation.

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

[...]

Re: [PATCH v4 bpf-next 00/14] bpf: Add d_path helper

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:47 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> hi,
> adding d_path helper to return full path for 'path' object.
>
> In a preparation for that, this patchset also adds support for BTF ID
> whitelists, because d_path can't be called from any probe due to its
> locks usage. The whitelists allow verifier to check if the caller is
> one of the functions from the whitelist.
>
> The whitelist is implemented in a generic way. This patchset introduces
> macros that allow to define lists of BTF IDs, which are compiled in
> the kernel image in a new .BTF.ids ELF section.
>
> The generic way of BTF ID lists allows us to use them in other places
> in kernel (than just for whitelists), that could use static BTF ID
> values compiled in and it's also implemented in this patchset.
>
> 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)); }'
>
> v4 changes:
>   - added ID sanity checks in btf_resolve_helper_id [Andrii]
>   - resolve bpf_ctx_convert via BTF_ID [Andrii]
>   - keep bpf_access_type in btf_struct_access [Andrii]
>   - rename whitelist to se and use struct btf_id_set [Andrii]
>   - several fixes for d_path prog/verifier tests [Andrii]
>   - added union and typedefs types support [Andrii]
>   - rename btfid to resolve_btfids [Andrii]
>   - fix segfault in resolve_btfids [John]
>   - rename section from .BTF_ids .BTF.ids (following .BTF.ext example)
>   - add .BTF.ids section info into btf.rst [John]
>   - updated over letter with more details [John]
>
> Also available at:
>   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   bpf/d_path
>
> thanks,
> jirka
>
>
> ---

Have you considered splitting this series into two? One with BTF ID
resolution and corresponding patches. I'm pretty confident in that one
and it seems ready (with some minor selftest changes). Then,
separately, d_path and that sub-struct address logic. That one depends
on the first one, but shouldn't really block BTF ID resolution from
going in sooner.

> Jiri Olsa (14):
>       bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object
>       bpf: Compile resolve_btfids tool at kernel compilation start
>       bpf: Add BTF_ID_LIST/BTF_ID macros
>       bpf: Resolve BTF IDs in vmlinux image
>       bpf: Remove btf_id helpers resolving
>       bpf: Use BTF_ID to resolve bpf_ctx_convert struct
>       bpf: Allow nested BTF object to be refferenced by BTF object + offset
>       bpf: Add BTF_SET_START/END macros
>       bpf: Add info about .BTF.ids section to btf.rst
>       bpf: Add d_path helper
>       tools headers: Adopt verbatim copy of btf_ids.h from kernel sources
>       selftests/bpf: Add verifier test for d_path helper
>       selftests/bpf: Add test for d_path helper
>       selftests/bpf: Add test for resolve_btfids
>

[...]

Re: [PATCH v4 bpf-next 07/14] bpf: Allow nested BTF object to be refferenced by BTF object + offset

[Jiri Olsa]

On Mon, Jun 29, 2020 at 06:52:21PM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 25, 2020 at 4:49 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > 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 will 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.
> >
> > Using btf_struct_access to implement new btf_struct_address
> > function, because it already walks down the given BTF object.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> This logic is very hard to follow. Each type I try to review it, I get
> lost very fast. TBH, this access_data struct is not just not helpful,
> but actually just complicates everything.

yea, it's one of the reasons I added extra function for that in first version

> 
> I'll get to this tomorrow morning with fresh brains and will try to do
> another pass.
> 
> [...]
> 
> >  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 7de98906ddf4..da7351184295 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3808,6 +3808,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >         struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> >         enum bpf_reg_type expected_type, type = reg->type;
> >         enum bpf_arg_type arg_type = fn->arg_type[arg];
> > +       const struct btf_type *btf_type;
> >         int err = 0;
> >
> >         if (arg_type == ARG_DONTCARE)
> > @@ -3887,24 +3888,34 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >                 expected_type = PTR_TO_BTF_ID;
> >                 if (type != expected_type)
> >                         goto err_type;
> > -               if (!fn->check_btf_id) {
> > -                       if (reg->btf_id != meta->btf_id) {
> > -                               verbose(env, "Helper has type %s got %s in R%d\n",
> > +               if (reg->off) {
> 
> 
> This non-zero offset only logic looks fishy, tbh. What if the struct
> you are trying to access is at offset zero? E.g., bpf_link is pretty
> much always a first field of whatever specific link struct it is
> contained within. The fact that we allow only non-zero offsets for
> such use case looks like an arbitrary limitation.

right, that's mistake, I was after path under struct file,
and did not realize this needs to be generic

thanks,
jirka

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

Re: [PATCH v4 bpf-next 00/14] bpf: Add d_path helper

[Jiri Olsa]

On Mon, Jun 29, 2020 at 06:54:30PM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 25, 2020 at 4:47 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > hi,
> > adding d_path helper to return full path for 'path' object.
> >
> > In a preparation for that, this patchset also adds support for BTF ID
> > whitelists, because d_path can't be called from any probe due to its
> > locks usage. The whitelists allow verifier to check if the caller is
> > one of the functions from the whitelist.
> >
> > The whitelist is implemented in a generic way. This patchset introduces
> > macros that allow to define lists of BTF IDs, which are compiled in
> > the kernel image in a new .BTF.ids ELF section.
> >
> > The generic way of BTF ID lists allows us to use them in other places
> > in kernel (than just for whitelists), that could use static BTF ID
> > values compiled in and it's also implemented in this patchset.
> >
> > 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)); }'
> >
> > v4 changes:
> >   - added ID sanity checks in btf_resolve_helper_id [Andrii]
> >   - resolve bpf_ctx_convert via BTF_ID [Andrii]
> >   - keep bpf_access_type in btf_struct_access [Andrii]
> >   - rename whitelist to se and use struct btf_id_set [Andrii]
> >   - several fixes for d_path prog/verifier tests [Andrii]
> >   - added union and typedefs types support [Andrii]
> >   - rename btfid to resolve_btfids [Andrii]
> >   - fix segfault in resolve_btfids [John]
> >   - rename section from .BTF_ids .BTF.ids (following .BTF.ext example)
> >   - add .BTF.ids section info into btf.rst [John]
> >   - updated over letter with more details [John]
> >
> > Also available at:
> >   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> >   bpf/d_path
> >
> > thanks,
> > jirka
> >
> >
> > ---
> 
> Have you considered splitting this series into two? One with BTF ID
> resolution and corresponding patches. I'm pretty confident in that one
> and it seems ready (with some minor selftest changes). Then,
> separately, d_path and that sub-struct address logic. That one depends
> on the first one, but shouldn't really block BTF ID resolution from
> going in sooner.

ok, will split it and send the first part for now

jirka

Re: [PATCH v4 bpf-next 14/14] selftests/bpf: Add test for resolve_btfids

[Jiri Olsa]

On Mon, Jun 29, 2020 at 06:43:51PM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 25, 2020 at 4:48 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test to resolve_btfids tool, that:
> >   - creates binary with BTF IDs list and set
> >   - process the binary with resolve_btfids tool
> >   - verifies that correct BTF ID values are in place
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/Makefile          |  20 +-
> >  .../selftests/bpf/test_resolve_btfids.c       | 201 ++++++++++++++++++
> >  2 files changed, 220 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/bpf/test_resolve_btfids.c
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 22aaec74ea0a..547322a5feff 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -37,7 +37,8 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
> >         test_cgroup_storage \
> >         test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
> >         test_progs-no_alu32 \
> > -       test_current_pid_tgid_new_ns
> > +       test_current_pid_tgid_new_ns \
> > +       test_resolve_btfids
> >
> >  # Also test bpf-gcc, if present
> >  ifneq ($(BPF_GCC),)
> > @@ -427,6 +428,23 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
> >         $(call msg,BINARY,,$@)
> >         $(CC) $(LDFLAGS) -o $@ $(filter %.a %.o,$^) $(LDLIBS)
> >
> > +# test_resolve_btfids
> > +#
> 
> useless comment, please drop

ok

> 
> > +$(SCRATCH_DIR)/resolve_btfids: $(BPFOBJ) FORCE
> > +       $(Q)$(MAKE) $(submake_extras) -C $(TOOLSDIR)/bpf/resolve_btfids \
> > +                   OUTPUT=$(SCRATCH_DIR)/ BPFOBJ=$(BPFOBJ)
> 
> Why do you need FORCE here? To force building this tool every single
> time, even if nothing changed? See what we did for bpftool rebuilds.

no, the build framework will recognize if the rebuild is needed,
and trigger it..  but it needs to be invoked, hence the FORCE

> It's not perfect, but works fine in practice.

we don't need to put the sources as dependency in here,
as you do for bpftool, the build system will take care
of that

> 
> > +
> > +$(OUTPUT)/test_resolve_btfids.o: test_resolve_btfids.c
> > +       $(call msg,CC,,$@)
> > +       $(CC) $(CFLAGS) -I$(TOOLSINCDIR) -D"BUILD_STR(s)=#s" -DVMLINUX_BTF="BUILD_STR($(VMLINUX_BTF))" -c -o $@ $<
> > +
> > +.PHONY: FORCE
> > +
> > +$(OUTPUT)/test_resolve_btfids: $(OUTPUT)/test_resolve_btfids.o $(SCRATCH_DIR)/resolve_btfids
> > +       $(call msg,BINARY,,$@)
> > +       $(CC) -o $@ $< $(BPFOBJ) -lelf -lz && \
> > +       $(SCRATCH_DIR)/resolve_btfids --btf $(VMLINUX_BTF) $@
> > +
> 
> Wouldn't it be better to make this just one of the tests of test_progs
> and let resolve_btfids process test_progs completely? That should
> still work, plus statically resolved BTF IDs against kernel would be
> available for other tests immediately. And you will have all the
> infrastructure of test_progs available. And this will be tested very
> regularly. Win-win-win-win?

ok, sounds good ;-)

> 
> >  EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR)                     \
> >         prog_tests/tests.h map_tests/tests.h verifier/tests.h           \
> >         feature                                                         \
> > diff --git a/tools/testing/selftests/bpf/test_resolve_btfids.c b/tools/testing/selftests/bpf/test_resolve_btfids.c
> > new file mode 100644
> > index 000000000000..48aeda2ed881
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_resolve_btfids.c
> > @@ -0,0 +1,201 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <string.h>
> > +#include <stdio.h>
> > +#include <sys/stat.h>
> > +#include <stdio.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <linux/err.h>
> > +#include <stdlib.h>
> > +#include <bpf/btf.h>
> > +#include <bpf/libbpf.h>
> > +#include <linux/btf.h>
> > +#include <linux/kernel.h>
> > +#include <linux/btf_ids.h>
> > +
> > +#define __CHECK(condition, format...) ({                               \
> > +       int __ret = !!(condition);                                      \
> > +       if (__ret) {                                                    \
> > +               fprintf(stderr, "%s:%d:FAIL ", __func__, __LINE__);     \
> > +               fprintf(stderr, format);                                \
> > +       }                                                               \
> > +       __ret;                                                          \
> > +})
> > +
> > +#define CHECK(condition, format...)                                    \
> > +       do {                                                            \
> > +               if (__CHECK(condition, format))                         \
> > +                       return -1;                                      \
> > +       } while (0)
> 
> it's better to make CHECK return value, makes its use more flexible
> 
> > +
> > +static struct btf *btf__parse_raw(const char *file)
> 
> How about adding this as a libbpf API? It's not the first time I see
> this being re-implemented. While simple, libbpf already implements
> this internally, so there should be no need to require users do this
> all the time. Follow up patch is ok, no need to block on this.

yea, I copied that code around few times already,
I'll add it to libbpf

> 
> > +{
> > +       struct btf *btf;
> > +       struct stat st;
> > +       __u8 *buf;
> > +       FILE *f;
> > +
> > +       if (stat(file, &st))
> > +               return NULL;
> > +
> > +       f = fopen(file, "rb");
> > +       if (!f)
> > +               return NULL;
> > +
> > +       buf = malloc(st.st_size);
> > +       if (!buf) {
> > +               btf = ERR_PTR(-ENOMEM);
> > +               goto exit_close;
> > +       }
> > +
> > +       if ((size_t) st.st_size != fread(buf, 1, st.st_size, f)) {
> > +               btf = ERR_PTR(-EINVAL);
> > +               goto exit_free;
> > +       }
> > +
> > +       btf = btf__new(buf, st.st_size);
> > +
> > +exit_free:
> > +       free(buf);
> > +exit_close:
> > +       fclose(f);
> > +       return btf;
> > +}
> > +
> 
> [...]
> 
> > +
> > +static int
> > +__resolve_symbol(struct btf *btf, int type_id)
> > +{
> > +       const struct btf_type *type;
> > +       const char *str;
> > +       unsigned int i;
> > +
> > +       type = btf__type_by_id(btf, type_id);
> > +       CHECK(!type, "Failed to get type for ID %d\n", type_id);
> 
> return otherwise you'll get crash on few lines below; it's unpleasant
> to debug crashes in VM in Travis CI

the CHECK macro does 'return' on error

> 
> > +
> > +       for (i = 0; i < ARRAY_SIZE(test_symbols); i++) {
> > +               if (test_symbols[i].id != -1)
> > +                       continue;
> > +
> > +               if (BTF_INFO_KIND(type->info) != test_symbols[i].type)
> > +                       continue;
> > +
> > +               str = btf__name_by_offset(btf, type->name_off);
> > +               if (!str) {
> 
> CHECK?

ok

> 
> > +                       fprintf(stderr, "failed to get name for BTF ID %d\n",
> > +                               type_id);
> > +                       continue;
> > +               }
> > +
> > +               if (!strcmp(str, test_symbols[i].name))
> > +                       test_symbols[i].id = type_id;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int resolve_symbols(void)
> > +{
> > +       const char *path = VMLINUX_BTF;
> > +       struct btf *btf;
> > +       int type_id;
> > +       __u32 nr;
> > +       int err;
> > +
> > +       btf = btf_open(path);
> > +       CHECK(libbpf_get_error(btf), "Failed to load BTF from %s\n", path);
> > +
> 
> exit, crash othewise

the CHECK macro does 'return' on error

> 
> > +       nr = btf__get_nr_types(btf);
> > +
> > +       for (type_id = 0; type_id < nr; type_id++) {
> 
> type_id = 1; type_id <= nr

damn.. not again ;-) sry

thanks,
jirka

Re: [PATCH v4 bpf-next 14/14] selftests/bpf: Add test for resolve_btfids

[Andrii Nakryiko]

On Tue, Jun 30, 2020 at 7:27 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Jun 29, 2020 at 06:43:51PM -0700, Andrii Nakryiko wrote:
> > On Thu, Jun 25, 2020 at 4:48 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding test to resolve_btfids tool, that:
> > >   - creates binary with BTF IDs list and set
> > >   - process the binary with resolve_btfids tool
> > >   - verifies that correct BTF ID values are in place
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/testing/selftests/bpf/Makefile          |  20 +-
> > >  .../selftests/bpf/test_resolve_btfids.c       | 201 ++++++++++++++++++
> > >  2 files changed, 220 insertions(+), 1 deletion(-)
> > >  create mode 100644 tools/testing/selftests/bpf/test_resolve_btfids.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index 22aaec74ea0a..547322a5feff 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -37,7 +37,8 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
> > >         test_cgroup_storage \
> > >         test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
> > >         test_progs-no_alu32 \
> > > -       test_current_pid_tgid_new_ns
> > > +       test_current_pid_tgid_new_ns \
> > > +       test_resolve_btfids
> > >
> > >  # Also test bpf-gcc, if present
> > >  ifneq ($(BPF_GCC),)
> > > @@ -427,6 +428,23 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
> > >         $(call msg,BINARY,,$@)
> > >         $(CC) $(LDFLAGS) -o $@ $(filter %.a %.o,$^) $(LDLIBS)
> > >
> > > +# test_resolve_btfids
> > > +#
> >
> > useless comment, please drop
>
> ok
>
> >
> > > +$(SCRATCH_DIR)/resolve_btfids: $(BPFOBJ) FORCE
> > > +       $(Q)$(MAKE) $(submake_extras) -C $(TOOLSDIR)/bpf/resolve_btfids \
> > > +                   OUTPUT=$(SCRATCH_DIR)/ BPFOBJ=$(BPFOBJ)
> >
> > Why do you need FORCE here? To force building this tool every single
> > time, even if nothing changed? See what we did for bpftool rebuilds.
>
> no, the build framework will recognize if the rebuild is needed,
> and trigger it..  but it needs to be invoked, hence the FORCE

And that's exactly what we tried to avoid with bpftool, to not invoke
sub-make (for good or bad reasons, not sure, and don't remember
anymore). In this case, it seems especially easy to ensure that
relevant resolve_btfids source code changes are captured properly, so
I'd probably just keep everything consistent and not use FORCE.


>
> > It's not perfect, but works fine in practice.
>
> we don't need to put the sources as dependency in here,
> as you do for bpftool, the build system will take care
> of that
>
> >
> > > +
> > > +$(OUTPUT)/test_resolve_btfids.o: test_resolve_btfids.c
> > > +       $(call msg,CC,,$@)
> > > +       $(CC) $(CFLAGS) -I$(TOOLSINCDIR) -D"BUILD_STR(s)=#s" -DVMLINUX_BTF="BUILD_STR($(VMLINUX_BTF))" -c -o $@ $<
> > > +
> > > +.PHONY: FORCE
> > > +
> > > +$(OUTPUT)/test_resolve_btfids: $(OUTPUT)/test_resolve_btfids.o $(SCRATCH_DIR)/resolve_btfids
> > > +       $(call msg,BINARY,,$@)
> > > +       $(CC) -o $@ $< $(BPFOBJ) -lelf -lz && \
> > > +       $(SCRATCH_DIR)/resolve_btfids --btf $(VMLINUX_BTF) $@
> > > +
> >
> > Wouldn't it be better to make this just one of the tests of test_progs
> > and let resolve_btfids process test_progs completely? That should
> > still work, plus statically resolved BTF IDs against kernel would be
> > available for other tests immediately. And you will have all the
> > infrastructure of test_progs available. And this will be tested very
> > regularly. Win-win-win-win?
>
> ok, sounds good ;-)
>
> >
> > >  EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR)                     \
> > >         prog_tests/tests.h map_tests/tests.h verifier/tests.h           \
> > >         feature                                                         \
> > > diff --git a/tools/testing/selftests/bpf/test_resolve_btfids.c b/tools/testing/selftests/bpf/test_resolve_btfids.c
> > > new file mode 100644
> > > index 000000000000..48aeda2ed881
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/test_resolve_btfids.c
> > > @@ -0,0 +1,201 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include <string.h>
> > > +#include <stdio.h>
> > > +#include <sys/stat.h>
> > > +#include <stdio.h>
> > > +#include <sys/stat.h>
> > > +#include <fcntl.h>
> > > +#include <unistd.h>
> > > +#include <linux/err.h>
> > > +#include <stdlib.h>
> > > +#include <bpf/btf.h>
> > > +#include <bpf/libbpf.h>
> > > +#include <linux/btf.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/btf_ids.h>
> > > +
> > > +#define __CHECK(condition, format...) ({                               \
> > > +       int __ret = !!(condition);                                      \
> > > +       if (__ret) {                                                    \
> > > +               fprintf(stderr, "%s:%d:FAIL ", __func__, __LINE__);     \
> > > +               fprintf(stderr, format);                                \
> > > +       }                                                               \
> > > +       __ret;                                                          \
> > > +})
> > > +
> > > +#define CHECK(condition, format...)                                    \
> > > +       do {                                                            \
> > > +               if (__CHECK(condition, format))                         \
> > > +                       return -1;                                      \
> > > +       } while (0)
> >
> > it's better to make CHECK return value, makes its use more flexible
> >
> > > +
> > > +static struct btf *btf__parse_raw(const char *file)
> >
> > How about adding this as a libbpf API? It's not the first time I see
> > this being re-implemented. While simple, libbpf already implements
> > this internally, so there should be no need to require users do this
> > all the time. Follow up patch is ok, no need to block on this.
>
> yea, I copied that code around few times already,
> I'll add it to libbpf

Awesome, thanks!

>
> >
> > > +{
> > > +       struct btf *btf;
> > > +       struct stat st;
> > > +       __u8 *buf;
> > > +       FILE *f;
> > > +
> > > +       if (stat(file, &st))
> > > +               return NULL;
> > > +
> > > +       f = fopen(file, "rb");
> > > +       if (!f)
> > > +               return NULL;
> > > +
> > > +       buf = malloc(st.st_size);
> > > +       if (!buf) {
> > > +               btf = ERR_PTR(-ENOMEM);
> > > +               goto exit_close;
> > > +       }
> > > +
> > > +       if ((size_t) st.st_size != fread(buf, 1, st.st_size, f)) {
> > > +               btf = ERR_PTR(-EINVAL);
> > > +               goto exit_free;
> > > +       }
> > > +
> > > +       btf = btf__new(buf, st.st_size);
> > > +
> > > +exit_free:
> > > +       free(buf);
> > > +exit_close:
> > > +       fclose(f);
> > > +       return btf;
> > > +}
> > > +
> >
> > [...]
> >
> > > +
> > > +static int
> > > +__resolve_symbol(struct btf *btf, int type_id)
> > > +{
> > > +       const struct btf_type *type;
> > > +       const char *str;
> > > +       unsigned int i;
> > > +
> > > +       type = btf__type_by_id(btf, type_id);
> > > +       CHECK(!type, "Failed to get type for ID %d\n", type_id);
> >
> > return otherwise you'll get crash on few lines below; it's unpleasant
> > to debug crashes in VM in Travis CI
>
> the CHECK macro does 'return' on error

ah, right, confusing. But it will change once you convert to test_progs :)

>
> >
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(test_symbols); i++) {
> > > +               if (test_symbols[i].id != -1)
> > > +                       continue;
> > > +
> > > +               if (BTF_INFO_KIND(type->info) != test_symbols[i].type)
> > > +                       continue;
> > > +
> > > +               str = btf__name_by_offset(btf, type->name_off);
> > > +               if (!str) {
> >
> > CHECK?
>
> ok
>
> >
> > > +                       fprintf(stderr, "failed to get name for BTF ID %d\n",
> > > +                               type_id);
> > > +                       continue;
> > > +               }
> > > +
> > > +               if (!strcmp(str, test_symbols[i].name))
> > > +                       test_symbols[i].id = type_id;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int resolve_symbols(void)
> > > +{
> > > +       const char *path = VMLINUX_BTF;
> > > +       struct btf *btf;
> > > +       int type_id;
> > > +       __u32 nr;
> > > +       int err;
> > > +
> > > +       btf = btf_open(path);
> > > +       CHECK(libbpf_get_error(btf), "Failed to load BTF from %s\n", path);
> > > +
> >
> > exit, crash othewise
>
> the CHECK macro does 'return' on error
>
> >
> > > +       nr = btf__get_nr_types(btf);
> > > +
> > > +       for (type_id = 0; type_id < nr; type_id++) {
> >
> > type_id = 1; type_id <= nr
>
> damn.. not again ;-) sry

no worries :)

>
> thanks,
> jirka
>

Re: [PATCH v4 bpf-next 07/14] bpf: Allow nested BTF object to be refferenced by BTF object + offset

[Andrii Nakryiko]

On Thu, Jun 25, 2020 at 4:49 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> 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 will 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.
>
> Using btf_struct_access to implement new btf_struct_address
> function, because it already walks down the given BTF object.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h   |  3 ++
>  kernel/bpf/btf.c      | 67 ++++++++++++++++++++++++++++++++++++++-----
>  kernel/bpf/verifier.c | 37 +++++++++++++++---------
>  3 files changed, 87 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 3d2ade703a35..c0fd1f3037dd 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1300,6 +1300,9 @@ 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_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 701a2cb5dfb2..f87e5f1dc64d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3863,10 +3863,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,
> -                     enum bpf_access_type atype,
> -                     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;
> @@ -3914,8 +3926,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,
> -                                        next_btf_id);
> +               return struct_access(log, elem_type, off, size, data);

hm... this should probably be `goto again;` to avoid recursion. This
should have been caught in the original patch that added this
recursive call.

>
>  error:
>                 bpf_log(log, "access beyond struct %s at off %u size %u\n",
> @@ -4043,9 +4054,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)

before you can do this type check, you need to btf_type_skip_modifiers() first.

> +                               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;
> @@ -4059,7 +4082,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;
>                         }
>                 }
> @@ -4083,6 +4106,36 @@ 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,
> +                     enum bpf_access_type atype __maybe_unused,
> +                     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)
> +{
> +       const struct btf_type *type;
> +       struct access_data data = {
> +               .op = ACCESS_EXPECT,
> +       };
> +
> +       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 7de98906ddf4..da7351184295 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3808,6 +3808,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>         struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
>         enum bpf_reg_type expected_type, type = reg->type;
>         enum bpf_arg_type arg_type = fn->arg_type[arg];
> +       const struct btf_type *btf_type;
>         int err = 0;
>
>         if (arg_type == ARG_DONTCARE)
> @@ -3887,24 +3888,34 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                 expected_type = PTR_TO_BTF_ID;
>                 if (type != expected_type)
>                         goto err_type;
> -               if (!fn->check_btf_id) {
> -                       if (reg->btf_id != meta->btf_id) {
> -                               verbose(env, "Helper has type %s got %s in R%d\n",
> +               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 (!fn->check_btf_id) {
> +                               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;
> +                               }
> +                       } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> +                               verbose(env, "Helper does not support %s in R%d\n",
>                                         kernel_type_name(reg->btf_id), regno);
>
>                                 return -EACCES;
>                         }

Ok, I think I'm grasping this a bit more. How about we actually don't
have two different cases (btf_struct_access and btf_struct_address),
but instead make unified btf_struct_access that will return the
earliest field that register points to (so it sort of iterates deeper
and deeper with each invocation). So e.g., let's assume we have this
type:


struct A {
  struct B {
    struct C {
      int x;
    } c;
  } b;
  struct D { int y; } d;
};

Now consider the extreme case of a BPF helper that expects a pointer
to the struct C or D (so uses a custom btf_id check function to say if
a passed argument is acceptable or not), ok?

Now you write BPF program as such, r1 has pointer to struct A,
originally (so verifier knows btf_id points to struct A):

int prog(struct A *a) {
   return fancy_helper(&a->b.c);
}

Now, when verifier checks fancy_helper first time, its btf_id check
will say "nope". But before giving up, verifier calls
btf_struct_access, it goes into struct A field, finds field b with
offset 0, it matches register's offset (always zero in this scenario),
sees that that field is a struct B, so returns that register now
points to struct B. Verifier passed that updated BTF ID to
fancy_helper's check, it still says no. Again, don't give up,
btf_struct_access again, but now register assumes it starts in struct
B. It finds field c of type struct C, so returns successfully. Again,
we are checking with fancy_helper's check_btf_id() check, now it
succeeds, so we keep register's BTF_ID as struct C and carry on.

Now assume fancy_helper only accepts struct D. So once we pass struct
C, it rejects. Again, btf_struct_access() is called, this time find
field x, which is int (and thus register is SCALAR now).
check_btf_id() rejects it, we call btf_struct_access() again, but this
time we can't really go deeper into type int, so we give up at this
point and return error.

Now, when register's offset is non-zero, the process is exactly the
same, we just need to keep locally adjusted offset, so that when we
find inner struct, we start with the offset within that struct, not
origin struct A's offset.

It's quite verbose explanation, but hopefully you get the idea. I
think implementation shouldn't be too hard, we might need to extend
register's state to have this extra local offset to get to the start
of a type we believe register points to (something like inner_offset,
or something). Then btf_struct_access can take into account both
register's off and inner_off to maintain this offset to inner type.

It should nicely work in all cases, not just partially as it is right now. WDYT?


> -               } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> -                       verbose(env, "Helper does not support %s in R%d\n",
> -                               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 (!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;
> +                       }
>                 }
>         } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
>                 if (meta->func_id == BPF_FUNC_spin_lock) {
> --
> 2.25.4
>

Re: [PATCH v4 bpf-next 07/14] bpf: Allow nested BTF object to be refferenced by BTF object + offset

[Andrii Nakryiko]

On Tue, Jun 30, 2020 at 1:05 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jun 25, 2020 at 4:49 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > 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 will 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.
> >
> > Using btf_struct_access to implement new btf_struct_address
> > function, because it already walks down the given BTF object.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/bpf.h   |  3 ++
> >  kernel/bpf/btf.c      | 67 ++++++++++++++++++++++++++++++++++++++-----
> >  kernel/bpf/verifier.c | 37 +++++++++++++++---------
> >  3 files changed, 87 insertions(+), 20 deletions(-)
> >

[...]

> >
> >  error:
> >                 bpf_log(log, "access beyond struct %s at off %u size %u\n",
> > @@ -4043,9 +4054,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)
>
> before you can do this type check, you need to btf_type_skip_modifiers() first.
>

Ignore this part, btf_resolve_size() (somewhat unexpectedly) already does that.

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

[...]

Re: [PATCH v4 bpf-next 07/14] bpf: Allow nested BTF object to be refferenced by BTF object + offset

[Jiri Olsa]

On Tue, Jun 30, 2020 at 01:05:52PM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 25, 2020 at 4:49 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > 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 will 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.
> >
> > Using btf_struct_access to implement new btf_struct_address
> > function, because it already walks down the given BTF object.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/bpf.h   |  3 ++
> >  kernel/bpf/btf.c      | 67 ++++++++++++++++++++++++++++++++++++++-----
> >  kernel/bpf/verifier.c | 37 +++++++++++++++---------
> >  3 files changed, 87 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 3d2ade703a35..c0fd1f3037dd 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1300,6 +1300,9 @@ 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_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 701a2cb5dfb2..f87e5f1dc64d 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -3863,10 +3863,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,
> > -                     enum bpf_access_type atype,
> > -                     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;
> > @@ -3914,8 +3926,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,
> > -                                        next_btf_id);
> > +               return struct_access(log, elem_type, off, size, data);
> 
> hm... this should probably be `goto again;` to avoid recursion. This
> should have been caught in the original patch that added this
> recursive call.
> 
> >
> >  error:
> >                 bpf_log(log, "access beyond struct %s at off %u size %u\n",
> > @@ -4043,9 +4054,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)
> 
> before you can do this type check, you need to btf_type_skip_modifiers() first.
> 
> > +                               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;
> > @@ -4059,7 +4082,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;
> >                         }
> >                 }
> > @@ -4083,6 +4106,36 @@ 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,
> > +                     enum bpf_access_type atype __maybe_unused,
> > +                     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)
> > +{
> > +       const struct btf_type *type;
> > +       struct access_data data = {
> > +               .op = ACCESS_EXPECT,
> > +       };
> > +
> > +       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 7de98906ddf4..da7351184295 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3808,6 +3808,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >         struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> >         enum bpf_reg_type expected_type, type = reg->type;
> >         enum bpf_arg_type arg_type = fn->arg_type[arg];
> > +       const struct btf_type *btf_type;
> >         int err = 0;
> >
> >         if (arg_type == ARG_DONTCARE)
> > @@ -3887,24 +3888,34 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >                 expected_type = PTR_TO_BTF_ID;
> >                 if (type != expected_type)
> >                         goto err_type;
> > -               if (!fn->check_btf_id) {
> > -                       if (reg->btf_id != meta->btf_id) {
> > -                               verbose(env, "Helper has type %s got %s in R%d\n",
> > +               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 (!fn->check_btf_id) {
> > +                               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;
> > +                               }
> > +                       } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> > +                               verbose(env, "Helper does not support %s in R%d\n",
> >                                         kernel_type_name(reg->btf_id), regno);
> >
> >                                 return -EACCES;
> >                         }
> 
> Ok, I think I'm grasping this a bit more. How about we actually don't
> have two different cases (btf_struct_access and btf_struct_address),
> but instead make unified btf_struct_access that will return the
> earliest field that register points to (so it sort of iterates deeper
> and deeper with each invocation). So e.g., let's assume we have this
> type:
> 
> 
> struct A {
>   struct B {
>     struct C {
>       int x;
>     } c;
>   } b;
>   struct D { int y; } d;
> };
> 
> Now consider the extreme case of a BPF helper that expects a pointer
> to the struct C or D (so uses a custom btf_id check function to say if
> a passed argument is acceptable or not), ok?
> 
> Now you write BPF program as such, r1 has pointer to struct A,
> originally (so verifier knows btf_id points to struct A):
> 
> int prog(struct A *a) {
>    return fancy_helper(&a->b.c);
> }
> 
> Now, when verifier checks fancy_helper first time, its btf_id check
> will say "nope". But before giving up, verifier calls
> btf_struct_access, it goes into struct A field, finds field b with
> offset 0, it matches register's offset (always zero in this scenario),
> sees that that field is a struct B, so returns that register now
> points to struct B. Verifier passed that updated BTF ID to
> fancy_helper's check, it still says no. Again, don't give up,
> btf_struct_access again, but now register assumes it starts in struct
> B. It finds field c of type struct C, so returns successfully. Again,
> we are checking with fancy_helper's check_btf_id() check, now it
> succeeds, so we keep register's BTF_ID as struct C and carry on.
> 
> Now assume fancy_helper only accepts struct D. So once we pass struct
> C, it rejects. Again, btf_struct_access() is called, this time find
> field x, which is int (and thus register is SCALAR now).
> check_btf_id() rejects it, we call btf_struct_access() again, but this
> time we can't really go deeper into type int, so we give up at this
> point and return error.
> 
> Now, when register's offset is non-zero, the process is exactly the
> same, we just need to keep locally adjusted offset, so that when we
> find inner struct, we start with the offset within that struct, not
> origin struct A's offset.
> 
> It's quite verbose explanation, but hopefully you get the idea. I
> think implementation shouldn't be too hard, we might need to extend
> register's state to have this extra local offset to get to the start
> of a type we believe register points to (something like inner_offset,
> or something). Then btf_struct_access can take into account both
> register's off and inner_off to maintain this offset to inner type.
> 
> It should nicely work in all cases, not just partially as it is right now. WDYT?

I think above should work nicely for my case, but we need
to keep the current btf_struct_access usage, which is to
check if we point to a pointer type and return the ID it
points to 

I think it's doable with the functionality you described,
we'll just check of the returned type is pointer and get
the ID it points to.. which makes me think we still need
functions like below (again bad names probably ;-) )

  btf_struct_walk
    - implements the walk through the type as you described
      above.. returns the type we point to and we can call
      it again to resolve the next type at the same offset

  btf_struct_address
    - calls btf_struct_walk and checks if the returned type ID
      matches the requested BTF ID of the helper argument if not
      and the returned type is struct, call btf_struct_walk again
      to get the next layer and repeat..

  btf_struct_access
    - calls btf_struct_walk repeatedly until the returned type is
      a pointer and then it returns the BTF ID it points to

thanks,
jirka

Re: [PATCH v4 bpf-next 07/14] bpf: Allow nested BTF object to be refferenced by BTF object + offset

[Andrii Nakryiko]

On Thu, Jul 2, 2020 at 3:09 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Jun 30, 2020 at 01:05:52PM -0700, Andrii Nakryiko wrote:
> > On Thu, Jun 25, 2020 at 4:49 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > 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 will 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.
> > >
> > > Using btf_struct_access to implement new btf_struct_address
> > > function, because it already walks down the given BTF object.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---

[...]

> >
> > Ok, I think I'm grasping this a bit more. How about we actually don't
> > have two different cases (btf_struct_access and btf_struct_address),
> > but instead make unified btf_struct_access that will return the
> > earliest field that register points to (so it sort of iterates deeper
> > and deeper with each invocation). So e.g., let's assume we have this
> > type:
> >
> >
> > struct A {
> >   struct B {
> >     struct C {
> >       int x;
> >     } c;
> >   } b;
> >   struct D { int y; } d;
> > };
> >
> > Now consider the extreme case of a BPF helper that expects a pointer
> > to the struct C or D (so uses a custom btf_id check function to say if
> > a passed argument is acceptable or not), ok?
> >
> > Now you write BPF program as such, r1 has pointer to struct A,
> > originally (so verifier knows btf_id points to struct A):
> >
> > int prog(struct A *a) {
> >    return fancy_helper(&a->b.c);
> > }
> >
> > Now, when verifier checks fancy_helper first time, its btf_id check
> > will say "nope". But before giving up, verifier calls
> > btf_struct_access, it goes into struct A field, finds field b with
> > offset 0, it matches register's offset (always zero in this scenario),
> > sees that that field is a struct B, so returns that register now
> > points to struct B. Verifier passed that updated BTF ID to
> > fancy_helper's check, it still says no. Again, don't give up,
> > btf_struct_access again, but now register assumes it starts in struct
> > B. It finds field c of type struct C, so returns successfully. Again,
> > we are checking with fancy_helper's check_btf_id() check, now it
> > succeeds, so we keep register's BTF_ID as struct C and carry on.
> >
> > Now assume fancy_helper only accepts struct D. So once we pass struct
> > C, it rejects. Again, btf_struct_access() is called, this time find
> > field x, which is int (and thus register is SCALAR now).
> > check_btf_id() rejects it, we call btf_struct_access() again, but this
> > time we can't really go deeper into type int, so we give up at this
> > point and return error.
> >
> > Now, when register's offset is non-zero, the process is exactly the
> > same, we just need to keep locally adjusted offset, so that when we
> > find inner struct, we start with the offset within that struct, not
> > origin struct A's offset.
> >
> > It's quite verbose explanation, but hopefully you get the idea. I
> > think implementation shouldn't be too hard, we might need to extend
> > register's state to have this extra local offset to get to the start
> > of a type we believe register points to (something like inner_offset,
> > or something). Then btf_struct_access can take into account both
> > register's off and inner_off to maintain this offset to inner type.
> >
> > It should nicely work in all cases, not just partially as it is right now. WDYT?
>
> I think above should work nicely for my case, but we need
> to keep the current btf_struct_access usage, which is to
> check if we point to a pointer type and return the ID it
> points to
>
> I think it's doable with the functionality you described,
> we'll just check of the returned type is pointer and get
> the ID it points to.. which makes me think we still need
> functions like below (again bad names probably ;-) )
>
>   btf_struct_walk
>     - implements the walk through the type as you described
>       above.. returns the type we point to and we can call
>       it again to resolve the next type at the same offset
>
>   btf_struct_address
>     - calls btf_struct_walk and checks if the returned type ID
>       matches the requested BTF ID of the helper argument if not
>       and the returned type is struct, call btf_struct_walk again
>       to get the next layer and repeat..
>
>   btf_struct_access
>     - calls btf_struct_walk repeatedly until the returned type is
>       a pointer and then it returns the BTF ID it points to
>

Sure, as long as all the BTF walking is contained in one place (in
btf_struct_walk), which was my main objection on your very first
version. All the other wrapper functions can have their own extra
restrictions, but still use the same struct-walking primitive
operation. Ok, glad we figured it out :)

> thanks,
> jirka
>

Re: [PATCH v4 bpf-next 10/14] bpf: Add d_path helper

[KP Singh]

On 6/28/20 9:42 PM, Jiri Olsa wrote:
> On Fri, Jun 26, 2020 at 01:38:27PM -0700, Andrii Nakryiko wrote:
>> On Thu, Jun 25, 2020 at 4:49 PM 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/uapi/linux/bpf.h       | 14 +++++++++-
>>>  kernel/trace/bpf_trace.c       | 47 ++++++++++++++++++++++++++++++++++
>>>  scripts/bpf_helpers_doc.py     |  2 ++
>>>  tools/include/uapi/linux/bpf.h | 14 +++++++++-
>>>  4 files changed, 75 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 0cb8ec948816..23274c81f244 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -3285,6 +3285,17 @@ union bpf_attr {
>>>   *             Dynamically cast a *sk* pointer to a *udp6_sock* pointer.
>>>   *     Return
>>>   *             *sk* if casting is valid, or NULL otherwise.
>>> + *
>>> + * 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
>>
>> It's important to note whether string is always zero-terminated (I'm
>> guessing it is, right?).
> 
> right, will add

Also note that bpf_probe_read_{kernel, user}_str return the length including
the NUL byte:

 * 	Return
 * 		On success, the strictly positive length of the string,
 * 		including the trailing NUL character. On error, a negative
 * 		value.

It would be good to keep this uniform. So you will need a len += 1 here as well.

- KP

> 
>>
>>> + *
>>>   */
>>>  #define __BPF_FUNC_MAPPER(FN)          \
>>>         FN(unspec),                     \
>>> @@ -3427,7 +3438,8 @@ union bpf_attr {
>>>         FN(skc_to_tcp_sock),            \
>>>         FN(skc_to_tcp_timewait_sock),   \
>>>         FN(skc_to_tcp_request_sock),    \
>>> -       FN(skc_to_udp6_sock),
>>> +       FN(skc_to_udp6_sock),           \
>>> +       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/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>> index b124d468688c..6f31e21565b6 100644
>>> --- a/kernel/trace/bpf_trace.c
>>> +++ b/kernel/trace/bpf_trace.c
>>> @@ -1060,6 +1060,51 @@ 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;
>>
>> if len above is zero, you won't zero-terminate it, so probably better
>> to move buf[len] = 0 out of if to do unconditionally
> 
> good catch, will change
> 
>>
>>> +               }
>>> +       }
>>> +
>>> +       return len;
>>> +}
>>> +
>>> +BTF_SET_START(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_SET_END(btf_whitelist_d_path)
>>> +
>>> +static bool bpf_d_path_allowed(const struct bpf_prog *prog)
>>> +{
>>> +       return btf_id_set_contains(&btf_whitelist_d_path, prog->aux->attach_btf_id);
>>> +}
>>> +
>>
>> This looks pretty great and clean, considering what's happening under
>> the covers. Nice work, thanks a lot!
>>
>>> +BTF_ID_LIST(bpf_d_path_btf_ids)
>>> +BTF_ID(struct, path)
>>
>> this is a bit more confusing to read and error-prone, but I couldn't
>> come up with any better way to do this... Still better than
>> alternatives.
>>
>>> +
>>> +static const struct bpf_func_proto bpf_d_path_proto = {
>>> +       .func           = bpf_d_path,
>>> +       .gpl_only       = true,
>>
>> Does it have to be GPL-only? What's the criteria? Sorry if this was
>> brought up previously.
> 
> I don't think it's needed to be gpl_only, I'll set it to false
> 
> thanks,
> jirka
> 

Re: [PATCH v4 bpf-next 10/14] bpf: Add d_path helper

[Jiri Olsa]

On Fri, Jul 17, 2020 at 01:13:23AM +0200, KP Singh wrote:
> 
> 
> On 6/28/20 9:42 PM, Jiri Olsa wrote:
> > On Fri, Jun 26, 2020 at 01:38:27PM -0700, Andrii Nakryiko wrote:
> >> On Thu, Jun 25, 2020 at 4:49 PM 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/uapi/linux/bpf.h       | 14 +++++++++-
> >>>  kernel/trace/bpf_trace.c       | 47 ++++++++++++++++++++++++++++++++++
> >>>  scripts/bpf_helpers_doc.py     |  2 ++
> >>>  tools/include/uapi/linux/bpf.h | 14 +++++++++-
> >>>  4 files changed, 75 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>> index 0cb8ec948816..23274c81f244 100644
> >>> --- a/include/uapi/linux/bpf.h
> >>> +++ b/include/uapi/linux/bpf.h
> >>> @@ -3285,6 +3285,17 @@ union bpf_attr {
> >>>   *             Dynamically cast a *sk* pointer to a *udp6_sock* pointer.
> >>>   *     Return
> >>>   *             *sk* if casting is valid, or NULL otherwise.
> >>> + *
> >>> + * 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
> >>
> >> It's important to note whether string is always zero-terminated (I'm
> >> guessing it is, right?).
> > 
> > right, will add
> 
> Also note that bpf_probe_read_{kernel, user}_str return the length including
> the NUL byte:
> 
>  * 	Return
>  * 		On success, the strictly positive length of the string,
>  * 		including the trailing NUL character. On error, a negative
>  * 		value.
> 
> It would be good to keep this uniform. So you will need a len += 1 here as well.

good point, will keep it that way

thanks,
jirka