[PATCH] usb: gadget: f_midi: added spinlock on transmit function

[PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe F. Tonello]

Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
potentially cause a race condition between both calls because f_midi_transmit
is not reentrant nor thread-safe. This is due to an implementation detail that
the transmit function looks for the next available usb request from the fifo
and only enqueues it if there is data to send, otherwise just re-uses it. So,
if both ALSA and USB frameworks calls this function at the same time,
kfifo_seek() will return the same usb_request, which will cause a race
condition.

To solve this problem a syncronization mechanism is necessary. In this case it
is used a spinlock since f_midi_transmit is also called by usb_request->complete
callback in interrupt context.

Cc: <stable@vger.kernel.org> # v4.4+
Fixes: e1e3d7ec5da3 ("usb: gadget: f_midi: pre-allocate IN requests")
Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
Tested-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 3cdb0741f3f8..8475e3dc82d4 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/kfifo.h>
+#include <linux/spinlock.h>
 
 #include <sound/core.h>
 #include <sound/initval.h>
@@ -95,6 +96,7 @@ struct f_midi {
 	unsigned int buflen, qlen;
 	/* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
 	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
+	spinlock_t transmit_lock;
 	unsigned int in_last_port;
 
 	struct gmidi_in_port	in_ports_array[/* in_ports */];
@@ -651,17 +653,22 @@ static void f_midi_transmit(struct f_midi *midi)
 {
 	struct usb_ep *ep = midi->in_ep;
 	int ret;
+	unsigned long flags;
 
 	/* We only care about USB requests if IN endpoint is enabled */
 	if (!ep || !ep->enabled)
 		goto drop_out;
 
+	spin_lock_irqsave(&midi->transmit_lock, flags);
+
 	do {
 		ret = f_midi_do_transmit(midi, ep);
 		if (ret < 0)
 			goto drop_out;
 	} while (ret);
 
+	spin_unlock_irqrestore(&midi->transmit_lock, flags);
+
 	return;
 
 drop_out:
@@ -1255,6 +1262,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	if (status)
 		goto setup_fail;
 
+	spin_lock_init(&midi->transmit_lock);
+
 	++opts->refcnt;
 	mutex_unlock(&opts->lock);
 
-- 
2.7.2

Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Balbi]

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


Hi,

"Felipe F. Tonello" <eu@felipetonello.com> writes:
> [ text/plain ]
> Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
> potentially cause a race condition between both calls because f_midi_transmit
> is not reentrant nor thread-safe. This is due to an implementation detail that
> the transmit function looks for the next available usb request from the fifo
> and only enqueues it if there is data to send, otherwise just re-uses it. So,
> if both ALSA and USB frameworks calls this function at the same time,
> kfifo_seek() will return the same usb_request, which will cause a race
> condition.
>
> To solve this problem a syncronization mechanism is necessary. In this case it
> is used a spinlock since f_midi_transmit is also called by usb_request->complete
> callback in interrupt context.
>
> Cc: <stable@vger.kernel.org> # v4.4+
> Fixes: e1e3d7ec5da3 ("usb: gadget: f_midi: pre-allocate IN requests")
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> Tested-by: Felipe F. Tonello <eu@felipetonello.com>

adding one's own Tested-by is a bit redundant. I'll remove it while
applying ;-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Balbi]

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


Hi,

"Felipe F. Tonello" <eu@felipetonello.com> writes:
> [ text/plain ]
> Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
> potentially cause a race condition between both calls because f_midi_transmit
> is not reentrant nor thread-safe. This is due to an implementation detail that
> the transmit function looks for the next available usb request from the fifo
> and only enqueues it if there is data to send, otherwise just re-uses it. So,
> if both ALSA and USB frameworks calls this function at the same time,
> kfifo_seek() will return the same usb_request, which will cause a race
> condition.
>
> To solve this problem a syncronization mechanism is necessary. In this case it
> is used a spinlock since f_midi_transmit is also called by usb_request->complete
> callback in interrupt context.
>
> Cc: <stable@vger.kernel.org> # v4.4+

this should be v4.5+

$ git describe e1e3d7ec5da3
v4.4-rc5-59-ge1e3d7ec5da3

