[tpmdd-devel,07/14] infiniband: utilize new device_add_cdev helper function

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.

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.

> 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?

>From ac7a35ea98066c9a9e3f3e54557c0ccda44b0ffc Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
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@obsidianresearch.com>
---
 drivers/infiniband/core/ucm.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

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.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot