Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] nvme-fc: fix double-free scenarios on hw queues
@ 2019-11-21 17:59 James Smart
  2019-11-21 19:31 ` Ewan D. Milne
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: James Smart @ 2019-11-21 17:59 UTC (permalink / raw)
  To: linux-nvme; +Cc: James Smart

If an error occurs on one of the ios used for creating an
association, the creating routine has error paths that are
invoked by the command failure and the error paths will free
up the controller resources created to that point.

But... the io was ultimately determined by an asynchronous
completion routine that detected the error and which
unconditionally invokes the error_recovery path which calls
delete_association. Delete association deletes all outstanding
io then tears down the controller resources. So the
create_association thread can be running in parallel with
the error_recovery thread. What was seen was the LLDD received
a call to delete a queue, causing the LLDD to do a free of a
resource, then the transport called the delete queue again
causing the driver to repeat the free call. The second free
routine corrupted the allocator. The transport shouldn't be
making the duplicate call, and the delete queue is just one
of the resources being freed.

To fix, it is realized that the create_association path is
completely serialized with one command at a time. So the
failed io completion will always be seen by the create_association
path and as of the failure, there are no ios to terminate and there
is no reason to be manipulating queue freeze states, etc.
The serialized condition stays true until the controller is
transitioned to the LIVE state. Thus the fix is to change the
error recovery path to check the controller state and only
invoke the teardown path if not already in the CONNECTING state.

Signed-off-by: James Smart <jsmart2021@gmail.com>
---
 drivers/nvme/host/fc.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 679a721ae229..2acb850bf9f4 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2910,10 +2910,22 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
 static void
 __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl)
 {
-	nvme_stop_keep_alive(&ctrl->ctrl);
+	/*
+	 * if state is connecting - the error occurred as part of a
+	 * reconnect attempt. The create_association error paths will
+	 * clean up any outstanding io.
+	 *
+	 * if it's a different state - ensure all pending io is
+	 * terminated. Given this can delay while waiting for the
+	 * aborted io to return, we recheck adapter state below
+	 * before changing state.
+	 */
+	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
+		nvme_stop_keep_alive(&ctrl->ctrl);
 
-	/* will block will waiting for io to terminate */
-	nvme_fc_delete_association(ctrl);
+		/* will block will waiting for io to terminate */
+		nvme_fc_delete_association(ctrl);
+	}
 
 	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING &&
 	    !nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
-- 
2.13.7


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] nvme-fc: fix double-free scenarios on hw queues
  2019-11-21 17:59 [PATCH] nvme-fc: fix double-free scenarios on hw queues James Smart
@ 2019-11-21 19:31 ` Ewan D. Milne
  2019-11-22 17:01 ` Himanshu Madhani
  2019-11-26 18:01 ` Keith Busch
  2 siblings, 0 replies; 4+ messages in thread
From: Ewan D. Milne @ 2019-11-21 19:31 UTC (permalink / raw)
  To: linux-nvme

On Thu, 2019-11-21 at 09:59 -0800, James Smart wrote:
> If an error occurs on one of the ios used for creating an
> association, the creating routine has error paths that are
> invoked by the command failure and the error paths will free
> up the controller resources created to that point.
> 
> But... the io was ultimately determined by an asynchronous
> completion routine that detected the error and which
> unconditionally invokes the error_recovery path which calls
> delete_association. Delete association deletes all outstanding
> io then tears down the controller resources. So the
> create_association thread can be running in parallel with
> the error_recovery thread. What was seen was the LLDD received
> a call to delete a queue, causing the LLDD to do a free of a
> resource, then the transport called the delete queue again
> causing the driver to repeat the free call. The second free
> routine corrupted the allocator. The transport shouldn't be
> making the duplicate call, and the delete queue is just one
> of the resources being freed.
> 
> To fix, it is realized that the create_association path is
> completely serialized with one command at a time. So the
> failed io completion will always be seen by the create_association
> path and as of the failure, there are no ios to terminate and there
> is no reason to be manipulating queue freeze states, etc.
> The serialized condition stays true until the controller is
> transitioned to the LIVE state. Thus the fix is to change the
> error recovery path to check the controller state and only
> invoke the teardown path if not already in the CONNECTING state.
> 
> Signed-off-by: James Smart <jsmart2021@gmail.com>
> ---
>  drivers/nvme/host/fc.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 679a721ae229..2acb850bf9f4 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2910,10 +2910,22 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
>  static void
>  __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl)
>  {
> -	nvme_stop_keep_alive(&ctrl->ctrl);
> +	/*
> +	 * if state is connecting - the error occurred as part of a
> +	 * reconnect attempt. The create_association error paths will
> +	 * clean up any outstanding io.
> +	 *
> +	 * if it's a different state - ensure all pending io is
> +	 * terminated. Given this can delay while waiting for the
> +	 * aborted io to return, we recheck adapter state below
> +	 * before changing state.
> +	 */
> +	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
> +		nvme_stop_keep_alive(&ctrl->ctrl);
>  
> -	/* will block will waiting for io to terminate */
> -	nvme_fc_delete_association(ctrl);
> +		/* will block will waiting for io to terminate */
> +		nvme_fc_delete_association(ctrl);
> +	}
>  
>  	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING &&
>  	    !nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))

