[PATCH bpf-next 0/5] Add fd_array_cnt attribute for BPF_PROG_LOAD

[PATCH bpf-next 0/5] Add fd_array_cnt attribute for BPF_PROG_LOAD

[Anton Protopopov]

Add a new attribute to the bpf(BPF_PROG_LOAD) system call. If this
new attribute is non-zero, then the fd_array is considered to be a
continuous array of the fd_array_cnt length and to contain only
proper map file descriptors, or btf file descriptors, or zeroes.

This change allows maps, which aren't referenced directly by a BPF
program, to be bound to the program _and_ also to be present during
the program verification (so BPF_PROG_BIND_MAP is not enough for this
use case).

The primary reason for this change is that it is a prerequisite for
adding "instruction set" maps, which are both non-referenced by the
program and must be present during the program verification.

The first three commits add the new functionality, the fourth adds
corresponding self-tests, and the last one is a small additional fix.

Anton Protopopov (5):
  bpf: add a __btf_get_by_fd helper
  bpf: move map/prog compatibility checks
  bpf: add fd_array_cnt attribute for prog_load
  selftests/bpf: Add tests for fd_array_cnt
  bpf: fix potential error return

 include/linux/btf.h                           |  13 +
 include/uapi/linux/bpf.h                      |  10 +
 kernel/bpf/btf.c                              |  13 +-
 kernel/bpf/core.c                             |   9 +-
 kernel/bpf/syscall.c                          |   2 +-
 kernel/bpf/verifier.c                         | 204 ++++++----
 tools/include/uapi/linux/bpf.h                |  10 +
 .../selftests/bpf/prog_tests/fd_array.c       | 374 ++++++++++++++++++
 8 files changed, 557 insertions(+), 78 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fd_array.c

-- 
2.34.1

[PATCH bpf-next 1/5] bpf: add a __btf_get_by_fd helper

[Anton Protopopov]

Add a new helper to get a pointer to a struct btf from a file
descriptor which doesn't increase a refcount.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 include/linux/btf.h | 13 +++++++++++++
 kernel/bpf/btf.c    | 13 ++++---------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 4214e76c9168..050051a578a8 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -4,6 +4,7 @@
 #ifndef _LINUX_BTF_H
 #define _LINUX_BTF_H 1
 
+#include <linux/file.h>
 #include <linux/types.h>
 #include <linux/bpfptr.h>
 #include <linux/bsearch.h>
@@ -143,6 +144,18 @@ void btf_get(struct btf *btf);
 void btf_put(struct btf *btf);
 const struct btf_header *btf_header(const struct btf *btf);
 int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_sz);
+
+static inline struct btf *__btf_get_by_fd(struct fd f)
+{
+	if (fd_empty(f))
+		return ERR_PTR(-EBADF);
+
+	if (unlikely(fd_file(f)->f_op != &btf_fops))
+		return ERR_PTR(-EINVAL);
+
+	return fd_file(f)->private_data;
+}
+
 struct btf *btf_get_by_fd(int fd);
 int btf_get_info_by_fd(const struct btf *btf,
 		       const union bpf_attr *attr,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e7a59e6462a9..ad5310fa1d3b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7743,17 +7743,12 @@ int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
 
 struct btf *btf_get_by_fd(int fd)
 {
-	struct btf *btf;
 	CLASS(fd, f)(fd);
+	struct btf *btf;
 
-	if (fd_empty(f))
-		return ERR_PTR(-EBADF);
-
-	if (fd_file(f)->f_op != &btf_fops)
-		return ERR_PTR(-EINVAL);
-
-	btf = fd_file(f)->private_data;
-	refcount_inc(&btf->refcnt);
+	btf = __btf_get_by_fd(f);
+	if (!IS_ERR(btf))
+		refcount_inc(&btf->refcnt);
 
 	return btf;
 }
-- 
2.34.1

[PATCH bpf-next 2/5] bpf: move map/prog compatibility checks

[Anton Protopopov]

Move some inlined map/prog compatibility checks from the
resolve_pseudo_ldimm64() function to the dedicated
check_map_prog_compatibility() function. Call the latter function
from the add_used_map_from_fd() function directly.

This simplifies code and optimizes logic a bit, as before these
changes the check_map_prog_compatibility() function was executed on
every map usage, which doesn't make sense, as it doesn't include any
per-instruction checks, only map type vs. prog type.

(This patch also simplifies a consequent patch which will call the
add_used_map_from_fd() function from another code path.)

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 kernel/bpf/verifier.c | 101 +++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 55 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f4c39bb50511..45c11d9cee60 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19075,6 +19075,12 @@ static bool is_tracing_prog_type(enum bpf_prog_type type)
 	}
 }
 
+static bool bpf_map_is_cgroup_storage(struct bpf_map *map)
+{
+	return (map->map_type == BPF_MAP_TYPE_CGROUP_STORAGE ||
+		map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
+}
+
 static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 					struct bpf_map *map,
 					struct bpf_prog *prog)
@@ -19153,25 +19159,48 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 			return -EINVAL;
 		}
 
-	return 0;
-}
+	if (bpf_map_is_cgroup_storage(map) &&
+	    bpf_cgroup_storage_assign(env->prog->aux, map)) {
+		verbose(env, "only one cgroup storage of each type is allowed\n");
+		return -EBUSY;
+	}
 
