Yonghong Song wrote:
>
>
> 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.
Also perhaps interesting is when task is read out of a map
with reference already pinned. I think you should clear
the MEM_RCU tag on all referenced objects?
