| Message ID | 1429972410-7146-2-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4/25/2015 5:33 PM, Akinobu Mita wrote: > Even if the device backend is initialized with protection info is > enabled, some requests don't have the protection info attached for > WRITE SAME command issued by block device helpers, WRITE command with > WRPROTECT=0 by SG_IO ioctl, etc. > > So when TCM loopback fabric module is used, se_cmd->t_prot_sg is NULL > for these requests and performing WRITE_INSERT of PI using software > emulation by sbc_dif_generate() causes kernel crash. > > To fix this, introduce SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC for > se_cmd_flags, which is used to determine that se_cmd->t_prot_sg needs > to be allocated or use pre-allocated protection information by scsi > mid-layer. > > 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 > --- > * No change from v2 > > drivers/target/target_core_transport.c | 30 ++++++++++++++++++------------ > include/target/target_core_base.h | 1 + > 2 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 7a9e7e2..fe52883 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -1450,6 +1450,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess > if (sgl_prot_count) { > se_cmd->t_prot_sg = sgl_prot; > se_cmd->t_prot_nents = sgl_prot_count; > + se_cmd->se_cmd_flags |= SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC; > } > > /* > @@ -2181,6 +2182,12 @@ static inline void transport_reset_sgl_orig(struct se_cmd *cmd) > > static inline void transport_free_pages(struct se_cmd *cmd) > { > + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { > + transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); > + cmd->t_prot_sg = NULL; > + cmd->t_prot_nents = 0; > + } > + Hi Akinobu, Any reason why this changed it's location to the start of the function? > if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) { > /* > * Release special case READ buffer payload required for > @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct se_cmd *cmd) > transport_free_sgl(cmd->t_bidi_data_sg, cmd->t_bidi_data_nents); > cmd->t_bidi_data_sg = NULL; > cmd->t_bidi_data_nents = 0; > - > - transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); > - cmd->t_prot_sg = NULL; > - cmd->t_prot_nents = 0; > } > > /** > @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd) > int ret = 0; > bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB); > > + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { > + if (cmd->prot_op != TARGET_PROT_NORMAL) { This seems wrong, What will happen for transports that will actually to allocate protection SGLs? The allocation is unreachable since SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is not set... I'd say this needs to be: if (cmd->prot_op != TARGET_PROT_NORMAL && !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { > + ret = target_alloc_sgl(&cmd->t_prot_sg, > + &cmd->t_prot_nents, > + cmd->prot_length, true); > + if (ret < 0) > + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > + } > + > + } > + > /* > * Determine is the TCM fabric module has already allocated physical > * memory, and is directly calling transport_generic_map_mem_to_cmd() > @@ -2371,14 +2385,6 @@ transport_generic_new_cmd(struct se_cmd *cmd) > return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > } > > - if (cmd->prot_op != TARGET_PROT_NORMAL) { > - ret = target_alloc_sgl(&cmd->t_prot_sg, > - &cmd->t_prot_nents, > - cmd->prot_length, true); > - if (ret < 0) > - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; > - } > - > ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents, > cmd->data_length, zero_flag); > if (ret < 0) > diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h > index 480e9f8..13efcdd 100644 > --- a/include/target/target_core_base.h > +++ b/include/target/target_core_base.h > @@ -167,6 +167,7 @@ enum se_cmd_flags_table { > SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000, > SCF_COMPARE_AND_WRITE = 0x00080000, > SCF_COMPARE_AND_WRITE_POST = 0x00100000, > + SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000, > }; > > /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */ > -- 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 4/26/2015 12:26 PM, Sagi Grimberg wrote: > On 4/25/2015 5:33 PM, Akinobu Mita wrote: >> Even if the device backend is initialized with protection info is >> enabled, some requests don't have the protection info attached for >> WRITE SAME command issued by block device helpers, WRITE command with >> WRPROTECT=0 by SG_IO ioctl, etc. >> >> So when TCM loopback fabric module is used, se_cmd->t_prot_sg is NULL >> for these requests and performing WRITE_INSERT of PI using software >> emulation by sbc_dif_generate() causes kernel crash. >> >> To fix this, introduce SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC for >> se_cmd_flags, which is used to determine that se_cmd->t_prot_sg needs >> to be allocated or use pre-allocated protection information by scsi >> mid-layer. >> >> 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 >> --- >> * No change from v2 >> >> drivers/target/target_core_transport.c | 30 >> ++++++++++++++++++------------ >> include/target/target_core_base.h | 1 + >> 2 files changed, 19 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/target/target_core_transport.c >> b/drivers/target/target_core_transport.c >> index 7a9e7e2..fe52883 100644 >> --- a/drivers/target/target_core_transport.c >> +++ b/drivers/target/target_core_transport.c >> @@ -1450,6 +1450,7 @@ int target_submit_cmd_map_sgls(struct se_cmd >> *se_cmd, struct se_session *se_sess >> if (sgl_prot_count) { >> se_cmd->t_prot_sg = sgl_prot; >> se_cmd->t_prot_nents = sgl_prot_count; >> + se_cmd->se_cmd_flags |= SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC; >> } >> >> /* >> @@ -2181,6 +2182,12 @@ static inline void >> transport_reset_sgl_orig(struct se_cmd *cmd) >> >> static inline void transport_free_pages(struct se_cmd *cmd) >> { >> + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { >> + transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); >> + cmd->t_prot_sg = NULL; >> + cmd->t_prot_nents = 0; >> + } >> + > > Hi Akinobu, > > Any reason why this changed it's location to the start of the function? > >> if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) { >> /* >> * Release special case READ buffer payload required for >> @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct >> se_cmd *cmd) >> transport_free_sgl(cmd->t_bidi_data_sg, cmd->t_bidi_data_nents); >> cmd->t_bidi_data_sg = NULL; >> cmd->t_bidi_data_nents = 0; >> - >> - transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); >> - cmd->t_prot_sg = NULL; >> - cmd->t_prot_nents = 0; >> } >> >> /** >> @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd) >> int ret = 0; >> bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB); >> >> + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { >> + if (cmd->prot_op != TARGET_PROT_NORMAL) { > > This seems wrong, > > What will happen for transports that will actually to allocate > protection SGLs? The allocation is unreachable since > SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is not set... Umm, actually this is reachable... But I still think the condition should be the other way around (saving a condition in some common cases). > > I'd say this needs to be: > > if (cmd->prot_op != TARGET_PROT_NORMAL && > !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { > >> + ret = target_alloc_sgl(&cmd->t_prot_sg, >> + &cmd->t_prot_nents, >> + cmd->prot_length, true); >> + if (ret < 0) >> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; >> + } >> + >> + } >> + >> /* >> * Determine is the TCM fabric module has already allocated >> physical >> * memory, and is directly calling >> transport_generic_map_mem_to_cmd() >> @@ -2371,14 +2385,6 @@ transport_generic_new_cmd(struct se_cmd *cmd) >> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; >> } >> >> - if (cmd->prot_op != TARGET_PROT_NORMAL) { >> - ret = target_alloc_sgl(&cmd->t_prot_sg, >> - &cmd->t_prot_nents, >> - cmd->prot_length, true); >> - if (ret < 0) >> - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; >> - } >> - >> ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents, >> cmd->data_length, zero_flag); >> if (ret < 0) >> diff --git a/include/target/target_core_base.h >> b/include/target/target_core_base.h >> index 480e9f8..13efcdd 100644 >> --- a/include/target/target_core_base.h >> +++ b/include/target/target_core_base.h >> @@ -167,6 +167,7 @@ enum se_cmd_flags_table { >> SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000, >> SCF_COMPARE_AND_WRITE = 0x00080000, >> SCF_COMPARE_AND_WRITE_POST = 0x00100000, >> + SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000, >> }; >> >> /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */ >> > -- 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
2015-04-26 18:44 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>: >>> @@ -2181,6 +2182,12 @@ static inline void >>> transport_reset_sgl_orig(struct se_cmd *cmd) >>> >>> static inline void transport_free_pages(struct se_cmd *cmd) >>> { >>> + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { >>> + transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); >>> + cmd->t_prot_sg = NULL; >>> + cmd->t_prot_nents = 0; >>> + } >>> + >> >> >> Hi Akinobu, >> >> Any reason why this changed it's location to the start of the function? Because when SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC is set, it will not reach the tail of the function. So when SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC is cleared and SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is set, se_cmd->t_prot_sg leaks. >>> if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) { >>> /* >>> * Release special case READ buffer payload required for >>> @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct >>> se_cmd *cmd) >>> transport_free_sgl(cmd->t_bidi_data_sg, cmd->t_bidi_data_nents); >>> cmd->t_bidi_data_sg = NULL; >>> cmd->t_bidi_data_nents = 0; >>> - >>> - transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); >>> - cmd->t_prot_sg = NULL; >>> - cmd->t_prot_nents = 0; >>> } >>> >>> /** >>> @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd) >>> int ret = 0; >>> bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB); >>> >>> + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { >>> + if (cmd->prot_op != TARGET_PROT_NORMAL) { >> >> >> This seems wrong, >> >> What will happen for transports that will actually to allocate >> protection SGLs? The allocation is unreachable since >> SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is not set... > > > Umm, actually this is reachable... But I still think the condition > should be the other way around (saving a condition in some common > cases). Do you mean you prefer below? if (cmd->prot_op != TARGET_PROT_NORMAL && !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { ... >> >> I'd say this needs to be: >> >> if (cmd->prot_op != TARGET_PROT_NORMAL && >> !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { -- 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 4/27/2015 3:57 PM, Akinobu Mita wrote: > 2015-04-26 18:44 GMT+09:00 Sagi Grimberg <sagig@dev.mellanox.co.il>: >>>> @@ -2181,6 +2182,12 @@ static inline void >>>> transport_reset_sgl_orig(struct se_cmd *cmd) >>>> >>>> static inline void transport_free_pages(struct se_cmd *cmd) >>>> { >>>> + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { >>>> + transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); >>>> + cmd->t_prot_sg = NULL; >>>> + cmd->t_prot_nents = 0; >>>> + } >>>> + >>> >>> >>> Hi Akinobu, >>> >>> Any reason why this changed it's location to the start of the function? > > Because when SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC is set, it will not > reach the tail of the function. So when SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC > is cleared and SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is set, > se_cmd->t_prot_sg leaks. I see. That's fine... > >>>> if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) { >>>> /* >>>> * Release special case READ buffer payload required for >>>> @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct >>>> se_cmd *cmd) >>>> transport_free_sgl(cmd->t_bidi_data_sg, cmd->t_bidi_data_nents); >>>> cmd->t_bidi_data_sg = NULL; >>>> cmd->t_bidi_data_nents = 0; >>>> - >>>> - transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); >>>> - cmd->t_prot_sg = NULL; >>>> - cmd->t_prot_nents = 0; >>>> } >>>> >>>> /** >>>> @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd) >>>> int ret = 0; >>>> bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB); >>>> >>>> + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { >>>> + if (cmd->prot_op != TARGET_PROT_NORMAL) { >>> >>> >>> This seems wrong, >>> >>> What will happen for transports that will actually to allocate >>> protection SGLs? The allocation is unreachable since >>> SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is not set... >> >> >> Umm, actually this is reachable... But I still think the condition >> should be the other way around (saving a condition in some common >> cases). > > Do you mean you prefer below? > > if (cmd->prot_op != TARGET_PROT_NORMAL && > !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { > ... > I think it will be better. -- 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_transport.c b/drivers/target/target_core_transport.c index 7a9e7e2..fe52883 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1450,6 +1450,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess if (sgl_prot_count) { se_cmd->t_prot_sg = sgl_prot; se_cmd->t_prot_nents = sgl_prot_count; + se_cmd->se_cmd_flags |= SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC; } /* @@ -2181,6 +2182,12 @@ static inline void transport_reset_sgl_orig(struct se_cmd *cmd) static inline void transport_free_pages(struct se_cmd *cmd) { + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { + transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); + cmd->t_prot_sg = NULL; + cmd->t_prot_nents = 0; + } + if (cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) { /* * Release special case READ buffer payload required for @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct se_cmd *cmd) transport_free_sgl(cmd->t_bidi_data_sg, cmd->t_bidi_data_nents); cmd->t_bidi_data_sg = NULL; cmd->t_bidi_data_nents = 0; - - transport_free_sgl(cmd->t_prot_sg, cmd->t_prot_nents); - cmd->t_prot_sg = NULL; - cmd->t_prot_nents = 0; } /** @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd) int ret = 0; bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB); + if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { + if (cmd->prot_op != TARGET_PROT_NORMAL) { + ret = target_alloc_sgl(&cmd->t_prot_sg, + &cmd->t_prot_nents, + cmd->prot_length, true); + if (ret < 0) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + } + + } + /* * Determine is the TCM fabric module has already allocated physical * memory, and is directly calling transport_generic_map_mem_to_cmd() @@ -2371,14 +2385,6 @@ transport_generic_new_cmd(struct se_cmd *cmd) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } - if (cmd->prot_op != TARGET_PROT_NORMAL) { - ret = target_alloc_sgl(&cmd->t_prot_sg, - &cmd->t_prot_nents, - cmd->prot_length, true); - if (ret < 0) - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - } - ret = target_alloc_sgl(&cmd->t_data_sg, &cmd->t_data_nents, cmd->data_length, zero_flag); if (ret < 0) diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 480e9f8..13efcdd 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -167,6 +167,7 @@ enum se_cmd_flags_table { SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000, SCF_COMPARE_AND_WRITE = 0x00080000, SCF_COMPARE_AND_WRITE_POST = 0x00100000, + SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000, }; /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
Even if the device backend is initialized with protection info is enabled, some requests don't have the protection info attached for WRITE SAME command issued by block device helpers, WRITE command with WRPROTECT=0 by SG_IO ioctl, etc. So when TCM loopback fabric module is used, se_cmd->t_prot_sg is NULL for these requests and performing WRITE_INSERT of PI using software emulation by sbc_dif_generate() causes kernel crash. To fix this, introduce SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC for se_cmd_flags, which is used to determine that se_cmd->t_prot_sg needs to be allocated or use pre-allocated protection information by scsi mid-layer. 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 --- * No change from v2 drivers/target/target_core_transport.c | 30 ++++++++++++++++++------------ include/target/target_core_base.h | 1 + 2 files changed, 19 insertions(+), 12 deletions(-)