Re: [PATCH v3] device-dax: Fix refcount leak in __devm_create_dev_dax() error path

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Mon, 13 Apr 2026 21:56:25 +0800
Guangshuo Li <lgs201920130244@gmail.com> wrote:

> After device_initialize(), the embedded struct device in dev_dax is
> expected to be released through the device core with put_device().
> 
> In __devm_create_dev_dax(), several failure paths after
> device_initialize() free dev_dax directly instead of dropping the device
> reference, which bypasses the normal device core lifetime handling and
> leaks the reference held on the embedded struct device.
> 
> The issue was identified by a static analysis tool I developed and
> confirmed by manual review.
> 
> Fix this by assigning dev->type before device_initialize(), so the
> release callback is available, use put_device() in the
> post-initialization error paths, and keep dev_dax range cleanup explicit
> since it is not handled by dev_dax_release().
> 
> Fixes: c2f3011ee697f ("device-dax: add an allocation interface for device-dax instances")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
Hi

Whilst I think your fix is correct the need to still handle one error path
via a different set of goto labels is not as easy to read as I'd like to see.

There is also some ordering stuff in here that is somewhat messy and
needs some more thought.  alloc_dev_dax_range() is unwound out of order
wrt to data->pgmap.

Thanks,

Jonathan

> ---
> v3:
>   - note that the issue was identified by my static analysis tool
>   - and confirmed by manual review
> 
> v2:
>   - clarify the commit message around the device reference leak
>   - drop the unsupported use-after-free claim
>   - set dev->type before device_initialize() so put_device() can use the
>     release callback on post-init failures
>   - simplify the post-initialization error paths to use explicit range
>     cleanup plus put_device()
> 
>  drivers/dax/bus.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index fde29e0ad68b..2d92674d0d6e 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -1453,6 +1453,7 @@ static struct dev_dax *__devm_create_dev_dax(struct dev_dax_data *data)
>  	}
>  
>  	dev = &dev_dax->dev;
> +	dev->type = &dev_dax_type;
>  	device_initialize(dev);
>  	dev_set_name(dev, "dax%d.%d", dax_region->id, dev_dax->id);
>  
> @@ -1499,7 +1500,6 @@ static struct dev_dax *__devm_create_dev_dax(struct dev_dax_data *data)
>  	dev->devt = inode->i_rdev;
>  	dev->bus = &dax_bus_type;
>  	dev->parent = parent;
> -	dev->type = &dev_dax_type;
>  
>  	rc = device_add(dev);
>  	if (rc) {
> @@ -1522,14 +1522,13 @@ static struct dev_dax *__devm_create_dev_dax(struct dev_dax_data *data)
>  	return dev_dax;
>  
>  err_alloc_dax:
> -	kfree(dev_dax->pgmap);
>  err_pgmap:
>  	free_dev_dax_ranges(dev_dax);
This bothers me somewhat as now the error paths are not unwinding in reverse of the setup.

>  err_range:
> -	free_dev_dax_id(dev_dax);
> +	put_device(dev);
> +	return ERR_PTR(rc);
It would be helpful to have some white space around this to make it easier
to spot given it's in the middle of the list.


>  err_id:
>  	kfree(dev_dax);
Cam we juggle things around a little more so that there is no path that
hits this?  I.e. move device_initialize() (and whatever needs setting for
release to work) to just after allocation?

I think the one complication is we need to ensure correct behavior if the
id has not been successfully allocated.  Perhaps using a flag value of -1 would
make this easy to check for.

Alternative would be to have a helper that does the allocate and ID allocation
parts and handles this kfree() internally.


> -
Unrelated and probably not a desirable change.  It's common to put a blank line
before simple returns like this one.

>  	return ERR_PTR(rc);
>  }
>