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

Message-ID: <20170629195322.GB9745@htj.duckdns.org>
Date:   Thu, 29 Jun 2017 15:53:22 -0400
From:   Tejun Heo <tj@...nel.org>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()?

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.

> One of the uses of spin_unlock_wait() is in ata_scsi_cmd_error_handler()
> in the file drivers/ata/libata-eh.c.  Your commit ad9e27624479b
> ("libata-eh-fw: update ata_scsi_error() for new EH") last touched it,
> though it predates that commit.
> 
> My question to you is whether the code in ata_scsi_cmd_error_handler()
> needs release semantics.  If it does, my recommendation is to replace
> the spin_unlock_wait(ap->lock) with this (adding the needed curly braces,
> of course):
> 
> 	spin_lock(ap->lock);
> 	spin_unlock(ap->lock);
> 
> If the code only needs acquire semantics, no change required.
> 
> If your code requires release semantics, and there is some reason why
> my suggested replacement above is a bad idea, please let me know!

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.

Thanks.

-- 
tejun