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: <sta...@vger.kernel.org> # v4.5+
> Fixes: e1e3d7ec5da3 ("usb: gadget: f_midi: pre-allocate IN requests")
> Signed-off-by: Felipe F. Tonello <e...@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
0x92698E6A.asc
Description: application/pgp-keys
