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

On Wed, 2018-03-14 at 11:46 -0700, tj@...nel.org wrote:
> > 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_enter() is doing rcu_read_lock_sched().  They don't
> interlock with each other in any way.

Can you clarify this further? From <linux/rcupdate.h>:

static inline void synchronize_rcu(void)
{
	synchronize_sched();
}

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

Since the RCU protection in blk_queue_enter() surrounds a percpu_ref_tryget_live()
call and since percpu_ref_tryget_live() calls rcu_read_lock/unlock_sched() itself
I don't think that it is allowed to switch to regular RCU protection in
blk_queue_enter() without modifying the RCU implementation.

Bart.