[PATCH v2 bpf-next 0/5] Add bpf_getxattr

[PATCH v2 bpf-next 0/5] Add bpf_getxattr

[KP Singh]

v1 -> v2

- Used kfuncs as suggested by Alexei
- Used Benjamin Tissoires' patch from the HID v4 series to add a
  sleepable kfunc set (I sent the patch as a part of this series as it
  seems to have been dropped from v5) and acked it. Hope this is okay.
- Added support for verifying string constants to kfuncs

Foundation for building more complex security policies using the
BPF LSM as presented in LSF/MM/BPF:

 http://vger.kernel.org/bpfconf2022_material/lsfmmbpf2022-xattr.pdf

Benjamin Tissoires (1):
  btf: Add a new kfunc set which allows to mark a function to be
    sleepable

KP Singh (4):
  bpf: kfunc support for ARG_PTR_TO_CONST_STR
  bpf: Allow kfuncs to be used in LSM programs
  bpf: Add a bpf_getxattr kfunc
  bpf/selftests: Add a selftest for bpf_getxattr

 include/linux/bpf_verifier.h                  |  2 +
 include/linux/btf.h                           |  2 +
 kernel/bpf/btf.c                              | 42 ++++++++-
 kernel/bpf/verifier.c                         | 85 +++++++++++--------
 kernel/trace/bpf_trace.c                      | 36 ++++++++
 .../testing/selftests/bpf/prog_tests/xattr.c  | 58 +++++++++++++
 tools/testing/selftests/bpf/progs/xattr.c     | 37 ++++++++
 7 files changed, 223 insertions(+), 39 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xattr.c
 create mode 100644 tools/testing/selftests/bpf/progs/xattr.c

-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v2 bpf-next 1/5] btf: Add a new kfunc set which allows to mark a function to be sleepable

[KP Singh]

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>

This allows to declare a kfunc as sleepable and prevents its use in
a non sleepable program.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Acked-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/btf.h |  2 ++
 kernel/bpf/btf.c    | 12 ++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 1bfed7fa0428..6e7517573d9e 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -18,6 +18,7 @@ enum btf_kfunc_type {
 	BTF_KFUNC_TYPE_RELEASE,
 	BTF_KFUNC_TYPE_RET_NULL,
 	BTF_KFUNC_TYPE_KPTR_ACQUIRE,
+	BTF_KFUNC_TYPE_SLEEPABLE,
 	BTF_KFUNC_TYPE_MAX,
 };
 
@@ -37,6 +38,7 @@ struct btf_kfunc_id_set {
 			struct btf_id_set *release_set;
 			struct btf_id_set *ret_null_set;
 			struct btf_id_set *kptr_acquire_set;
+			struct btf_id_set *sleepable_set;
 		};
 		struct btf_id_set *sets[BTF_KFUNC_TYPE_MAX];
 	};
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f08037c31dd7..668ecf61649b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6171,7 +6171,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 	struct bpf_verifier_log *log = &env->log;
 	u32 i, nargs, ref_id, ref_obj_id = 0;
 	bool is_kfunc = btf_is_kernel(btf);
-	bool rel = false, kptr_get = false;
+	bool rel = false, kptr_get = false, sleepable = false;
 	const char *func_name, *ref_tname;
 	const struct btf_type *t, *ref_t;
 	const struct btf_param *args;
@@ -6202,9 +6202,10 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 	}
 
 	if (is_kfunc) {
-		/* Only kfunc can be release func */
 		rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
 						BTF_KFUNC_TYPE_RELEASE, func_id);
+		sleepable = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
+						      BTF_KFUNC_TYPE_SLEEPABLE, func_id);
 		kptr_get = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
 						     BTF_KFUNC_TYPE_KPTR_ACQUIRE, func_id);
 	}
@@ -6404,6 +6405,13 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			func_name);
 		return -EINVAL;
 	}
+
+	if (sleepable && !env->prog->aux->sleepable) {
+		bpf_log(log, "kernel function %s is sleepable but the program is not\n",
+			func_name);
+		return -EINVAL;
+	}
+
 	/* returns argument register number > 0 in case of reference release kfunc */
 	return rel ? ref_regno : 0;
 }
-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v2 bpf-next 2/5] bpf: kfunc support for ARG_PTR_TO_CONST_STR

[KP Singh]

kfuncs can handle pointers to memory when the next argument is
the size of the memory that can be read and verify these as
ARG_CONST_SIZE_OR_ZERO

Similarly add support for string constants (const char *) and
verify it similar to ARG_PTR_TO_CONST_STR.

Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/bpf_verifier.h |  2 +
 kernel/bpf/btf.c             | 29 ++++++++++++
 kernel/bpf/verifier.c        | 85 ++++++++++++++++++++----------------
 3 files changed, 79 insertions(+), 37 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 3930c963fa67..60d490354397 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -548,6 +548,8 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state
 			     u32 regno);
 int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 		   u32 regno, u32 mem_size);
+int check_const_str(struct bpf_verifier_env *env,
+		    const struct bpf_reg_state *reg, int regno);
 
 /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
 static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 668ecf61649b..02d7951591ae 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6162,6 +6162,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
 	return true;
 }
 
+static bool btf_param_is_const_str_ptr(const struct btf *btf,
+				       const struct btf_param *param)
+{
+	const struct btf_type *t;
+
+	t = btf_type_by_id(btf, param->type);
+	if (!btf_type_is_ptr(t))
+		return false;
+
+	t = btf_type_by_id(btf, t->type);
+	if (!(BTF_INFO_KIND(t->info) == BTF_KIND_CONST))
+		return false;
+
+	t = btf_type_skip_modifiers(btf, t->type, NULL);
+	if (!strcmp(btf_name_by_offset(btf, t->name_off), "char"))
+		return true;
+
+	return false;
+}
+
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    const struct btf *btf, u32 func_id,
 				    struct bpf_reg_state *regs,
@@ -6344,6 +6364,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		} else if (ptr_to_mem_ok) {
 			const struct btf_type *resolve_ret;
 			u32 type_size;
+			int err;
 
 			if (is_kfunc) {
 				bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
@@ -6354,6 +6375,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				 * When arg_mem_size is true, the pointer can be
 				 * void *.
 				 */
+				if (btf_param_is_const_str_ptr(btf, &args[i])) {
+					err = check_const_str(env, reg, regno);
+					if (err < 0)
+						return err;
+					i++;
+					continue;
+				}
+
 				if (!btf_type_is_scalar(ref_t) &&
 				    !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
 				    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2859901ffbe3..14a434792d7b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5840,6 +5840,52 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
 	return state->stack[spi].spilled_ptr.id;
 }
 
+int check_const_str(struct bpf_verifier_env *env,
+		    const struct bpf_reg_state *reg, int regno)
+{
+	struct bpf_map *map = reg->map_ptr;
+	int map_off;
+	u64 map_addr;
+	char *str_ptr;
+	int err;
+
+	if (!bpf_map_is_rdonly(map)) {
+		verbose(env, "R%d does not point to a readonly map'\n", regno);
+		return -EACCES;
+	}
+
+	if (!tnum_is_const(reg->var_off)) {
+		verbose(env, "R%d is not a constant address'\n", regno);
+		return -EACCES;
+	}
+
+	if (!map->ops->map_direct_value_addr) {
+		verbose(env,
+			"no direct value access support for this map type\n");
+		return -EACCES;
+	}
+
+	err = check_map_access(env, regno, reg->off, map->value_size - reg->off,
+			       false, ACCESS_HELPER);
+	if (err)
+		return err;
+
+	map_off = reg->off + reg->var_off.value;
+	err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
+	if (err) {
+		verbose(env, "direct value access on string failed\n");
+		return err;
+	}
+
+	str_ptr = (char *)(long)(map_addr);
+	if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) {
+		verbose(env, "string is not zero-terminated\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			  struct bpf_call_arg_meta *meta,
 			  const struct bpf_func_proto *fn)
@@ -6074,44 +6120,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			return err;
 		err = check_ptr_alignment(env, reg, 0, size, true);
 	} else if (arg_type == ARG_PTR_TO_CONST_STR) {
-		struct bpf_map *map = reg->map_ptr;
-		int map_off;
-		u64 map_addr;
-		char *str_ptr;
-
-		if (!bpf_map_is_rdonly(map)) {
-			verbose(env, "R%d does not point to a readonly map'\n", regno);
-			return -EACCES;
-		}
-
-		if (!tnum_is_const(reg->var_off)) {
-			verbose(env, "R%d is not a constant address'\n", regno);
-			return -EACCES;
-		}
-
-		if (!map->ops->map_direct_value_addr) {
-			verbose(env, "no direct value access support for this map type\n");
-			return -EACCES;
-		}
-
-		err = check_map_access(env, regno, reg->off,
-				       map->value_size - reg->off, false,
-				       ACCESS_HELPER);
-		if (err)
-			return err;
-
-		map_off = reg->off + reg->var_off.value;
-		err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
-		if (err) {
-			verbose(env, "direct value access on string failed\n");
+		err = check_const_str(env, reg, regno);
+		if (err < 0)
 			return err;
-		}
-
-		str_ptr = (char *)(long)(map_addr);
-		if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) {
-			verbose(env, "string is not zero-terminated\n");
-			return -EINVAL;
-		}
 	} else if (arg_type == ARG_PTR_TO_KPTR) {
 		if (process_kptr_func(env, regno, meta))
 			return -EACCES;
-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v2 bpf-next 3/5] bpf: Allow kfuncs to be used in LSM programs

[KP Singh]

In preparation for the addition of bpf_getxattr kfunc.

Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 kernel/bpf/btf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 02d7951591ae..541cf4635aa1 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7264,6 +7264,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
 	case BPF_PROG_TYPE_STRUCT_OPS:
 		return BTF_KFUNC_HOOK_STRUCT_OPS;
 	case BPF_PROG_TYPE_TRACING:
+	case BPF_PROG_TYPE_LSM:
 		return BTF_KFUNC_HOOK_TRACING;
 	case BPF_PROG_TYPE_SYSCALL:
 		return BTF_KFUNC_HOOK_SYSCALL;
-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v2 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

[KP Singh]

LSMs like SELinux store security state in xattrs. bpf_getxattr enables
BPF LSM to implement similar functionality. In combination with
bpf_local_storage, xattrs can be used to develop more complex security
policies.

This kfunc wraps around __vfs_getxattr which can sleep and is,
therefore, limited to sleepable programs using the newly added
sleepable_set for kfuncs.

Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 kernel/trace/bpf_trace.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 4be976cf7d63..b5682d55ebde 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -20,6 +20,7 @@
 #include <linux/fprobe.h>
 #include <linux/bsearch.h>
 #include <linux/sort.h>
+#include <linux/xattr.h>
 
 #include <net/bpf_sk_storage.h>
 
@@ -1181,6 +1182,41 @@ static const struct bpf_func_proto bpf_get_func_arg_cnt_proto = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
+				     const char *name, void *value, int size)
+{
+	return __vfs_getxattr(dentry, inode, name, value, size);
+}
+
+BTF_SET_START(bpf_trace_kfunc_ids)
+BTF_ID(func, bpf_getxattr)
+BTF_SET_END(bpf_trace_kfunc_ids)
+
+BTF_SET_START(bpf_trace_sleepable_kfunc_ids)
+BTF_ID(func, bpf_getxattr)
+BTF_SET_END(bpf_trace_sleepable_kfunc_ids)
+
+static const struct btf_kfunc_id_set bpf_trace_kfunc_set = {
+	.owner = THIS_MODULE,
+	.check_set = &bpf_trace_kfunc_ids,
+	.sleepable_set = &bpf_trace_sleepable_kfunc_ids,
+};
+
+static int __init bpf_trace_kfunc_init(void)
+{
+	int ret;
+
+	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
+					&bpf_trace_kfunc_set);
+	if (!ret)
+		return ret;
+
+	return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM,
+					&bpf_trace_kfunc_set);
+
+}
+late_initcall(bpf_trace_kfunc_init);
+
 static const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
