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

[PATCH v2 0/3] 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. 

The second patch fixes the SCSI translated data to put the VPD page
length in the correct starting byte.

The third patch 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 (3):
  libata: fix reading concurrent positioning ranges log
  libata: fix translation of concurrent positioning ranges
  scsi: sd: Fix 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: 700170bf6b4d773e328fa54ebb70ba444007c702
-- 
2.17.1

[PATCH v2 1/3] libata: fix reading concurrent positioning ranges log

[Tyler Erickson]

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.

This resolves this error from the dmesg output:
    ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1

Cc: stable@vger.kernel.org
Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
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>
---
 drivers/ata/libata-core.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 40e816419f48..3ea10f72cb70 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2010,16 +2010,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)
@@ -2455,15 +2455,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;
-- 
2.17.1

[PATCH v2 2/3] libata: fix translation of concurrent positioning ranges

[Tyler Erickson]

Fixing the page length in the SCSI translation for the concurrent
positioning ranges VPD page. It was writing starting in offset 3
rather than offset 2 where the MSB is supposed to start for
the VPD page length.

Cc: stable@vger.kernel.org
Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
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>
---
 drivers/ata/libata-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 42cecf95a4e5..86dbb1cdfabd 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2125,7 +2125,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.17.1

[PATCH v2 3/3] scsi: sd: Fix 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

Cc: stable@vger.kernel.org
Fixes: e815d36548f0 ("scsi: sd: add concurrent positioning ranges support")
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>
---
 drivers/scsi/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 749316462075..f25b0cc5dd21 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3072,7 +3072,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.17.1

Re: [PATCH v2 0/3] ata,sd: Fix reading concurrent positioning ranges

[Damien Le Moal]

On 6/3/22 07:51, Tyler Erickson wrote:
> 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. 
> 
> The second patch fixes the SCSI translated data to put the VPD page
> length in the correct starting byte.
> 
> The third patch 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 (3):
>   libata: fix reading concurrent positioning ranges log
>   libata: fix translation of concurrent positioning ranges
>   scsi: sd: Fix 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: 700170bf6b4d773e328fa54ebb70ba444007c702

Looks all good to me. I tested this and really wonder how I did not catch
these mistakes earlier :)

Using a tcmu emulator for various concurrent positioning range configs to
test, I got a lockdep splat when unplugging the drive:

[  760.702859] ======================================================
[  760.702863] WARNING: possible circular locking dependency detected
[  760.702868] 5.18.0+ #1509 Not tainted
[  760.702875] ------------------------------------------------------
[...]
[  760.702966] the existing dependency chain (in reverse order) is:
[  760.702969]
[  760.702969] -> #1 (&q->sysfs_lock){+.+.}-{3:3}:
[  760.702982]        __mutex_lock+0x15b/0x1480
[  760.702998]        blk_ia_range_sysfs_show+0x41/0xc0
[  760.703010]        sysfs_kf_seq_show+0x1f2/0x360
[  760.703022]        seq_read_iter+0x40f/0x1130
[  760.703036]        new_sync_read+0x2d8/0x520
[  760.703049]        vfs_read+0x31a/0x450
[  760.703060]        ksys_read+0xf7/0x1d0
[  760.703070]        do_syscall_64+0x34/0x80
[  760.703081]        entry_SYSCALL_64_after_hwframe+0x46/0xb0
[  760.703093]
[  760.703093] -> #0 (kn->active#385){++++}-{0:0}:
[  760.703108]        __lock_acquire+0x2ba6/0x6a20
[  760.703125]        lock_acquire+0x19f/0x510
[  760.703136]        __kernfs_remove+0x739/0x940
[  760.703145]        kernfs_remove_by_name_ns+0x90/0xe0
[  760.703154]        remove_files+0x8c/0x1a0
[  760.703165]        sysfs_remove_group+0x77/0x150
[  760.703175]        sysfs_remove_groups+0x4f/0x90
[  760.703186]        __kobject_del+0x7d/0x1b0
[  760.703196]        kobject_del+0x31/0x50
[  760.703203]        disk_unregister_independent_access_ranges+0x153/0x290
[  760.703214]        blk_unregister_queue+0x166/0x210
[  760.703226]        del_gendisk+0x2f8/0x7c0
[  760.703233]        sd_remove+0x5e/0xb0 [sd_mod]
[  760.703252]        device_release_driver_internal+0x3ad/0x750
[  760.703262]        bus_remove_device+0x2a6/0x570
[  760.703269]        device_del+0x48f/0xb50
[  760.703280]        __scsi_remove_device+0x21b/0x2b0 [scsi_mod]
[  760.703339]        scsi_remove_device+0x3a/0x50 [scsi_mod]
[  760.703391]        tcm_loop_port_unlink+0xca/0x160 [tcm_loop]
[  760.703407]        target_fabric_port_unlink+0xd5/0x120 [target_core_mod]
[  760.703494]        configfs_unlink+0x37f/0x7a0
[  760.703502]        vfs_unlink+0x295/0x800
[  760.703514]        do_unlinkat+0x2d9/0x560
[  760.703520]        __x64_sys_unlink+0xa5/0xf0
[  760.703528]        do_syscall_64+0x34/0x80
[  760.703537]        entry_SYSCALL_64_after_hwframe+0x46/0xb0
[  760.703548]
[  760.703548] other info that might help us debug this:
[  760.703548]
[  760.703551]  Possible unsafe locking scenario:
[  760.703551]
[  760.703554]        CPU0                    CPU1
[  760.703556]        ----                    ----
[  760.703558]   lock(&q->sysfs_lock);
[  760.703565]                                lock(kn->active#385);
[  760.703573]                                lock(&q->sysfs_lock);
[  760.703579]   lock(kn->active#385);
[  760.703587]
[  760.703587]  *** DEADLOCK ***

This needs to be checked too, but that is not related to your fixes.

I will queue the libata patches for rc1 update.

Martin,

Do you want to take patch 3 or should I just take it ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/3] scsi: sd: Fix interpretation of VPD B9h length

[Damien Le Moal]

On 6/3/22 07:51, 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
> 
> Cc: stable@vger.kernel.org
> Fixes: e815d36548f0 ("scsi: sd: add concurrent positioning ranges support")
> 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>
> ---
>  drivers/scsi/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 749316462075..f25b0cc5dd21 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3072,7 +3072,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");

Martin,

If you take this one:

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/3] ata,sd: Fix reading concurrent positioning ranges

