[patch v2] usb: gadget: f_midi: missing unlock on error path

[patch v2] usb: gadget: f_midi: missing unlock on error path

[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 <dan.carpenter@oracle.com>
---
v2: Felipe asked for this to be fixed a different way.

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index fb1fe96d..7d28944 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -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);

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[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 >>)

>> 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 <andrzej.p@samsung.com>
:::::: CC: Felipe Balbi <balbi@ti.com>

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

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[Julia Lawall]

On Tue, 5 Jan 2016, kbuild test robot wrote:

> 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.

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


>
> 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 <andrzej.p@samsung.com>
> :::::: CC: Felipe Balbi <balbi@ti.com>
>
> ---
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[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

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[Dan Carpenter]

On Tue, Jan 05, 2016 at 01:28:11PM +0100, Julia Lawall wrote:
> 
> 
> On Tue, 5 Jan 2016, kbuild test robot wrote:
> 
> > 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.
> 
> 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

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[Michal Nazarewicz]

On Tue, Jan 05 2016, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> ---
> v2: Felipe asked for this to be fixed a different way.
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index fb1fe96d..7d28944 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -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);

How about some refactoring first:

---- >8 ----------------------------------------------------------------
From 81220372e4acce8f1ffee00338c24472469c1abe Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz <mina86@mina86.com>
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 <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_midi.c | 42 ++++++++++++------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 898a570..9338625 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -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 <dan.carpenter@oracle.com>
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 <dan.carpenter@oracle.com>
[mina86@mina86.com: rebased on top of refactoring patch!
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_midi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 9338625..de0bac5 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -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);
 }

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

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[Dan Carpenter]

On Tue, Jan 05, 2016 at 02:55:31PM +0100, Michal Nazarewicz wrote:
> @@ -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

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[Felipe Ferreri Tonello]

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

Hi Dan,

On 05/01/16 12:44, Dan Carpenter wrote:
> On Tue, Jan 05, 2016 at 01:28:11PM +0100, Julia Lawall wrote:
>>
>>
>> On Tue, 5 Jan 2016, kbuild test robot wrote:
>>
>>> 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.
>>
>> 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

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[Julia Lawall]

On Tue, 5 Jan 2016, Felipe Ferreri Tonello wrote:

> Hi Dan,
> 
> On 05/01/16 12:44, Dan Carpenter wrote:
> > On Tue, Jan 05, 2016 at 01:28:11PM +0100, Julia Lawall wrote:
> >>
> >>
> >> On Tue, 5 Jan 2016, kbuild test robot wrote:
> >>
> >>> 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.
> >>
> >> 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?

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

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[Dan Carpenter]

On Tue, Jan 05, 2016 at 08:51:18PM +0000, Felipe Ferreri Tonello wrote:
> 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

Re: [patch v2] usb: gadget: f_midi: missing unlock on error path

[Michal Nazarewicz]

On Wed, Jan 06 2016, Dan Carpenter wrote:
> On Tue, Jan 05, 2016 at 08:51:18PM +0000, Felipe Ferreri Tonello wrote:
>> 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.

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.

> 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. :)

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