[bpf-next,v2,1/2] bpf: Add KF_OBTAIN for obtaining objects without reference count

Commit Message

Juntong Deng Aug. 28, 2024, 8:04 p.m. UTC

Not all structures in the kernel contain reference count, such as
struct socket (its reference count is actually in struct file),
so it makes no sense to use a combination of KF_ACQUIRE and KF_RELEASE
to trick the verifier to make the pointer to struct socket valid.

In this example, we cannot just use BTF_TYPE_SAFE_TRUSTED method,
because not all files are sockets. It is not enough to make private_data
in struct file to be trusted. We need a kfunc like bpf_socket_from_file.

This patch adds KF_OBTAIN flag for the cases where a valid pointer can
be obtained but there is no need to manipulate the reference count
(e.g. the structure itself has no reference count, the actual reference
count is in another structure).

For KF_OBTAIN kfuncs, the passed argument must be valid pointers.
KF_OBTAIN kfuncs guarantees that if the pointer passed in is valid,
then the pointer returned by KF_OBTAIN kfuncs is also valid.

For example, bpf_socket_from_file is a KF_OBTAIN kfunc, and if the
struct file pointer passed in is valid, then the struct socket pointer
returned is also valid.

KF_OBTAIN kfuncs use ref_obj_id to ensure that the returned pointer has
the correct ownership and lifetime. For example, if we pass pointer A to
KF_OBTAIN kfunc and get returned pointer B, then once pointer A is
released, pointer B will become invalid.

Signed-off-by: Juntong Deng <[e-mail]>
---
v1 -> v2: Add more example information to commit message, no code changed.

 include/linux/btf.h   |  1 +
 kernel/bpf/verifier.c | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Alexei Starovoitov Aug. 29, 2024, 12:38 a.m. UTC | #1

On Wed, Aug 28, 2024 at 1:05 PM Juntong Deng <[e-mail]> wrote:
>
>  static bool is_kfunc_sleepable(struct bpf_kfunc_call_arg_meta *meta)
> @@ -12845,6 +12851,12 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                         /* For mark_ptr_or_null_reg, see 93c230e3f5bd6 */
>                         regs[BPF_REG_0].id = ++env->id_gen;
>                 }
> +
> +               if (is_kfunc_obtain(&meta)) {
> +                       regs[BPF_REG_0].type |= PTR_TRUSTED;
> +                       regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
> +               }

The long term plan for the verifier is to do KF_TRUSTED_ARGS
by default in all kfuncs.
In that sense a PTR_TO_BTF_ID returned from a kfunc
has to be either trusted/rcu or acquired.
Currently we have only one odd set of kfuncs iter_next() that
return old/deprecated style PTR_TO_BTF_ID without either TRUSTED
or UNTRUSTED flag.
That is being fixed. They will become TRUSTED | RCU.
After that all new kfuncs should be reviewed from point of view
whether structs that they return either trusted|rcu.
So KF_OBTAIN is partially unnecessary.

But what you want to achieve with KF_OBTAIN is more than just TRUSTED.
You want to propagate ref_obj_id and we cannot do that.
When types change they cannot have the same ref_obj_id.
Think of sock_from_file(). If we add such a wrapper as a kfunc
it will be returning a different object.
(struct *)file != (struct sock *)file->private_data
They has to have different ids.

The patch 2 is just buggy:
+struct mm_struct *bpf_kfunc_obtain_test(struct task_struct *task)
+{
+       return task->mm;
+}

This is wrong:
- mm has to be refcnted. Acquiring a task doesn't mean that task->mm
stays valid.
- ref_obj_id cannot be copied. It's incorrect from verifier ref tracking pov.

pw-bot: cr

Patch

diff --git a/include/linux/btf.h b/include/linux/btf.h
index b8a583194c4a..d3f695123af9 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -75,6 +75,7 @@ 
 #define KF_ITER_NEXT    (1 << 9) /* kfunc implements BPF iter next method */
 #define KF_ITER_DESTROY (1 << 10) /* kfunc implements BPF iter destructor */
 #define KF_RCU_PROTECTED (1 << 11) /* kfunc should be protected by rcu cs when they are invoked */
+#define KF_OBTAIN        (1 << 12) /* kfunc is an obtain function */
 
 /*
  * Tag marking a kernel function as a kfunc. This is meant to minimize the
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5437dca56159..67a67c2b4a55 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10985,9 +10985,15 @@  static bool is_kfunc_release(struct bpf_kfunc_call_arg_meta *meta)
 	return meta->kfunc_flags & KF_RELEASE;
 }
 
+static bool is_kfunc_obtain(struct bpf_kfunc_call_arg_meta *meta)
+{
+	return meta->kfunc_flags & KF_OBTAIN;
+}
+
 static bool is_kfunc_trusted_args(struct bpf_kfunc_call_arg_meta *meta)
 {
-	return (meta->kfunc_flags & KF_TRUSTED_ARGS) || is_kfunc_release(meta);
+	return (meta->kfunc_flags & KF_TRUSTED_ARGS) || is_kfunc_release(meta) ||
+		is_kfunc_obtain(meta);
 }
 
 static bool is_kfunc_sleepable(struct bpf_kfunc_call_arg_meta *meta)
@@ -12845,6 +12851,12 @@  static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			/* For mark_ptr_or_null_reg, see 93c230e3f5bd6 */
 			regs[BPF_REG_0].id = ++env->id_gen;
 		}
+
+		if (is_kfunc_obtain(&meta)) {
+			regs[BPF_REG_0].type |= PTR_TRUSTED;
+			regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
+		}
+
 		mark_btf_func_reg_size(env, BPF_REG_0, sizeof(void *));
 		if (is_kfunc_acquire(&meta)) {
 			int id = acquire_reference_state(env, insn_idx);