[PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups

[PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups

[Oleg Nesterov]

Yonghong,

I am resending 1-5 of 6 as you suggested with your acks included.

The next (final) patch will change this code to use __next_thread when

	https://lore.kernel.org/all/20230824143142.GA31222@redhat.com/

is merged.

Oleg.

[PATCH bpf-next v2 1/5] bpf: task_group_seq_get_next: cleanup the usage of next_thread()

[Oleg Nesterov]

1. find_pid_ns() + get_pid_task() under rcu_read_lock() guarantees that we
   can safely iterate the task->thread_group list. Even if this task exits
   right after get_pid_task() (or goto retry) and pid_alive() returns 0.

   Kill the unnecessary pid_alive() check.

2. next_thread() simply can't return NULL, kill the bogus "if (!next_task)"
   check.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Acked-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/task_iter.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index c4ab9d6cdbe9..4d1125108014 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -75,15 +75,8 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 		return NULL;
 
 retry:
-	if (!pid_alive(task)) {
-		put_task_struct(task);
-		return NULL;
-	}
-
 	next_task = next_thread(task);
 	put_task_struct(task);
-	if (!next_task)
-		return NULL;
 
 	saved_tid = *tid;
 	*tid = __task_pid_nr_ns(next_task, PIDTYPE_PID, common->ns);
-- 
2.25.1.362.g51ebf55

[PATCH bpf-next v2 2/5] bpf: task_group_seq_get_next: cleanup the usage of get/put_task_struct

[Oleg Nesterov]

get_pid_task() makes no sense, the code does put_task_struct() soon after.
Use find_task_by_pid_ns() instead of find_pid_ns + get_pid_task and kill
put_task_struct(), this allows to do get_task_struct() only once before
return.

While at it, kill the unnecessary "if (!pid)" check in the "if (!*tid)"
block, this matches the next usage of find_pid_ns() + get_pid_task() in
this function.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/task_iter.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 4d1125108014..1589ec3faded 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -42,9 +42,6 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 	if (!*tid) {
 		/* The first time, the iterator calls this function. */
 		pid = find_pid_ns(common->pid, common->ns);
-		if (!pid)
-			return NULL;
-
 		task = get_pid_task(pid, PIDTYPE_TGID);
 		if (!task)
 			return NULL;
@@ -66,17 +63,12 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 		return task;
 	}
 
-	pid = find_pid_ns(common->pid_visiting, common->ns);
-	if (!pid)
-		return NULL;
-
-	task = get_pid_task(pid, PIDTYPE_PID);
+	task = find_task_by_pid_ns(common->pid_visiting, common->ns);
 	if (!task)
 		return NULL;
 
 retry:
 	next_task = next_thread(task);
-	put_task_struct(task);
 
 	saved_tid = *tid;
 	*tid = __task_pid_nr_ns(next_task, PIDTYPE_PID, common->ns);
@@ -88,7 +80,6 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 		return NULL;
 	}
 
-	get_task_struct(next_task);
 	common->pid_visiting = *tid;
 
 	if (skip_if_dup_files && task->files == task->group_leader->files) {
@@ -96,6 +87,7 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 		goto retry;
 	}
 
+	get_task_struct(next_task);
 	return next_task;
 }
 
-- 
2.25.1.362.g51ebf55

[PATCH bpf-next v2 3/5] bpf: task_group_seq_get_next: fix the skip_if_dup_files check

[Oleg Nesterov]

Unless I am notally confused it is wrong. We are going to return or
skip next_task so we need to check next_task-files, not task->files.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/task_iter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 1589ec3faded..2264870ae3fc 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -82,7 +82,7 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 
 	common->pid_visiting = *tid;
 
