Skip to contentSkip to conversation

usb: gadget: f_midi: missing unlock on error path

PATCH Superseded Dan Carpenter 3 participants 3 messages Dec 21, 2015 – Dec 22, 2015

Subsystems (2)
USB Kernel janitors

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 usb: gadget: f_midi: missing unlock on error path

We added a new error path to this function and we forgot to drop the lock.

Fixes e1e3d7ec5da3 ('usb: gadget: f_midi: pre-allocate IN requests')
Signed-off-by Dan Carpenter <[email protected]>
drivers/usb/gadget/function/f_midi.c +3 -1
index fb1fe96d..92b9ec8 100644
@@ -1213,8 +1213,10 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	midi->in_last_port = 0;
 
 	status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
-	if (status)
+	if (status) {
+		mutex_unlock(&opts->lock);
 		goto setup_fail;
+	}
 
 	++opts->refcnt;
 	mutex_unlock(&opts->lock);
#1 lore raw

Hi Dan,

Quoted 11 lines · Dan Carpenter

We added a new error path to this function and we forgot to drop the lock.

Fixes e1e3d7ec5da3 ('usb: gadget: f_midi: pre-allocate IN requests')
Signed-off-by Dan Carpenter <[email protected]>
drivers/usb/gadget/function/f_midi.c +0 -0
index fb1fe96d..92b9ec8 100644

drivers/usb/gadget/function/f_midi.c static struct usb_function *f_midi_alloc(struct usb_function_instance *fi) · line 1217

Quoted 6 lines
drivers/usb/gadget/function/f_midi.c +2 -1
@@ -1213,8 +1213,10 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	midi->in_last_port = 0;
 
 	status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
-	if (status)
+	if (status) {
+		mutex_unlock(&opts->lock);

This is fine. But looking into this further, I believe this mutex_unlock should be under the setup_fail label, because even if the alloc() fails, it will be need to unlock the mutex in any circumstance anyway.

drivers/usb/gadget/function/f_midi.c static struct usb_function *f_midi_alloc(struct usb_function_instance *fi) · line 1222

Quoted 6 lines
drivers/usb/gadget/function/f_midi.c +1 -0
 		goto setup_fail;
+	}
 
 	++opts->refcnt;
 	mutex_unlock(&opts->lock);

Felipe

#2 lore raw
In reply to Dan Carpenter
Quoted 11 lines

We added a new error path to this function and we forgot to drop the lock.

Fixes e1e3d7ec5da3 ('usb: gadget: f_midi: pre-allocate IN requests')
Signed-off-by Dan Carpenter <[email protected]>
drivers/usb/gadget/function/f_midi.c +0 -0
index fb1fe96d..92b9ec8 100644
@@ -1213,8 +1213,10 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	midi->in_last_port = 0;

drivers/usb/gadget/function/f_midi.c static struct usb_function *f_midi_alloc(struct usb_function_instance *fi) · line 1222

Quoted 10 lines
drivers/usb/gadget/function/f_midi.c +3 -1
 
 	status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
-	if (status)
+	if (status) {
+		mutex_unlock(&opts->lock);
 		goto setup_fail;
+	}
 
 	++opts->refcnt;
 	mutex_unlock(&opts->lock);

How do you think about to move this unlock call and the one that belongs to the handling of a kstrdup() failure below the shown jump target?

Regards,
Markus