| Message ID | 1428934918-4004-1-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4/13/2015 5:21 PM, Akinobu Mita wrote: > When CONFIG_DEBUG_SG=y and DIF protection support enabled, kernel > BUG()s are triggered due to the following two issues: > > 1) prot_sg is not initialized by sg_init_table(). > > When CONFIG_DEBUG_SG=y, scatterlist helpers check sg entry has a > correct magic value. > > 2) vmalloc'ed buffer is passed to sg_set_buf(). > > sg_set_buf() uses virt_to_page() to convert virtual address to struct > page, but it doesn't work with vmalloc address. vmalloc_to_page() > should be used instead. As prot_buf isn't usually too large, so > fix it by allocating prot_buf by kmalloc instead of vmalloc. > Hi Akinobu, This obviously fixes a bug, but I'm afraid that for certain workloads (large IO size) this would trigger higher order allocations. how about removing the prot_buf altogether? The only reason why this bounce is used is to allow code sharing with rd_mcp DIF mode calling sbc_verify_[write|read]. But I'd say it doesn't make sense anymore given these fixes. IMO, the t_prot_sg can be used just like t_data_sg. In the read case, we just read the data into t_prot_sg using bvec_iter (better to reuse fd_do_rw code) and just call __sbc_dif_verify_read (that would not copy sg's). In the write case, we call sbc_dif_verify_write() with NULL to avoid the copy and then just write it to the file (reusing fd_do_rw code). Thoughts? Sagi. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-04-13 at 23:21 +0900, Akinobu Mita wrote: > When CONFIG_DEBUG_SG=y and DIF protection support enabled, kernel > BUG()s are triggered due to the following two issues: > > 1) prot_sg is not initialized by sg_init_table(). > > When CONFIG_DEBUG_SG=y, scatterlist helpers check sg entry has a > correct magic value. > > 2) vmalloc'ed buffer is passed to sg_set_buf(). > > sg_set_buf() uses virt_to_page() to convert virtual address to struct > page, but it doesn't work with vmalloc address. vmalloc_to_page() > should be used instead. As prot_buf isn't usually too large, so > fix it by allocating prot_buf by kmalloc instead of vmalloc. > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > Cc: Nicholas Bellinger <nab@linux-iscsi.org> > Cc: Sagi Grimberg <sagig@mellanox.com> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> > Cc: target-devel@vger.kernel.org > Cc: linux-scsi@vger.kernel.org > --- > drivers/target/target_core_file.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > Thanks for testing with CONFIG_DEBUG_SG=y. Applied to target-pending/for-next, with CC' for v3.14.y stable. Thank you, --nab -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index 44620fb..8ca1883 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -274,7 +274,7 @@ static int fd_do_prot_rw(struct se_cmd *cmd, struct fd_prot *fd_prot, se_dev->prot_length; if (!is_write) { - fd_prot->prot_buf = vzalloc(prot_size); + fd_prot->prot_buf = kzalloc(prot_size, GFP_KERNEL); if (!fd_prot->prot_buf) { pr_err("Unable to allocate fd_prot->prot_buf\n"); return -ENOMEM; @@ -286,9 +286,10 @@ static int fd_do_prot_rw(struct se_cmd *cmd, struct fd_prot *fd_prot, fd_prot->prot_sg_nents, GFP_KERNEL); if (!fd_prot->prot_sg) { pr_err("Unable to allocate fd_prot->prot_sg\n"); - vfree(fd_prot->prot_buf); + kfree(fd_prot->prot_buf); return -ENOMEM; } + sg_init_table(fd_prot->prot_sg, fd_prot->prot_sg_nents); size = prot_size; for_each_sg(fd_prot->prot_sg, sg, fd_prot->prot_sg_nents, i) { @@ -318,7 +319,7 @@ static int fd_do_prot_rw(struct se_cmd *cmd, struct fd_prot *fd_prot, if (is_write || ret < 0) { kfree(fd_prot->prot_sg); - vfree(fd_prot->prot_buf); + kfree(fd_prot->prot_buf); } return ret; @@ -658,11 +659,11 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, 0, fd_prot.prot_sg, 0); if (rc) { kfree(fd_prot.prot_sg); - vfree(fd_prot.prot_buf); + kfree(fd_prot.prot_buf); return rc; } kfree(fd_prot.prot_sg); - vfree(fd_prot.prot_buf); + kfree(fd_prot.prot_buf); } } else { memset(&fd_prot, 0, sizeof(struct fd_prot)); @@ -678,7 +679,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, 0, fd_prot.prot_sg, 0); if (rc) { kfree(fd_prot.prot_sg); - vfree(fd_prot.prot_buf); + kfree(fd_prot.prot_buf); return rc; } } @@ -714,7 +715,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, if (ret < 0) { kfree(fd_prot.prot_sg); - vfree(fd_prot.prot_buf); + kfree(fd_prot.prot_buf); return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; }
When CONFIG_DEBUG_SG=y and DIF protection support enabled, kernel BUG()s are triggered due to the following two issues: 1) prot_sg is not initialized by sg_init_table(). When CONFIG_DEBUG_SG=y, scatterlist helpers check sg entry has a correct magic value. 2) vmalloc'ed buffer is passed to sg_set_buf(). sg_set_buf() uses virt_to_page() to convert virtual address to struct page, but it doesn't work with vmalloc address. vmalloc_to_page() should be used instead. As prot_buf isn't usually too large, so fix it by allocating prot_buf by kmalloc instead of vmalloc. Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> Cc: Nicholas Bellinger <nab@linux-iscsi.org> Cc: Sagi Grimberg <sagig@mellanox.com> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: Christoph Hellwig <hch@lst.de> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> Cc: target-devel@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_file.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)