[PATCH bpf-next v1 0/2] Add __ref annotation for kfuncs

[PATCH bpf-next v1 0/2] Add __ref annotation for kfuncs

[Kumar Kartikeya Dwivedi]

We decided to delay adding this until a usecase came up, and since now there is
one, instead of KF_TRUSTED_ARGS that applies to all arguments, add a __ref
suffix that has the same effect but only for the argument it is applied to.

Kumar Kartikeya Dwivedi (2):
  bpf: Add support for per-parameter trusted args
  selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation

 Documentation/bpf/kfuncs.rst                 | 18 +++++++++
 kernel/bpf/btf.c                             | 39 +++++++++++++-------
 net/bpf/test_run.c                           |  9 ++++-
 tools/testing/selftests/bpf/verifier/calls.c | 38 +++++++++++++++----
 4 files changed, 81 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Kumar Kartikeya Dwivedi]

Similar to how we detect mem, size pairs in kfunc, teach verifier to
treat __ref suffix on argument name to imply that it must be a trusted
arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
but limited to the specific parameter. This is required to ensure that
kfunc that operate on some object only work on acquired pointers and not
normal PTR_TO_BTF_ID with same type which can be obtained by pointer
walking. Release functions need not specify such suffix on release
arguments as they are already expected to receive one referenced
argument.

Cc: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
 kernel/bpf/btf.c             | 39 ++++++++++++++++++++++++------------
 net/bpf/test_run.c           |  9 +++++++--
 3 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
index c0b7dae6dbf5..41dff6337446 100644
--- a/Documentation/bpf/kfuncs.rst
+++ b/Documentation/bpf/kfuncs.rst
@@ -72,6 +72,24 @@ argument as its size. By default, without __sz annotation, the size of the type
 of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
 pointer.
 
+2.2.2 __ref Annotation
+----------------------
+
+This annotation is used to indicate that the argument is trusted, i.e. it will
+be a pointer from an acquire function (defined later), and its offset will be
+zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc flag but
+only on the parameter it is applied to. An example is shown below::
+
+        void bpf_task_send_signal(struct task_struct *task__ref, int signal)
+        {
+        ...
+        }
+
+Here, bpf_task_send_signal will only act on trusted task_struct pointers, and
+cannot be used on pointers obtained using pointer walking. This ensures that
+caller always calls this kfunc on a task whose lifetime is guaranteed for the
+duration of the call.
+
 .. _BPF_kfunc_nodef:
 
 2.3 Using an existing kernel function
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 7ac971ea98d1..3ce9b2deef9c 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
 	return true;
 }
 
-static bool is_kfunc_arg_mem_size(const struct btf *btf,
-				  const struct btf_param *arg,
-				  const struct bpf_reg_state *reg)
+static bool btf_param_match_suffix(const struct btf *btf,
+				   const struct btf_param *arg,
+				   const char *suffix)
 {
-	int len, sfx_len = sizeof("__sz") - 1;
-	const struct btf_type *t;
+	int len, sfx_len = strlen(suffix);
 	const char *param_name;
 
-	t = btf_type_skip_modifiers(btf, arg->type, NULL);
-	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
-		return false;
-
 	/* In the future, this can be ported to use BTF tagging */
 	param_name = btf_name_by_offset(btf, arg->name_off);
 	if (str_is_empty(param_name))
@@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const struct btf *btf,
 	if (len < sfx_len)
 		return false;
 	param_name += len - sfx_len;
-	if (strncmp(param_name, "__sz", sfx_len))
+	return !strncmp(param_name, suffix, sfx_len);
+}
+
+static bool is_kfunc_arg_ref(const struct btf *btf,
+			     const struct btf_param *arg)
+{
+	return btf_param_match_suffix(btf, arg, "__ref");
+}
+
+static bool is_kfunc_arg_mem_size(const struct btf *btf,
+				  const struct btf_param *arg,
+				  const struct bpf_reg_state *reg)
+{
+	const struct btf_type *t;
+
+	t = btf_type_skip_modifiers(btf, arg->type, NULL);
+	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
 		return false;
 
-	return true;
+	return btf_param_match_suffix(btf, arg, "__sz");
 }
 
 static int btf_check_func_arg_match(struct bpf_verifier_env *env,
@@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    u32 kfunc_flags)
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
-	bool rel = false, kptr_get = false, trusted_arg = false;
+	bool rel = false, kptr_get = false, kf_trusted_args = false;
 	struct bpf_verifier_log *log = &env->log;
 	u32 i, nargs, ref_id, ref_obj_id = 0;
 	bool is_kfunc = btf_is_kernel(btf);
@@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		/* Only kfunc can be release func */
 		rel = kfunc_flags & KF_RELEASE;
 		kptr_get = kfunc_flags & KF_KPTR_GET;
-		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
+		kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
 	}
 
 	/* check that BTF function arguments match actual types that the
@@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		enum bpf_arg_type arg_type = ARG_DONTCARE;
 		u32 regno = i + 1;
 		struct bpf_reg_state *reg = &regs[regno];
+		bool trusted_arg = false;
 
 		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
 		if (btf_type_is_scalar(t)) {
@@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		/* Check if argument must be a referenced pointer, args + i has
 		 * been verified to be a pointer (after skipping modifiers).
 		 */
+		trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
 		if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
 			bpf_log(log, "R%d must be referenced\n", regno);
 			return -EINVAL;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index cbc9cd5058cb..247bfe52e585 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -691,7 +691,11 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
 {
 }
 
-noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
+noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
+{
+}
+
+noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
 {
 }
 
@@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
 BTF_SET8_END(test_sk_check_kfunc_ids)
 
 static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
-- 
2.34.1

[PATCH bpf-next v1 2/2] selftests/bpf: Extend KF_TRUSTED_ARGS test for __ref annotation

[Kumar Kartikeya Dwivedi]