-static bool bpf_map_is_cgroup_storage(struct bpf_map *map)
-{
-	return (map->map_type == BPF_MAP_TYPE_CGROUP_STORAGE ||
-		map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
+	if (map->map_type == BPF_MAP_TYPE_ARENA) {
+		if (env->prog->aux->arena) {
+			verbose(env, "Only one arena per program\n");
+			return -EBUSY;
+		}
+		if (!env->allow_ptr_leaks || !env->bpf_capable) {
+			verbose(env, "CAP_BPF and CAP_PERFMON are required to use arena\n");
+			return -EPERM;
+		}
+		if (!env->prog->jit_requested) {
+			verbose(env, "JIT is required to use arena\n");
+			return -EOPNOTSUPP;
+		}
+		if (!bpf_jit_supports_arena()) {
+			verbose(env, "JIT doesn't support arena\n");
+			return -EOPNOTSUPP;
+		}
+		env->prog->aux->arena = (void *)map;
+		if (!bpf_arena_get_user_vm_start(env->prog->aux->arena)) {
+			verbose(env, "arena's user address must be set via map_extra or mmap()\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
 }
 
 /* Add map behind fd to used maps list, if it's not already there, and return
- * its index. Also set *reused to true if this map was already in the list of
- * used maps.
+ * its index.
  * Returns <0 on error, or >= 0 index, on success.
  */
-static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd, bool *reused)
+static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd)
 {
 	CLASS(fd, f)(fd);
 	struct bpf_map *map;
-	int i;
+	int i, err;
 
 	map = __bpf_map_get(f);
 	if (IS_ERR(map)) {
@@ -19180,12 +19209,9 @@ static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd, bool *reus
 	}
 
 	/* check whether we recorded this map already */
-	for (i = 0; i < env->used_map_cnt; i++) {
-		if (env->used_maps[i] == map) {
-			*reused = true;
+	for (i = 0; i < env->used_map_cnt; i++)
+		if (env->used_maps[i] == map)
 			return i;
-		}
-	}
 
 	if (env->used_map_cnt >= MAX_USED_MAPS) {
 		verbose(env, "The total number of maps per program has reached the limit of %u\n",
@@ -19193,6 +19219,10 @@ static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd, bool *reus
 		return -E2BIG;
 	}
 
+	err = check_map_prog_compatibility(env, map, env->prog);
+	if (err)
+		return err;
+
 	if (env->prog->sleepable)
 		atomic64_inc(&map->sleepable_refcnt);
 
@@ -19203,7 +19233,6 @@ static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd, bool *reus
 	 */
 	bpf_map_inc(map);
 
-	*reused = false;
 	env->used_maps[env->used_map_cnt++] = map;
 
 	return env->used_map_cnt - 1;
@@ -19240,7 +19269,6 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 			int map_idx;
 			u64 addr;
 			u32 fd;
-			bool reused;
 
 			if (i == insn_cnt - 1 || insn[1].code != 0 ||
 			    insn[1].dst_reg != 0 || insn[1].src_reg != 0 ||
@@ -19301,7 +19329,7 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 				break;
 			}
 
-			map_idx = add_used_map_from_fd(env, fd, &reused);
+			map_idx = add_used_map_from_fd(env, fd);
 			if (map_idx < 0)
 				return map_idx;
 			map = env->used_maps[map_idx];
@@ -19309,10 +19337,6 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 			aux = &env->insn_aux_data[i];
 			aux->map_index = map_idx;
 
-			err = check_map_prog_compatibility(env, map, env->prog);
-			if (err)
-				return err;
-
 			if (insn[0].src_reg == BPF_PSEUDO_MAP_FD ||
 			    insn[0].src_reg == BPF_PSEUDO_MAP_IDX) {
 				addr = (unsigned long)map;
@@ -19343,39 +19367,6 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 			insn[0].imm = (u32)addr;
 			insn[1].imm = addr >> 32;
 
-			/* proceed with extra checks only if its newly added used map */
-			if (reused)
-				goto next_insn;
-
-			if (bpf_map_is_cgroup_storage(map) &&
-			    bpf_cgroup_storage_assign(env->prog->aux, map)) {
-				verbose(env, "only one cgroup storage of each type is allowed\n");
-				return -EBUSY;
-			}
-			if (map->map_type == BPF_MAP_TYPE_ARENA) {
-				if (env->prog->aux->arena) {
-					verbose(env, "Only one arena per program\n");
-					return -EBUSY;
-				}
-				if (!env->allow_ptr_leaks || !env->bpf_capable) {
-					verbose(env, "CAP_BPF and CAP_PERFMON are required to use arena\n");
-					return -EPERM;
-				}
-				if (!env->prog->jit_requested) {
-					verbose(env, "JIT is required to use arena\n");
-					return -EOPNOTSUPP;
-				}
-				if (!bpf_jit_supports_arena()) {
-					verbose(env, "JIT doesn't support arena\n");
-					return -EOPNOTSUPP;
-				}
-				env->prog->aux->arena = (void *)map;
-				if (!bpf_arena_get_user_vm_start(env->prog->aux->arena)) {
-					verbose(env, "arena's user address must be set via map_extra or mmap()\n");
-					return -EINVAL;
-				}
-			}
-
 next_insn:
 			insn++;
 			i++;
-- 
2.34.1

[PATCH bpf-next 3/5] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov]

The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
of file descriptors: maps or btfs. This field was introduced as a
sparse array. Introduce a new attribute, fd_array_cnt, which, if
present, indicates that the fd_array is a continuous array of the
corresponding length.

If fd_array_cnt is non-zero, then every map in the fd_array will be
bound to the program, as if it was used by the program. This
functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
maps can be used by the verifier during the program load.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 include/uapi/linux/bpf.h       |  10 +++
 kernel/bpf/syscall.c           |   2 +-
 kernel/bpf/verifier.c          | 107 ++++++++++++++++++++++++++++-----
 tools/include/uapi/linux/bpf.h |  10 +++
 4 files changed, 114 insertions(+), 15 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 4162afc6b5d0..2acf9b336371 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1573,6 +1573,16 @@ union bpf_attr {
 		 * If provided, prog_flags should have BPF_F_TOKEN_FD flag set.
 		 */
 		__s32		prog_token_fd;
+		/* The fd_array_cnt can be used to pass the length of the
+		 * fd_array array. In this case all the [map] file descriptors
+		 * passed in this array will be bound to the program, even if
+		 * the maps are not referenced directly. The functionality is
+		 * similar to the BPF_PROG_BIND_MAP syscall, but maps can be
+		 * used by the verifier during the program load. If provided,
+		 * then the fd_array[0,...,fd_array_cnt-1] is expected to be
+		 * continuous.
+		 */
+		__u32		fd_array_cnt;
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 58190ca724a2..7e3fbc23c742 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2729,7 +2729,7 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
 }
 
 /* last field in 'union bpf_attr' used by this command */
-#define BPF_PROG_LOAD_LAST_FIELD prog_token_fd
+#define BPF_PROG_LOAD_LAST_FIELD fd_array_cnt
 
 static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
 {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 45c11d9cee60..2e262f6516b3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19192,22 +19192,10 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 	return 0;
 }
 
-/* Add map behind fd to used maps list, if it's not already there, and return
- * its index.
- * Returns <0 on error, or >= 0 index, on success.
- */
-static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd)
+static int add_used_map(struct bpf_verifier_env *env, struct bpf_map *map)
 {
-	CLASS(fd, f)(fd);
-	struct bpf_map *map;
 	int i, err;
 
-	map = __bpf_map_get(f);
-	if (IS_ERR(map)) {
-		verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
-		return PTR_ERR(map);
-	}
-
 	/* check whether we recorded this map already */
 	for (i = 0; i < env->used_map_cnt; i++)
 		if (env->used_maps[i] == map)
@@ -19238,6 +19226,24 @@ static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd)
 	return env->used_map_cnt - 1;
 }
 
+/* Add map behind fd to used maps list, if it's not already there, and return
+ * its index.
+ * Returns <0 on error, or >= 0 index, on success.
+ */
+static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd)
+{
+	struct bpf_map *map;
+	CLASS(fd, f)(fd);
+
+	map = __bpf_map_get(f);
+	if (IS_ERR(map)) {
+		verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
+		return PTR_ERR(map);
+	}
+
+	return add_used_map(env, map);
+}
+
 /* find and rewrite pseudo imm in ld_imm64 instructions:
  *
  * 1. if it accesses map FD, replace it with actual map pointer.
@@ -22537,6 +22543,76 @@ struct btf *bpf_get_btf_vmlinux(void)
 	return btf_vmlinux;
 }
 
+/*
+ * The add_fd_from_fd_array() is executed only if fd_array_cnt is given.  In
+ * this case expect that every file descriptor in the array is either a map or
+ * a BTF, or a hole (0). Everything else is considered to be trash.
+ */
+static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
+{
+	struct bpf_map *map;
+	CLASS(fd, f)(fd);
+	int ret;
+
+	map = __bpf_map_get(f);
+	if (IS_ERR(map)) {
+		if (!IS_ERR(__btf_get_by_fd(f)))
+			return 0;
+
+		/* allow holes */
+		if (!fd)
+			return 0;
+
+		verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
+		return PTR_ERR(map);
+	}
+
+	ret = add_used_map(env, map);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int env_init_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
+{
+	int size = sizeof(int) * attr->fd_array_cnt;
+	int *copy;
+	int ret;
+	int i;
+
+	if (attr->fd_array_cnt >= MAX_USED_MAPS)
+		return -E2BIG;
+
+	env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
+
+	/*
+	 * The only difference between old (no fd_array_cnt is given) and new
+	 * APIs is that in the latter case the fd_array is expected to be
+	 * continuous and is scanned for map fds right away
+	 */
+	if (!size)
+		return 0;
+
+	copy = kzalloc(size, GFP_KERNEL);
+	if (!copy)
+		return -ENOMEM;
+
+	if (copy_from_bpfptr_offset(copy, env->fd_array, 0, size)) {
+		ret = -EFAULT;
+		goto free_copy;
+	}
+
+	for (i = 0; i < attr->fd_array_cnt; i++) {
+		ret = add_fd_from_fd_array(env, copy[i]);
+		if (ret)
+			goto free_copy;
+	}
+
+free_copy:
+	kfree(copy);
+	return ret;
+}
+
 int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size)
 {
 	u64 start_time = ktime_get_ns();
@@ -22568,7 +22644,9 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
 		env->insn_aux_data[i].orig_idx = i;
 	env->prog = *prog;
 	env->ops = bpf_verifier_ops[env->prog->type];
-	env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
+	ret = env_init_fd_array(env, attr, uattr);
+	if (ret)
+		goto err_free_aux_data;
 
 	env->allow_ptr_leaks = bpf_allow_ptr_leaks(env->prog->aux->token);
 	env->allow_uninit_stack = bpf_allow_uninit_stack(env->prog->aux->token);
@@ -22786,6 +22864,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
 err_unlock:
 	if (!is_priv)
 		mutex_unlock(&bpf_verifier_lock);
+err_free_aux_data:
 	vfree(env->insn_aux_data);
 err_free_env:
 	kvfree(env);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 4162afc6b5d0..2acf9b336371 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1573,6 +1573,16 @@ union bpf_attr {
 		 * If provided, prog_flags should have BPF_F_TOKEN_FD flag set.
 		 */
 		__s32		prog_token_fd;
+		/* The fd_array_cnt can be used to pass the length of the
+		 * fd_array array. In this case all the [map] file descriptors
+		 * passed in this array will be bound to the program, even if
+		 * the maps are not referenced directly. The functionality is
+		 * similar to the BPF_PROG_BIND_MAP syscall, but maps can be
+		 * used by the verifier during the program load. If provided,
+		 * then the fd_array[0,...,fd_array_cnt-1] is expected to be
+		 * continuous.
+		 */
+		__u32		fd_array_cnt;
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
-- 
2.34.1

[PATCH bpf-next 4/5] selftests/bpf: Add tests for fd_array_cnt

[Anton Protopopov]

Add a new set of tests to test the new field in PROG_LOAD-related
part of bpf_attr: fd_array_cnt.

Add the following test cases:

  * fd_array_cnt/no-fd-array: program is loaded in a normal
    way, without any fd_array present

  * fd_array_cnt/fd-array-ok: pass two extra non-used maps,
    check that they're bound to the program

  * fd_array_cnt/fd-array-dup-input: pass a few extra maps,
    only two of which are unique

  * fd_array_cnt/fd-array-ref-maps-in-array: pass a map in
    fd_array which is also referenced from within the program

  * fd_array_cnt/fd-array-trash-input: pass array with some trash

  * fd_array_cnt/fd-array-with-holes: pass an array with holes (fd=0)

  * fd_array_cnt/fd-array-2big: pass too large array

All the tests above are using the bpf(2) syscall directly,
no libbpf involved.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 .../selftests/bpf/prog_tests/fd_array.c       | 374 ++++++++++++++++++
 1 file changed, 374 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fd_array.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fd_array.c b/tools/testing/selftests/bpf/prog_tests/fd_array.c
new file mode 100644
index 000000000000..8b091b428e31
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/fd_array.c
@@ -0,0 +1,374 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+
+#include <linux/btf.h>
+#include <sys/syscall.h>
+#include <bpf/bpf.h>
+
+static inline int _bpf_map_create(void)
+{
+	static union bpf_attr attr = {
+		.map_type = BPF_MAP_TYPE_ARRAY,
+		.key_size = 4,
+		.value_size = 8,
+		.max_entries = 1,
+	};
+
+	return syscall(__NR_bpf, BPF_MAP_CREATE, &attr, sizeof(attr));
+}
+
+#define BTF_INFO_ENC(kind, kind_flag, vlen) \
+	((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN))
+#define BTF_TYPE_ENC(name, info, size_or_type) (name), (info), (size_or_type)
+#define BTF_INT_ENC(encoding, bits_offset, nr_bits) \
+	((encoding) << 24 | (bits_offset) << 16 | (nr_bits))
+#define BTF_TYPE_INT_ENC(name, encoding, bits_offset, bits, sz) \
+	BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_INT, 0, 0), sz), \
+	BTF_INT_ENC(encoding, bits_offset, bits)
+
+static int _btf_create(void)
+{
+	struct btf_blob {
+		struct btf_header btf_hdr;
+		__u32 types[8];
+		__u32 str;
+	} raw_btf = {
+		.btf_hdr = {
+			.magic = BTF_MAGIC,
+			.version = BTF_VERSION,
+			.hdr_len = sizeof(struct btf_header),
+			.type_len = sizeof(__u32) * 8,
+			.str_off = sizeof(__u32) * 8,
+			.str_len = sizeof(__u32),
+		},
+		.types = {
+			/* long */
+			BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 64, 8),  /* [1] */
+			/* unsigned long */
+			BTF_TYPE_INT_ENC(0, 0, 0, 64, 8),  /* [2] */
+		},
+	};
+	static union bpf_attr attr = {
+		.btf_size = sizeof(raw_btf),
+	};
+
+	attr.btf = (long)&raw_btf;
+
+	return syscall(__NR_bpf, BPF_BTF_LOAD, &attr, sizeof(attr));
+}
+
+static bool map_exists(__u32 id)
+{
+	int fd;
+
+	fd = bpf_map_get_fd_by_id(id);
+	if (fd >= 0) {
+		close(fd);
+		return true;
+	}
+	return false;
+}
+
+static inline int bpf_prog_get_map_ids(int prog_fd, __u32 *nr_map_ids, __u32 *map_ids)
+{
+	__u32 len = sizeof(struct bpf_prog_info);
+	struct bpf_prog_info info = {
+		.nr_map_ids = *nr_map_ids,
+		.map_ids = ptr_to_u64(map_ids),
+	};
+	int err;
+
+	err = bpf_prog_get_info_by_fd(prog_fd, &info, &len);
+	if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd"))
+		return -1;
+
+	*nr_map_ids = info.nr_map_ids;
+
+	return 0;
+}
+
+static int __load_test_prog(int map_fd, int *fd_array, int fd_array_cnt)
+{
+	/* A trivial program which uses one map */
+	struct bpf_insn insns[] = {
+		BPF_LD_MAP_FD(BPF_REG_1, map_fd),
+		BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+	union bpf_attr attr = {
+		.prog_type = BPF_PROG_TYPE_XDP, /* we don't care */
+		.insns     = ptr_to_u64(insns),
+		.insn_cnt  = ARRAY_SIZE(insns),
+		.license   = ptr_to_u64("GPL"),
+		.fd_array = ptr_to_u64(fd_array),
+		.fd_array_cnt = fd_array_cnt,
+	};
+
+	return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr));
+}
+
+static int load_test_prog(int *fd_array, int fd_array_cnt)
+{
+	int map_fd;
+	int ret;
+
+	map_fd = _bpf_map_create();
+	if (!ASSERT_GE(map_fd, 0, "_bpf_map_create"))
+		return map_fd;
+
+	ret = __load_test_prog(map_fd, fd_array, fd_array_cnt);
+	close(map_fd);
+
+	/* switch back to returning the actual value */
+	if (ret < 0)
+		return -errno;
+	return ret;
+}
+
+static bool check_expected_map_ids(int prog_fd, int expected, __u32 *map_ids, __u32 *nr_map_ids)
+{
+	int err;
+
+	err = bpf_prog_get_map_ids(prog_fd, nr_map_ids, map_ids);
+	if (!ASSERT_OK(err, "bpf_prog_get_map_ids"))
+		return false;
+	if (!ASSERT_EQ(*nr_map_ids, expected, "unexpected nr_map_ids"))
+		return false;
+
+	return true;
+}
+
+/*
+ * Load a program, which uses one map. No fd_array maps are present.
+ * On return only one map is expected to be bound to prog.
+ */
+static void check_fd_array_cnt__no_fd_array(void)
+{
+	__u32 map_ids[16];
+	__u32 nr_map_ids;
+	int prog_fd;
+
+	prog_fd = load_test_prog(NULL, 0);
+	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
+		return;
+	nr_map_ids = ARRAY_SIZE(map_ids);
+	check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids);
+	close(prog_fd);
+}
+
+/*
+ * Load a program, which uses one map, and pass two extra, non-equal, maps in
+ * fd_array with fd_array_cnt=2. On return three maps are expected to be bound
+ * to the program.
+ */
+static void check_fd_array_cnt__fd_array_ok(void)
+{
+	int extra_fds[128];
+	__u32 map_ids[16];
+	__u32 nr_map_ids;
+	int prog_fd;
+
+	extra_fds[0] = _bpf_map_create();
+	if (!ASSERT_GE(extra_fds[0], 0, "_bpf_map_create"))
+		return;
+	extra_fds[1] = _bpf_map_create();
+	if (!ASSERT_GE(extra_fds[1], 0, "_bpf_map_create"))
+		return;
+	prog_fd = load_test_prog(extra_fds, 2);
+	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
+		return;
+	nr_map_ids = ARRAY_SIZE(map_ids);
+	if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
+		return;
+
+	/* maps should still exist when original file descriptors are closed */
+	close(extra_fds[0]);
+	close(extra_fds[1]);
+	if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map_ids[0] should exist"))
+		return;
+	if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map_ids[1] should exist"))
+		return;
+
+	close(prog_fd);
+}
+
+/*
+ * Load a program with a few extra maps duplicated in the fd_array.
+ * After the load maps should only be referenced once.
+ */
+static void check_fd_array_cnt__duplicated_maps(void)
+{
+	int extra_fds[128];
+	__u32 map_ids[16];
+	__u32 nr_map_ids;
+	int prog_fd;
+
+	extra_fds[0] = extra_fds[2] = _bpf_map_create();
+	if (!ASSERT_GE(extra_fds[0], 0, "_bpf_map_create"))
+		return;
+	extra_fds[1] = extra_fds[3] = _bpf_map_create();
+	if (!ASSERT_GE(extra_fds[1], 0, "_bpf_map_create"))
+		return;
+	prog_fd = load_test_prog(extra_fds, 4);
+	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
+		return;
+	nr_map_ids = ARRAY_SIZE(map_ids);
+	if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
+		return;
+
+	/* maps should still exist when original file descriptors are closed */
+	close(extra_fds[0]);
+	close(extra_fds[1]);
+	if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
+		return;
+	if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map should exist"))
+		return;
+
+	close(prog_fd);
+}
+
+/*
+ * Check that if maps which are referenced by a program are
+ * passed in fd_array, then they will be referenced only once
+ */
+static void check_fd_array_cnt__referenced_maps_in_fd_array(void)
+{
+	int extra_fds[128];
+	__u32 map_ids[16];
+	__u32 nr_map_ids;
+	int prog_fd;
+
+	extra_fds[0] = _bpf_map_create();
+	if (!ASSERT_GE(extra_fds[0], 0, "_bpf_map_create"))
+		return;
+	prog_fd = __load_test_prog(extra_fds[0], extra_fds, 1);
+	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
+		return;
+	nr_map_ids = ARRAY_SIZE(map_ids);
+	if (!check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids))
+		return;
+
+	/* map should still exist when original file descriptor is closed */
+	close(extra_fds[0]);
+	if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
+		return;
+
+	/* map should disappear when the program is closed */
+	close(prog_fd);
+}
+
+/*
+ * Test that a program with trash in fd_array can't be loaded:
+ * only map and BTF file descriptors should be accepted.
+ */
+static void check_fd_array_cnt__fd_array_with_trash(void)
+{
+	int extra_fds[128];
+	int prog_fd;
+
+	extra_fds[0] = _bpf_map_create();
+	if (!ASSERT_GE(extra_fds[0], 0, "_bpf_map_create"))
+		return;
+	extra_fds[1] = _btf_create();
+	if (!ASSERT_GE(extra_fds[1], 0, "_btf_create"))
+		return;
+
+	/* trash 1: not a file descriptor */
+	extra_fds[2] = 0xbeef;
+	prog_fd = load_test_prog(extra_fds, 3);
+	if (!ASSERT_EQ(prog_fd, -EBADF, "prog should have been rejected with -EBADF"))
+		return;
+
+	/* trash 2: not a map or btf */
+	extra_fds[2] = socket(AF_INET, SOCK_STREAM, 0);
+	if (!ASSERT_GE(extra_fds[2], 0, "socket"))
+		return;
+
+	prog_fd = load_test_prog(extra_fds, 3);
+	if (!ASSERT_EQ(prog_fd, -EINVAL, "prog should have been rejected with -EINVAL"))
+		return;
+
+	close(extra_fds[2]);
+	close(extra_fds[1]);
+	close(extra_fds[0]);
+}
+
+/*
+ * Test that a program with zeroes (= holes) in fd_array can be loaded:
+ * only map and BTF file descriptors should be accepted.
+ */
+static void check_fd_array_cnt__fd_array_with_holes(void)
+{
+	int extra_fds[128];
+	int prog_fd;
+
+	extra_fds[0] = _bpf_map_create();
+	if (!ASSERT_GE(extra_fds[0], 0, "_bpf_map_create"))
+		return;
+	/* punch a hole*/
+	extra_fds[1] = 0;
+	extra_fds[2] = _btf_create();
+	if (!ASSERT_GE(extra_fds[1], 0, "_btf_create"))
+		return;
+	/* punch a hole*/
+	extra_fds[3] = 0;
+
+	prog_fd = load_test_prog(extra_fds, 4);
+	ASSERT_GE(prog_fd, 0, "prog with holes should have been loaded");
+
+	close(extra_fds[2]);
+	close(extra_fds[0]);
+}
+
+
+/*
+ * Test that a program with too big fd_array can't be loaded.
+ */
+static void check_fd_array_cnt__fd_array_too_big(void)
+{
+	int extra_fds[128];
+	int prog_fd;
+	int i;
+
+	for (i = 0; i < 65; i++) {
+		extra_fds[i] = _bpf_map_create();
+		if (!ASSERT_GE(extra_fds[i], 0, "_bpf_map_create"))
+			goto cleanup_fds;
+	}
+
+	prog_fd = load_test_prog(extra_fds, 65);
+	ASSERT_EQ(prog_fd, -E2BIG, "prog should have been rejected with -E2BIG");
+
+cleanup_fds:
+	while (i > 0)
+		close(extra_fds[--i]);
+}
+
+void test_fd_array_cnt(void)
+{
+	if (test__start_subtest("no-fd-array"))
+		check_fd_array_cnt__no_fd_array();
+
+	if (test__start_subtest("fd-array-ok"))
+		check_fd_array_cnt__fd_array_ok();
+
+	if (test__start_subtest("fd-array-dup-input"))
+		check_fd_array_cnt__duplicated_maps();
+
+	if (test__start_subtest("fd-array-ref-maps-in-array"))
+		check_fd_array_cnt__referenced_maps_in_fd_array();
+
+	if (test__start_subtest("fd-array-trash-input"))
+		check_fd_array_cnt__fd_array_with_trash();
+
+	if (test__start_subtest("fd-array-with-holes"))
+		check_fd_array_cnt__fd_array_with_holes();
+
+	if (test__start_subtest("fd-array-2big"))
+		check_fd_array_cnt__fd_array_too_big();
+}
-- 
2.34.1

