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:24:33 -0800	[thread overview]
Message-ID: <993bca84-deb7-88c9-1ac5-f2aeeda2be39@meta.com> (raw)
In-Reply-To: <dd614512-762d-d5e0-adb0-4ab480a03e69@linux.dev>

On 11/22/22 5:19 PM, Martin KaFai Lau wrote:
> On 11/22/22 5:06 PM, Yonghong Song wrote:
>> 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);
> 
> Ah, got it.
> 
>>>> @@ -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
> 
>  From looking at check_ld_abs() again, I just noticed this comment:
> 
>          /* Disallow usage of BPF_LD_[ABS|IND] with reference tracking, as
>           * gen_ld_abs() may terminate the program at runtime, leading to
>           * reference leak.
>           */
> 
> I think active_rcu_lock should be tested.  My question was more on why 
> the env->prog->aux->sleepable test is also needed.

Will remove env->prog->aux->sleepable in the next version. It is a 
leftover missed with v8 not to focus on sleepable aspect of the lock.