Re: [PATCH] locking, rwsem: Fix down_write_killable()

[Michal Hocko <mhocko@kernel.org>]

On Thu 12-05-16 13:57:45, Peter Zijlstra wrote:
[...]
> Subject: locking, rwsem: Fix down_write_killable()
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed, 11 May 2016 11:41:28 +0200
> 
> The new signal_pending exit path in __rwsem_down_write_failed_common()
> was fingered as breaking his kernel by Tetsuo Handa.
> 
> Upon inspection it was found that there are two things wrong with it;
> 
>  - it forgets to remove WAITING_BIAS if it leaves the list empty, or
>  - it forgets to wake further waiters that were blocked on the now
>    removed waiter.
> 
> Especially the first issue causes new lock attempts to block and stall
> indefinitely, as the code assumes that pending waiters mean there is
> an owner that will wake when it releases the lock.

Just to prevent from confusion. I think that the patch is doing clearly
the proper thing. Even if the state WAITING_BIAS was OK for some reason
it would be too subtle and it is better to clean up the state when
failing. So the follow up discussion is just to clarify what the heck
was going on here.

> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Waiman Long <Waiman.Long@hpe.com>
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: Michal Hocko <mhocko@kernel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/locking/rwsem-xadd.c |   21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -487,23 +487,32 @@ __rwsem_down_write_failed_common(struct
>  
>  		/* Block until there are no active lockers. */
>  		do {
> -			if (signal_pending_state(state, current)) {
> -				raw_spin_lock_irq(&sem->wait_lock);
> -				ret = ERR_PTR(-EINTR);
> -				goto out;
> -			}
> +			if (signal_pending_state(state, current))
> +				goto out_nolock;
> +
>  			schedule();
>  			set_current_state(state);
>  		} while ((count = sem->count) & RWSEM_ACTIVE_MASK);
>  
>  		raw_spin_lock_irq(&sem->wait_lock);
>  	}
> -out:
>  	__set_current_state(TASK_RUNNING);
>  	list_del(&waiter.list);
>  	raw_spin_unlock_irq(&sem->wait_lock);
>  
>  	return ret;
> +
> +out_nolock:
> +	__set_current_state(TASK_RUNNING);
> +	raw_spin_lock_irq(&sem->wait_lock);
> +	list_del(&waiter.list);
> +	if (list_empty(&sem->wait_list))
> +		rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
> +	else
> +		__rwsem_do_wake(sem, RWSEM_WAKE_ANY);
> +	raw_spin_unlock_irq(&sem->wait_lock);
> +
> +	return ERR_PTR(-EINTR);
>  }
>  
>  __visible struct rw_semaphore * __sched

-- 
Michal Hocko
SUSE Labs