au0828_v4l2_device_register()

Commit Message

Shuah Khan March 28, 2016, 5:08 p.m. UTC

Hi Mauro/Javier,

I can't figure out when au0828_v4l2_device_register() was added. Must be in
Linux 4.5 as I can't find this change in Linux 4.4 This used to be a call to
v4l2_device_register() from au0828_usb_probe(). When the code was moved, locking
bugs are introduced.

Notice that au0828_v4l2_device_register() does the following in error legs:

                mutex_unlock(&dev->lock);
                kfree(dev);


And au0828_usb_probe() also does the same cleanup when au0828_v4l2_device_register()
returns error:

        retval = au0828_v4l2_device_register(interface, dev);
        if (retval) {
                au0828_usb_v4l2_media_release(dev);
                mutex_unlock(&dev->lock);
                kfree(dev);
                return retval;
        }

We could be seeing some problems if this fails.

Please let me know if you would like a patch to fix this.

The following is the right fix:



thanks,
-- Shuah

Comments

Mauro Carvalho Chehab March 28, 2016, 6:18 p.m. UTC | #1

Hi Shuah,

Em Mon, 28 Mar 2016 11:08:09 -0600
Shuah Khan <[e-mail]> escreveu:

> Hi Mauro/Javier,
> 
> I can't figure out when au0828_v4l2_device_register() was added. Must be in
> Linux 4.5 as I can't find this change in Linux 4.4 This used to be a call to
> v4l2_device_register() from au0828_usb_probe(). When the code was moved, locking
> bugs are introduced.

I guess this was introduced before 4.5

> 
> Notice that au0828_v4l2_device_register() does the following in error legs:
> 
>                 mutex_unlock(&dev->lock);
>                 kfree(dev);
> 
> 
> And au0828_usb_probe() also does the same cleanup when au0828_v4l2_device_register()
> returns error:
> 
>         retval = au0828_v4l2_device_register(interface, dev);
>         if (retval) {
>                 au0828_usb_v4l2_media_release(dev);
>                 mutex_unlock(&dev->lock);
>                 kfree(dev);
>                 return retval;
>         }
> 
> We could be seeing some problems if this fails.
> 
> Please let me know if you would like a patch to fix this.

Yes, sure!

Regards,
Mauro

> 
> The following is the right fix:
> 
> diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c
> index 32d7db9..7d0ec4c 100644
> --- a/drivers/media/usb/au0828/au0828-video.c
> +++ b/drivers/media/usb/au0828/au0828-video.c
> @@ -679,8 +679,6 @@ int au0828_v4l2_device_register(struct usb_interface *interface,
>         if (retval) {
>                 pr_err("%s() v4l2_device_register failed\n",
>                        __func__);
> -               mutex_unlock(&dev->lock);
> -               kfree(dev);
>                 return retval;
>         }
>  
> @@ -691,8 +689,6 @@ int au0828_v4l2_device_register(struct usb_interface *interface,
>         if (retval) {
>                 pr_err("%s() v4l2_ctrl_handler_init failed\n",
>                        __func__);
> -               mutex_unlock(&dev->lock);
> -               kfree(dev);
>                 return retval;
>         }
>         dev->v4l2_dev.ctrl_handler = &dev->v4l2_ctrl_hdl;
> 
> 
> thanks,
> -- Shuah
>

Patch

diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c
index 32d7db9..7d0ec4c 100644
--- a/drivers/media/usb/au0828/au0828-video.c
+++ b/drivers/media/usb/au0828/au0828-video.c
@@ -679,8 +679,6 @@  int au0828_v4l2_device_register(struct usb_interface *interface,
        if (retval) {
                pr_err("%s() v4l2_device_register failed\n",
                       __func__);
-               mutex_unlock(&dev->lock);
-               kfree(dev);
                return retval;
        }
 
@@ -691,8 +689,6 @@  int au0828_v4l2_device_register(struct usb_interface *interface,
        if (retval) {
                pr_err("%s() v4l2_ctrl_handler_init failed\n",
                       __func__);
-               mutex_unlock(&dev->lock);
-               kfree(dev);
                return retval;
        }
        dev->v4l2_dev.ctrl_handler = &dev->v4l2_ctrl_hdl;