Extend the existing test for KF_TRUSTED_ARGS by also checking whether
the same happens when a __ref suffix is present in argument name of a
kfunc.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/testing/selftests/bpf/verifier/calls.c | 38 +++++++++++++++-----
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
index 3fb4f69b1962..891fcda50d9d 100644
--- a/tools/testing/selftests/bpf/verifier/calls.c
+++ b/tools/testing/selftests/bpf/verifier/calls.c
@@ -219,7 +219,7 @@
 	.errstr = "variable ptr_ access var_off=(0x0; 0x7) disallowed",
 },
 {
-	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID",
+	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (KF_TRUSTED_ARGS)",
 	.insns = {
 	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
 	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
@@ -227,10 +227,30 @@
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
 	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
 	BPF_EXIT_INSN(),
-	BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
-	BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16),
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
-	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_6, 16),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
+	.fixup_kfunc_btf_id = {
+		{ "bpf_kfunc_call_test_acquire", 3 },
+		{ "bpf_kfunc_call_test_trusted", 7 },
+	},
+	.result_unpriv = REJECT,
+	.result = REJECT,
+	.errstr = "R1 must be referenced",
+},
+{
+	"calls: invalid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID (__ref)",
+	.insns = {
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
+	BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+	BPF_EXIT_INSN(),
+	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 16),
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
 	BPF_MOV64_IMM(BPF_REG_0, 0),
 	BPF_EXIT_INSN(),
@@ -238,8 +258,7 @@
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 	.fixup_kfunc_btf_id = {
 		{ "bpf_kfunc_call_test_acquire", 3 },
-		{ "bpf_kfunc_call_test_ref", 8 },
-		{ "bpf_kfunc_call_test_ref", 10 },
+		{ "bpf_kfunc_call_test_ref", 7 },
 	},
 	.result_unpriv = REJECT,
 	.result = REJECT,
@@ -259,14 +278,17 @@
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
 	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
 	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
+	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
 	BPF_MOV64_IMM(BPF_REG_0, 0),
 	BPF_EXIT_INSN(),
 	},
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 	.fixup_kfunc_btf_id = {
 		{ "bpf_kfunc_call_test_acquire", 3 },
-		{ "bpf_kfunc_call_test_ref", 8 },
-		{ "bpf_kfunc_call_test_release", 10 },
+		{ "bpf_kfunc_call_test_trusted", 8 },
+		{ "bpf_kfunc_call_test_ref", 10 },
+		{ "bpf_kfunc_call_test_release", 12 },
 	},
 	.result_unpriv = REJECT,
 	.result = ACCEPT,
-- 
2.34.1

RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Roberto Sassu]

> From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> Sent: Wednesday, July 27, 2022 10:16 AM
> Similar to how we detect mem, size pairs in kfunc, teach verifier to
> treat __ref suffix on argument name to imply that it must be a trusted
> arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
> but limited to the specific parameter. This is required to ensure that
> kfunc that operate on some object only work on acquired pointers and not
> normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> walking. Release functions need not specify such suffix on release
> arguments as they are already expected to receive one referenced
> argument.

Thanks, Kumar. I will try it.

Roberto

> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
>  kernel/bpf/btf.c             | 39 ++++++++++++++++++++++++------------
>  net/bpf/test_run.c           |  9 +++++++--
>  3 files changed, 51 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> index c0b7dae6dbf5..41dff6337446 100644
> --- a/Documentation/bpf/kfuncs.rst
> +++ b/Documentation/bpf/kfuncs.rst
> @@ -72,6 +72,24 @@ argument as its size. By default, without __sz annotation,
> the size of the type
>  of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
>  pointer.
> 
> +2.2.2 __ref Annotation
> +----------------------
> +
> +This annotation is used to indicate that the argument is trusted, i.e. it will
> +be a pointer from an acquire function (defined later), and its offset will be
> +zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc flag
> but
> +only on the parameter it is applied to. An example is shown below::
> +
> +        void bpf_task_send_signal(struct task_struct *task__ref, int signal)
> +        {
> +        ...
> +        }
> +
> +Here, bpf_task_send_signal will only act on trusted task_struct pointers, and
> +cannot be used on pointers obtained using pointer walking. This ensures that
> +caller always calls this kfunc on a task whose lifetime is guaranteed for the
> +duration of the call.
> +
>  .. _BPF_kfunc_nodef:
> 
>  2.3 Using an existing kernel function
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 7ac971ea98d1..3ce9b2deef9c 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct
> bpf_verifier_log *log,
>  	return true;
>  }
> 
> -static bool is_kfunc_arg_mem_size(const struct btf *btf,
> -				  const struct btf_param *arg,
> -				  const struct bpf_reg_state *reg)
> +static bool btf_param_match_suffix(const struct btf *btf,
> +				   const struct btf_param *arg,
> +				   const char *suffix)
>  {
> -	int len, sfx_len = sizeof("__sz") - 1;
> -	const struct btf_type *t;
> +	int len, sfx_len = strlen(suffix);
>  	const char *param_name;
> 
> -	t = btf_type_skip_modifiers(btf, arg->type, NULL);
> -	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> -		return false;
> -
>  	/* In the future, this can be ported to use BTF tagging */
>  	param_name = btf_name_by_offset(btf, arg->name_off);
>  	if (str_is_empty(param_name))
> @@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const struct btf
> *btf,
>  	if (len < sfx_len)
>  		return false;
>  	param_name += len - sfx_len;
> -	if (strncmp(param_name, "__sz", sfx_len))
> +	return !strncmp(param_name, suffix, sfx_len);
> +}
> +
> +static bool is_kfunc_arg_ref(const struct btf *btf,
> +			     const struct btf_param *arg)
> +{
> +	return btf_param_match_suffix(btf, arg, "__ref");
> +}
> +
> +static bool is_kfunc_arg_mem_size(const struct btf *btf,
> +				  const struct btf_param *arg,
> +				  const struct bpf_reg_state *reg)
> +{
> +	const struct btf_type *t;
> +
> +	t = btf_type_skip_modifiers(btf, arg->type, NULL);
> +	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
>  		return false;
> 
> -	return true;
> +	return btf_param_match_suffix(btf, arg, "__sz");
>  }
> 
>  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> @@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct
> bpf_verifier_env *env,
>  				    u32 kfunc_flags)
>  {
>  	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> -	bool rel = false, kptr_get = false, trusted_arg = false;
> +	bool rel = false, kptr_get = false, kf_trusted_args = false;
>  	struct bpf_verifier_log *log = &env->log;
>  	u32 i, nargs, ref_id, ref_obj_id = 0;
>  	bool is_kfunc = btf_is_kernel(btf);
> @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct
> bpf_verifier_env *env,
>  		/* Only kfunc can be release func */
>  		rel = kfunc_flags & KF_RELEASE;
>  		kptr_get = kfunc_flags & KF_KPTR_GET;
> -		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> +		kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
>  	}
> 
>  	/* check that BTF function arguments match actual types that the
> @@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct
> bpf_verifier_env *env,
>  		enum bpf_arg_type arg_type = ARG_DONTCARE;
>  		u32 regno = i + 1;
>  		struct bpf_reg_state *reg = &regs[regno];
> +		bool trusted_arg = false;
> 
>  		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
>  		if (btf_type_is_scalar(t)) {
> @@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct
> bpf_verifier_env *env,
>  		/* Check if argument must be a referenced pointer, args + i has
>  		 * been verified to be a pointer (after skipping modifiers).
>  		 */
> +		trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
>  		if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
>  			bpf_log(log, "R%d must be referenced\n", regno);
>  			return -EINVAL;
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index cbc9cd5058cb..247bfe52e585 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -691,7 +691,11 @@ noinline void bpf_kfunc_call_test_mem_len_fail2(u64
> *mem, int len)
>  {
>  }
> 
> -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
> +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
> +{
> +}
> +
> +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
>  {
>  }
> 
> @@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
>  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
>  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
>  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
> -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
>  BTF_SET8_END(test_sk_check_kfunc_ids)
> 
>  static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
> --
> 2.34.1

RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Roberto Sassu]

> From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> Sent: Thursday, July 28, 2022 9:46 AM
> > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > Sent: Wednesday, July 27, 2022 10:16 AM
> > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > treat __ref suffix on argument name to imply that it must be a trusted
> > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
> > but limited to the specific parameter. This is required to ensure that
> > kfunc that operate on some object only work on acquired pointers and not
> > normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> > walking. Release functions need not specify such suffix on release
> > arguments as they are already expected to receive one referenced
> > argument.
> 
> Thanks, Kumar. I will try it.

Uhm. I realized that I was already using another suffix,
__maybe_null, to indicate that a caller can pass NULL as
argument.

Wouldn't probably work well with two suffixes.

Have you considered to extend BTF_ID_FLAGS to take five
extra arguments, to set flags for each kfunc parameter?

Thanks

Roberto

> Roberto
> 
> > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
> >  kernel/bpf/btf.c             | 39 ++++++++++++++++++++++++------------
> >  net/bpf/test_run.c           |  9 +++++++--
> >  3 files changed, 51 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > index c0b7dae6dbf5..41dff6337446 100644
> > --- a/Documentation/bpf/kfuncs.rst
> > +++ b/Documentation/bpf/kfuncs.rst
> > @@ -72,6 +72,24 @@ argument as its size. By default, without __sz
> annotation,
> > the size of the type
> >  of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
> >  pointer.
> >
> > +2.2.2 __ref Annotation
> > +----------------------
> > +
> > +This annotation is used to indicate that the argument is trusted, i.e. it will
> > +be a pointer from an acquire function (defined later), and its offset will be
> > +zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc
> flag
> > but
> > +only on the parameter it is applied to. An example is shown below::
> > +
> > +        void bpf_task_send_signal(struct task_struct *task__ref, int signal)
> > +        {
> > +        ...
> > +        }
> > +
> > +Here, bpf_task_send_signal will only act on trusted task_struct pointers, and
> > +cannot be used on pointers obtained using pointer walking. This ensures that
> > +caller always calls this kfunc on a task whose lifetime is guaranteed for the
> > +duration of the call.
> > +
> >  .. _BPF_kfunc_nodef:
> >
> >  2.3 Using an existing kernel function
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 7ac971ea98d1..3ce9b2deef9c 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct
> > bpf_verifier_log *log,
> >  	return true;
> >  }
> >
> > -static bool is_kfunc_arg_mem_size(const struct btf *btf,
> > -				  const struct btf_param *arg,
> > -				  const struct bpf_reg_state *reg)
> > +static bool btf_param_match_suffix(const struct btf *btf,
> > +				   const struct btf_param *arg,
> > +				   const char *suffix)
> >  {
> > -	int len, sfx_len = sizeof("__sz") - 1;
> > -	const struct btf_type *t;
> > +	int len, sfx_len = strlen(suffix);
> >  	const char *param_name;
> >
> > -	t = btf_type_skip_modifiers(btf, arg->type, NULL);
> > -	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> > -		return false;
> > -
> >  	/* In the future, this can be ported to use BTF tagging */
> >  	param_name = btf_name_by_offset(btf, arg->name_off);
> >  	if (str_is_empty(param_name))
> > @@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const struct
> btf
> > *btf,
> >  	if (len < sfx_len)
> >  		return false;
> >  	param_name += len - sfx_len;
> > -	if (strncmp(param_name, "__sz", sfx_len))
> > +	return !strncmp(param_name, suffix, sfx_len);
> > +}
> > +
> > +static bool is_kfunc_arg_ref(const struct btf *btf,
> > +			     const struct btf_param *arg)
> > +{
> > +	return btf_param_match_suffix(btf, arg, "__ref");
> > +}
> > +
> > +static bool is_kfunc_arg_mem_size(const struct btf *btf,
> > +				  const struct btf_param *arg,
> > +				  const struct bpf_reg_state *reg)
> > +{
> > +	const struct btf_type *t;
> > +
> > +	t = btf_type_skip_modifiers(btf, arg->type, NULL);
> > +	if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> >  		return false;
> >
> > -	return true;
> > +	return btf_param_match_suffix(btf, arg, "__sz");
> >  }
> >
> >  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > @@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> >  				    u32 kfunc_flags)
> >  {
> >  	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> > -	bool rel = false, kptr_get = false, trusted_arg = false;
> > +	bool rel = false, kptr_get = false, kf_trusted_args = false;
> >  	struct bpf_verifier_log *log = &env->log;
> >  	u32 i, nargs, ref_id, ref_obj_id = 0;
> >  	bool is_kfunc = btf_is_kernel(btf);
> > @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> >  		/* Only kfunc can be release func */
> >  		rel = kfunc_flags & KF_RELEASE;
> >  		kptr_get = kfunc_flags & KF_KPTR_GET;
> > -		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> > +		kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
> >  	}
> >
> >  	/* check that BTF function arguments match actual types that the
> > @@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> >  		enum bpf_arg_type arg_type = ARG_DONTCARE;
> >  		u32 regno = i + 1;
> >  		struct bpf_reg_state *reg = &regs[regno];
> > +		bool trusted_arg = false;
> >
> >  		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
> >  		if (btf_type_is_scalar(t)) {
> > @@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct
> > bpf_verifier_env *env,
> >  		/* Check if argument must be a referenced pointer, args + i has
> >  		 * been verified to be a pointer (after skipping modifiers).
> >  		 */
> > +		trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
> >  		if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
> >  			bpf_log(log, "R%d must be referenced\n", regno);
> >  			return -EINVAL;
> > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > index cbc9cd5058cb..247bfe52e585 100644
> > --- a/net/bpf/test_run.c
> > +++ b/net/bpf/test_run.c
> > @@ -691,7 +691,11 @@ noinline void
> bpf_kfunc_call_test_mem_len_fail2(u64
> > *mem, int len)
> >  {
> >  }
> >
> > -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
> > +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
> > +{
> > +}
> > +
> > +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
> >  {
> >  }
> >
> > @@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
> >  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
> >  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
> >  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
> > -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
> > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
> > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
> >  BTF_SET8_END(test_sk_check_kfunc_ids)
> >
> >  static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
> > --
> > 2.34.1

Re: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Kumar Kartikeya Dwivedi]

