Re: [PATCH 1/4] dax: Fix dax_mapping_release() use after free

[Dave Jiang <dave.jiang@intel.com>]

On 6/2/23 23:13, Dan Williams wrote:
> A CONFIG_DEBUG_KOBJECT_RELEASE test of removing a device-dax region
> provider (like modprobe -r dax_hmem) yields:
> 
>   kobject: 'mapping0' (ffff93eb460e8800): kobject_release, parent 0000000000000000 (delayed 2000)
>   [..]
>   DEBUG_LOCKS_WARN_ON(1)
>   WARNING: CPU: 23 PID: 282 at kernel/locking/lockdep.c:232 __lock_acquire+0x9fc/0x2260
>   [..]
>   RIP: 0010:__lock_acquire+0x9fc/0x2260
>   [..]
>   Call Trace:
>    <TASK>
>   [..]
>    lock_acquire+0xd4/0x2c0
>    ? ida_free+0x62/0x130
>    _raw_spin_lock_irqsave+0x47/0x70
>    ? ida_free+0x62/0x130
>    ida_free+0x62/0x130
>    dax_mapping_release+0x1f/0x30
>    device_release+0x36/0x90
>    kobject_delayed_cleanup+0x46/0x150
> 
> Due to attempting ida_free() on an ida object that has already been
> freed. Devices typically only hold a reference on their parent while
> registered. If a child needs a parent object to complete its release it
> needs to hold a reference that it drops from its release callback.
> Arrange for a dax_mapping to pin its parent dev_dax instance until
> dax_mapping_release().
> 
> Fixes: 0b07ce872a9e ("device-dax: introduce 'mapping' devices")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>   drivers/dax/bus.c |    5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 227800053309..aee695f86b44 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -635,10 +635,12 @@ EXPORT_SYMBOL_GPL(alloc_dax_region);
>   static void dax_mapping_release(struct device *dev)
>   {
>   	struct dax_mapping *mapping = to_dax_mapping(dev);
> -	struct dev_dax *dev_dax = to_dev_dax(dev->parent);
> +	struct device *parent = dev->parent;
> +	struct dev_dax *dev_dax = to_dev_dax(parent);
>   
>   	ida_free(&dev_dax->ida, mapping->id);
>   	kfree(mapping);
> +	put_device(parent);
>   }
>   
>   static void unregister_dax_mapping(void *data)
> @@ -778,6 +780,7 @@ static int devm_register_dax_mapping(struct dev_dax *dev_dax, int range_id)
>   	dev = &mapping->dev;
>   	device_initialize(dev);
>   	dev->parent = &dev_dax->dev;
> +	get_device(dev->parent);
>   	dev->type = &dax_mapping_type;
>   	dev_set_name(dev, "mapping%d", mapping->id);
>   	rc = device_add(dev);
>