On 11/22/22 4:24 PM, Martin KaFai Lau wrote:
On 11/22/22 11:53 AM, Yonghong Song wrote:
+ 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
(PTR_TRUSTED or
+ * ref_obj_id != 0). MEM_RCU itself can already indicate
+ * trustedness inside the rcu read lock region. But Mark it
+ * as PTR_TRUSTED as well similar to MEM_ALLOC.
+ */
+ if (!env->cur_state->active_rcu_lock ||
+ (!(reg->type & PTR_TRUSTED) && !reg->ref_obj_id))
Can is_trusted_reg() be reused or MEM_ALLOC is not applicable here?
Currently MEM_ALLOC is returned by bpf_obj_new() which takes prog btf.
currently MEM_RCU is only enabled with btf_struct_access() for
vmlinux btf. So if MEM_ALLOC is in reg->type, then MEM_RCU
should not appear.
But I guess we could still use is_trusted_reg() here since it
does cover the use case here.
+ flag &= ~MEM_RCU;
+ else
+ flag |= PTR_TRUSTED;
+ } else if (reg->type & MEM_RCU) {
+ /* ptr (reg) is marked as MEM_RCU, but value reg is not
marked as MEM_RCU.
+ * Mark the value reg as PTR_UNTRUSTED conservatively.
+ */
+ flag |= PTR_UNTRUSTED;
Should PTR_UNTRUSTED tagging be limited to ret == PTR_TO_BTF_ID instead
of tagging SCALAR also?
We should be okay here. flag is a local variable. It is used in
below function when reg_type is not SCALAR_VALUE.
static void mark_btf_ld_reg(struct bpf_verifier_env *env,
struct bpf_reg_state *regs, u32 regno,
enum bpf_reg_type reg_type,
struct btf *btf, u32 btf_id,
enum bpf_type_flag flag)
{
if (reg_type == SCALAR_VALUE) {
mark_reg_unknown(env, regs, regno);
return;
}
mark_reg_known_zero(env, regs, regno);
regs[regno].type = PTR_TO_BTF_ID | flag;
regs[regno].btf = btf;
regs[regno].btf_id = btf_id;
}
[ ... ]
@@ -11754,6 +11840,11 @@ static int check_ld_abs(struct
bpf_verifier_env *env, struct bpf_insn *insn)
return -EINVAL;
}
+ if (env->prog->aux->sleepable && env->cur_state->active_rcu_lock) {
I don't know the details about ld_abs :). Why sleepable check is needed
here?
Do we still care about ld_abs??
Actually I added this since spin_lock excludes this. But taking a deep
look at the function convert_bpf_ld_abs, it is converted to a bunch of
bpf insns and bpf_skb_load_helper_{8,16,32}() which eventually calls
skb_copy_bits(). Checking skb_copy_bits(), it seems the function is not
sleepable. Will remove this in the next revision.
Others lgtm.
