Re: [PATCH v3 1/5] target: ensure se_cmd-t_prot_sg is allocated when required

2015-04-27 Thread Akinobu Mita 
2015-04-26 18:44 GMT+09:00 Sagi Grimberg sa...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] target: ensure se_cmd-t_prot_sg is allocated when required

2015-04-27 Thread Sagi Grimberg 

On 4/27/2015 3:57 PM, Akinobu Mita wrote:

2015-04-26 18:44 GMT+09:00 Sagi Grimberg sa...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] target: ensure se_cmd-t_prot_sg is allocated when required

2015-04-26 Thread Sagi Grimberg 

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.m...@gmail.com
Cc: Nicholas Bellinger n...@linux-iscsi.org
Cc: Sagi Grimberg sa...@mellanox.com
Cc: Martin K. Petersen martin.peter...@oracle.com
Cc: Christoph Hellwig h...@lst.de
Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com
Cc: target-de...@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 = 0x0002,
SCF_COMPARE_AND_WRITE   = 0x0008,