Re: [PATCH bpf-next v8 3/4] bpf: Add kfunc bpf_rcu_read_lock/unlock()

From: Yonghong Song <yhs@meta.com>
To: Martin KaFai Lau <martin.lau@linux.dev>, Yonghong Song <yhs@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	kernel-team@fb.com, Martin KaFai Lau <martin.lau@kernel.org>,
	bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v8 3/4] bpf: Add kfunc bpf_rcu_read_lock/unlock()
Date: Tue, 22 Nov 2022 17:06:54 -0800	[thread overview]
Message-ID: <200910d3-23f2-e7e0-a03a-85d781d7341a@meta.com> (raw)
In-Reply-To: <3ee3af12-0542-e33c-2e9b-c6838de6ba64@linux.dev>

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.