linux-kernel - Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 13 Jun 2018 10:13:09 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Mikulas Patocka <mpatocka@...hat.com>
cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ming Lei <ming.lei@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        USB list <linux-usb@...r.kernel.org>,
        Kernel development list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

On Wed, 13 Jun 2018, Mikulas Patocka wrote:

[Skipping lots of material...]

> BTW I found this piece of code in sound/usb/usx2y/usbusx2yaudio.c:
>                         urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs();
>                         if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
>                                 snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
>                                 err = -EPIPE;
>                                 goto cleanup;
>                         } else
>                                 if (i == 0)
>                                         usX2Y->wait_iso_frame = urb->start_frame;
>                         urb->transfer_flags = 0;
> It seems like a bug to modify transfer_flags after the urb is submitted.

Yes, it is definitely a bug.

> I have a single-core machine with usb2 soundcard. When I increase mplayer
> priority (to real-time or high non-realtime priority), the sound is
> stuttering. The reason for the stuttering is that mplayer at high priority
> preempts the softirq thread, preventing URBs from being completed. It was
> caused by the patch 428aac8a81058 that offloads URB completion to softirq.
> --- linux-4.17.orig/drivers/usb/host/ehci-q.c	2018-06-12 22:35:21.000000000 +0200
> +++ linux-4.17/drivers/usb/host/ehci-q.c	2018-06-12 22:44:09.000000000 +0200
> @@ -238,6 +238,8 @@ static int qtd_copy_status (
>  
>  static void
>  ehci_urb_done(struct ehci_hcd *ehci, struct urb *urb, int status)
> +__releases(ehci->lock)
> +__acquires(ehci->lock)
>  {
>  	if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) {
>  		/* ... update hc-wide periodic stats */
> @@ -264,7 +266,17 @@ ehci_urb_done(struct ehci_hcd *ehci, str
>  #endif
>  
>  	usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
> -	usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status);
> +	if (urb->transfer_flags & URB_FAST_COMPLETION) {
> +		/*
> +		 * USB audio experiences skipping of we offload completion
> +		 * to ksoftirq.
> +		 */

This comment seems unnecessary.

> +		spin_unlock(&ehci->lock);
> +		usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status);
> +		spin_lock(&ehci->lock);
> +	} else {
> +		usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status);
> +	}
>  }

I'm not at all sure about this.  Have you audited all of ehci-hcd to 
make sure the driver doesn't assume that ehci->lock remains held while 
an URB is given back?  It's been so long since I worked on this area 
that I don't remember the answer offhand.

Alan Stern