Re: [PATCH] nvme-apple: drop invalid put of admin queue reference count

[Jens Axboe]

On 4/3/26 2:27 PM, Fedor Pchelkin wrote:
> Commit 03b3bcd319b3 ("nvme: fix admin request_queue lifetime") moved the
> admin queue reference ->put call into nvme_free_ctrl() - a controller
> device release callback performed for every nvme driver doing
> nvme_init_ctrl().
> 
> nvme-apple sets refcount of the admin queue to 1 at allocation during the
> probe function and then puts it twice now:
> 
> nvme_free_ctrl()
>   blk_put_queue(ctrl->admin_q) // #1
>   ->free_ctrl()
>     apple_nvme_free_ctrl()
>       blk_put_queue(anv->ctrl.admin_q) // #2
> 
> Note that there is a commit 941f7298c70c ("nvme-apple: remove an extra
> queue reference") which intended to drop having an extra admin queue
> reference.  Looks like at that moment it accidentally fixed a refcount
> leak, which existed since the driver's introduction.  There were an
> initial ->set and an extra ->get call at driver's probe function, and only
> a single ->put inside apple_nvme_free_ctrl().
> 
> However now after commit 03b3bcd319b3 ("nvme: fix admin request_queue
> lifetime") the refcount is imbalanced again.  Fix it by removing extra
> ->put call from apple_nvme_free_ctrl().  Compile tested only.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Fixes: 03b3bcd319b3 ("nvme: fix admin request_queue lifetime")
> Cc: stable@xxxxxxxxxxxxxxx # depends on 941f7298c70c
> Signed-off-by: Fedor Pchelkin <pchelkin@xxxxxxxxx>
> ---
> 
> Also nvme-apple seems not to have a blk_mq_destroy_queue() call for
> admin queue since introduction - if it's needed, the proper place would
> be in apple_nvme_remove() just before calling nvme_uninit_ctrl(), I guess?
> 
>  drivers/nvme/host/apple.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
> index ed61b97fde59..1d82f0541b0b 100644
> --- a/drivers/nvme/host/apple.c
> +++ b/drivers/nvme/host/apple.c
> @@ -1269,8 +1269,6 @@ static void apple_nvme_free_ctrl(struct nvme_ctrl *ctrl)
>  {
>  	struct apple_nvme *anv = ctrl_to_apple_nvme(ctrl);
>  
> -	if (anv->ctrl.admin_q)
> -		blk_put_queue(anv->ctrl.admin_q);
>  	put_device(anv->dev);
>  }

Could this just be:

static void apple_nvme_free_ctrl(struct nvme_ctrl *ctrl)
{
	put_device(ctrl->dev);
}

at this point?

-- 
Jens Axboe