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

[Martin KaFai Lau <martin.lau@linux.dev>]

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.