[PATCH bpf-next 5/5] bpf: fix potential error return

[Anton Protopopov]

The bpf_remove_insns() function returns WARN_ON_ONCE(error), where
error is a result of bpf_adj_branches(), and thus should be always 0
However, if for any reason it is not 0, then it will be converted to
boolean by WARN_ON_ONCE and returned to user space as 1, not an actual
error value. Fix this by returning the original err after the WARN check.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 14d9288441f2..a15059918768 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -539,6 +539,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 
 int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
 {
+	int err;
+
 	/* Branch offsets can't overflow when program is shrinking, no need
 	 * to call bpf_adj_branches(..., true) here
 	 */
@@ -546,7 +548,12 @@ int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
 		sizeof(struct bpf_insn) * (prog->len - off - cnt));
 	prog->len -= cnt;
 
-	return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
+	err = bpf_adj_branches(prog, off, off + cnt, off, false);
+	WARN_ON_ONCE(err);
+	if (err)
+		return err;
+
+	return 0;
 }
 
 static void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp)
-- 
2.34.1

Re: [PATCH bpf-next 0/5] Add fd_array_cnt attribute for BPF_PROG_LOAD

[Eduard Zingerman]

On Fri, 2024-11-15 at 00:46 +0000, Anton Protopopov wrote:
> Add a new attribute to the bpf(BPF_PROG_LOAD) system call. If this
> new attribute is non-zero, then the fd_array is considered to be a
> continuous array of the fd_array_cnt length and to contain only
> proper map file descriptors, or btf file descriptors, or zeroes.
> 
> This change allows maps, which aren't referenced directly by a BPF
> program, to be bound to the program _and_ also to be present during
> the program verification (so BPF_PROG_BIND_MAP is not enough for this
> use case).
> 
> The primary reason for this change is that it is a prerequisite for
> adding "instruction set" maps, which are both non-referenced by the
> program and must be present during the program verification.
> 
> The first three commits add the new functionality, the fourth adds
> corresponding self-tests, and the last one is a small additional fix.

When I apply this series on top of [1] (there is a small merge conflict),
I get an error message from KASAN, the message is at the end of this email.
Probably triggered by processing of preloaded BPF programs.
Also added a few nits for individual patches.

[1] fab974e64874 ("libbpf: Fix memory leak in bpf_program__attach_uprobe_multi")

---

[    1.107455] ------------[ cut here ]------------
[    1.107545] Trying to vfree() nonexistent vm area (000000003f161725)
[    1.107640] WARNING: CPU: 6 PID: 1 at mm/vmalloc.c:3345 vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
[    1.107731] Modules linked in:
[    1.107922] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-2.fc40 04/01/2014
[    1.108057] RIP: 0010:vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
[ 1.108123] Code: ea 03 42 80 3c 22 00 0f 85 2d 04 00 00 48 8b 38 48 85 ff 0f 85 76 ff ff ff 0f 0b 4c 89 e6 48 c7 c7 60 47 94 83 e8 5e b2 83 ff <0f> 0b 48 83 c4 60 5b 5d 41 5c 41 5d 41 5e 41 5f e9 34 f8 dd 01 89
All code
========
   0:	ea                   	(bad)
   1:	03 42 80             	add    -0x80(%rdx),%eax
   4:	3c 22                	cmp    $0x22,%al
   6:	00 0f                	add    %cl,(%rdi)
   8:	85 2d 04 00 00 48    	test   %ebp,0x48000004(%rip)        # 0x48000012
   e:	8b 38                	mov    (%rax),%edi
  10:	48 85 ff             	test   %rdi,%rdi
  13:	0f 85 76 ff ff ff    	jne    0xffffffffffffff8f
  19:	0f 0b                	ud2
  1b:	4c 89 e6             	mov    %r12,%rsi
  1e:	48 c7 c7 60 47 94 83 	mov    $0xffffffff83944760,%rdi
  25:	e8 5e b2 83 ff       	call   0xffffffffff83b288
  2a:*	0f 0b                	ud2		<-- trapping instruction
  2c:	48 83 c4 60          	add    $0x60,%rsp
  30:	5b                   	pop    %rbx
  31:	5d                   	pop    %rbp
  32:	41 5c                	pop    %r12
  34:	41 5d                	pop    %r13
  36:	41 5e                	pop    %r14
  38:	41 5f                	pop    %r15
  3a:	e9 34 f8 dd 01       	jmp    0x1ddf873
  3f:	89                   	.byte 0x89

