| Message ID | 20220617030440.116427-3-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target unmap/writespace fixes and enhancements | expand |
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.
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.
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.
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;
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(-)