linux-kernel - Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bd5a8849-e533-7d27-750b-0d89b8918c22@akamai.com>
Date:   Tue, 3 Jan 2017 15:46:48 -0500
From:   Jason Baron <jbaron@...mai.com>
To:     James Bottomley <jejb@...ux.vnet.ibm.com>,
        David Miller <davem@...emloft.net>,
        <Bart.VanAssche@...disk.com>
CC:     <hch@...radead.org>, <linux-kernel@...r.kernel.org>,
        <sagi@...mberg.me>, <sathya.prakash@...adcom.com>,
        <suganath-prabu.subramani@...adcom.com>,
        <martin.petersen@...cle.com>, <hare@...e.de>,
        <linux-scsi@...r.kernel.org>, <hch@....de>,
        <Sreekanth.Reddy@...adcom.com>, <chaitra.basappa@...adcom.com>,
        <dledford@...hat.com>
Subject: Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands

On 01/01/2017 12:39 PM, James Bottomley wrote:
> On Sun, 2017-01-01 at 11:33 -0500, David Miller wrote:
>> From: Bart Van Assche <Bart.VanAssche@...disk.com>
>> Date: Sun, 1 Jan 2017 14:22:11 +0000
>>
>>> My recommendation is to revert commit 18f6084a989b ("scsi: mpt3sas: Fix
>>> secure erase premature termination"). Since the mpt3sas driver uses the
>>> single-queue approach and since the SCSI core unlocks the block layer
>>> request queue lock before the .queuecommand callback function is called,
>>> multiple threads can execute that callback function (scsih_qcmd() in this
>>> case) simultaneously. This means that using scsi_internal_device_block()
>>> from inside .queuecommand to serialize SCSI command execution is wrong.
>>
>> But this causes a regression for the thing being fixed by that
>> commit.
>
> Right, we don't do that; that's why I didn't list it as one of the
> options.
>
>> Why don't we figure out what that semantics that commit was trying to
>> achieve and implement that properly?
>
> Now that I look at the reviews, each of the reviewers said what the
> correct thing to do was: return SAM_STAT_BUSY if SATL commands are
> outstanding like the spec says.  You all get negative brownie points
> for not insisting on a rework.
>
> Does this patch (compile tested only) fix the problems for everyone?
>

Hi,

Yes, with this patch applied my system boots :)

...

> @@ -4083,6 +4077,16 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
>  		return 0;
>  	}
>
> +	/*
> +	 * Bug work around for firmware SATL handling
> +	 */
> +	if (sas_device_priv_data->ata_command_pending) {
> +		scmd->result = SAM_STAT_BUSY;
> +		scmd->scsi_done(scmd);
> +		return 0;
> +	}
> +	set_satl_pending(scmd, true);
> +
>  	sas_target_priv_data = sas_device_priv_data->sas_target;
>
>  	/* invalid device handle */


I was also wondering if 2 threads could both fall through and do:
'set_satl_pending(scmd, true)'; ?

Afaict there is nothing preventing that race?

Thanks,

-Jason