| Message ID | 20170221190905.GD13138@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21/02/17 12:09 PM, Jason Gunthorpe wrote: > On Mon, Feb 20, 2017 at 10:00:46PM -0700, Logan Gunthorpe wrote: >> This patch updates core/ucm.c which didn't originally use the >> cdev.kobj.parent with it's parent device. I did not look heavily into >> whether this was a bug or not, but it seems likely to me there would >> be a use before free. > > Hum, is probably safe - ib_ucm_remove_one can only happen on module > unload and the cdev holds the module lock while open. Ah, yes looks like you are correct. > Even so device_add_cdev should be used anyhow. Agreed. >> I also took a look at core/uverbs_main.c, core/user_mad.c and >> hw/hfi1/device.c which utilize cdev.kobj.parent but because the >> infiniband core seems to use kobjs internally instead of struct devices >> they could not be converted to use the new helper API and still >> directly manipulate the internals of the kobj. > > Yes, and hfi1 had the same use-afte-free bug when it was first > submitted as well. IHMO cdev should have a helper entry for this style > of use case as well. I agree, but I'm not sure what that api should look like. Same thing but kobject_add_cdev??? >> diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c >> index e0a995b..38ea316 100644 >> +++ b/drivers/infiniband/core/ucm.c >> @@ -1283,18 +1283,20 @@ static void ib_ucm_add_one(struct ib_device *device) >> set_bit(devnum, dev_map); >> } >> >> + device_initialize(&ucm_dev->dev); > > Ah, this needs more help. Once device_initialize is called put_device > should be the error-unwind, can you use something more like this? Is that true? Once device_register or device_add is called then you need to use put_device. But I didn't think that's true for device_initialize. In fact device_add is what does the first get_device so this doesn't add up to me. device_initialize only inits some structures it doesn't do anything that needs to be torn down -- at least that I'm aware of. I know the DAX code only uses put_device after device_add. [1] Logan [1] http://lxr.free-electrons.com/source/drivers/dax/dax.c#L713
On Tue, Feb 21, 2017 at 04:54:05PM -0700, Logan Gunthorpe wrote: > Is that true? Once device_register or device_add is called then you need > to use put_device. General rule is once kref_init has been called then you should use kref_put and not kfree. device_initialize ultimately calls kref_init.. Reasoning that you don't 'need' to use put_device until device_add is just too hard. For instance, there is still another bug in ib_ucm_add_one: if (device_add(&ucm_dev->dev)) goto err_cdev; if (device_create_file(&ucm_dev->dev, &dev_attr_ibdev)) goto err_dev; [....] err_dev: device_unregister(&ucm_dev->dev); err_cdev: ib_ucm_cdev_del(ucm_dev); err: kfree(ucm_dev); return; } If we go down the err_dev path then device_unregister will probably kfree ucm_dev - the argument is not guarenteed valid after device_unregister returns - this is what makes it different from device_del. The simplest fix is to change the unwind into: err_dev: device_del(&ucm_dev->dev); err_cdev: ib_ucm_cdev_del(ucm_dev); err: put_device(&ucm_dev->dev); return; } And the only way to keep our idiomatic goto unwind working is if all 'goto errs' can call put_device - which requires the device_initialize be done before any errors are possible. > In fact device_add is what does the first get_device so this doesn't > add up to me. Not quite, kref_init creates a kref with a count of 1 - eg the caller owns the ref, and that ref must be put to trigger kref release. Thus good kref usage should always pair a kref_put with the kref_init. The get_device at the start of device_add pairs with the put_device at the end of device_del and the kref_init pairs with the put_device at the end of device_unregister. (notice that device_unregister ends up calling put_device twice in a row..) I'll send you a clean patch for your v2. > I know the DAX code only uses put_device after device_add. Looks to me like that code fails to call cdev_del if device_add fails? This approach is problematic because it is trying do the ida removals in the release function. That is not necessary and has the side effect of making the release function uncallable until too late in the process. Jason
On 02/22/2017 12:54 AM, Logan Gunthorpe wrote: [...] >> >> Ah, this needs more help. Once device_initialize is called put_device >> should be the error-unwind, can you use something more like this? > > Is that true? Once device_register or device_add is called then you need > to use put_device. [...] device_initialize() documentation: NOTE: Use put_device() to give up your reference instead of freeing @dev directly once you have called this function.
diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c index 7713ef089c3ccc..acda8d941381f3 100644 --- a/drivers/infiniband/core/ucm.c +++ b/drivers/infiniband/core/ucm.c @@ -1202,12 +1202,16 @@ static void ib_ucm_release_dev(struct device *dev) struct ib_ucm_device *ucm_dev; ucm_dev = container_of(dev, struct ib_ucm_device, dev); + kfree(ucm_dev); +} + +static void ib_ucm_cdev_del(struct ib_ucm_device *ucm_dev) +{ cdev_del(&ucm_dev->cdev); if (ucm_dev->devnum < IB_UCM_MAX_DEVICES) clear_bit(ucm_dev->devnum, dev_map); else clear_bit(ucm_dev->devnum - IB_UCM_MAX_DEVICES, overflow_map); - kfree(ucm_dev); } static const struct file_operations ucm_fops = { @@ -1263,6 +1267,7 @@ static void ib_ucm_add_one(struct ib_device *device) if (!ucm_dev) return; + device_initialize(&ucm_dev->dev); ucm_dev->ib_dev = device; devnum = find_first_zero_bit(dev_map, IB_UCM_MAX_DEVICES); @@ -1280,18 +1285,19 @@ static void ib_ucm_add_one(struct ib_device *device) set_bit(devnum, dev_map); } + ucm_dev->dev.devt = base; + ucm_dev->dev.release = ib_ucm_release_dev; + cdev_init(&ucm_dev->cdev, &ucm_fops); ucm_dev->cdev.owner = THIS_MODULE; kobject_set_name(&ucm_dev->cdev.kobj, "ucm%d", ucm_dev->devnum); - if (cdev_add(&ucm_dev->cdev, base, 1)) + if (device_add_cdev(&ucm_dev->dev, &ucm_dev->cdev)) goto err; ucm_dev->dev.class = &cm_class; ucm_dev->dev.parent = device->dma_device; - ucm_dev->dev.devt = ucm_dev->cdev.dev; - ucm_dev->dev.release = ib_ucm_release_dev; dev_set_name(&ucm_dev->dev, "ucm%d", ucm_dev->devnum); - if (device_register(&ucm_dev->dev)) + if (device_add(&ucm_dev->dev)) goto err_cdev; if (device_create_file(&ucm_dev->dev, &dev_attr_ibdev)) @@ -1303,13 +1309,9 @@ static void ib_ucm_add_one(struct ib_device *device) err_dev: device_unregister(&ucm_dev->dev); err_cdev: - cdev_del(&ucm_dev->cdev); - if (ucm_dev->devnum < IB_UCM_MAX_DEVICES) - clear_bit(devnum, dev_map); - else - clear_bit(devnum, overflow_map); + ib_ucm_cdev_del(ucm_dev); err: - kfree(ucm_dev); + put_device(&ucm_dev->dev); return; } @@ -1320,6 +1322,7 @@ static void ib_ucm_remove_one(struct ib_device *device, void *client_data) if (!ucm_dev) return; + ib_ucm_cdev_del(ucm_dev); device_unregister(&ucm_dev->dev); }