On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > Sent: Thursday, July 28, 2022 9:46 AM
> > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > Sent: Wednesday, July 27, 2022 10:16 AM
> > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > treat __ref suffix on argument name to imply that it must be a trusted
> > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
> > > but limited to the specific parameter. This is required to ensure that
> > > kfunc that operate on some object only work on acquired pointers and not
> > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> > > walking. Release functions need not specify such suffix on release
> > > arguments as they are already expected to receive one referenced
> > > argument.
> >
> > Thanks, Kumar. I will try it.
>
> Uhm. I realized that I was already using another suffix,
> __maybe_null, to indicate that a caller can pass NULL as
> argument.
>
> Wouldn't probably work well with two suffixes.
>

Then you can maybe extend it to parse two suffixes at most (for now atleast)?

> Have you considered to extend BTF_ID_FLAGS to take five
> extra arguments, to set flags for each kfunc parameter?
>

I didn't understand this. Flags parameter is an OR of the flags you
set, why would we want to extend it to take 5 args?
You can just or f1 | f2 | f3 | f4 | f5, as many as you want.

> Thanks
>
> Roberto
>
> > Roberto
> >
> > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > >  Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
> > >  kernel/bpf/btf.c             | 39 ++++++++++++++++++++++++------------
> > >  net/bpf/test_run.c           |  9 +++++++--
> > >  3 files changed, 51 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > > index c0b7dae6dbf5..41dff6337446 100644
> > > --- a/Documentation/bpf/kfuncs.rst
> > > +++ b/Documentation/bpf/kfuncs.rst
> > > @@ -72,6 +72,24 @@ argument as its size. By default, without __sz
> > annotation,
> > > the size of the type
> > >  of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
> > >  pointer.
> > >
> > > +2.2.2 __ref Annotation
> > > +----------------------
> > > +
> > > +This annotation is used to indicate that the argument is trusted, i.e. it will
> > > +be a pointer from an acquire function (defined later), and its offset will be
> > > +zero. This annotation has the same effect as the KF_TRUSTED_ARGS kfunc
> > flag
> > > but
> > > +only on the parameter it is applied to. An example is shown below::
> > > +
> > > +        void bpf_task_send_signal(struct task_struct *task__ref, int signal)
> > > +        {
> > > +        ...
> > > +        }
> > > +
> > > +Here, bpf_task_send_signal will only act on trusted task_struct pointers, and
> > > +cannot be used on pointers obtained using pointer walking. This ensures that
> > > +caller always calls this kfunc on a task whose lifetime is guaranteed for the
> > > +duration of the call.
> > > +
> > >  .. _BPF_kfunc_nodef:
> > >
> > >  2.3 Using an existing kernel function
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 7ac971ea98d1..3ce9b2deef9c 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct
> > > bpf_verifier_log *log,
> > >     return true;
> > >  }
> > >
> > > -static bool is_kfunc_arg_mem_size(const struct btf *btf,
> > > -                             const struct btf_param *arg,
> > > -                             const struct bpf_reg_state *reg)
> > > +static bool btf_param_match_suffix(const struct btf *btf,
> > > +                              const struct btf_param *arg,
> > > +                              const char *suffix)
> > >  {
> > > -   int len, sfx_len = sizeof("__sz") - 1;
> > > -   const struct btf_type *t;
> > > +   int len, sfx_len = strlen(suffix);
> > >     const char *param_name;
> > >
> > > -   t = btf_type_skip_modifiers(btf, arg->type, NULL);
> > > -   if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> > > -           return false;
> > > -
> > >     /* In the future, this can be ported to use BTF tagging */
> > >     param_name = btf_name_by_offset(btf, arg->name_off);
> > >     if (str_is_empty(param_name))
> > > @@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const struct
> > btf
> > > *btf,
> > >     if (len < sfx_len)
> > >             return false;
> > >     param_name += len - sfx_len;
> > > -   if (strncmp(param_name, "__sz", sfx_len))
> > > +   return !strncmp(param_name, suffix, sfx_len);
> > > +}
> > > +
> > > +static bool is_kfunc_arg_ref(const struct btf *btf,
> > > +                        const struct btf_param *arg)
> > > +{
> > > +   return btf_param_match_suffix(btf, arg, "__ref");
> > > +}
> > > +
> > > +static bool is_kfunc_arg_mem_size(const struct btf *btf,
> > > +                             const struct btf_param *arg,
> > > +                             const struct bpf_reg_state *reg)
> > > +{
> > > +   const struct btf_type *t;
> > > +
> > > +   t = btf_type_skip_modifiers(btf, arg->type, NULL);
> > > +   if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> > >             return false;
> > >
> > > -   return true;
> > > +   return btf_param_match_suffix(btf, arg, "__sz");
> > >  }
> > >
> > >  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > @@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > >                                 u32 kfunc_flags)
> > >  {
> > >     enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> > > -   bool rel = false, kptr_get = false, trusted_arg = false;
> > > +   bool rel = false, kptr_get = false, kf_trusted_args = false;
> > >     struct bpf_verifier_log *log = &env->log;
> > >     u32 i, nargs, ref_id, ref_obj_id = 0;
> > >     bool is_kfunc = btf_is_kernel(btf);
> > > @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > >             /* Only kfunc can be release func */
> > >             rel = kfunc_flags & KF_RELEASE;
> > >             kptr_get = kfunc_flags & KF_KPTR_GET;
> > > -           trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> > > +           kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
> > >     }
> > >
> > >     /* check that BTF function arguments match actual types that the
> > > @@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > >             enum bpf_arg_type arg_type = ARG_DONTCARE;
> > >             u32 regno = i + 1;
> > >             struct bpf_reg_state *reg = &regs[regno];
> > > +           bool trusted_arg = false;
> > >
> > >             t = btf_type_skip_modifiers(btf, args[i].type, NULL);
> > >             if (btf_type_is_scalar(t)) {
> > > @@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct
> > > bpf_verifier_env *env,
> > >             /* Check if argument must be a referenced pointer, args + i has
> > >              * been verified to be a pointer (after skipping modifiers).
> > >              */
> > > +           trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
> > >             if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
> > >                     bpf_log(log, "R%d must be referenced\n", regno);
> > >                     return -EINVAL;
> > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > > index cbc9cd5058cb..247bfe52e585 100644
> > > --- a/net/bpf/test_run.c
> > > +++ b/net/bpf/test_run.c
> > > @@ -691,7 +691,11 @@ noinline void
> > bpf_kfunc_call_test_mem_len_fail2(u64
> > > *mem, int len)
> > >  {
> > >  }
> > >
> > > -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
> > > +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
> > > +{
> > > +}
> > > +
> > > +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
> > >  {
> > >  }
> > >
> > > @@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
> > >  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
> > >  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
> > >  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
> > > -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
> > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
> > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
> > >  BTF_SET8_END(test_sk_check_kfunc_ids)
> > >
> > >  static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
> > > --
> > > 2.34.1
>

Re: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Kumar Kartikeya Dwivedi]

