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

[PATCH 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 2/2] uio: Fix uio driver to refcount device

[Greg Kroah-Hartman]

On Thu, Mar 19, 2015 at 05:55:28PM +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.
> 
> 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(-)

This patch is line-wrapped and can not be applied :(

Please fix up your email client and resend.  I've applied the first
patch, which worked just fine, no need to resend it.

thanks,

greg k-h

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

[Brian Russell]

On 20/03/15 12:18, Greg Kroah-Hartman wrote:
> On Thu, Mar 19, 2015 at 05:55:28PM +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.
>>
>> 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(-)
> 
> This patch is line-wrapped and can not be applied :(
> 
> Please fix up your email client and resend.  I've applied the first
> patch, which worked just fine, no need to resend it.
> 

Sorry about that, I've resent. Thanks for the review.

Brian

> thanks,
> 
> greg k-h
>