potential NULL dereference in futex_wait_requeue_pi()

Hi Darren,

The patch 52400ba94675: "futex: add requeue_pi functionality" from Apr 3, 2009, leads to the following warning:
kernel/futex.c:2373 futex_wait_requeue_pi()
error: potential NULL dereference 'pi_mutex'.

Quoted 6 lines · Dan Carpenter

Hi Darren,

The patch 52400ba94675: "futex: add requeue_pi functionality" from Apr 3, 2009, leads to the following warning:
kernel/futex.c:2373 futex_wait_requeue_pi()
error: potential NULL dereference 'pi_mutex'.

This sounds like the oops I saw that I reported at https://lkml.org/lkml/2012/7/13/328

Quoted 8 lines · Dave Jones

Quoted 6 lines · Dan Carpenter

Hi Darren,

The patch 52400ba94675: "futex: add requeue_pi functionality" from Apr 3, 2009, leads to the following warning:
kernel/futex.c:2373 futex_wait_requeue_pi()
error: potential NULL dereference 'pi_mutex'.

Quoted 1 line

This sounds like the oops I saw that I reported at https://lkml.org/lkml/2012/7/13/328

I'm looking into the report Dave sent today (delayed from piles of urgent issues after returning from vacation, my apologies).

Dan, under which workload are you seeing this?

Hi Darren,

The patch 52400ba94675: "futex: add requeue_pi functionality" from Apr 3, 2009, leads to the following warning:
kernel/futex.c:2373 futex_wait_requeue_pi()
error: potential NULL dereference 'pi_mutex'.

  2330          if (!q.rt_waiter) {
  2331                  /*
  2332                   * Got the lock. We might not be the anticipated owner if we
  2333                   * did a lock-steal - fix up the PI-state in that case.
  2334                   */
  2335                  if (q.pi_state && (q.pi_state->owner != current)) {
  2336                          spin_lock(q.lock_ptr);
  2337                          ret = fixup_pi_state_owner(uaddr2, &q, current);

pi_mutex is NULL here and it looks like fixup_pi_state_owner() can return -EFAULT.

  2338                          spin_unlock(q.lock_ptr);
  2339                  }
  2340          } else {

[snipped]

2366 }
2367
2368 /*

  2369           * If fixup_pi_state_owner() faulted and was unable to handle the
  2370           * fault, unlock the rt_mutex and return the fault to userspace.
  2371           */
  2372          if (ret == -EFAULT) {
  2373                  if (rt_mutex_owner(pi_mutex) == current)
                                           ^^^^^^^^

This will oops if pi_mutex is NULL.

  2374                          rt_mutex_unlock(pi_mutex);
  2375          } else if (ret == -EINTR) {

Quoted 4 lines

This will oops if pi_mutex is NULL.

  2374                          rt_mutex_unlock(pi_mutex);
  2375          } else if (ret == -EINTR) {

Nice Dan, thanks for taking a closer look. This appears to be a simple fix, can you try the following:

futex: Test for pi_mutex on fault in futex_wait_requeue_pi

If fixup_pi_state_owner() faults, pi_mutex may be NULL. Test for pi_mutex != NULL before testing the owner against current and possibly unlocking it.

Signed-off-by Darren Hart <[email protected]>

CC Dave Jones <[email protected]>

CC Dan Carpenter <[email protected]>

CC Thomas Gleixner <[email protected]>

kernel/futex.c +0 -0

index e2b0fb9..05018bf 100644

Quoted 25 lines · Darren Hart

Quoted 4 lines

This will oops if pi_mutex is NULL.

  2374                          rt_mutex_unlock(pi_mutex);
  2375          } else if (ret == -EINTR) {

Nice Dan, thanks for taking a closer look. This appears to be a simple fix, can you try the following:

futex: Test for pi_mutex on fault in futex_wait_requeue_pi

If fixup_pi_state_owner() faults, pi_mutex may be NULL. Test for pi_mutex != NULL before testing the owner against current and possibly unlocking it.

Signed-off-by Darren Hart <[email protected]>

CC Dave Jones <[email protected]>

CC Dan Carpenter <[email protected]>

CC Thomas Gleixner <[email protected]>

kernel/futex.c +0 -0

index e2b0fb9..05018bf 100644

kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2376

Quoted 8 lines

kernel/futex.c +1 -1

@@ -2370,7 +2370,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 	 * fault, unlock the rt_mutex and return the fault to userspace.
 	 */
 	if (ret == -EFAULT) {
-		if (rt_mutex_owner(pi_mutex) == current)
+		if (pi_mutex && rt_mutex_owner(pi_mutex) == current)
 			rt_mutex_unlock(pi_mutex);
 	} else if (ret == -EINTR) {
 		/*

Doesn't fix the oops for me unfortunatly. It looks like it happens further up, so this might be a spearate bug after all.

I added this..

(quoted diff) +6 -0

@@ -2344,7 +2351,13 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
                 * the pi_state.
                 */
                WARN_ON(!&q.pi_state);
+
                pi_mutex = &q.pi_state->pi_mutex;
+               if (pi_mutex == NULL) {
+                       ret = -EINVAL;
+                       goto out;
+               }
+
                ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
 
 

But that didn't seem to fix it either. Somehow we still do this ..

BUG: unable to handle kernel NULL pointer dereference at 0000000000000028

IP: [<ffffffff810d68be>] __lock_acquire+0x5e/0x1ae0

	lock_acquire+0xad/0x220
	_raw_spin_lock+0x46/0x80
	rt_mutex_finish_proxy_lock+0x34/0xe0
	futex_wait_requeue_pi.constprop.20+0x2e5/0x400
	do_futex+0xea/0xa20
	sys_futex+0x107/0x1a0
	system_call_fastpath+0x1a/0x1f

Ah, could it somehow be that we have a pi_mutex here, but it hasn't been initialised ?

The code: line fingers this as the failure in kernel/lockdep.c

        if (lock->key == &__lockdep_no_validate__)
    3f9e:       49 8b 07                mov    (%r15),%rax

r15 (lock) is somehow '0x28' here, which is why the NULL check I added didn't trigger.

This isn't helped by the fact that there seems to be another unrelated bug in futexes that trinity triggers. If you want to try this, running it with "-c futex" will reproduce it very quickly.