From 33a9813825710fdc2b980d566ee391fd093a36c6 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Thu, 24 Aug 2023 16:31:42 +0200 Subject: introduce __next_thread(), fix next_tid() vs exec() race Patch series "introduce __next_thread(), change next_thread()". After commit dce8f8ed1de1 ("document while_each_thread(), change first_tid() to use for_each_thread()") + this series 1. We have only one lockless user of next_thread(), task_group_seq_get_next(). I think it should be changed too. 2. We have only one user of task_struct->thread_group, thread_group_empty(). The next patches will change thread_group_empty() and kill ->thread_group. This patch (of 2): next_tid(start) does: rcu_read_lock(); if (pid_alive(start)) { pos = next_thread(start); if (thread_group_leader(pos)) pos = NULL; else get_task_struct(pos); it should return pos = NULL when next_thread() wraps to the 1st thread in the thread group, group leader, and the thread_group_leader() check tries to detect this case. But this can race with exec. To simplify, suppose we have a main thread M and a single sub-thread T, next_tid(T) should return NULL. Now suppose that T execs. If next_tid(T) is called after T changes the leadership and before it does release_task() which removes the old leader from list, then next_thread() returns M and thread_group_leader(M) = F. Lockless use of next_thread() should be avoided. After this change only task_group_seq_get_next() does this, and I believe it should be changed as well. Link: https://lkml.kernel.org/r/20230824143112.GA31208@redhat.com Link: https://lkml.kernel.org/r/20230824143142.GA31222@redhat.com Signed-off-by: Oleg Nesterov Cc: Eric W. Biederman Cc: Peter Zijlstra Signed-off-by: Andrew Morton --- include/linux/sched/signal.h | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'include/linux/sched') diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 0014d3adaf84..7fb34b8cda54 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -715,6 +715,17 @@ bool same_thread_group(struct task_struct *p1, struct task_struct *p2) return p1->signal == p2->signal; } +/* + * returns NULL if p is the last thread in the thread group + */ +static inline struct task_struct *__next_thread(struct task_struct *p) +{ + return list_next_or_null_rcu(&p->signal->thread_head, + &p->thread_node, + struct task_struct, + thread_node); +} + static inline struct task_struct *next_thread(const struct task_struct *p) { return list_entry_rcu(p->thread_group.next, -- cgit v1.2.3 From d639cf4abb4d171ab2456904da5668c42b5c1937 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Thu, 24 Aug 2023 16:32:01 +0200 Subject: change next_thread() to use __next_thread() ?: group_leader This relies on fact that group leader is always the 1st entry in the signal->thread_head list. With or without this change, if the lockless next_thread(last_thread) races with exec it can return the old or the new leader. We are almost ready to kill task->thread_group, after this change its only user is thread_group_empty(). Link: https://lkml.kernel.org/r/20230824143201.GB31222@redhat.com Signed-off-by: Oleg Nesterov Cc: Eric W. Biederman Cc: Peter Zijlstra Signed-off-by: Andrew Morton --- include/linux/sched/signal.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'include/linux/sched') diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 7fb34b8cda54..cffc882d367f 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -726,10 +726,9 @@ static inline struct task_struct *__next_thread(struct task_struct *p) thread_node); } -static inline struct task_struct *next_thread(const struct task_struct *p) +static inline struct task_struct *next_thread(struct task_struct *p) { - return list_entry_rcu(p->thread_group.next, - struct task_struct, thread_group); + return __next_thread(p) ?: p->group_leader; } static inline int thread_group_empty(struct task_struct *p) -- cgit v1.2.3 From e34a35ee1f52312af130b5ebd42fa28313fc6149 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Sat, 26 Aug 2023 13:14:06 +0200 Subject: change thread_group_empty() to use task_struct->thread_node Patch series "kill task_struct->thread_group". This patch (of 2): It could use list_is_singular() but this way it is cheaper. Plus the thread_group_leader() check makes it clear that thread_group_empty() can only return true if p is a group leader. This was not immediately obvious before this patch. task_struct->thread_group no longer has users, it can die. Link: https://lkml.kernel.org/r/20230826111200.GA22982@redhat.com Link: https://lkml.kernel.org/r/20230826111406.GA23238@redhat.com Signed-off-by: Oleg Nesterov Cc: Eric W. Biederman Cc: Peter Zijlstra Signed-off-by: Andrew Morton --- include/linux/sched/signal.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'include/linux/sched') diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index cffc882d367f..d7fa3ca2fa53 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -733,7 +733,8 @@ static inline struct task_struct *next_thread(struct task_struct *p) static inline int thread_group_empty(struct task_struct *p) { - return list_empty(&p->thread_group); + return thread_group_leader(p) && + list_is_last(&p->thread_node, &p->signal->thread_head); } #define delay_group_leader(p) \ -- cgit v1.2.3