On 12/08/2015 01:48 AM, Himanshu Madhani wrote:
> From: Quinn Tran <quinn.t...@qlogic.com>
> 
> During LUN/Target reset, the TMR code attempt to intercept
> cmds and try to aborted them.  Current code assume cmds are
> always intercepted at the back end device.  The cleanup code
> would issue a "queue_status() & check_stop_free()" to terminate
> the command.  However, when a cmd is intercepted at the front
> end/Fabric layer, current code introduce premature free or
> cause Fabric to double free.
> 
> When command is intercepted at Fabric layer, it means a
> check_stop_free(cmd_kref--) has been called.  The extra
> check_stop_free in the Lun Reset cleanup code causes early
> free.  When a cmd in the Fabric layer is completed, the normal
> free code adds another another free which introduce a double free.
> 
> To fix the issue:
> - add a new flag/CMD_T_SENT_STATUS to track command that have
>  made it down to fabric layer after back end good/bad completion.
> - if cmd reach Fabric Layer at Lun Reset time, add an extra
>  cmd_kref count to prevent premature free.
> 
> Signed-off-by: Quinn Tran <quinn.t...@qlogic.com>
> Signed-off-by: Himanshu Madhani <himanshu.madh...@qlogic.com>
> ---
>  drivers/target/target_core_tmr.c       |   33 
> +++++++++++++++++++++++++++++++-
>  drivers/target/target_core_transport.c |   30 +++++++++++++++++++++++++++++
>  include/target/target_core_base.h      |    1 +
>  3 files changed, 63 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/target/target_core_tmr.c 
> b/drivers/target/target_core_tmr.c
> index 28fb301..41f8b57 100644
> --- a/drivers/target/target_core_tmr.c
> +++ b/drivers/target/target_core_tmr.c
> @@ -243,7 +243,9 @@ static void core_tmr_drain_state_list(
>  {
>       LIST_HEAD(drain_task_list);
>       struct se_cmd *cmd, *next;
> -     unsigned long flags;
> +     unsigned long flags, flags2;
> +     int rmkref;
> +     struct se_session *se_sess;
>  
>       /*
>        * Complete outstanding commands with TASK_ABORTED SAM status.
> @@ -282,6 +284,16 @@ static void core_tmr_drain_state_list(
>               if (prout_cmd == cmd)
>                       continue;
>  
> +             se_sess = cmd->se_sess;
> +             /* take an extra kref to prevent cmd free race condition. */
> +             spin_lock_irqsave(&se_sess->sess_cmd_lock, flags2);
> +             if (!kref_get_unless_zero(&cmd->cmd_kref)) {
> +                     /* cmd is already in the free process */
> +                     spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags2);
> +                     continue;
> +             }
> +             spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags2);
> +
>               list_move_tail(&cmd->state_list, &drain_task_list);
>               cmd->state_active = false;
>       }
> @@ -320,9 +332,28 @@ static void core_tmr_drain_state_list(
>               target_stop_cmd(cmd, &flags);
>  
>               cmd->transport_state |= CMD_T_ABORTED;
> +
> +             /* CMD_T_SENT_STATUS: cmd is down in fabric layer.
> +              * A check stop has been called.  Keep the extra kref
> +              * from above because core_tmr_handle_tas_abort will
> +              * generate another check_stop.
> +              *
> +              * !CMD_T_SENT_STATUS: cmd intercepted at back end.
> +              * Remove the extra kref from above because only
> +              * 1 check_stop is required or generated by
> +              * core_tmr_handle_tas_abort()
> +              */
> +             rmkref = 0;
> +             if (!((cmd->t_state == TRANSPORT_COMPLETE) &&
> +                 (cmd->transport_state & CMD_T_SENT_STATUS)))
> +                     rmkref = 1;
> +
>               spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  
>               core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
> +
> +             if (rmkref)
> +                     target_put_sess_cmd(cmd);
>       }
>  }
>  
> diff --git a/drivers/target/target_core_transport.c 
> b/drivers/target/target_core_transport.c
> index 4fdcee2..cdd18bf 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -639,9 +639,14 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int 
> remove)
>  static void target_complete_failure_work(struct work_struct *work)
>  {
>       struct se_cmd *cmd = container_of(work, struct se_cmd, work);
> +     unsigned long flags;
>  
>       transport_generic_request_failure(cmd,
>                       TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE);
> +
> +     spin_lock_irqsave(&cmd->t_state_lock, flags);
> +     cmd->transport_state |= CMD_T_SENT_STATUS;
> +     spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  }
>  
>  /*
> @@ -1659,6 +1664,7 @@ void transport_generic_request_failure(struct se_cmd 
> *cmd,
>               sense_reason_t sense_reason)
>  {
>       int ret = 0, post_ret = 0;
> +     unsigned long flags;
>  
>       pr_debug("-----[ Storage Engine Exception for cmd: %p ITT: 0x%08llx"
>               " CDB: 0x%02x\n", cmd, cmd->tag, cmd->t_task_cdb[0]);
> @@ -1670,6 +1676,10 @@ void transport_generic_request_failure(struct se_cmd 
> *cmd,
>               (cmd->transport_state & CMD_T_STOP) != 0,
>               (cmd->transport_state & CMD_T_SENT) != 0);
>  
> +     spin_lock_irqsave(&cmd->t_state_lock, flags);
> +     cmd->transport_state |= CMD_T_SENT_STATUS;
> +     spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> +
>       /*
>        * For SAM Task Attribute emulation for failed struct se_cmd
>        */
> @@ -1951,6 +1961,7 @@ static void transport_complete_task_attr(struct se_cmd 
> *cmd)
>  static void transport_complete_qf(struct se_cmd *cmd)
>  {
>       int ret = 0;
> +     unsigned long flags;
>  
>       transport_complete_task_attr(cmd);
>  
> @@ -1986,6 +1997,10 @@ out:
>       }
>       transport_lun_remove_cmd(cmd);
>       transport_cmd_check_stop_to_fabric(cmd);
> +
> +     spin_lock_irqsave(&cmd->t_state_lock, flags);
> +     cmd->transport_state |= CMD_T_SENT_STATUS;
> +     spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>  }
>  
>  static void transport_handle_queue_full(
> @@ -2032,6 +2047,7 @@ static void target_complete_ok_work(struct work_struct 
> *work)
>  {
>       struct se_cmd *cmd = container_of(work, struct se_cmd, work);
>       int ret;
> +     unsigned long flags;
>  
>       /*
>        * Check if we need to move delayed/dormant tasks from cmds on the
> @@ -2060,6 +2076,10 @@ static void target_complete_ok_work(struct work_struct 
> *work)
>  
>               transport_lun_remove_cmd(cmd);
>               transport_cmd_check_stop_to_fabric(cmd);
> +
> +             spin_lock_irqsave(&cmd->t_state_lock, flags);
> +             cmd->transport_state |= CMD_T_SENT_STATUS;
> +             spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>               return;
>       }
>       /*
> @@ -2086,6 +2106,11 @@ static void target_complete_ok_work(struct work_struct 
> *work)
>  
>                       transport_lun_remove_cmd(cmd);
>                       transport_cmd_check_stop_to_fabric(cmd);
> +
> +                     spin_lock_irqsave(&cmd->t_state_lock, flags);
> +                     cmd->transport_state |= CMD_T_SENT_STATUS;
> +                     spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> +
>                       return;
>               }
>       }
> @@ -2136,6 +2161,7 @@ queue_rsp:
>               ret = cmd->se_tfo->queue_status(cmd);
>               if (ret == -EAGAIN || ret == -ENOMEM)
>                       goto queue_full;
> +
>               break;
>       default:
>               break;
> @@ -2143,6 +2169,10 @@ queue_rsp:
>  
>       transport_lun_remove_cmd(cmd);
>       transport_cmd_check_stop_to_fabric(cmd);
> +
> +     spin_lock_irqsave(&cmd->t_state_lock, flags);
> +     cmd->transport_state |= CMD_T_SENT_STATUS;
> +     spin_unlock_irqrestore(&cmd->t_state_lock, flags);
>       return;
>  
>  queue_full:
> diff --git a/include/target/target_core_base.h 
> b/include/target/target_core_base.h
> index aabf0ac..efccd71 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -490,6 +490,7 @@ struct se_cmd {
>  #define CMD_T_DEV_ACTIVE     (1 << 7)
>  #define CMD_T_REQUEST_STOP   (1 << 8)
>  #define CMD_T_BUSY           (1 << 9)
> +#define CMD_T_SENT_STATUS (1 << 10)
>       spinlock_t              t_state_lock;
>       struct kref             cmd_kref;
>       struct completion       t_transport_stop_comp;
> 
Same here: using bitops would save you taking the spinlock when
modifying flags.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                   zSeries & Storage
h...@suse.de                          +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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

Reply via email to