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

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

[Tyler Erickson]

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

[PATCH 1/2] [PATCH v1 1/2] libata: fix reading concurrent positioning ranges log

[Tyler Erickson]

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)
 {
 	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;
-- 
2.25.1

[PATCH 2/2] [PATCH v1 2/2] sd: Fixing interpretation of VPD B9h length

[Tyler Erickson]

Fixing the interpretation of the length of the B9h VPD page
(concurrent positioning ranges). Adding 4 is necessary as
the first 4 bytes of the page is the header with page number
and length information. Adding 3 was likely a misinterpretation
of the SBC-5 specification which sets all offsets starting at zero.

This fixes the error in dmesg:
[ 9.014456] sd 1:0:0:0: [sda] Invalid Concurrent Positioning Ranges VPD page

Signed-off-by: Tyler Erickson <tyler.erickson@seagate.com>
Reviewed-by: Muhammad Ahmad <muhammad.ahmad@seagate.com>
Tested-by: Michael English <michael.english@seagate.com>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index dc6e55761fd1..14867e8cd687 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3067,7 +3067,7 @@ static void sd_read_cpr(struct scsi_disk *sdkp)
 		goto out;
 
 	/* We must have at least a 64B header and one 32B range descriptor */
-	vpd_len = get_unaligned_be16(&buffer[2]) + 3;
+	vpd_len = get_unaligned_be16(&buffer[2]) + 4;
 	if (vpd_len > buf_len || vpd_len < 64 + 32 || (vpd_len & 31)) {
 		sd_printk(KERN_ERR, sdkp,
 			  "Invalid Concurrent Positioning Ranges VPD page\n");
-- 
2.25.1

Re: [PATCH 1/2] [PATCH v1 1/2] libata: fix reading concurrent positioning ranges log

[Damien Le Moal]

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.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] [PATCH v1 2/2] sd: Fixing interpretation of VPD B9h length

[Damien Le Moal]

On 6/1/22 02:50, Tyler Erickson wrote:
> Fixing the interpretation of the length of the B9h VPD page
> (concurrent positioning ranges). Adding 4 is necessary as
> the first 4 bytes of the page is the header with page number
> and length information. Adding 3 was likely a misinterpretation
> of the SBC-5 specification which sets all offsets starting at zero.
> 
> This fixes the error in dmesg:
> [ 9.014456] sd 1:0:0:0: [sda] Invalid Concurrent Positioning Ranges VPD page
> 
> Signed-off-by: Tyler Erickson <tyler.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. Your patch format is also starnge.
It is missing the "---" separator after the tags. This is not going to
apply. Did you generate this with git format-patch ?

> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index dc6e55761fd1..14867e8cd687 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3067,7 +3067,7 @@ static void sd_read_cpr(struct scsi_disk *sdkp)
>  		goto out;
>  
>  	/* We must have at least a 64B header and one 32B range descriptor */
> -	vpd_len = get_unaligned_be16(&buffer[2]) + 3;
> +	vpd_len = get_unaligned_be16(&buffer[2]) + 4;
>  	if (vpd_len > buf_len || vpd_len < 64 + 32 || (vpd_len & 31)) {
>  		sd_printk(KERN_ERR, sdkp,
>  			  "Invalid Concurrent Positioning Ranges VPD page\n");


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH 2/2] [PATCH v1 2/2] sd: Fixing interpretation of VPD B9h length

[Tyler Erickson]

Thanks for the feedback. 
I did use git format-patch. I will double check my work and see if I can figure out what caused the formatting issues and resubmit with the changes you have mentioned.

Tyler Erickson
Seagate Technology


Seagate Internal

-----Original Message-----
From: Damien Le Moal <damien.lemoal@opensource.wdc.com> 
Sent: Tuesday, May 31, 2022 3:30 PM
To: Tyler Erickson <tyler.erickson@seagate.com>; jejb@linux.ibm.com; martin.petersen@oracle.com
Cc: linux-scsi@vger.kernel.org; linux-ide@vger.kernel.org; Muhammad Ahmad <muhammad.ahmad@seagate.com>; Michael English <michael.english@seagate.com>
Subject: Re: [PATCH 2/2] [PATCH v1 2/2] sd: Fixing interpretation of VPD B9h length


This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.


On 6/1/22 02:50, Tyler Erickson wrote:
> Fixing the interpretation of the length of the B9h VPD page 
> (concurrent positioning ranges). Adding 4 is necessary as the first 4 
> bytes of the page is the header with page number and length 
> information. Adding 3 was likely a misinterpretation of the SBC-5 
> specification which sets all offsets starting at zero.
>
> This fixes the error in dmesg:
> [ 9.014456] sd 1:0:0:0: [sda] Invalid Concurrent Positioning Ranges 
> VPD page
>
> Signed-off-by: Tyler Erickson <tyler.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. Your patch format is also starnge.
It is missing the "---" separator after the tags. This is not going to apply. Did you generate this with git format-patch ?

>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 
> dc6e55761fd1..14867e8cd687 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3067,7 +3067,7 @@ static void sd_read_cpr(struct scsi_disk *sdkp)
>               goto out;
>
>       /* We must have at least a 64B header and one 32B range descriptor */
> -     vpd_len = get_unaligned_be16(&buffer[2]) + 3;
> +     vpd_len = get_unaligned_be16(&buffer[2]) + 4;
>       if (vpd_len > buf_len || vpd_len < 64 + 32 || (vpd_len & 31)) {
>               sd_printk(KERN_ERR, sdkp,
>                         "Invalid Concurrent Positioning Ranges VPD 
> page\n");


--
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] [PATCH v1 1/2] libata: fix reading concurrent positioning ranges log

[Sergey Shtylyov]

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

Re: [PATCH 1/2] [PATCH v1 1/2] libata: fix reading concurrent positioning ranges log

[Damien Le Moal]

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


-- 
Damien Le Moal
Western Digital Research