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.
> 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
> diff --git a/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.