On 12/1/22 2:05 PM, Martin KaFai Lau wrote:
On 12/1/22 9:47 AM, Yonghong Song wrote:On 11/30/22 11:05 PM, Martin KaFai Lau wrote:On 11/28/22 6:37 PM, Yonghong Song wrote:diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.hindex c05aa6e1f6f5..6f192dd9025e 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h@@ -683,7 +683,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 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)@@ -4775,12 +4780,10 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,/* Mark value register as MEM_RCU only if it is protected by * bpf_rcu_read_lock() and the ptr reg is trusted. MEM_RCU * itself can already indicate trustedness inside the rcu - * read lock region. Also mark it as PTR_TRUSTED. + * read lock region. */ if (!env->cur_state->active_rcu_lock || !is_trusted_reg(reg)) flag &= ~MEM_RCU;How about dereferencing a PTR_TO_BTF_ID | MEM_RCU, like: /* parent: PTR_TO_BTF_ID | MEM_RCU */ parent = current->real_parent; /* gparent: PTR_TO_BTF_ID */ gparent = parent->real_parent; Should "gparent" have MEM_RCU also?Currently, no. We have logic in the code like 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 * itself can already indicate trustedness inside the rcu* read lock region. */if (!env->cur_state->active_rcu_lock || !is_trusted_reg(reg))flag &= ~MEM_RCU; } Since 'parent' is not trusted, so it will not be marked as MEM_RCU. It can be marked as MEM_RCU if we do (based on the current patch)or adding a is_rcu_trusted_reg() to consider both is_trusted_reg and MEM_RCU before cleaning ~MEM_RCU here. It seems the check_kfunc_args() below will need a similar is_rcu_trusted_reg() test also.
Sounds good. Will do.
parent = current->real_parent; parent2 = bpf_task_acquire_rcu(parent); if (!parent2) goto out; gparent = parent2->real_parent; Now gparent will be marked as MEM_RCU.Also, should PTR_MAYBE_NULL be added to "parent"?I think we might need to do this. Although from kernel code, task->real_parent, current->cgroups seem not NULL. But for sure there are cases where the rcu ptr could be NULL. This might be conservative for some cases, and if it is absolutely performance critical, we later could tag related __rcu member with btf_decl_tag to indicate its non-null status.- else - flag |= PTR_TRUSTED; } 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.@@ -5945,7 +5948,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 = {@@ -6124,7 +6127,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 @@ -8022,6 +8025,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);@@ -8706,13 +8714,19 @@ 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);+ if (!is_trusted_reg(reg) && !is_rcu_reg(reg)) {+ verbose(env, "R%d must be referenced, trusted or rcu\n", regno);return -EINVAL; } + + if (is_kfunc_rcu(meta) != is_rcu_reg(reg)) {I think is_trusted_reg(reg) should also be acceptable to bpf_task_acquire_rcu().Yes, good point. trusted is a super set of rcu.nit. bpf_task_acquire_not_zero() may be a better kfunc name.Will use this one.
