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@xxxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Andy Grover <agrover@xxxxxxxxxx>
> Cc: David Disseldorp <ddiss@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
