linux-kernel - Re: [PATCH 1/4] UIO: hold a reference to the device's owner while the device is open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080410210229.GF3193@local>
Date:	Thu, 10 Apr 2008 23:02:29 +0200
From:	"Hans J. Koch" <hjk@...utronix.de>
To:	Uwe Kleine-König <Uwe.Kleine-Koenig@...i.com>
Cc:	"Hans J. Koch" <hjk@...utronix.de>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	linux-kernel@...r.kernel.org
Subject: Re: [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@...i.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/