> @@ -651,17 +653,22 @@ static void f_midi_transmit(struct f_midi *midi)
>  {
>  	struct usb_ep *ep = midi->in_ep;
>  	int ret;
> +	unsigned long flags;
>  
>  	/* We only care about USB requests if IN endpoint is enabled */
>  	if (!ep || !ep->enabled)
>  		goto drop_out;
>  
> +	spin_lock_irqsave(&midi->transmit_lock, flags);
> +
>  	do {
>  		ret = f_midi_do_transmit(midi, ep);

you wrote this commit on top of 'next' right ? I know that because of
this call to f_midi_do_transmit() here which is introduced by commit
aac887442d5e ("usb: gadget: f_midi: move some of f_midi_transmit to
separate func") which is not in Linus' tree yet.

This prevents me from taking this patch during current -rc.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

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

Hi Balbi,

On 09/03/16 07:20, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello" <eu@felipetonello.com> writes:
>> [ text/plain ]
>> Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
>> potentially cause a race condition between both calls because f_midi_transmit
>> is not reentrant nor thread-safe. This is due to an implementation detail that
>> the transmit function looks for the next available usb request from the fifo
>> and only enqueues it if there is data to send, otherwise just re-uses it. So,
>> if both ALSA and USB frameworks calls this function at the same time,
>> kfifo_seek() will return the same usb_request, which will cause a race
>> condition.
>>
>> To solve this problem a syncronization mechanism is necessary. In this case it
>> is used a spinlock since f_midi_transmit is also called by usb_request->complete
>> callback in interrupt context.
>>
>> Cc: <stable@vger.kernel.org> # v4.4+
> 
> this should be v4.5+
> 
> $ git describe e1e3d7ec5da3
> v4.4-rc5-59-ge1e3d7ec5da3
> 
>> @@ -651,17 +653,22 @@ static void f_midi_transmit(struct f_midi *midi)
>>  {
>>  	struct usb_ep *ep = midi->in_ep;
>>  	int ret;
>> +	unsigned long flags;
>>  
>>  	/* We only care about USB requests if IN endpoint is enabled */
>>  	if (!ep || !ep->enabled)
>>  		goto drop_out;
>>  
>> +	spin_lock_irqsave(&midi->transmit_lock, flags);
>> +
>>  	do {
>>  		ret = f_midi_do_transmit(midi, ep);
> 
> you wrote this commit on top of 'next' right ? I know that because of
> this call to f_midi_do_transmit() here which is introduced by commit
> aac887442d5e ("usb: gadget: f_midi: move some of f_midi_transmit to
> separate func") which is not in Linus' tree yet.
> 
> This prevents me from taking this patch during current -rc.

Yes, but then what should I do? Because if I patch on Linus' tree, then
the patch won't apply to your tree.

-- 
Felipe

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

Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Balbi]

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


Hi,

Felipe Ferreri Tonello <eu@felipetonello.com> writes:
> [ text/plain ]
> Hi Balbi,
>
> On 09/03/16 07:20, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> "Felipe F. Tonello" <eu@felipetonello.com> writes:
>>> [ text/plain ]
>>> Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
>>> potentially cause a race condition between both calls because f_midi_transmit
>>> is not reentrant nor thread-safe. This is due to an implementation detail that
>>> the transmit function looks for the next available usb request from the fifo
>>> and only enqueues it if there is data to send, otherwise just re-uses it. So,
>>> if both ALSA and USB frameworks calls this function at the same time,
>>> kfifo_seek() will return the same usb_request, which will cause a race
>>> condition.
>>>
>>> To solve this problem a syncronization mechanism is necessary. In this case it
>>> is used a spinlock since f_midi_transmit is also called by usb_request->complete
>>> callback in interrupt context.
>>>
>>> Cc: <stable@vger.kernel.org> # v4.4+
>> 
>> this should be v4.5+
>> 
>> $ git describe e1e3d7ec5da3
>> v4.4-rc5-59-ge1e3d7ec5da3
>> 
>>> @@ -651,17 +653,22 @@ static void f_midi_transmit(struct f_midi *midi)
>>>  {
>>>  	struct usb_ep *ep = midi->in_ep;
>>>  	int ret;
>>> +	unsigned long flags;
>>>  
>>>  	/* We only care about USB requests if IN endpoint is enabled */
>>>  	if (!ep || !ep->enabled)
>>>  		goto drop_out;
>>>  
>>> +	spin_lock_irqsave(&midi->transmit_lock, flags);
>>> +
>>>  	do {
>>>  		ret = f_midi_do_transmit(midi, ep);
>> 
>> you wrote this commit on top of 'next' right ? I know that because of
>> this call to f_midi_do_transmit() here which is introduced by commit
>> aac887442d5e ("usb: gadget: f_midi: move some of f_midi_transmit to
>> separate func") which is not in Linus' tree yet.
>> 
>> This prevents me from taking this patch during current -rc.
>
> Yes, but then what should I do? Because if I patch on Linus' tree, then
> the patch won't apply to your tree.

it should apply to my "fixes" branch where fixes for current -rc should
go :-) Note that v4.5-rc7 doesn't have f_midi_do_transmit()

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

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

Hi Balbi,

On 09/03/16 10:33, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <eu@felipetonello.com> writes:
>> [ text/plain ]
>> Hi Balbi,
>>
>> On 09/03/16 07:20, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> "Felipe F. Tonello" <eu@felipetonello.com> writes:
>>>> [ text/plain ]
>>>> Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
>>>> potentially cause a race condition between both calls because f_midi_transmit
>>>> is not reentrant nor thread-safe. This is due to an implementation detail that
>>>> the transmit function looks for the next available usb request from the fifo
>>>> and only enqueues it if there is data to send, otherwise just re-uses it. So,
>>>> if both ALSA and USB frameworks calls this function at the same time,
>>>> kfifo_seek() will return the same usb_request, which will cause a race
>>>> condition.
>>>>
>>>> To solve this problem a syncronization mechanism is necessary. In this case it
>>>> is used a spinlock since f_midi_transmit is also called by usb_request->complete
>>>> callback in interrupt context.
>>>>
>>>> Cc: <stable@vger.kernel.org> # v4.4+
>>>
>>> this should be v4.5+
>>>
>>> $ git describe e1e3d7ec5da3
>>> v4.4-rc5-59-ge1e3d7ec5da3
>>>
>>>> @@ -651,17 +653,22 @@ static void f_midi_transmit(struct f_midi *midi)
>>>>  {
>>>>  	struct usb_ep *ep = midi->in_ep;
>>>>  	int ret;
>>>> +	unsigned long flags;
>>>>  
>>>>  	/* We only care about USB requests if IN endpoint is enabled */
>>>>  	if (!ep || !ep->enabled)
>>>>  		goto drop_out;
>>>>  
>>>> +	spin_lock_irqsave(&midi->transmit_lock, flags);
>>>> +
>>>>  	do {
>>>>  		ret = f_midi_do_transmit(midi, ep);
>>>
>>> you wrote this commit on top of 'next' right ? I know that because of
>>> this call to f_midi_do_transmit() here which is introduced by commit
>>> aac887442d5e ("usb: gadget: f_midi: move some of f_midi_transmit to
>>> separate func") which is not in Linus' tree yet.
>>>
>>> This prevents me from taking this patch during current -rc.
>>
>> Yes, but then what should I do? Because if I patch on Linus' tree, then
>> the patch won't apply to your tree.
> 
> it should apply to my "fixes" branch where fixes for current -rc should
> go :-) Note that v4.5-rc7 doesn't have f_midi_do_transmit()

Right, but then you will have to fix the merge conflicts by hand. :(

-- 
Felipe

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

[PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe F. Tonello]

Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
potentially cause a race condition between both calls because f_midi_transmit
is not reentrant nor thread-safe. This is due to an implementation detail that
the transmit function looks for the next available usb request from the fifo
and only enqueues it if there is data to send, otherwise just re-uses it. So,
if both ALSA and USB frameworks calls this function at the same time,
kfifo_seek() will return the same usb_request, which will cause a race
condition.

To solve this problem a syncronization mechanism is necessary. In this case it
is used a spinlock since f_midi_transmit is also called by usb_request->complete
callback in interrupt context.

Cc: <stable@vger.kernel.org> # v4.5+
Fixes: e1e3d7ec5da3 ("usb: gadget: f_midi: pre-allocate IN requests")
Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index fb1fe96d3215..ecb46d6abf0e 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/kfifo.h>
+#include <linux/spinlock.h>
 
 #include <sound/core.h>
 #include <sound/initval.h>
@@ -92,6 +93,7 @@ struct f_midi {
 	/* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
 	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
 	unsigned int in_last_port;
+	spinlock_t transmit_lock;
 };
 
 static inline struct f_midi *func_to_midi(struct usb_function *f)
@@ -535,12 +537,15 @@ static void f_midi_drop_out_substreams(struct f_midi *midi)
 static void f_midi_transmit(struct f_midi *midi)
 {
 	struct usb_ep *ep = midi->in_ep;
+	unsigned long flags;
 	bool active;
 
 	/* We only care about USB requests if IN endpoint is enabled */
 	if (!ep || !ep->enabled)
 		goto drop_out;
 
+	spin_lock_irqsave(&midi->transmit_lock, flags);
+
 	do {
 		struct usb_request *req = NULL;
 		unsigned int len, i;
@@ -552,14 +557,17 @@ static void f_midi_transmit(struct f_midi *midi)
 		len = kfifo_peek(&midi->in_req_fifo, &req);
 		if (len != 1) {
 			ERROR(midi, "%s: Couldn't get usb request\n", __func__);
+			spin_unlock_irqrestore(&midi->transmit_lock, flags);
 			goto drop_out;
 		}
 
 		/* If buffer overrun, then we ignore this transmission.
 		 * IMPORTANT: This will cause the user-space rawmidi device to block until a) usb
 		 * requests have been completed or b) snd_rawmidi_write() times out. */
-		if (req->length > 0)
+		if (req->length > 0) {
+			spin_unlock_irqrestore(&midi->transmit_lock, flags);
 			return;
+		}
 
 		for (i = midi->in_last_port; i < MAX_PORTS; i++) {
 			struct gmidi_in_port *port = midi->in_port[i];
@@ -611,6 +619,8 @@ static void f_midi_transmit(struct f_midi *midi)
 		}
 	} while (active);
 
+	spin_unlock_irqrestore(&midi->transmit_lock, flags);
+
 	return;
 
 drop_out:
@@ -1216,6 +1226,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	if (status)
 		goto setup_fail;
 
+	spin_lock_init(&midi->transmit_lock);
+
 	++opts->refcnt;
 	mutex_unlock(&opts->lock);
 
-- 
2.7.2

Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

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

Hi,

On 09/03/16 16:17, Felipe F. Tonello wrote:
> Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
> potentially cause a race condition between both calls because f_midi_transmit
> is not reentrant nor thread-safe. This is due to an implementation detail that
> the transmit function looks for the next available usb request from the fifo
> and only enqueues it if there is data to send, otherwise just re-uses it. So,
> if both ALSA and USB frameworks calls this function at the same time,
> kfifo_seek() will return the same usb_request, which will cause a race
> condition.
> 
> To solve this problem a syncronization mechanism is necessary. In this case it
> is used a spinlock since f_midi_transmit is also called by usb_request->complete
> callback in interrupt context.
> 
> Cc: <stable@vger.kernel.org> # v4.5+
> Fixes: e1e3d7ec5da3 ("usb: gadget: f_midi: pre-allocate IN requests")
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>

I'm sorry but I forgot to add v2 to the subject prefix.

Anyway, the changes from the previous patch is that this patch is on top
of Linus' v4.5-rc7 tag.

> ---
>  drivers/usb/gadget/function/f_midi.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index fb1fe96d3215..ecb46d6abf0e 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -24,6 +24,7 @@
>  #include <linux/slab.h>
>  #include <linux/device.h>
>  #include <linux/kfifo.h>
> +#include <linux/spinlock.h>
>  
>  #include <sound/core.h>
>  #include <sound/initval.h>
> @@ -92,6 +93,7 @@ struct f_midi {
>  	/* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
>  	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>  	unsigned int in_last_port;
> +	spinlock_t transmit_lock;
>  };
>  
>  static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -535,12 +537,15 @@ static void f_midi_drop_out_substreams(struct f_midi *midi)
>  static void f_midi_transmit(struct f_midi *midi)
>  {
>  	struct usb_ep *ep = midi->in_ep;
> +	unsigned long flags;
>  	bool active;
>  
>  	/* We only care about USB requests if IN endpoint is enabled */
>  	if (!ep || !ep->enabled)
>  		goto drop_out;
>  
> +	spin_lock_irqsave(&midi->transmit_lock, flags);
> +
>  	do {
>  		struct usb_request *req = NULL;
>  		unsigned int len, i;
> @@ -552,14 +557,17 @@ static void f_midi_transmit(struct f_midi *midi)
>  		len = kfifo_peek(&midi->in_req_fifo, &req);
>  		if (len != 1) {
>  			ERROR(midi, "%s: Couldn't get usb request\n", __func__);
> +			spin_unlock_irqrestore(&midi->transmit_lock, flags);
>  			goto drop_out;
>  		}
>  
>  		/* If buffer overrun, then we ignore this transmission.
>  		 * IMPORTANT: This will cause the user-space rawmidi device to block until a) usb
>  		 * requests have been completed or b) snd_rawmidi_write() times out. */
> -		if (req->length > 0)
> +		if (req->length > 0) {
> +			spin_unlock_irqrestore(&midi->transmit_lock, flags);
>  			return;
> +		}
>  
>  		for (i = midi->in_last_port; i < MAX_PORTS; i++) {
>  			struct gmidi_in_port *port = midi->in_port[i];
> @@ -611,6 +619,8 @@ static void f_midi_transmit(struct f_midi *midi)
>  		}
>  	} while (active);
>  
> +	spin_unlock_irqrestore(&midi->transmit_lock, flags);
> +
>  	return;
>  
>  drop_out:
> @@ -1216,6 +1226,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  	if (status)
>  		goto setup_fail;
>  
> +	spin_lock_init(&midi->transmit_lock);
> +
>  	++opts->refcnt;
>  	mutex_unlock(&opts->lock);
>  
> 

-- 
Felipe

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