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]

Message-ID: <20180314184636.GC2943022@devbig577.frc2.facebook.com>
Date:   Wed, 14 Mar 2018 11:46:36 -0700
From:   "tj@...nel.org" <tj@...nel.org>
To:     Bart Van Assche <Bart.VanAssche@....com>
Cc:     "torvalds@...ux-foundation.org" <torvalds@...ux-foundation.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>,
        "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()

Hello, Bart.

On Tue, Mar 06, 2018 at 05:52:50PM +0000, Bart Van Assche wrote:
> 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

Yeah but, for it to be meaningful, just like barriers, it has to be
paired with something on the other side.

> 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.

Hah, the pairing is between scsi_device_quiesce() and
blk_queue_enter()?  But that doesn't make sense either because
scsi_device_quiesce() is doing regular synchronize_rcu() and
blk_queue_entre() is doing rcu_read_lock_sched().  They don't
interlock with each other in any way.

So, the right thing to do here would be somehow moving the RCU
synchronization into blk_set_preempt_only() and switching to regular
RCU protection in blk_queue_enter().  The code as-is isn't really
doing anything.

I'll drop this patch from the series for now.  Let's revisit it later.

Thanks.

-- 
tejun