Reviewed-by: Ewan D. Milne <emilne@redhat.com>


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] nvme-fc: fix double-free scenarios on hw queues
  2019-11-21 17:59 [PATCH] nvme-fc: fix double-free scenarios on hw queues James Smart
  2019-11-21 19:31 ` Ewan D. Milne
@ 2019-11-22 17:01 ` Himanshu Madhani
  2019-11-26 18:01 ` Keith Busch
  2 siblings, 0 replies; 4+ messages in thread
From: Himanshu Madhani @ 2019-11-22 17:01 UTC (permalink / raw)
  To: James Smart, linux-nvme



On 11/21/19, 12:00 PM, "Linux-nvme on behalf of James Smart" <linux-nvme-bounces@lists.infradead.org on behalf of jsmart2021@gmail.com> wrote:

    If an error occurs on one of the ios used for creating an
    association, the creating routine has error paths that are
    invoked by the command failure and the error paths will free
    up the controller resources created to that point.
    
    But... the io was ultimately determined by an asynchronous
    completion routine that detected the error and which
    unconditionally invokes the error_recovery path which calls
    delete_association. Delete association deletes all outstanding
    io then tears down the controller resources. So the
    create_association thread can be running in parallel with
    the error_recovery thread. What was seen was the LLDD received
    a call to delete a queue, causing the LLDD to do a free of a
    resource, then the transport called the delete queue again
    causing the driver to repeat the free call. The second free
    routine corrupted the allocator. The transport shouldn't be
    making the duplicate call, and the delete queue is just one
    of the resources being freed.
    
    To fix, it is realized that the create_association path is
    completely serialized with one command at a time. So the
    failed io completion will always be seen by the create_association
    path and as of the failure, there are no ios to terminate and there
    is no reason to be manipulating queue freeze states, etc.
    The serialized condition stays true until the controller is
    transitioned to the LIVE state. Thus the fix is to change the
    error recovery path to check the controller state and only
    invoke the teardown path if not already in the CONNECTING state.
    
    Signed-off-by: James Smart <jsmart2021@gmail.com>
    ---
     drivers/nvme/host/fc.c | 18 +++++++++++++++---
     1 file changed, 15 insertions(+), 3 deletions(-)
    
    diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
    index 679a721ae229..2acb850bf9f4 100644
    --- a/drivers/nvme/host/fc.c
    +++ b/drivers/nvme/host/fc.c
    @@ -2910,10 +2910,22 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
     static void
     __nvme_fc_terminate_io(struct nvme_fc_ctrl *ctrl)
     {
    -	nvme_stop_keep_alive(&ctrl->ctrl);
    +	/*
    +	 * if state is connecting - the error occurred as part of a
    +	 * reconnect attempt. The create_association error paths will
    +	 * clean up any outstanding io.
    +	 *
    +	 * if it's a different state - ensure all pending io is
    +	 * terminated. Given this can delay while waiting for the
    +	 * aborted io to return, we recheck adapter state below
    +	 * before changing state.
    +	 */
    +	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
    +		nvme_stop_keep_alive(&ctrl->ctrl);
     
    -	/* will block will waiting for io to terminate */
    -	nvme_fc_delete_association(ctrl);
    +		/* will block will waiting for io to terminate */
    +		nvme_fc_delete_association(ctrl);
    +	}
     
     	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING &&
     	    !nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
    -- 
    2.13.7
    
Reviewed-by: Himanshu Madhani <hmadhani@marvell.com>
    
    _______________________________________________
    Linux-nvme mailing list
    Linux-nvme@lists.infradead.org
    https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=lanuADUiz2ynuLzGAQkJPTKDNod8UMh5Vi117R7DgS4&m=Vk2A_OK6OxwR1xqGsZrj6snG3gFAGnqmbBuHm9pi_E8&s=wFqks65MM_XSxeEc_PsYTElI1efUDTmkWe_pX3pTGZU&e= 
    


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] nvme-fc: fix double-free scenarios on hw queues
  2019-11-21 17:59 [PATCH] nvme-fc: fix double-free scenarios on hw queues James Smart
  2019-11-21 19:31 ` Ewan D. Milne
  2019-11-22 17:01 ` Himanshu Madhani
@ 2019-11-26 18:01 ` Keith Busch
  2 siblings, 0 replies; 4+ messages in thread
From: Keith Busch @ 2019-11-26 18:01 UTC (permalink / raw)
  To: James Smart; +Cc: linux-nvme

Applied to nvme/for-5.5

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-21 17:59 [PATCH] nvme-fc: fix double-free scenarios on hw queues James Smart
2019-11-21 19:31 ` Ewan D. Milne
2019-11-22 17:01 ` Himanshu Madhani
2019-11-26 18:01 ` Keith Busch

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git