netdev - Re: [PATCH bpf-next v3 11/21] bpf: add task and task/file iterator targets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzZd_DiCThN_6boqtkJ15d2ms+88B+Uxm45zbc1=_7Mjqg@mail.gmail.com>
Date:   Fri, 8 May 2020 12:36:04 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next v3 11/21] bpf: add task and task/file iterator targets

On Wed, May 6, 2020 at 10:40 PM Yonghong Song <yhs@...com> 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@...com>
> ---
>  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@...com>


[...]

> +
> +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.
> +        */

[...]