| Message ID | 168577283412.1672036.16111545266174261446.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Accepted |
| Commit | 6d24b170a9db0456f577b1ab01226a2254c016a8 |
| Headers | show |
| Series | dax: Fix use after free and other cleanups | expand |
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") Reviewed-by: Ira Weiny <ira.weiny@intel.com>
On Fri, Jun 02, 2023 at 11:13:54PM -0700, 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: Fan Ni <fan.ni@samsung.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); > >
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); >
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);
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> --- drivers/dax/bus.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)