On Wed, Feb 5, 2020 at 11:08 AM Dan Carpenter <[email protected]> wrote: > > On Wed, Feb 05, 2020 at 10:23:00AM -0800, Dan Williams wrote: > > > > > 506 if (device_add(dev) != 0) { > > > > > 507 dev_err(dev, "%s: failed\n", __func__); > > > > > 508 put_device(dev); > > > > > ^^^^^^^^^^^^^^^ > > > > > 509 } > > > > > 510 put_device(dev); > > > > > ^^^^^^^^^^^^^^ > > > > > 511 if (dev->parent) > > > > > 512 put_device(dev->parent); > > > > > 513 } > > > > > > > > > > We call get_device() from __nd_device_register(), I guess. It seems > > > > > buggy to call put device twice on error. > > > > > > > > The registration path does: > > > > > > > > get_device(dev); > > > > > > > > async_schedule_dev_domain(nd_async_device_register, dev, > > > > &nd_async_domain); > > > > > > > > ...and device_add() does its own get_device(). > > > > > > device_add() does its own put_device() at the end so it's a net zero. > > > > > > > It does it's own, yes, but the put_device() after device_add() failure > > is there to drop the reference taken by device_initialize(). > > Otherwise, device_add() has always documented: > > > > * NOTE: _Never_ directly free @dev after calling this function, even > > * if it returned an error! Always use put_device() to give up your > > * reference instead. > > > > ...so what am I missing? > > The "never call kfree" is hopefully straight forward because the kobject > needs to do its own cleanup. > > __nvdimm_create() allocates the dev. > nd_device_register() calls device_initialize() which call kobject_init() > so the refcount is 1. > __nd_device_register() call get_device() so the refcount is now two. > nd_async_device_register() decrements the refcount once on success. > > But if device_add() fails then it decrements it twice. Now the refcount > is zero so we call nvdimm_release(). This leads to a use after free on > the next line: > > put_device(dev); > if (dev->parent) > > There is a trick here because depending on the debug options it > might free immediately or it might call nvdimm_release() after 4 > seconds. See kobject_release() for details. > > Either way if device_add() fails we return back to __nvdimm_create() > and return the zero reference count "nvdimm" pointer, which is going > to be a problem.
Ugh, sorry I thought you were pointing out that there's too many put_device() not the use after free. Yes, the use after free is a bug that needs fixing. _______________________________________________ Linux-nvdimm mailing list -- [email protected] To unsubscribe send an email to [email protected]
