From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sagi Grimberg Subject: Re: [PATCH v3 1/5] target: ensure se_cmd->t_prot_sg is allocated when required Date: Sun, 26 Apr 2015 12:26:10 +0300 Message-ID: <553CAF32.2030904@dev.mellanox.co.il> References: <1429972410-7146-1-git-send-email-akinobu.mita@gmail.com> <1429972410-7146-2-git-send-email-akinobu.mita@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1429972410-7146-2-git-send-email-akinobu.mita@gmail.com> Sender: target-devel-owner@vger.kernel.org To: Akinobu Mita , target-devel@vger.kernel.org Cc: Nicholas Bellinger , Sagi Grimberg , "Martin K. Petersen" , Christoph Hellwig , "James E.J. Bottomley" , linux-scsi@vger.kernel.org List-Id: linux-scsi@vger.kernel.org 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 > Cc: Nicholas Bellinger > Cc: Sagi Grimberg > Cc: "Martin K. Petersen" > Cc: Christoph Hellwig > Cc: "James E.J. Bottomley" > 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 */ >