Re: [PATCH v2] ata: libata-scsi: reject data-in commands with a data-out buffer

["Zhao Gongyi" <zhaogongyi@bytedance.com>]

Hi Damien,

Thanks for the review.

On 10/28/26 9:59 PM, Damien Le Moal wrote:
> On 2026/10/28 18:39, Zhao Gongyi wrote:
> 
> No they cannot, as long as the kernel is correct for internally issued commands.
> But t is not clear here if you are still talking about commands passed through
> SG_IO.
> 

Yes, by "translated commands" I meant commands submitted through SG_IO
and subsequently translated by libata, not commands issued internally
by the kernel. I will make that explicit and narrow the description to
the affected ZBC IN and GET PHYSICAL ELEMENT STATUS paths.

> 
> Why is this being checked here and not in the SCSI layer (sg driver and bsg)? It
> seems sensible to check this there.
> 

I checked the SG and BSG submission paths. For data-direction purposes,
they derive the request direction from the SG_IO descriptor and otherwise
treat the CDB as opaque. I could not find a generic helper that derives
the transfer direction of arbitrary standard, service-action, ATA
pass-through, and vendor-specific CDBs.

For simulated commands, libata itself performs the CPU-side write. For
the ZBC IN and GET PHYSICAL ELEMENT STATUS translation paths, the actual
ATA data direction is known after xlat_func() has built qc->tf, and their
completion handlers may modify the receive buffer.

This is why I currently think that checking these paths in libata is the
smallest fix suitable for stable. A broader SG/BSG direction-validation
policy may also be useful, but would likely be a separate change.

> 
> Test details are not needed in the commit message.
> 

Agreed. I will remove the detailed validation results from the commit
message and keep them below the --- separator.

> 
> The CDB may be perfectly correct. The error is in the SG_IO descriptor setup,
> NOT in the CDB. So returning invalid field in CDB seems very misleading.
> This is not something that SAT defines as far as I know, so we need to find a
> sense code that is more informative. Please have a loo at
> include/scsi/scsi_sense.h to see if something better exist.
> 

Agreed. INVALID FIELD IN CDB is misleading here. I propose returning
ABORTED COMMAND / DATA PHASE ERROR (4b/00) instead.

Would this approach be acceptable? If so, I will send a v3 with the
clarified description, the shorter commit message, and the updated sense
data.

Thanks,
Gongyi