linux-kernel - Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

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: <b87d51d8-5a89-480e-b229-7750c241aa6f@kernel.org>
Date: Mon, 17 Jun 2024 08:22:07 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Igor Pylypiv <ipylypiv@...gle.com>, Niklas Cassel <cassel@...nel.org>
Cc: Tejun Heo <tj@...nel.org>, Hannes Reinecke <hare@...e.de>,
 linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when
 ATA ERR/DF are set

On 6/15/24 04:18, Igor Pylypiv wrote:
> SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
> specifies that SATL shall generate sense data for ATA PASS-THROUGH
> commands when either CK_COND is set or when ATA_ERR or ATA_DF status
> bits are set.
> 
> ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
> or ATA_DF bits are set. It looks like qc->err_mask can be used as
> an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
> when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
> indication if no other bits were set in qc->err_mask.
> 
> ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
> commands because qc->err_mask can be zero (i.e. "no error") even when
> the corresponding command has failed with ATA_ERR/ATA_DF bits set.

If there was a failed command, qc->err_mask will be non-zero since the command
completion will be signaled by an error interrupt. So I am confused here. Are
you seeing this happening in practice ? If yes, doing what with which  adapter ?

> Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
> should not prevent SATL from generating sense data for ATA PASS-THROUGH.

Same here, this is strange: ATA_QCFLAG_SENSE_VALID indicates that we have sense
data for the command, regardless if the command is passthrough or not. So if
that flag is set, we should not overwrite the already valid sense data with
sense data generated from the error and status bits.
Do you see an issue without this change ? If yes, what are the adapter and
operations you are running ?

> 
> Signed-off-by: Igor Pylypiv <ipylypiv@...gle.com>
> ---
>  drivers/ata/libata-scsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 032cf11d0bcc..79e8103ef3a9 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
>  
>  	/* For ATA pass thru (SAT) commands, generate a sense block if
> -	 * user mandated it or if there's an error.  Note that if we
> -	 * generate because the user forced us to [CK_COND =1], a check
> +	 * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
> +	 * if we generate because the user forced us to [CK_COND=1], a check
>  	 * condition is generated and the ATA register values are returned
>  	 * whether the command completed successfully or not. If there
>  	 * was no error, we use the following sense data:
> @@ -1641,7 +1641,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
>  	 */
>  	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> -	    ((cdb[2] & 0x20) || need_sense))
> +	    ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))
>  		ata_gen_passthru_sense(qc);
>  	else if (need_sense)
>  		ata_gen_ata_sense(qc);

-- 
Damien Le Moal
Western Digital Research