Merge branch 'locking-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip

Pull futex fixes from Ingo Molnar: "This contains two futex fixes: one fixes a race condition, the other clarifies shared/private futex comments" * 'locking-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip: futex: Fix a race condition between REQUEUE_PI and task death futex: Mention key referencing differences between shared and private futexes

Merge branch 'locking-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
Pull futex fixes from Ingo Molnar: "This contains two futex fixes: one fixes a race condition, the other clarifies shared/private futex comments" * 'locking-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip: futex: Fix a race condition between REQUEUE_PI and task death futex: Mention key referencing differences between shared and private futexes
Linus Torvalds
2 parents aea4869f68 30a6b8031f
Showing 1 changed file Side-by-side Diff
kernel/futex.c
@@ -143,9 +143,8 @@
  *
  * Where (A) orders the waiters increment and the futex value read through
  * atomic operations (see hb_waiters_inc) and where (B) orders the write
- * to futex and the waiters read -- this is done by the barriers in
- * get_futex_key_refs(), through either ihold or atomic_inc, depending on the
- * futex type.
+ * to futex and the waiters read -- this is done by the barriers for both
+ * shared and private futexes in get_futex_key_refs().
  *
  * This yields the following case (where X:=waiters, Y:=futex):
  *
  
@@ -344,13 +343,20 @@
 		futex_get_mm(key); /* implies MB (B) */
 		break;
 	default:
+		/*
+		 * Private futexes do not hold reference on an inode or
+		 * mm, therefore the only purpose of calling get_futex_key_refs
+		 * is because we need the barrier for the lockless waiter check.
+		 */
 		smp_mb(); /* explicit MB (B) */
 	}
 }
  
 /*
  * Drop a reference to the resource addressed by a key.
- * The hash bucket spinlock must not be held.
+ * The hash bucket spinlock must not be held. This is
+ * a no-op for private futexes, see comment in the get
+ * counterpart.
  */
 static void drop_futex_key_refs(union futex_key *key)
 {
  
@@ -641,8 +647,14 @@
 	return pi_state;
 }
  
+/*
+ * Must be called with the hb lock held.
+ */
 static void free_pi_state(struct futex_pi_state *pi_state)
 {
+	if (!pi_state)
+		return;
+
 	if (!atomic_dec_and_test(&pi_state->refcount))
 		return;
  
@@ -1521,15 +1533,6 @@
 	}
  
 retry:
-	if (pi_state != NULL) {
-		/*
-		 * We will have to lookup the pi_state again, so free this one
-		 * to keep the accounting correct.
-		 */
-		free_pi_state(pi_state);
-		pi_state = NULL;
-	}
-
 	ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1, VERIFY_READ);
 	if (unlikely(ret != 0))
 		goto out;
@@ -1619,6 +1622,8 @@
 		case 0:
 			break;
 		case -EFAULT:
+			free_pi_state(pi_state);
+			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
 			put_futex_key(&key2);
@@ -1634,6 +1639,8 @@
 			 *   exit to complete.
 			 * - The user space value changed.
 			 */
+			free_pi_state(pi_state);
+			pi_state = NULL;
 			double_unlock_hb(hb1, hb2);
 			hb_waiters_dec(hb2);
 			put_futex_key(&key2);
@@ -1710,6 +1717,7 @@
 	}
  
 out_unlock:
+	free_pi_state(pi_state);
 	double_unlock_hb(hb1, hb2);
 	hb_waiters_dec(hb2);
  
@@ -1727,8 +1735,6 @@
 out_put_key1:
 	put_futex_key(&key1);
 out:
-	if (pi_state != NULL)
-		free_pi_state(pi_state);
 	return ret ? ret : task_count;
 }
...	...	@@ -143,9 +143,8 @@
143	143	*
144	144	* Where (A) orders the waiters increment and the futex value read through
145	145	* atomic operations (see hb_waiters_inc) and where (B) orders the write
146		- * to futex and the waiters read -- this is done by the barriers in
147		- * get_futex_key_refs(), through either ihold or atomic_inc, depending on the
148		- * futex type.
	146	+ * to futex and the waiters read -- this is done by the barriers for both
	147	+ * shared and private futexes in get_futex_key_refs().
149	148	*
150	149	* This yields the following case (where X:=waiters, Y:=futex):
151	150	*
152	151
...	...	@@ -344,13 +343,20 @@
344	343	futex_get_mm(key); /* implies MB (B) */
345	344	break;
346	345	default:
	346	+ /*
	347	+ * Private futexes do not hold reference on an inode or
	348	+ * mm, therefore the only purpose of calling get_futex_key_refs
	349	+ * is because we need the barrier for the lockless waiter check.
	350	+ */
347	351	smp_mb(); /* explicit MB (B) */
348	352	}
349	353	}
350	354
351	355	/*
352	356	* Drop a reference to the resource addressed by a key.
353		- * The hash bucket spinlock must not be held.
	357	+ * The hash bucket spinlock must not be held. This is
	358	+ * a no-op for private futexes, see comment in the get
	359	+ * counterpart.
354	360	*/
355	361	static void drop_futex_key_refs(union futex_key *key)
356	362	{
357	363
...	...	@@ -641,8 +647,14 @@
641	647	return pi_state;
642	648	}
643	649
	650	+/*
	651	+ * Must be called with the hb lock held.
	652	+ */
644	653	static void free_pi_state(struct futex_pi_state *pi_state)
645	654	{
	655	+ if (!pi_state)
	656	+ return;
	657	+
646	658	if (!atomic_dec_and_test(&pi_state->refcount))
647	659	return;
648	660
...	...	@@ -1521,15 +1533,6 @@
1521	1533	}
1522	1534
1523	1535	retry:
1524		- if (pi_state != NULL) {
1525		- /*
1526		- * We will have to lookup the pi_state again, so free this one
1527		- * to keep the accounting correct.
1528		- */
1529		- free_pi_state(pi_state);
1530		- pi_state = NULL;
1531		- }
1532		-
1533	1536	ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1, VERIFY_READ);
1534	1537	if (unlikely(ret != 0))
1535	1538	goto out;
...	...	@@ -1619,6 +1622,8 @@
1619	1622	case 0:
1620	1623	break;
1621	1624	case -EFAULT:
	1625	+ free_pi_state(pi_state);
	1626	+ pi_state = NULL;
1622	1627	double_unlock_hb(hb1, hb2);
1623	1628	hb_waiters_dec(hb2);
1624	1629	put_futex_key(&key2);
...	...	@@ -1634,6 +1639,8 @@
1634	1639	* exit to complete.
1635	1640	* - The user space value changed.
1636	1641	*/
	1642	+ free_pi_state(pi_state);
	1643	+ pi_state = NULL;
1637	1644	double_unlock_hb(hb1, hb2);
1638	1645	hb_waiters_dec(hb2);
1639	1646	put_futex_key(&key2);
...	...	@@ -1710,6 +1717,7 @@
1710	1717	}
1711	1718
1712	1719	out_unlock:
	1720	+ free_pi_state(pi_state);
1713	1721	double_unlock_hb(hb1, hb2);
1714	1722	hb_waiters_dec(hb2);
1715	1723
...	...	@@ -1727,8 +1735,6 @@
1727	1735	out_put_key1:
1728	1736	put_futex_key(&key1);
1729	1737	out:
1730		- if (pi_state != NULL)
1731		- free_pi_state(pi_state);
1732	1738	return ret ? ret : task_count;
1733	1739	}
1734	1740