[PATCH 0/4] target unmap/writespace fixes and enhancements

[PATCH 0/4] target unmap/writespace fixes and enhancements

[Mike Christie]

The following patches were made over Linus's tree. The fix a crash that's
hit when using writesame with the NDOB bit, and allow uses to config
unmap after it's been setup in LIO.

[PATCH 1/4] scsi: target: Fix WRITE_SAME NDOB size check

[Mike Christie]

If the WRITE_SAME NDOB bit is set then there is not going to be a
buffer. LIO core will then complain:

TARGET_CORE[iSCSI]: Expected Transfer Length: 0 does not match SCSI CDB
Length: 512 for SAM Opcode: 0x93

This fixes the issue by detecting when the bit is set and adjusting the
size.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_sbc.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index ca1b2312d6e7..6d98b016a942 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -976,7 +976,11 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 				return TCM_INVALID_CDB_FIELD;
 			}
 
-			size = sbc_get_size(cmd, 1);
+			/* If NDOB is set there will not be a Data-Out buffer */
+			if (cdb[1] & 0x01)
+				size = 0;
+			else
+				size = sbc_get_size(cmd, 1);
 			cmd->t_task_lba = get_unaligned_be64(&cdb[12]);
 
 			ret = sbc_setup_write_same(cmd, cdb[10], ops);
@@ -1075,7 +1079,11 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 			return TCM_INVALID_CDB_FIELD;
 		}
 
-		size = sbc_get_size(cmd, 1);
+		/* If NDOB is set there will not be a Data-Out buffer */
+		if (cdb[1] & 0x01)
+			size = 0;
+		else
+			size = sbc_get_size(cmd, 1);
 		cmd->t_task_lba = get_unaligned_be64(&cdb[2]);
 
 		ret = sbc_setup_write_same(cmd, cdb[1], ops);
-- 
2.25.1

[PATCH 2/4] scsi: target: Fix WRITE_SAME NDOB handling in file

[Mike Christie]

If NDOB is set we don't have a buffer. We will then crash when trying to
access the t_data_sg. This has us allocate a page to use for the data
buffer that gets passed to vfs_iter_write.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_file.c | 32 +++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index e68f1cc8ef98..2011836ab7f4 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -433,6 +433,9 @@ fd_execute_write_same(struct se_cmd *cmd)
 	struct fd_dev *fd_dev = FD_DEV(se_dev);
 	loff_t pos = cmd->t_task_lba * se_dev->dev_attrib.block_size;
 	sector_t nolb = sbc_get_write_same_sectors(cmd);
+	bool ndob = cmd->t_task_cdb[1] & 0x01;
+	struct scatterlist *sg, ndob_sg;
+	struct page *pg = NULL;
 	struct iov_iter iter;
 	struct bio_vec *bvec;
 	unsigned int len = 0, i;
@@ -447,13 +450,13 @@ fd_execute_write_same(struct se_cmd *cmd)
 		       " backends not supported\n");
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	}
+	sg = &cmd->t_data_sg[0];
 
 	if (cmd->t_data_nents > 1 ||
-	    cmd->t_data_sg[0].length != cmd->se_dev->dev_attrib.block_size) {
+	    (sg && sg->length != cmd->se_dev->dev_attrib.block_size)) {
 		pr_err("WRITE_SAME: Illegal SGL t_data_nents: %u length: %u"
 			" block_size: %u\n",
-			cmd->t_data_nents,
-			cmd->t_data_sg[0].length,
+			cmd->t_data_nents, sg->length,
 			cmd->se_dev->dev_attrib.block_size);
 		return TCM_INVALID_CDB_FIELD;
 	}
@@ -462,10 +465,23 @@ fd_execute_write_same(struct se_cmd *cmd)
 	if (!bvec)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