[Christoph Hellwig]

On Fri, Jun 03, 2022 at 10:30:04AM +0900, Damien Le Moal wrote:
> Looks all good to me. I tested this and really wonder how I did not catch
> these mistakes earlier :)
> 
> Using a tcmu emulator for various concurrent positioning range configs to
> test, I got a lockdep splat when unplugging the drive:

You probably want something like this:

---
From 4340b85be3532149310326b5f0caf329e1f4c748 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Mon, 23 May 2022 09:18:44 +0200
Subject: block: don't take sysfs_lock in blk_ia_range_sysfs_show

sysfs already synchronizes internally against kobject removal, so remove
the extra lock.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-ia-ranges.c  | 8 +-------
 include/linux/blkdev.h | 1 -
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/block/blk-ia-ranges.c b/block/blk-ia-ranges.c
index 853be76b9808b..e9e7ebf02737f 100644
--- a/block/blk-ia-ranges.c
+++ b/block/blk-ia-ranges.c
@@ -54,13 +54,8 @@ static ssize_t blk_ia_range_sysfs_show(struct kobject *kobj,
 		container_of(attr, struct blk_ia_range_sysfs_entry, attr);
 	struct blk_independent_access_range *iar =
 		container_of(kobj, struct blk_independent_access_range, kobj);
-	ssize_t ret;
 
-	mutex_lock(&iar->queue->sysfs_lock);
-	ret = entry->show(iar, buf);
-	mutex_unlock(&iar->queue->sysfs_lock);
-
-	return ret;
+	return entry->show(iar, buf);
 }
 
 static const struct sysfs_ops blk_ia_range_sysfs_ops = {
@@ -149,7 +144,6 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
 	}
 
 	for (i = 0; i < iars->nr_ia_ranges; i++) {
-		iars->ia_range[i].queue = q;
 		ret = kobject_init_and_add(&iars->ia_range[i].kobj,
 					   &blk_ia_range_ktype, &iars->kobj,
 					   "%d", i);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index a72203ed25454..0ceb85ca52af4 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -348,7 +348,6 @@ static inline int blkdev_zone_mgmt_ioctl(struct block_device *bdev,
  */
 struct blk_independent_access_range {
 	struct kobject		kobj;
-	struct request_queue	*queue;
 	sector_t		sector;
 	sector_t		nr_sectors;
 };
-- 
2.30.2

Re: [PATCH v2 0/3] ata,sd: Fix reading concurrent positioning ranges

[Damien Le Moal]

On 6/3/22 14:23, Christoph Hellwig wrote:
> On Fri, Jun 03, 2022 at 10:30:04AM +0900, Damien Le Moal wrote:
>> Looks all good to me. I tested this and really wonder how I did not catch
>> these mistakes earlier :)
>>
>> Using a tcmu emulator for various concurrent positioning range configs to
>> test, I got a lockdep splat when unplugging the drive:
> 
> You probably want something like this:
> 
> ---
> From 4340b85be3532149310326b5f0caf329e1f4c748 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Mon, 23 May 2022 09:18:44 +0200
> Subject: block: don't take sysfs_lock in blk_ia_range_sysfs_show
> 
> sysfs already synchronizes internally against kobject removal, so remove
> the extra lock.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-ia-ranges.c  | 8 +-------
>  include/linux/blkdev.h | 1 -
>  2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/block/blk-ia-ranges.c b/block/blk-ia-ranges.c
> index 853be76b9808b..e9e7ebf02737f 100644
> --- a/block/blk-ia-ranges.c
> +++ b/block/blk-ia-ranges.c
> @@ -54,13 +54,8 @@ static ssize_t blk_ia_range_sysfs_show(struct kobject *kobj,
>  		container_of(attr, struct blk_ia_range_sysfs_entry, attr);
>  	struct blk_independent_access_range *iar =
>  		container_of(kobj, struct blk_independent_access_range, kobj);
> -	ssize_t ret;
>  
> -	mutex_lock(&iar->queue->sysfs_lock);
> -	ret = entry->show(iar, buf);
> -	mutex_unlock(&iar->queue->sysfs_lock);
> -
> -	return ret;
> +	return entry->show(iar, buf);

I sent a patch that Jens applied doing exactly that. But forgot to add the
removal of the queue pointer you have below. Sending an incremental patch
for that.

>  }
>  
>  static const struct sysfs_ops blk_ia_range_sysfs_ops = {
> @@ -149,7 +144,6 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
>  	}
>  
>  	for (i = 0; i < iars->nr_ia_ranges; i++) {
> -		iars->ia_range[i].queue = q;
>  		ret = kobject_init_and_add(&iars->ia_range[i].kobj,
>  					   &blk_ia_range_ktype, &iars->kobj,
>  					   "%d", i);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index a72203ed25454..0ceb85ca52af4 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -348,7 +348,6 @@ static inline int blkdev_zone_mgmt_ioctl(struct block_device *bdev,
>   */
>  struct blk_independent_access_range {
>  	struct kobject		kobj;
> -	struct request_queue	*queue;
>  	sector_t		sector;
>  	sector_t		nr_sectors;
>  };


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/3] libata: fix reading concurrent positioning ranges log

[Hannes Reinecke]

On 6/3/22 00:51, Tyler Erickson wrote:
> 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.
> 
> This resolves this error from the dmesg output:
>      ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
> 
> Cc: stable@vger.kernel.org
> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
> 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>
> ---
>   drivers/ata/libata-core.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 40e816419f48..3ea10f72cb70 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2010,16 +2010,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]);
>   }
>   
Maybe we should change to name of the function here; 
'ata_log_supported()' suggests a bool return.

