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

[Ming Lei <ming.lei@redhat.com>]

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