-- 
2.37.0.rc0.104.g0611611a94-goog

[PATCH v2 bpf-next 5/5] bpf/selftests: Add a selftest for bpf_getxattr

[KP Singh]

A simple test that adds an xattr on a copied /bin/ls and reads it back
when the copied ls is executed.

Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/xattr.c  | 58 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/xattr.c     | 37 ++++++++++++
 2 files changed, 95 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xattr.c
 create mode 100644 tools/testing/selftests/bpf/progs/xattr.c

diff --git a/tools/testing/selftests/bpf/prog_tests/xattr.c b/tools/testing/selftests/bpf/prog_tests/xattr.c
new file mode 100644
index 000000000000..442b6c1aed0e
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/xattr.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2022 Google LLC.
+ */
+
+#include <test_progs.h>
+#include <sys/xattr.h>
+#include "xattr.skel.h"
+
+#define XATTR_NAME "security.bpf"
+#define XATTR_VALUE "test_progs"
+
+static unsigned int duration;
+
+void test_xattr(void)
+{
+	struct xattr *skel = NULL;
+	char tmp_dir_path[] = "/tmp/xattrXXXXXX";
+	char tmp_exec_path[64];
+	char cmd[256];
+	int err;
+
+	if (CHECK(!mkdtemp(tmp_dir_path), "mkdtemp",
+		  "unable to create tmpdir: %d\n", errno))
+		goto close_prog;
+
+	snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_ls",
+		 tmp_dir_path);
+	snprintf(cmd, sizeof(cmd), "cp /bin/ls %s", tmp_exec_path);
+	if (CHECK_FAIL(system(cmd)))
+		goto close_prog_rmdir;
+
+	if (CHECK(setxattr(tmp_exec_path, XATTR_NAME, XATTR_VALUE,
+			   sizeof(XATTR_VALUE), 0),
+		  "setxattr", "unable to setxattr: %d", errno))
+		goto close_prog_rmdir;
+
+	skel = xattr__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_load"))
+		goto close_prog_rmdir;
+
+	err = xattr__attach(skel);
+	if (!ASSERT_OK(err, "xattr__attach failed"))
+		goto close_prog_rmdir;
+
+	snprintf(cmd, sizeof(cmd), "%s -l", tmp_exec_path);
+	if (CHECK_FAIL(system(cmd)))
+		goto close_prog_rmdir;
+
+	ASSERT_EQ(skel->bss->result, 1, "xattr result");
+
+close_prog_rmdir:
+	snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp_dir_path);
+	system(cmd);
+close_prog:
+	xattr__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/xattr.c b/tools/testing/selftests/bpf/progs/xattr.c
new file mode 100644
index 000000000000..ccc078fb8ebd
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/xattr.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2022 Google LLC.
+ */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+#define XATTR_NAME "security.bpf"
+#define XATTR_VALUE "test_progs"
+
+__u64 result = 0;
+
+extern ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
+			    const char *name, void *value, int size) __ksym;
+
+SEC("lsm.s/bprm_committed_creds")
+void BPF_PROG(bprm_cc, struct linux_binprm *bprm)
+{
+	struct task_struct *current = bpf_get_current_task_btf();
+	char dir_xattr_value[64] = {0};
+	int xattr_sz = 0;
+
+	xattr_sz = bpf_getxattr(bprm->file->f_path.dentry,
+				bprm->file->f_path.dentry->d_inode, XATTR_NAME,
+				dir_xattr_value, 64);
+
+	if (xattr_sz <= 0)
+		return;
+
+	if (!bpf_strncmp(dir_xattr_value, sizeof(XATTR_VALUE), XATTR_VALUE))
+		result = 1;
+}
-- 
2.37.0.rc0.104.g0611611a94-goog

Re: [PATCH v2 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

[kernel test robot]

Hi KP,

I love your patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/KP-Singh/Add-bpf_getxattr/20220621-093013
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20220621/202206211053.VsuVPf7q-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/dd49d2ffb18adceafa98bd517008f59aa9bc910b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review KP-Singh/Add-bpf_getxattr/20220621-093013
        git checkout dd49d2ffb18adceafa98bd517008f59aa9bc910b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash kernel/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> kernel/trace/bpf_trace.c:1185:25: warning: no previous prototype for 'bpf_getxattr' [-Wmissing-prototypes]
    1185 | noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
         |                         ^~~~~~~~~~~~


vim +/bpf_getxattr +1185 kernel/trace/bpf_trace.c

  1184	
> 1185	noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
  1186					     const char *name, void *value, int size)
  1187	{
  1188		return __vfs_getxattr(dentry, inode, name, value, size);
  1189	}
  1190	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

[kernel test robot]

Hi KP,

I love your patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/KP-Singh/Add-bpf_getxattr/20220621-093013
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a015-20220620 (https://download.01.org/0day-ci/archive/20220621/202206211035.p3LxbVfK-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project af6d2a0b6825e71965f3e2701a63c239fa0ad70f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/dd49d2ffb18adceafa98bd517008f59aa9bc910b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review KP-Singh/Add-bpf_getxattr/20220621-093013
        git checkout dd49d2ffb18adceafa98bd517008f59aa9bc910b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> kernel/trace/bpf_trace.c:1185:25: warning: no previous prototype for function 'bpf_getxattr' [-Wmissing-prototypes]
   noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
                           ^
   kernel/trace/bpf_trace.c:1185:17: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
                   ^
                   static 
   1 warning generated.


vim +/bpf_getxattr +1185 kernel/trace/bpf_trace.c

  1184	
> 1185	noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
  1186					     const char *name, void *value, int size)
  1187	{
  1188		return __vfs_getxattr(dentry, inode, name, value, size);
  1189	}
  1190	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

[KP Singh]

On Tue, Jun 21, 2022 at 5:20 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi KP,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on bpf-next/master]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/KP-Singh/Add-bpf_getxattr/20220621-093013
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> config: x86_64-randconfig-a015-20220620 (https://download.01.org/0day-ci/archive/20220621/202206211035.p3LxbVfK-lkp@intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project af6d2a0b6825e71965f3e2701a63c239fa0ad70f)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/dd49d2ffb18adceafa98bd517008f59aa9bc910b
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review KP-Singh/Add-bpf_getxattr/20220621-093013
>         git checkout dd49d2ffb18adceafa98bd517008f59aa9bc910b
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> >> kernel/trace/bpf_trace.c:1185:25: warning: no previous prototype for function 'bpf_getxattr' [-Wmissing-prototypes]
>    noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
>                            ^
>    kernel/trace/bpf_trace.c:1185:17: note: declare 'static' if the function is not intended to be used outside of this translation unit
>    noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
>                    ^
>                    static
>    1 warning generated.
>

So it looks like this needs a function prototype. Let's do an initial round
of reviews on this series and I can respin with something like:

diff --git a/kernel/trace/bpf_trace.h b/kernel/trace/bpf_trace.h
index 9acbc11ac7bb..3f62e5d35037 100644
--- a/kernel/trace/bpf_trace.h
+++ b/kernel/trace/bpf_trace.h
@@ -25,6 +25,11 @@ TRACE_EVENT(bpf_trace_printk,
        TP_printk("%s", __get_str(bpf_string))
 );

+/* Prototypes for kernel functions exposed to tracing and LSM
+ * programs
+ */
+ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
+                    const char *name, void *value, int size);
 #endif /* _TRACE_BPF_TRACE_H */

(or anything else folks suggest)

- KP

>
> vim +/bpf_getxattr +1185 kernel/trace/bpf_trace.c
>
>   1184
> > 1185  noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
>   1186                                       const char *name, void *value, int size)
>   1187  {
>   1188          return __vfs_getxattr(dentry, inode, name, value, size);
>   1189  }
>   1190
>
> --
> 0-DAY CI Kernel Test Service
> https://01.org/lkp

Re: [PATCH v2 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

[KP Singh]

[-- Attachment #1: Type: text/plain, Size: 3222 bytes --]

On Tue, Jun 21, 2022 at 5:20 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi KP,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on bpf-next/master]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/KP-Singh/Add-bpf_getxattr/20220621-093013
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> config: x86_64-randconfig-a015-20220620 (https://download.01.org/0day-ci/archive/20220621/202206211035.p3LxbVfK-lkp(a)intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project af6d2a0b6825e71965f3e2701a63c239fa0ad70f)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/dd49d2ffb18adceafa98bd517008f59aa9bc910b
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review KP-Singh/Add-bpf_getxattr/20220621-093013
>         git checkout dd49d2ffb18adceafa98bd517008f59aa9bc910b
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> >> kernel/trace/bpf_trace.c:1185:25: warning: no previous prototype for function 'bpf_getxattr' [-Wmissing-prototypes]
>    noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
>                            ^
>    kernel/trace/bpf_trace.c:1185:17: note: declare 'static' if the function is not intended to be used outside of this translation unit
>    noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
>                    ^
>                    static
>    1 warning generated.
>

So it looks like this needs a function prototype. Let's do an initial round
of reviews on this series and I can respin with something like:

diff --git a/kernel/trace/bpf_trace.h b/kernel/trace/bpf_trace.h
index 9acbc11ac7bb..3f62e5d35037 100644
--- a/kernel/trace/bpf_trace.h
+++ b/kernel/trace/bpf_trace.h
@@ -25,6 +25,11 @@ TRACE_EVENT(bpf_trace_printk,
        TP_printk("%s", __get_str(bpf_string))
 );

+/* Prototypes for kernel functions exposed to tracing and LSM
+ * programs
+ */
+ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
+                    const char *name, void *value, int size);
 #endif /* _TRACE_BPF_TRACE_H */

(or anything else folks suggest)

- KP

>
> vim +/bpf_getxattr +1185 kernel/trace/bpf_trace.c
>
>   1184
> > 1185  noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
>   1186                                       const char *name, void *value, int size)
>   1187  {
>   1188          return __vfs_getxattr(dentry, inode, name, value, size);
>   1189  }
>   1190
>
> --
> 0-DAY CI Kernel Test Service
> https://01.org/lkp

