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

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Jul 26, 2022 at 02:13:17PM +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 ?
> 
> > +		u8	type;
> > +	} task;
> >  };
> >  
> 
> SNIP
> 
> >  
> >  /* BPF syscall commands, see bpf(2) man-page for more details. */
> > diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> > index 8c921799def4..7979aacb651e 100644
> > --- a/kernel/bpf/task_iter.c
> > +++ b/kernel/bpf/task_iter.c
> > @@ -12,6 +12,8 @@
> >  
> >  struct bpf_iter_seq_task_common {
> >  	struct pid_namespace *ns;
> > +	u32	tid;
> > +	u8	type;
> >  };
> >  
> >  struct bpf_iter_seq_task_info {
> > @@ -22,18 +24,31 @@ struct bpf_iter_seq_task_info {
> >  	u32 tid;
> >  };
> >  
> > -static struct task_struct *task_seq_get_next(struct pid_namespace *ns,
> > +static struct task_struct *task_seq_get_next(struct bpf_iter_seq_task_common *common,
> >  					     u32 *tid,
> >  					     bool skip_if_dup_files)
> >  {
> >  	struct task_struct *task = NULL;
> >  	struct pid *pid;
> >  
> > +	if (common->type == BPF_TASK_ITER_TID) {
> > +		if (*tid)
> > +			return NULL;
> 
> I tested and this condition breaks it for fd iterations, not sure about
> the task and vma, because they share this function
> 
> if bpf_seq_read is called with small buffer there will be multiple calls
> to task_file_seq_get_next and second one will stop in here, even if there
> are more files to be displayed for the task in filter

I mean there will be multiple calls of following sequence:

  bpf_seq_read
    task_file_seq_start
      task_seq_get_next

and 2nd one will return NULL in task_seq_get_next,
because info->tid is already set
 
jirka

> 
> it'd be nice to have some test for this ;-) or perhaps compare with the
> not filtered output
> 
> SNIP
> 
> >  static const struct seq_operations task_seq_ops = {
> >  	.start	= task_seq_start,
> >  	.next	= task_seq_next,
> > @@ -137,8 +166,7 @@ struct bpf_iter_seq_task_file_info {
> >  static struct file *
> >  task_file_seq_get_next(struct bpf_iter_seq_task_file_info *info)
> >  {
> > -	struct pid_namespace *ns = info->common.ns;
> > -	u32 curr_tid = info->tid;
> > +	u32 saved_tid = info->tid;
> >  	struct task_struct *curr_task;
> >  	unsigned int curr_fd = info->fd;
> >  
> > @@ -151,21 +179,18 @@ task_file_seq_get_next(struct bpf_iter_seq_task_file_info *info)
> >  		curr_task = info->task;
> >  		curr_fd = info->fd;
> >  	} else {
> > -                curr_task = task_seq_get_next(ns, &curr_tid, true);
> > +		curr_task = task_seq_get_next(&info->common, &info->tid, true);
> >                  if (!curr_task) {
> >                          info->task = NULL;
> > -                        info->tid = curr_tid;
> >                          return NULL;
> >                  }
> 
> nit, looks like we're missing proper indent in here
> 
> 
> >  
> > -                /* set info->task and info->tid */
> > +		/* set info->task */
> >  		info->task = curr_task;
> > -		if (curr_tid == info->tid) {
> > +		if (saved_tid == info->tid)
> >  			curr_fd = info->fd;
> > -		} else {
> > -			info->tid = curr_tid;
> > +		else
> 
> SNIP