mbox series

[0/2] ata,sd: Fix reading concurrent positioning ranges

Message ID 20220531175009.850-1-tyler.erickson@seagate.com
Headers show
Series ata,sd: Fix reading concurrent positioning ranges | expand

Message

Tyler Erickson May 31, 2022, 5:50 p.m. UTC 
This patch series fixes reading the concurrent positioning ranges.

The first patch fixes reading this in libata, where it was reading
more data than a drive necessarily supports, resulting in a 
command abort. There was also a change to the SCSI translated
data to put the VPD page length in the correct starting byte.

In sd, the fix is adding 4 instead of 3 for the header length.
Using 3 will always result in an error and was likely used incorrectly
as T10 specifications list all tables starting at byte 0, and byte 3 is
the page length, which would mean there are 4 total bytes before the
remaining data that contains the ranges and other information.

Tyler Erickson (2):
  libata: fix reading concurrent positioning ranges log
  sd: Fixing interpretation of VPD B9h length

 drivers/ata/libata-core.c | 21 +++++++++++++--------
 drivers/ata/libata-scsi.c |  2 +-
 drivers/scsi/sd.c         |  2 +-
 3 files changed, 15 insertions(+), 10 deletions(-)


base-commit: af2d861d4cd2a4da5137f795ee3509e6f944a25b

Comments

Damien Le Moal May 31, 2022, 9:28 p.m. UTC | #1 
On 6/1/22 02:50, Tyler Erickson wrote:
> From: Tyler Erickson <tyler.j.erickson@seagate.com>
> 
> The concurrent positioning ranges log is not a fixed size and may depend
> on how many ranges are supported by the device. This patch uses the size
> reported in the GPL directory to determine the number of pages supported
> by the device before attempting to read this log page.
> 
> Also fixing the page length in the SCSI translation for the concurrent
> positioning ranges VPD page.
> 
> This resolves this error from the dmesg output:
>     ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
> 
> Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com>
> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
> Tested-by: Michael English <michael.english@seagate.com>

This needs a Fixes tag and cc stable.

> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index ca64837641be..3d57fa84e2be 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2003,16 +2003,16 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  	return err_mask;
>  }
>  
> -static bool ata_log_supported(struct ata_device *dev, u8 log)
> +static int ata_log_supported(struct ata_device *dev, u8 log)
>  {
>  	struct ata_port *ap = dev->link->ap;
>  
>  	if (dev->horkage & ATA_HORKAGE_NO_LOG_DIR)
> -		return false;
> +		return 0;
>  
>  	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, ap->sector_buf, 1))
> -		return false;
> -	return get_unaligned_le16(&ap->sector_buf[log * 2]) ? true : false;
> +		return 0;
> +	return get_unaligned_le16(&ap->sector_buf[log * 2]);
>  }
>  
>  static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
> @@ -2448,15 +2448,20 @@ static void ata_dev_config_cpr(struct ata_device *dev)
>  	struct ata_cpr_log *cpr_log = NULL;
>  	u8 *desc, *buf = NULL;
>  
> -	if (ata_id_major_version(dev->id) < 11 ||
> -	    !ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES))
> +	if (ata_id_major_version(dev->id) < 11)
> +		goto out;> +
> +	buf_len = ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES);
> +	if (buf_len == 0)
>  		goto out;
>  
>  	/*
>  	 * Read the concurrent positioning ranges log (0x47). We can have at
> -	 * most 255 32B range descriptors plus a 64B header.
> +	 * most 255 32B range descriptors plus a 64B header. This log varies in
> +	 * size, so use the size reported in the GPL directory. Reading beyond
> +	 * the supported length will result in an error.
>  	 */
> -	buf_len = (64 + 255 * 32 + 511) & ~511;
> +	buf_len <<= 9;
>  	buf = kzalloc(buf_len, GFP_KERNEL);
>  	if (!buf)
>  		goto out;
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 06c9d90238d9..0ea9c3115529 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2101,7 +2101,7 @@ static unsigned int ata_scsiop_inq_b9(struct ata_scsi_args *args, u8 *rbuf)
>  
>  	/* SCSI Concurrent Positioning Ranges VPD page: SBC-5 rev 1 or later */
>  	rbuf[1] = 0xb9;
> -	put_unaligned_be16(64 + (int)cpr_log->nr_cpr * 32 - 4, &rbuf[3]);
> +	put_unaligned_be16(64 + (int)cpr_log->nr_cpr * 32 - 4, &rbuf[2]);
>  
>  	for (i = 0; i < cpr_log->nr_cpr; i++, desc += 32) {
>  		desc[0] = cpr_log->cpr[i].num;

This scsi hunk needs to be a different patch with a fixes tag and cc stable.
Sergey Shtylyov June 1, 2022, 10:29 a.m. UTC | #2 
Hello!

On 5/31/22 8:50 PM, Tyler Erickson wrote:

> From: Tyler Erickson <tyler.j.erickson@seagate.com>
> 
> The concurrent positioning ranges log is not a fixed size and may depend
> on how many ranges are supported by the device. This patch uses the size
> reported in the GPL directory to determine the number of pages supported
> by the device before attempting to read this log page.
> 
> Also fixing the page length in the SCSI translation for the concurrent
> positioning ranges VPD page.
> 
> This resolves this error from the dmesg output:
>     ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
> 
> Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com>
> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
> Tested-by: Michael English <michael.english@seagate.com>
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index ca64837641be..3d57fa84e2be 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2003,16 +2003,16 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  	return err_mask;
>  }
>  
> -static bool ata_log_supported(struct ata_device *dev, u8 log)
> +static int ata_log_supported(struct ata_device *dev, u8 log)

   Maybe *unsigned int*? The 'buf_len' variable below is 'size_t' which is *unsigned* type...

