On Mon, Jun 17, 2024 at 12:41:26PM +0200, Niklas Cassel wrote:
> On Fri, Jun 14, 2024 at 07:18:32PM +0000, 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.
> >
> > 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.
> >
> > Signed-off-by: Igor Pylypiv <ipylypiv@xxxxxxxxxx>
> > ---
> > 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);
> > -
>
> Are you sure that this is safe?
>
> atapi_eh_request_sense() is called both by:
> ata_eh_analyze_tf():
> tmp = atapi_eh_request_sense(.., qc->scsicmd->sense_buffer, ..)
>
> and by:
> atapi_eh_clear_ua():
> atapi_eh_request_sense(.., sense_buffer, ..);
> where sense_buffer is dev->link->ap->sector_buf.
>
Thanks for pointing this out, Niklas!
ata_eh_analyze_tf() case is safe because qc->scsicmd->sense_buffer is cleared
by scsi_queue_rq().
atapi_eh_clear_ua() case is safe right now because the sense buffer contents
are not being used. However, someone might start using the sense data in
the future so it is not safe to leave it as-is.
There's one more place where this function is being called:
zpready():
atapi_eh_request_sense(..., sense_buf, ...);
where sense_buffer is dev->link->ap->sector_buf.
This one is actually using the obtained sense buffer so it would be
a nasty bug if we don't do a memset().
I think we should explicitly memset buffers before passing them to
atapi_eh_request_sense() in atapi_eh_clear_ua() and zpready() so that
atapi_eh_request_sense() can have the same behavior as ata_eh_request_sense()
with regards to sense buffer expectations i.e. both functions will expect
buffers that are already memeset to zero.
>
> Wouldn't a better fix be for ata_gen_* functions to return early if
> ATA_QCFLAG_SENSE_VALID is set?
>
It would be possible to return early if ATA_QCFLAG_SENSE_VALID is set once
we factor out "ATA Status Return sense data descriptor" population out of
ata_gen_passthru_sense() into a separate function. I'll factor out the
descriptor population code in v2.
I think that it is still benefitial to remove the redundant memset() from
the ata_eh_analyze_tf() -> atapi_eh_request_sense() path?
>
> > /* 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 */
> > --
> > 2.45.2.627.g7a2c4fd464-goog
> >
Thank you,
Igor
