[PATCH] nvme: move stopping keep-alive into nvme_uninit_ctrl()

[PATCH] nvme: move stopping keep-alive into nvme_uninit_ctrl()

[Ming Lei]

Commit 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
moves starting keep-alive from nvme_start_ctrl() into
nvme_init_ctrl_finish(), but don't move stopping keep-alive into
nvme_uninit_ctrl(), so keep-alive work can be started and keep pending
after failing to start controller, finally use-after-free is triggered if
nvme host driver is unloaded.

This patch fixes kernel panic when running nvme/004 in case that connection
failure is triggered, by moving stopping keep-alive into nvme_uninit_ctrl().

This way is reasonable because keep-alive is now started in
nvme_init_ctrl_finish().

Fixes: 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
Cc: Hannes Reinecke <hare@suse.de>
Cc: Mark O'Donovan <shiftee@posteo.net>
Reported-by: Changhui Zhong <czhong@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 053d5b4909cd..562afa71ea85 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4612,7 +4612,6 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
 {
 	nvme_mpath_stop(ctrl);
 	nvme_auth_stop(ctrl);
-	nvme_stop_keep_alive(ctrl);
 	nvme_stop_failfast_work(ctrl);
 	flush_work(&ctrl->async_event_work);
 	cancel_work_sync(&ctrl->fw_act_work);
@@ -4648,6 +4647,7 @@ EXPORT_SYMBOL_GPL(nvme_start_ctrl);
 
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
 {
+	nvme_stop_keep_alive(ctrl);
 	nvme_hwmon_exit(ctrl);
 	nvme_fault_inject_fini(&ctrl->fault_inject);
 	dev_pm_qos_hide_latency_tolerance(ctrl->device);
-- 
2.45.2

Re: [PATCH] nvme: move stopping keep-alive into nvme_uninit_ctrl()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] nvme: move stopping keep-alive into nvme_uninit_ctrl()

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH] nvme: move stopping keep-alive into nvme_uninit_ctrl()

[Hannes Reinecke]

On 8/9/24 15:54, Ming Lei wrote:
> Commit 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
> moves starting keep-alive from nvme_start_ctrl() into
> nvme_init_ctrl_finish(), but don't move stopping keep-alive into
> nvme_uninit_ctrl(), so keep-alive work can be started and keep pending
> after failing to start controller, finally use-after-free is triggered if
> nvme host driver is unloaded.
> 
> This patch fixes kernel panic when running nvme/004 in case that connection
> failure is triggered, by moving stopping keep-alive into nvme_uninit_ctrl().
> 
> This way is reasonable because keep-alive is now started in
> nvme_init_ctrl_finish().
> 
> Fixes: 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Mark O'Donovan <shiftee@posteo.net>
> Reported-by: Changhui Zhong <czhong@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/nvme/host/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 053d5b4909cd..562afa71ea85 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4612,7 +4612,6 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
>   {
>   	nvme_mpath_stop(ctrl);
>   	nvme_auth_stop(ctrl);
> -	nvme_stop_keep_alive(ctrl);
>   	nvme_stop_failfast_work(ctrl);
>   	flush_work(&ctrl->async_event_work);
>   	cancel_work_sync(&ctrl->fw_act_work);

Huh? What happened here?
Commit 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
has _exactly_ the same hunk.
Someone else must've changed it afterwards, so please update the 'fixes'
tag to refer to the correct commit.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH] nvme: move stopping keep-alive into nvme_uninit_ctrl()

[Ming Lei]