>  {
>  	struct ata_port *ap = dev->link->ap;
>  
>  	if (dev->horkage & ATA_HORKAGE_NO_LOG_DIR)
> -		return false;
> +		return 0;
>  
>  	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, ap->sector_buf, 1))
> -		return false;
> -	return get_unaligned_le16(&ap->sector_buf[log * 2]) ? true : false;
> +		return 0;
> +	return get_unaligned_le16(&ap->sector_buf[log * 2]);
>  }
>  
>  static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
> @@ -2448,15 +2448,20 @@ static void ata_dev_config_cpr(struct ata_device *dev)
>  	struct ata_cpr_log *cpr_log = NULL;
>  	u8 *desc, *buf = NULL;
>  
> -	if (ata_id_major_version(dev->id) < 11 ||
> -	    !ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES))
> +	if (ata_id_major_version(dev->id) < 11)
> +		goto out;
> +
> +	buf_len = ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES);
> +	if (buf_len == 0)
>  		goto out;
[...]

MBR, Sergey
Damien Le Moal June 2, 2022, 6:35 a.m. UTC | #3 
On 2022/06/01 19:29, Sergey Shtylyov wrote:
> Hello!
> 
> On 5/31/22 8:50 PM, Tyler Erickson wrote:
> 
>> From: Tyler Erickson <tyler.j.erickson@seagate.com>
>>
>> The concurrent positioning ranges log is not a fixed size and may depend
>> on how many ranges are supported by the device. This patch uses the size
>> reported in the GPL directory to determine the number of pages supported
>> by the device before attempting to read this log page.
>>
>> Also fixing the page length in the SCSI translation for the concurrent
>> positioning ranges VPD page.
>>
>> This resolves this error from the dmesg output:
>>     ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
>>
>> Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com>
>> Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
>> Tested-by: Michael English <michael.english@seagate.com>
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index ca64837641be..3d57fa84e2be 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2003,16 +2003,16 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>>  	return err_mask;
>>  }
>>  
>> -static bool ata_log_supported(struct ata_device *dev, u8 log)
>> +static int ata_log_supported(struct ata_device *dev, u8 log)
> 
>    Maybe *unsigned int*? The 'buf_len' variable below is 'size_t' which is *unsigned* type...

int is fine I think. The value is 16-bits so no overflow possible. And having a
signed value, we can change the code to return an error code if ever needed.

> 
>>  {
>>  	struct ata_port *ap = dev->link->ap;
>>  
>>  	if (dev->horkage & ATA_HORKAGE_NO_LOG_DIR)
>> -		return false;
>> +		return 0;
>>  
>>  	if (ata_read_log_page(dev, ATA_LOG_DIRECTORY, 0, ap->sector_buf, 1))
>> -		return false;
>> -	return get_unaligned_le16(&ap->sector_buf[log * 2]) ? true : false;
>> +		return 0;
>> +	return get_unaligned_le16(&ap->sector_buf[log * 2]);
>>  }
>>  
>>  static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
>> @@ -2448,15 +2448,20 @@ static void ata_dev_config_cpr(struct ata_device *dev)
>>  	struct ata_cpr_log *cpr_log = NULL;
>>  	u8 *desc, *buf = NULL;
>>  
>> -	if (ata_id_major_version(dev->id) < 11 ||
>> -	    !ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES))
>> +	if (ata_id_major_version(dev->id) < 11)
>> +		goto out;
>> +
>> +	buf_len = ata_log_supported(dev, ATA_LOG_CONCURRENT_POSITIONING_RANGES);
>> +	if (buf_len == 0)
>>  		goto out;
> [...]
> 
> MBR, Sergey