usb: gadget: f_midi: missing unlock on error path

PATCH In review Dan Carpenter 5 participants 11 messages Jan 5, 2016 – Jan 6, 2016

Subsystems (2)

USB Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2016-01-05 10:28 UTC #0 lore raw

PATCH v2 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]>

---
v2: Felipe asked for this to be fixed a different way.

drivers/usb/gadget/function/f_midi.c +8 -8

index fb1fe96d..7d28944 100644
@@ -1163,24 +1163,25 @@ static void f_midi_unbind(struct usb_configuration *c, struct usb_function *f)
 
 static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 {
-	struct f_midi *midi;
+	struct f_midi *midi = NULL;
 	struct f_midi_opts *opts;
-	int status, i;
+	int status;
+	int i = 0;
 
 	opts = container_of(fi, struct f_midi_opts, func_inst);
 
 	mutex_lock(&opts->lock);
 	/* sanity check */
 	if (opts->in_ports > MAX_PORTS || opts->out_ports > MAX_PORTS) {
-		mutex_unlock(&opts->lock);
-		return ERR_PTR(-EINVAL);
+		status = -EINVAL;
+		goto setup_fail;
 	}
 
 	/* allocate and initialize one new instance */
 	midi = kzalloc(sizeof(*midi), GFP_KERNEL);
 	if (!midi) {
-		mutex_unlock(&opts->lock);
-		return ERR_PTR(-ENOMEM);
+		status = -ENOMEM;
+		goto setup_fail;
 	}
 
 	for (i = 0; i < opts->in_ports; i++) {

@@ -1188,7 +1189,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 
 		if (!port) {
 			status = -ENOMEM;
-			mutex_unlock(&opts->lock);
 			goto setup_fail;
 		}

@@ -1202,7 +1202,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	midi->id = kstrdup(opts->id, GFP_KERNEL);
 	if (opts->id && !midi->id) {
 		status = -ENOMEM;
-		mutex_unlock(&opts->lock);
 		goto setup_fail;
 	}
 	midi->in_ports = opts->in_ports;

@@ -1229,6 +1228,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	return &midi->func;
 
 setup_fail:
+	mutex_unlock(&opts->lock);
 	for (--i; i >= 0; i--)
 		kfree(midi->in_port[i]);
 	kfree(midi);

kbuild test robot

build bot

2016-01-05 11:53 UTC #1 lore raw

coccinelle warnings: (new ones prefixed by >>)

Hi Dan,

[auto build test WARNING on balbi-usb/next]
[also build test WARNING on v4.4-rc8 next-20160105]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Dan-Carpenter/usb-gadget-f_midi-missing-unlock-on-error-path/20160105-183115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next

coccinelle warnings: (new ones prefixed by >>)

>> drivers/usb/gadget/function/f_midi.c:1233:14-21: ERROR: midi is NULL but dereferenced.

vim +1233 drivers/usb/gadget/function/f_midi.c

e1e3d7ec Felipe F. Tonello     2015-12-01  1217  
6f1de344 Andrzej Pietrasiewicz 2014-10-16  1218  	++opts->refcnt;
6f1de344 Andrzej Pietrasiewicz 2014-10-16  1219  	mutex_unlock(&opts->lock);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1220  
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1221  	midi->func.name		= "gmidi function";
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1222  	midi->func.bind		= f_midi_bind;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1223  	midi->func.unbind	= f_midi_unbind;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1224  	midi->func.set_alt	= f_midi_set_alt;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1225  	midi->func.disable	= f_midi_disable;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1226  	midi->func.free_func	= f_midi_free;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1227  
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1228  	return &midi->func;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1229  
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1230  setup_fail:
39920a18 Dan Carpenter         2016-01-05  1231  	mutex_unlock(&opts->lock);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1232  	for (--i; i >= 0; i--)
b85e9de9 Andrzej Pietrasiewicz 2014-10-16 @1233  		kfree(midi->in_port[i]);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1234  	kfree(midi);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1235  	return ERR_PTR(status);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1236  }

b85e9de9 Andrzej Pietrasiewicz 2014-10-16 1237
b85e9de9 Andrzej Pietrasiewicz 2014-10-16 1238 DECLARE_USB_FUNCTION_INIT(midi, f_midi_alloc_inst, f_midi_alloc);

:::::: The code at line 1233 was first introduced by commit
:::::: b85e9de9e818de0dcbc50b7b4242192eb6194855 usb: gadget: f_midi: convert to new function interface with backward compatibility

:::::: TO: Andrzej Pietrasiewicz <[email protected]> :::::: CC: Felipe Balbi <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Julia Lawall

2016-01-05 12:28 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 6 lines · kbuild test robot

Hi Dan,
[auto build test WARNING on balbi-usb/next]
[also build test WARNING on v4.4-rc8 next-20160105]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

Quoted 7 lines

url:    https://github.com/0day-ci/linux/commits/Dan-Carpenter/usb-gadget-f_midi-missing-unlock-on-error-path/20160105-183115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
coccinelle warnings: (new ones prefixed by >>)
Quoted 1 line
Quoted 1 line
drivers/usb/gadget/function/f_midi.c:1233:14-21: ERROR: midi is NULL but dereferenced.

It's a false positive for coccinelle, but I wonder if avoiding duplicating the mutex_lock is really worth it? There is a slightly subtle interaction between the possibility of midi being NULL and the value of i that make it all work.

julia

Quoted 40 lines

vim +1233 drivers/usb/gadget/function/f_midi.c
e1e3d7ec Felipe F. Tonello     2015-12-01  1217
6f1de344 Andrzej Pietrasiewicz 2014-10-16  1218  	++opts->refcnt;
6f1de344 Andrzej Pietrasiewicz 2014-10-16  1219  	mutex_unlock(&opts->lock);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1220
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1221  	midi->func.name		= "gmidi function";
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1222  	midi->func.bind		= f_midi_bind;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1223  	midi->func.unbind	= f_midi_unbind;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1224  	midi->func.set_alt	= f_midi_set_alt;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1225  	midi->func.disable	= f_midi_disable;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1226  	midi->func.free_func	= f_midi_free;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1227
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1228  	return &midi->func;
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1229
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1230  setup_fail:
39920a18 Dan Carpenter         2016-01-05  1231  	mutex_unlock(&opts->lock);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1232  	for (--i; i >= 0; i--)
b85e9de9 Andrzej Pietrasiewicz 2014-10-16 @1233  		kfree(midi->in_port[i]);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1234  	kfree(midi);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1235  	return ERR_PTR(status);
b85e9de9 Andrzej Pietrasiewicz 2014-10-16  1236  }
b85e9de9 Andrzej Pietrasiewicz 2014-10-16 1237
b85e9de9 Andrzej Pietrasiewicz 2014-10-16 1238 DECLARE_USB_FUNCTION_INIT(midi, f_midi_alloc_inst, f_midi_alloc);
:::::: The code at line 1233 was first introduced by commit
:::::: b85e9de9e818de0dcbc50b7b4242192eb6194855 usb: gadget: f_midi: convert to new function interface with backward compatibility
:::::: TO: Andrzej Pietrasiewicz <[email protected]> :::::: CC: Felipe Balbi <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2016-01-05 12:37 UTC #3 lore raw

