Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751403AbdDBWi1 (ORCPT ); Sun, 2 Apr 2017 18:38:27 -0400 Received: from mail.linux-iscsi.org ([67.23.28.174]:58614 "EHLO linux-iscsi.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750983AbdDBWiZ (ORCPT ); Sun, 2 Apr 2017 18:38:25 -0400 Message-ID: <1491172703.8846.43.camel@haakon3.risingtidesystems.com> Subject: Re: [PATCH 1/2] iscsi-target: Fix TMR reference leak during session shutdown From: "Nicholas A. Bellinger" To: Bart Van Assche Cc: "target-devel@vger.kernel.org" , "linux-scsi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "cyl@datera.io" , "jcs@datera.io" , "rlm@daterainc.com" Date: Sun, 02 Apr 2017 15:38:23 -0700 In-Reply-To: <1490893673.2753.8.camel@sandisk.com> References: <1490862594-2712-1-git-send-email-nab@linux-iscsi.org> <1490862594-2712-2-git-send-email-nab@linux-iscsi.org> <1490893673.2753.8.camel@sandisk.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.4.4-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2913 Lines: 65 On Thu, 2017-03-30 at 17:08 +0000, Bart Van Assche wrote: > 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. No. Your assumption is incorrect wrt transport_generic_free_cmd() having a wrong return value during LUN_RESET. It's incorrect because when iscsit_free_cmd() is called with shutdown=true resulting in transport_generic_free_cmd() with wait_for_tasks=true, target_wait_free_cmd() checks for CMD_T_ABORTED to determine if se_cmd has been aborted by target_core_tmr.c logic, and returns aborted=true back up to transport_generic_free_cmd(). When transport_generic_free_cmd() gets aborted=true, it waits for se_cmd->cmd_wait_comp to finish and calls cmd->se_tfo->release_cmd() to release se_cmd back to the pre-allocated session pool. At this point, transport_generic_free_cmd() with aborted=true must return '1' it's caller, signaling iscsit_free_cmd() to not attempt to perform the extra __iscsi_free_cmd() or target_put_sess_cmd(), because se_cmd has already been released back to the session pool.