[PATCH 0/4] Various rwsem ACQUIRE fixes

[PATCH 0/4] Various rwsem ACQUIRE fixes

[Peter Zijlstra]

These are the patches I ended up with after we started with Jan's patch (edited).

[PATCH 1/4] locking/rwsem: Add missing ACQUIRE to read_slowpath exit when queue is empty

[Peter Zijlstra]

From: Jan Stancek <jstancek@redhat.com>

LTP mtest06 has been observed to occasionally hit "still mapped when
deleted" and following BUG_ON on arm64.

The extra mapcount originated from pagefault handler, which handled
pagefault for vma that has already been detached. vma is detached
under mmap_sem write lock by detach_vmas_to_be_unmapped(), which
also invalidates vmacache.

When the pagefault handler (under mmap_sem read lock) calls
find_vma(), vmacache_valid() wrongly reports vmacache as valid.

After rwsem down_read() returns via 'queue empty' path (as of v5.2),
it does so without an ACQUIRE on sem->count:

  down_read()
    __down_read()
      rwsem_down_read_failed()
        __rwsem_down_read_failed_common()
          raw_spin_lock_irq(&sem->wait_lock);
          if (list_empty(&sem->wait_list)) {
            if (atomic_long_read(&sem->count) >= 0) {
              raw_spin_unlock_irq(&sem->wait_lock);
              return sem;

The problem can be reproduced by running LTP mtest06 in a loop and
building the kernel (-j $NCPUS) in parallel. It does reproduces since
v4.20 on arm64 HPE Apollo 70 (224 CPUs, 256GB RAM, 2 nodes). It
triggers reliably in about an hour.

The patched kernel ran fine for 10+ hours.

Cc: dbueso@suse.de
Cc: mingo@redhat.com
Fixes: 4b486b535c33 ("locking/rwsem: Exit read lock slowpath if queue empty & no writer")
Signed-off-by: Jan Stancek <jstancek@redhat.com>
Reviewed-by: Will Deacon <will@kernel.org>
Acked-by: Waiman Long <longman@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/50b8914e20d1d62bb2dee42d342836c2c16ebee7.1563438048.git.jstancek@redhat.com
---
 kernel/locking/rwsem.c |    2 ++
 1 file changed, 2 insertions(+)

--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -1032,6 +1032,8 @@ rwsem_down_read_slowpath(struct rw_semap
 		 */
 		if (adjustment && !(atomic_long_read(&sem->count) &
 		     (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))) {
+			/* Provide lock ACQUIRE */
+			smp_acquire__after_ctrl_dep();
 			raw_spin_unlock_irq(&sem->wait_lock);
 			rwsem_set_reader_owned(sem);
 			lockevent_inc(rwsem_rlock_fast);

[PATCH 2/4] rwsem: Add missing ACQUIRE to read_slowpath sleep loop

[Peter Zijlstra]

While reviewing another read_slowpath patch, both Will and I noticed
another missing ACQUIRE, namely:

  X = 0;

  CPU0			CPU1

  rwsem_down_read()
    for (;;) {
      set_current_state(TASK_UNINTERRUPTIBLE);

                        X = 1;
                        rwsem_up_write();
                          rwsem_mark_wake()
                            atomic_long_add(adjustment, &sem->count);
                            smp_store_release(&waiter->task, NULL);

      if (!waiter.task)
        break;

      ...
    }

  r = X;

Allows 'r == 0'.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reported-by: Will Deacon <will@kernel.org>
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/locking/rwsem.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -1069,8 +1069,10 @@ rwsem_down_read_slowpath(struct rw_semap
 	/* wait to be given the lock */
 	while (true) {
 		set_current_state(state);
-		if (!waiter.task)
+		if (!smp_load_acquire(&waiter.task)) {
+			/* Orders against rwsem_mark_wake()'s smp_store_release() */
 			break;
+		}
 		if (signal_pending_state(state, current)) {
 			raw_spin_lock_irq(&sem->wait_lock);
 			if (waiter.task)

[PATCH 3/4] tty/ldsem: Add missing ACQUIRE to read_failed sleep loop

[Peter Zijlstra]

While reviewing rwsem down_slowpath, Will noticed ldsem had a copy of
a bug we just found for rwsem.

  X = 0;

  CPU0			CPU1

  rwsem_down_read()
    for (;;) {
      set_current_state(TASK_UNINTERRUPTIBLE);

                        X = 1;
                        rwsem_up_write();
                          rwsem_mark_wake()
                            atomic_long_add(adjustment, &sem->count);
                            smp_store_release(&waiter->task, NULL);

      if (!waiter.task)
        break;

      ...
    }

  r = X;

Allows 'r == 0'.

Cc: Peter Hurley <peter@hurleysoftware.com>
Fixes: 4898e640caf0 ("tty: Add timed, writer-prioritized rw semaphore")
Reported-by: Will Deacon <will@kernel.org>
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/tty/tty_ldsem.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

--- a/drivers/tty/tty_ldsem.c
+++ b/drivers/tty/tty_ldsem.c
@@ -93,8 +93,7 @@ static void __ldsem_wake_readers(struct
 
 	list_for_each_entry_safe(waiter, next, &sem->read_wait, list) {
 		tsk = waiter->task;
-		smp_mb();
-		waiter->task = NULL;
+		smp_store_release(&waiter->task, NULL);
 		wake_up_process(tsk);
 		put_task_struct(tsk);
 	}
@@ -194,7 +193,7 @@ down_read_failed(struct ld_semaphore *se
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 
-		if (!waiter.task)
+		if (!smp_load_acquire(&waiter.task))
 			break;
 		if (!timeout)
 			break;

[PATCH 4/4] rwsem: Add ACQUIRE comments

[Peter Zijlstra]

Since we just reviewed read_slowpath for ACQUIRE correctness, add a
few coments to retain our findings.

Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/locking/rwsem.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -1000,6 +1000,7 @@ rwsem_down_read_slowpath(struct rw_semap
 	atomic_long_add(-RWSEM_READER_BIAS, &sem->count);
 	adjustment = 0;
 	if (rwsem_optimistic_spin(sem, false)) {
+		/* rwsem_optimistic_spin() implies ACQUIRE on success */
 		/*
 		 * Wake up other readers in the wait list if the front
 		 * waiter is a reader.
@@ -1014,6 +1015,7 @@ rwsem_down_read_slowpath(struct rw_semap
 		}
 		return sem;
 	} else if (rwsem_reader_phase_trylock(sem, waiter.last_rowner)) {
+		/* rwsem_reader_phase_trylock() implies ACQUIRE on success */
 		return sem;
 	}
 
@@ -1067,10 +1069,10 @@ rwsem_down_read_slowpath(struct rw_semap
 	wake_up_q(&wake_q);
 
 	/* wait to be given the lock */
-	while (true) {
+	for (;;) {
 		set_current_state(state);
 		if (!smp_load_acquire(&waiter.task)) {
-			/* Orders against rwsem_mark_wake()'s smp_store_release() */
+			/* Matches rwsem_mark_wake()'s smp_store_release(). */
 			break;
 		}
 		if (signal_pending_state(state, current)) {
@@ -1078,6 +1080,7 @@ rwsem_down_read_slowpath(struct rw_semap
 			if (waiter.task)
 				goto out_nolock;
 			raw_spin_unlock_irq(&sem->wait_lock);
+			/* Ordered by sem->wait_lock against rwsem_mark_wake(). */
 			break;
 		}
 		schedule();
@@ -1087,6 +1090,7 @@ rwsem_down_read_slowpath(struct rw_semap
 	__set_current_state(TASK_RUNNING);
 	lockevent_inc(rwsem_rlock);
 	return sem;
+
 out_nolock:
 	list_del(&waiter.list);
 	if (list_empty(&sem->wait_list)) {
@@ -1127,8 +1131,10 @@ rwsem_down_write_slowpath(struct rw_sema
 
 	/* do optimistic spinning and steal lock if possible */
 	if (rwsem_can_spin_on_owner(sem, RWSEM_WR_NONSPINNABLE) &&
-	    rwsem_optimistic_spin(sem, true))
+	    rwsem_optimistic_spin(sem, true)) {
+		/* rwsem_optimistic_spin() implies ACQUIRE on success */
 		return sem;
+	}
 
 	/*
 	 * Disable reader optimistic spinning for this rwsem after
@@ -1188,9 +1194,11 @@ rwsem_down_write_slowpath(struct rw_sema
 wait:
 	/* wait until we successfully acquire the lock */
 	set_current_state(state);
-	while (true) {
-		if (rwsem_try_write_lock(sem, wstate))
+	for (;;) {
+		if (rwsem_try_write_lock(sem, wstate)) {
+			/* rwsem_try_write_lock() implies ACQUIRE on success */
 			break;
+		}
 
 		raw_spin_unlock_irq(&sem->wait_lock);
 

Re: [PATCH 2/4] rwsem: Add missing ACQUIRE to read_slowpath sleep loop

[Waiman Long]

On 7/18/19 9:49 AM, Peter Zijlstra wrote:
> While reviewing another read_slowpath patch, both Will and I noticed
> another missing ACQUIRE, namely:
>
>   X = 0;
>
>   CPU0			CPU1
>
>   rwsem_down_read()
>     for (;;) {
>       set_current_state(TASK_UNINTERRUPTIBLE);
>
>                         X = 1;
>                         rwsem_up_write();
>                           rwsem_mark_wake()
>                             atomic_long_add(adjustment, &sem->count);
>                             smp_store_release(&waiter->task, NULL);
>
>       if (!waiter.task)
>         break;
>
>       ...
>     }
>
>   r = X;
>
> Allows 'r == 0'.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reported-by: Will Deacon <will@kernel.org>
> Acked-by: Will Deacon <will@kernel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/locking/rwsem.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -1069,8 +1069,10 @@ rwsem_down_read_slowpath(struct rw_semap
>  	/* wait to be given the lock */
>  	while (true) {
>  		set_current_state(state);
> -		if (!waiter.task)
> +		if (!smp_load_acquire(&waiter.task)) {
> +			/* Orders against rwsem_mark_wake()'s smp_store_release() */
>  			break;
> +		}
>  		if (signal_pending_state(state, current)) {
>  			raw_spin_lock_irq(&sem->wait_lock);
>  			if (waiter.task)
>
>
Acked-by: Waiman Long <longman@redhat.com>

Thanks for fixing this old problem.

This problem does not apply to x86 and we do our testing mostly on x86.
That is why we seldom notice this kind of issue. Maybe we should be
doing more testing on ARM64 and PPC.

Cheers,
Longman

Re: [PATCH 1/4] locking/rwsem: Add missing ACQUIRE to read_slowpath exit when queue is empty

[Andrea Parri]

> @@ -1032,6 +1032,8 @@ rwsem_down_read_slowpath(struct rw_semap
>  		 */
>  		if (adjustment && !(atomic_long_read(&sem->count) &
>  		     (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))) {
> +			/* Provide lock ACQUIRE */
> +			smp_acquire__after_ctrl_dep();

Does this also make the lock RCtso?  Or maybe RCtso was already
guaranteed (and I'm failing to see why)?

Thanks,
  Andrea

Re: [PATCH 0/4] Various rwsem ACQUIRE fixes

[Davidlohr Bueso]

On 2019-07-18 06:49, Peter Zijlstra wrote:
> These are the patches I ended up with after we started with Jan's
> patch (edited).

This series looks good to me.

Acked-by: Davidlohr Bueso <dbueso@suse.de>

Re: [PATCH 1/4] locking/rwsem: Add missing ACQUIRE to read_slowpath exit when queue is empty

[Peter Zijlstra]

On Fri, Jul 19, 2019 at 10:49:56AM +0200, Andrea Parri wrote:
> > @@ -1032,6 +1032,8 @@ rwsem_down_read_slowpath(struct rw_semap
> >  		 */
> >  		if (adjustment && !(atomic_long_read(&sem->count) &
> >  		     (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))) {
> > +			/* Provide lock ACQUIRE */
> > +			smp_acquire__after_ctrl_dep();
> 
> Does this also make the lock RCtso?  Or maybe RCtso was already
> guaranteed (and I'm failing to see why)?

I didn't specifically look for that, but I suspect not, esp given the
next patch.