linux-kernel - Re: [PATCH 5/7] block: Remove superflous rcu_read_[un]lock_sched() in blk_queue

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:   Tue, 6 Mar 2018 17:52:50 +0000
From:   Bart Van Assche <Bart.VanAssche@....com>
To:     "torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>,
        "tj@...nel.org" <tj@...nel.org>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "paulmck@...ux.vnet.ibm.com" <paulmck@...ux.vnet.ibm.com>,
        "bcrl@...ck.org" <bcrl@...ck.org>,
        "kent.overstreet@...il.com" <kent.overstreet@...il.com>
CC:     "security@...nel.org" <security@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel-team@...com" <kernel-team@...com>,
        "axboe@...nel.dk" <axboe@...nel.dk>
Subject: Re: [PATCH 5/7] block: Remove superflous rcu_read_[un]lock_sched() in
 blk_queue_enter()

On Tue, 2018-03-06 at 09:33 -0800, Tejun Heo wrote:
> 3a0a529971ec ("block, scsi: Make SCSI quiesce and resume work
> reliably") added rcu_read_[un]lock_sched() to blk_queue_enter() along
> with other changes but it doesn't seem to be doing anything.
> 
> blk_queue_enter() is called with @q - the pointer to the target
> request_queue, so the caller obviously has to guarantee that @q can be
> dereferenced, and inside the RCU-sched protected area, there's nothing
> which needs further protection.
> 
> Let's remove the superflous rcu_read_[un]lock_sched().
> 
> [ ... ]
>
> This came up while auditing percpu_ref users for problematic RCU
> usages.  I couldn't understand what the RCU protection was doing.
> It'd be great if you can take a look and tell me whether I missed
> something.

Hello Tejun,

I think the rcu_read_lock() and rcu_read_unlock() are really necessary in this
code. In the LWN article "The RCU-barrier menagerie" it is explained that RCU
can be used to enforce write ordering globally if the code that reads the writes
that are ordered with an RCU read lock (https://lwn.net/Articles/573497/). See
also the following comment in scsi_device_quiesce():

	/*
	 * Ensure that the effect of blk_set_preempt_only() will be visible
	 * for percpu_ref_tryget() callers that occur after the queue
	 * unfreeze even if the queue was already frozen before this function
	 * was called. See also https://lwn.net/Articles/573497/.
	 */

Since this patch introduces a subtle and hard to debug race condition, please
drop this patch.

Thanks,

Bart.