linux-kernel - Re: [PATCH] locking, rwsem: Fix down_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160512165958.GC4940@dhcp22.suse.cz>
Date:	Thu, 12 May 2016 18:59:58 +0200
From:	Michal Hocko <mhocko@...nel.org>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	LKML <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	"David S. Miller" <davem@...emloft.net>,
	Tony Luck <tony.luck@...el.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Chris Zankel <chris@...kel.net>,
	Max Filippov <jcmvbkbc@...il.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	Waiman Long <Waiman.Long@....com>
Subject: Re: [PATCH] locking, rwsem: Fix down_write_killable()

On Thu 12-05-16 13:57:45, Peter Zijlstra wrote:
[...]
> Subject: locking, rwsem: Fix down_write_killable()
> From: Peter Zijlstra <peterz@...radead.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@...emloft.net>
> Cc: Waiman Long <Waiman.Long@....com>
> Cc: Chris Zankel <chris@...kel.net>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Davidlohr Bueso <dave@...olabs.net>
> Cc: Tony Luck <tony.luck@...el.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: Max Filippov <jcmvbkbc@...il.com>
> Reported-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Tested-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Tested-by: Michal Hocko <mhocko@...nel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.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