[PATCH] send_sigqueue: simplify and fix the race

send_sigqueue() checks PF_EXITING, then locks p->sighand->siglock. This is unsafe: 'p' can exit in between and set ->sighand = NULL. The race is theoretical, the window is tiny and irqs are disabled by the caller, so I don't think we need the fix for -stable tree. Convert send_sigqueue() to use lock_task_sighand() helper. Also, delete 'p->flags & PF_EXITING' re-check, it is unneeded and the comment is wrong. Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>

[PATCH] send_sigqueue: simplify and fix the race
send_sigqueue() checks PF_EXITING, then locks p->sighand->siglock. This is unsafe: 'p' can exit in between and set ->sighand = NULL. The race is theoretical, the window is tiny and irqs are disabled by the caller, so I don't think we need the fix for -stable tree. Convert send_sigqueue() to use lock_task_sighand() helper. Also, delete 'p->flags & PF_EXITING' re-check, it is unneeded and the comment is wrong. Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
Oleg Nesterov · Linus Torvalds
1 parent a1d5e21e3e
Showing 1 changed file with 4 additions and 37 deletions Side-by-side Diff
kernel/signal.c
@@ -1309,12 +1309,10 @@
 	__sigqueue_free(q);
 }
  
-int
-send_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
+int send_sigqueue(int sig, struct sigqueue *q, struct task_struct *p)
 {
 	unsigned long flags;
 	int ret = 0;
-	struct sighand_struct *sh;
  
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
  
  
  
@@ -1328,48 +1326,17 @@
 	 */
 	rcu_read_lock();
  
-	if (unlikely(p->flags & PF_EXITING)) {
+	if (!likely(lock_task_sighand(p, &flags))) {
 		ret = -1;
 		goto out_err;
 	}
  
-retry:
-	sh = rcu_dereference(p->sighand);
-
-	spin_lock_irqsave(&sh->siglock, flags);
-	if (p->sighand != sh) {
-		/* We raced with exec() in a multithreaded process... */
-		spin_unlock_irqrestore(&sh->siglock, flags);
-		goto retry;
-	}
-
-	/*
-	 * We do the check here again to handle the following scenario:
-	 *
-	 * CPU 0		CPU 1
-	 * send_sigqueue
-	 * check PF_EXITING
-	 * interrupt		exit code running
-	 *			__exit_signal
-	 *			lock sighand->siglock
-	 *			unlock sighand->siglock
-	 * lock sh->siglock
-	 * add(tsk->pending) 	flush_sigqueue(tsk->pending)
-	 *
-	 */
-
-	if (unlikely(p->flags & PF_EXITING)) {
-		ret = -1;
-		goto out;
-	}
-
 	if (unlikely(!list_empty(&q->list))) {
 		/*
 		 * If an SI_TIMER entry is already queue just increment
 		 * the overrun count.
 		 */
-		if (q->info.si_code != SI_TIMER)
-			BUG();
+		BUG_ON(q->info.si_code != SI_TIMER);
 		q->info.si_overrun++;
 		goto out;
 	}
@@ -1385,7 +1352,7 @@
 		signal_wake_up(p, sig == SIGKILL);
  
 out:
-	spin_unlock_irqrestore(&sh->siglock, flags);
+	unlock_task_sighand(p, &flags);
 out_err:
 	rcu_read_unlock();
...	...	@@ -1309,12 +1309,10 @@
1309	1309	__sigqueue_free(q);
1310	1310	}
1311	1311
1312		-int
1313		-send_sigqueue(int sig, struct sigqueue q, struct task_struct p)
	1312	+int send_sigqueue(int sig, struct sigqueue q, struct task_struct p)
1314	1313	{
1315	1314	unsigned long flags;
1316	1315	int ret = 0;
1317		- struct sighand_struct *sh;
1318	1316
1319	1317	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
1320	1318
1321	1319
1322	1320
...	...	@@ -1328,48 +1326,17 @@
1328	1326	*/
1329	1327	rcu_read_lock();
1330	1328
1331		- if (unlikely(p->flags & PF_EXITING)) {
	1329	+ if (!likely(lock_task_sighand(p, &flags))) {
1332	1330	ret = -1;
1333	1331	goto out_err;
1334	1332	}
1335	1333
1336		-retry:
1337		- sh = rcu_dereference(p->sighand);
1338		-
1339		- spin_lock_irqsave(&sh->siglock, flags);
1340		- if (p->sighand != sh) {
1341		- /* We raced with exec() in a multithreaded process... */
1342		- spin_unlock_irqrestore(&sh->siglock, flags);
1343		- goto retry;
1344		- }
1345		-
1346		- /*
1347		- * We do the check here again to handle the following scenario:
1348		- *
1349		- * CPU 0 CPU 1
1350		- * send_sigqueue
1351		- * check PF_EXITING
1352		- * interrupt exit code running
1353		- * __exit_signal
1354		- * lock sighand->siglock
1355		- * unlock sighand->siglock
1356		- * lock sh->siglock
1357		- * add(tsk->pending) flush_sigqueue(tsk->pending)
1358		- *
1359		- */
1360		-
1361		- if (unlikely(p->flags & PF_EXITING)) {
1362		- ret = -1;
1363		- goto out;
1364		- }
1365		-
1366	1334	if (unlikely(!list_empty(&q->list))) {
1367	1335	/*
1368	1336	* If an SI_TIMER entry is already queue just increment
1369	1337	* the overrun count.
1370	1338	*/
1371		- if (q->info.si_code != SI_TIMER)
1372		- BUG();
	1339	+ BUG_ON(q->info.si_code != SI_TIMER);
1373	1340	q->info.si_overrun++;
1374	1341	goto out;
1375	1342	}
...	...	@@ -1385,7 +1352,7 @@
1385	1352	signal_wake_up(p, sig == SIGKILL);
1386	1353
1387	1354	out:
1388		- spin_unlock_irqrestore(&sh->siglock, flags);
	1355	+ unlock_task_sighand(p, &flags);
1389	1356	out_err:
1390	1357	rcu_read_unlock();
1391	1358