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

[Yonghong Song <yhs@meta.com>]

On 11/21/22 2:56 PM, Martin KaFai Lau wrote:
> On 11/21/22 12:01 PM, Yonghong Song wrote:
>>
>>
>> On 11/21/22 11:41 AM, Martin KaFai Lau wrote:
>>> On 11/21/22 9:05 AM, Yonghong Song wrote:
>>>> @@ -4704,6 +4715,15 @@ static int check_ptr_to_btf_access(struct 
>>>> bpf_verifier_env *env,
>>>>           return -EACCES;
>>>>       }
>>>> +    /* Access rcu protected memory */
>>>> +    if ((reg->type & MEM_RCU) && env->prog->aux->sleepable &&
>>>> +        !env->cur_state->active_rcu_lock) {
>>>> +        verbose(env,
>>>> +            "R%d is ptr_%s access rcu-protected memory with off=%d, 
>>>> not rcu protected\n",
>>>> +            regno, tname, off);
>>>> +        return -EACCES;
>>>> +    }
>>>> +
>>>>       if (env->ops->btf_struct_access && !type_is_alloc(reg->type)) {
>>>>           if (!btf_is_kernel(reg->btf)) {
>>>>               verbose(env, "verifier internal error: reg->btf must 
>>>> be kernel btf\n");
>>>> @@ -4731,12 +4751,27 @@ static int check_ptr_to_btf_access(struct 
>>>> bpf_verifier_env *env,
>>>>       if (ret < 0)
>>>>           return ret;
>>>> +    /* The value is a rcu pointer. The load needs to be in a rcu 
>>>> lock region,
>>>> +     * similar to rcu_dereference().
>>>> +     */
>>>> +    if ((flag & MEM_RCU) && env->prog->aux->sleepable && 
>>>> !env->cur_state->active_rcu_lock) {
>>>> +        verbose(env,
>>>> +            "R%d is rcu dereference ptr_%s with off=%d, not in 
>>>> rcu_read_lock region\n",
>>>> +            regno, tname, off);
>>>> +        return -EACCES;
>>>> +    }
>>>
>>> Would this make the existing rdonly use case fail?
>>>
>>> SEC("fentry.s/" SYS_PREFIX "sys_getpgid")
>>> int task_real_parent(void *ctx)
>>> {
>>>      struct task_struct *task, *real_parent;
>>>
>>>      task = bpf_get_current_task_btf();
>>>          real_parent = task->real_parent;
>>>          bpf_printk("pid %u\n", real_parent->pid);
>>>          return 0;
>>> }
>>
>> Right, it will fail. To fix the issue, user can do
>>     bpf_rcu_read_lock();
>>     real_parent = task->real_parent;
>>     bpf_printk("pid %u\n", real_parent->pid);
>>     bpf_rcu_read_unlock();
>>
>> But this raised a good question. How do we deal with
>> legacy sleepable programs with newly-added rcu tagging
>> capabilities.
>>
>> My current option is to error out if rcu usage is not right.
>> But this might break existing sleepable programs.
>>
>> Another option intends to not break existing, like above,
>> codes. In this case, MEM_RCU will not tagged if it is
>> not inside bpf_rcu_read_lock() region.
> 
> hmm.... it is to make MEM_RCU to mean a reg is protected by the current 
> active_rcu_lock or not?

Yes, for example, in 'real_parent = task->real_parent' where
'real_parent' in task_struct is tagged with __rcu in the struct
definition. So the 'real_parent' variable in the above assignment
will be tagged with MEM_RCU.

> 
>> In this case, the above non-rcu-protected code should work. And the
>> following should work as well although it is a little
>> bit awkward.
>>     real_parent = task->real_parent; // real_parent not tagged with rcu
>>     bpf_rcu_read_lock();
>>     bpf_printk("pid %u\n", real_parent->pid);
>>     bpf_rcu_read_unlock();
> 
> I think it should be fine.  bpf_rcu_read_lock() just not useful in this 
> example but nothing break or crash.  Also, after bpf_rcu_read_unlock(), 
> real_parent will continue to be readable because the MEM_RCU is not set?

That is correct. the variable real_parent is not tagged with MEM_RCU
and it will stay that way for the rest of its life cycle.

With new PTR_TRUSTED mechanism, real_parent will be marked as normal
PTR_TO_BTF_ID and it is not marked as PTR_UNTRUSTED for backward
compatibility. So in the above code, real_parent->pid is just a normal
load (not related to rcu/trusted/untrusted). People may think it
is okay, but actually it does not okay. Verifier could add more state
to issue proper warnings, but I am not sure whether it is worthwhile
or not. As you mentioned, nothing breaks. It is just the current
existing way. So we should be able to live with this.

> 
> On top of the active_rcu_lock, should MEM_RCU be set only when it is 
> dereferenced from a PTR_TRUSTED ptr (or with ref_obj_id != 0)?

I didn't consider PTR_TRUSTED because it is just introduced yesterday...

My current implementation inherits the old ptr_to_btf_id way where by
default any ptr_to_btf_id is trusted. But since we have PTR_TRUSTED
we should be able to use it for a stronger guarantee.

> I am thinking about the following more common case:
> 
>      /* bpf_get_current_task_btf() may need to be changed
>       * to set PTR_TRUSTED at the retval?
>       */
>      /* task: PTR_TO_BTF_ID | PTR_TRUSTED */
>      task = bpf_get_current_task_btf();
> 
>      bpf_rcu_read_lock();
> 
>      /* real_parent: PTR_TO_BTF_ID | PTR_TRUSTED | MEM_RCU */
>      real_parent = task->real_parent;
> 
>          /* bpf_task_acquire() needs to change to use 
> refcount_inc_not_zero */
>      real_parent = bpf_task_acquire(real_parent);
> 
>      bpf_rcu_read_unlock();
> 
>      /* real_parent is accessible here (after checking NULL) and
>       * can be passed to kfunc
>       */
> 

Yes, the above is a typical use case. Or alternatively after
     real_parent = task->real_parent;
     /* use real_parent inside the bpf_rcu_read_lock() region */

I will try to utilize PTR_TRUSTED concept in the next revision.