Re: [PATCH v2 bpf-next 3/5] bpf: Allow kfuncs to be used in LSM programs

[Kumar Kartikeya Dwivedi]

On Tue, Jun 21, 2022 at 06:58:09AM IST, KP Singh wrote:
> In preparation for the addition of bpf_getxattr kfunc.
>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
>  kernel/bpf/btf.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 02d7951591ae..541cf4635aa1 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7264,6 +7264,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
>  	case BPF_PROG_TYPE_STRUCT_OPS:
>  		return BTF_KFUNC_HOOK_STRUCT_OPS;
>  	case BPF_PROG_TYPE_TRACING:
> +	case BPF_PROG_TYPE_LSM:
>  		return BTF_KFUNC_HOOK_TRACING;

Should we define another BTF_KFUNC_HOOK_LSM instead? Otherwise when you register
for tracing or lsm progs, you write to the same hook instead, so kfunc enabled
for tracing progs also gets enabled for lsm, I guess that is not what user
intends when registering kfunc set.

>  	case BPF_PROG_TYPE_SYSCALL:
>  		return BTF_KFUNC_HOOK_SYSCALL;
> --
> 2.37.0.rc0.104.g0611611a94-goog
>

--
Kartikeya

Re: [PATCH v2 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

[Kumar Kartikeya Dwivedi]

On Tue, Jun 21, 2022 at 12:48:08PM IST, KP Singh wrote:
> On Tue, Jun 21, 2022 at 5:20 AM kernel test robot <lkp@intel.com> wrote:
> >
> > Hi KP,
> >
> > I love your patch! Perhaps something to improve:
> >
> > [auto build test WARNING on bpf-next/master]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/KP-Singh/Add-bpf_getxattr/20220621-093013
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > config: x86_64-randconfig-a015-20220620 (https://download.01.org/0day-ci/archive/20220621/202206211035.p3LxbVfK-lkp@intel.com/config)
> > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project af6d2a0b6825e71965f3e2701a63c239fa0ad70f)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # https://github.com/intel-lab-lkp/linux/commit/dd49d2ffb18adceafa98bd517008f59aa9bc910b
> >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> >         git fetch --no-tags linux-review KP-Singh/Add-bpf_getxattr/20220621-093013
> >         git checkout dd49d2ffb18adceafa98bd517008f59aa9bc910b
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/
> >
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> > >> kernel/trace/bpf_trace.c:1185:25: warning: no previous prototype for function 'bpf_getxattr' [-Wmissing-prototypes]
> >    noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> >                            ^
> >    kernel/trace/bpf_trace.c:1185:17: note: declare 'static' if the function is not intended to be used outside of this translation unit
> >    noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> >                    ^
> >                    static
> >    1 warning generated.
> >
>
> So it looks like this needs a function prototype. Let's do an initial round
> of reviews on this series and I can respin with something like:
>
> diff --git a/kernel/trace/bpf_trace.h b/kernel/trace/bpf_trace.h
> index 9acbc11ac7bb..3f62e5d35037 100644
> --- a/kernel/trace/bpf_trace.h
> +++ b/kernel/trace/bpf_trace.h
> @@ -25,6 +25,11 @@ TRACE_EVENT(bpf_trace_printk,
>         TP_printk("%s", __get_str(bpf_string))
>  );
>
> +/* Prototypes for kernel functions exposed to tracing and LSM
> + * programs
> + */
> +ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> +                    const char *name, void *value, int size);
>  #endif /* _TRACE_BPF_TRACE_H */
>
> (or anything else folks suggest)
>

You can silence this warning using __diag_push, e.g. see kfunc definitions in
net/netfilter/nf_conntrack_bpf.c.

> - KP
>
> >
> > vim +/bpf_getxattr +1185 kernel/trace/bpf_trace.c
> >
> >   1184
> > > 1185  noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> >   1186                                       const char *name, void *value, int size)
> >   1187  {
> >   1188          return __vfs_getxattr(dentry, inode, name, value, size);
> >   1189  }
> >   1190
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://01.org/lkp

--
Kartikeya

Re: [PATCH v2 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

[Kumar Kartikeya Dwivedi]

[-- Attachment #1: Type: text/plain, Size: 3556 bytes --]

On Tue, Jun 21, 2022 at 12:48:08PM IST, KP Singh wrote:
> On Tue, Jun 21, 2022 at 5:20 AM kernel test robot <lkp@intel.com> wrote:
> >
> > Hi KP,
> >
> > I love your patch! Perhaps something to improve:
> >
> > [auto build test WARNING on bpf-next/master]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/KP-Singh/Add-bpf_getxattr/20220621-093013
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> > config: x86_64-randconfig-a015-20220620 (https://download.01.org/0day-ci/archive/20220621/202206211035.p3LxbVfK-lkp(a)intel.com/config)
> > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project af6d2a0b6825e71965f3e2701a63c239fa0ad70f)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # https://github.com/intel-lab-lkp/linux/commit/dd49d2ffb18adceafa98bd517008f59aa9bc910b
> >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> >         git fetch --no-tags linux-review KP-Singh/Add-bpf_getxattr/20220621-093013
> >         git checkout dd49d2ffb18adceafa98bd517008f59aa9bc910b
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/
> >
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> > >> kernel/trace/bpf_trace.c:1185:25: warning: no previous prototype for function 'bpf_getxattr' [-Wmissing-prototypes]
> >    noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> >                            ^
> >    kernel/trace/bpf_trace.c:1185:17: note: declare 'static' if the function is not intended to be used outside of this translation unit
> >    noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> >                    ^
> >                    static
> >    1 warning generated.
> >
>
> So it looks like this needs a function prototype. Let's do an initial round
> of reviews on this series and I can respin with something like:
>
> diff --git a/kernel/trace/bpf_trace.h b/kernel/trace/bpf_trace.h
> index 9acbc11ac7bb..3f62e5d35037 100644
> --- a/kernel/trace/bpf_trace.h
> +++ b/kernel/trace/bpf_trace.h
> @@ -25,6 +25,11 @@ TRACE_EVENT(bpf_trace_printk,
>         TP_printk("%s", __get_str(bpf_string))
>  );
>
> +/* Prototypes for kernel functions exposed to tracing and LSM
> + * programs
> + */
> +ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> +                    const char *name, void *value, int size);
>  #endif /* _TRACE_BPF_TRACE_H */
>
> (or anything else folks suggest)
>

You can silence this warning using __diag_push, e.g. see kfunc definitions in
net/netfilter/nf_conntrack_bpf.c.

> - KP
>
> >
> > vim +/bpf_getxattr +1185 kernel/trace/bpf_trace.c
> >
> >   1184
> > > 1185  noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> >   1186                                       const char *name, void *value, int size)
> >   1187  {
> >   1188          return __vfs_getxattr(dentry, inode, name, value, size);
> >   1189  }
> >   1190
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://01.org/lkp

--
Kartikeya

Re: [PATCH v2 bpf-next 0/5] Add bpf_getxattr

[Benjamin Tissoires]

On Tue, Jun 21, 2022 at 3:28 AM KP Singh <kpsingh@kernel.org> wrote:
>
> v1 -> v2
>
> - Used kfuncs as suggested by Alexei
> - Used Benjamin Tissoires' patch from the HID v4 series to add a
>   sleepable kfunc set (I sent the patch as a part of this series as it
>   seems to have been dropped from v5) and acked it. Hope this is okay.

FWIW, this is perfectly OK with me.

The reason I dropped the patch from the series is because I don't
absolutely need it anymore: all of my sleepable kfuncs are now
declared as SYSCALL type, which is by definition sleepable.

I still believe it's valuable to be able to define sleepable kfuncs however.

Cheers,
Benjamin

> - Added support for verifying string constants to kfuncs
>
> Foundation for building more complex security policies using the
> BPF LSM as presented in LSF/MM/BPF:
>
>  http://vger.kernel.org/bpfconf2022_material/lsfmmbpf2022-xattr.pdf
>
> Benjamin Tissoires (1):
>   btf: Add a new kfunc set which allows to mark a function to be
>     sleepable
>
> KP Singh (4):
>   bpf: kfunc support for ARG_PTR_TO_CONST_STR
>   bpf: Allow kfuncs to be used in LSM programs
>   bpf: Add a bpf_getxattr kfunc
>   bpf/selftests: Add a selftest for bpf_getxattr
>
>  include/linux/bpf_verifier.h                  |  2 +
>  include/linux/btf.h                           |  2 +
>  kernel/bpf/btf.c                              | 42 ++++++++-
>  kernel/bpf/verifier.c                         | 85 +++++++++++--------
>  kernel/trace/bpf_trace.c                      | 36 ++++++++
>  .../testing/selftests/bpf/prog_tests/xattr.c  | 58 +++++++++++++
>  tools/testing/selftests/bpf/progs/xattr.c     | 37 ++++++++
>  7 files changed, 223 insertions(+), 39 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/xattr.c
>  create mode 100644 tools/testing/selftests/bpf/progs/xattr.c
>
> --
> 2.37.0.rc0.104.g0611611a94-goog
>

Re: [PATCH v2 bpf-next 2/5] bpf: kfunc support for ARG_PTR_TO_CONST_STR

[Kumar Kartikeya Dwivedi]

