bpf: Handle MEM_RCU type properly

Commit 9bb00b2895cb ("bpf: Add kfunc bpf_rcu_read_lock/unlock()")
introduced MEM_RCU and bpf_rcu_read_lock/unlock() support. In that
commit, a rcu pointer is tagged with both MEM_RCU and PTR_TRUSTED
so that it can be passed into kfuncs or helpers as an argument.

Martin raised a good question in [1] such that the rcu pointer,
although being able to accessing the object, might have reference
count of 0. This might cause a problem if the rcu pointer is passed
to a kfunc which expects trusted arguments where ref count should
be greater than 0.

This patch makes the following changes related to MEM_RCU pointer:
  - MEM_RCU pointer might be NULL (PTR_MAYBE_NULL).
  - Introduce KF_RCU so MEM_RCU ptr can be acquired with
    a KF_RCU tagged kfunc which assumes ref count of rcu ptr
    could be zero.
  - For mem access 'b = ptr->a', say 'ptr' is a MEM_RCU ptr, and
    'a' is tagged with __rcu as well. Let us mark 'b' as
    MEM_RCU | PTR_MAYBE_NULL.

 [1] https://lore.kernel.org/bpf/ac70f574-4023-664e-b711-e0d3b18117fd@linux.dev/

Fixes: 9bb00b2895cb ("bpf: Add kfunc bpf_rcu_read_lock/unlock()")
Signed-off-by: Yonghong Song <yhs@fb.com>
Link: https://lore.kernel.org/r/20221203184602.477272-1-yhs@fb.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index b5090e89cb3f7ce9158c5da9d47e7ee242accdb3..c07b351a5bc77567e7e0c4cf6a0acb6d11965680 100644 (file)
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -682,7 +682,7 @@ static inline bool bpf_prog_check_recur(const struct bpf_prog *prog)
        }
 }
 
