lkml.org 
[lkml]   [2008]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/4] UIO: hold a reference to the device's owner while the device is open
On Thu, Apr 10, 2008 at 02:37:00PM +0200, Uwe Kleine-König wrote:
> Otherwise the device might just disappear while /dev/uioX is being used
> which results in an Oops.

Hi Uwe,
thanks for this one, good catch! Looks fine to me. There are some minor issues, see
below.
And I'd like to hear Greg's opinion: Do you agree we can omit
try_module_get() in uio_mmap()?

Thanks,
Hans

>
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
> ---
> drivers/uio/uio.c | 40 +++++++++++++++++++++++-----------------
> 1 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 1175908..005fc55 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -301,25 +301,35 @@ static int uio_open(struct inode *inode, struct file *filep)
> if (!idev)
> return -ENODEV;
>
> + if (!try_module_get(idev->owner)) {
> + ret = -ENODEV;
> + goto err_module_get;
> + }
> +
> listener = kmalloc(sizeof(*listener), GFP_KERNEL);
> - if (!listener)
> - return -ENOMEM;
> + if (!listener) {
> + ret = -ENOMEM;
> + goto err_alloc_listener;
> + }
>
> listener->dev = idev;
> listener->event_count = atomic_read(&idev->event);
> filep->private_data = listener;
>
> if (idev->info->open) {
> - if (!try_module_get(idev->owner))
> - return -ENODEV;
> ret = idev->info->open(idev->info, inode);
> - module_put(idev->owner);
> - }
> + if (ret) {
> + kfree(listener);
> +err_alloc_listener:
>
> - if (ret)
> - kfree(listener);
> + module_put(idev->owner);
> +err_module_get:
>
> - return ret;
> + return ret;
> + }
> + }
> +
> + return 0;
> }

I really don't like these labels inside the if-block. I find it hard to
read. What about this:


if (idev->info->open) {
ret = idev->info->open(idev->info, inode);
if (ret)
kfree(listener);
return ret;
}

err_alloc_listener:
module_put(idev->owner);
err_module_get:
return ret;



The label err_module_get should probably be omitted because it's used only
once and has just one line of code. You could simply write "return ret"
instead of "goto err_module_get".

}


>
> static int uio_fasync(int fd, struct file *filep, int on)
> @@ -336,12 +346,11 @@ 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 (!try_module_get(idev->owner))
> - return -ENODEV;
> + if (idev->info->release)
> ret = idev->info->release(idev->info, inode);
> - module_put(idev->owner);
> - }
> +
> + module_put(idev->owner);
> +
> if (filep->f_flags & FASYNC)
> ret = uio_fasync(-1, filep, 0);
> kfree(listener);
> @@ -510,10 +519,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
> return -EINVAL;
>
> if (idev->info->mmap) {
> - if (!try_module_get(idev->owner))
> - return -ENODEV;
> ret = idev->info->mmap(idev->info, vma);
> - module_put(idev->owner);
> return ret;
> }
>
> --
> 1.5.4.5
--
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/

\
 
 \ /
  Last update: 2008-04-10 23:05    [W:0.753 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site