linux-kernel - Re: spin_unlock_wait() in ata_scsi_cmd_error

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] [day] [month] [year] [list]

Date:   Thu, 29 Jun 2017 13:48:00 -0700
From:   "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:     Tejun Heo <tj@...nel.org>
Cc:     linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()?

On Thu, Jun 29, 2017 at 04:17:54PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Thu, Jun 29, 2017 at 01:14:43PM -0700, Paul E. McKenney wrote:
> > On Thu, Jun 29, 2017 at 03:53:22PM -0400, Tejun Heo wrote:
> > > Hello, Paul.
> > > 
> > > On Thu, Jun 29, 2017 at 11:10:57AM -0700, Paul E. McKenney wrote:
> > > > If this code fragment doesn't deadlock, then CPU 0's spin_unlock_wait()
> > > > must have executed before CPU 1's spin_lock().  However, even on x86,
> > > > CPU 0's prior writes can be reordered with its subsequent reads, which
> > > > means that r1 == 0 is possible, which means that the above condition
> > > > could hold, even on x86.
> > > 
> > > I see.  Ah, that's a mind bender.
> > 
> > It has indeed been providing at least its share of entertainment over
> > the past little while.  ;-)
> 
> lol :)
> 
> > > That part of the code should be dead now.  I don't think we no longer
> > > have any driver which doesn't have error handler set.  I should rip
> > > out that if/else.  Also, ACQUIRE semantics should be enough there.
> > > Nothing changes from the EH side there.
> > 
> > It looks like we actually might get rid of spin_unlock_wait entirely.
> > But how about if I just pull the spin_lock_irqsave() before the "if"
> > and the spin_lock_irqrestore() after the "if"?  Same effect, only
> > difference is that the "if" and the "ap->eh_tries = ATA_EH_MAX_TRIES"
> > end up under the lock, and I bet that you won't be able to measure
> > the difference.  (Please see below.)
> > 
> > I will do this because I just now happened to be editing that file on
> > my "eradicate spin_unlock_wait()" quest, but can easily rework the
> > patch as desired.  If you want something different, just let me know!
> 
> Sounds good to me.  That path isn't hot at all.  No change made at
> this level is gonna have any actual impact.  Please go for whatever is
> the simplest.  For moving out the lock/unlock outside if/else,
> 
>  Acked-by: Tejun Heo <tj@...nel.org>

Applied, and thank you!

							Thanx, Paul