[PATCH 4/7] target: Allow for target_submit_cmd() returning errors

2012-07-16 Thread Roland Dreier 
From: Roland Dreier rol...@purestorage.com

We want it to be possible for target_submit_cmd() to return errors up
to its fabric module callers.  For now just update the prototype to
return an int, and update all callers to handle non-zero return values
as an error.

Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Arun Easi arun.e...@qlogic.com
Cc: Chris Boot bo...@bootc.net
Cc: Stefan Richter stef...@s5r6.in-berlin.de
Cc: Mark Rustad mark.d.rus...@intel.com
Cc: Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
Cc: Felipe Balbi ba...@ti.com
Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |7 +++
 drivers/target/sbp/sbp_target.c|8 +---
 drivers/target/target_core_transport.c |9 +
 drivers/target/tcm_fc/tfc_cmd.c|8 +---
 drivers/usb/gadget/tcm_usb_gadget.c|   20 
 include/target/target_core_fabric.h|2 +-
 6 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 3cb2b97..7db7ca7 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -599,10 +599,9 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, 
struct qla_tgt_cmd *cmd,
return -EINVAL;
}
 
-   target_submit_cmd(se_cmd, se_sess, cdb, cmd-sense_buffer[0],
-   cmd-unpacked_lun, data_length, fcp_task_attr,
-   data_dir, flags);
-   return 0;
+   return target_submit_cmd(se_cmd, se_sess, cdb, cmd-sense_buffer[0],
+cmd-unpacked_lun, data_length, fcp_task_attr,
+data_dir, flags);
 }
 
 static void tcm_qla2xxx_do_rsp(struct work_struct *work)
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 2425ca4..fb75d05 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -1235,9 +1235,11 @@ static void sbp_handle_command(struct sbp_target_request 
*req)
pr_debug(sbp_handle_command ORB:0x%llx unpacked_lun:%d data_len:%d 
data_dir:%d\n,
req-orb_pointer, unpacked_lun, data_length, data_dir);
 
-   target_submit_cmd(req-se_cmd, sess-se_sess, req-cmd_buf,
-   req-sense_buf, unpacked_lun, data_length,
-   MSG_SIMPLE_TAG, data_dir, 0);
+   if (target_submit_cmd(req-se_cmd, sess-se_sess, req-cmd_buf,
+ req-sense_buf, unpacked_lun, data_length,
+ MSG_SIMPLE_TAG, data_dir, 0))
+   goto err;
+
return;
 
 err:
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 82e40e1..34ca821 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1544,7 +1544,7 @@ EXPORT_SYMBOL(transport_handle_cdb_direct);
  * This may only be called from process context, and also currently
  * assumes internal allocation of fabric payload buffer by target-core.
  **/
-void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
+int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
unsigned char *cdb, unsigned char *sense, u32 unpacked_lun,
u32 data_length, int task_attr, int data_dir, int flags)
 {
@@ -1583,7 +1583,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct 
se_session *se_sess,
transport_send_check_condition_and_sense(se_cmd,
se_cmd-scsi_sense_reason, 0);
target_put_sess_cmd(se_sess, se_cmd);
-   return;
+   return 0;
}
/*
 * Sanitize CDBs via transport_generic_cmd_sequencer() and
@@ -1592,7 +1592,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct 
se_session *se_sess,
rc = target_setup_cmd_from_cdb(se_cmd, cdb);
if (rc != 0) {
transport_generic_request_failure(se_cmd);
-   return;
+   return 0;
}
 
/*
@@ -1608,7 +1608,8 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct 
se_session *se_sess,
 * when fabric has filled the incoming buffer.
 */
transport_handle_cdb_direct(se_cmd);
-   return;
+
+   return 0;
 }
 EXPORT_SYMBOL(target_submit_cmd);
 
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index f03fb97..c7c90aa 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -541,9 +541,11 @@ static void ft_send_work(struct work_struct *work)
 * Use a single se_cmd-cmd_kref as we expect to release se_cmd
 * directly from ft_check_stop_free callback in response path.
 */