ata_check_log_page() ?

>   static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
> @@ -2455,15 +2455,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;

I don't get it.
You just returned the actual length of the log page from the previous 
function. Why do you need to calculate the length here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH v2 2/3] libata: fix translation of concurrent positioning ranges

[Hannes Reinecke]

On 6/3/22 00:51, Tyler Erickson wrote:
> Fixing the page length in the SCSI translation for the concurrent
> positioning ranges VPD page. It was writing starting in offset 3
> rather than offset 2 where the MSB is supposed to start for
> the VPD page length.
> 
> Cc: stable@vger.kernel.org
> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
> 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>
> ---
>   drivers/ata/libata-scsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 42cecf95a4e5..86dbb1cdfabd 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2125,7 +2125,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;
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH v2 3/3] scsi: sd: Fix interpretation of VPD B9h length

[Hannes Reinecke]

On 6/3/22 00:51, 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
> 
> Cc: stable@vger.kernel.org
> Fixes: e815d36548f0 ("scsi: sd: add concurrent positioning ranges support")
> 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>
> ---
>   drivers/scsi/sd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 749316462075..f25b0cc5dd21 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3072,7 +3072,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");

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH v2 1/3] libata: fix reading concurrent positioning ranges log

[Damien Le Moal]

On 6/3/22 15:17, Hannes Reinecke wrote:
> On 6/3/22 00:51, Tyler Erickson wrote:
>> 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.
>>
>> This resolves this error from the dmesg output:
>>      ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
>>
>> Cc: stable@vger.kernel.org
>> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
>> 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>
>> ---
>>   drivers/ata/libata-core.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 40e816419f48..3ea10f72cb70 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2010,16 +2010,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]);
>>   }
>>   
> Maybe we should change to name of the function here; 
> 'ata_log_supported()' suggests a bool return.
> 
> ata_check_log_page() ?
> 
>>   static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
>> @@ -2455,15 +2455,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;
> 
> I don't get it.
> You just returned the actual length of the log page from the previous 
> function. Why do you need to calculate the length here?

Calculate ? This is only converting from 512B sectors to bytes.
The calculation was mine, a gross error :) This is what this patch is fixing.

> 
> Cheers,
> 
> Hannes


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/3] libata: fix reading concurrent positioning ranges log

