[PATCH 0/2] virtio_scsi pi_bytes{out,in} miscalculated on 4 KiB devices

[PATCH 0/2] virtio_scsi pi_bytes{out,in} miscalculated on 4 KiB devices

[Greg Edwards]

When the VIRTIO_SCSI_F_T10_PI feature bit is enabled, the virtio_scsi driver
does not correctly calculate pi_bytes{out,in} when the underlying device has a
4 KiB logical block size.  The current code assumes a 512 byte logical block
size and protection interval exponent of 0 (512 bytes + 8 bytes PI).

The first patch moves bio_integrity_intervals() and bio_integrity_bytes() into
blkdev.h so drivers can make use of them.  The second patch modifies
virtio_scsi to call bio_integrity_bytes() to get the values for
pi_bytes{out,in}.

Greg Edwards (2):
  block: move bio_integrity_{intervals,bytes} into blkdev.h
  scsi: virtio_scsi: fix pi_bytes{out,in} on 4 KiB block size devices

 block/bio-integrity.c      | 22 ----------------------
 drivers/scsi/virtio_scsi.c |  8 ++++----
 include/linux/blkdev.h     | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 26 deletions(-)

-- 
2.17.1

[PATCH 1/2] block: move bio_integrity_{intervals,bytes} into blkdev.h

[Greg Edwards]

This allows bio_integrity_bytes() to be called from drivers instead of
open coding it.

Signed-off-by: Greg Edwards <gedwards@ddn.com>
---
 block/bio-integrity.c  | 22 ----------------------
 include/linux/blkdev.h | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index add7c7c85335..67b5fb861a51 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -159,28 +159,6 @@ int bio_integrity_add_page(struct bio *bio, struct page *page,
 }
 EXPORT_SYMBOL(bio_integrity_add_page);
 
-/**
- * bio_integrity_intervals - Return number of integrity intervals for a bio
- * @bi:		blk_integrity profile for device
- * @sectors:	Size of the bio in 512-byte sectors
- *
- * Description: The block layer calculates everything in 512 byte
- * sectors but integrity metadata is done in terms of the data integrity
- * interval size of the storage device.  Convert the block layer sectors
- * to the appropriate number of integrity intervals.
- */
-static inline unsigned int bio_integrity_intervals(struct blk_integrity *bi,
-						   unsigned int sectors)
-{
-	return sectors >> (bi->interval_exp - 9);
-}
-
-static inline unsigned int bio_integrity_bytes(struct blk_integrity *bi,
-					       unsigned int sectors)
-{
-	return bio_integrity_intervals(bi, sectors) * bi->tuple_size;
-}
-
 /**
  * bio_integrity_process - Process integrity metadata for a bio
  * @bio:	bio to generate/verify integrity metadata for
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 79226ca8f80f..553cc97900f6 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1877,6 +1877,28 @@ static inline bool integrity_req_gap_front_merge(struct request *req,
 				bip_next->bip_vec[0].bv_offset);
 }
 
+/**
+ * bio_integrity_intervals - Return number of integrity intervals for a bio
+ * @bi:		blk_integrity profile for device
+ * @sectors:	Size of the bio in 512-byte sectors
+ *
+ * Description: The block layer calculates everything in 512 byte
+ * sectors but integrity metadata is done in terms of the data integrity
+ * interval size of the storage device.  Convert the block layer sectors
+ * to the appropriate number of integrity intervals.
+ */
+static inline unsigned int bio_integrity_intervals(struct blk_integrity *bi,
+						   unsigned int sectors)
+{
+	return sectors >> (bi->interval_exp - 9);
+}
+
+static inline unsigned int bio_integrity_bytes(struct blk_integrity *bi,
+					       unsigned int sectors)
+{
+	return bio_integrity_intervals(bi, sectors) * bi->tuple_size;
+}
+
 #else /* CONFIG_BLK_DEV_INTEGRITY */
 
 struct bio;
@@ -1950,6 +1972,18 @@ static inline bool integrity_req_gap_front_merge(struct request *req,
 	return false;
 }
 
+static inline unsigned int bio_integrity_intervals(struct blk_integrity *bi,
+						   unsigned int sectors)
+{
+	return 0;
+}
+
+static inline unsigned int bio_integrity_bytes(struct blk_integrity *bi,
+					       unsigned int sectors)
+{
+	return 0;
+}
+
 #endif /* CONFIG_BLK_DEV_INTEGRITY */
 
 struct block_device_operations {
-- 
2.17.1

Re: [PATCH 1/2] block: move bio_integrity_{intervals,bytes} into blkdev.h

[Martin K. Petersen]

Greg,

> This allows bio_integrity_bytes() to be called from drivers instead of
> open coding it.

Looks fine.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 2/2] scsi: virtio_scsi: fix pi_bytes{out,in} on 4 KiB block size devices

[Greg Edwards]

The current calculation for pi_bytes{out,in} assumes a 512 byte logical
block size and a protection interval exponent of 0, i.e. 512 bytes data
+ 8 bytes PI.  When run on a 4 KiB logical block size device with a
protection interval exponent of 0, i.e. 4096 bytes data + 8 bytes PI,
the driver miscalculates the pi_bytes{out,in} by a factor of 8x (64
bytes).