+	if (ndob) {
+		pg = alloc_page(GFP_KERNEL | __GFP_ZERO);
+		if (!pg) {
+			kfree(bvec);
+			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+		}
+
+		sg_init_table(&ndob_sg, 1);
+		sg_set_page(&ndob_sg, pg, cmd->se_dev->dev_attrib.block_size,
+			    0);
+		sg = &ndob_sg;
+	}
+
 	for (i = 0; i < nolb; i++) {
-		bvec[i].bv_page = sg_page(&cmd->t_data_sg[0]);
-		bvec[i].bv_len = cmd->t_data_sg[0].length;
-		bvec[i].bv_offset = cmd->t_data_sg[0].offset;
+		bvec[i].bv_page = sg_page(sg);
+		bvec[i].bv_len = sg->length;
+		bvec[i].bv_offset = sg->offset;
 
 		len += se_dev->dev_attrib.block_size;
 	}
@@ -474,6 +490,10 @@ fd_execute_write_same(struct se_cmd *cmd)
 	ret = vfs_iter_write(fd_dev->fd_file, &iter, &pos, 0);
 
 	kfree(bvec);
+
+	if (pg)
+		__free_page(pg);
+
 	if (ret < 0 || ret != len) {
 		pr_err("vfs_iter_write() returned %zd for write same\n", ret);
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-- 
2.25.1

[PATCH 3/4] scsi: target: Fix WRITE_SAME NDOB handling in iblock

[Mike Christie]

If NDOB is set we don't have a buffer and we will crash when we access
the t_data_sg. This has us allocate a page to use for the data
buffer that gets passed to the block layer. For the
iblock_execute_zero_out case we just add a check for if NDOB was set.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_iblock.c | 36 ++++++++++++++++++++++++-----
 drivers/target/target_core_iblock.h |  1 +
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 378c80313a0f..767ed2e9474b 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -320,6 +320,9 @@ static void iblock_complete_cmd(struct se_cmd *cmd)
 		status = SAM_STAT_GOOD;
 
 	target_complete_cmd(cmd, status);
+
+	if (ibr->pg)
+		__free_page(ibr->pg);
 	kfree(ibr);
 }
 
@@ -444,13 +447,17 @@ iblock_execute_unmap(struct se_cmd *cmd, sector_t lba, sector_t nolb)
 }
 
 static sense_reason_t