On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com> wrote:
> >
> > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > > Sent: Thursday, July 28, 2022 9:46 AM
> > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > Sent: Wednesday, July 27, 2022 10:16 AM
> > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > > treat __ref suffix on argument name to imply that it must be a trusted
> > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
> > > > but limited to the specific parameter. This is required to ensure that
> > > > kfunc that operate on some object only work on acquired pointers and not
> > > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> > > > walking. Release functions need not specify such suffix on release
> > > > arguments as they are already expected to receive one referenced
> > > > argument.
> > >
> > > Thanks, Kumar. I will try it.
> >
> > Uhm. I realized that I was already using another suffix,
> > __maybe_null, to indicate that a caller can pass NULL as
> > argument.
> >
> > Wouldn't probably work well with two suffixes.
> >
>
> Then you can maybe extend it to parse two suffixes at most (for now atleast)?
>
> > Have you considered to extend BTF_ID_FLAGS to take five
> > extra arguments, to set flags for each kfunc parameter?
> >
>
> I didn't understand this. Flags parameter is an OR of the flags you
> set, why would we want to extend it to take 5 args?
> You can just or f1 | f2 | f3 | f4 | f5, as many as you want.

Oh, so you mean having 5 more args to indicate flags on each
parameter? It is possible, but I think the scheme for now works ok. If
you extend it to parse two suffixes, it should be fine. Yes, the
variable name would be ugly, but you can just make a copy into a
properly named one. This is the best we can do without switching to
BTF tags. We can revisit this when we start having 4 or 5 tags on a
single parameter.

To make it a bit less verbose you could probably call maybe_null just null?

RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Roberto Sassu]

> From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> Sent: Thursday, July 28, 2022 10:45 AM
> On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > > Sent: Thursday, July 28, 2022 9:46 AM
> > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > Sent: Wednesday, July 27, 2022 10:16 AM
> > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > > treat __ref suffix on argument name to imply that it must be a trusted
> > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
> > > > but limited to the specific parameter. This is required to ensure that
> > > > kfunc that operate on some object only work on acquired pointers and not
> > > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> > > > walking. Release functions need not specify such suffix on release
> > > > arguments as they are already expected to receive one referenced
> > > > argument.
> > >
> > > Thanks, Kumar. I will try it.
> >
> > Uhm. I realized that I was already using another suffix,
> > __maybe_null, to indicate that a caller can pass NULL as
> > argument.
> >
> > Wouldn't probably work well with two suffixes.
> >
> 
> Then you can maybe extend it to parse two suffixes at most (for now atleast)?
> 
> > Have you considered to extend BTF_ID_FLAGS to take five
> > extra arguments, to set flags for each kfunc parameter?
> >
> 
> I didn't understand this. Flags parameter is an OR of the flags you
> set, why would we want to extend it to take 5 args?
> You can just or f1 | f2 | f3 | f4 | f5, as many as you want.

Instead of using the suffix as mechanism to set per-parameter
flags, I would do:

BTF_ID_FLAGS(func, bpf_verify_pkcs7_signature, KF_SLEEPABLE, 0, 0, KF_REF | KF_MAYBE_NULL, 0, 0)

where the first argument after the kfunc name is used for
function-wide flags, the second for first parameter flags, ...

Roberto

