| Message ID | 20170202005853.23456-18-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Feb 01, 2017 at 04:58:34PM -0800, Bart Van Assche wrote: > Avoid that transport_cmd_check_stop_to_fabric() leaks a command > reference if CMD_T_STOP has been set. Are you sure we're actually leaking here? If I remember the code correctly it should get cleaned up later in a convoluted call stack, including transport_cmd_finish_abort and ->aborted_task. -- 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 Mon, 2017-02-06 at 01:22 -0800, Christoph Hellwig wrote: > On Wed, Feb 01, 2017 at 04:58:34PM -0800, Bart Van Assche wrote: > > Avoid that transport_cmd_check_stop_to_fabric() leaks a command > > reference if CMD_T_STOP has been set. > > Are you sure we're actually leaking here? If I remember the code > correctly it should get cleaned up later in a convoluted call stack, > including transport_cmd_finish_abort and ->aborted_task. Hello Christoph, transport_cmd_finish_abort() skips transport_put_cmd() if transport_cmd_check_stop_to_fabric() returns 1 because CMD_T_STOP is set. The only function in kernel v4.9 that sets CMD_T_STOP is __transport_wait_for_tasks(). That function doesn't drop the cmd refcount so I think this patch really fixes a reference leak. The reason why this has not yet been noticed is probably because in kernel v4.9 a few functions bypass the reference counting mechanism. E.g. target_wait_for_sess_cmds() and transport_generic_free_cmd() call .release_cmd() directly instead of calling kref_put(). 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 Mon, Feb 06, 2017 at 05:21:09PM +0000, Bart Van Assche wrote: > Hello Christoph, > > transport_cmd_finish_abort() skips transport_put_cmd() if > transport_cmd_check_stop_to_fabric() returns 1 because CMD_T_STOP is > set. The only function in kernel v4.9 that sets CMD_T_STOP is > __transport_wait_for_tasks(). That function doesn't drop the cmd > refcount so I think this patch really fixes a reference leak. > > The reason why this has not yet been noticed is probably because in > kernel v4.9 a few functions bypass the reference counting mechanism. > E.g. target_wait_for_sess_cmds() and transport_generic_free_cmd() call > .release_cmd() directly instead of calling kref_put(). So it fixes the missing drop of the reference, but probably not a leak of the actual memoery because of the latter. I really don't like the way this has been handled before, so I welcome your patch in principle. I just need to review it very carefully as this area is rather hairy, and good comments help with that. -- 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 Wed, 2017-02-01 at 16:58 -0800, Bart Van Assche wrote: > Avoid that transport_cmd_check_stop_to_fabric() leaks a command > reference if CMD_T_STOP has been set. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Reviewed-by: 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 | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index d241c4d27352..25bc214a4eee 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -628,13 +628,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 Separate from this patch series, this not a bug. Note the existing callers of __transport_wait_for_tasks() that does wait_for_completion(&cmd->t_transport_stop_comp) currently expects to drop any outstanding se_cmd->cmd_kref from it's own calling context, once CMD_T_STOP has been intercepted from this completion callback, and cleared in __transport_wait_for_tasks(). It works the same way above in transport_cmd_check_stop(), as well as CMD_T_STOP checks in target_complete_cmd() and target_execute_cmd(). -- 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 Tue, 2017-02-07 at 06:25 -0800, Nicholas A. Bellinger wrote: > On Wed, 2017-02-01 at 16:58 -0800, Bart Van Assche wrote: > > Avoid that transport_cmd_check_stop_to_fabric() leaks a command > > reference if CMD_T_STOP has been set. > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > Reviewed-by: 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 | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > > index d241c4d27352..25bc214a4eee 100644 > > --- a/drivers/target/target_core_transport.c > > +++ b/drivers/target/target_core_transport.c > > @@ -628,13 +628,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 > > Separate from this patch series, this not a bug. A later patch changes direct calls to .release_cmd() into kref_put() calls so this change is needed. 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
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index d241c4d27352..25bc214a4eee 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -628,13 +628,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