In reply to Dan Carpenter

It's a false positive, if midi is NULL then i starts as zero so it won't go inside the for loop. Smatch has the same false positive.

regards,
dan carpenter

Dan Carpenter

author

2016-01-05 12:44 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 14 lines · Julia Lawall

Quoted 10 lines · kbuild test robot
Hi Dan,
[auto build test WARNING on balbi-usb/next]
[also build test WARNING on v4.4-rc8 next-20160105]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url:    https://github.com/0day-ci/linux/commits/Dan-Carpenter/usb-gadget-f_midi-missing-unlock-on-error-path/20160105-183115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next

Quoted 6 lines

Quoted 3 lines
coccinelle warnings: (new ones prefixed by >>)
Quoted 1 line
Quoted 1 line
drivers/usb/gadget/function/f_midi.c:1233:14-21: ERROR: midi is NULL but dereferenced.
It's a false positive for coccinelle, but I wonder if avoiding duplicating the mutex_lock is really worth it?

It's not the most beautiful code in the world. I considered a bunch of different ways to write it... This is what Felipe Tonello wanted, though.

regards,
dan carpenter

Michal Nazarewicz

Acked-by on v2 1/1

2016-01-05 13:55 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 5 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]>

Acked-by Michal Nazarewicz <[email protected]>

Quoted 56 lines

---
v2: Felipe asked for this to be fixed a different way.

drivers/usb/gadget/function/f_midi.c +7 -8

