linux-kernel - Re: [PATCH 2/2] usb: gadget: f_midi: added spinlock on transmit function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20151225080334.GC9700@shlinux2>
Date:	Fri, 25 Dec 2015 16:03:34 +0800
From:	Peter Chen <hzpeterchen@...il.com>
To:	"Felipe F. Tonello" <eu@...ipetonello.com>
Cc:	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	Felipe Balbi <balbi@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Robert Baldyga <r.baldyga@...sung.com>,
	Clemens Ladisch <clemens@...isch.de>
Subject: Re: [PATCH 2/2] usb: gadget: f_midi: added spinlock on transmit
 function

On Tue, Dec 22, 2015 at 04:08:06PM +0000, Felipe F. Tonello wrote:
> Since f_midi_transmit is called by both ALSA and USB frameworks, it can
> potentially cause a race condition between both calls. This is bad because the
> way f_midi_transmit is implemented can't handle concurrent calls. This is due
> to the fact that the usb request fifo looks for the next element and only if
> it has data to process it enqueues the request, otherwise re-uses it. If both
> (ALSA and USB) frameworks calls this function at the same time, the
> 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.
> 
> On benchmarks realized by me, spinlocks were more efficient then scheduling
> the f_midi_transmit tasklet in process context and using a mutex to
> synchronize. Also it performs better then previous implementation that

%s/then/than/

> allocated a usb_request for every new transmit made.
> 
> Signed-off-by: Felipe F. Tonello <eu@...ipetonello.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 b70a830..00a15e9 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>
> @@ -97,6 +98,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)
> @@ -574,12 +576,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;
> @@ -591,14 +596,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];
> @@ -650,6 +658,8 @@ static void f_midi_transmit(struct f_midi *midi)
>  		}
>  	} while (active);
>  
> +	spin_unlock_irqrestore(&midi->transmit_lock, flags);
> +
>  	return;
>  
>  drop_out:
> @@ -1255,6 +1265,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.5.0
> 
> --

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/