[6/6] target/iscsi: Fix shutdown logic in iscsit_free_cmd()
diff mbox

Message ID 20170330171244.8346-7-bart.vanassche@sandisk.com
State New, archived
Headers show

Commit Message

Bart Van Assche March 30, 2017, 5:12 p.m. UTC
The approach in the iSCSI target driver with regard to command
reference counting is as follows:
- When a command or TMF is submitted to the LIO core,
  target_get_sess_cmd() is called with the second argument set
  to true such that cmd_kref is initialized to two.
- During regular execution of a command target_put_sess_cmd() is
  called once. However, if a connection is shut down it is not
  sure that target_put_sess_cmd() has already been called.
- If the 'shutdown' argument of iscsit_free_cmd() is true, this
  function is responsible for calling target_put_sess_cmd() if
  that has not yet happened before that function was called.

Since it is not possible to determine whether or not
target_put_sess_cmd() has already been called by examining the
CMD_T_ABORTED flag and cmd_kref only (e.g. if a LUN RESET is in
progress concurrently with iscsit_free_cmd()), use the refcount
that was introduced by the previous patch to figure out whether
or not target_put_sess_cmd() has to be called from inside
iscsit_free_cmd().

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/target/iscsi/iscsi_target_util.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Nicholas A. Bellinger April 2, 2017, 11:15 p.m. UTC | #1
On Thu, 2017-03-30 at 10:12 -0700, Bart Van Assche wrote:
> The approach in the iSCSI target driver with regard to command
> reference counting is as follows:
> - When a command or TMF is submitted to the LIO core,
>   target_get_sess_cmd() is called with the second argument set
>   to true such that cmd_kref is initialized to two.
> - During regular execution of a command target_put_sess_cmd() is
>   called once. However, if a connection is shut down it is not
>   sure that target_put_sess_cmd() has already been called.
> - If the 'shutdown' argument of iscsit_free_cmd() is true, this
>   function is responsible for calling target_put_sess_cmd() if
>   that has not yet happened before that function was called.
> 
> Since it is not possible to determine whether or not
> target_put_sess_cmd() has already been called by examining the
> CMD_T_ABORTED flag and cmd_kref only (e.g. if a LUN RESET is in
> progress concurrently with iscsit_free_cmd()), use the refcount
> that was introduced by the previous patch to figure out whether
> or not target_put_sess_cmd() has to be called from inside
> iscsit_free_cmd().
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
>  drivers/target/iscsi/iscsi_target_util.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
> index 1cacfe1003e3..04c7356e4402 100644
> --- a/drivers/target/iscsi/iscsi_target_util.c
> +++ b/drivers/target/iscsi/iscsi_target_util.c
> @@ -770,12 +770,11 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool check_queues)
>  void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown)
>  {
>  	struct se_cmd *se_cmd = cmd->se_cmd.se_tfo ? &cmd->se_cmd : NULL;
> -	int rc;
>  
>  	__iscsit_free_cmd(cmd, shutdown);
>  	if (se_cmd) {
> -		rc = transport_generic_free_cmd(&cmd->se_cmd, shutdown);
> -		if (!rc && shutdown && se_cmd && se_cmd->se_sess) {
> +		transport_generic_free_cmd(&cmd->se_cmd, shutdown);
> +		if (shutdown && atomic_read(&cmd->refcnt)) {
>  			__iscsit_free_cmd(cmd, shutdown);
>  			iscsit_put_cmd(cmd);
>  		}

Also based on incorrect assumptions about transport_generic_free_cmd().
There is no reason to add a new driver specific reference count for
handling this.

The correct bug-fix for this is:

iscsi-target: Fix TMR reference leak during session shutdown
https://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git/commit/?id=efb2ea770bb3b0f40007530bc8b0c22f36e1c5eb

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index 1cacfe1003e3..04c7356e4402 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -770,12 +770,11 @@  void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool check_queues)
 void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown)
 {
 	struct se_cmd *se_cmd = cmd->se_cmd.se_tfo ? &cmd->se_cmd : NULL;
-	int rc;
 
 	__iscsit_free_cmd(cmd, shutdown);
 	if (se_cmd) {
-		rc = transport_generic_free_cmd(&cmd->se_cmd, shutdown);
-		if (!rc && shutdown && se_cmd && se_cmd->se_sess) {
+		transport_generic_free_cmd(&cmd->se_cmd, shutdown);
+		if (shutdown && atomic_read(&cmd->refcnt)) {
 			__iscsit_free_cmd(cmd, shutdown);
 			iscsit_put_cmd(cmd);
 		}