On 4/18/25 20:45, Niklas Cassel wrote:
>
>
> On 18 April 2025 11:30:35 CEST, Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
>> 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@xxxxxxxxxxxxxxx
>>>> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
>>>> ---
>>>> 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.
>>
>
> I linked to the wrong line.
>
> https://github.com/torvalds/linux/blob/v6.15-rc2/drivers/ata/libata-scsi.c#L4081
>
> The rest of the comment is still valid.
>
> This case that this patch tries to fix can already not happen.
You are absolutely correct ! How did I miss that :)
Sending V3 with this patch dropped.
--
Damien Le Moal
Western Digital Research
