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@xxxxxx>
> > ---
> > 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
