On Wed, May 6, 2020 at 10:40 PM Yonghong Song <yhs@xxxxxx> wrote:
>
> Only the tasks belonging to "current" pid namespace
> are enumerated.
>
> For task/file target, the bpf program will have access to
> struct task_struct *task
> u32 fd
> struct file *file
> where fd/file is an open file for the task.
>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
> kernel/bpf/Makefile | 2 +-
> kernel/bpf/task_iter.c | 332 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 333 insertions(+), 1 deletion(-)
> create mode 100644 kernel/bpf/task_iter.c
>
Looks good, though I'd just use bpf_iter_seq_task_file_info directly,
given it is a state of iterator by design. Probably would simplify
code a bit as well. But either way I can't find any bug in the code,
so:
Acked-by: Andrii Nakryiko <andriin@xxxxxx>
[...]
> +
> +struct bpf_iter_seq_task_common {
> + struct pid_namespace *ns;
> +};
> +
> +struct bpf_iter_seq_task_info {
> + /* The first field must be struct bpf_iter_seq_task_common.
> + * this is assumed by {init, fini}_seq_pidns() callback functions.
> + */
Awesome, thanks!
> + struct bpf_iter_seq_task_common common;
> + u32 tid;
> +};
> +
[...]
> +
> +static void *task_seq_next(struct seq_file *seq, void *v, loff_t *pos)
nit: v is very non-descriptive name, prev_task?
> +{
> + struct bpf_iter_seq_task_info *info = seq->private;
> + struct task_struct *task;
> +
> + ++*pos;
> + ++info->tid;
> + put_task_struct((struct task_struct *)v);
> + task = task_seq_get_next(info->common.ns, &info->tid);
> + if (!task)
> + return NULL;
> +
> + return task;
> +}
> +
> +struct bpf_iter__task {
> + __bpf_md_ptr(struct bpf_iter_meta *, meta);
> + __bpf_md_ptr(struct task_struct *, task);
> +};
> +
> +DEFINE_BPF_ITER_FUNC(task, struct bpf_iter_meta *meta, struct task_struct *task)
> +
> +static int __task_seq_show(struct seq_file *seq, void *v, bool in_stop)
same nit: v -> task? Can also specify `struct task_struct *` type,
same for above.
[...]
> +
> +struct bpf_iter_seq_task_file_info {
> + /* The first field must be struct bpf_iter_seq_task_common.
> + * this is assumed by {init, fini}_seq_pidns() callback functions.
> + */
> + struct bpf_iter_seq_task_common common;
> + struct task_struct *task;
> + struct files_struct *files;
> + u32 tid;
> + u32 fd;
> +};
> +
> +static struct file *task_file_seq_get_next(struct pid_namespace *ns, u32 *tid,
> + int *fd, struct task_struct **task,
> + struct files_struct **fstruct)
Why not just passing struct bpf_iter_seq_task_file_info* directly,
instead of these 5 individual pointers?
> +{
> + struct files_struct *curr_files;
> + struct task_struct *curr_task;
> + u32 curr_tid = *tid, max_fds;
> + int curr_fd = *fd;
> +
> + /* If this function returns a non-NULL file object,
> + * it held a reference to the task/files_struct/file.
> + * Otherwise, it does not hold any reference.
> + */
[...]