This leads to errors on all reads and writes on 4 KiB logical block size
devices when CONFIG_BLK_DEV_INTEGRITY is enabled and the
VIRTIO_SCSI_F_T10_PI feature bit has been negotiated.

Fixes: e6dc783a38ec0 ("virtio-scsi: Enable DIF/DIX modes in SCSI host LLD")
Signed-off-by: Greg Edwards <gedwards@ddn.com>
---
 drivers/scsi/virtio_scsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 6dc8891ccb74..a1e85ab76f74 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -513,12 +513,12 @@ static void virtio_scsi_init_hdr_pi(struct virtio_device *vdev,
 
 	if (sc->sc_data_direction == DMA_TO_DEVICE)
 		cmd_pi->pi_bytesout = cpu_to_virtio32(vdev,
-							blk_rq_sectors(rq) *
-							bi->tuple_size);
+							bio_integrity_bytes(bi,
+							blk_rq_sectors(rq)));
 	else if (sc->sc_data_direction == DMA_FROM_DEVICE)
 		cmd_pi->pi_bytesin = cpu_to_virtio32(vdev,
-						       blk_rq_sectors(rq) *
-						       bi->tuple_size);
+							bio_integrity_bytes(bi,
+							blk_rq_sectors(rq)));
 }
 #endif
 
-- 
2.17.1

Re: [PATCH 2/2] scsi: virtio_scsi: fix pi_bytes{out,in} on 4 KiB block size devices

[Martin K. Petersen]

Greg,

> The current calculation for pi_bytes{out,in} assumes a 512 byte logical
> block size and a protection interval exponent of 0, i.e. 512 bytes data
> + 8 bytes PI.

The block layer always deals with units of 512 bytes. Regardless of the
underlying logical block size.

> When run on a 4 KiB logical block size device with a protection
> interval exponent of 0, i.e. 4096 bytes data + 8 bytes PI, the driver
> miscalculates the pi_bytes{out,in} by a factor of 8x (64 bytes).

> @@ -513,12 +513,12 @@ static void virtio_scsi_init_hdr_pi(struct virtio_device *vdev,
>  
>  	if (sc->sc_data_direction == DMA_TO_DEVICE)
>  		cmd_pi->pi_bytesout = cpu_to_virtio32(vdev,
> -							blk_rq_sectors(rq) *
> -							bi->tuple_size);
> +							bio_integrity_bytes(bi,
> +							blk_rq_sectors(rq)));

The formatting/alignment could be improved here. Otherwise OK.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v2] scsi: virtio_scsi: fix pi_bytes{out,in} on 4 KiB block size devices

[Greg Edwards]

When the underlying device is a 4 KiB logical block size device with a
protection interval exponent of 0, i.e. 4096 bytes data + 8 bytes PI, the
driver miscalculates the pi_bytes{out,in} by a factor of 8x (64 bytes).

This leads to errors on all reads and writes on 4 KiB logical block size
devices when CONFIG_BLK_DEV_INTEGRITY is enabled and the
VIRTIO_SCSI_F_T10_PI feature bit has been negotiated.

Fixes: e6dc783a38ec0 ("virtio-scsi: Enable DIF/DIX modes in SCSI host LLD")
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Greg Edwards <gedwards@ddn.com>
---
v1 -> v2:
  * change formatting per Martin's suggestion

 drivers/scsi/virtio_scsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 6dc8891ccb74..1c72db94270e 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -513,12 +513,12 @@ static void virtio_scsi_init_hdr_pi(struct virtio_device *vdev,
 
 	if (sc->sc_data_direction == DMA_TO_DEVICE)
 		cmd_pi->pi_bytesout = cpu_to_virtio32(vdev,
-							blk_rq_sectors(rq) *
-							bi->tuple_size);
+						      bio_integrity_bytes(bi,
+							blk_rq_sectors(rq)));
 	else if (sc->sc_data_direction == DMA_FROM_DEVICE)
 		cmd_pi->pi_bytesin = cpu_to_virtio32(vdev,
-						       blk_rq_sectors(rq) *
-						       bi->tuple_size);
+						     bio_integrity_bytes(bi,
+							blk_rq_sectors(rq)));
 }
 #endif
 
-- 
2.17.1

Re: [PATCH 0/2] virtio_scsi pi_bytes{out,in} miscalculated on 4 KiB devices

[Jens Axboe]

On 7/25/18 7:22 AM, Greg Edwards wrote:
> When the VIRTIO_SCSI_F_T10_PI feature bit is enabled, the virtio_scsi driver
> does not correctly calculate pi_bytes{out,in} when the underlying device has a
> 4 KiB logical block size.  The current code assumes a 512 byte logical block
> size and protection interval exponent of 0 (512 bytes + 8 bytes PI).
> 
> The first patch moves bio_integrity_intervals() and bio_integrity_bytes() into
> blkdev.h so drivers can make use of them.  The second patch modifies
> virtio_scsi to call bio_integrity_bytes() to get the values for
> pi_bytes{out,in}.

Applied, thanks.

-- 
Jens Axboe