-#define BPF_REG_TRUSTED_MODIFIERS (MEM_ALLOC | MEM_RCU | PTR_TRUSTED)
+#define BPF_REG_TRUSTED_MODIFIERS (MEM_ALLOC | PTR_TRUSTED)
 
 static inline bool bpf_type_has_unsafe_modifiers(u32 type)
 {

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 9ed00077db6eed19f43d4d3cdd965d4366e0ddca..cbd6e4096f8c4134c57f217daa279da3c21a3f12 100644 (file)
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -70,6 +70,7 @@
 #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer arguments */
 #define KF_SLEEPABLE    (1 << 5) /* kfunc may sleep */
 #define KF_DESTRUCTIVE  (1 << 6) /* kfunc performs destructive actions */
+#define KF_RCU          (1 << 7) /* kfunc only takes rcu pointer arguments */
 
 /*
  * Return the name of the passed struct, if exists, or halt the build if for

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index a5a511430f2a9cae6343cdcd49e286ce11af5d58..cca642358e80c618d059aff9da07a9609ea23c88 100644 (file)
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1837,6 +1837,19 @@ struct task_struct *bpf_task_acquire(struct task_struct *p)
        return p;
 }
 
+/**
+ * bpf_task_acquire_not_zero - Acquire a reference to a rcu task object. A task
+ * acquired by this kfunc which is not stored in a map as a kptr, must be
+ * released by calling bpf_task_release().
+ * @p: The task on which a reference is being acquired.
+ */
+struct task_struct *bpf_task_acquire_not_zero(struct task_struct *p)
+{
+       if (!refcount_inc_not_zero(&p->rcu_users))
+               return NULL;
+       return p;
+}
+
 /**
  * bpf_task_kptr_get - Acquire a reference on a struct task_struct kptr. A task
  * kptr acquired by this kfunc which is not subsequently stored in a map, must
@@ -2013,6 +2026,7 @@ BTF_ID_FLAGS(func, bpf_list_push_back)
 BTF_ID_FLAGS(func, bpf_list_pop_front, KF_ACQUIRE | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_list_pop_back, KF_ACQUIRE | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_task_acquire, KF_ACQUIRE | KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_task_acquire_not_zero, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_task_kptr_get, KF_ACQUIRE | KF_KPTR_GET | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_task_release, KF_RELEASE)
 #ifdef CONFIG_CGROUPS

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index b0db9c10567bedf5cdade64ddd5477943dc1e186..66b82f52b5bc980ce7015fb52283553522993b70 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4275,7 +4275,7 @@ static bool is_trusted_reg(const struct bpf_reg_state *reg)
                return true;
 
        /* If a register is not referenced, it is trusted if it has the
-        * MEM_ALLOC, MEM_RCU or PTR_TRUSTED type modifiers, and no others. Some of the
+        * MEM_ALLOC or PTR_TRUSTED type modifiers, and no others. Some of the
         * other type modifiers may be safe, but we elect to take an opt-in
         * approach here as some (e.g. PTR_UNTRUSTED and PTR_MAYBE_NULL) are
         * not.
@@ -4287,6 +4287,11 @@ static bool is_trusted_reg(const struct bpf_reg_state *reg)
               !bpf_type_has_unsafe_modifiers(reg->type);
 }
 
+static bool is_rcu_reg(const struct bpf_reg_state *reg)
+{
+       return reg->type & MEM_RCU;
+}
+
 static int check_pkt_ptr_alignment(struct bpf_verifier_env *env,
                                   const struct bpf_reg_state *reg,
                                   int off, int size, bool strict)
@@ -4785,14 +4790,16 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 
        if (flag & MEM_RCU) {
                /* Mark value register as MEM_RCU only if it is protected by
-                * bpf_rcu_read_lock() and the ptr reg is trusted. MEM_RCU
+                * bpf_rcu_read_lock() and the ptr reg is rcu or trusted. MEM_RCU
                 * itself can already indicate trustedness inside the rcu
-                * read lock region. Also mark it as PTR_TRUSTED.
+                * read lock region. Also mark rcu pointer as PTR_MAYBE_NULL since
+                * it could be null in some cases.
                 */
-               if (!env->cur_state->active_rcu_lock || !is_trusted_reg(reg))
+               if (!env->cur_state->active_rcu_lock ||
+                   !(is_trusted_reg(reg) || is_rcu_reg(reg)))
                        flag &= ~MEM_RCU;
                else
-                       flag |= PTR_TRUSTED;
+                       flag |= PTR_MAYBE_NULL;
        } else if (reg->type & MEM_RCU) {
                /* ptr (reg) is marked as MEM_RCU, but the struct field is not tagged
                 * with __rcu. Mark the flag as PTR_UNTRUSTED conservatively.
@@ -5957,7 +5964,7 @@ static const struct bpf_reg_types btf_ptr_types = {
        .types = {
                PTR_TO_BTF_ID,
                PTR_TO_BTF_ID | PTR_TRUSTED,
-               PTR_TO_BTF_ID | MEM_RCU | PTR_TRUSTED,
+               PTR_TO_BTF_ID | MEM_RCU,
        },
 };
 static const struct bpf_reg_types percpu_btf_ptr_types = {
@@ -6136,7 +6143,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
        case PTR_TO_BTF_ID:
        case PTR_TO_BTF_ID | MEM_ALLOC:
        case PTR_TO_BTF_ID | PTR_TRUSTED:
-       case PTR_TO_BTF_ID | MEM_RCU | PTR_TRUSTED:
+       case PTR_TO_BTF_ID | MEM_RCU:
        case PTR_TO_BTF_ID | MEM_ALLOC | PTR_TRUSTED:
                /* When referenced PTR_TO_BTF_ID is passed to release function,
                 * it's fixed offset must be 0. In the other cases, fixed offset
@@ -8038,6 +8045,11 @@ static bool is_kfunc_destructive(struct bpf_kfunc_call_arg_meta *meta)
        return meta->kfunc_flags & KF_DESTRUCTIVE;
 }
 
+static bool is_kfunc_rcu(struct bpf_kfunc_call_arg_meta *meta)
+{
+       return meta->kfunc_flags & KF_RCU;
+}
+
 static bool is_kfunc_arg_kptr_get(struct bpf_kfunc_call_arg_meta *meta, int arg)
 {
        return arg == 0 && (meta->kfunc_flags & KF_KPTR_GET);
@@ -8722,13 +8734,20 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
                switch (kf_arg_type) {
                case KF_ARG_PTR_TO_ALLOC_BTF_ID:
                case KF_ARG_PTR_TO_BTF_ID:
-                       if (!is_kfunc_trusted_args(meta))
+                       if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta))
                                break;
 
                        if (!is_trusted_reg(reg)) {
-                               verbose(env, "R%d must be referenced or trusted\n", regno);
-                               return -EINVAL;
+                               if (!is_kfunc_rcu(meta)) {
+                                       verbose(env, "R%d must be referenced or trusted\n", regno);
+                                       return -EINVAL;
+                               }
+                               if (!is_rcu_reg(reg)) {
+                                       verbose(env, "R%d must be a rcu pointer\n", regno);
+                                       return -EINVAL;
+                               }
                        }
+
                        fallthrough;
                case KF_ARG_PTR_TO_CTX:
                        /* Trusted arguments have the same offset checks as release arguments */
@@ -8839,7 +8858,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
                case KF_ARG_PTR_TO_BTF_ID:
                        /* Only base_type is checked, further checks are done here */
                        if ((base_type(reg->type) != PTR_TO_BTF_ID ||
-                            bpf_type_has_unsafe_modifiers(reg->type)) &&
+                            (bpf_type_has_unsafe_modifiers(reg->type) && !is_rcu_reg(reg))) &&
                            !reg2btf_ids[base_type(reg->type)]) {
                                verbose(env, "arg#%d is %s ", i, reg_type_str(env, reg->type));
                                verbose(env, "expected %s or socket\n",
@@ -8954,7 +8973,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
                } else if (rcu_unlock) {
                        bpf_for_each_reg_in_vstate(env->cur_state, state, reg, ({
                                if (reg->type & MEM_RCU) {
-                                       reg->type &= ~(MEM_RCU | PTR_TRUSTED);
+                                       reg->type &= ~(MEM_RCU | PTR_MAYBE_NULL);
                                        reg->type |= PTR_UNTRUSTED;
                                }
                        }));
@@ -11294,7 +11313,7 @@ static void mark_ptr_or_null_reg(struct bpf_func_state *state,
                                 bool is_null)
 {
        if (type_may_be_null(reg->type) && reg->id == id &&
-           !WARN_ON_ONCE(!reg->id)) {
+           (is_rcu_reg(reg) || !WARN_ON_ONCE(!reg->id))) {
                /* Old offset (both fixed and variable parts) should have been
                 * known-zero, because we don't allow pointer arithmetic on
                 * pointers that might be NULL. If we see this happening, don't