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

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

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?

> 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?

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 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
	 */