[patch 0/3] futex_compat fixes

[patch 0/3] futex_compat fixes

[arnd]

After looking at the futex_compat code some more, this is what I
ended up with. Please look closely at the changes, as I have not
actually tested them, being on a 32 bit machine at the moment.

I'd suggest merging the first patch in 2.6.23 instead of the original
one from David Miller, and the other two for 2.6.24.

	Arnd <><

-- 

[patch 1/3] futex_compat: fix list traversal bugs

[arnd]

[-- Attachment #1: futex_compat.diff --]
[-- Type: text/plain, Size: 1735 bytes --]

The futex list traversal on the compat side appears to have
a bug.

It's loop termination condition compares:

        while (compat_ptr(uentry) != &head->list)

But that can't be right because "uentry" has the special
"pi" indicator bit still potentially set at bit 0.  This
is cleared by fetch_robust_entry() into the "entry"
return value.

What this seems to mean is that the list won't terminate
when list iteration gets back to the the head.  And we'll
also process the list head like a normal entry, which could
cause all kinds of problems.

So we should check for equality with "entry".  That pointer
is of the non-compat type so we have to do a little casting
to keep the compiler and sparse happy.

The same problem can in theory occur with the 'pending'
variable, although that has not been reported from users
so far.

Based on the original patch from David Miller.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Miller <davem@davemloft.net>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Index: linux-2.6/kernel/futex_compat.c
===================================================================
--- linux-2.6.orig/kernel/futex_compat.c
+++ linux-2.6/kernel/futex_compat.c
@@ -61,10 +61,10 @@ void compat_exit_robust_list(struct task
 	if (fetch_robust_entry(&upending, &pending,
 			       &head->list_op_pending, &pip))
 		return;
-	if (upending)
+	if (pending)
 		handle_futex_death((void __user *)pending + futex_offset, curr, pip);
 
-	while (compat_ptr(uentry) != &head->list) {
+	while (entry != (struct robust_list __user *) &head->list) {
 		/*
 		 * A pending lock might already be on the list, so
 		 * dont process it twice:

-- 

Re: [patch 1/3] futex_compat: fix list traversal bugs

[Ingo Molnar]

* arnd@arndb.de <arnd@arndb.de> wrote:

> The futex list traversal on the compat side appears to have a bug.
> 
> It's loop termination condition compares:
> 
>         while (compat_ptr(uentry) != &head->list)
> 
> But that can't be right because "uentry" has the special "pi" 
> indicator bit still potentially set at bit 0.  This is cleared by 
> fetch_robust_entry() into the "entry" return value.

hm. I remember having tested this - not well enough i guess :-/

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

[patch 2/3] futex_compat: simplify pointer magic

[arnd]

[-- Attachment #1: futex_compat_type.diff --]
[-- Type: text/plain, Size: 3119 bytes --]

The futex_compat code currently mixes pointers to robust_list
and compat_robust_list for no apparent reason other than
requiring extra typecasts. It also uses an extra argument
to fetch_robust_entry() that is not useful but has caused
bugs to be introduced before.

Clean up the code to get rid of all this and make the compat
version look more like the native one.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Miller <davem@davemloft.net>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Index: linux-2.6/kernel/futex_compat.c
===================================================================
--- linux-2.6.orig/kernel/futex_compat.c
+++ linux-2.6/kernel/futex_compat.c
@@ -16,15 +16,17 @@
 /*
  * Fetch a robust-list pointer. Bit 0 signals PI futexes:
  */
-static inline int
-fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **entry,
-		   compat_uptr_t __user *head, int *pi)
+static inline int fetch_robust_entry(struct compat_robust_list __user **entry,
+				     compat_uptr_t __user *head,
+				     unsigned int *pi)
 {
-	if (get_user(*uentry, head))
+	compat_uptr_t uentry;
+
+	if (get_user(uentry, head))
 		return -EFAULT;
 
-	*entry = compat_ptr((*uentry) & ~1);
-	*pi = (unsigned int)(*uentry) & 1;
+	*entry = compat_ptr((uentry) & ~1);
+	*pi = (unsigned int)(uentry) & 1;
 
 	return 0;
 }
@@ -38,16 +40,16 @@ fetch_robust_entry(compat_uptr_t *uentry
 void compat_exit_robust_list(struct task_struct *curr)
 {
 	struct compat_robust_list_head __user *head = curr->compat_robust_list;
-	struct robust_list __user *entry, *pending;
+	struct compat_robust_list __user *entry, *pending;
 	unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
-	compat_uptr_t uentry, upending;
+
 	compat_long_t futex_offset;
 
 	/*
 	 * Fetch the list head (which was registered earlier, via
 	 * sys_set_robust_list()):
 	 */
-	if (fetch_robust_entry(&uentry, &entry, &head->list.next, &pi))
+	if (fetch_robust_entry(&entry, &head->list.next, &pi))
 		return;
 	/*
 	 * Fetch the relative futex offset:
@@ -58,13 +60,12 @@ void compat_exit_robust_list(struct task
 	 * Fetch any possibly pending lock-add first, and handle it
 	 * if it exists:
 	 */
-	if (fetch_robust_entry(&upending, &pending,
-			       &head->list_op_pending, &pip))
+	if (fetch_robust_entry(&pending, &head->list_op_pending, &pip))
 		return;
 	if (pending)
 		handle_futex_death((void __user *)pending + futex_offset, curr, pip);
 
-	while (entry != (struct robust_list __user *) &head->list) {
+	while (entry != &head->list) {
 		/*
 		 * A pending lock might already be on the list, so
 		 * dont process it twice:
@@ -73,12 +74,10 @@ void compat_exit_robust_list(struct task
 			if (handle_futex_death((void __user *)entry + futex_offset,
 						curr, pi))
 				return;
-
 		/*
 		 * Fetch the next entry in the list:
 		 */
-		if (fetch_robust_entry(&uentry, &entry,
-				       (compat_uptr_t __user *)&entry->next, &pi))
+		if (fetch_robust_entry(&entry, &entry->next, &pi))
 			return;
 		/*
 		 * Avoid excessively long or circular lists:

-- 

Re: [patch 2/3] futex_compat: simplify pointer magic

[Ingo Molnar]

* arnd@arndb.de <arnd@arndb.de> wrote:

> The futex_compat code currently mixes pointers to robust_list and 
> compat_robust_list for no apparent reason other than requiring extra 
> typecasts. It also uses an extra argument to fetch_robust_entry() that 
> is not useful but has caused bugs to be introduced before.
> 
> Clean up the code to get rid of all this and make the compat version 
> look more like the native one.

yeah.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

[patch 3/3] futex_compat: update to match native version

[arnd]

[-- Attachment #1: futex_compat_clean.diff --]
[-- Type: text/plain, Size: 1752 bytes --]

A few changes have gone into the futex code but not into
the futex_compat code, the most significant one being a
fix for FUTEX_WAKE_OP in commit
f54f098612d7f86463b5fb4763d03533d634de73.

This brings both versions in sync again.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Miller <davem@davemloft.net>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Index: linux-2.6/kernel/futex_compat.c
===================================================================
--- linux-2.6.orig/kernel/futex_compat.c
+++ linux-2.6/kernel/futex_compat.c
@@ -62,8 +62,10 @@ void compat_exit_robust_list(struct task
 	 */
 	if (fetch_robust_entry(&pending, &head->list_op_pending, &pip))
 		return;
+
 	if (pending)
-		handle_futex_death((void __user *)pending + futex_offset, curr, pip);
+		handle_futex_death((void __user *)pending + futex_offset,
+				   curr, pip);
 
 	while (entry != &head->list) {
 		/*
@@ -142,7 +144,7 @@ asmlinkage long compat_sys_futex(u32 __u
 {
 	struct timespec ts;
 	ktime_t t, *tp = NULL;
-	int val2 = 0;
+	u32 val2 = 0;
 	int cmd = op & FUTEX_CMD_MASK;
 
 	if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI)) {
@@ -156,8 +158,13 @@ asmlinkage long compat_sys_futex(u32 __u
 			t = ktime_add(ktime_get(), t);
 		tp = &t;
 	}
-	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE)
-		val2 = (int) (unsigned long) utime;
+	/*
+	 * requeue parameter in 'utime' if cmd == FUTEX_REQUEUE.
+	 * number of waiters to wake in 'utime' if cmd == FUTEX_WAKE_OP.
+	 */
+	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE ||
+	    cmd == FUTEX_WAKE_OP)
+		val2 = (u32) (unsigned long) utime;
 
 	return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
 }

-- 

Re: [patch 3/3] futex_compat: update to match native version

[Ingo Molnar]

* arnd@arndb.de <arnd@arndb.de> wrote:

> A few changes have gone into the futex code but not into the 
> futex_compat code, the most significant one being a fix for 
> FUTEX_WAKE_OP in commit f54f098612d7f86463b5fb4763d03533d634de73.
> 
> This brings both versions in sync again.

> -	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE)
> -		val2 = (int) (unsigned long) utime;
> +	/*
> +	 * requeue parameter in 'utime' if cmd == FUTEX_REQUEUE.
> +	 * number of waiters to wake in 'utime' if cmd == FUTEX_WAKE_OP.
> +	 */
> +	if (cmd == FUTEX_REQUEUE || cmd == FUTEX_CMP_REQUEUE ||
> +	    cmd == FUTEX_WAKE_OP)
> +		val2 = (u32) (unsigned long) utime;

thanks.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo