All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bart Van Assche <Bart.VanAssche@sandisk.com>
To: "target-devel@vger.kernel.org" <target-devel@vger.kernel.org>,
	"nab@linux-iscsi.org" <nab@linux-iscsi.org>
Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"cyl@datera.io" <cyl@datera.io>, "jcs@datera.io" <jcs@datera.io>,
	"rlm@daterainc.com" <rlm@daterainc.com>
Subject: Re: [PATCH 1/2] iscsi-target: Fix TMR reference leak during session shutdown
Date: Thu, 30 Mar 2017 17:08:10 +0000	[thread overview]
Message-ID: <1490893673.2753.8.camel@sandisk.com> (raw)
In-Reply-To: <1490862594-2712-2-git-send-email-nab@linux-iscsi.org>

On Thu, 2017-03-30 at 08:29 +0000, Nicholas A. Bellinger wrote:
> diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
> index 5041a9c..b464033 100644
> --- a/drivers/target/iscsi/iscsi_target_util.c
> +++ b/drivers/target/iscsi/iscsi_target_util.c
> @@ -737,21 +737,23 @@ void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown)
>  {
>  	struct se_cmd *se_cmd = NULL;
>  	int rc;
> +	bool op_scsi = false;
>  	/*
>  	 * Determine if a struct se_cmd is associated with
>  	 * this struct iscsi_cmd.
>  	 */
>  	switch (cmd->iscsi_opcode) {
>  	case ISCSI_OP_SCSI_CMD:
> -		se_cmd = &cmd->se_cmd;
> -		__iscsit_free_cmd(cmd, true, shutdown);
> +		op_scsi = true;
>  		/*
>  		 * Fallthrough
>  		 */
>  	case ISCSI_OP_SCSI_TMFUNC:
> -		rc = transport_generic_free_cmd(&cmd->se_cmd, shutdown);
> -		if (!rc && shutdown && se_cmd && se_cmd->se_sess) {
> -			__iscsit_free_cmd(cmd, true, shutdown);
> +		se_cmd = &cmd->se_cmd;
> +		__iscsit_free_cmd(cmd, op_scsi, shutdown);
> +		rc = transport_generic_free_cmd(se_cmd, shutdown);
> +		if (!rc && shutdown && se_cmd->se_sess) {
> +			__iscsit_free_cmd(cmd, op_scsi, shutdown);
>  			target_put_sess_cmd(se_cmd);
>  		}
>  		break;

Hello Nic,

I agree that this patch fixes a leak. However, an existing bug in
iscsit_free_cmd() is not addressed by this patch. Before the TMF code started
using kref_get() / kref_put() it was possible for transport_generic_free_cmd()
to determine whether or not iscsit_free_cmd() should call __iscsit_free_cmd()
by checking the command reference count. I think that since the TMF code
manipulates the command reference count it is no longer possible for
transport_generic_free_cmd() to determine this. If iscsit_free_cmd() is called
while a LUN RESET is in progress then the return value of
transport_generic_free_cmd() will be wrong. I will post a few patches that not
only address what I just described but also the leak fixed by this patch.

Bart.

  reply	other threads:[~2017-03-30 17:08 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-30  8:29 [PATCH 0/2] target: Bug-fixes for v4.11-rc Nicholas A. Bellinger
2017-03-30  8:29 ` [PATCH 1/2] iscsi-target: Fix TMR reference leak during session shutdown Nicholas A. Bellinger
2017-03-30 17:08   ` Bart Van Assche [this message]
2017-04-02 22:38     ` Nicholas A. Bellinger
2017-03-30  8:29 ` [PATCH 2/2] target: Avoid mappedlun symlink creation during lun shutdown Nicholas A. Bellinger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1490893673.2753.8.camel@sandisk.com \
    --to=bart.vanassche@sandisk.com \
    --cc=cyl@datera.io \
    --cc=jcs@datera.io \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=nab@linux-iscsi.org \
    --cc=rlm@daterainc.com \
    --cc=target-devel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.