Re: [PATCH 07/14] infiniband: utilize new device_add_cdev helper function

On Mon, Feb 20, 2017 at 10:00:46PM -0700, Logan Gunthorpe wrote: Hum, is probably safe - ib_ucm_remove_one can only happen on module unload and the cdev holds the module lock while open. Even so device_add_cdev should be used anyhow. 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. Ah, this needs more help. Once device_initialize is called put_device should be the error-unwind, can you use something more like this? From: Jason Gunthorpe <jgunthorpe(a)obsidianresearch.com&gt; Date: Tue, 21 Feb 2017 12:07:55 -0700 Subject: [PATCH] infiniband: utilize new device_add_cdev helper The use after free is not triggerable here because the cdev holds the module lock and the only device_unregister is only triggered by module ouload, however make the change for consistency. To make this work the cdev_del needs to move out of the struct device release function. Signed-off-by: Jason Gunthorpe <jgunthorpe(a)obsidianresearch.com&gt; --- drivers/infiniband/core/ucm.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) 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); } -- 2.7.4