| Message ID | 20170208222507.25715-15-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 02/08/2017 11:24 PM, Bart Van Assche wrote: > Use the se_cmd reference counting mechanism instead of calling the > .release_cmd() method directly. Fix a reference leak triggered by > transport_cmd_check_stop_to_fabric() that did not matter until > now because of the direct .release_cmd() calls. If CMD_T_STOP is > set for a command, transport_cmd_finish_abort() namely calls > transport_cmd_check_stop_to_fabric() and the latter function > returns 1 although the command refcount is not yet zero. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Andy Grover <agrover@redhat.com> > Cc: David Disseldorp <ddiss@suse.de> > --- > drivers/target/target_core_transport.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
On Wed, 2017-02-08 at 14:24 -0800, Bart Van Assche wrote: > Use the se_cmd reference counting mechanism instead of calling the > .release_cmd() method directly. Fix a reference leak triggered by > transport_cmd_check_stop_to_fabric() that did not matter until > now because of the direct .release_cmd() calls. If CMD_T_STOP is > set for a command, transport_cmd_finish_abort() namely calls > transport_cmd_check_stop_to_fabric() and the latter function > returns 1 although the command refcount is not yet zero. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Andy Grover <agrover@redhat.com> > Cc: David Disseldorp <ddiss@suse.de> > --- > drivers/target/target_core_transport.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 1e1f1ed2ef17..7b5ae36e3c58 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -622,13 +622,11 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists, > pr_debug("%s:%d CMD_T_STOP for ITT: 0x%08llx\n", > __func__, __LINE__, cmd->tag); > > - spin_unlock_irqrestore(&cmd->t_state_lock, flags); > - > complete_all(&cmd->t_transport_stop_comp); > - return 1; > + } else { > + cmd->transport_state &= ~CMD_T_ACTIVE; > } > This is wrong. __transport_wait_for_tasks() logic that sets CMD_T_STOP clears CMD_T_ACTIVE after se_cmd->t_transport_stop_comp has completed. When that happens, transport_cmd_check_stop() must exit and not invoke se_cmd->se_tfo->check_stop_free() to drop se_cmd->cmd_kref, because all of the existing code beyond __transport_wait_for_tasks() expects to drop the reference if CMD_T_STOP was set, and se_cmd was quiesced. > - cmd->transport_state &= ~CMD_T_ACTIVE; > if (remove_from_lists) { > /* > * Some fabric modules like tcm_loop can release > @@ -2532,7 +2530,7 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks) > if (aborted) { > pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag); > wait_for_completion(&cmd->cmd_wait_comp); > - cmd->se_tfo->release_cmd(cmd); > + transport_put_cmd(cmd); > ret = 1; > } > return ret; This is broken. The final kref_put of se_cmd->cmd_kref in transport_put_cmd() is what wakes up se_cmd->cmd_wait_comp, and allows forward process to continue. This can't possibly work. > @@ -2695,7 +2693,8 @@ void target_wait_for_sess_cmds(struct se_session *se_sess) > " fabric state: %d\n", se_cmd, se_cmd->t_state, > se_cmd->se_tfo->get_cmd_state(se_cmd)); > > - se_cmd->se_tfo->release_cmd(se_cmd); > + WARN_ON_ONCE(!se_cmd->se_sess); > + target_put_sess_cmd(se_cmd); > } Right above the printk here is another wait_for_completion(&cmd->cmd_wait_comp), which is only woken up when target_put_sess_cmd() -> kref_put() for se_cmd->cmd_kref reaches zero. This can't possibly work either. -- 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
On Thu, 2017-02-09 at 01:51 -0800, Nicholas A. Bellinger wrote: > This is wrong. __transport_wait_for_tasks() logic that sets CMD_T_STOP > clears CMD_T_ACTIVE after se_cmd->t_transport_stop_comp has completed. I will drop that change. > @@ -2532,7 +2530,7 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks) > > if (aborted) { > > pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag); > > wait_for_completion(&cmd->cmd_wait_comp); > > - cmd->se_tfo->release_cmd(cmd); > > + transport_put_cmd(cmd); > > ret = 1; > > } > > return ret; > > This is broken. > > The final kref_put of se_cmd->cmd_kref in transport_put_cmd() is what > wakes up se_cmd->cmd_wait_comp, and allows forward process to continue. Have you noticed that a later patch removes cmd_wait_comp? Anyway, I will fold the removal of cmd_wait_comp into this patch. Bart.-- 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
On Thu, 2017-02-09 at 18:58 +0000, Bart Van Assche wrote: > On Thu, 2017-02-09 at 01:51 -0800, Nicholas A. Bellinger wrote: > > This is wrong. __transport_wait_for_tasks() logic that sets CMD_T_STOP > > clears CMD_T_ACTIVE after se_cmd->t_transport_stop_comp has completed. > > I will drop that change. > > > @@ -2532,7 +2530,7 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks) > > > if (aborted) { > > > pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag); > > > wait_for_completion(&cmd->cmd_wait_comp); > > > - cmd->se_tfo->release_cmd(cmd); > > > + transport_put_cmd(cmd); > > > ret = 1; > > > } > > > return ret; > > > > This is broken. > > > > The final kref_put of se_cmd->cmd_kref in transport_put_cmd() is what > > wakes up se_cmd->cmd_wait_comp, and allows forward process to continue. > > Have you noticed that a later patch removes cmd_wait_comp? Anyway, I will > fold the removal of cmd_wait_comp into this patch. This patch breaks everything as-is. If the end result it's not the case, then the ordering of this series is totally wrong. -- 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
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 1e1f1ed2ef17..7b5ae36e3c58 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -622,13 +622,11 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists, pr_debug("%s:%d CMD_T_STOP for ITT: 0x%08llx\n", __func__, __LINE__, cmd->tag); - spin_unlock_irqrestore(&cmd->t_state_lock, flags); - complete_all(&cmd->t_transport_stop_comp); - return 1; + } else { + cmd->transport_state &= ~CMD_T_ACTIVE; } - cmd->transport_state &= ~CMD_T_ACTIVE; if (remove_from_lists) { /* * Some fabric modules like tcm_loop can release @@ -2532,7 +2530,7 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks) if (aborted) { pr_debug("Detected CMD_T_ABORTED for ITT: %llu\n", cmd->tag); wait_for_completion(&cmd->cmd_wait_comp); - cmd->se_tfo->release_cmd(cmd); + transport_put_cmd(cmd); ret = 1; } return ret; @@ -2695,7 +2693,8 @@ void target_wait_for_sess_cmds(struct se_session *se_sess) " fabric state: %d\n", se_cmd, se_cmd->t_state, se_cmd->se_tfo->get_cmd_state(se_cmd)); - se_cmd->se_tfo->release_cmd(se_cmd); + WARN_ON_ONCE(!se_cmd->se_sess); + target_put_sess_cmd(se_cmd); } spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
Use the se_cmd reference counting mechanism instead of calling the .release_cmd() method directly. Fix a reference leak triggered by transport_cmd_check_stop_to_fabric() that did not matter until now because of the direct .release_cmd() calls. If CMD_T_STOP is set for a command, transport_cmd_finish_abort() namely calls transport_cmd_check_stop_to_fabric() and the latter function returns 1 although the command refcount is not yet zero. Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Andy Grover <agrover@redhat.com> Cc: David Disseldorp <ddiss@suse.de> --- drivers/target/target_core_transport.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)