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.
