Re: [PATCH bpf-next v5 2/5] selftests/bpf: Add tests for open-coded style process file iterator

[Juntong Deng]

On 2024/12/10 18:51, Alexei Starovoitov wrote:
> On Tue, Dec 10, 2024 at 8:23 AM Juntong Deng <juntong.deng@xxxxxxxxxxx> wrote:
> > > > +SEC("fentry/" SYS_PREFIX "sys_nanosleep")
> > > > +int test_bpf_iter_task_file(void *ctx)
> > > > +{
> > > > +    struct bpf_iter_task_file task_file_it;
> > > > +    struct bpf_iter_task_file_item *item;
> > > > +    struct task_struct *task;
> > > > +
> > > > +    task = bpf_get_current_task_btf();
> > > > +    if (task->parent->pid != parent_pid)
> > > > +            return 0;
> > > > +
> > > > +    count++;
> > > > +
> > > > +    bpf_rcu_read_lock();
> > >
> > > What does the RCU read lock do here exactly?
> >
> > Thanks for your reply.
> >
> > This is used to solve the problem previously discussed in v3 [0].
> >
> > Task ref may be released during iteration.
> >
> > [0]:
> > https://lore.kernel.org/bpf/CAADnVQ+0LUXxmfm1YgyGDz=cciy3+dGGM-Zysq84fpAdaB74Qw@xxxxxxxxxxxxxx/
>
> I think you misunderstood my comment.
>
> "If this object _was_ RCU protected ..."
>
> Adding rcu_read_lock doesn't make 'task' pointer RCU protected.
> That's not how RCU works.
>
> So patch 1 doing:
>
> item->task = task;
>
> is not correct.
>
> See bpf_iter_task_vma_new(). It's doing:
> kit->data->task = get_task_struct(task);
> to make sure task stays valid while iterating.
>
> pw-bot: cr

Thanks for your reply.

Sorry for the misunderstanding.

I will fix it in the next version.