linux-kernel - Re: [BUG]locking/rwsem: only clean RWSEM_FLAG

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]

Date:   Thu, 11 Nov 2021 10:42:10 +0800
From:   Maria Yu <quic_aiquny@...cinc.com>
To:     <peterz@...radead.org>
CC:     <boqun.feng@...il.com>, <hdanton@...a.com>,
        <linux-kernel@...r.kernel.org>, <longman@...hat.com>,
        <mazhenhua@...omi.com>, <mingo@...hat.com>, <will@...nel.org>,
        <quic_aiquny@...cinc.com>
Subject: Re: [BUG]locking/rwsem: only clean RWSEM_FLAG_HANDOFF when already set

From: quic_aiquny@...cinc.com

There is an important information that when the issue reported, the first waiter can be changed. This is via other hook changes which can not always add to the tail of the waitlist which is not the same of the current upstream code.

The current original Xiaomi's issue when we checked the log, we can find out that the scenario is that write waiter set the sem->count with RWSEM_FLAG_HANDOFF bit and then another reader champ in and got the first waiter.
So When the reader go through rwsem_down_read_slowpath if it have RWSEM_FLAG_HANDOFF bit set, it will clear the RWSEM_FLAG_HANDOFF bit.

So it left the wstate of the local variable as WRITER_HANDOFF, but the reader thought it was a Reader Handoff and cleared it.
While the writer only checkes the wstate local variable of WRITER_HANDOFF, and do the add(-RWSEM_FLAG_HANDOFF) action. If it do a addnot(RWSEM_FLAG_HANDOFF), then the writer will not make the sem->count underflow.

The current scenario looked like this:
--------------------------------
	wstate = WRITER_HANDOFF;
	raw_spin_lock_irq(&sem->wait_lock);
	if (rwsem_try_write_lock(sem, wstate))
	raw_spin_unlock_irq(&sem->wait_lock);
	   :
	   if (signal_pending_state(state, current))       //other *important* reader rwsem_down_read_slowpath add to the first of the waitlist, and clear the RWSEM_FLAG_HANDOFF
	     goto out_nolock
	     out_nolock:
	     add(-RWSEM_FLAG_HANDOFF, sem->count)                     // make the count to be negative.
--------------------------------

So if we do a andnot it will be much more safer, and working in this scenario:
--------------------------------
	wstate = WRITER_HANDOFF;
	raw_spin_lock_irq(&sem->wait_lock);
	if (rwsem_try_write_lock(sem, wstate))
	raw_spin_unlock_irq(&sem->wait_lock);
	   :
	   if (signal_pending_state(state, current))       //other *important* reader rwsem_down_read_slowpath add to the first of the waitlist, and clear the RWSEM_FLAG_HANDOFF
	     goto out_nolock
	     out_nolock:
	     addnot(RWSEM_FLAG_HANDOFF, sem->count)                // make the count unchanged.
--------------------------------


So if we do a andnot it will be much more safer, and working in the not killed normal senario:
--------------------------------
	wstate = WRITER_HANDOFF;
	raw_spin_lock_irq(&sem->wait_lock);
	if (rwsem_try_write_lock(sem, wstate))
	raw_spin_unlock_irq(&sem->wait_lock);
	   :
	   if (signal_pending_state(state, current))       //other *important* reader rwsem_down_read_slowpath add to the first of the waitlist, and clear the RWSEM_FLAG_HANDOFF
		if (wstate == WRITER_HANDOFF)
			trylock_again:
			raw_spin_lock_irq(&sem->wait_lock);
			if (rwsem_try_write_lock(sem, wstate))		//writer will set the RWSEM_FLAG_HANDOFF flag again.
	raw_spin_unlock_irq(&sem->wait_lock);
--------------------------------



Thx and BRs,
Aiqun Yu (Maria)