On Tue, Jun 21, 2022 at 06:58:08AM IST, KP Singh wrote:
> kfuncs can handle pointers to memory when the next argument is
> the size of the memory that can be read and verify these as
> ARG_CONST_SIZE_OR_ZERO
>
> Similarly add support for string constants (const char *) and
> verify it similar to ARG_PTR_TO_CONST_STR.
>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
>  include/linux/bpf_verifier.h |  2 +
>  kernel/bpf/btf.c             | 29 ++++++++++++
>  kernel/bpf/verifier.c        | 85 ++++++++++++++++++++----------------
>  3 files changed, 79 insertions(+), 37 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 3930c963fa67..60d490354397 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -548,6 +548,8 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state
>  			     u32 regno);
>  int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
>  		   u32 regno, u32 mem_size);
> +int check_const_str(struct bpf_verifier_env *env,
> +		    const struct bpf_reg_state *reg, int regno);
>
>  /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
>  static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 668ecf61649b..02d7951591ae 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6162,6 +6162,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
>  	return true;
>  }
>
> +static bool btf_param_is_const_str_ptr(const struct btf *btf,
> +				       const struct btf_param *param)
> +{
> +	const struct btf_type *t;
> +
> +	t = btf_type_by_id(btf, param->type);
> +	if (!btf_type_is_ptr(t))
> +		return false;
> +
> +	t = btf_type_by_id(btf, t->type);
> +	if (!(BTF_INFO_KIND(t->info) == BTF_KIND_CONST))
> +		return false;
> +
> +	t = btf_type_skip_modifiers(btf, t->type, NULL);
> +	if (!strcmp(btf_name_by_offset(btf, t->name_off), "char"))
> +		return true;
> +
> +	return false;
> +}
> +
>  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  				    const struct btf *btf, u32 func_id,
>  				    struct bpf_reg_state *regs,
> @@ -6344,6 +6364,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  		} else if (ptr_to_mem_ok) {
>  			const struct btf_type *resolve_ret;
>  			u32 type_size;
> +			int err;
>
>  			if (is_kfunc) {
>  				bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
> @@ -6354,6 +6375,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>  				 * When arg_mem_size is true, the pointer can be
>  				 * void *.
>  				 */
> +				if (btf_param_is_const_str_ptr(btf, &args[i])) {

Here, we need to check whether reg is a PTR_TO_MAP_VALUE, otherwise in
check_const_str, reg->map_ptr may be NULL. Probably best to do it in
btf_param_is_const_str_ptr itself.

> +					err = check_const_str(env, reg, regno);
> +					if (err < 0)
> +						return err;
> +					i++;
> +					continue;
> +				}
> +
>  				if (!btf_type_is_scalar(ref_t) &&
>  				    !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
>  				    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2859901ffbe3..14a434792d7b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5840,6 +5840,52 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
>  	return state->stack[spi].spilled_ptr.id;
>  }
>
> +int check_const_str(struct bpf_verifier_env *env,
> +		    const struct bpf_reg_state *reg, int regno)
> +{
> +	struct bpf_map *map = reg->map_ptr;
> +	int map_off;
> +	u64 map_addr;
> +	char *str_ptr;
> +	int err;
> +
> +	if (!bpf_map_is_rdonly(map)) {
> +		verbose(env, "R%d does not point to a readonly map'\n", regno);
> +		return -EACCES;
> +	}
> +
> +	if (!tnum_is_const(reg->var_off)) {
> +		verbose(env, "R%d is not a constant address'\n", regno);
> +		return -EACCES;
> +	}
> +
> +	if (!map->ops->map_direct_value_addr) {
> +		verbose(env,
> +			"no direct value access support for this map type\n");
> +		return -EACCES;
> +	}
> +
> +	err = check_map_access(env, regno, reg->off, map->value_size - reg->off,
> +			       false, ACCESS_HELPER);
> +	if (err)
> +		return err;
> +
> +	map_off = reg->off + reg->var_off.value;
> +	err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
> +	if (err) {
> +		verbose(env, "direct value access on string failed\n");
> +		return err;
> +	}
> +
> +	str_ptr = (char *)(long)(map_addr);
> +	if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) {
> +		verbose(env, "string is not zero-terminated\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  			  struct bpf_call_arg_meta *meta,
>  			  const struct bpf_func_proto *fn)
> @@ -6074,44 +6120,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  			return err;
>  		err = check_ptr_alignment(env, reg, 0, size, true);
>  	} else if (arg_type == ARG_PTR_TO_CONST_STR) {
> -		struct bpf_map *map = reg->map_ptr;
> -		int map_off;
> -		u64 map_addr;
> -		char *str_ptr;
> -
> -		if (!bpf_map_is_rdonly(map)) {
> -			verbose(env, "R%d does not point to a readonly map'\n", regno);
> -			return -EACCES;
> -		}
> -
> -		if (!tnum_is_const(reg->var_off)) {
> -			verbose(env, "R%d is not a constant address'\n", regno);
> -			return -EACCES;
> -		}
> -
> -		if (!map->ops->map_direct_value_addr) {
> -			verbose(env, "no direct value access support for this map type\n");
> -			return -EACCES;
> -		}
> -
> -		err = check_map_access(env, regno, reg->off,
> -				       map->value_size - reg->off, false,
> -				       ACCESS_HELPER);
> -		if (err)
> -			return err;
> -
> -		map_off = reg->off + reg->var_off.value;
> -		err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
> -		if (err) {
> -			verbose(env, "direct value access on string failed\n");
> +		err = check_const_str(env, reg, regno);
> +		if (err < 0)
>  			return err;
> -		}
> -
> -		str_ptr = (char *)(long)(map_addr);
> -		if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) {
> -			verbose(env, "string is not zero-terminated\n");
> -			return -EINVAL;
> -		}
>  	} else if (arg_type == ARG_PTR_TO_KPTR) {
>  		if (process_kptr_func(env, regno, meta))
>  			return -EACCES;
> --
> 2.37.0.rc0.104.g0611611a94-goog
>

--
Kartikeya

Re: [PATCH v2 bpf-next 3/5] bpf: Allow kfuncs to be used in LSM programs

[Alexei Starovoitov]

On Tue, Jun 21, 2022 at 5:36 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Tue, Jun 21, 2022 at 06:58:09AM IST, KP Singh wrote:
> > In preparation for the addition of bpf_getxattr kfunc.
> >
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  kernel/bpf/btf.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 02d7951591ae..541cf4635aa1 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -7264,6 +7264,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
> >       case BPF_PROG_TYPE_STRUCT_OPS:
> >               return BTF_KFUNC_HOOK_STRUCT_OPS;
> >       case BPF_PROG_TYPE_TRACING:
> > +     case BPF_PROG_TYPE_LSM:
> >               return BTF_KFUNC_HOOK_TRACING;
>
> Should we define another BTF_KFUNC_HOOK_LSM instead? Otherwise when you register
> for tracing or lsm progs, you write to the same hook instead, so kfunc enabled
> for tracing progs also gets enabled for lsm, I guess that is not what user
> intends when registering kfunc set.

It's probably ok for this case.
We might combine BTF_KFUNC_HOOK* into fewer hooks and
scope them by attach_btf_id.
Everything is 'tracing' like at the end.
Upcoming hid-bpf is 'tracing'. lsm is 'tracing'.
but we may need to reduce the scope of kfuncs
based on attach point or based on argument type.
tc vs xdp don't need to be separate sets.
Their types (skb vs xdp_md) are different, so only
right kfuncs can be used, but bpf_ct_release() is needed
in both tc and xdp.
So we could combine tc and xdp into 'btf_kfunc_hook_networking'
without compromising the safety.
acquire vs release ideally would be indicated with btf_tag,
but gcc still doesn't have support for btf_tag and we cannot
require the kernel to be built with clang.
acquire, release, sleepable, ret_null should be flags on a kfunc
instead of a set. It would be easier to manage and less boilerplate
code. Not clear how to do this cleanly.
export_symbol approach is a bit heavy and requires name being unique
across kernel and modules.
func name mangling or typedef-ing comes to mind. not so clean either.

Re: [PATCH v2 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

[KP Singh]

On Tue, Jun 21, 2022 at 2:41 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Tue, Jun 21, 2022 at 12:48:08PM IST, KP Singh wrote:
> > On Tue, Jun 21, 2022 at 5:20 AM kernel test robot <lkp@intel.com> wrote:
> > >
> > > Hi KP,
> > >
> > > I love your patch! Perhaps something to improve:
> > >
> > > [auto build test WARNING on bpf-next/master]
[...]

> >
>
> You can silence this warning using __diag_push, e.g. see kfunc definitions in
> net/netfilter/nf_conntrack_bpf.c.

Thanks, done.

>
> > - KP
> >
> > >
> > > vim +/bpf_getxattr +1185 kernel/trace/bpf_trace.c
> > >
> > >   1184
> > > > 1185  noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> > >   1186                                       const char *name, void *value, int size)
> > >   1187  {
> > >   1188          return __vfs_getxattr(dentry, inode, name, value, size);
> > >   1189  }
> > >   1190
> > >
> > > --
> > > 0-DAY CI Kernel Test Service
> > > https://01.org/lkp
>
> --
> Kartikeya

Re: [PATCH v2 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

[KP Singh]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Tue, Jun 21, 2022 at 2:41 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Tue, Jun 21, 2022 at 12:48:08PM IST, KP Singh wrote:
> > On Tue, Jun 21, 2022 at 5:20 AM kernel test robot <lkp@intel.com> wrote:
> > >
> > > Hi KP,
> > >
> > > I love your patch! Perhaps something to improve:
> > >
> > > [auto build test WARNING on bpf-next/master]
[...]

> >
>
> You can silence this warning using __diag_push, e.g. see kfunc definitions in
> net/netfilter/nf_conntrack_bpf.c.

Thanks, done.

>
> > - KP
> >
> > >
> > > vim +/bpf_getxattr +1185 kernel/trace/bpf_trace.c
> > >
> > >   1184
> > > > 1185  noinline __weak ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> > >   1186                                       const char *name, void *value, int size)
> > >   1187  {
> > >   1188          return __vfs_getxattr(dentry, inode, name, value, size);
> > >   1189  }
> > >   1190
> > >
> > > --
> > > 0-DAY CI Kernel Test Service
> > > https://01.org/lkp
>
> --
> Kartikeya

Re: [PATCH v2 bpf-next 3/5] bpf: Allow kfuncs to be used in LSM programs

[KP Singh]

On Tue, Jun 21, 2022 at 6:01 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Jun 21, 2022 at 5:36 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Tue, Jun 21, 2022 at 06:58:09AM IST, KP Singh wrote:
> > > In preparation for the addition of bpf_getxattr kfunc.
> > >
> > > Signed-off-by: KP Singh <kpsingh@kernel.org>

[...]

> > > +++ b/kernel/bpf/btf.c
> > > @@ -7264,6 +7264,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
> > >       case BPF_PROG_TYPE_STRUCT_OPS:
> > >               return BTF_KFUNC_HOOK_STRUCT_OPS;
> > >       case BPF_PROG_TYPE_TRACING:
> > > +     case BPF_PROG_TYPE_LSM:
> > >               return BTF_KFUNC_HOOK_TRACING;
> >
> > Should we define another BTF_KFUNC_HOOK_LSM instead? Otherwise when you register
> > for tracing or lsm progs, you write to the same hook instead, so kfunc enabled
> > for tracing progs also gets enabled for lsm, I guess that is not what user
> > intends when registering kfunc set.
>
> It's probably ok for this case.
> We might combine BTF_KFUNC_HOOK* into fewer hooks and

I did this intentionally. We do this similarly for helpers too and we
unfortunately, we do realize that LSM hooks are not there in all
places (esp. where one is auditing stuff).

So in reality one does use a combination of LSM and tracing hooks
with the policy decision coming from the LSM hook but the state
gets accumulated from different hooks.


> scope them by attach_btf_id.
> Everything is 'tracing' like at the end.
> Upcoming hid-bpf is 'tracing'. lsm is 'tracing'.
> but we may need to reduce the scope of kfuncs
> based on attach point or based on argument type.
> tc vs xdp don't need to be separate sets.
> Their types (skb vs xdp_md) are different, so only
> right kfuncs can be used, but bpf_ct_release() is needed
> in both tc and xdp.
> So we could combine tc and xdp into 'btf_kfunc_hook_networking'
> without compromising the safety.
> acquire vs release ideally would be indicated with btf_tag,
> but gcc still doesn't have support for btf_tag and we cannot
> require the kernel to be built with clang.
> acquire, release, sleepable, ret_null should be flags on a kfunc
> instead of a set. It would be easier to manage and less boilerplate

+1 This would be awesome, I gave it a shot to use btf_tag but hit this
feature gap in GCC.

- KP

> code. Not clear how to do this cleanly.
> export_symbol approach is a bit heavy and requires name being unique
> across kernel and modules.
> func name mangling or typedef-ing comes to mind. not so clean either.

Re: [PATCH v2 bpf-next 2/5] bpf: kfunc support for ARG_PTR_TO_CONST_STR

[KP Singh]

On Tue, Jun 21, 2022 at 2:50 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Tue, Jun 21, 2022 at 06:58:08AM IST, KP Singh wrote:
> > kfuncs can handle pointers to memory when the next argument is
> > the size of the memory that can be read and verify these as
> > ARG_CONST_SIZE_OR_ZERO
> >
> > Similarly add support for string constants (const char *) and
> > verify it similar to ARG_PTR_TO_CONST_STR.
> >
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---

[...]

> >                       if (is_kfunc) {
> >                               bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
> > @@ -6354,6 +6375,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >                                * When arg_mem_size is true, the pointer can be
> >                                * void *.
> >                                */
> > +                             if (btf_param_is_const_str_ptr(btf, &args[i])) {
>
> Here, we need to check whether reg is a PTR_TO_MAP_VALUE, otherwise in
> check_const_str, reg->map_ptr may be NULL. Probably best to do it in
> btf_param_is_const_str_ptr itself.

I added it to the check_const_str as:

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 14a434792d7b..5300e022398a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5843,12 +5843,16 @@ static u32 stack_slot_get_id(struct
bpf_verifier_env *env, struct bpf_reg_state
 int check_const_str(struct bpf_verifier_env *env,
                    const struct bpf_reg_state *reg, int regno)
 {
-       struct bpf_map *map = reg->map_ptr;
+       struct bpf_map *map;
        int map_off;
        u64 map_addr;
        char *str_ptr;
        int err;

+       if (reg->type != PTR_TO_MAP_VALUE)
+               return -EACCES;
+
+       map = reg->map_ptr;
        if (!bpf_map_is_rdonly(map)) {
                verbose(env, "R%d does not point to a readonly map'\n", regno);
                return -EACCES;

>
> > +                                     err = check_const_str(env, reg, regno);
> > +                                     if (err < 0)
> > +                                             return err;
> > +                                     i++;
> > +                                     continue;
> > +                             }

[...]

> > --
> > 2.37.0.rc0.104.g0611611a94-goog
> >

>
> --
> Kartikeya

Re: [PATCH v2 bpf-next 3/5] bpf: Allow kfuncs to be used in LSM programs

[Kumar Kartikeya Dwivedi]

On Tue, Jun 21, 2022 at 09:45:15PM IST, KP Singh wrote:
> On Tue, Jun 21, 2022 at 6:01 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Jun 21, 2022 at 5:36 AM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > On Tue, Jun 21, 2022 at 06:58:09AM IST, KP Singh wrote:
> > > > In preparation for the addition of bpf_getxattr kfunc.
> > > >
> > > > Signed-off-by: KP Singh <kpsingh@kernel.org>
>
> [...]
>
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -7264,6 +7264,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
> > > >       case BPF_PROG_TYPE_STRUCT_OPS:
> > > >               return BTF_KFUNC_HOOK_STRUCT_OPS;
> > > >       case BPF_PROG_TYPE_TRACING:
> > > > +     case BPF_PROG_TYPE_LSM:
> > > >               return BTF_KFUNC_HOOK_TRACING;
> > >
> > > Should we define another BTF_KFUNC_HOOK_LSM instead? Otherwise when you register
> > > for tracing or lsm progs, you write to the same hook instead, so kfunc enabled
> > > for tracing progs also gets enabled for lsm, I guess that is not what user
> > > intends when registering kfunc set.
> >
> > It's probably ok for this case.
> > We might combine BTF_KFUNC_HOOK* into fewer hooks and
>
> I did this intentionally. We do this similarly for helpers too and we
> unfortunately, we do realize that LSM hooks are not there in all
> places (esp. where one is auditing stuff).
>
> So in reality one does use a combination of LSM and tracing hooks
> with the policy decision coming from the LSM hook but the state
> gets accumulated from different hooks.
>
>
> > scope them by attach_btf_id.
> > Everything is 'tracing' like at the end.
> > Upcoming hid-bpf is 'tracing'. lsm is 'tracing'.
> > but we may need to reduce the scope of kfuncs
> > based on attach point or based on argument type.
> > tc vs xdp don't need to be separate sets.
> > Their types (skb vs xdp_md) are different, so only
> > right kfuncs can be used, but bpf_ct_release() is needed
> > in both tc and xdp.
> > So we could combine tc and xdp into 'btf_kfunc_hook_networking'
> > without compromising the safety.
> > acquire vs release ideally would be indicated with btf_tag,
> > but gcc still doesn't have support for btf_tag and we cannot
> > require the kernel to be built with clang.
> > acquire, release, sleepable, ret_null should be flags on a kfunc
> > instead of a set. It would be easier to manage and less boilerplate
>
> +1 This would be awesome, I gave it a shot to use btf_tag but hit this
> feature gap in GCC.
>
> - KP
>
> > code. Not clear how to do this cleanly.
> > export_symbol approach is a bit heavy and requires name being unique
> > across kernel and modules.
> > func name mangling or typedef-ing comes to mind. not so clean either.

How does this approach look to 'tag' a kfunc?

https://godbolt.org/z/jGeK6Y49K

Then we find the function whose symbol should be emitted and available in BTF
with the prefix (__kfunc_name_) and read off the tag values. Due to the extern
inline it should still inline the definition, so there is no overhead at
runtime.

As you see in godbolt, __foobar_1_2 symbol is visible, with 1 and 2 being tag
values. Substituting tag name with underlying value makes parsing easier. We
can have fixed number of slots and keep others as 0 (ignore).

It can be extended to also tag arguments, which I already did before at [0], but
never sent out. Then we don't need the suffixes or special naming of arguments
to indicate some tag.

e.g.

bpf_ct_release will be:

KFUNC_CALL_0(void, bpf_ct_release, __kfunc_release,
	     KARG(struct nf_conn *, nfct, __ptr_ref))
{
	...
}

__kfunc_acquire and __kfunc_ret_null will go in the same place as __kfunc_release.

We may also allow tagging as ptr_to_btf_id or ptr_to_mem etc. to force passing a
certain register type and avoid ambiguity.

 [0]: https://gist.github.com/kkdwivedi/7839cc9e4f002acc3e15350b1b86c88c

--
Kartikeya

Re: [PATCH v2 bpf-next 2/5] bpf: kfunc support for ARG_PTR_TO_CONST_STR

[Joanne Koong]

On Mon, Jun 20, 2022 at 6:29 PM KP Singh <kpsingh@kernel.org> wrote:
>
> kfuncs can handle pointers to memory when the next argument is
> the size of the memory that can be read and verify these as
> ARG_CONST_SIZE_OR_ZERO
>
> Similarly add support for string constants (const char *) and
> verify it similar to ARG_PTR_TO_CONST_STR.
>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
>  include/linux/bpf_verifier.h |  2 +
>  kernel/bpf/btf.c             | 29 ++++++++++++
>  kernel/bpf/verifier.c        | 85 ++++++++++++++++++++----------------
>  3 files changed, 79 insertions(+), 37 deletions(-)
>
[...]
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 668ecf61649b..02d7951591ae 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6162,6 +6162,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
>         return true;
>  }
>
> +static bool btf_param_is_const_str_ptr(const struct btf *btf,
> +                                      const struct btf_param *param)
> +{
> +       const struct btf_type *t;
> +
> +       t = btf_type_by_id(btf, param->type);
> +       if (!btf_type_is_ptr(t))
> +               return false;
> +
> +       t = btf_type_by_id(btf, t->type);
> +       if (!(BTF_INFO_KIND(t->info) == BTF_KIND_CONST))
"if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)" looks clearer to me
> +               return false;
> +
> +       t = btf_type_skip_modifiers(btf, t->type, NULL);
> +       if (!strcmp(btf_name_by_offset(btf, t->name_off), "char"))
"return !strcmp(btf_name_by_offset(btf, t->name_off), "char")" looks
clearer to me here too
> +               return true;
> +
> +       return false;
> +}
> +
>  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                                     const struct btf *btf, u32 func_id,
>                                     struct bpf_reg_state *regs,
> @@ -6344,6 +6364,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                 } else if (ptr_to_mem_ok) {
>                         const struct btf_type *resolve_ret;
>                         u32 type_size;
> +                       int err;
>
>                         if (is_kfunc) {
>                                 bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
> @@ -6354,6 +6375,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                                  * When arg_mem_size is true, the pointer can be
>                                  * void *.
>                                  */
> +                               if (btf_param_is_const_str_ptr(btf, &args[i])) {
> +                                       err = check_const_str(env, reg, regno);
> +                                       if (err < 0)
> +                                               return err;
> +                                       i++;
> +                                       continue;
If I'm understanding it correctly, this patch is intended to allow
helper functions to take in a kfunc as an arg as long as the next arg
is the size of the memory. Do we need to check the memory size access
here (eg like a call to check_mem_size_reg() in the verifier) to
ensure that memory accesses of that size are safe?
> +                               }
> +
>                                 if (!btf_type_is_scalar(ref_t) &&
>                                     !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
>                                     (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2859901ffbe3..14a434792d7b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5840,6 +5840,52 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
>         return state->stack[spi].spilled_ptr.id;
>  }
[...]
> +
>  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                           struct bpf_call_arg_meta *meta,
>                           const struct bpf_func_proto *fn)
> @@ -6074,44 +6120,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                         return err;
>                 err = check_ptr_alignment(env, reg, 0, size, true);
>         } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> -               struct bpf_map *map = reg->map_ptr;
> -               int map_off;
> -               u64 map_addr;
> -               char *str_ptr;
> -
> -               if (!bpf_map_is_rdonly(map)) {
> -                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> -                       return -EACCES;
> -               }
> -
> -               if (!tnum_is_const(reg->var_off)) {
> -                       verbose(env, "R%d is not a constant address'\n", regno);
> -                       return -EACCES;
> -               }
> -
> -               if (!map->ops->map_direct_value_addr) {
> -                       verbose(env, "no direct value access support for this map type\n");
> -                       return -EACCES;
> -               }
> -
> -               err = check_map_access(env, regno, reg->off,
> -                                      map->value_size - reg->off, false,
> -                                      ACCESS_HELPER);
> -               if (err)
> -                       return err;
> -
> -               map_off = reg->off + reg->var_off.value;
> -               err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
> -               if (err) {
> -                       verbose(env, "direct value access on string failed\n");
> +               err = check_const_str(env, reg, regno);
> +               if (err < 0)
>                         return err;
nit: I don't think you need the if check here since thsi function will
return err automatically in the next line

> -               }
> -
> -               str_ptr = (char *)(long)(map_addr);
> -               if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) {
> -                       verbose(env, "string is not zero-terminated\n");
> -                       return -EINVAL;
> -               }
>         } else if (arg_type == ARG_PTR_TO_KPTR) {
>                 if (process_kptr_func(env, regno, meta))
>                         return -EACCES;
> --
> 2.37.0.rc0.104.g0611611a94-goog
>

Re: [PATCH v2 bpf-next 3/5] bpf: Allow kfuncs to be used in LSM programs

[Alexei Starovoitov]

On Tue, Jun 21, 2022 at 11:05:45PM +0530, Kumar Kartikeya Dwivedi wrote:
> On Tue, Jun 21, 2022 at 09:45:15PM IST, KP Singh wrote:
> > On Tue, Jun 21, 2022 at 6:01 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Tue, Jun 21, 2022 at 5:36 AM Kumar Kartikeya Dwivedi
> > > <memxor@gmail.com> wrote:
> > > >
> > > > On Tue, Jun 21, 2022 at 06:58:09AM IST, KP Singh wrote:
> > > > > In preparation for the addition of bpf_getxattr kfunc.
> > > > >
> > > > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> >
> > [...]
> >
> > > > > +++ b/kernel/bpf/btf.c
> > > > > @@ -7264,6 +7264,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
> > > > >       case BPF_PROG_TYPE_STRUCT_OPS:
> > > > >               return BTF_KFUNC_HOOK_STRUCT_OPS;
> > > > >       case BPF_PROG_TYPE_TRACING:
> > > > > +     case BPF_PROG_TYPE_LSM:
> > > > >               return BTF_KFUNC_HOOK_TRACING;
> > > >
> > > > Should we define another BTF_KFUNC_HOOK_LSM instead? Otherwise when you register
> > > > for tracing or lsm progs, you write to the same hook instead, so kfunc enabled
> > > > for tracing progs also gets enabled for lsm, I guess that is not what user
> > > > intends when registering kfunc set.
> > >
> > > It's probably ok for this case.
> > > We might combine BTF_KFUNC_HOOK* into fewer hooks and
> >
> > I did this intentionally. We do this similarly for helpers too and we
> > unfortunately, we do realize that LSM hooks are not there in all
> > places (esp. where one is auditing stuff).
> >
> > So in reality one does use a combination of LSM and tracing hooks
> > with the policy decision coming from the LSM hook but the state
> > gets accumulated from different hooks.
> >
> >
> > > scope them by attach_btf_id.
> > > Everything is 'tracing' like at the end.
> > > Upcoming hid-bpf is 'tracing'. lsm is 'tracing'.
> > > but we may need to reduce the scope of kfuncs
> > > based on attach point or based on argument type.
> > > tc vs xdp don't need to be separate sets.
> > > Their types (skb vs xdp_md) are different, so only
> > > right kfuncs can be used, but bpf_ct_release() is needed
> > > in both tc and xdp.
> > > So we could combine tc and xdp into 'btf_kfunc_hook_networking'
> > > without compromising the safety.
> > > acquire vs release ideally would be indicated with btf_tag,
> > > but gcc still doesn't have support for btf_tag and we cannot
> > > require the kernel to be built with clang.
> > > acquire, release, sleepable, ret_null should be flags on a kfunc
> > > instead of a set. It would be easier to manage and less boilerplate
> >
> > +1 This would be awesome, I gave it a shot to use btf_tag but hit this
> > feature gap in GCC.
> >
> > - KP
> >
> > > code. Not clear how to do this cleanly.
> > > export_symbol approach is a bit heavy and requires name being unique
> > > across kernel and modules.
> > > func name mangling or typedef-ing comes to mind. not so clean either.
> 
> How does this approach look to 'tag' a kfunc?
> 
> https://godbolt.org/z/jGeK6Y49K
> 
> Then we find the function whose symbol should be emitted and available in BTF
> with the prefix (__kfunc_name_) and read off the tag values. Due to the extern
> inline it should still inline the definition, so there is no overhead at
> runtime.

No run-time overhead, but code size duplication is prohibitive.
Both functions will have full body.
We can hack around with another noinline function that both will call,
but it's getting ugly.

> As you see in godbolt, __foobar_1_2 symbol is visible, with 1 and 2 being tag
> values. Substituting tag name with underlying value makes parsing easier. We
> can have fixed number of slots and keep others as 0 (ignore).
> 
> It can be extended to also tag arguments, which I already did before at [0], but
> never sent out. Then we don't need the suffixes or special naming of arguments
> to indicate some tag.
> 
> e.g.
> 
> bpf_ct_release will be:
> 
> KFUNC_CALL_0(void, bpf_ct_release, __kfunc_release,
> 	     KARG(struct nf_conn *, nfct, __ptr_ref))
> {
> 	...
> }

-struct nf_conn *
-bpf_skb_ct_lookup(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,
-		  u32 tuple__sz, struct bpf_ct_opts *opts, u32 opts__sz)
+KFUNC_CALL_5(struct nf_conn *, bpf_skb_ct_lookup,
+	     KARG(struct __sk_buff *, skb_ctx, PTR_TO_CTX),
+	     KARG(struct bpf_sock_tuple *, bpf_tuple, PTR_TO_MEM),
+	     KARG(u32, tuple__sz, SIZE),
+	     KARG(struct bpf_ct_opts *, opts, PTR_TO_MEM),
+	     KARG(u32, opts__sz, SIZE))

I think it's too ugly for majority of kernel folks to accept.
We will have a lot more kfuncs than we have now. Above code will be
present in many different subsystems. The trade-off is not worth it.
__sz suffix convention works. Similar approach can be used.
This is orthogonal discussion anyway.

Back to kfunc tagging.
How about we use
  BTF_SET_START8(list)
  BTF_ID_FLAGS(func, bpf_xdp_ct_lookup, ACQUIRE | RET_NULL)
  BTF_ID_FLAGS(func, bpf_ct_release, RELEASE)
  BTF_SET_END8(list)

where BTF_SET_START8 macro will do "__BTF_ID__set8__" #name
so that resolve_btfid side knows that it needs to ignore 4 bytes after btf_id
when it's doing set sorting.
The kernel side btf_id_set_contains() would need to call bsearch()
with sizeof(u64) instead of sizeof(u32).

The verifier side will be cleaner and faster too.
Instead of two bsearch calls (that are about to become 3 calls with sleepable set):
                rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
                                                BTF_KFUNC_TYPE_RELEASE, func_id);
                kptr_get = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
                                                     BTF_KFUNC_TYPE_KPTR_ACQUIRE, func_id);
It will be a single call plus flag check.

Re: [PATCH v2 bpf-next 2/5] bpf: kfunc support for ARG_PTR_TO_CONST_STR

[KP Singh]

On Tue, Jun 21, 2022 at 8:04 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Mon, Jun 20, 2022 at 6:29 PM KP Singh <kpsingh@kernel.org> wrote:
> >
> > kfuncs can handle pointers to memory when the next argument is
> > the size of the memory that can be read and verify these as
> > ARG_CONST_SIZE_OR_ZERO
> >
> > Similarly add support for string constants (const char *) and
> > verify it similar to ARG_PTR_TO_CONST_STR.
> >
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  include/linux/bpf_verifier.h |  2 +
> >  kernel/bpf/btf.c             | 29 ++++++++++++
> >  kernel/bpf/verifier.c        | 85 ++++++++++++++++++++----------------
> >  3 files changed, 79 insertions(+), 37 deletions(-)
> >
> [...]
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 668ecf61649b..02d7951591ae 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6162,6 +6162,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
> >         return true;
> >  }
> >
> > +static bool btf_param_is_const_str_ptr(const struct btf *btf,
> > +                                      const struct btf_param *param)
> > +{
> > +       const struct btf_type *t;
> > +
> > +       t = btf_type_by_id(btf, param->type);
> > +       if (!btf_type_is_ptr(t))
> > +               return false;
> > +
> > +       t = btf_type_by_id(btf, t->type);
> > +       if (!(BTF_INFO_KIND(t->info) == BTF_KIND_CONST))
> "if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)" looks clearer to me
> > +               return false;
> > +
> > +       t = btf_type_skip_modifiers(btf, t->type, NULL);
> > +       if (!strcmp(btf_name_by_offset(btf, t->name_off), "char"))
> "return !strcmp(btf_name_by_offset(btf, t->name_off), "char")" looks
> clearer to me here too

Agreed. Updated.

> > +               return true;
> > +
> > +       return false;
> > +}
> > +
> >  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >                                     const struct btf *btf, u32 func_id,
> >                                     struct bpf_reg_state *regs,
> > @@ -6344,6 +6364,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >                 } else if (ptr_to_mem_ok) {
> >                         const struct btf_type *resolve_ret;
> >                         u32 type_size;
> > +                       int err;
> >
> >                         if (is_kfunc) {
> >                                 bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
> > @@ -6354,6 +6375,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >                                  * When arg_mem_size is true, the pointer can be
> >                                  * void *.
> >                                  */
> > +                               if (btf_param_is_const_str_ptr(btf, &args[i])) {
> > +                                       err = check_const_str(env, reg, regno);
> > +                                       if (err < 0)
> > +                                               return err;
> > +                                       i++;
> > +                                       continue;
> If I'm understanding it correctly, this patch is intended to allow
> helper functions to take in a kfunc as an arg as long as the next arg
> is the size of the memory. Do we need to check the memory size access
> here (eg like a call to check_mem_size_reg() in the verifier) to
> ensure that memory accesses of that size are safe?

No, this is different. We already have the verification for where we pair a
void * pointer to a size argument in the next arg.

https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/btf.c#n6366

This logic is similar to the verification we do for ARG_PTR_TO_CONST_STR where
we do not need a matching size argument and we just check for a null
terminated string
passed via a R/O map.


> > +                               }
> > +
> >                                 if (!btf_type_is_scalar(ref_t) &&
> >                                     !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
> >                                     (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 2859901ffbe3..14a434792d7b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5840,6 +5840,52 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
> >         return state->stack[spi].spilled_ptr.id;
> >  }
> [...]
> > +
> >  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >                           struct bpf_call_arg_meta *meta,
> >                           const struct bpf_func_proto *fn)
> > @@ -6074,44 +6120,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >                         return err;
> >                 err = check_ptr_alignment(env, reg, 0, size, true);
> >         } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > -               struct bpf_map *map = reg->map_ptr;
> > -               int map_off;
> > -               u64 map_addr;
> > -               char *str_ptr;
> > -
> > -               if (!bpf_map_is_rdonly(map)) {
> > -                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > -                       return -EACCES;
> > -               }
> > -
> > -               if (!tnum_is_const(reg->var_off)) {
> > -                       verbose(env, "R%d is not a constant address'\n", regno);
> > -                       return -EACCES;
> > -               }
> > -
> > -               if (!map->ops->map_direct_value_addr) {
> > -                       verbose(env, "no direct value access support for this map type\n");
> > -                       return -EACCES;
> > -               }
> > -
> > -               err = check_map_access(env, regno, reg->off,
> > -                                      map->value_size - reg->off, false,
> > -                                      ACCESS_HELPER);
> > -               if (err)
> > -                       return err;
> > -
> > -               map_off = reg->off + reg->var_off.value;
> > -               err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
> > -               if (err) {
> > -                       verbose(env, "direct value access on string failed\n");
> > +               err = check_const_str(env, reg, regno);
> > +               if (err < 0)
> >                         return err;
> nit: I don't think you need the if check here since thsi function will
> return err automatically in the next line

Makes sense. Fixed.

>
> > -               }
> > -
> > -               str_ptr = (char *)(long)(map_addr);
> > -               if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) {
> > -                       verbose(env, "string is not zero-terminated\n");
> > -                       return -EINVAL;
> > -               }
> >         } else if (arg_type == ARG_PTR_TO_KPTR) {
> >                 if (process_kptr_func(env, regno, meta))
> >                         return -EACCES;
> > --
> > 2.37.0.rc0.104.g0611611a94-goog
> >

Re: [PATCH v2 bpf-next 3/5] bpf: Allow kfuncs to be used in LSM programs

[KP Singh]

On Tue, Jun 21, 2022 at 10:12 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Jun 21, 2022 at 11:05:45PM +0530, Kumar Kartikeya Dwivedi wrote:
> > On Tue, Jun 21, 2022 at 09:45:15PM IST, KP Singh wrote:
> > > On Tue, Jun 21, 2022 at 6:01 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Tue, Jun 21, 2022 at 5:36 AM Kumar Kartikeya Dwivedi
> > > > <memxor@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jun 21, 2022 at 06:58:09AM IST, KP Singh wrote:
> > > > > > In preparation for the addition of bpf_getxattr kfunc.
> > > > > >
> > > > > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > >
> > > [...]
> > >
> > > > > > +++ b/kernel/bpf/btf.c
> > > > > > @@ -7264,6 +7264,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
> > > > > >       case BPF_PROG_TYPE_STRUCT_OPS:
> > > > > >               return BTF_KFUNC_HOOK_STRUCT_OPS;
> > > > > >       case BPF_PROG_TYPE_TRACING:
> > > > > > +     case BPF_PROG_TYPE_LSM:
> > > > > >               return BTF_KFUNC_HOOK_TRACING;
> > > > >
> > > > > Should we define another BTF_KFUNC_HOOK_LSM instead? Otherwise when you register
> > > > > for tracing or lsm progs, you write to the same hook instead, so kfunc enabled
> > > > > for tracing progs also gets enabled for lsm, I guess that is not what user
> > > > > intends when registering kfunc set.
> > > >
> > > > It's probably ok for this case.
> > > > We might combine BTF_KFUNC_HOOK* into fewer hooks and
> > >
> > > I did this intentionally. We do this similarly for helpers too and we
> > > unfortunately, we do realize that LSM hooks are not there in all
> > > places (esp. where one is auditing stuff).
> > >
> > > So in reality one does use a combination of LSM and tracing hooks
> > > with the policy decision coming from the LSM hook but the state
> > > gets accumulated from different hooks.
> > >
> > >
> > > > scope them by attach_btf_id.
> > > > Everything is 'tracing' like at the end.
> > > > Upcoming hid-bpf is 'tracing'. lsm is 'tracing'.
> > > > but we may need to reduce the scope of kfuncs
> > > > based on attach point or based on argument type.
> > > > tc vs xdp don't need to be separate sets.
> > > > Their types (skb vs xdp_md) are different, so only
> > > > right kfuncs can be used, but bpf_ct_release() is needed
> > > > in both tc and xdp.
> > > > So we could combine tc and xdp into 'btf_kfunc_hook_networking'
> > > > without compromising the safety.
> > > > acquire vs release ideally would be indicated with btf_tag,
> > > > but gcc still doesn't have support for btf_tag and we cannot
> > > > require the kernel to be built with clang.
> > > > acquire, release, sleepable, ret_null should be flags on a kfunc
> > > > instead of a set. It would be easier to manage and less boilerplate
> > >
> > > +1 This would be awesome, I gave it a shot to use btf_tag but hit this
> > > feature gap in GCC.
> > >
> > > - KP
> > >
> > > > code. Not clear how to do this cleanly.
> > > > export_symbol approach is a bit heavy and requires name being unique
> > > > across kernel and modules.
> > > > func name mangling or typedef-ing comes to mind. not so clean either.
> >
> > How does this approach look to 'tag' a kfunc?
> >
> > https://godbolt.org/z/jGeK6Y49K
> >
> > Then we find the function whose symbol should be emitted and available in BTF
> > with the prefix (__kfunc_name_) and read off the tag values. Due to the extern
> > inline it should still inline the definition, so there is no overhead at
> > runtime.
>
> No run-time overhead, but code size duplication is prohibitive.
> Both functions will have full body.
> We can hack around with another noinline function that both will call,
> but it's getting ugly.
>
> > As you see in godbolt, __foobar_1_2 symbol is visible, with 1 and 2 being tag
> > values. Substituting tag name with underlying value makes parsing easier. We
> > can have fixed number of slots and keep others as 0 (ignore).
> >
> > It can be extended to also tag arguments, which I already did before at [0], but
> > never sent out. Then we don't need the suffixes or special naming of arguments
> > to indicate some tag.
> >
> > e.g.
> >
> > bpf_ct_release will be:
> >
> > KFUNC_CALL_0(void, bpf_ct_release, __kfunc_release,
> >            KARG(struct nf_conn *, nfct, __ptr_ref))
> > {
> >       ...
> > }
>
> -struct nf_conn *
> -bpf_skb_ct_lookup(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,
> -                 u32 tuple__sz, struct bpf_ct_opts *opts, u32 opts__sz)
> +KFUNC_CALL_5(struct nf_conn *, bpf_skb_ct_lookup,
> +            KARG(struct __sk_buff *, skb_ctx, PTR_TO_CTX),
> +            KARG(struct bpf_sock_tuple *, bpf_tuple, PTR_TO_MEM),
> +            KARG(u32, tuple__sz, SIZE),
> +            KARG(struct bpf_ct_opts *, opts, PTR_TO_MEM),
> +            KARG(u32, opts__sz, SIZE))
>
> I think it's too ugly for majority of kernel folks to accept.

I agree. It's quite a creative idea :D

but will pollute the symbol table a lot.

> We will have a lot more kfuncs than we have now. Above code will be
> present in many different subsystems. The trade-off is not worth it.
> __sz suffix convention works. Similar approach can be used.
> This is orthogonal discussion anyway.
>
> Back to kfunc tagging.
> How about we use
>   BTF_SET_START8(list)
>   BTF_ID_FLAGS(func, bpf_xdp_ct_lookup, ACQUIRE | RET_NULL)
>   BTF_ID_FLAGS(func, bpf_ct_release, RELEASE)
>   BTF_SET_END8(list)

SGTM. Kumar is this something you can send a patch for?

Also, while we are at it, it would be great documenting what these sets
are in:

https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/include/linux/btf.h#n31

- KP




>
> where BTF_SET_START8 macro will do "__BTF_ID__set8__" #name
> so that resolve_btfid side knows that it needs to ignore 4 bytes after btf_id
> when it's doing set sorting.
> The kernel side btf_id_set_contains() would need to call bsearch()
> with sizeof(u64) instead of sizeof(u32).
>
> The verifier side will be cleaner and faster too.
> Instead of two bsearch calls (that are about to become 3 calls with sleepable set):
>                 rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
>                                                 BTF_KFUNC_TYPE_RELEASE, func_id);
>                 kptr_get = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
>                                                      BTF_KFUNC_TYPE_KPTR_ACQUIRE, func_id);
> It will be a single call plus flag check.