-   target_submit_cmd(cmd-se_cmd, cmd-sess-se_sess, fcp-fc_cdb,
-   cmd-ft_sense_buffer[0],

Re: [PATCH 4/7] target: Allow for target_submit_cmd() returning errors

2012-07-16 Thread Nicholas A. Bellinger 
On Mon, 2012-07-16 at 11:04 -0700, Roland Dreier wrote:
 From: Roland Dreier rol...@purestorage.com
 
 We want it to be possible for target_submit_cmd() to return errors up
 to its fabric module callers.  For now just update the prototype to
 return an int, and update all callers to handle non-zero return values
 as an error.
 

Mmmm.  The original target_submit_cmd() code had been propagating up a
return value, but then we decided (via Agrover's patch) that it made
more sense for target_submit_cmd() to always handle exceptions via
normal TFO callbacks, and not have the fabric worry about the return
here..

Also, I'm not sure if the error paths that this patch now accesses after
target_submit_cmd() failure are going to deal with different types of
target_submit_cmd() failures correctly.

So NACK for the moment, as I don't really see why this is necessary in
the first place..?

 Cc: Chad Dupuis chad.dup...@qlogic.com
 Cc: Arun Easi arun.e...@qlogic.com
 Cc: Chris Boot bo...@bootc.net
 Cc: Stefan Richter stef...@s5r6.in-berlin.de
 Cc: Mark Rustad mark.d.rus...@intel.com
 Cc: Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
 Cc: Felipe Balbi ba...@ti.com
 Signed-off-by: Roland Dreier rol...@purestorage.com
 ---
  drivers/scsi/qla2xxx/tcm_qla2xxx.c |7 +++
  drivers/target/sbp/sbp_target.c|8 +---
  drivers/target/target_core_transport.c |9 +
  drivers/target/tcm_fc/tfc_cmd.c|8 +---
  drivers/usb/gadget/tcm_usb_gadget.c|   20 
  include/target/target_core_fabric.h|2 +-
  6 files changed, 27 insertions(+), 27 deletions(-)
 
 diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
 b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
 index 3cb2b97..7db7ca7 100644
 --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
 +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
 @@ -599,10 +599,9 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, 
 struct qla_tgt_cmd *cmd,
   return -EINVAL;
   }
  
 - target_submit_cmd(se_cmd, se_sess, cdb, cmd-sense_buffer[0],
 - cmd-unpacked_lun, data_length, fcp_task_attr,
 - data_dir, flags);
 - return 0;
 + return target_submit_cmd(se_cmd, se_sess, cdb, cmd-sense_buffer[0],
 +  cmd-unpacked_lun, data_length, fcp_task_attr,
 +  data_dir, flags);
  }
  
  static void tcm_qla2xxx_do_rsp(struct work_struct *work)
 diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
 index 2425ca4..fb75d05 100644
 --- a/drivers/target/sbp/sbp_target.c
 +++ b/drivers/target/sbp/sbp_target.c
 @@ -1235,9 +1235,11 @@ static void sbp_handle_command(struct 
 sbp_target_request *req)
   pr_debug(sbp_handle_command ORB:0x%llx unpacked_lun:%d data_len:%d 
 data_dir:%d\n,
   req-orb_pointer, unpacked_lun, data_length, data_dir);
  
 - target_submit_cmd(req-se_cmd, sess-se_sess, req-cmd_buf,
 - req-sense_buf, unpacked_lun, data_length,
 - MSG_SIMPLE_TAG, data_dir, 0);
 + if (target_submit_cmd(req-se_cmd, sess-se_sess, req-cmd_buf,
 +   req-sense_buf, unpacked_lun, data_length,
 +   MSG_SIMPLE_TAG, data_dir, 0))
 + goto err;
 +
   return;
  
  err:
 diff --git a/drivers/target/target_core_transport.c 
 b/drivers/target/target_core_transport.c
 index 82e40e1..34ca821 100644
 --- a/drivers/target/target_core_transport.c
 +++ b/drivers/target/target_core_transport.c
 @@ -1544,7 +1544,7 @@ EXPORT_SYMBOL(transport_handle_cdb_direct);
   * This may only be called from process context, and also currently
   * assumes internal allocation of fabric payload buffer by target-core.
   **/
 -void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 +int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
   unsigned char *cdb, unsigned char *sense, u32 unpacked_lun,
   u32 data_length, int task_attr, int data_dir, int flags)
  {
 @@ -1583,7 +1583,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct 
 se_session *se_sess,
   transport_send_check_condition_and_sense(se_cmd,
   se_cmd-scsi_sense_reason, 0);
   target_put_sess_cmd(se_sess, se_cmd);
 - return;
 + return 0;
   }
   /*
* Sanitize CDBs via transport_generic_cmd_sequencer() and
 @@ -1592,7 +1592,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct 
 se_session *se_sess,
   rc = target_setup_cmd_from_cdb(se_cmd, cdb);
   if (rc != 0) {
   transport_generic_request_failure(se_cmd);
 - return;
 + return 0;
   }
  
   /*
 @@ -1608,7 +1608,8 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct 
 se_session *se_sess,
* when fabric has filled the incoming buffer.
*/

Re: [PATCH 4/7] target: Allow for target_submit_cmd() returning errors

2012-07-16 Thread Roland Dreier 
On Mon, Jul 16, 2012 at 4:00 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 Mmmm.  The original target_submit_cmd() code had been propagating up a
 return value, but then we decided (via Agrover's patch) that it made
 more sense for target_submit_cmd() to always handle exceptions via
 normal TFO callbacks, and not have the fabric worry about the return
 here..

 Also, I'm not sure if the error paths that this patch now accesses after
 target_submit_cmd() failure are going to deal with different types of
 target_submit_cmd() failures correctly.

 So NACK for the moment, as I don't really see why this is necessary in
 the first place..?

Read on in the series to see why this is needed; in short, for qla2xxx
at least, we need a race-free way to check for sess_tearing_down
atomically with actually adding the command to sess_cmd_list.

I'm OK with returning failure via callback, but

 a) I'm not sure we can use the normal TFO callbacks in case
we can't add the command to sess_cmd_list (seems like it
opens the door to other use-after-frees in qla2xxx at least)
 b) Maybe it's OK if we say that failure to add the command to
the sess_cmd_list is the only time submit cmd fails?

The qla2xxx race/use-after-free is definitely real, we hit it in testing
here with active IO across ACL changes.

 - R.
--
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