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

[Damien Le Moal <dlemoal@kernel.org>]

On 2026/10/29 11:37, Zhao Gongyi wrote:
> 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.

Yeah, forgot about that. Ideally, we should get the ioctl(SG_IO) to return
-EINVAL for an invalid data direction but that indeed requires parsing the
commands, which we do *not* want to do.

> 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.

Yep, these are the only ones we can catch easily. Other commands we do not look
at/know about will keep potentially triggering iommu page faults, but that is
fine as that is not a crash. However, ideally, that should lead to the command
being failed, which it seems we are unable to do... Did you check if we can
propagate the fault into a the request to indicate a failure?

>> 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.

Not sure if that one is ideal either. Let me check and get back to you next week.


-- 
Damien Le Moal
Western Digital Research