Code starting with the faulting instruction
===========================================
   0:	0f 0b                	ud2
   2:	48 83 c4 60          	add    $0x60,%rsp
   6:	5b                   	pop    %rbx
   7:	5d                   	pop    %rbp
   8:	41 5c                	pop    %r12
   a:	41 5d                	pop    %r13
   c:	41 5e                	pop    %r14
   e:	41 5f                	pop    %r15
  10:	e9 34 f8 dd 01       	jmp    0x1ddf849
  15:	89                   	.byte 0x89
[    1.108379] RSP: 0018:ffff88810034f368 EFLAGS: 00010296
[    1.108459] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[    1.108576] RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000001
[    1.108682] RBP: 0000000000000000 R08: 0000000000000000 R09: fffffbfff08dfaa4
[    1.108791] R10: 0000000000000003 R11: ffffffff8475a8f0 R12: ffffc900001d6000
[    1.108896] R13: ffff888104e5064c R14: ffffc900001d49c0 R15: 0000000000000005
[    1.108999] FS:  0000000000000000(0000) GS:ffff88815b300000(0000) knlGS:0000000000000000
[    1.109104] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.109234] CR2: 0000000000000000 CR3: 0000000004698000 CR4: 0000000000750ef0
[    1.109352] PKRU: 55555554
[    1.109397] Call Trace:
[    1.109442]  <TASK>
[    1.109489] ? __warn.cold (kernel/panic.c:748) 
[    1.109564] ? vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
[    1.109623] ? report_bug (lib/bug.c:180 lib/bug.c:219) 
[    1.109710] ? handle_bug (arch/x86/kernel/traps.c:285) 
[    1.109775] ? exc_invalid_op (arch/x86/kernel/traps.c:309 (discriminator 1)) 
[    1.109838] ? asm_exc_invalid_op (./arch/x86/include/asm/idtentry.h:621) 
[    1.109914] ? vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
[    1.109982] ? vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
[    1.110047] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.110128] ? kfree (mm/slub.c:4579 (discriminator 3) mm/slub.c:4727 (discriminator 3)) 
[    1.110191] ? bpf_check (kernel/bpf/verifier.c:22799 (discriminator 1)) 
[    1.110252] ? bpf_check (kernel/bpf/verifier.c:22859) 
[    1.110317] bpf_check (kernel/bpf/verifier.c:22861) 
[    1.110382] ? kasan_save_stack (mm/kasan/common.c:49) 
[    1.110443] ? kasan_save_track (mm/kasan/common.c:60 (discriminator 1) mm/kasan/common.c:69 (discriminator 1)) 
[    1.110515] ? __pfx_bpf_check (kernel/bpf/verifier.c:22606) 
[    1.110612] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.110690] ? kasan_save_track (mm/kasan/common.c:60 (discriminator 1) mm/kasan/common.c:69 (discriminator 1)) 
[    1.110746] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.110820] ? __kasan_kmalloc (mm/kasan/common.c:377 mm/kasan/common.c:394) 
[    1.110885] ? bpf_prog_load (kernel/bpf/syscall.c:2947) 
[    1.110942] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.111015] bpf_prog_load (kernel/bpf/syscall.c:2947) 
[    1.111073] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.111163] ? __pfx_bpf_prog_load (kernel/bpf/syscall.c:2735) 
[    1.111240] ? lock_acquire (kernel/locking/lockdep.c:5798) 
[    1.111315] ? __pfx_bpf_check_uarg_tail_zero (kernel/bpf/syscall.c:87) 
[    1.111401] __sys_bpf (kernel/bpf/syscall.c:5759) 
[    1.111464] ? __pfx___sys_bpf (kernel/bpf/syscall.c:5721) 
[    1.111522] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.111610] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.111690] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.111766] ? kern_sys_bpf (kernel/bpf/syscall.c:5909) 
[    1.111837] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.111912] ? skel_map_update_elem.constprop.0 (./tools/lib/bpf/skel_internal.h:239) 
[    1.111989] ? __pfx_skel_map_update_elem.constprop.0 (./tools/lib/bpf/skel_internal.h:239) 
[    1.112089] kern_sys_bpf (kernel/bpf/syscall.c:5909) 
[    1.112156] ? __pfx_kern_sys_bpf (kernel/bpf/syscall.c:5909) 
[    1.112226] bpf_load_and_run.constprop.0 (./tools/lib/bpf/skel_internal.h:342) 
[    1.112303] ? __pfx_bpf_load_and_run.constprop.0 (./tools/lib/bpf/skel_internal.h:309) 
[    1.112402] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.112480] ? kasan_save_track (mm/kasan/common.c:60 (discriminator 1) mm/kasan/common.c:69 (discriminator 1)) 
[    1.112550] load (kernel/bpf/preload/bpf_preload_kern.c:46 kernel/bpf/preload/bpf_preload_kern.c:78) 
[    1.112614] ? __pfx_load (kernel/bpf/preload/bpf_preload_kern.c:75) 
[    1.112673] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
[    1.112750] ? _raw_spin_unlock_irqrestore (./include/linux/spinlock_api_smp.h:152 (discriminator 3) kernel/locking/spinlock.c:194 (discriminator 3)) 
[    1.112837] ? __pfx_crypto_kfunc_init (kernel/bpf/crypto.c:374) 
[    1.112920] ? __pfx_load (kernel/bpf/preload/bpf_preload_kern.c:75) 
[    1.112981] do_one_initcall (init/main.c:1269) 
[    1.113045] ? __pfx_do_one_initcall (init/main.c:1260) 
[    1.113131] ? __kmalloc_noprof (./include/trace/events/kmem.h:54 (discriminator 2) mm/slub.c:4265 (discriminator 2) mm/slub.c:4276 (discriminator 2)) 
[    1.113191] ? kernel_init_freeable (init/main.c:1341 init/main.c:1366 init/main.c:1580) 
[    1.113277] kernel_init_freeable (init/main.c:1330 (discriminator 3) init/main.c:1347 (discriminator 3) init/main.c:1366 (discriminator 3) init/main.c:1580 (discriminator 3)) 
[    1.113359] ? __pfx_kernel_init (init/main.c:1461) 
[    1.113426] kernel_init (init/main.c:1471) 
[    1.113486] ? __pfx_kernel_init (init/main.c:1461) 
[    1.113554] ret_from_fork (arch/x86/kernel/process.c:147) 
[    1.113616] ? __pfx_kernel_init (init/main.c:1461) 
[    1.113677] ret_from_fork_asm (arch/x86/entry/entry_64.S:257) 
[    1.113752]  </TASK>
[    1.113796] irq event stamp: 168993
[    1.113857] hardirqs last enabled at (169001): __up_console_sem (./arch/x86/include/asm/irqflags.h:42 ./arch/x86/include/asm/irqflags.h:97 ./arch/x86/include/asm/irqflags.h:155 kernel/printk/printk.c:344) 
[    1.113992] hardirqs last disabled at (169008): __up_console_sem (kernel/printk/printk.c:342 (discriminator 3)) 
[    1.114128] softirqs last enabled at (168746): irq_exit_rcu (kernel/softirq.c:589 kernel/softirq.c:428 kernel/softirq.c:637 kernel/softirq.c:649) 
[    1.114264] softirqs last disabled at (168741): irq_exit_rcu (kernel/softirq.c:589 kernel/softirq.c:428 kernel/softirq.c:637 kernel/softirq.c:649) 
[    1.114399] ---[ end trace 0000000000000000 ]---

