Re: [PATCH v2 2/5] ata: libata-scsi: Fail MODE SELECT for unsupported mode pages

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

On 4/18/25 17:40, Niklas Cassel wrote:
> On Fri, Apr 18, 2025 at 04:55:14PM +0900, Damien Le Moal wrote:
>> For devices that do not support CDL, the subpage F2h of the control mode
>> page 0Ah should not be supported. However, the function
>> ata_mselect_control_ata_feature() does not fail for a device that does
>> not have the ATA_DFLAG_CDL device flag set, which can lead to an invalid
>> SET FEATURES command (which will be failed by the device) to be issued.
>>
>> Modify ata_mselect_control_ata_feature() to return -EOPNOTSUPP if it is
>> executed for a device without CDL support. This error code is checked by
>> ata_scsi_mode_select_xlat() (through ata_mselect_control()) to fail the
>> MODE SELECT command immediately with an ILLEGAL REQUEST / INVALID FIELD
>> IN CDB asc/ascq as mandated by the SPC specifications for unsupported
>> mode pages.
>>
>> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>  drivers/ata/libata-scsi.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 24e662c837e3..15661b05cb48 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -3896,6 +3896,15 @@ static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc,
>>  	struct ata_taskfile *tf = &qc->tf;
>>  	u8 cdl_action;
>>  
>> +	/*
>> +	 * The sub-page f2h should only be supported for devices that support
>> +	 * the T2A and T2B command duration limits mode pages (note here the
>> +	 * "should" which is what SAT-6 defines). So fail this command if the
>> +	 * device does not support CDL.
>> +	 */
>> +	if (!(dev->flags & ATA_DFLAG_CDL))
>> +		return -EOPNOTSUPP;
>> +
>>  	/*
>>  	 * The first four bytes of ATA Feature Control mode page are a header,
>>  	 * so offsets in mpage are off by 4 compared to buf.  Same for len.
>> @@ -4101,6 +4110,8 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
>>  	case CONTROL_MPAGE:
>>  		ret = ata_mselect_control(qc, spg, p, pg_len, &fp);
>>  		if (ret < 0) {
>> +			if (ret == -EOPNOTSUPP)
>> +				goto invalid_fld;
>>  			fp += hdr_len + bd_len;
>>  			goto invalid_param;
>>  		}
>> -- 
> 
> I would prefer if we did not merge this patch, as it is already handled in
> higher up in the (only) calling function:
> https://github.com/torvalds/linux/blob/v6.15-rc2/drivers/ata/libata-scsi.c#L2582-L2589

This code you point to is for mode sense. This patch deals with mode select,
where we are not checking for the subpage support, which is wrong.

> 
> We only break if "dev->flags & ATA_DFLAG_CDL && pg == CONTROL_MPAGE"
> 
> if this expression is false, we do a fallthrough,
> which means fp = 3; goto invalid_fld;
> 
> 
> Kind regards,
> Niklas


-- 
Damien Le Moal
Western Digital Research