[Hannes Reinecke]

On 6/3/22 09:07, Damien Le Moal wrote:
> On 6/3/22 15:17, Hannes Reinecke wrote:
>> On 6/3/22 00:51, Tyler Erickson wrote:
>>> 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.
>>>
>>> This resolves this error from the dmesg output:
>>>       ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
>>> 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>
>>> ---
>>>    drivers/ata/libata-core.c | 21 +++++++++++++--------
>>>    1 file changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index 40e816419f48..3ea10f72cb70 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -2010,16 +2010,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]);
>>>    }
>>>    
>> Maybe we should change to name of the function here;
>> 'ata_log_supported()' suggests a bool return.
>>
>> ata_check_log_page() ?
>>
>>>    static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
>>> @@ -2455,15 +2455,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;
>>
>> I don't get it.
>> You just returned the actual length of the log page from the previous
>> function. Why do you need to calculate the length here?
> 
> Calculate ? This is only converting from 512B sectors to bytes.
> The calculation was mine, a gross error :) This is what this patch is fixing.
> 
Sigh. Can't we have a 'bytes_to_sectors' helper? All this shifting by 9 
is getting on my nerves ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH v2 1/3] libata: fix reading concurrent positioning ranges log

[Damien Le Moal]

On 6/3/22 16:42, Hannes Reinecke wrote:
> On 6/3/22 09:07, Damien Le Moal wrote:
>> On 6/3/22 15:17, Hannes Reinecke wrote:
>>> On 6/3/22 00:51, Tyler Erickson wrote:
>>>> 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.
>>>>
>>>> This resolves this error from the dmesg output:
>>>>       ata6.00: Read log 0x47 page 0x00 failed, Emask 0x1
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
>>>> 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>
>>>> ---
>>>>    drivers/ata/libata-core.c | 21 +++++++++++++--------
>>>>    1 file changed, 13 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>>> index 40e816419f48..3ea10f72cb70 100644
>>>> --- a/drivers/ata/libata-core.c
>>>> +++ b/drivers/ata/libata-core.c
>>>> @@ -2010,16 +2010,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]);
>>>>    }
>>>>    
>>> Maybe we should change to name of the function here;
>>> 'ata_log_supported()' suggests a bool return.
>>>
>>> ata_check_log_page() ?
>>>
>>>>    static bool ata_identify_page_supported(struct ata_device *dev, u8 page)
>>>> @@ -2455,15 +2455,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;
>>>
>>> I don't get it.
>>> You just returned the actual length of the log page from the previous
>>> function. Why do you need to calculate the length here?
>>
>> Calculate ? This is only converting from 512B sectors to bytes.
>> The calculation was mine, a gross error :) This is what this patch is fixing.
>>
> Sigh. Can't we have a 'bytes_to_sectors' helper? All this shifting by 9 
> is getting on my nerves ...

Haha ! Yes, we can do that. But not in this patch since that is a bug fix.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/3] scsi: sd: Fix interpretation of VPD B9h length

[Damien Le Moal]

On 2022/06/03 10:30, Damien Le Moal wrote:
> On 6/3/22 07:51, 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
>>
>> Cc: stable@vger.kernel.org
>> Fixes: e815d36548f0 ("scsi: sd: add concurrent positioning ranges support")
>> 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>
>> ---
>>  drivers/scsi/sd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 749316462075..f25b0cc5dd21 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -3072,7 +3072,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");
> 
> Martin,
> 
> If you take this one:
> 
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 

Martin,

Ping ?
How do you want to handle this one ? I can take it if you want (need your
acked-by) or you can take it through the scsi tree.

Thanks !

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/3] ata,sd: Fix reading concurrent positioning ranges

[Martin K. Petersen]

On Thu, 2 Jun 2022 16:51:10 -0600, Tyler Erickson wrote:

> 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.
> 
> The second patch fixes the SCSI translated data to put the VPD page
> length in the correct starting byte.
> 
> [...]

Applied to 5.19/scsi-fixes, thanks!

[3/3] scsi: sd: Fix interpretation of VPD B9h length
      https://git.kernel.org/mkp/scsi/c/f92de9d11042

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 0/3] ata,sd: Fix reading concurrent positioning ranges

[Damien Le Moal]

On 6/3/22 07:51, Tyler Erickson wrote:
> 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. 
> 
> The second patch fixes the SCSI translated data to put the VPD page
> length in the correct starting byte.
> 
> The third patch 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 (3):
>   libata: fix reading concurrent positioning ranges log
>   libata: fix translation of concurrent positioning ranges
>   scsi: sd: Fix interpretation of VPD B9h length

Applied 1-2 to for-5.19-fixes. Thanks !

> 
>  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: 700170bf6b4d773e328fa54ebb70ba444007c702


-- 
Damien Le Moal
Western Digital Research