Re: [PATCH v2 bpf-next 3/5] bpf: Allow kfuncs to be used in LSM programs

[Kumar Kartikeya Dwivedi]

On Wed, Jun 22, 2022 at 01:55:29AM IST, KP Singh wrote:
> On Tue, Jun 21, 2022 at 10:12 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Jun 21, 2022 at 11:05:45PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > On Tue, Jun 21, 2022 at 09:45:15PM IST, KP Singh wrote:
> > > > On Tue, Jun 21, 2022 at 6:01 PM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jun 21, 2022 at 5:36 AM Kumar Kartikeya Dwivedi
> > > > > <memxor@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 21, 2022 at 06:58:09AM IST, KP Singh wrote:
> > > > > > > In preparation for the addition of bpf_getxattr kfunc.
> > > > > > >
> > > > > > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > > >
> > > > [...]
> > > >
> > > > > > > +++ b/kernel/bpf/btf.c
> > > > > > > @@ -7264,6 +7264,7 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
> > > > > > >       case BPF_PROG_TYPE_STRUCT_OPS:
> > > > > > >               return BTF_KFUNC_HOOK_STRUCT_OPS;
> > > > > > >       case BPF_PROG_TYPE_TRACING:
> > > > > > > +     case BPF_PROG_TYPE_LSM:
> > > > > > >               return BTF_KFUNC_HOOK_TRACING;
> > > > > >
> > > > > > Should we define another BTF_KFUNC_HOOK_LSM instead? Otherwise when you register
> > > > > > for tracing or lsm progs, you write to the same hook instead, so kfunc enabled
> > > > > > for tracing progs also gets enabled for lsm, I guess that is not what user
> > > > > > intends when registering kfunc set.
> > > > >
> > > > > It's probably ok for this case.
> > > > > We might combine BTF_KFUNC_HOOK* into fewer hooks and
> > > >
> > > > I did this intentionally. We do this similarly for helpers too and we
> > > > unfortunately, we do realize that LSM hooks are not there in all
> > > > places (esp. where one is auditing stuff).
> > > >
> > > > So in reality one does use a combination of LSM and tracing hooks
> > > > with the policy decision coming from the LSM hook but the state
> > > > gets accumulated from different hooks.
> > > >
> > > >
> > > > > scope them by attach_btf_id.
> > > > > Everything is 'tracing' like at the end.
> > > > > Upcoming hid-bpf is 'tracing'. lsm is 'tracing'.
> > > > > but we may need to reduce the scope of kfuncs
> > > > > based on attach point or based on argument type.
> > > > > tc vs xdp don't need to be separate sets.
> > > > > Their types (skb vs xdp_md) are different, so only
> > > > > right kfuncs can be used, but bpf_ct_release() is needed
> > > > > in both tc and xdp.
> > > > > So we could combine tc and xdp into 'btf_kfunc_hook_networking'
> > > > > without compromising the safety.
> > > > > acquire vs release ideally would be indicated with btf_tag,
> > > > > but gcc still doesn't have support for btf_tag and we cannot
> > > > > require the kernel to be built with clang.
> > > > > acquire, release, sleepable, ret_null should be flags on a kfunc
> > > > > instead of a set. It would be easier to manage and less boilerplate
> > > >
> > > > +1 This would be awesome, I gave it a shot to use btf_tag but hit this
> > > > feature gap in GCC.
> > > >
> > > > - KP
> > > >
> > > > > code. Not clear how to do this cleanly.
> > > > > export_symbol approach is a bit heavy and requires name being unique
> > > > > across kernel and modules.
> > > > > func name mangling or typedef-ing comes to mind. not so clean either.
> > >
> > > How does this approach look to 'tag' a kfunc?
> > >
> > > https://godbolt.org/z/jGeK6Y49K
> > >
> > > Then we find the function whose symbol should be emitted and available in BTF
> > > with the prefix (__kfunc_name_) and read off the tag values. Due to the extern
> > > inline it should still inline the definition, so there is no overhead at
> > > runtime.
> >
> > No run-time overhead, but code size duplication is prohibitive.
> > Both functions will have full body.
> > We can hack around with another noinline function that both will call,
> > but it's getting ugly.
> >

You can just make the __foobar_1_2 noinline instead: https://godbolt.org/z/zT18h81T6

but anyway...

> > > As you see in godbolt, __foobar_1_2 symbol is visible, with 1 and 2 being tag
> > > values. Substituting tag name with underlying value makes parsing easier. We
> > > can have fixed number of slots and keep others as 0 (ignore).
> > >
> > > It can be extended to also tag arguments, which I already did before at [0], but
> > > never sent out. Then we don't need the suffixes or special naming of arguments
> > > to indicate some tag.
> > >
> > > e.g.
> > >
> > > bpf_ct_release will be:
> > >
> > > KFUNC_CALL_0(void, bpf_ct_release, __kfunc_release,
> > >            KARG(struct nf_conn *, nfct, __ptr_ref))
> > > {
> > >       ...
> > > }
> >
> > -struct nf_conn *
> > -bpf_skb_ct_lookup(struct __sk_buff *skb_ctx, struct bpf_sock_tuple *bpf_tuple,
> > -                 u32 tuple__sz, struct bpf_ct_opts *opts, u32 opts__sz)
> > +KFUNC_CALL_5(struct nf_conn *, bpf_skb_ct_lookup,
> > +            KARG(struct __sk_buff *, skb_ctx, PTR_TO_CTX),
> > +            KARG(struct bpf_sock_tuple *, bpf_tuple, PTR_TO_MEM),
> > +            KARG(u32, tuple__sz, SIZE),
> > +            KARG(struct bpf_ct_opts *, opts, PTR_TO_MEM),
> > +            KARG(u32, opts__sz, SIZE))
> >
> > I think it's too ugly for majority of kernel folks to accept.
>
> I agree. It's quite a creative idea :D
>
> but will pollute the symbol table a lot.
>
> > We will have a lot more kfuncs than we have now. Above code will be
> > present in many different subsystems. The trade-off is not worth it.
> > __sz suffix convention works. Similar approach can be used.
> > This is orthogonal discussion anyway.
> >
> > Back to kfunc tagging.
> > How about we use
> >   BTF_SET_START8(list)
> >   BTF_ID_FLAGS(func, bpf_xdp_ct_lookup, ACQUIRE | RET_NULL)
> >   BTF_ID_FLAGS(func, bpf_ct_release, RELEASE)
> >   BTF_SET_END8(list)
>
> SGTM. Kumar is this something you can send a patch for?
>
> Also, while we are at it, it would be great documenting what these sets
> are in:
>