-	if (skip_if_dup_files && task->files == task->group_leader->files) {
+	if (skip_if_dup_files && next_task->files == next_task->group_leader->files) {
 		task = next_task;
 		goto retry;
 	}
-- 
2.25.1.362.g51ebf55

[PATCH bpf-next v2 4/5] bpf: task_group_seq_get_next: kill next_task

[Oleg Nesterov]

It only adds the unnecessary confusion and compicates the "retry" code.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/task_iter.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 2264870ae3fc..f51f476ec679 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -35,7 +35,7 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 						   u32 *tid,
 						   bool skip_if_dup_files)
 {
-	struct task_struct *task, *next_task;
+	struct task_struct *task;
 	struct pid *pid;
 	u32 saved_tid;
 
@@ -68,10 +68,10 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 		return NULL;
 
 retry:
-	next_task = next_thread(task);
+	task = next_thread(task);
 
 	saved_tid = *tid;
-	*tid = __task_pid_nr_ns(next_task, PIDTYPE_PID, common->ns);
+	*tid = __task_pid_nr_ns(task, PIDTYPE_PID, common->ns);
 	if (!*tid || *tid == common->pid) {
 		/* Run out of tasks of a process.  The tasks of a
 		 * thread_group are linked as circular linked list.
@@ -82,13 +82,11 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 
 	common->pid_visiting = *tid;
 
-	if (skip_if_dup_files && next_task->files == next_task->group_leader->files) {
-		task = next_task;
+	if (skip_if_dup_files && task->files == task->group_leader->files)
 		goto retry;
-	}
 
-	get_task_struct(next_task);
-	return next_task;
+	get_task_struct(task);
+	return task;
 }
 
 static struct task_struct *task_seq_get_next(struct bpf_iter_seq_task_common *common,
-- 
2.25.1.362.g51ebf55

[PATCH bpf-next v2 5/5] bpf: task_group_seq_get_next: simplify the "next tid" logic

[Oleg Nesterov]

Kill saved_tid. It looks ugly to update *tid and then restore the
previous value if __task_pid_nr_ns() returns 0. Change this code
to update *tid and common->pid_visiting once before return.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/task_iter.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index f51f476ec679..7473068ed313 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -37,7 +37,7 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 {
 	struct task_struct *task;
 	struct pid *pid;
-	u32 saved_tid;
+	u32 next_tid;
 
 	if (!*tid) {
 		/* The first time, the iterator calls this function. */
@@ -70,21 +70,18 @@ static struct task_struct *task_group_seq_get_next(struct bpf_iter_seq_task_comm
 retry:
 	task = next_thread(task);
 
-	saved_tid = *tid;
-	*tid = __task_pid_nr_ns(task, PIDTYPE_PID, common->ns);
-	if (!*tid || *tid == common->pid) {
+	next_tid = __task_pid_nr_ns(task, PIDTYPE_PID, common->ns);
+	if (!next_tid || next_tid == common->pid) {
 		/* Run out of tasks of a process.  The tasks of a
 		 * thread_group are linked as circular linked list.
 		 */
-		*tid = saved_tid;
 		return NULL;
 	}
 
-	common->pid_visiting = *tid;
-
 	if (skip_if_dup_files && task->files == task->group_leader->files)
 		goto retry;
 
+	*tid = common->pid_visiting = next_tid;
 	get_task_struct(task);
 	return task;
 }
-- 
2.25.1.362.g51ebf55

Re: [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups

[Oleg Nesterov]

sorry for noise, forgot to mention...

this (hopefully simple) series was only compile-tested.

On 09/05, Oleg Nesterov wrote:
>
> Yonghong,
>
> I am resending 1-5 of 6 as you suggested with your acks included.
>
> The next (final) patch will change this code to use __next_thread when
>
> 	https://lore.kernel.org/all/20230824143142.GA31222@redhat.com/
>
> is merged.
>
> Oleg.

Re: [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups

[Alexei Starovoitov]

On Tue, Sep 5, 2023 at 12:59 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> sorry for noise, forgot to mention...
>
> this (hopefully simple) series was only compile-tested.
>
> On 09/05, Oleg Nesterov wrote:
> >
> > Yonghong,
> >
> > I am resending 1-5 of 6 as you suggested with your acks included.
> >
> > The next (final) patch will change this code to use __next_thread when
> >
> >       https://lore.kernel.org/all/20230824143142.GA31222@redhat.com/
> >
> > is merged.

The patch set looks fine.
What is the next step?
Should we merge it now in bpf-next and then during the next merge window
you'll follow up with __next_thread clean up?

Re: [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups

[Oleg Nesterov]

On 09/06, Alexei Starovoitov wrote:
>
> On Tue, Sep 5, 2023 at 12:59 PM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > The next (final) patch will change this code to use __next_thread when
> > >
> > >       https://lore.kernel.org/all/20230824143142.GA31222@redhat.com/
> > >
> > > is merged.
>
> The patch set looks fine.
> What is the next step?
> Should we merge it now in bpf-next and then during the next merge window
> you'll follow up with __next_thread clean up?

Thanks, works for me.

Oleg.

Re: [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue, 5 Sep 2023 17:46:12 +0200 you wrote:
> Yonghong,
> 
> I am resending 1-5 of 6 as you suggested with your acks included.
> 
> The next (final) patch will change this code to use __next_thread when
> 
> 	https://lore.kernel.org/all/20230824143142.GA31222@redhat.com/
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2,1/5] bpf: task_group_seq_get_next: cleanup the usage of next_thread()
    https://git.kernel.org/bpf/bpf-next/c/f9aedd66c46b
  - [bpf-next,v2,2/5] bpf: task_group_seq_get_next: cleanup the usage of get/put_task_struct
    https://git.kernel.org/bpf/bpf-next/c/ad98e2e5f84f
  - [bpf-next,v2,3/5] bpf: task_group_seq_get_next: fix the skip_if_dup_files check
    https://git.kernel.org/bpf/bpf-next/c/c12e785e8648
  - [bpf-next,v2,4/5] bpf: task_group_seq_get_next: kill next_task
    https://git.kernel.org/bpf/bpf-next/c/c40a3b44f7c5
  - [bpf-next,v2,5/5] bpf: task_group_seq_get_next: simplify the "next tid" logic
    https://git.kernel.org/bpf/bpf-next/c/1e48f069c90f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html