Re: [PATCH -mm] media: video: usbvision: add mutex_unlock() to error paths

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 11 Dec 2007 17:56:50 -0800 Daniel Walker <dwalker@mvista.com> wrote:

> There are a few error paths which don't unlock the usbvision->lock.
> 
> So I've added mutex_unlock() calls to fix those paths.
> 
> Signed-off-by: Daniel Walker <dwalker@mvista.com>
> 
> ---
>  drivers/media/video/usbvision/usbvision-video.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: linux-2.6.23/drivers/media/video/usbvision/usbvision-video.c
> ===================================================================
> --- linux-2.6.23.orig/drivers/media/video/usbvision/usbvision-video.c
> +++ linux-2.6.23/drivers/media/video/usbvision/usbvision-video.c
> @@ -1290,6 +1290,7 @@ static int usbvision_radio_open(struct i
>  		errCode = usbvision_set_alternate(usbvision);
>  		if (errCode < 0) {
>  			usbvision->last_error = errCode;
> +			mutex_unlock(&usbvision->lock);
>  			return -EBUSY;
>  		}
>  
> @@ -1806,6 +1807,7 @@ static int __devinit usbvision_probe(str
>  					      usbvision->num_alt,GFP_KERNEL);
>  	if (usbvision->alt_max_pkt_size == NULL) {
>  		err("usbvision: out of memory!\n");
> +		mutex_unlock(&usbvision->lock);
>  		return -ENOMEM;
>  	}
>  

Well yes.  But the bug which you've found is a *direct* consequence of a
coding mistake in those functions: they have multiple deeply-nested `return'
statements.  This is a common cause of locking errors and resource leaks and
is why we prefer the `goto place-which-unwinds' approach.

We can easily fix that in one case, at least.

One does wonder whether usbvision_radio_open() should be returning the
usbvision_set_alternate() result to the caller here, rather than
overwriting it with -EBUSY.


--- a/drivers/media/video/usbvision/usbvision-video.c~media-video-usbvision-add-mutex_unlock-to-error-paths-fix
+++ a/drivers/media/video/usbvision/usbvision-video.c
@@ -1290,8 +1290,8 @@ static int usbvision_radio_open(struct i
 		errCode = usbvision_set_alternate(usbvision);
 		if (errCode < 0) {
 			usbvision->last_error = errCode;
-			mutex_unlock(&usbvision->lock);
-			return -EBUSY;
+			errCode = -EBUSY;
+			goto out;
 		}
 
 		// If so far no errors then we shall start the radio
@@ -1308,6 +1308,7 @@ static int usbvision_radio_open(struct i
 			usbvision->initialized = 0;
 		}
 	}
+out:
 	mutex_unlock(&usbvision->lock);
 	return errCode;
 }
_