index fb1fe96d..7d28944 100644
@@ -1163,24 +1163,25 @@ static void f_midi_unbind(struct usb_configuration *c, struct usb_function *f)
 
 static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 {
-	struct f_midi *midi;
+	struct f_midi *midi = NULL;
 	struct f_midi_opts *opts;
-	int status, i;
+	int status;
+	int i = 0;
 
 	opts = container_of(fi, struct f_midi_opts, func_inst);
 
 	mutex_lock(&opts->lock);
 	/* sanity check */
 	if (opts->in_ports > MAX_PORTS || opts->out_ports > MAX_PORTS) {
-		mutex_unlock(&opts->lock);
-		return ERR_PTR(-EINVAL);
+		status = -EINVAL;
+		goto setup_fail;
 	}
 
 	/* allocate and initialize one new instance */
 	midi = kzalloc(sizeof(*midi), GFP_KERNEL);
 	if (!midi) {
-		mutex_unlock(&opts->lock);
-		return ERR_PTR(-ENOMEM);
+		status = -ENOMEM;
+		goto setup_fail;
 	}
 
 	for (i = 0; i < opts->in_ports; i++) {

@@ -1188,7 +1189,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 
 		if (!port) {
 			status = -ENOMEM;
-			mutex_unlock(&opts->lock);
 			goto setup_fail;
 		}

@@ -1202,7 +1202,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	midi->id = kstrdup(opts->id, GFP_KERNEL);
 	if (opts->id && !midi->id) {
 		status = -ENOMEM;
-		mutex_unlock(&opts->lock);
 		goto setup_fail;
 	}
 	midi->in_ports = opts->in_ports;

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

Quoted 7 lines

drivers/usb/gadget/function/f_midi.c +1 -0

@@ -1229,6 +1228,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	return &midi->func;
 
 setup_fail:
+	mutex_unlock(&opts->lock);
 	for (--i; i >= 0; i--)
 		kfree(midi->in_port[i]);
 	kfree(midi);

How about some refactoring first:

---- >8 ---------------------------------------------------------------- From 81220372e4acce8f1ffee00338c24472469c1abe Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz <[email protected]>
Date: Tue, 5 Jan 2016 14:43:42 +0100
Subject: [PATCH 1/2] usb: gadget: f_midi: use flexible array member for
gmidi_in_port elements

Reduce number of allocations, simplify memory management and reduce memory usage by stacking the gmidi_in_port elements at the end of the f_midi structure using a flexible array.

Also, observe that gmidi_in_port::midi pointer is *never* used for any purpose so it can be safely removed.

Signed-off-by Michal Nazarewicz <[email protected]>

---
 drivers/usb/gadget/function/f_midi.c | 42 ++++++++++++------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

drivers/usb/gadget/function/f_midi.c +14 ~~-28~~

index 898a570..9338625 100644
@@ -55,7 +55,6 @@ static const char f_midi_longname[] = "MIDI Gadget";
  * USB <- IN endpoint  <- rawmidi
  */
 struct gmidi_in_port {
-	struct f_midi *midi;
 	int active;
 	uint8_t cable;
 	uint8_t state;

@@ -78,7 +77,6 @@ struct f_midi {
 
 	struct snd_rawmidi_substream *in_substream[MAX_PORTS];
 	struct snd_rawmidi_substream *out_substream[MAX_PORTS];
-	struct gmidi_in_port	*in_port[MAX_PORTS];
 
 	unsigned long		out_triggered;
 	struct tasklet_struct	tasklet;

@@ -87,6 +85,8 @@ struct f_midi {
 	int index;
 	char *id;
 	unsigned int buflen, qlen;
+
+	struct gmidi_in_port	in_ports_array[/* in_ports */];
 };
 
 static inline struct f_midi *func_to_midi(struct usb_function *f)

@@ -529,11 +529,11 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
 	req->length = 0;
 	req->complete = f_midi_complete;
 
-	for (i = 0; i < MAX_PORTS; i++) {
-		struct gmidi_in_port *port = midi->in_port[i];
+	for (i = 0; i < midi->in_ports; i++) {
+		struct gmidi_in_port *port = midi->in_ports_array + i;
 		struct snd_rawmidi_substream *substream = midi->in_substream[i];
 
-		if (!port || !port->active || !substream)
+		if (!port->active || !substream)
 			continue;
 
 		while (req->length + 3 < midi->buflen) {

@@ -568,12 +568,12 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
 {
 	struct f_midi *midi = substream->rmidi->private_data;
 
-	if (!midi->in_port[substream->number])
+	if (substream->number > midi->in_ports)
 		return -EINVAL;
 
 	VDBG(midi, "%s()\n", __func__);
 	midi->in_substream[substream->number] = substream;
-	midi->in_port[substream->number]->state = STATE_UNKNOWN;
+	midi->in_ports_array[substream->number].state = STATE_UNKNOWN;
 	return 0;
 }

@@ -589,11 +589,11 @@ static void f_midi_in_trigger(struct snd_rawmidi_substream *substream, int up)
 {
 	struct f_midi *midi = substream->rmidi->private_data;
 
-	if (!midi->in_port[substream->number])
+	if (substream->number > midi->in_ports)
 		return;
 
 	VDBG(midi, "%s() %d\n", __func__, up);
-	midi->in_port[substream->number]->active = up;
+	midi->in_ports_array[substream->number].active = up;
 	if (up)
 		tasklet_hi_schedule(&midi->tasklet);
 }

@@ -1073,8 +1073,6 @@ static void f_midi_free(struct usb_function *f)
 	opts = container_of(f->fi, struct f_midi_opts, func_inst);
 	kfree(midi->id);
 	mutex_lock(&opts->lock);
-	for (i = opts->in_ports - 1; i >= 0; --i)
-		kfree(midi->in_port[i]);
 	kfree(midi);
 	--opts->refcnt;
 	mutex_unlock(&opts->lock);

@@ -1115,26 +1113,16 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	}
 
 	/* allocate and initialize one new instance */
-	midi = kzalloc(sizeof(*midi), GFP_KERNEL);
+	midi = kzalloc(
+		sizeof(*midi) + opts->in_ports * sizeof(*midi->in_ports_array),
+		GFP_KERNEL);
 	if (!midi) {
 		mutex_unlock(&opts->lock);
 		return ERR_PTR(-ENOMEM);
 	}
 
-	for (i = 0; i < opts->in_ports; i++) {
-		struct gmidi_in_port *port = kzalloc(sizeof(*port), GFP_KERNEL);
-
-		if (!port) {
-			status = -ENOMEM;
-			mutex_unlock(&opts->lock);
-			goto setup_fail;
-		}
-
-		port->midi = midi;
-		port->active = 0;
-		port->cable = i;
-		midi->in_port[i] = port;
-	}
+	for (i = 0; i < opts->in_ports; i++)
+		midi->in_ports_array[i].cable = i;
 
 	/* set up ALSA midi devices */
 	midi->id = kstrdup(opts->id, GFP_KERNEL);

@@ -1161,8 +1149,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	return &midi->func;
 
 setup_fail:
-	for (--i; i >= 0; i--)
-		kfree(midi->in_port[i]);
 	kfree(midi);
 	return ERR_PTR(status);
 }

---- >8 ---------------------------------------------------------------- From 57bbb33864f7480c15dfeea627d3589775ca2491 Mon Sep 17 00:00:00 2001
From: Dan Carpenter <[email protected]>
Date: Tue, 5 Jan 2016 13:28:09 +0300
Subject: [PATCH 2/2] 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]>

[[email protected]: rebased on top of refactoring patch!

Signed-off-by Michal Nazarewicz <[email protected]>

---
 drivers/usb/gadget/function/f_midi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/usb/gadget/function/f_midi.c +6 -6

index 9338625..de0bac5 100644
@@ -1099,7 +1099,7 @@ static void f_midi_unbind(struct usb_configuration *c, struct usb_function *f)
 
 static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 {
-	struct f_midi *midi;
+	struct f_midi *midi = NULL;
 	struct f_midi_opts *opts;
 	int status, i;

@@ -1108,8 +1108,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	mutex_lock(&opts->lock);
 	/* sanity check */
 	if (opts->in_ports > MAX_PORTS || opts->out_ports > MAX_PORTS) {
-		mutex_unlock(&opts->lock);
-		return ERR_PTR(-EINVAL);
+		status = -EINVAL;
+		goto setup_fail;
 	}
 
 	/* allocate and initialize one new instance */

@@ -1117,8 +1117,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 		sizeof(*midi) + opts->in_ports * sizeof(*midi->in_ports_array),
 		GFP_KERNEL);
 	if (!midi) {
-		mutex_unlock(&opts->lock);
-		return ERR_PTR(-ENOMEM);
+		status = -ENOMEM;
+		goto setup_fail;
 	}
 
 	for (i = 0; i < opts->in_ports; i++)

@@ -1128,7 +1128,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	midi->id = kstrdup(opts->id, GFP_KERNEL);
 	if (opts->id && !midi->id) {
 		status = -ENOMEM;
-		mutex_unlock(&opts->lock);
 		goto setup_fail;
 	}
 	midi->in_ports = opts->in_ports;

@@ -1149,6 +1148,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	return &midi->func;
 
 setup_fail:
+	mutex_unlock(&opts->lock);
 	kfree(midi);
 	return ERR_PTR(status);
 }

Signature · 4 lines

Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--

Dan Carpenter

author

2016-01-05 14:03 UTC #6 lore raw

In reply to Dan Carpenter

Review static int f_midi_in_open(struct snd_rawmidi_substream *substream) · line 571

Quoted 6 lines · Michal Nazarewicz

(quoted diff) +1 -1

@@ -568,12 +568,12 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
 {
 	struct f_midi *midi = substream->rmidi->private_data;
 
-	if (!midi->in_port[substream->number])
+	if (substream->number > midi->in_ports)

This is off by one. It should be >= midi->in_ports.

regards,
dan carpenter

Felipe Ferreri Tonello

2016-01-05 20:51 UTC #7 lore raw

In reply to Dan Carpenter

Hi Dan,

Quoted 17 lines · Dan Carpenter

Quoted 16 lines · Julia Lawall
Quoted 12 lines · kbuild test robot
Hi Dan,
[auto build test WARNING on balbi-usb/next]
[also build test WARNING on v4.4-rc8 next-20160105]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url:    https://github.com/0day-ci/linux/commits/Dan-Carpenter/usb-gadget-f_midi-missing-unlock-on-error-path/20160105-183115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
coccinelle warnings: (new ones prefixed by >>)

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/usb/gadget/function/f_midi.c:1233:14-21: ERROR: midi is NULL but dereferenced.
It's a false positive for coccinelle, but I wonder if avoiding duplicating the mutex_lock is really worth it?
It's not the most beautiful code in the world. I considered a bunch of different ways to write it... This is what Felipe Tonello wanted, though.

This case is not a matter of been pretty but a matter of been less error prone.

What would you suggest?

Thanks,
Felipe

Julia Lawall

2016-01-05 21:21 UTC #8 lore raw

In reply to Dan Carpenter

Quoted 25 lines · Felipe Ferreri Tonello

Hi Dan,
Quoted 22 lines · Dan Carpenter
Quoted 20 lines · Julia Lawall
Quoted 6 lines · kbuild test robot
Hi Dan,
[auto build test WARNING on balbi-usb/next]
[also build test WARNING on v4.4-rc8 next-20160105]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
Quoted 7 lines
url:    https://github.com/0day-ci/linux/commits/Dan-Carpenter/usb-gadget-f_midi-missing-unlock-on-error-path/20160105-183115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
coccinelle warnings: (new ones prefixed by >>)
Quoted 1 line
Quoted 1 line
drivers/usb/gadget/function/f_midi.c:1233:14-21: ERROR: midi is NULL but dereferenced.
It's a false positive for coccinelle, but I wonder if avoiding duplicating the mutex_lock is really worth it?

Quoted 8 lines

Quoted 3 lines
It's not the most beautiful code in the world. I considered a bunch of different ways to write it... This is what Felipe Tonello wanted, though.
This case is not a matter of been pretty but a matter of been less error prone.
What would you suggest?

I thought to be a little less subtle about midi, it would be easier to keep the first few mutex_unlocks up where they were. But up to you.

julia

Dan Carpenter

author

2016-01-05 22:08 UTC #9 lore raw

In reply to Dan Carpenter

Quoted 4 lines · Felipe Ferreri Tonello

This case is not a matter of been pretty but a matter of been less error prone.
What would you suggest?

Normally it's better to unwind in the reverse order from how we allocated so it would be:

lock
allocate midi
allocate ports

free ports
free midi
unlock

We could move the midi allocation outside the lock, but we can't move ports allocation. And also we want to drop the lock as soon as we can so it's better to do that early like my patch does instead of after the frees. It's less symetric that way and thus more error prone but it's better for performance.

Anyway, I don't think it really matters, this is a minor thing.

Also I hope that Smatch will be able to avoid that false positive about the midi dereference by the end of 2016. :)

regards,
dan carpenter

Michal Nazarewicz

2016-01-06 00:02 UTC #10 lore raw

In reply to Dan Carpenter

Quoted 13 lines · Dan Carpenter

Quoted 4 lines · Felipe Ferreri Tonello
This case is not a matter of been pretty but a matter of been less error prone.
What would you suggest?
Normally it's better to unwind in the reverse order from how we allocated so it would be:
lock
allocate midi
allocate ports

Quoted 6 lines

free ports
free midi
unlock
We could move the midi allocation outside the lock, but we can't move ports allocation.

I still think the easiest way to go is getting rid of ports allocation all together. With that gone, it’s trivial to initialise midi to NULL and then we can do kfree whenever we want.

Quoted 9 lines

And also we want to drop the lock as soon as we can
so it's better to do that early like my patch does instead of after the frees. It's less symetric that way and thus more error prone but it's better for performance.
Anyway, I don't think it really matters, this is a minor thing.
Also I hope that Smatch will be able to avoid that false positive about the midi dereference by the end of 2016. :)

Signature · 4 lines

Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--