linux-kernel - Re: [PATCH v1 1/4] ata: libata: Remove redundant sense

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]

Date: Mon, 17 Jun 2024 08:13:51 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Igor Pylypiv <ipylypiv@...gle.com>, Niklas Cassel <cassel@...nel.org>
Cc: Hannes Reinecke <hare@...e.de>, linux-ide@...r.kernel.org,
 linux-kernel@...r.kernel.org, Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH v1 1/4] ata: libata: Remove redundant sense_buffer memsets

On 6/15/24 04:18, Igor Pylypiv wrote:
> scsi_queue_rq() memsets sense_buffer before a command is dispatched.
> 
> Libata is not memsetting sense_buffer before setting sense data that
> was obtained from a disk so there should be no reason to do a memset
> for ATA PASS-THROUGH / ATAPI.

This sentence is not very clear at all... I assume that the first part of the
sentence is for non passthrough commands. In this case, libata does not clear
the sense buffer because the scsi layer did that already, in scsi_queue_rq() as
noted above.

For passthrough commands, the same should be happening as well since passthrough
commands are also executed through blk_execute_rq() -> scsi_queue_rq(). So I do
not really understand (but I do agree that the memset() in libata seem useless).

> Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid
> sense data that was previously obtained from a disk. A follow-up patch
> will modify ata_gen_passthru_sense() to stop generating sense data based
> on ATA status register bits if a valid sense data is already present.

This fix should come first in the series, since that commit will likely need to
go into current rc and cc-stable. And that will simplify this patch as well.

> 
> Signed-off-by: Igor Pylypiv <ipylypiv@...gle.com>
> ---
>  drivers/ata/libata-eh.c   | 2 --
>  drivers/ata/libata-scsi.c | 4 ----
>  2 files changed, 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 214b935c2ced..b5e05efe73f6 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev,
>  	struct ata_port *ap = dev->link->ap;
>  	struct ata_taskfile tf;
>  
> -	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
> -
>  	/* initialize sense_buf with the error register,
>  	 * for the case where they are -not- overwritten
>  	 */
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index cdf29b178ddc..032cf11d0bcc 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>  	unsigned char *desc = sb + 8;
>  	u8 sense_key, asc, ascq;
>  
> -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> -
>  	/*
>  	 * Use ata_to_sense_error() to map status register bits
>  	 * onto sense key, asc & ascq.
> @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>  	u64 block;
>  	u8 sense_key, asc, ascq;
>  
> -	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> -
>  	if (ata_dev_disabled(dev)) {
>  		/* Device disabled after error recovery */
>  		/* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */

-- 
Damien Le Moal
Western Digital Research