Re: [PATCH bpf-next 3/5] bpf: add fd_array_cnt attribute for prog_load

[Eduard Zingerman]

On Fri, 2024-11-15 at 00:46 +0000, Anton Protopopov wrote:

[...]

> @@ -22537,6 +22543,76 @@ struct btf *bpf_get_btf_vmlinux(void)
>  	return btf_vmlinux;
>  }
>  
> +/*
> + * The add_fd_from_fd_array() is executed only if fd_array_cnt is given.  In
> + * this case expect that every file descriptor in the array is either a map or
> + * a BTF, or a hole (0). Everything else is considered to be trash.
> + */
> +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> +{
> +	struct bpf_map *map;
> +	CLASS(fd, f)(fd);
> +	int ret;
> +
> +	map = __bpf_map_get(f);
> +	if (IS_ERR(map)) {
> +		if (!IS_ERR(__btf_get_by_fd(f)))
> +			return 0;
> +
> +		/* allow holes */
> +		if (!fd)
> +			return 0;
> +
> +		verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> +		return PTR_ERR(map);
> +	}
> +
> +	ret = add_used_map(env, map);
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}

Nit: keeping this function "flat" would allow easier extension, if necessary.
     E.g.:

    static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
    {
    	struct bpf_map *map;
    	CLASS(fd, f)(fd);
    	int ret;

    	/* allow holes */
    	if (!fd) {
    		return 0;
    	}
    	map = __bpf_map_get(f);
    	if (!IS_ERR(map)) {
    		ret = add_used_map(env, map);
    		return ret < 0 ? ret : 0;
    	}
    	if (!IS_ERR(__btf_get_by_fd(f))) {
    		return 0;
    	}
    	verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
    	return -EINVAL;
    }


> +
> +static int env_init_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> +{
> +	int size = sizeof(int) * attr->fd_array_cnt;
> +	int *copy;
> +	int ret;
> +	int i;
> +
> +	if (attr->fd_array_cnt >= MAX_USED_MAPS)
> +		return -E2BIG;
> +
> +	env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> +
> +	/*
> +	 * The only difference between old (no fd_array_cnt is given) and new
> +	 * APIs is that in the latter case the fd_array is expected to be
> +	 * continuous and is scanned for map fds right away
> +	 */
> +	if (!size)
> +		return 0;
> +
> +	copy = kzalloc(size, GFP_KERNEL);
> +	if (!copy)
> +		return -ENOMEM;
> +
> +	if (copy_from_bpfptr_offset(copy, env->fd_array, 0, size)) {
> +		ret = -EFAULT;
> +		goto free_copy;
> +	}
> +
> +	for (i = 0; i < attr->fd_array_cnt; i++) {
> +		ret = add_fd_from_fd_array(env, copy[i]);
> +		if (ret)
> +			goto free_copy;
> +	}
> +
> +free_copy:
> +	kfree(copy);
> +	return ret;
> +}
> +
>  int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size)
>  {
>  	u64 start_time = ktime_get_ns();

[...]

Re: [PATCH bpf-next 4/5] selftests/bpf: Add tests for fd_array_cnt

[Eduard Zingerman]

On Fri, 2024-11-15 at 00:46 +0000, Anton Protopopov wrote:

[...]

> @@ -0,0 +1,374 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +
> +#include <linux/btf.h>
> +#include <sys/syscall.h>
> +#include <bpf/bpf.h>
> +
> +static inline int _bpf_map_create(void)
> +{
> +	static union bpf_attr attr = {
> +		.map_type = BPF_MAP_TYPE_ARRAY,
> +		.key_size = 4,
> +		.value_size = 8,
> +		.max_entries = 1,
> +	};
> +
> +	return syscall(__NR_bpf, BPF_MAP_CREATE, &attr, sizeof(attr));
> +}
> +
> +#define BTF_INFO_ENC(kind, kind_flag, vlen) \
> +	((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN))
> +#define BTF_TYPE_ENC(name, info, size_or_type) (name), (info), (size_or_type)
> +#define BTF_INT_ENC(encoding, bits_offset, nr_bits) \
> +	((encoding) << 24 | (bits_offset) << 16 | (nr_bits))
> +#define BTF_TYPE_INT_ENC(name, encoding, bits_offset, bits, sz) \
> +	BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_INT, 0, 0), sz), \
> +	BTF_INT_ENC(encoding, bits_offset, bits)

Nit: these macro are already defined in tools/testing/selftests/bpf/test_btf.h .

> +
> +static int _btf_create(void)
> +{
> +	struct btf_blob {
> +		struct btf_header btf_hdr;
> +		__u32 types[8];
> +		__u32 str;
> +	} raw_btf = {
> +		.btf_hdr = {
> +			.magic = BTF_MAGIC,
> +			.version = BTF_VERSION,
> +			.hdr_len = sizeof(struct btf_header),
> +			.type_len = sizeof(__u32) * 8,
> +			.str_off = sizeof(__u32) * 8,
> +			.str_len = sizeof(__u32),

Nit: offsetof(struct btf_blob, str), sizeof(raw_btf.str), sizeof(raw_btf.types)
     are legal in this position.

> +		},
> +		.types = {
> +			/* long */
> +			BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 64, 8),  /* [1] */
> +			/* unsigned long */
> +			BTF_TYPE_INT_ENC(0, 0, 0, 64, 8),  /* [2] */
> +		},
> +	};
> +	static union bpf_attr attr = {
> +		.btf_size = sizeof(raw_btf),
> +	};
> +
> +	attr.btf = (long)&raw_btf;
> +
> +	return syscall(__NR_bpf, BPF_BTF_LOAD, &attr, sizeof(attr));
> +}

[...]

> +static void check_fd_array_cnt__fd_array_ok(void)
> +{
> +	int extra_fds[128];
> +	__u32 map_ids[16];
> +	__u32 nr_map_ids;
> +	int prog_fd;
> +
> +	extra_fds[0] = _bpf_map_create();
> +	if (!ASSERT_GE(extra_fds[0], 0, "_bpf_map_create"))
> +		return;
> +	extra_fds[1] = _bpf_map_create();
> +	if (!ASSERT_GE(extra_fds[1], 0, "_bpf_map_create"))
> +		return;
> +	prog_fd = load_test_prog(extra_fds, 2);
> +	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> +		return;
> +	nr_map_ids = ARRAY_SIZE(map_ids);
> +	if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))

Nit: should probably close prog_fd and extra_fds (and in tests below).

> +		return;
> +
> +	/* maps should still exist when original file descriptors are closed */
> +	close(extra_fds[0]);
> +	close(extra_fds[1]);
> +	if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map_ids[0] should exist"))
> +		return;
> +	if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map_ids[1] should exist"))
> +		return;
> +
> +	close(prog_fd);
> +}

[...]

Re: [PATCH bpf-next 0/5] Add fd_array_cnt attribute for BPF_PROG_LOAD

[Anton Protopopov]

On 24/11/15 07:06PM, Eduard Zingerman wrote:
> On Fri, 2024-11-15 at 00:46 +0000, Anton Protopopov wrote:
> > Add a new attribute to the bpf(BPF_PROG_LOAD) system call. If this
> > new attribute is non-zero, then the fd_array is considered to be a
> > continuous array of the fd_array_cnt length and to contain only
> > proper map file descriptors, or btf file descriptors, or zeroes.
> > 
> > This change allows maps, which aren't referenced directly by a BPF
> > program, to be bound to the program _and_ also to be present during
> > the program verification (so BPF_PROG_BIND_MAP is not enough for this
> > use case).
> > 
> > The primary reason for this change is that it is a prerequisite for
> > adding "instruction set" maps, which are both non-referenced by the
> > program and must be present during the program verification.
> > 
> > The first three commits add the new functionality, the fourth adds
> > corresponding self-tests, and the last one is a small additional fix.
> 
> When I apply this series on top of [1] (there is a small merge conflict),
> I get an error message from KASAN, the message is at the end of this email.
> Probably triggered by processing of preloaded BPF programs.

Thanks for pointing to this warning. Unluckily, I can't reproduce it locally,
and neither I have a conflict (I've rebased my branch on top of the current
master, which contains [1]). Could you please tell me which environment you
were using to trigger it? Is this BPF CI?