> > Thanks
> >
> > Roberto
> >
> > > Roberto
> > >
> > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > > ---
> > > >  Documentation/bpf/kfuncs.rst | 18 +++++++++++++++++
> > > >  kernel/bpf/btf.c             | 39 ++++++++++++++++++++++++------------
> > > >  net/bpf/test_run.c           |  9 +++++++--
> > > >  3 files changed, 51 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > > > index c0b7dae6dbf5..41dff6337446 100644
> > > > --- a/Documentation/bpf/kfuncs.rst
> > > > +++ b/Documentation/bpf/kfuncs.rst
> > > > @@ -72,6 +72,24 @@ argument as its size. By default, without __sz
> > > annotation,
> > > > the size of the type
> > > >  of the pointer is used. Without __sz annotation, a kfunc cannot accept a
> void
> > > >  pointer.
> > > >
> > > > +2.2.2 __ref Annotation
> > > > +----------------------
> > > > +
> > > > +This annotation is used to indicate that the argument is trusted, i.e. it will
> > > > +be a pointer from an acquire function (defined later), and its offset will be
> > > > +zero. This annotation has the same effect as the KF_TRUSTED_ARGS
> kfunc
> > > flag
> > > > but
> > > > +only on the parameter it is applied to. An example is shown below::
> > > > +
> > > > +        void bpf_task_send_signal(struct task_struct *task__ref, int signal)
> > > > +        {
> > > > +        ...
> > > > +        }
> > > > +
> > > > +Here, bpf_task_send_signal will only act on trusted task_struct pointers,
> and
> > > > +cannot be used on pointers obtained using pointer walking. This ensures
> that
> > > > +caller always calls this kfunc on a task whose lifetime is guaranteed for
> the
> > > > +duration of the call.
> > > > +
> > > >  .. _BPF_kfunc_nodef:
> > > >
> > > >  2.3 Using an existing kernel function
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index 7ac971ea98d1..3ce9b2deef9c 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -6141,18 +6141,13 @@ static bool __btf_type_is_scalar_struct(struct
> > > > bpf_verifier_log *log,
> > > >     return true;
> > > >  }
> > > >
> > > > -static bool is_kfunc_arg_mem_size(const struct btf *btf,
> > > > -                             const struct btf_param *arg,
> > > > -                             const struct bpf_reg_state *reg)
> > > > +static bool btf_param_match_suffix(const struct btf *btf,
> > > > +                              const struct btf_param *arg,
> > > > +                              const char *suffix)
> > > >  {
> > > > -   int len, sfx_len = sizeof("__sz") - 1;
> > > > -   const struct btf_type *t;
> > > > +   int len, sfx_len = strlen(suffix);
> > > >     const char *param_name;
> > > >
> > > > -   t = btf_type_skip_modifiers(btf, arg->type, NULL);
> > > > -   if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> > > > -           return false;
> > > > -
> > > >     /* In the future, this can be ported to use BTF tagging */
> > > >     param_name = btf_name_by_offset(btf, arg->name_off);
> > > >     if (str_is_empty(param_name))
> > > > @@ -6161,10 +6156,26 @@ static bool is_kfunc_arg_mem_size(const
> struct
> > > btf
> > > > *btf,
> > > >     if (len < sfx_len)
> > > >             return false;
> > > >     param_name += len - sfx_len;
> > > > -   if (strncmp(param_name, "__sz", sfx_len))
> > > > +   return !strncmp(param_name, suffix, sfx_len);
> > > > +}
> > > > +
> > > > +static bool is_kfunc_arg_ref(const struct btf *btf,
> > > > +                        const struct btf_param *arg)
> > > > +{
> > > > +   return btf_param_match_suffix(btf, arg, "__ref");
> > > > +}
> > > > +
> > > > +static bool is_kfunc_arg_mem_size(const struct btf *btf,
> > > > +                             const struct btf_param *arg,
> > > > +                             const struct bpf_reg_state *reg)
> > > > +{
> > > > +   const struct btf_type *t;
> > > > +
> > > > +   t = btf_type_skip_modifiers(btf, arg->type, NULL);
> > > > +   if (!btf_type_is_scalar(t) || reg->type != SCALAR_VALUE)
> > > >             return false;
> > > >
> > > > -   return true;
> > > > +   return btf_param_match_suffix(btf, arg, "__sz");
> > > >  }
> > > >
> > > >  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > > @@ -6174,7 +6185,7 @@ static int btf_check_func_arg_match(struct
> > > > bpf_verifier_env *env,
> > > >                                 u32 kfunc_flags)
> > > >  {
> > > >     enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> > > > -   bool rel = false, kptr_get = false, trusted_arg = false;
> > > > +   bool rel = false, kptr_get = false, kf_trusted_args = false;
> > > >     struct bpf_verifier_log *log = &env->log;
> > > >     u32 i, nargs, ref_id, ref_obj_id = 0;
> > > >     bool is_kfunc = btf_is_kernel(btf);
> > > > @@ -6211,7 +6222,7 @@ static int btf_check_func_arg_match(struct
> > > > bpf_verifier_env *env,
> > > >             /* Only kfunc can be release func */
> > > >             rel = kfunc_flags & KF_RELEASE;
> > > >             kptr_get = kfunc_flags & KF_KPTR_GET;
> > > > -           trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
> > > > +           kf_trusted_args = kfunc_flags & KF_TRUSTED_ARGS;
> > > >     }
> > > >
> > > >     /* check that BTF function arguments match actual types that the
> > > > @@ -6221,6 +6232,7 @@ static int btf_check_func_arg_match(struct
> > > > bpf_verifier_env *env,
> > > >             enum bpf_arg_type arg_type = ARG_DONTCARE;
> > > >             u32 regno = i + 1;
> > > >             struct bpf_reg_state *reg = &regs[regno];
> > > > +           bool trusted_arg = false;
> > > >
> > > >             t = btf_type_skip_modifiers(btf, args[i].type, NULL);
> > > >             if (btf_type_is_scalar(t)) {
> > > > @@ -6239,6 +6251,7 @@ static int btf_check_func_arg_match(struct
> > > > bpf_verifier_env *env,
> > > >             /* Check if argument must be a referenced pointer, args + i has
> > > >              * been verified to be a pointer (after skipping modifiers).
> > > >              */
> > > > +           trusted_arg = kf_trusted_args || is_kfunc_arg_ref(btf, args + i);
> > > >             if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
> > > >                     bpf_log(log, "R%d must be referenced\n", regno);
> > > >                     return -EINVAL;
> > > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > > > index cbc9cd5058cb..247bfe52e585 100644
> > > > --- a/net/bpf/test_run.c
> > > > +++ b/net/bpf/test_run.c
> > > > @@ -691,7 +691,11 @@ noinline void
> > > bpf_kfunc_call_test_mem_len_fail2(u64
> > > > *mem, int len)
> > > >  {
> > > >  }
> > > >
> > > > -noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
> > > > +noinline void bpf_kfunc_call_test_trusted(struct prog_test_ref_kfunc *p)
> > > > +{
> > > > +}
> > > > +
> > > > +noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p__ref)
> > > >  {
> > > >  }
> > > >
> > > > @@ -718,7 +722,8 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
> > > >  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
> > > >  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
> > > >  BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
> > > > -BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
> > > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_trusted, KF_TRUSTED_ARGS)
> > > > +BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref)
> > > >  BTF_SET8_END(test_sk_check_kfunc_ids)
> > > >
> > > >  static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
> > > > --
> > > > 2.34.1
> >

Re: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Alexei Starovoitov]

On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com> wrote:
> > >
> > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > > > Sent: Thursday, July 28, 2022 9:46 AM
> > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > > Sent: Wednesday, July 27, 2022 10:16 AM
> > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > > > treat __ref suffix on argument name to imply that it must be a trusted
> > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS flag
> > > > > but limited to the specific parameter. This is required to ensure that
> > > > > kfunc that operate on some object only work on acquired pointers and not
> > > > > normal PTR_TO_BTF_ID with same type which can be obtained by pointer
> > > > > walking. Release functions need not specify such suffix on release
> > > > > arguments as they are already expected to receive one referenced
> > > > > argument.
> > > >
> > > > Thanks, Kumar. I will try it.
> > >
> > > Uhm. I realized that I was already using another suffix,
> > > __maybe_null, to indicate that a caller can pass NULL as
> > > argument.
> > >
> > > Wouldn't probably work well with two suffixes.
> > >
> >
> > Then you can maybe extend it to parse two suffixes at most (for now atleast)?
> >
> > > Have you considered to extend BTF_ID_FLAGS to take five
> > > extra arguments, to set flags for each kfunc parameter?
> > >
> >
> > I didn't understand this. Flags parameter is an OR of the flags you
> > set, why would we want to extend it to take 5 args?
> > You can just or f1 | f2 | f3 | f4 | f5, as many as you want.
>
> Oh, so you mean having 5 more args to indicate flags on each
> parameter? It is possible, but I think the scheme for now works ok. If
> you extend it to parse two suffixes, it should be fine. Yes, the
> variable name would be ugly, but you can just make a copy into a
> properly named one. This is the best we can do without switching to
> BTF tags. We can revisit this when we start having 4 or 5 tags on a
> single parameter.
>
> To make it a bit less verbose you could probably call maybe_null just null?

Thank you for posting the patch.
It still feels that this extra flexibility gets convoluted.
I'm not sure Roberto's kfunc actually needs __ref.
All pointers should be pointers. Hacking -1 and -2 into a pointer
is something that key infra did, but it doesn't mean that
we have to carry over it into bpf kfunc.

RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Roberto Sassu]

> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> Sent: Tuesday, August 2, 2022 6:46 AM
> 
> On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@gmail.com>
> wrote:
> > >
> > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> > > >
> > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > > > > Sent: Thursday, July 28, 2022 9:46 AM
> > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > > > Sent: Wednesday, July 27, 2022 10:16 AM
> > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > > > > treat __ref suffix on argument name to imply that it must be a trusted
> > > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS
> flag
> > > > > > but limited to the specific parameter. This is required to ensure that
> > > > > > kfunc that operate on some object only work on acquired pointers and
> not
> > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by
> pointer
> > > > > > walking. Release functions need not specify such suffix on release
> > > > > > arguments as they are already expected to receive one referenced
> > > > > > argument.
> > > > >
> > > > > Thanks, Kumar. I will try it.
> > > >
> > > > Uhm. I realized that I was already using another suffix,
> > > > __maybe_null, to indicate that a caller can pass NULL as
> > > > argument.
> > > >
> > > > Wouldn't probably work well with two suffixes.
> > > >
> > >
> > > Then you can maybe extend it to parse two suffixes at most (for now
> atleast)?
> > >
> > > > Have you considered to extend BTF_ID_FLAGS to take five
> > > > extra arguments, to set flags for each kfunc parameter?
> > > >
> > >
> > > I didn't understand this. Flags parameter is an OR of the flags you
> > > set, why would we want to extend it to take 5 args?
> > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want.
> >
> > Oh, so you mean having 5 more args to indicate flags on each
> > parameter? It is possible, but I think the scheme for now works ok. If
> > you extend it to parse two suffixes, it should be fine. Yes, the
> > variable name would be ugly, but you can just make a copy into a
> > properly named one. This is the best we can do without switching to
> > BTF tags. We can revisit this when we start having 4 or 5 tags on a
> > single parameter.
> >
> > To make it a bit less verbose you could probably call maybe_null just null?
> 
> Thank you for posting the patch.
> It still feels that this extra flexibility gets convoluted.
> I'm not sure Roberto's kfunc actually needs __ref.
> All pointers should be pointers. Hacking -1 and -2 into a pointer
> is something that key infra did, but it doesn't mean that
> we have to carry over it into bpf kfunc.

There is a separate parameter for the keyring IDs that only
verify_pkcs7_signature() understands. Type casting is done
internally in the bpf_verify_pkcs7_signature() kfunc.
 
The other is always a valid struct key pointer or NULL, coming
from bpf_lookup_user_key() (acquire function). I extended
Kumar's patch further to annotate the struct key parameter
with the _ref_null suffix, to accept a referenced pointer or NULL,
instead of just referenced.

Roberto

Re: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Alexei Starovoitov]

On Tue, Aug 2, 2022 at 3:07 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > Sent: Tuesday, August 2, 2022 6:46 AM
> >
> > On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > wrote:
> > > >
> > > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > > > >
> > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > > > > > Sent: Thursday, July 28, 2022 9:46 AM
> > > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > > > > Sent: Wednesday, July 27, 2022 10:16 AM
> > > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > > > > > treat __ref suffix on argument name to imply that it must be a trusted
> > > > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS
> > flag
> > > > > > > but limited to the specific parameter. This is required to ensure that
> > > > > > > kfunc that operate on some object only work on acquired pointers and
> > not
> > > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by
> > pointer
> > > > > > > walking. Release functions need not specify such suffix on release
> > > > > > > arguments as they are already expected to receive one referenced
> > > > > > > argument.
> > > > > >
> > > > > > Thanks, Kumar. I will try it.
> > > > >
> > > > > Uhm. I realized that I was already using another suffix,
> > > > > __maybe_null, to indicate that a caller can pass NULL as
> > > > > argument.
> > > > >
> > > > > Wouldn't probably work well with two suffixes.
> > > > >
> > > >
> > > > Then you can maybe extend it to parse two suffixes at most (for now
> > atleast)?
> > > >
> > > > > Have you considered to extend BTF_ID_FLAGS to take five
> > > > > extra arguments, to set flags for each kfunc parameter?
> > > > >
> > > >
> > > > I didn't understand this. Flags parameter is an OR of the flags you
> > > > set, why would we want to extend it to take 5 args?
> > > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want.
> > >
> > > Oh, so you mean having 5 more args to indicate flags on each
> > > parameter? It is possible, but I think the scheme for now works ok. If
> > > you extend it to parse two suffixes, it should be fine. Yes, the
> > > variable name would be ugly, but you can just make a copy into a
> > > properly named one. This is the best we can do without switching to
> > > BTF tags. We can revisit this when we start having 4 or 5 tags on a
> > > single parameter.
> > >
> > > To make it a bit less verbose you could probably call maybe_null just null?
> >
> > Thank you for posting the patch.
> > It still feels that this extra flexibility gets convoluted.
> > I'm not sure Roberto's kfunc actually needs __ref.
> > All pointers should be pointers. Hacking -1 and -2 into a pointer
> > is something that key infra did, but it doesn't mean that
> > we have to carry over it into bpf kfunc.
>
> There is a separate parameter for the keyring IDs that only
> verify_pkcs7_signature() understands. Type casting is done
> internally in the bpf_verify_pkcs7_signature() kfunc.
>
> The other is always a valid struct key pointer or NULL, coming
> from bpf_lookup_user_key() (acquire function). I extended
> Kumar's patch further to annotate the struct key parameter
> with the _ref_null suffix, to accept a referenced pointer or NULL,
> instead of just referenced.

I don't think it's a good tradeoff complexity wise.
!=null check can be done in runtime by the helper.
The type cast is a sign of something fishy in the design.

RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Roberto Sassu]

> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> Sent: Tuesday, August 2, 2022 5:06 PM
> On Tue, Aug 2, 2022 at 3:07 AM Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > > Sent: Tuesday, August 2, 2022 6:46 AM
> > >
> > > On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi
> > > <memxor@gmail.com> wrote:
> > > >
> > > > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi
> <memxor@gmail.com>
> > > wrote:
> > > > >
> > > > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu
> <roberto.sassu@huawei.com>
> > > wrote:
> > > > > >
> > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > > > > > > Sent: Thursday, July 28, 2022 9:46 AM
> > > > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > > > > > Sent: Wednesday, July 27, 2022 10:16 AM
> > > > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > > > > > > treat __ref suffix on argument name to imply that it must be a
> trusted
> > > > > > > > arg when passed to kfunc, similar to the effect of
> KF_TRUSTED_ARGS
> > > flag
> > > > > > > > but limited to the specific parameter. This is required to ensure that
> > > > > > > > kfunc that operate on some object only work on acquired pointers
> and
> > > not
> > > > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by
> > > pointer
> > > > > > > > walking. Release functions need not specify such suffix on release
> > > > > > > > arguments as they are already expected to receive one referenced
> > > > > > > > argument.
> > > > > > >
> > > > > > > Thanks, Kumar. I will try it.
> > > > > >
> > > > > > Uhm. I realized that I was already using another suffix,
> > > > > > __maybe_null, to indicate that a caller can pass NULL as
> > > > > > argument.
> > > > > >
> > > > > > Wouldn't probably work well with two suffixes.
> > > > > >
> > > > >
> > > > > Then you can maybe extend it to parse two suffixes at most (for now
> > > atleast)?
> > > > >
> > > > > > Have you considered to extend BTF_ID_FLAGS to take five
> > > > > > extra arguments, to set flags for each kfunc parameter?
> > > > > >
> > > > >
> > > > > I didn't understand this. Flags parameter is an OR of the flags you
> > > > > set, why would we want to extend it to take 5 args?
> > > > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want.
> > > >
> > > > Oh, so you mean having 5 more args to indicate flags on each
> > > > parameter? It is possible, but I think the scheme for now works ok. If
> > > > you extend it to parse two suffixes, it should be fine. Yes, the
> > > > variable name would be ugly, but you can just make a copy into a
> > > > properly named one. This is the best we can do without switching to
> > > > BTF tags. We can revisit this when we start having 4 or 5 tags on a
> > > > single parameter.
> > > >
> > > > To make it a bit less verbose you could probably call maybe_null just null?
> > >
> > > Thank you for posting the patch.
> > > It still feels that this extra flexibility gets convoluted.
> > > I'm not sure Roberto's kfunc actually needs __ref.
> > > All pointers should be pointers. Hacking -1 and -2 into a pointer
> > > is something that key infra did, but it doesn't mean that
> > > we have to carry over it into bpf kfunc.
> >
> > There is a separate parameter for the keyring IDs that only
> > verify_pkcs7_signature() understands. Type casting is done
> > internally in the bpf_verify_pkcs7_signature() kfunc.
> >
> > The other is always a valid struct key pointer or NULL, coming
> > from bpf_lookup_user_key() (acquire function). I extended
> > Kumar's patch further to annotate the struct key parameter
> > with the _ref_null suffix, to accept a referenced pointer or NULL,
> > instead of just referenced.
> 
> I don't think it's a good tradeoff complexity wise.
> !=null check can be done in runtime by the helper.

