From 1a00ef57d9f120b711b6b1193d12ba3789d47ec2 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Tue, 5 Sep 2023 17:46:46 +0200 Subject: [PATCH 1/5] bpf: task_group_seq_get_next: cleanup the usage of next_thread() 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 Acked-by: "Eric W. Biederman" Acked-by: Yonghong Song Link: https://lore.kernel.org/r/20230905154646.GA24928@redhat.com Signed-off-by: Alexei Starovoitov --- 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 c4ab9d6cdbe9c..4d11251080141 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); From 4981921350452a7639fac9ac8f19be4d25febdca Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Tue, 5 Sep 2023 17:46:49 +0200 Subject: [PATCH 2/5] bpf: task_group_seq_get_next: cleanup the usage of get/put_task_struct 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 Acked-by: Yonghong Song Link: https://lore.kernel.org/r/20230905154649.GA24935@redhat.com Signed-off-by: Alexei Starovoitov --- 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 4d11251080141..1589ec3faded8 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; } From 87abbf7a54f6c9c51374b0701cd7ab47534516ae Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Tue, 5 Sep 2023 17:46:51 +0200 Subject: [PATCH 3/5] bpf: task_group_seq_get_next: fix the skip_if_dup_files check 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 Acked-by: Yonghong Song Link: https://lore.kernel.org/r/20230905154651.GA24940@redhat.com Signed-off-by: Alexei Starovoitov --- 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 1589ec3faded8..2264870ae3fce 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; } From 0ee9808b0a211ba1e572073c6afe5897f8300b9c Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Tue, 5 Sep 2023 17:46:54 +0200 Subject: [PATCH 4/5] bpf: task_group_seq_get_next: kill next_task It only adds the unnecessary confusion and compicates the "retry" code. Signed-off-by: Oleg Nesterov Acked-by: Yonghong Song Link: https://lore.kernel.org/r/20230905154654.GA24945@redhat.com Signed-off-by: Alexei Starovoitov --- 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 2264870ae3fce..f51f476ec6799 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, From 780aa8dfcb73f4703b1c4be11c21c8dca36502ad Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Tue, 5 Sep 2023 17:46:56 +0200 Subject: [PATCH 5/5] bpf: task_group_seq_get_next: simplify the "next tid" logic 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 Acked-by: Yonghong Song Link: https://lore.kernel.org/r/20230905154656.GA24950@redhat.com Signed-off-by: Alexei Starovoitov --- 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 f51f476ec6799..7473068ed313e 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; }