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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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