> Also added a few nits for individual patches.

Thanks for looking! I will reply there.

> [1] fab974e64874 ("libbpf: Fix memory leak in bpf_program__attach_uprobe_multi")
> 
> ---
> 
> [    1.107455] ------------[ cut here ]------------
> [    1.107545] Trying to vfree() nonexistent vm area (000000003f161725)
> [    1.107640] WARNING: CPU: 6 PID: 1 at mm/vmalloc.c:3345 vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
> [    1.107731] Modules linked in:
> [    1.107922] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-2.fc40 04/01/2014
> [    1.108057] RIP: 0010:vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
> [ 1.108123] Code: ea 03 42 80 3c 22 00 0f 85 2d 04 00 00 48 8b 38 48 85 ff 0f 85 76 ff ff ff 0f 0b 4c 89 e6 48 c7 c7 60 47 94 83 e8 5e b2 83 ff <0f> 0b 48 83 c4 60 5b 5d 41 5c 41 5d 41 5e 41 5f e9 34 f8 dd 01 89
> All code
> ========
>    0:	ea                   	(bad)
>    1:	03 42 80             	add    -0x80(%rdx),%eax
>    4:	3c 22                	cmp    $0x22,%al
>    6:	00 0f                	add    %cl,(%rdi)
>    8:	85 2d 04 00 00 48    	test   %ebp,0x48000004(%rip)        # 0x48000012
>    e:	8b 38                	mov    (%rax),%edi
>   10:	48 85 ff             	test   %rdi,%rdi
>   13:	0f 85 76 ff ff ff    	jne    0xffffffffffffff8f
>   19:	0f 0b                	ud2
>   1b:	4c 89 e6             	mov    %r12,%rsi
>   1e:	48 c7 c7 60 47 94 83 	mov    $0xffffffff83944760,%rdi
>   25:	e8 5e b2 83 ff       	call   0xffffffffff83b288
>   2a:*	0f 0b                	ud2		<-- trapping instruction
>   2c:	48 83 c4 60          	add    $0x60,%rsp
>   30:	5b                   	pop    %rbx
>   31:	5d                   	pop    %rbp
>   32:	41 5c                	pop    %r12
>   34:	41 5d                	pop    %r13
>   36:	41 5e                	pop    %r14
>   38:	41 5f                	pop    %r15
>   3a:	e9 34 f8 dd 01       	jmp    0x1ddf873
>   3f:	89                   	.byte 0x89
> 
> Code starting with the faulting instruction
> ===========================================
>    0:	0f 0b                	ud2
>    2:	48 83 c4 60          	add    $0x60,%rsp
>    6:	5b                   	pop    %rbx
>    7:	5d                   	pop    %rbp
>    8:	41 5c                	pop    %r12
>    a:	41 5d                	pop    %r13
>    c:	41 5e                	pop    %r14
>    e:	41 5f                	pop    %r15
>   10:	e9 34 f8 dd 01       	jmp    0x1ddf849
>   15:	89                   	.byte 0x89
> [    1.108379] RSP: 0018:ffff88810034f368 EFLAGS: 00010296
> [    1.108459] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [    1.108576] RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000001
> [    1.108682] RBP: 0000000000000000 R08: 0000000000000000 R09: fffffbfff08dfaa4
> [    1.108791] R10: 0000000000000003 R11: ffffffff8475a8f0 R12: ffffc900001d6000
> [    1.108896] R13: ffff888104e5064c R14: ffffc900001d49c0 R15: 0000000000000005
> [    1.108999] FS:  0000000000000000(0000) GS:ffff88815b300000(0000) knlGS:0000000000000000
> [    1.109104] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    1.109234] CR2: 0000000000000000 CR3: 0000000004698000 CR4: 0000000000750ef0
> [    1.109352] PKRU: 55555554
> [    1.109397] Call Trace:
> [    1.109442]  <TASK>
> [    1.109489] ? __warn.cold (kernel/panic.c:748) 
> [    1.109564] ? vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
> [    1.109623] ? report_bug (lib/bug.c:180 lib/bug.c:219) 
> [    1.109710] ? handle_bug (arch/x86/kernel/traps.c:285) 
> [    1.109775] ? exc_invalid_op (arch/x86/kernel/traps.c:309 (discriminator 1)) 
> [    1.109838] ? asm_exc_invalid_op (./arch/x86/include/asm/idtentry.h:621) 
> [    1.109914] ? vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
> [    1.109982] ? vfree (mm/vmalloc.c:3345 (discriminator 1) mm/vmalloc.c:3326 (discriminator 1)) 
> [    1.110047] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.110128] ? kfree (mm/slub.c:4579 (discriminator 3) mm/slub.c:4727 (discriminator 3)) 
> [    1.110191] ? bpf_check (kernel/bpf/verifier.c:22799 (discriminator 1)) 
> [    1.110252] ? bpf_check (kernel/bpf/verifier.c:22859) 
> [    1.110317] bpf_check (kernel/bpf/verifier.c:22861) 
> [    1.110382] ? kasan_save_stack (mm/kasan/common.c:49) 
> [    1.110443] ? kasan_save_track (mm/kasan/common.c:60 (discriminator 1) mm/kasan/common.c:69 (discriminator 1)) 
> [    1.110515] ? __pfx_bpf_check (kernel/bpf/verifier.c:22606) 
> [    1.110612] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.110690] ? kasan_save_track (mm/kasan/common.c:60 (discriminator 1) mm/kasan/common.c:69 (discriminator 1)) 
> [    1.110746] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.110820] ? __kasan_kmalloc (mm/kasan/common.c:377 mm/kasan/common.c:394) 
> [    1.110885] ? bpf_prog_load (kernel/bpf/syscall.c:2947) 
> [    1.110942] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.111015] bpf_prog_load (kernel/bpf/syscall.c:2947) 
> [    1.111073] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.111163] ? __pfx_bpf_prog_load (kernel/bpf/syscall.c:2735) 
> [    1.111240] ? lock_acquire (kernel/locking/lockdep.c:5798) 
> [    1.111315] ? __pfx_bpf_check_uarg_tail_zero (kernel/bpf/syscall.c:87) 
> [    1.111401] __sys_bpf (kernel/bpf/syscall.c:5759) 
> [    1.111464] ? __pfx___sys_bpf (kernel/bpf/syscall.c:5721) 
> [    1.111522] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.111610] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.111690] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.111766] ? kern_sys_bpf (kernel/bpf/syscall.c:5909) 
> [    1.111837] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.111912] ? skel_map_update_elem.constprop.0 (./tools/lib/bpf/skel_internal.h:239) 
> [    1.111989] ? __pfx_skel_map_update_elem.constprop.0 (./tools/lib/bpf/skel_internal.h:239) 
> [    1.112089] kern_sys_bpf (kernel/bpf/syscall.c:5909) 
> [    1.112156] ? __pfx_kern_sys_bpf (kernel/bpf/syscall.c:5909) 
> [    1.112226] bpf_load_and_run.constprop.0 (./tools/lib/bpf/skel_internal.h:342) 
> [    1.112303] ? __pfx_bpf_load_and_run.constprop.0 (./tools/lib/bpf/skel_internal.h:309) 
> [    1.112402] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.112480] ? kasan_save_track (mm/kasan/common.c:60 (discriminator 1) mm/kasan/common.c:69 (discriminator 1)) 
> [    1.112550] load (kernel/bpf/preload/bpf_preload_kern.c:46 kernel/bpf/preload/bpf_preload_kern.c:78) 
> [    1.112614] ? __pfx_load (kernel/bpf/preload/bpf_preload_kern.c:75) 
> [    1.112673] ? srso_alias_return_thunk (arch/x86/lib/retpoline.S:182) 
> [    1.112750] ? _raw_spin_unlock_irqrestore (./include/linux/spinlock_api_smp.h:152 (discriminator 3) kernel/locking/spinlock.c:194 (discriminator 3)) 
> [    1.112837] ? __pfx_crypto_kfunc_init (kernel/bpf/crypto.c:374) 
> [    1.112920] ? __pfx_load (kernel/bpf/preload/bpf_preload_kern.c:75) 
> [    1.112981] do_one_initcall (init/main.c:1269) 
> [    1.113045] ? __pfx_do_one_initcall (init/main.c:1260) 
> [    1.113131] ? __kmalloc_noprof (./include/trace/events/kmem.h:54 (discriminator 2) mm/slub.c:4265 (discriminator 2) mm/slub.c:4276 (discriminator 2)) 
> [    1.113191] ? kernel_init_freeable (init/main.c:1341 init/main.c:1366 init/main.c:1580) 
> [    1.113277] kernel_init_freeable (init/main.c:1330 (discriminator 3) init/main.c:1347 (discriminator 3) init/main.c:1366 (discriminator 3) init/main.c:1580 (discriminator 3)) 
> [    1.113359] ? __pfx_kernel_init (init/main.c:1461) 
> [    1.113426] kernel_init (init/main.c:1471) 
> [    1.113486] ? __pfx_kernel_init (init/main.c:1461) 
> [    1.113554] ret_from_fork (arch/x86/kernel/process.c:147) 
> [    1.113616] ? __pfx_kernel_init (init/main.c:1461) 
> [    1.113677] ret_from_fork_asm (arch/x86/entry/entry_64.S:257) 
> [    1.113752]  </TASK>
> [    1.113796] irq event stamp: 168993
> [    1.113857] hardirqs last enabled at (169001): __up_console_sem (./arch/x86/include/asm/irqflags.h:42 ./arch/x86/include/asm/irqflags.h:97 ./arch/x86/include/asm/irqflags.h:155 kernel/printk/printk.c:344) 
> [    1.113992] hardirqs last disabled at (169008): __up_console_sem (kernel/printk/printk.c:342 (discriminator 3)) 
> [    1.114128] softirqs last enabled at (168746): irq_exit_rcu (kernel/softirq.c:589 kernel/softirq.c:428 kernel/softirq.c:637 kernel/softirq.c:649) 
> [    1.114264] softirqs last disabled at (168741): irq_exit_rcu (kernel/softirq.c:589 kernel/softirq.c:428 kernel/softirq.c:637 kernel/softirq.c:649) 
> [    1.114399] ---[ end trace 0000000000000000 ]---
> 
> 

Re: [PATCH bpf-next 3/5] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov]

On 24/11/15 07:06PM, Eduard Zingerman wrote:
> On Fri, 2024-11-15 at 00:46 +0000, Anton Protopopov wrote:
> 
> [...]
> 
> > @@ -22537,6 +22543,76 @@ struct btf *bpf_get_btf_vmlinux(void)
> >  	return btf_vmlinux;
> >  }
> >  
> > +/*
> > + * The add_fd_from_fd_array() is executed only if fd_array_cnt is given.  In
> > + * this case expect that every file descriptor in the array is either a map or
> > + * a BTF, or a hole (0). Everything else is considered to be trash.
> > + */
> > +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> > +{
> > +	struct bpf_map *map;
> > +	CLASS(fd, f)(fd);
> > +	int ret;
> > +
> > +	map = __bpf_map_get(f);
> > +	if (IS_ERR(map)) {
> > +		if (!IS_ERR(__btf_get_by_fd(f)))
> > +			return 0;
> > +
> > +		/* allow holes */
> > +		if (!fd)
> > +			return 0;
> > +
> > +		verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> > +		return PTR_ERR(map);
> > +	}
> > +
> > +	ret = add_used_map(env, map);
> > +	if (ret < 0)
> > +		return ret;
> > +	return 0;
> > +}
> 
> Nit: keeping this function "flat" would allow easier extension, if necessary.
>      E.g.:
> 
>     static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
>     {
>     	struct bpf_map *map;
>     	CLASS(fd, f)(fd);
>     	int ret;
> 
>     	/* allow holes */
>     	if (!fd) {
>     		return 0;
>     	}
>     	map = __bpf_map_get(f);
>     	if (!IS_ERR(map)) {
>     		ret = add_used_map(env, map);
>     		return ret < 0 ? ret : 0;
>     	}
>     	if (!IS_ERR(__btf_get_by_fd(f))) {
>     		return 0;
>     	}
>     	verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
>     	return -EINVAL;
>     }

Thanks, this makes sense, I will change it to a flat version in v2.

> [...]

Re: [PATCH bpf-next 4/5] selftests/bpf: Add tests for fd_array_cnt

[Anton Protopopov]

On 24/11/15 07:06PM, Eduard Zingerman wrote:
> On Fri, 2024-11-15 at 00:46 +0000, Anton Protopopov wrote:
> 
> [...]
> 
> > @@ -0,0 +1,374 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <test_progs.h>
> > +
> > +#include <linux/btf.h>
> > +#include <sys/syscall.h>
> > +#include <bpf/bpf.h>
> > +
> > +static inline int _bpf_map_create(void)
> > +{
> > +	static union bpf_attr attr = {
> > +		.map_type = BPF_MAP_TYPE_ARRAY,
> > +		.key_size = 4,
> > +		.value_size = 8,
> > +		.max_entries = 1,
> > +	};
> > +
> > +	return syscall(__NR_bpf, BPF_MAP_CREATE, &attr, sizeof(attr));
> > +}
> > +
> > +#define BTF_INFO_ENC(kind, kind_flag, vlen) \
> > +	((!!(kind_flag) << 31) | ((kind) << 24) | ((vlen) & BTF_MAX_VLEN))
> > +#define BTF_TYPE_ENC(name, info, size_or_type) (name), (info), (size_or_type)
> > +#define BTF_INT_ENC(encoding, bits_offset, nr_bits) \
> > +	((encoding) << 24 | (bits_offset) << 16 | (nr_bits))
> > +#define BTF_TYPE_INT_ENC(name, encoding, bits_offset, bits, sz) \
> > +	BTF_TYPE_ENC(name, BTF_INFO_ENC(BTF_KIND_INT, 0, 0), sz), \
> > +	BTF_INT_ENC(encoding, bits_offset, bits)
> 
> Nit: these macro are already defined in tools/testing/selftests/bpf/test_btf.h .

Ok, right, I will move them to some common place in v2.

> > +
> > +static int _btf_create(void)
> > +{
> > +	struct btf_blob {
> > +		struct btf_header btf_hdr;
> > +		__u32 types[8];
> > +		__u32 str;
> > +	} raw_btf = {
> > +		.btf_hdr = {
> > +			.magic = BTF_MAGIC,
> > +			.version = BTF_VERSION,
> > +			.hdr_len = sizeof(struct btf_header),
> > +			.type_len = sizeof(__u32) * 8,
> > +			.str_off = sizeof(__u32) * 8,
> > +			.str_len = sizeof(__u32),
> 
> Nit: offsetof(struct btf_blob, str), sizeof(raw_btf.str), sizeof(raw_btf.types)
>      are legal in this position.

Ok, thanks. (I've copy-pasted this part from some other test, will
change the similar code at the source as well then.)

> > +		},
> > +		.types = {
> > +			/* long */
> > +			BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 64, 8),  /* [1] */
> > +			/* unsigned long */
> > +			BTF_TYPE_INT_ENC(0, 0, 0, 64, 8),  /* [2] */
> > +		},
> > +	};
> > +	static union bpf_attr attr = {
> > +		.btf_size = sizeof(raw_btf),
> > +	};
> > +
> > +	attr.btf = (long)&raw_btf;
> > +
> > +	return syscall(__NR_bpf, BPF_BTF_LOAD, &attr, sizeof(attr));
> > +}
> 
> [...]
> 
> > +static void check_fd_array_cnt__fd_array_ok(void)
> > +{
> > +	int extra_fds[128];
> > +	__u32 map_ids[16];
> > +	__u32 nr_map_ids;
> > +	int prog_fd;
> > +
> > +	extra_fds[0] = _bpf_map_create();
> > +	if (!ASSERT_GE(extra_fds[0], 0, "_bpf_map_create"))
> > +		return;
> > +	extra_fds[1] = _bpf_map_create();
> > +	if (!ASSERT_GE(extra_fds[1], 0, "_bpf_map_create"))
> > +		return;
> > +	prog_fd = load_test_prog(extra_fds, 2);
> > +	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> > +		return;
> > +	nr_map_ids = ARRAY_SIZE(map_ids);
> > +	if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
> 
> Nit: should probably close prog_fd and extra_fds (and in tests below).

Ah, thanks! I will also check the other tests in this file for the
same bugs.

> 
> > +		return;
> > +
> > +	/* maps should still exist when original file descriptors are closed */
> > +	close(extra_fds[0]);
> > +	close(extra_fds[1]);
> > +	if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map_ids[0] should exist"))
> > +		return;
> > +	if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map_ids[1] should exist"))
> > +		return;
> > +
> > +	close(prog_fd);
> > +}
> 
> [...]
>