On Mon, Aug 12, 2024 at 01:56:01PM +0200, Hannes Reinecke wrote:
> On 8/9/24 15:54, Ming Lei wrote:
> > Commit 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
> > moves starting keep-alive from nvme_start_ctrl() into
> > nvme_init_ctrl_finish(), but don't move stopping keep-alive into
> > nvme_uninit_ctrl(), so keep-alive work can be started and keep pending
> > after failing to start controller, finally use-after-free is triggered if
> > nvme host driver is unloaded.
> > 
> > This patch fixes kernel panic when running nvme/004 in case that connection
> > failure is triggered, by moving stopping keep-alive into nvme_uninit_ctrl().
> > 
> > This way is reasonable because keep-alive is now started in
> > nvme_init_ctrl_finish().
> > 
> > Fixes: 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Mark O'Donovan <shiftee@posteo.net>
> > Reported-by: Changhui Zhong <czhong@redhat.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   drivers/nvme/host/core.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 053d5b4909cd..562afa71ea85 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -4612,7 +4612,6 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
> >   {
> >   	nvme_mpath_stop(ctrl);
> >   	nvme_auth_stop(ctrl);
> > -	nvme_stop_keep_alive(ctrl);
> >   	nvme_stop_failfast_work(ctrl);
> >   	flush_work(&ctrl->async_event_work);
> >   	cancel_work_sync(&ctrl->fw_act_work);
> 
> Huh? What happened here?
> Commit 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
> has _exactly_ the same hunk.
> Someone else must've changed it afterwards, so please update the 'fixes'
> tag to refer to the correct commit.

It is exactly 4733b65d82bd ("nvme: start keep-alive after admin
queue setup"), which moves nvme_start_keep_alive() into
nvme_init_ctrl_finish(), but not move nvme_stop_keep_alive() to
nvme_uninit_ctrl().


Thanks,
Ming

Re: [PATCH] nvme: move stopping keep-alive into nvme_uninit_ctrl()

[Keith Busch]

On Mon, Aug 12, 2024 at 01:56:01PM +0200, Hannes Reinecke wrote:
> On 8/9/24 15:54, Ming Lei wrote:
> > @@ -4612,7 +4612,6 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
> >   {
> >   	nvme_mpath_stop(ctrl);
> >   	nvme_auth_stop(ctrl);
> > -	nvme_stop_keep_alive(ctrl);
> >   	nvme_stop_failfast_work(ctrl);
> >   	flush_work(&ctrl->async_event_work);
> >   	cancel_work_sync(&ctrl->fw_act_work);
> 
> Huh? What happened here?
> Commit 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
> has _exactly_ the same hunk.
> Someone else must've changed it afterwards, so please update the 'fixes'
> tag to refer to the correct commit.

Yes, someone did change it :)

  commit 3af755a46881c32fecaecfdeaf3a8f0a869deca5
  Author: Hannes Reinecke <hare@suse.de>
  Date:   Tue Nov 21 09:01:03 2023 +0100

      nvme: move nvme_stop_keep_alive() back to original position

So 4733b65d82bd moved it to the wrong place and Ming's patch is the
correct one?

Re: [PATCH] nvme: move stopping keep-alive into nvme_uninit_ctrl()

[Hannes Reinecke]

On 8/12/24 17:14, Keith Busch wrote:
> On Mon, Aug 12, 2024 at 01:56:01PM +0200, Hannes Reinecke wrote:
>> On 8/9/24 15:54, Ming Lei wrote:
>>> @@ -4612,7 +4612,6 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
>>>    {
>>>    	nvme_mpath_stop(ctrl);
>>>    	nvme_auth_stop(ctrl);
>>> -	nvme_stop_keep_alive(ctrl);
>>>    	nvme_stop_failfast_work(ctrl);
>>>    	flush_work(&ctrl->async_event_work);
>>>    	cancel_work_sync(&ctrl->fw_act_work);
>>
>> Huh? What happened here?
>> Commit 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
>> has _exactly_ the same hunk.
>> Someone else must've changed it afterwards, so please update the 'fixes'
>> tag to refer to the correct commit.
> 
> Yes, someone did change it :)
> 
>    commit 3af755a46881c32fecaecfdeaf3a8f0a869deca5
>    Author: Hannes Reinecke <hare@suse.de>
>    Date:   Tue Nov 21 09:01:03 2023 +0100
> 
>        nvme: move nvme_stop_keep_alive() back to original position
> 
> So 4733b65d82bd moved it to the wrong place and Ming's patch is the
> correct one?

That's perfectly fine, it's just that the 'Fixes' tag doesn't tell the 
full story.
All I was asking it to update the 'Fixes' tag to the latest patch
(ie 3af755a46881 ("nvme: move nvme_stop_keep_alive() back to original 
position")) such that the poor people having to do backports can
rewind via the 'Fixes' tag and get all necessary patches.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich