[PATCH v9 2/2] uio: Fix uio driver to refcount device

[PATCH v9 2/2] uio: Fix uio driver to refcount device

[Brian Russell]

Protect uio driver from its owner being unplugged while there are open fds.
Embed struct device in struct uio_device, use refcounting on device, free
uio_device on release.
info struct passed in uio_register_device can be freed on unregister, so null
out the field in uio_unregister_device and check accesses.

Signed-off-by: Brian Russell <brussell@brocade.com>
---
 drivers/uio/uio.c          | 58 +++++++++++++++++++++++++++++++---------------
 include/linux/uio_driver.h |  2 +-
 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 65bf067..4629421 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -270,7 +270,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		if (!map_found) {
 			map_found = 1;
 			idev->map_dir = kobject_create_and_add("maps",
-							&idev->dev->kobj);
+							&idev->dev.kobj);
 			if (!idev->map_dir)
 				goto err_map;
 		}
@@ -295,7 +295,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		if (!portio_found) {
 			portio_found = 1;
 			idev->portio_dir = kobject_create_and_add("portio",
-							&idev->dev->kobj);
+							&idev->dev.kobj);
 			if (!idev->portio_dir)
 				goto err_portio;
 		}
@@ -334,7 +334,7 @@ err_map_kobj:
 		kobject_put(&map->kobj);
 	}
 	kobject_put(idev->map_dir);
-	dev_err(idev->dev, "error creating sysfs files (%d)\n", ret);
+	dev_err(&idev->dev, "error creating sysfs files (%d)\n", ret);
 	return ret;
 }
 
@@ -371,7 +371,7 @@ static int uio_get_minor(struct uio_device *idev)
 		idev->minor = retval;
 		retval = 0;
 	} else if (retval == -ENOSPC) {
-		dev_err(idev->dev, "too many uio devices\n");
+		dev_err(&idev->dev, "too many uio devices\n");
 		retval = -EINVAL;
 	}
 	mutex_unlock(&minor_lock);
@@ -434,9 +434,11 @@ static int uio_open(struct inode *inode, struct file *filep)
 		goto out;
 	}
 
+	get_device(&idev->dev);
+
 	if (!try_module_get(idev->owner)) {
 		ret = -ENODEV;
-		goto out;
+		goto err_module_get;
 	}
 
 	listener = kmalloc(sizeof(*listener), GFP_KERNEL);
@@ -462,6 +464,9 @@ err_infoopen:
 err_alloc_listener:
 	module_put(idev->owner);
 
+ err_module_get:
+	put_device(&idev->dev);
+
 out:
 	return ret;
 }
@@ -480,11 +485,12 @@ static int uio_release(struct inode *inode, struct file *filep)
 	struct uio_listener *listener = filep->private_data;
 	struct uio_device *idev = listener->dev;
 
-	if (idev->info->release)
+	if (idev->info && idev->info->release)
 		ret = idev->info->release(idev->info, inode);
 
 	module_put(idev->owner);
 	kfree(listener);
+	put_device(&idev->dev);
 	return ret;
 }
 
@@ -493,7 +499,7 @@ static unsigned int uio_poll(struct file *filep, poll_table *wait)
 	struct uio_listener *listener = filep->private_data;
 	struct uio_device *idev = listener->dev;
 
-	if (!idev->info->irq)
+	if (!idev->info || !idev->info->irq)
 		return -EIO;
 
 	poll_wait(filep, &idev->wait, wait);
@@ -511,7 +517,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 	ssize_t retval;
 	s32 event_count;
 
-	if (!idev->info->irq)
+	if (!idev->info || !idev->info->irq)
 		return -EIO;
 
 	if (count != sizeof(s32))
@@ -559,7 +565,7 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
 	ssize_t retval;
 	s32 irq_on;
 
-	if (!idev->info->irq)
+	if (!idev->info || !idev->info->irq)
 		return -EIO;
 
 	if (count != sizeof(s32))
@@ -785,6 +791,13 @@ static void release_uio_class(void)
 	uio_major_cleanup();
 }
 
+static void uio_device_release(struct device *dev)
+{
+	struct uio_device *idev = dev_get_drvdata(dev);
+
+	kfree(idev);
+}
+
 /**
  * uio_register_device - register a new userspace IO device
  * @owner:	module that creates the new device
@@ -805,7 +818,7 @@ int __uio_register_device(struct module *owner,
 
 	info->uio_dev = NULL;
 
-	idev = devm_kzalloc(parent, sizeof(*idev), GFP_KERNEL);
+	idev = kzalloc(sizeof(*idev), GFP_KERNEL);
 	if (!idev) {
 		return -ENOMEM;
 	}
@@ -819,14 +832,19 @@ int __uio_register_device(struct module *owner,
 	if (ret)
 		return ret;
 
-	idev->dev = device_create(&uio_class, parent,
-				  MKDEV(uio_major, idev->minor), idev,
-				  "uio%d", idev->minor);
-	if (IS_ERR(idev->dev)) {
-		printk(KERN_ERR "UIO: device register failed\n");
-		ret = PTR_ERR(idev->dev);
+	idev->dev.devt = MKDEV(uio_major, idev->minor);
+	idev->dev.class = &uio_class;
+	idev->dev.parent = parent;
+	idev->dev.release = uio_device_release;
+	dev_set_drvdata(&idev->dev, idev);
+
+	ret = dev_set_name(&idev->dev, "uio%d", idev->minor);
+	if (ret)
+		goto err_device_create;
+
+	ret = device_register(&idev->dev);
+	if (ret)
 		goto err_device_create;
-	}
 
 	ret = uio_dev_add_attributes(idev);
 	if (ret)
@@ -854,7 +872,7 @@ int __uio_register_device(struct module *owner,
 err_request_irq:
 	uio_dev_del_attributes(idev);
 err_uio_dev_add_attributes:
-	device_destroy(&uio_class, MKDEV(uio_major, idev->minor));
+	device_unregister(&idev->dev);
 err_device_create:
 	uio_free_minor(idev);
 	return ret;
@@ -881,7 +899,9 @@ void uio_unregister_device(struct uio_info *info)
 
 	free_irq(idev->info->irq, idev);
 
-	device_destroy(&uio_class, MKDEV(uio_major, idev->minor));
+	idev->info = NULL;
+
+	device_unregister(&idev->dev);
 
 	return;
 }
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 32c0e83..a11feeb 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -65,7 +65,7 @@ struct uio_port {
 
 struct uio_device {
         struct module           *owner;
-        struct device           *dev;
+        struct device           dev;
         int                     minor;
         atomic_t                event;
         struct fasync_struct    *async_queue;
-- 
2.1.4

Re: [PATCH v9 2/2] uio: Fix uio driver to refcount device

[Greg Kroah-Hartman]

On Fri, Mar 20, 2015 at 02:54:44PM +0000, Brian Russell wrote:
> Protect uio driver from its owner being unplugged while there are open fds.
> Embed struct device in struct uio_device, use refcounting on device, free
> uio_device on release.
> info struct passed in uio_register_device can be freed on unregister, so null
> out the field in uio_unregister_device and check accesses.

That's really not protecting anything except heavy-handed problems...

Look at the code:

> @@ -493,7 +499,7 @@ static unsigned int uio_poll(struct file *filep, poll_table *wait)
>  	struct uio_listener *listener = filep->private_data;
>  	struct uio_device *idev = listener->dev;
>  
> -	if (!idev->info->irq)
> +	if (!idev->info || !idev->info->irq)
>  		return -EIO;
>  

Great, you checked the irq value, but what if it changes the very next
line:

>  	poll_wait(filep, &idev->wait, wait);

Or any other line within this function?  Or any other function that you
try to check the value for in the beginning...

This really isn't protecting anything "properly", sorry.  Either we
don't care about it (hint, I don't think we really do), or we need to
properly lock things and check, and protect, things that way.

Please do the first one, as the reference count should be all that we
need to care about here.

Sorry I missed this on the previous review, just realized it now this
time around.

thanks,

greg k-h

Re: [PATCH v9 2/2] uio: Fix uio driver to refcount device

[Brian Russell]

On 23/03/15 20:41, Greg Kroah-Hartman wrote:
> On Fri, Mar 20, 2015 at 02:54:44PM +0000, Brian Russell wrote:
>> Protect uio driver from its owner being unplugged while there are open fds.
>> Embed struct device in struct uio_device, use refcounting on device, free
>> uio_device on release.
>> info struct passed in uio_register_device can be freed on unregister, so null
>> out the field in uio_unregister_device and check accesses.
> 
> That's really not protecting anything except heavy-handed problems...
> 
> Look at the code:
> 
>> @@ -493,7 +499,7 @@ static unsigned int uio_poll(struct file *filep, poll_table *wait)
>>  	struct uio_listener *listener = filep->private_data;
>>  	struct uio_device *idev = listener->dev;
>>  
>> -	if (!idev->info->irq)
>> +	if (!idev->info || !idev->info->irq)
>>  		return -EIO;
>>  
> 
> Great, you checked the irq value, but what if it changes the very next
> line:
> 
>>  	poll_wait(filep, &idev->wait, wait);
> 
> Or any other line within this function?  Or any other function that you
> try to check the value for in the beginning...
> 
> This really isn't protecting anything "properly", sorry.  Either we
> don't care about it (hint, I don't think we really do), or we need to
> properly lock things and check, and protect, things that way.
> 

The checks for irq value are already there. I added the checks for the
idev->info ptr and deliberately nulled it in uio_unregister_device as
the caller module may free uio_info after unregistering (dpdk's igb_uio
does anyway) and then release will be called later when fds are closed.

So I think I definitely need the check in uio_release. I didn't think
it hurt to return early from poll/read/write if we know the device
has been unregistered?

Thanks,

Brian

> Please do the first one, as the reference count should be all that we
> need to care about here.
> 
> Sorry I missed this on the previous review, just realized it now this
> time around.
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v9 2/2] uio: Fix uio driver to refcount device

[Guenter Roeck]

On Tue, Mar 24, 2015 at 12:59:22PM +0000, Brian Russell wrote:
> 
> 
> On 23/03/15 20:41, Greg Kroah-Hartman wrote:
> > On Fri, Mar 20, 2015 at 02:54:44PM +0000, Brian Russell wrote:
> >> Protect uio driver from its owner being unplugged while there are open fds.
> >> Embed struct device in struct uio_device, use refcounting on device, free
> >> uio_device on release.
> >> info struct passed in uio_register_device can be freed on unregister, so null
> >> out the field in uio_unregister_device and check accesses.
> > 
> > That's really not protecting anything except heavy-handed problems...
> > 
> > Look at the code:
> > 
> >> @@ -493,7 +499,7 @@ static unsigned int uio_poll(struct file *filep, poll_table *wait)
> >>  	struct uio_listener *listener = filep->private_data;
> >>  	struct uio_device *idev = listener->dev;
> >>  
> >> -	if (!idev->info->irq)
> >> +	if (!idev->info || !idev->info->irq)
> >>  		return -EIO;
> >>  
> > 
> > Great, you checked the irq value, but what if it changes the very next
> > line:
> > 
> >>  	poll_wait(filep, &idev->wait, wait);
> > 
> > Or any other line within this function?  Or any other function that you
> > try to check the value for in the beginning...
> > 
> > This really isn't protecting anything "properly", sorry.  Either we
> > don't care about it (hint, I don't think we really do), or we need to
> > properly lock things and check, and protect, things that way.
> > 
> 
> The checks for irq value are already there. I added the checks for the
> idev->info ptr and deliberately nulled it in uio_unregister_device as
> the caller module may free uio_info after unregistering (dpdk's igb_uio
> does anyway) and then release will be called later when fds are closed.
> 
> So I think I definitely need the check in uio_release. I didn't think
> it hurt to return early from poll/read/write if we know the device
> has been unregistered?
> 

What is the final verdict on this patch ? We are seeing the crash in our
system, and I would like to apply a 'final' patch if possible to get it
fixed.

Thanks,
Guenter

Re: [PATCH v9 2/2] uio: Fix uio driver to refcount device

[Brian Russell]

> On 8 Jun 2015, at 20:25, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Tue, Mar 24, 2015 at 12:59:22PM +0000, Brian Russell wrote:
>> 
>> 
>>> On 23/03/15 20:41, Greg Kroah-Hartman wrote:
>>>> On Fri, Mar 20, 2015 at 02:54:44PM +0000, Brian Russell wrote:
>>>> Protect uio driver from its owner being unplugged while there are open fds.
>>>> Embed struct device in struct uio_device, use refcounting on device, free
>>>> uio_device on release.
>>>> info struct passed in uio_register_device can be freed on unregister, so null
>>>> out the field in uio_unregister_device and check accesses.
>>> 
>>> That's really not protecting anything except heavy-handed problems...
>>> 
>>> Look at the code:
>>> 
>>>> @@ -493,7 +499,7 @@ static unsigned int uio_poll(struct file *filep, poll_table *wait)
>>>>    struct uio_listener *listener = filep->private_data;
>>>>    struct uio_device *idev = listener->dev;
>>>> 
>>>> -    if (!idev->info->irq)
>>>> +    if (!idev->info || !idev->info->irq)
>>>>        return -EIO;
>>> 
>>> Great, you checked the irq value, but what if it changes the very next
>>> line:
>>> 
>>>>    poll_wait(filep, &idev->wait, wait);
>>> 
>>> Or any other line within this function?  Or any other function that you
>>> try to check the value for in the beginning...
>>> 
>>> This really isn't protecting anything "properly", sorry.  Either we
>>> don't care about it (hint, I don't think we really do), or we need to
>>> properly lock things and check, and protect, things that way.
>> 
>> The checks for irq value are already there. I added the checks for the
>> idev->info ptr and deliberately nulled it in uio_unregister_device as
>> the caller module may free uio_info after unregistering (dpdk's igb_uio
>> does anyway) and then release will be called later when fds are closed.
>> 
>> So I think I definitely need the check in uio_release. I didn't think
>> it hurt to return early from poll/read/write if we know the device
>> has been unregistered?
> 
> What is the final verdict on this patch ? We are seeing the crash in our
> system, and I would like to apply a 'final' patch if possible to get it
> fixed.
> 
It needs a bit more work. uio_info needs to live as long as the corresponding uio_device. Since they seem to always be 1:1, uio_info could embedded within uio_device (but then all the users of uio need changed) or uio_info could be a refcounted object.

Brian

> Thanks,
> Guenter

Re: [PATCH v9 2/2] uio: Fix uio driver to refcount device

[Jean-François Dagenais]

Hi guys,

We have hot-removal scenarios here which require exactly this fix. The
app above has open fds on /dev/uioN
and the current uio.c makes the kernel OOPS.

Any updates? I'd be happy to test a new patch.

On Mon, Jun 8, 2015 at 4:07 PM, Brian Russell <brussell@brocade.com> wrote:
>
>> On 8 Jun 2015, at 20:25, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>>> On Tue, Mar 24, 2015 at 12:59:22PM +0000, Brian Russell wrote:
>>>
>>>
>>>> On 23/03/15 20:41, Greg Kroah-Hartman wrote:
>>>>> On Fri, Mar 20, 2015 at 02:54:44PM +0000, Brian Russell wrote:
>>>>> Protect uio driver from its owner being unplugged while there are open fds.
>>>>> Embed struct device in struct uio_device, use refcounting on device, free
>>>>> uio_device on release.
>>>>> info struct passed in uio_register_device can be freed on unregister, so null
>>>>> out the field in uio_unregister_device and check accesses.
>>>>
>>>> That's really not protecting anything except heavy-handed problems...
>>>>
>>>> Look at the code:
>>>>
>>>>> @@ -493,7 +499,7 @@ static unsigned int uio_poll(struct file *filep, poll_table *wait)
>>>>>    struct uio_listener *listener = filep->private_data;
>>>>>    struct uio_device *idev = listener->dev;
>>>>>
>>>>> -    if (!idev->info->irq)
>>>>> +    if (!idev->info || !idev->info->irq)
>>>>>        return -EIO;
>>>>
>>>> Great, you checked the irq value, but what if it changes the very next
>>>> line:
>>>>
>>>>>    poll_wait(filep, &idev->wait, wait);
>>>>
>>>> Or any other line within this function?  Or any other function that you
>>>> try to check the value for in the beginning...
>>>>
>>>> This really isn't protecting anything "properly", sorry.  Either we
>>>> don't care about it (hint, I don't think we really do), or we need to
>>>> properly lock things and check, and protect, things that way.
>>>
>>> The checks for irq value are already there. I added the checks for the
>>> idev->info ptr and deliberately nulled it in uio_unregister_device as
>>> the caller module may free uio_info after unregistering (dpdk's igb_uio
>>> does anyway) and then release will be called later when fds are closed.
>>>
>>> So I think I definitely need the check in uio_release. I didn't think
>>> it hurt to return early from poll/read/write if we know the device
>>> has been unregistered?
>>
>> What is the final verdict on this patch ? We are seeing the crash in our
>> system, and I would like to apply a 'final' patch if possible to get it
>> fixed.
>>
> It needs a bit more work. uio_info needs to live as long as the corresponding uio_device. Since they seem to always be 1:1, uio_info could embedded within uio_device (but then all the users of uio need changed) or uio_info could be a refcounted object.
>
> Brian
>
>> Thanks,
>> Guenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/