Yes, the resolve_btfids method looks ok to me. I will send a patch for both of
these things.

> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/include/linux/btf.h#n31
>
> - KP
>
>
>
>
> >
> > where BTF_SET_START8 macro will do "__BTF_ID__set8__" #name
> > so that resolve_btfid side knows that it needs to ignore 4 bytes after btf_id
> > when it's doing set sorting.
> > The kernel side btf_id_set_contains() would need to call bsearch()
> > with sizeof(u64) instead of sizeof(u32).
> >
> > The verifier side will be cleaner and faster too.
> > Instead of two bsearch calls (that are about to become 3 calls with sleepable set):
> >                 rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> >                                                 BTF_KFUNC_TYPE_RELEASE, func_id);
> >                 kptr_get = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog),
> >                                                      BTF_KFUNC_TYPE_KPTR_ACQUIRE, func_id);
> > It will be a single call plus flag check.

--
Kartikeya

Re: [PATCH v2 bpf-next 2/5] bpf: kfunc support for ARG_PTR_TO_CONST_STR

[KP Singh]

On Tue, Jun 21, 2022 at 3:19 PM KP Singh <kpsingh@kernel.org> wrote:
>
> On Tue, Jun 21, 2022 at 8:04 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > On Mon, Jun 20, 2022 at 6:29 PM KP Singh <kpsingh@kernel.org> wrote:
> > >
> > > kfuncs can handle pointers to memory when the next argument is
> > > the size of the memory that can be read and verify these as
> > > ARG_CONST_SIZE_OR_ZERO
> > >
> > > Similarly add support for string constants (const char *) and
> > > verify it similar to ARG_PTR_TO_CONST_STR.
> > >
> > > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > > ---
> > >  include/linux/bpf_verifier.h |  2 +
> > >  kernel/bpf/btf.c             | 29 ++++++++++++
> > >  kernel/bpf/verifier.c        | 85 ++++++++++++++++++++----------------
> > >  3 files changed, 79 insertions(+), 37 deletions(-)
> > >
> > [...]
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 668ecf61649b..02d7951591ae 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -6162,6 +6162,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
> > >         return true;
> > >  }
> > >
> > > +static bool btf_param_is_const_str_ptr(const struct btf *btf,
> > > +                                      const struct btf_param *param)
> > > +{
> > > +       const struct btf_type *t;
> > > +
> > > +       t = btf_type_by_id(btf, param->type);
> > > +       if (!btf_type_is_ptr(t))
> > > +               return false;
> > > +
> > > +       t = btf_type_by_id(btf, t->type);
> > > +       if (!(BTF_INFO_KIND(t->info) == BTF_KIND_CONST))
> > "if (BTF_INFO_KIND(t->info) != BTF_KIND_CONST)" looks clearer to me
> > > +               return false;
> > > +
> > > +       t = btf_type_skip_modifiers(btf, t->type, NULL);
> > > +       if (!strcmp(btf_name_by_offset(btf, t->name_off), "char"))
> > "return !strcmp(btf_name_by_offset(btf, t->name_off), "char")" looks
> > clearer to me here too
>
> Agreed. Updated.
>
> > > +               return true;
> > > +
> > > +       return false;
> > > +}
> > > +
> > >  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >                                     const struct btf *btf, u32 func_id,
> > >                                     struct bpf_reg_state *regs,
> > > @@ -6344,6 +6364,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >                 } else if (ptr_to_mem_ok) {
> > >                         const struct btf_type *resolve_ret;
> > >                         u32 type_size;
> > > +                       int err;
> > >
> > >                         if (is_kfunc) {
> > >                                 bool arg_mem_size = i + 1 < nargs && is_kfunc_arg_mem_size(btf, &args[i + 1], &regs[regno + 1]);
> > > @@ -6354,6 +6375,14 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >                                  * When arg_mem_size is true, the pointer can be
> > >                                  * void *.
> > >                                  */
> > > +                               if (btf_param_is_const_str_ptr(btf, &args[i])) {
> > > +                                       err = check_const_str(env, reg, regno);
> > > +                                       if (err < 0)
> > > +                                               return err;
> > > +                                       i++;
> > > +                                       continue;
> > If I'm understanding it correctly, this patch is intended to allow
> > helper functions to take in a kfunc as an arg as long as the next arg
> > is the size of the memory. Do we need to check the memory size access
> > here (eg like a call to check_mem_size_reg() in the verifier) to
> > ensure that memory accesses of that size are safe?

I see what confused you, it's the i++ that's incorrectly added here. Kumar
spotted it in my next rev.

>
> No, this is different. We already have the verification for where we pair a
> void * pointer to a size argument in the next arg.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/btf.c#n6366
>
> This logic is similar to the verification we do for ARG_PTR_TO_CONST_STR where
> we do not need a matching size argument and we just check for a null
> terminated string
> passed via a R/O map.
>
>
> > > +                               }
> > > +
> > >                                 if (!btf_type_is_scalar(ref_t) &&
> > >                                     !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
> > >                                     (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 2859901ffbe3..14a434792d7b 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -5840,6 +5840,52 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
> > >         return state->stack[spi].spilled_ptr.id;
> > >  }
> > [...]
> > > +
> > >  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > >                           struct bpf_call_arg_meta *meta,
> > >                           const struct bpf_func_proto *fn)
> > > @@ -6074,44 +6120,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > >                         return err;
> > >                 err = check_ptr_alignment(env, reg, 0, size, true);
> > >         } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > > -               struct bpf_map *map = reg->map_ptr;
> > > -               int map_off;
> > > -               u64 map_addr;
> > > -               char *str_ptr;
> > > -
> > > -               if (!bpf_map_is_rdonly(map)) {
> > > -                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > > -                       return -EACCES;
> > > -               }
> > > -
> > > -               if (!tnum_is_const(reg->var_off)) {
> > > -                       verbose(env, "R%d is not a constant address'\n", regno);
> > > -                       return -EACCES;
> > > -               }
> > > -
> > > -               if (!map->ops->map_direct_value_addr) {
> > > -                       verbose(env, "no direct value access support for this map type\n");
> > > -                       return -EACCES;
> > > -               }
> > > -
> > > -               err = check_map_access(env, regno, reg->off,
> > > -                                      map->value_size - reg->off, false,
> > > -                                      ACCESS_HELPER);
> > > -               if (err)
> > > -                       return err;
> > > -
> > > -               map_off = reg->off + reg->var_off.value;
> > > -               err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
> > > -               if (err) {
> > > -                       verbose(env, "direct value access on string failed\n");
> > > +               err = check_const_str(env, reg, regno);
> > > +               if (err < 0)
> > >                         return err;
> > nit: I don't think you need the if check here since thsi function will
> > return err automatically in the next line
>
> Makes sense. Fixed.
>
> >
> > > -               }
> > > -
> > > -               str_ptr = (char *)(long)(map_addr);
> > > -               if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) {
> > > -                       verbose(env, "string is not zero-terminated\n");
> > > -                       return -EINVAL;
> > > -               }
> > >         } else if (arg_type == ARG_PTR_TO_KPTR) {
> > >                 if (process_kptr_func(env, regno, meta))
> > >                         return -EACCES;
> > > --
> > > 2.37.0.rc0.104.g0611611a94-goog
> > >