* [PATCH bpf-next v2 1/5] bpf: task_group_seq_get_next: cleanup the usage of next_thread()
2023-09-05 15:46 [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups Oleg Nesterov
@ 2023-09-05 15:46 ` Oleg Nesterov
2023-09-05 15:46 ` [PATCH bpf-next v2 2/5] bpf: task_group_seq_get_next: cleanup the usage of get/put_task_struct Oleg Nesterov
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Oleg Nesterov @ 2023-09-05 15:46 UTC (permalink / raw)
To: Yonghong Song; +Cc: Eric W. Biederman, Kui-Feng Lee, bpf
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH bpf-next v2 2/5] bpf: task_group_seq_get_next: cleanup the usage of get/put_task_struct
2023-09-05 15:46 [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups Oleg Nesterov
2023-09-05 15:46 ` [PATCH bpf-next v2 1/5] bpf: task_group_seq_get_next: cleanup the usage of next_thread() Oleg Nesterov
@ 2023-09-05 15:46 ` Oleg Nesterov
2023-09-05 15:46 ` [PATCH bpf-next v2 3/5] bpf: task_group_seq_get_next: fix the skip_if_dup_files check Oleg Nesterov
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Oleg Nesterov @ 2023-09-05 15:46 UTC (permalink / raw)
To: Yonghong Song; +Cc: Eric W. Biederman, Kui-Feng Lee, bpf
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH bpf-next v2 3/5] bpf: task_group_seq_get_next: fix the skip_if_dup_files check
2023-09-05 15:46 [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups Oleg Nesterov
2023-09-05 15:46 ` [PATCH bpf-next v2 1/5] bpf: task_group_seq_get_next: cleanup the usage of next_thread() Oleg Nesterov
2023-09-05 15:46 ` [PATCH bpf-next v2 2/5] bpf: task_group_seq_get_next: cleanup the usage of get/put_task_struct Oleg Nesterov
@ 2023-09-05 15:46 ` Oleg Nesterov
2023-09-05 15:46 ` [PATCH bpf-next v2 4/5] bpf: task_group_seq_get_next: kill next_task Oleg Nesterov
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Oleg Nesterov @ 2023-09-05 15:46 UTC (permalink / raw)
To: Yonghong Song; +Cc: Eric W. Biederman, Kui-Feng Lee, bpf
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH bpf-next v2 4/5] bpf: task_group_seq_get_next: kill next_task
2023-09-05 15:46 [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups Oleg Nesterov
` (2 preceding siblings ...)
2023-09-05 15:46 ` [PATCH bpf-next v2 3/5] bpf: task_group_seq_get_next: fix the skip_if_dup_files check Oleg Nesterov
@ 2023-09-05 15:46 ` Oleg Nesterov
2023-09-05 15:46 ` [PATCH bpf-next v2 5/5] bpf: task_group_seq_get_next: simplify the "next tid" logic Oleg Nesterov
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Oleg Nesterov @ 2023-09-05 15:46 UTC (permalink / raw)
To: Yonghong Song; +Cc: Eric W. Biederman, Kui-Feng Lee, bpf
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH bpf-next v2 5/5] bpf: task_group_seq_get_next: simplify the "next tid" logic
2023-09-05 15:46 [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups Oleg Nesterov
` (3 preceding siblings ...)
2023-09-05 15:46 ` [PATCH bpf-next v2 4/5] bpf: task_group_seq_get_next: kill next_task Oleg Nesterov
@ 2023-09-05 15:46 ` Oleg Nesterov
2023-09-05 19:58 ` [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups Oleg Nesterov
2023-09-07 16:00 ` patchwork-bot+netdevbpf
6 siblings, 0 replies; 10+ messages in thread
From: Oleg Nesterov @ 2023-09-05 15:46 UTC (permalink / raw)
To: Yonghong Song; +Cc: Eric W. Biederman, Kui-Feng Lee, bpf
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups
2023-09-05 15:46 [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups Oleg Nesterov
` (4 preceding siblings ...)
2023-09-05 15:46 ` [PATCH bpf-next v2 5/5] bpf: task_group_seq_get_next: simplify the "next tid" logic Oleg Nesterov
@ 2023-09-05 19:58 ` Oleg Nesterov
2023-09-06 20:00 ` Alexei Starovoitov
2023-09-07 16:00 ` patchwork-bot+netdevbpf
6 siblings, 1 reply; 10+ messages in thread
From: Oleg Nesterov @ 2023-09-05 19:58 UTC (permalink / raw)
To: Yonghong Song; +Cc: Eric W. Biederman, Kui-Feng Lee, bpf
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups
2023-09-05 15:46 [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups Oleg Nesterov
` (5 preceding siblings ...)
2023-09-05 19:58 ` [PATCH bpf-next v2 0/5] bpf: task_group_seq_get_next: misc cleanups Oleg Nesterov
@ 2023-09-07 16:00 ` patchwork-bot+netdevbpf
6 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-09-07 16:00 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: yonghong.song, ebiederm, sinquersw, bpf
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
^ permalink raw reply [flat|nested] 10+ messages in thread