Not sure I follow. bpf_lookup_user_key() might not find
the key you asked for, so it will return NULL.

Did you mean that I should not set KF_RET_NULL for
bpf_lookup_user_key()?

That anyway won’t help if you use the system keyring,
the alternative parameter. The user keyring is the other
one, returned by bpf_lookup_user_key().

When you specify a system keyring, the user keyring should
be NULL, to indicate that the parameter should not be
used. This was suggested by both John and Daniel.

So, it seems unavoidable to annotate the keyring parameter
with __ref_null, or at least with __null. __ref_null would be
better, to require a referenced parameter.

> The type cast is a sign of something fishy in the design.

Since this is what verify_pkcs7_signature() accepts, I guess
it is the only option.

Roberto

RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Roberto Sassu]

> From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> Sent: Tuesday, August 2, 2022 6:01 PM
> > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > Sent: Tuesday, August 2, 2022 5:06 PM
> > On Tue, Aug 2, 2022 at 3:07 AM Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > >
> > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > > > Sent: Tuesday, August 2, 2022 6:46 AM
> > > >
> > > > On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi
> > > > <memxor@gmail.com> wrote:
> > > > >
> > > > > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi
> > <memxor@gmail.com>
> > > > wrote:
> > > > > >
> > > > > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu
> > <roberto.sassu@huawei.com>
> > > > wrote:
> > > > > > >
> > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > > > > > > > Sent: Thursday, July 28, 2022 9:46 AM
> > > > > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > > > > > > Sent: Wednesday, July 27, 2022 10:16 AM
> > > > > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier
> to
> > > > > > > > > treat __ref suffix on argument name to imply that it must be a
> > trusted
> > > > > > > > > arg when passed to kfunc, similar to the effect of
> > KF_TRUSTED_ARGS
> > > > flag
> > > > > > > > > but limited to the specific parameter. This is required to ensure
> that
> > > > > > > > > kfunc that operate on some object only work on acquired pointers
> > and
> > > > not
> > > > > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by
> > > > pointer
> > > > > > > > > walking. Release functions need not specify such suffix on release
> > > > > > > > > arguments as they are already expected to receive one referenced
> > > > > > > > > argument.
> > > > > > > >
> > > > > > > > Thanks, Kumar. I will try it.
> > > > > > >
> > > > > > > Uhm. I realized that I was already using another suffix,
> > > > > > > __maybe_null, to indicate that a caller can pass NULL as
> > > > > > > argument.
> > > > > > >
> > > > > > > Wouldn't probably work well with two suffixes.
> > > > > > >
> > > > > >
> > > > > > Then you can maybe extend it to parse two suffixes at most (for now
> > > > atleast)?
> > > > > >
> > > > > > > Have you considered to extend BTF_ID_FLAGS to take five
> > > > > > > extra arguments, to set flags for each kfunc parameter?
> > > > > > >
> > > > > >
> > > > > > I didn't understand this. Flags parameter is an OR of the flags you
> > > > > > set, why would we want to extend it to take 5 args?
> > > > > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want.
> > > > >
> > > > > Oh, so you mean having 5 more args to indicate flags on each
> > > > > parameter? It is possible, but I think the scheme for now works ok. If
> > > > > you extend it to parse two suffixes, it should be fine. Yes, the
> > > > > variable name would be ugly, but you can just make a copy into a
> > > > > properly named one. This is the best we can do without switching to
> > > > > BTF tags. We can revisit this when we start having 4 or 5 tags on a
> > > > > single parameter.
> > > > >
> > > > > To make it a bit less verbose you could probably call maybe_null just null?
> > > >
> > > > Thank you for posting the patch.
> > > > It still feels that this extra flexibility gets convoluted.
> > > > I'm not sure Roberto's kfunc actually needs __ref.
> > > > All pointers should be pointers. Hacking -1 and -2 into a pointer
> > > > is something that key infra did, but it doesn't mean that
> > > > we have to carry over it into bpf kfunc.
> > >
> > > There is a separate parameter for the keyring IDs that only
> > > verify_pkcs7_signature() understands. Type casting is done
> > > internally in the bpf_verify_pkcs7_signature() kfunc.
> > >
> > > The other is always a valid struct key pointer or NULL, coming
> > > from bpf_lookup_user_key() (acquire function). I extended
> > > Kumar's patch further to annotate the struct key parameter
> > > with the _ref_null suffix, to accept a referenced pointer or NULL,
> > > instead of just referenced.
> >
> > I don't think it's a good tradeoff complexity wise.
> > !=null check can be done in runtime by the helper.
> 
> Not sure I follow. bpf_lookup_user_key() might not find
> the key you asked for, so it will return NULL.
> 
> Did you mean that I should not set KF_RET_NULL for
> bpf_lookup_user_key()?
> 
> That anyway won’t help if you use the system keyring,
> the alternative parameter. The user keyring is the other
> one, returned by bpf_lookup_user_key().
> 
> When you specify a system keyring, the user keyring should
> be NULL, to indicate that the parameter should not be
> used. This was suggested by both John and Daniel.
> 
> So, it seems unavoidable to annotate the keyring parameter
> with __ref_null, or at least with __null. __ref_null would be
> better, to require a referenced parameter.
> 
> > The type cast is a sign of something fishy in the design.
> 
> Since this is what verify_pkcs7_signature() accepts, I guess
> it is the only option.

I added this topic for the agenda of BPF office hours tomorrow.

Roberto