Re: [PATCH bpf-next 1/3] bpf: Parameterize task iterators.

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Wed, 27 Jul 2022 at 09:01, Kui-Feng Lee <kuifeng@fb.com> wrote:
>
> On Tue, 2022-07-26 at 14:13 +0200, Jiri Olsa wrote:
> > On Mon, Jul 25, 2022 at 10:17:11PM -0700, Kui-Feng Lee wrote:
> > > Allow creating an iterator that loops through resources of one
> > > task/thread.
> > >
> > > People could only create iterators to loop through all resources of
> > > files, vma, and tasks in the system, even though they were
> > > interested
> > > in only the resources of a specific task or process.  Passing the
> > > additional parameters, people can now create an iterator to go
> > > through all resources or only the resources of a task.
> > >
> > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > ---
> > >  include/linux/bpf.h            |  4 ++
> > >  include/uapi/linux/bpf.h       | 23 ++++++++++
> > >  kernel/bpf/task_iter.c         | 81 +++++++++++++++++++++++++-----
> > > ----
> > >  tools/include/uapi/linux/bpf.h | 23 ++++++++++
> > >  4 files changed, 109 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 11950029284f..c8d164404e20 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -1718,6 +1718,10 @@ int bpf_obj_get_user(const char __user
> > > *pathname, int flags);
> > >
> > >  struct bpf_iter_aux_info {
> > >         struct bpf_map *map;
> > > +       struct {
> > > +               __u32   tid;
> >
> > should be just u32 ?
>
> Or, should change the following 'type' to __u8?

Would it be better to use a pidfd instead of a tid here? Unset pidfd
would mean going over all tasks, and any fd > 0 implies attaching to a
specific task (as is the convention in BPF land). Most of the new
UAPIs working on processes are using pidfds (to work with a stable
handle instead of a reusable ID).
The iterator taking an fd also gives an opportunity to BPF LSMs to
attach permissions/policies to it (once we have a file local storage
map) e.g. whether creating a task iterator for that specific pidfd
instance (backed by the struct file) would be allowed or not.
You are using getpid in the selftest and keeping track of last_tgid in
the iterator, so I guess you don't even need to extend pidfd_open to
work on thread IDs right now for your use case (and fdtable and mm are
shared for POSIX threads anyway, so for those two it won't make a
difference).

What is your opinion?

>
> [...]