[PATCH 3/3] nvme-core: preventing double freeing in ctrl release

[Keith Busch]

On Tue, Apr 25, 2023 at 06:18:36PM -0300, Irvin Cote wrote:
> The teardown logic and the release method for
> the controller both free the same resources.
> This introduces double-freeing issues.
> Since the release of the ctrl is the last step
> of the teardown, we can solve this by setting the freed
> pointers to NULL in the teardown path and add checks
> in the release method.
> The only issue is with ida_free as ida doesn't offer any way to
> indicate that an ID has already been freed.
> In our specific case we can set the ID to -1 to indicate that.
> We can do that because, the way we've setup ida to manage ctrl instances,
> it always returns non-negative IDS.

Your line wrap setting is way too short. 72 characters is recommended.
 
> @@ -5091,14 +5091,15 @@ static void nvme_free_ctrl(struct device *dev)
>  		container_of(dev, struct nvme_ctrl, ctrl_device);
>  	struct nvme_subsystem *subsys = ctrl->subsys;
>  
> -	if (!subsys || ctrl->instance != subsys->instance)
> +	if ((!subsys || ctrl->instance != subsys->instance) && ctrl->instance != -1)
>  		ida_free(&nvme_instance_ida, ctrl->instance);
>  
>  	nvme_free_cels(ctrl);
>  	nvme_mpath_uninit(ctrl);
>  	nvme_auth_stop(ctrl);
>  	nvme_auth_free(ctrl);
> -	__free_page(ctrl->discard_page);
> +	if(ctrl->discard_page)
> +		__free_page(ctrl->discard_page);
>  	free_opal_dev(ctrl->opal_dev);
>  
>  	if (subsys) {
> @@ -5202,8 +5203,10 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>  	kfree_const(ctrl->device->kobj.name);
>  out_release_instance:
>  	ida_free(&nvme_instance_ida, ctrl->instance);
> +	ctrl->instance = -1;

Isn't the problem that the nvme_put_ctrl() in this function's 'goto' error is
doubling the rest of the unwinding, which means your ida_free() is happening
twice?

I think instead the nvme_get_ctrl() should be the last thing done in the
init_ctrl() function, then the rest of the cleanup on error would be simpler.

>  out_free_discard_page:
>  	__free_page(ctrl->discard_page);
> +	ctrl->discard_page = NULL;
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(nvme_init_ctrl);
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 65e4b9f1b632..ff5c876739bc 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2681,8 +2681,10 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
>  
>  	nvme_free_tagset(dev);
>  	put_device(dev->dev);
> -	kfree(dev->queues);
> -	kfree(dev);
> +	if(dev->queues)
> +		kfree(dev->queues);
> +	if(dev)
> +		kfree(dev);

kfree() already checks for NULL, so these are redundant.

>  }
>  
>  static void nvme_reset_work(struct work_struct *work)
> @@ -2973,8 +2975,10 @@ static struct nvme_dev *nvme_pci_alloc_dev(struct pci_dev *pdev,
>  out_put_device:
>  	put_device(dev->dev);
>  	kfree(dev->queues);
> +	dev->queues = NULL;
>  out_free_dev:
>  	kfree(dev);
> +	dev = NULL;
>  	nvme_put_ctrl(&dev->ctrl);
>  	return ERR_PTR(ret);
>  }
> --