-iblock_execute_zero_out(struct block_device *bdev, struct se_cmd *cmd)
+iblock_execute_zero_out(struct block_device *bdev, struct se_cmd *cmd,
+			bool ndob)
 {
 	struct se_device *dev = cmd->se_dev;
 	struct scatterlist *sg = &cmd->t_data_sg[0];
 	unsigned char *buf, *not_zero;
 	int ret;
 
+	if (ndob)
+		goto issue_zero;
+
 	buf = kmap(sg_page(sg)) + sg->offset;
 	if (!buf)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
@@ -464,6 +471,7 @@ iblock_execute_zero_out(struct block_device *bdev, struct se_cmd *cmd)
 	if (not_zero)
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
+issue_zero:
 	ret = blkdev_issue_zeroout(bdev,
 				target_to_linux_sector(dev, cmd->t_task_lba),
 				target_to_linux_sector(dev,
@@ -481,13 +489,15 @@ iblock_execute_write_same(struct se_cmd *cmd)
 {
 	struct block_device *bdev = IBLOCK_DEV(cmd->se_dev)->ibd_bd;
 	struct iblock_req *ibr;
-	struct scatterlist *sg;
+	struct scatterlist *sg, ndob_sg;
+	struct page *pg = NULL;
 	struct bio *bio;
 	struct bio_list list;
 	struct se_device *dev = cmd->se_dev;
 	sector_t block_lba = target_to_linux_sector(dev, cmd->t_task_lba);
 	sector_t sectors = target_to_linux_sector(dev,
 					sbc_get_write_same_sectors(cmd));
+	bool ndob = cmd->t_task_cdb[1] & 0x01;
 
 	if (cmd->prot_op) {
 		pr_err("WRITE_SAME: Protection information with IBLOCK"
@@ -497,7 +507,7 @@ iblock_execute_write_same(struct se_cmd *cmd)
 	sg = &cmd->t_data_sg[0];
 
 	if (cmd->t_data_nents > 1 ||
-	    sg->length != cmd->se_dev->dev_attrib.block_size) {
+	    (sg && sg->length != cmd->se_dev->dev_attrib.block_size)) {
 		pr_err("WRITE_SAME: Illegal SGL t_data_nents: %u length: %u"
 			" block_size: %u\n", cmd->t_data_nents, sg->length,
 			cmd->se_dev->dev_attrib.block_size);
@@ -505,14 +515,26 @@ iblock_execute_write_same(struct se_cmd *cmd)
 	}
 
 	if (bdev_write_zeroes_sectors(bdev)) {
-		if (!iblock_execute_zero_out(bdev, cmd))
+		if (!iblock_execute_zero_out(bdev, cmd, ndob))
 			return 0;
 	}
 
+	if (ndob) {
+		pg = alloc_page(GFP_KERNEL | __GFP_ZERO);
+		if (!pg)
+			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+		sg_init_table(&ndob_sg, 1);
+		sg_set_page(&ndob_sg, pg, cmd->se_dev->dev_attrib.block_size,
+			    0);
+		sg = &ndob_sg;
+	}
+
 	ibr = kzalloc(sizeof(struct iblock_req), GFP_KERNEL);
 	if (!ibr)
-		goto fail;
+		goto fail_free_pg;
 	cmd->priv = ibr;
+	ibr->pg = pg;
 
 	bio = iblock_get_bio(cmd, block_lba, 1, REQ_OP_WRITE);
 	if (!bio)
@@ -548,7 +570,9 @@ iblock_execute_write_same(struct se_cmd *cmd)
 		bio_put(bio);
 fail_free_ibr:
 	kfree(ibr);
-fail:
+fail_free_pg:
+	if (pg)
+		__free_page(pg);
 	return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 }
 
diff --git a/drivers/target/target_core_iblock.h b/drivers/target/target_core_iblock.h
index 8c55375d2f75..0f3cf72b544b 100644
--- a/drivers/target/target_core_iblock.h
+++ b/drivers/target/target_core_iblock.h
@@ -14,6 +14,7 @@
 struct iblock_req {
 	refcount_t pending;
 	atomic_t ib_bio_err_cnt;
+	struct page *pg;
 } ____cacheline_aligned;
 
 #define IBDF_HAS_UDEV_PATH		0x01
-- 
2.25.1

[PATCH 4/4] scsi: target: Detect unmap support post configuration

[Mike Christie]

On our backend we can do something similar to LIO where we can enable and
disable unmap support on the fly. In the scsi/block layer we can detect
this by just doing a rescan. However, LIO cannot detect this change
because we only check during the initial configuration. This patch
allows unmap detection to also happen when the user tries to turn it on.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_configfs.c | 27 +++++++++++++++++++--------
 drivers/target/target_core_file.c     | 15 ++++++++++++++-
 drivers/target/target_core_iblock.c   | 11 ++++++++++-
 include/target/target_core_backend.h  |  1 +
 4 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index bbcbbfa72b07..416514c5c7ac 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -732,6 +732,7 @@ static ssize_t emulate_tpu_store(struct config_item *item,
 		const char *page, size_t count)
 {
 	struct se_dev_attrib *da = to_attrib(item);
+	struct se_device *dev = da->da_dev;
 	bool flag;
 	int ret;
 
@@ -744,8 +745,11 @@ static ssize_t emulate_tpu_store(struct config_item *item,
 	 * Discard supported is detected iblock_create_virtdevice().
 	 */
 	if (flag && !da->max_unmap_block_desc_count) {
-		pr_err("Generic Block Discard not supported\n");
-		return -ENOSYS;
+		if (!dev->transport->configure_unmap ||
+		    !dev->transport->configure_unmap(dev)) {
+			pr_err("Generic Block Discard not supported\n");
+			return -ENOSYS;
+		}
 	}
 
 	da->emulate_tpu = flag;
@@ -758,6 +762,7 @@ static ssize_t emulate_tpws_store(struct config_item *item,
 		const char *page, size_t count)
 {
 	struct se_dev_attrib *da = to_attrib(item);
+	struct se_device *dev = da->da_dev;
 	bool flag;
 	int ret;
 
@@ -770,8 +775,11 @@ static ssize_t emulate_tpws_store(struct config_item *item,
 	 * Discard supported is detected iblock_create_virtdevice().
 	 */
 	if (flag && !da->max_unmap_block_desc_count) {
-		pr_err("Generic Block Discard not supported\n");
-		return -ENOSYS;
+		if (!dev->transport->configure_unmap ||
+		    !dev->transport->configure_unmap(dev)) {
+			pr_err("Generic Block Discard not supported\n");
+			return -ENOSYS;
+		}
 	}
 
 	da->emulate_tpws = flag;
@@ -964,6 +972,7 @@ static ssize_t unmap_zeroes_data_store(struct config_item *item,
 		const char *page, size_t count)
 {
 	struct se_dev_attrib *da = to_attrib(item);
+	struct se_device *dev = da->da_dev;
 	bool flag;
 	int ret;
 
@@ -982,10 +991,12 @@ static ssize_t unmap_zeroes_data_store(struct config_item *item,
 	 * Discard supported is detected iblock_configure_device().
 	 */
 	if (flag && !da->max_unmap_block_desc_count) {
-		pr_err("dev[%p]: Thin Provisioning LBPRZ will not be set"
-		       " because max_unmap_block_desc_count is zero\n",
-		       da->da_dev);
-		return -ENOSYS;
+		if (!dev->transport->configure_unmap ||
+		    !dev->transport->configure_unmap(dev)) {
+			pr_err("dev[%p]: Thin Provisioning LBPRZ will not be set because max_unmap_block_desc_count is zero\n",
+			       da->da_dev);
+			return -ENOSYS;
+		}
 	}
 	da->unmap_zeroes_data = flag;
 	pr_debug("dev[%p]: SE Device Thin Provisioning LBPRZ bit: %d\n",
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 2011836ab7f4..a7c6df01d554 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -86,6 +86,18 @@ static struct se_device *fd_alloc_device(struct se_hba *hba, const char *name)
 	return &fd_dev->dev;
 }
 
+static bool fd_configure_unmap(struct se_device *dev)
+{
+	struct file *file = FD_DEV(dev)->fd_file;
+	struct inode *inode = file->f_mapping->host;
+
+	if (!S_ISBLK(inode->i_mode))
+		return true;
+
+	return target_configure_unmap_from_queue(&dev->dev_attrib,
+						 I_BDEV(inode));
+}
+
 static int fd_configure_device(struct se_device *dev)
 {
 	struct fd_dev *fd_dev = FD_DEV(dev);
@@ -150,7 +162,7 @@ static int fd_configure_device(struct se_device *dev)
 			dev_size, div_u64(dev_size, fd_dev->fd_block_size),
 			fd_dev->fd_block_size);
 
-		if (target_configure_unmap_from_queue(&dev->dev_attrib, bdev))
+		if (fd_configure_unmap(dev))
 			pr_debug("IFILE: BLOCK Discard support available,"
 				 " disabled by default\n");
 		/*
@@ -944,6 +956,7 @@ static const struct target_backend_ops fileio_ops = {
 	.configure_device	= fd_configure_device,
 	.destroy_device		= fd_destroy_device,
 	.free_device		= fd_free_device,
+	.configure_unmap	= fd_configure_unmap,
 	.parse_cdb		= fd_parse_cdb,
 	.set_configfs_dev_params = fd_set_configfs_dev_params,
 	.show_configfs_dev_params = fd_show_configfs_dev_params,
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 767ed2e9474b..c435b6dd3ae2 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -76,6 +76,14 @@ static struct se_device *iblock_alloc_device(struct se_hba *hba, const char *nam
 	return NULL;
 }
 
+static bool iblock_configure_unmap(struct se_device *dev)
+{
+	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
+
+	return target_configure_unmap_from_queue(&dev->dev_attrib,
+						 ib_dev->ibd_bd);
+}
+
 static int iblock_configure_device(struct se_device *dev)
 {
 	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
@@ -119,7 +127,7 @@ static int iblock_configure_device(struct se_device *dev)
 	dev->dev_attrib.hw_max_sectors = queue_max_hw_sectors(q);
 	dev->dev_attrib.hw_queue_depth = q->nr_requests;
 
-	if (target_configure_unmap_from_queue(&dev->dev_attrib, bd))
+	if (iblock_configure_unmap(dev))
 		pr_debug("IBLOCK: BLOCK Discard support available,"
 			 " disabled by default\n");
 
@@ -923,6 +931,7 @@ static const struct target_backend_ops iblock_ops = {
 	.configure_device	= iblock_configure_device,
 	.destroy_device		= iblock_destroy_device,
 	.free_device		= iblock_free_device,
+	.configure_unmap	= iblock_configure_unmap,
 	.plug_device		= iblock_plug_device,
 	.unplug_device		= iblock_unplug_device,
 	.parse_cdb		= iblock_parse_cdb,
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 773963a1e0b5..a3c193df25b3 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -37,6 +37,7 @@ struct target_backend_ops {
 	struct se_dev_plug *(*plug_device)(struct se_device *se_dev);
 	void (*unplug_device)(struct se_dev_plug *se_plug);
 
+	bool (*configure_unmap)(struct se_device *se_dev);
 	ssize_t (*set_configfs_dev_params)(struct se_device *,
 					   const char *, ssize_t);
 	ssize_t (*show_configfs_dev_params)(struct se_device *, char *);
-- 
2.25.1

Re: [PATCH 1/4] scsi: target: Fix WRITE_SAME NDOB size check

[Christoph Hellwig]

On Thu, Jun 16, 2022 at 10:04:36PM -0500, Mike Christie wrote:
> If the WRITE_SAME NDOB bit is set then there is not going to be a
> buffer. LIO core will then complain:
> 
> TARGET_CORE[iSCSI]: Expected Transfer Length: 0 does not match SCSI CDB
> Length: 512 for SAM Opcode: 0x93
> 
> This fixes the issue by detecting when the bit is set and adjusting the
> size.


The patch looks good and useful, but right the taret code doesn't even
support MI_REPORT_SUPPORTED_OPERATION_CODES to report support for NDOB,
so who ends up using it?

Re: [PATCH 2/4] scsi: target: Fix WRITE_SAME NDOB handling in file

[Christoph Hellwig]

On Thu, Jun 16, 2022 at 10:04:37PM -0500, Mike Christie wrote:
> If NDOB is set we don't have a buffer. We will then crash when trying to
> access the t_data_sg. This has us allocate a page to use for the data
> buffer that gets passed to vfs_iter_write.

But only due to the last patch, so this should be reordered before
that.

I also think this should just use ZERO_PAGE() or even better switch to
FALLOC_FL_ZERO_RANGE as a first pass.

Re: [PATCH 3/4] scsi: target: Fix WRITE_SAME NDOB handling in iblock

[Christoph Hellwig]

On Thu, Jun 16, 2022 at 10:04:38PM -0500, Mike Christie wrote:
> If NDOB is set we don't have a buffer and we will crash when we access
> the t_data_sg. This has us allocate a page to use for the data
> buffer that gets passed to the block layer. For the
> iblock_execute_zero_out case we just add a check for if NDOB was set.

Same as for the file backend applies here.

Re: [PATCH 4/4] scsi: target: Detect unmap support post configuration

[Christoph Hellwig]

On Thu, Jun 16, 2022 at 10:04:39PM -0500, Mike Christie wrote:
> On our backend we can do something similar to LIO where we can enable and
> disable unmap support on the fly. In the scsi/block layer we can detect
> this by just doing a rescan. However, LIO cannot detect this change
> because we only check during the initial configuration. This patch
> allows unmap detection to also happen when the user tries to turn it on.

Please also call the new callback from the generic code during initial
setup instead of leaving that to the backends.

Re: [PATCH 1/4] scsi: target: Fix WRITE_SAME NDOB size check

[michael.christie]

On 6/19/22 1:22 AM, Christoph Hellwig wrote:
> On Thu, Jun 16, 2022 at 10:04:36PM -0500, Mike Christie wrote:
>> If the WRITE_SAME NDOB bit is set then there is not going to be a
>> buffer. LIO core will then complain:
>>
>> TARGET_CORE[iSCSI]: Expected Transfer Length: 0 does not match SCSI CDB
>> Length: 512 for SAM Opcode: 0x93
>>
>> This fixes the issue by detecting when the bit is set and adjusting the
>> size.
> 
> 
> The patch looks good and useful, but right the taret code doesn't even
> support MI_REPORT_SUPPORTED_OPERATION_CODES to report support for NDOB,
> so who ends up using it?

sg_write_same allows it. We found the bug because some user just decided
to do:

sg_write_same ... -nbod .. /dev/sdb

and it crashed the box.

I didn't know about the MI_REPORT_SUPPORTED_OPERATION_CODES part of it.
I don't need support for the feature. I just want to fix the crash.
I prefer just returning failure since nothing ever has ever used it if
other people prefer that as well.

Re: [PATCH 2/4] scsi: target: Fix WRITE_SAME NDOB handling in file

[michael.christie]

On 6/19/22 1:25 AM, Christoph Hellwig wrote:
> On Thu, Jun 16, 2022 at 10:04:37PM -0500, Mike Christie wrote:
>> If NDOB is set we don't have a buffer. We will then crash when trying to
>> access the t_data_sg. This has us allocate a page to use for the data
>> buffer that gets passed to vfs_iter_write.
> 
> But only due to the last patch, so this should be reordered before
> that.

I didn't get that part. You can hit this bug with what is upstream right
now. You don't need patch 4.

> 
> I also think this should just use ZERO_PAGE() or even better switch to
> FALLOC_FL_ZERO_RANGE as a first pass.
> 

Ok.

Re: [PATCH 2/4] scsi: target: Fix WRITE_SAME NDOB handling in file

[michael.christie]

On 6/19/22 11:26 AM, michael.christie@oracle.com wrote:
> On 6/19/22 1:25 AM, Christoph Hellwig wrote:
>> On Thu, Jun 16, 2022 at 10:04:37PM -0500, Mike Christie wrote:
>>> If NDOB is set we don't have a buffer. We will then crash when trying to
>>> access the t_data_sg. This has us allocate a page to use for the data
>>> buffer that gets passed to vfs_iter_write.
>>
>> But only due to the last patch, so this should be reordered before
>> that.
> 
> I didn't get that part. You can hit this bug with what is upstream right
> now. You don't need patch 4.
> 

I see you meant the patch before this one :)

Without patch 1, you still hit the crash. In the current code, we print error,
call the execute_cmd callout, then for file and iblock crash because there is
no sg and we assume there always will be one.

Re: [PATCH 1/4] scsi: target: Fix WRITE_SAME NDOB size check

[Christoph Hellwig]

On Sun, Jun 19, 2022 at 11:25:33AM -0500, michael.christie@oracle.com wrote:
> sg_write_same allows it. We found the bug because some user just decided
> to do:
> 
> sg_write_same ... -nbod .. /dev/sdb
> 
> and it crashed the box.

Oh.

> I didn't know about the MI_REPORT_SUPPORTED_OPERATION_CODES part of it.
> I don't need support for the feature. I just want to fix the crash.
> I prefer just returning failure since nothing ever has ever used it if
> other people prefer that as well.

I think the feature is generally useful, and I know Martin had patches
to use it in Linux.  But I think a minimal fix for the remotely
exploitable crash has the highest priority.  Where does it crash?
Maybe we just need a better sanity check somewhere if a command
claims to transfer data but has not payload?

Re: [PATCH 1/4] scsi: target: Fix WRITE_SAME NDOB size check

[Mike Christie]

On 6/20/22 1:45 AM, Christoph Hellwig wrote:
> On Sun, Jun 19, 2022 at 11:25:33AM -0500, michael.christie@oracle.com wrote:
>> sg_write_same allows it. We found the bug because some user just decided
>> to do:
>>
>> sg_write_same ... -nbod .. /dev/sdb
>>
>> and it crashed the box.
> 
> Oh.
> 
>> I didn't know about the MI_REPORT_SUPPORTED_OPERATION_CODES part of it.
>> I don't need support for the feature. I just want to fix the crash.
>> I prefer just returning failure since nothing ever has ever used it if
>> other people prefer that as well.
> 
> I think the feature is generally useful, and I know Martin had patches
> to use it in Linux.  But I think a minimal fix for the remotely

I'll work with Martin to find if there is an oracle user to test and on a
longer term feature addition.

> exploitable crash has the highest priority.  Where does it crash?

It crashes when we first access the sg in file and iblock's
execute_write_same functions.

> Maybe we just need a better sanity check somewhere if a command
> claims to transfer data but has not payload?

I'll look into it and send a patch.