[PATCH 0/1] add pid::tasks_lock to fix send_sigio() and send_sigurg()

[PATCH 0/1] add pid::tasks_lock to fix send_sigio() and send_sigurg()

[Oleg Nesterov]

Not for inclusion yet, just RFC. In particular, I'd like to have
a review from sashiko, I am sure I missed something ;)

Only compile tested, the changelog is obviously incomplete,
__change_pid() should die after this change, etc.

And. I am still not sure that the new lock should be per pid, perhaps
a simple global lock which aligns with tasklist makes more sense, it
should not be contended too much.

Please comment.

Oleg.

 fs/fcntl.c          |  4 ++++
 include/linux/pid.h |  1 +
 kernel/pid.c        | 24 +++++++++++++++++++-----
 3 files changed, 24 insertions(+), 5 deletions(-)

[PATCH 1/1] add pid::tasks_lock to fix send_sigio() and send_sigurg()

[Oleg Nesterov]

send_sigio() and send_sigurg() assume that do_each_pid_task() is
rcu-safe but it is not. A lockless user can:

	- race with a change_pid() that moves the task to another hlist,
	  and continue to scan the wrong list.

	- race with a copy_process() that adds a new task to the start of hlist,
	  and miss the new child.

As for send_sigio/sigurg in particular, they can also race with de_thread(),
this needs another note.

Add the new per pid lock to protect pid->tasks hlist. Note that detach_pid()
doesn't take this lock, so do_each_pid_task() without tasklist_lock needs
rcu_read_lock().

The "type <= PIDTYPE_TGID" case is wrong too, but this needs another patch.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/fcntl.c          |  4 ++++
 include/linux/pid.h |  1 +
 kernel/pid.c        | 24 +++++++++++++++++++-----
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index c158f082f1da..74c8a96e64b2 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -930,9 +930,11 @@ void send_sigio(struct fown_struct *fown, int fd, int band)
 		rcu_read_unlock();
 	} else {
 		rcu_read_lock();
+		spin_lock(&pid->tasks_lock);
 		do_each_pid_task(pid, type, p) {
 			send_sigio_to_task(p, fown, fd, band, type);
 		} while_each_pid_task(pid, type, p);
+		spin_unlock(&pid->tasks_lock);
 		rcu_read_unlock();
 	}
  out_unlock_fown:
@@ -976,9 +978,11 @@ int send_sigurg(struct file *file)
 		rcu_read_unlock();
 	} else {
 		rcu_read_lock();
+		spin_lock(&pid->tasks_lock);
 		do_each_pid_task(pid, type, p) {
 			send_sigurg_to_task(p, fown, type);
 		} while_each_pid_task(pid, type, p);
+		spin_unlock(&pid->tasks_lock);
 		rcu_read_unlock();
 	}
  out_unlock_fown:
diff --git a/include/linux/pid.h b/include/linux/pid.h
index f5476d7140a3..ca5177fdae81 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -59,6 +59,7 @@ struct pid {
 	refcount_t count;
 	unsigned int level;
 	spinlock_t lock;
+	spinlock_t tasks_lock;
 	struct {
 		u64 ino;
 		struct rhash_head pidfs_hash;
diff --git a/kernel/pid.c b/kernel/pid.c
index 95b8ccfa8269..41fe55fefc3c 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -45,9 +45,11 @@
 #include <linux/pidfs.h>
 #include <net/sock.h>
 #include <uapi/linux/pidfd.h>
+#include "sched/sched.h"	/* double_lock() */
 
 struct pid init_struct_pid = {
 	.count		= REFCOUNT_INIT(1),
+	.tasks_lock	= __SPIN_LOCK_UNLOCKED(init_struct_pid.tasks_lock),
 	.tasks		= {
 		{ .first = NULL },
 		{ .first = NULL },
@@ -193,6 +195,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
 	pid->level = ns->level;
 	refcount_set(&pid->count, 1);
 	spin_lock_init(&pid->lock);
+	spin_lock_init(&pid->tasks_lock);
 	for (type = 0; type < PIDTYPE_MAX; ++type)
 		INIT_HLIST_HEAD(&pid->tasks[type]);
 	init_waitqueue_head(&pid->wait_pidfd);
@@ -396,7 +399,8 @@ void attach_pid(struct task_struct *task, enum pid_type type)
 	lockdep_assert_held_write(&tasklist_lock);
 
 	pid = *task_pid_ptr(task, type);
-	hlist_add_head_rcu(&task->pid_links[type], &pid->tasks[type]);
+	scoped_guard(spinlock, &pid->tasks_lock)
+		hlist_add_head_rcu(&task->pid_links[type], &pid->tasks[type]);
 }
 
 static void __change_pid(struct pid **pids, struct task_struct *task,
@@ -427,10 +431,15 @@ void detach_pid(struct pid **pids, struct task_struct *task, enum pid_type type)
 }
 
 void change_pid(struct pid **pids, struct task_struct *task, enum pid_type type,
-		struct pid *pid)
+		struct pid *new)
 {
-	__change_pid(pids, task, type, pid);
-	attach_pid(task, type);
+	struct pid *old = task->signal->pids[type];
+
+	double_lock(&old->tasks_lock, &new->tasks_lock);
+	__change_pid(pids, task, type, new);
+	hlist_add_head_rcu(&task->pid_links[type], &new->tasks[type]);
+	spin_unlock(&old->tasks_lock);
+	spin_unlock(&new->tasks_lock);
 }
 
 void exchange_tids(struct task_struct *left, struct task_struct *right)
@@ -441,6 +450,7 @@ void exchange_tids(struct task_struct *left, struct task_struct *right)
 	struct hlist_head *head2 = &pid2->tasks[PIDTYPE_PID];
 
 	lockdep_assert_held_write(&tasklist_lock);
+	double_lock(&pid1->tasks_lock, &pid2->tasks_lock);
 
 	/* Swap the single entry tid lists */
 	hlists_swap_heads_rcu(head1, head2);
@@ -452,6 +462,9 @@ void exchange_tids(struct task_struct *left, struct task_struct *right)
 	/* Swap the cached value */
 	WRITE_ONCE(left->pid, pid_nr(pid2));
 	WRITE_ONCE(right->pid, pid_nr(pid1));
+
+	spin_unlock(&pid1->tasks_lock);
+	spin_unlock(&pid2->tasks_lock);
 }
 
 /* transfer_pid is an optimization of attach_pid(new), detach_pid(old) */
@@ -460,7 +473,8 @@ void transfer_pid(struct task_struct *old, struct task_struct *new,
 {
 	WARN_ON_ONCE(type == PIDTYPE_PID);
 	lockdep_assert_held_write(&tasklist_lock);
-	hlist_replace_rcu(&old->pid_links[type], &new->pid_links[type]);
+	scoped_guard(spinlock, &old->signal->pids[type]->tasks_lock)
+		hlist_replace_rcu(&old->pid_links[type], &new->pid_links[type]);
 }
 
 struct task_struct *pid_task(struct pid *pid, enum pid_type type)
-- 
2.55.0

Re: [PATCH 0/1] add pid::tasks_lock to fix send_sigio() and send_sigurg()

[Oleg Nesterov]

On 10/17, Oleg Nesterov wrote:
>
> Not for inclusion yet, just RFC. In particular, I'd like to have
> a review from sashiko, I am sure I missed something ;)

https://sashiko.dev/#/patchset/atOY-5ZjqVTQw2JE%40redhat.com

Indeed I missed something obvious. I forgot that attach_pid() is called
under siglock. So this change needs more work.

Thanks Sashiko.

Oleg.