Re: [PATCH] bpf: task_group_seq_get_next: cleanup the usage of next_thread()

[Eric W. Biederman]

Oleg Nesterov <oleg@xxxxxxxxxx> writes:

> OK, it seems that you are not going to take these preparatory
> cleanups ;)
>
> I'll resend along with the s/next_thread/__next_thread/ change.
> I was going to do the last change later, but this recent discussion
> https://lore.kernel.org/all/20230824143112.GA31208@xxxxxxxxxx/
> makes me think we should do this right now.

For the record I find this code confusing, and wrong.

It looks like it wants to keep the task_struct pointer or possibly the
struct pid pointer like proc does, but then it winds up keeping a
userspace pid value and regenerating both the struct pid pointer and
the struct task_struct pointer.

Which means that task_group_seq_get_next is unnecessarily slow and has
a built in race condition which means it could wind up iterating through
a different process.

This whole thing looks to be a bad (aka racy) reimplementation of
first_tid and next_tid from proc.  I thought the changes were to
adapt to the needs of bpf, but on closer examination the code is
just racy.

For this code to be correct bpf_iter_seq_task_common needs to store
at a minimum a struct pid pointer.


Oleg your patch makes it easier to see what the how
far this is from first_tid/next_tid in proc.

Acked-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>

Eric