* Re: BUG: bad usercopy in ld_usb_read
From: Greg KH @ 2019-08-08 12:46 UTC (permalink / raw)
To: syzbot, Michael Hund
Cc: akpm, andreyknvl, cai, keescook, linux-kernel, linux-mm,
linux-usb, syzkaller-bugs, tglx
In-Reply-To: <0000000000005c056c058f9a5437@google.com>
On Thu, Aug 08, 2019 at 05:38:06AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=13aeaece600000
> kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> dashboard link: https://syzkaller.appspot.com/bug?extid=45b2f40f0778cfa7634e
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+45b2f40f0778cfa7634e@syzkaller.appspotmail.com
>
> ldusb 6-1:0.124: Read buffer overflow, -131383996186150 bytes dropped
That's a funny number :)
Nice overflow found, I see you are now starting to fuzz the char device
nodes of usb drivers...
Michael, care to fix this up?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] usb: dwc3: remove generic PHYs forwarding for XHCI device
From: Felipe Balbi @ 2019-08-08 12:47 UTC (permalink / raw)
To: Marek Szyprowski, linux-usb, linux-samsung-soc, Roger Quadros
Cc: linux-kernel, Marek Szyprowski, Mathias Nyman,
Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
In-Reply-To: <20190719093037.16181-1-m.szyprowski@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 2825 bytes --]
Hi,
Marek Szyprowski <m.szyprowski@samsung.com> writes:
> Commit 08f871a3aca2 ("usb: dwc3: host: convey the PHYs to xhci") added
> forwarding of the generic PHYs from DWC3 core to the instantiated XHCI-plat
> device. However XHCI(-plat) driver never gained support for generic PHYs,
> thus the lookup added by that commit is never used. In meantime the commit
> d64ff406e51e ("usb: dwc3: use bus->sysdev for DMA configuration")
> incorrectly changed the device used for creating lookup, making the lookup
> useless and generic PHYs inaccessible from XHCI device.
>
> However since commit 178a0bce05cb ("usb: core: hcd: integrate the PHY
> wrapper into the HCD core") USB HCD already handles generic PHYs acquired
> from the HCD's 'sysdev', which in this case is DWC3 core device. This means
> that creating any custom lookup entries for XHCI driver is no longer needed
> and can be simply removed.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/usb/dwc3/host.c | 22 ++++------------------
> 1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index f55947294f7c..8deea8c91e03 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -85,7 +85,7 @@ int dwc3_host_init(struct dwc3 *dwc)
> DWC3_XHCI_RESOURCES_NUM);
> if (ret) {
> dev_err(dwc->dev, "couldn't add resources to xHCI device\n");
> - goto err1;
> + goto err;
> }
>
> memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
> @@ -112,37 +112,23 @@ int dwc3_host_init(struct dwc3 *dwc)
> ret = platform_device_add_properties(xhci, props);
> if (ret) {
> dev_err(dwc->dev, "failed to add properties to xHCI\n");
> - goto err1;
> + goto err;
> }
> }
>
> - phy_create_lookup(dwc->usb2_generic_phy, "usb2-phy",
> - dev_name(dwc->dev));
> - phy_create_lookup(dwc->usb3_generic_phy, "usb3-phy",
> - dev_name(dwc->dev));
> -
> ret = platform_device_add(xhci);
> if (ret) {
> dev_err(dwc->dev, "failed to register xHCI device\n");
> - goto err2;
> + goto err;
> }
>
> return 0;
> -err2:
> - phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
> - dev_name(dwc->dev));
> - phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
> - dev_name(dwc->dev));
> -err1:
> +err:
> platform_device_put(xhci);
> return ret;
> }
>
> void dwc3_host_exit(struct dwc3 *dwc)
> {
> - phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
> - dev_name(dwc->dev));
> - phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
> - dev_name(dwc->dev));
> platform_device_unregister(dwc->xhci);
> }
Roger, could you verify that this doesn't regress any of your platforms?
Thanks
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: KASAN: use-after-free Read in dvb_usb_device_exit (2)
From: syzbot @ 2019-08-08 12:51 UTC (permalink / raw)
To: allison, andreyknvl, hdanton, linux-kernel, linux-media,
linux-usb, mail, mchehab, oneukum, sean, syzkaller-bugs, tglx
In-Reply-To: <CAAeHK+yzpyCX4dVKwgYXg5oca1yecJ+T5R=6WbEtLzowRSN-9g@mail.gmail.com>
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger
crash:
Reported-and-tested-by:
syzbot+c58e976e022432ee60b4@syzkaller.appspotmail.com
Tested on:
commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git
kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
patch: https://syzkaller.appspot.com/x/patch.diff?x=1622161c600000
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply
* Re: KASAN: use-after-free Read in device_release_driver_internal
From: Andrey Konovalov @ 2019-08-08 13:03 UTC (permalink / raw)
To: Dmitry Vyukov, Alan Stern
Cc: syzbot, LKML, USB list, Oliver Neukum, syzkaller-bugs
In-Reply-To: <CACT4Y+ZD=YYvLER5jDAvCbw3kBKcNkQJEJN5yFc7O6aLaFORDg@mail.gmail.com>
On Thu, Aug 8, 2019 at 2:44 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Thu, Aug 8, 2019 at 2:28 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> >
> > On Wed, Aug 7, 2019 at 8:31 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Wed, 7 Aug 2019, syzbot wrote:
> > >
> > > > Hello,
> > > >
> > > > syzbot has tested the proposed patch and the reproducer did not trigger
> > > > crash:
> > > >
> > > > Reported-and-tested-by:
> > > > syzbot+1b2449b7b5dc240d107a@syzkaller.appspotmail.com
> > > >
> > > > Tested on:
> > > >
> > > > commit: 6a3599ce usb-fuzzer: main usb gadget fuzzer driver
> > > > git tree: https://github.com/google/kasan.git
> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=700ca426ab83faae
> > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > > patch: https://syzkaller.appspot.com/x/patch.diff?x=132eec8c600000
> > > >
> > > > Note: testing is done by a robot and is best-effort only.
> > >
> > > Andrey, is there any way to get the console output from this test?
> >
> > Dmitry, would it be possible to link console log for successful tests as well?
>
> Yes. Start by filing a feature request at
> https://github.com/google/syzkaller/issues
Filed https://github.com/google/syzkaller/issues/1322
Alan, for now I've applied your patch and run the reproducer manually:
[ 90.844643][ T74] usb 1-1: new high-speed USB device number 2
using dummy_hcd
[ 91.085789][ T74] usb 1-1: Using ep0 maxpacket: 16
[ 91.204698][ T74] usb 1-1: config 0 has an invalid interface
number: 234 but max is 0
[ 91.209137][ T74] usb 1-1: config 0 has no interface number 0
[ 91.211599][ T74] usb 1-1: config 0 interface 234 altsetting 0
endpoint 0x8D has an inva1
[ 91.216162][ T74] usb 1-1: config 0 interface 234 altsetting 0
endpoint 0x7 has invalid 4
[ 91.218211][ T74] usb 1-1: config 0 interface 234 altsetting 0
bulk endpoint 0x7 has inv4
[ 91.220131][ T74] usb 1-1: config 0 interface 234 altsetting 0
bulk endpoint 0x8F has in0
[ 91.222052][ T74] usb 1-1: New USB device found, idVendor=0421,
idProduct=0486, bcdDevic7
[ 91.223851][ T74] usb 1-1: New USB device strings: Mfr=0,
Product=0, SerialNumber=0
[ 91.233180][ T74] usb 1-1: config 0 descriptor??
[ 91.270222][ T74] rndis_wlan 1-1:0.234: Refcount before probe: 3
[ 91.275464][ T74] rndis_wlan 1-1:0.234: invalid descriptor buffer length
[ 91.277558][ T74] usb 1-1: bad CDC descriptors
[ 91.279716][ T74] rndis_wlan 1-1:0.234: Refcount after probe: 3
[ 91.281378][ T74] rndis_host 1-1:0.234: Refcount before probe: 3
[ 91.283303][ T74] rndis_host 1-1:0.234: invalid descriptor buffer length
[ 91.284724][ T74] usb 1-1: bad CDC descriptors
[ 91.286004][ T74] rndis_host 1-1:0.234: Refcount after probe: 3
[ 91.287318][ T74] cdc_acm 1-1:0.234: Refcount before probe: 3
[ 91.288513][ T74] cdc_acm 1-1:0.234: invalid descriptor buffer length
[ 91.289835][ T74] cdc_acm 1-1:0.234: No union descriptor, testing
for castrated device
[ 91.291555][ T74] cdc_acm 1-1:0.234: Refcount after probe: 3
[ 91.292766][ T74] cdc_acm: probe of 1-1:0.234 failed with error -12
[ 92.001549][ T96] usb 1-1: USB disconnect, device number 2
^ permalink raw reply
* Re: usb zero copy dma handling
From: Greg KH @ 2019-08-08 13:05 UTC (permalink / raw)
To: Robin Murphy; +Cc: yvahkhfo.1df7f8c2, security, linux-usb, linux-arm-kernel
In-Reply-To: <20190808100726.GB23844@kroah.com>
On Thu, Aug 08, 2019 at 12:07:26PM +0200, Greg KH wrote:
> On Thu, Aug 08, 2019 at 10:46:24AM +0100, Robin Murphy wrote:
> > On 2019-08-08 9:58 am, Greg KH wrote:
> > > On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote:
> > > > Hello linux-usb and linux-arm.
> > > >
> > > > Ccing security@ because "the kernel dma code is mapping randomish
> > > > kernel/user mem to a user process" seems to have security implications
> > > > even though i didnt research that aspect past "its a 100% reliable way
> > > > to crash a raspi from userspace".
> > > >
> > > > tried submitting this through linux-arm-kernel ~2 weeks ago but
> > > > the only "response" i got was phishing-spam.
> > > > tried to follow up through raspi-internals chat, they suggested
> > > > i try linux-usb instead, but otoh the original reporter was
> > > > deflected from -usb to "try some other mls, they might care".
> > > > https://www.spinics.net/lists/linux-usb/msg173277.html
> > > >
> > > > if i am not following some arcane ritual or indenting convention required
> > > > by regular users of these lists i apologize in advance, but i am not a
> > > > kernel developer, i am just here as a user with a bug and a patch.
> > > > (and the vger FAQ link 404s...)
> > >
> > > The "arcane ritual" should be really well documented by now, it's in
> > > Documentation/SubmittingPatches in your kernel tree, and you can read it
> > > online at:
> > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> > >
> > >
> > > > i rediffed against HEAD even though the two weeks old patch still applied
> > > > cleanly with +2 offset.
> > > >
> > > > # stepping off soap box # actual technical content starts here #
> > > >
> > > > this is a followup to that thread from 2018-11:
> > > > https://www.spinics.net/lists/arm-kernel/msg685598.html
> > > >
> > > > the issue was discussed in more detail than i can claim
> > > > to fully understand back then, but no fix ever merged.
> > > > but i would really like to use rtl_433 on a raspi without
> > > > having to build a custom-patched kernel first.
> > > >
> > > > the attached patch is my stripdown/cleanup of a devel-diff
> > > > provided to me by the original reporter Steve Markgraf.
> > > > credits to him for the good parts, blame to me for the bad parts.
> > > >
> > > > this does not cover the additional case of "PIO-based usb controllers"
> > > > mainly because i dont understand what that means (or how to handle it)
> > > > and if its broken right now (as the thread indicates) it might
> > > > as well stay broken until someone who understands cares enough.
> > > >
> > > > could you please get this on track for merging?
> > >
> > >
> > > >
> > > > regards,
> > > > x23
> > > >
> > > >
> > > >
> > >
> > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > > > index b265ab5405f9..69594c2169ea 100644
> > > > --- a/drivers/usb/core/devio.c
> > > > +++ b/drivers/usb/core/devio.c
> > > > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> > > > usbm->vma_use_count = 1;
> > > > INIT_LIST_HEAD(&usbm->memlist);
> > > > +#ifdef CONFIG_X86
> > > > if (remap_pfn_range(vma, vma->vm_start,
> > > > virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> > > > size, vma->vm_page_prot) < 0) {
> > > > +#else /* !CONFIG_X86 */
> > > > + if (dma_mmap_coherent(ps->dev->bus->sysdev,
> > > > + vma, mem, dma_handle, size) < 0) {
> > > > +#endif /* !CONFIG_X86 */
> > > > dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> > > > return -EAGAIN;
> > > > }
> > >
> > > First off, we need this in a format we could apply it in (hint, read the
> > > above links).
> > >
> > > But the main issue here is what exactly is this "fixing"? What is wrong
> > > with the existing code that non-x86 systems have such a problem with?
> > > Shouldn't all of these dma issues be handled by the platform with the
> > > remap_pfn_range() call itself?
> >
> > If usbm->mem is (or ever can be) a CPU address returned by
> > dma_alloc_coherent(), then doing virt_to_phys() on it is bogus and may yield
> > a nonsense 'PFN' to begin with. However, it it can can ever come from a
> > regular page allocation/kmalloc/vmalloc then unconditionally passing it to
> > dma_mmap_coherent wouldn't be right either.
>
> usbm->mem comes from a call to usb_alloc_coherent() which calls
> hcd_buffer_alloc() which tries to allocate memory in the best possible
> way for that specific host controller. If the host controller has a
> pool of memory, it uses that, if the host controller has PIO it uses
> kmalloc(), if there are some "pools" of host controller memory it uses
> dma_pool_alloc() and as a total last resort, calls dma_alloc_coherent().
>
> So yes, this could happen.
>
> So how to fix this properly? What host controller driver is being used
> here that ends up defaulting to dma_alloc_coherent()? Shouldn't that be
> fixed up no matter what?
>
> And then, if what you say is correct then a real fix for devio.c could
> be made, but that is NOT going to just depend on the arch the system is
> running on, as all of this depends on the host controller being accessed
> at that moment for that device.
Also see this thread:
https://lore.kernel.org/linux-usb/20190801220134.3295-1-gavinli@thegavinli.com/
where this just came up and how the proposed patch here would cause
warnings to occur in the kernel log of users for no good reason. That
issue is supposed to be fixed "soon"...
thanks,
greg k-h
^ permalink raw reply
* Re: KASAN: use-after-free Read in device_release_driver_internal
From: Alan Stern @ 2019-08-08 13:59 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Dmitry Vyukov, syzbot, LKML, USB list, Oliver Neukum,
syzkaller-bugs
In-Reply-To: <CAAeHK+yPJR2kZ5Mkry+bGFVuedF9F76=5GdKkF1eLkr9FWyvqA@mail.gmail.com>
On Thu, 8 Aug 2019, Andrey Konovalov wrote:
> On Thu, Aug 8, 2019 at 2:44 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > On Thu, Aug 8, 2019 at 2:28 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> > >
> > > On Wed, Aug 7, 2019 at 8:31 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > >
> > > > On Wed, 7 Aug 2019, syzbot wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > syzbot has tested the proposed patch and the reproducer did not trigger
> > > > > crash:
> > > > >
> > > > > Reported-and-tested-by:
> > > > > syzbot+1b2449b7b5dc240d107a@syzkaller.appspotmail.com
> > > > >
> > > > > Tested on:
> > > > >
> > > > > commit: 6a3599ce usb-fuzzer: main usb gadget fuzzer driver
> > > > > git tree: https://github.com/google/kasan.git
> > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=700ca426ab83faae
> > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > > > patch: https://syzkaller.appspot.com/x/patch.diff?x=132eec8c600000
> > > > >
> > > > > Note: testing is done by a robot and is best-effort only.
> > > >
> > > > Andrey, is there any way to get the console output from this test?
> > >
> > > Dmitry, would it be possible to link console log for successful tests as well?
> >
> > Yes. Start by filing a feature request at
> > https://github.com/google/syzkaller/issues
>
> Filed https://github.com/google/syzkaller/issues/1322
>
> Alan, for now I've applied your patch and run the reproducer manually:
>
> [ 90.844643][ T74] usb 1-1: new high-speed USB device number 2
> using dummy_hcd
> [ 91.085789][ T74] usb 1-1: Using ep0 maxpacket: 16
> [ 91.204698][ T74] usb 1-1: config 0 has an invalid interface
> number: 234 but max is 0
> [ 91.209137][ T74] usb 1-1: config 0 has no interface number 0
> [ 91.211599][ T74] usb 1-1: config 0 interface 234 altsetting 0
> endpoint 0x8D has an inva1
> [ 91.216162][ T74] usb 1-1: config 0 interface 234 altsetting 0
> endpoint 0x7 has invalid 4
> [ 91.218211][ T74] usb 1-1: config 0 interface 234 altsetting 0
> bulk endpoint 0x7 has inv4
> [ 91.220131][ T74] usb 1-1: config 0 interface 234 altsetting 0
> bulk endpoint 0x8F has in0
> [ 91.222052][ T74] usb 1-1: New USB device found, idVendor=0421,
> idProduct=0486, bcdDevic7
> [ 91.223851][ T74] usb 1-1: New USB device strings: Mfr=0,
> Product=0, SerialNumber=0
> [ 91.233180][ T74] usb 1-1: config 0 descriptor??
> [ 91.270222][ T74] rndis_wlan 1-1:0.234: Refcount before probe: 3
> [ 91.275464][ T74] rndis_wlan 1-1:0.234: invalid descriptor buffer length
> [ 91.277558][ T74] usb 1-1: bad CDC descriptors
> [ 91.279716][ T74] rndis_wlan 1-1:0.234: Refcount after probe: 3
> [ 91.281378][ T74] rndis_host 1-1:0.234: Refcount before probe: 3
> [ 91.283303][ T74] rndis_host 1-1:0.234: invalid descriptor buffer length
> [ 91.284724][ T74] usb 1-1: bad CDC descriptors
> [ 91.286004][ T74] rndis_host 1-1:0.234: Refcount after probe: 3
> [ 91.287318][ T74] cdc_acm 1-1:0.234: Refcount before probe: 3
> [ 91.288513][ T74] cdc_acm 1-1:0.234: invalid descriptor buffer length
> [ 91.289835][ T74] cdc_acm 1-1:0.234: No union descriptor, testing
> for castrated device
> [ 91.291555][ T74] cdc_acm 1-1:0.234: Refcount after probe: 3
> [ 91.292766][ T74] cdc_acm: probe of 1-1:0.234 failed with error -12
> [ 92.001549][ T96] usb 1-1: USB disconnect, device number 2
Ah, that looks right, thank you. The patch worked correctly -- good
work Oliver!
Alan Stern
^ permalink raw reply
* Re: [PATCH v4 2/2] usbip: Implement SG support to vhci-hcd and stub driver
From: Suwan Kim @ 2019-08-08 14:21 UTC (permalink / raw)
To: shuah, valentina.manea.m, gregkh, stern; +Cc: linux-usb, linux-kernel
In-Reply-To: <20190806123154.23798-3-suwan.kim027@gmail.com>
On Tue, Aug 06, 2019 at 09:31:54PM +0900, Suwan Kim wrote:
> There are bugs on vhci with usb 3.0 storage device. In USB, each SG
> list entry buffer should be divisible by the bulk max packet size.
> But with native SG support, this problem doesn't matter because the
> SG buffer is treated as contiguous buffer. But without native SG
> support, USB storage driver breaks SG list into several URBs and the
> error occurs because of a buffer size of URB that cannot be divided
> by the bulk max packet size. The error situation is as follows.
>
> When USB Storage driver requests 31.5 KB data and has SG list which
> has 3584 bytes buffer followed by 7 4096 bytes buffer for some
> reason. USB Storage driver splits this SG list into several URBs
> because VHCI doesn't support SG and sends them separately. So the
> first URB buffer size is 3584 bytes. When receiving data from device,
> USB 3.0 device sends data packet of 1024 bytes size because the max
> packet size of BULK pipe is 1024 bytes. So device sends 4096 bytes.
> But the first URB buffer has only 3584 bytes buffer size. So host
> controller terminates the transfer even though there is more data to
> receive. So, vhci needs to support SG transfer to prevent this error.
>
> In this patch, vhci supports SG regardless of whether the server's
> host controller supports SG or not, because stub driver splits SG
> list into several URBs if the server's host controller doesn't
> support SG.
>
> To support SG, vhci_map_urb_for_dma() sets URB_DMA_MAP_SG flag in
> urb->transfer_flags if URB has SG list and this flag will tell stub
> driver to use SG list.
>
> vhci sends each SG list entry to stub driver. Then, stub driver sees
> the total length of the buffer and allocates SG table and pages
> according to the total buffer length calling sgl_alloc(). After stub
> driver receives completed URB, it again sends each SG list entry to
> vhci.
>
> If the server's host controller doesn't support SG, stub driver
> breaks a single SG request into several URBs and submits them to
> the server's host controller. When all the split URBs are completed,
> stub driver reassembles the URBs into a single return command and
> sends it to vhci.
>
> Moreover, in the situation where vhci supports SG, but stub driver
> does not, or vice versa, usbip works normally. Because there is no
> protocol modification, there is no problem in communication between
> server and client even if the one has a kernel without SG support.
>
> In the case of vhci supports SG and stub driver doesn't, because
> vhci sends only the total length of the buffer to stub driver as
> it did before the patch applied, stub driver only needs to allocate
> the required length of buffers regardless of whether vhci supports
> SG or not.
>
> If stub driver needs to send data buffer to vhci because of IN pipe,
> stub driver also sends only total length of buffer as metadata and
> then sends real data as vhci does. Then vhci receive data from stub
> driver and store it to the corresponding buffer of SG list entry.
>
> In the case of stub driver that supports SG, buffer is allocated by
> sgl_alloc(). However, stub driver that does not support SG allocates
> buffer using only kmalloc(). Therefore, if vhci supports SG and stub
> driver doesn't, stub driver has to allocate buffer with kmalloc() as
> much as the total length of SG buffer which is quite huge when vhci
> sends SG request, so it has big overhead in buffer allocation.
>
> And for the case of stub driver supports SG and vhci doesn't, since
> the USB storage driver checks that vhci doesn't support SG and sends
> the request to stub driver by splitting the SG list into multiple
> URBs, stub driver allocates a buffer with kmalloc() as it did before
> this patch.
>
> VUDC also works well with this patch. Tests are done with two USB
> gadget created by CONFIGFS USB gadget. Both use the BULK pipe.
>
> 1. Serial gadget
> 2. Mass storage gadget
>
> * Serial gadget test
>
> Serial gadget on the host sends and receives data using cat command
> on the /dev/ttyGS<N>. The client uses minicom to communicate with
> the serial gadget.
>
> * Mass storage gadget test
>
> After connecting the gadget with vhci, use "dd" to test read and
> write operation on the client side.
>
> Read - dd if=/dev/sd<N> iflag=direct of=/dev/null bs=1G count=1
> Write - dd if=<my file path> iflag=direct of=/dev/sd<N> bs=1G count=1
>
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
> v3 - v4
>
> - Rewrite the description about the vhci bug with USB 3.0 storage
> device.
> - Add the description about the test with VUDC.
> - Fix the error message in stub_recv_cmd_unlink().
>
> v2 - v3
>
> - Rewrite the commit log to make it more clear.
> - Add the description about the mismatch situation in commit log.
> - Run chechpatch.pl and fix errors with coding style and typos.
> - Fix the error path of usbip_recv_xbuff() to stop receiving data
> after TCP error occurs.
> - Consolidate the duplicated error path in usbip_recv_xbuff() and
> vhci_send_cmd_submit().
> - Undo the unnecessary changes
> * Undo the deletion of empty line in stub_send_ret_submit()
> * Move memset() lines in stub_send_ret_submit() to the original
> position.
>
> v1 - v2
>
> - Add the logic that splits single SG request into several URBs in
> stub driver if server’s HC doesn’t support SG.
> ---
> drivers/usb/usbip/stub.h | 7 +-
> drivers/usb/usbip/stub_main.c | 55 ++++++---
> drivers/usb/usbip/stub_rx.c | 204 ++++++++++++++++++++++---------
> drivers/usb/usbip/stub_tx.c | 99 +++++++++++----
> drivers/usb/usbip/usbip_common.c | 59 ++++++---
> drivers/usb/usbip/vhci_hcd.c | 15 ++-
> drivers/usb/usbip/vhci_tx.c | 63 ++++++++--
> 7 files changed, 376 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/usb/usbip/stub.h b/drivers/usb/usbip/stub.h
> index 35618ceb2791..d11270560c24 100644
> --- a/drivers/usb/usbip/stub.h
> +++ b/drivers/usb/usbip/stub.h
> @@ -52,7 +52,11 @@ struct stub_priv {
> unsigned long seqnum;
> struct list_head list;
> struct stub_device *sdev;
> - struct urb *urb;
> + struct urb **urbs;
> + struct scatterlist *sgl;
> + int num_urbs;
> + int completed_urbs;
> + int urb_status;
>
> int unlinking;
> };
> @@ -86,6 +90,7 @@ extern struct usb_device_driver stub_driver;
> struct bus_id_priv *get_busid_priv(const char *busid);
> void put_busid_priv(struct bus_id_priv *bid);
> int del_match_busid(char *busid);
> +void stub_free_priv_and_urb(struct stub_priv *priv);
> void stub_device_cleanup_urbs(struct stub_device *sdev);
>
> /* stub_rx.c */
> diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
> index 2e4bfccd4bfc..3ee636215e9c 100644
> --- a/drivers/usb/usbip/stub_main.c
> +++ b/drivers/usb/usbip/stub_main.c
> @@ -6,6 +6,7 @@
> #include <linux/string.h>
> #include <linux/module.h>
> #include <linux/device.h>
> +#include <linux/scatterlist.h>
>
> #include "usbip_common.h"
> #include "stub.h"
> @@ -288,6 +289,42 @@ static struct stub_priv *stub_priv_pop_from_listhead(struct list_head *listhead)
> return NULL;
> }
>
> +void stub_free_priv_and_urb(struct stub_priv *priv)
> +{
> + struct urb *urb;
> + int i;
> +
> + for (i = 0; i < priv->num_urbs; i++) {
> + urb = priv->urbs[i];
> +
> + if (!urb)
> + return;
> +
> + kfree(urb->setup_packet);
> + urb->setup_packet = NULL;
> +
> +
> + if (urb->transfer_buffer && !priv->sgl) {
> + kfree(urb->transfer_buffer);
> + urb->transfer_buffer = NULL;
> + }
> +
> + if (urb->num_sgs) {
> + sgl_free(urb->sg);
> + urb->sg = NULL;
> + urb->num_sgs = 0;
> + }
> +
> + usb_free_urb(urb);
> + }
> +
> + list_del(&priv->list);
I find list_del() error in my implementation. When client closes
connection with server, stub driver calls stub_device_cleanup_urbs()
to free all the privs and urbs. stub_device_cleanup_urbs() takes
stub_priv from the lists calling stub_priv_pop() and calls
stub_free_priv_and_urb() to free priv and urbs. But the error occurs
when stub_free_priv_and_urb() tries to call list_del() to delete
stub priv from the list because it was already deleted from the
list in stub_priv_pop().
> + if (priv->sgl)
> + sgl_free(priv->sgl);
> + kfree(priv->urbs);
> + kmem_cache_free(stub_priv_cache, priv);
> +}
> +
> static struct stub_priv *stub_priv_pop(struct stub_device *sdev)
> {
> unsigned long flags;
> @@ -314,25 +351,15 @@ static struct stub_priv *stub_priv_pop(struct stub_device *sdev)
> void stub_device_cleanup_urbs(struct stub_device *sdev)
> {
> struct stub_priv *priv;
> - struct urb *urb;
> + int i;
>
> dev_dbg(&sdev->udev->dev, "Stub device cleaning up urbs\n");
>
> while ((priv = stub_priv_pop(sdev))) {
> - urb = priv->urb;
> - dev_dbg(&sdev->udev->dev, "free urb seqnum %lu\n",
> - priv->seqnum);
> - usb_kill_urb(urb);
> + for (i = 0; i < priv->num_urbs; i++)
> + usb_kill_urb(priv->urbs[i]);
>
> - kmem_cache_free(stub_priv_cache, priv);
> -
> - kfree(urb->transfer_buffer);
> - urb->transfer_buffer = NULL;
> -
> - kfree(urb->setup_packet);
> - urb->setup_packet = NULL;
> -
> - usb_free_urb(urb);
> + stub_free_priv_and_urb(priv);
> }
> }
I modified stub_free_priv_and_urb() and stub_device_cleanup_urbs()
to support SG. In stub_device_cleanup_urbs(), the code is the same
as stub_free_priv_and_urb() except that it does not call list_del()
when freeing stub priv and urbs. So I removed the duplicated code
and make stub_device_cleanup_urbs() call stub_free_priv_and_urb()
to free stub priv and urbs. But when calling stub_free_priv_and_urb()
in stub_device_cleanup_urbs(), it should avoid calling list_del()
but it calls. I will fix it up and send v5.
Regards
Suwan Kim
^ permalink raw reply
* Re: Re: possible deadlock in open_rio
From: syzbot @ 2019-08-08 14:33 UTC (permalink / raw)
To: Alan Stern
Cc: andreyknvl, gregkh, linux-kernel, linux-usb, oliver, stern,
syzkaller-bugs
In-Reply-To: <Pine.LNX.4.44L0.1908081027560.1652-100000@iolanthe.rowland.org>
> On Wed, 7 Aug 2019, Oliver Neukum wrote:
>> Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern:
>> > On Wed, 7 Aug 2019, Oliver Neukum wrote:
>> > > technically yes. However in practical terms the straight revert I
>> sent
>> > > out yesterday should fix it.
>> >
>> > I didn't see the revert, and it doesn't appear to have reached the
>> > mailing list archive. Can you post it again?
>> As soon as our VPN server is back up again.
> The revert may not be necessay; a little fix should get rid of the
> locking violation. The key is to avoid calling the registration or
> deregistration routines while holding the rio500_mutex, and to
> recognize that the probe and disconnect routines are both protected by
> the device mutex.
> How does this patch look?
> Alan Stern
> #syz test: https://github.com/google/kasan.git 7f7867ff
This crash does not have a reproducer. I cannot test it.
> Index: usb-devel/drivers/usb/misc/rio500.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/misc/rio500.c
> +++ usb-devel/drivers/usb/misc/rio500.c
> @@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac
> {
> struct usb_device *dev = interface_to_usbdev(intf);
> struct rio_usb_data *rio = &rio_instance;
> - int retval = 0;
> + int retval;
> + char *ibuf, *obuf;
> - mutex_lock(&rio500_mutex);
> if (rio->present) {
> dev_info(&intf->dev, "Second USB Rio at address %d refused\n",
> dev->devnum);
> - retval = -EBUSY;
> - goto bail_out;
> - } else {
> - dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
> + return -EBUSY;
> }
> + dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
> retval = usb_register_dev(intf, &usb_rio_class);
> if (retval) {
> dev_err(&dev->dev,
> "Not able to get a minor for this device.\n");
> - retval = -ENOMEM;
> - goto bail_out;
> + goto err_register;
> }
> - rio->rio_dev = dev;
> -
> - if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
> + obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
> + if (!obuf) {
> dev_err(&dev->dev,
> "probe_rio: Not enough memory for the output buffer\n");
> - usb_deregister_dev(intf, &usb_rio_class);
> - retval = -ENOMEM;
> - goto bail_out;
> + goto err_obuf;
> }
> - dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
> + dev_dbg(&intf->dev, "obuf address: %p\n", obuf);
> - if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
> + ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
> + if (!ibuf) {
> dev_err(&dev->dev,
> "probe_rio: Not enough memory for the input buffer\n");
> - usb_deregister_dev(intf, &usb_rio_class);
> - kfree(rio->obuf);
> - retval = -ENOMEM;
> - goto bail_out;
> + goto err_ibuf;
> }
> - dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
> + dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);
> + mutex_lock(&rio500_mutex);
> + rio->rio_dev = dev;
> + rio->ibuf = ibuf;
> + rio->obuf = obuf;
> usb_set_intfdata (intf, rio);
> rio->present = 1;
> -bail_out:
> mutex_unlock(&rio500_mutex);
> return retval;
> +
> + err_ibuf:
> + kfree(obuf);
> + err_obuf:
> + usb_deregister_dev(intf, &usb_rio_class);
> + err_register:
> + return -ENOMEM;
> }
> static void disconnect_rio(struct usb_interface *intf)
> @@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in
> struct rio_usb_data *rio = usb_get_intfdata (intf);
> usb_set_intfdata (intf, NULL);
> - mutex_lock(&rio500_mutex);
> if (rio) {
> usb_deregister_dev(intf, &usb_rio_class);
> + mutex_lock(&rio500_mutex);
> if (rio->isopen) {
> rio->isopen = 0;
> /* better let it finish - the release will do whats needed */
> @@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in
> dev_info(&intf->dev, "USB Rio disconnected.\n");
> rio->present = 0;
> + mutex_unlock(&rio500_mutex);
> }
> - mutex_unlock(&rio500_mutex);
> }
> static const struct usb_device_id rio_table[] = {
^ permalink raw reply
* Re: Re: possible deadlock in open_rio
From: syzbot @ 2019-08-08 14:33 UTC (permalink / raw)
To: Alan Stern
Cc: andreyknvl, gregkh, linux-kernel, linux-usb, oliver, stern,
syzkaller-bugs
In-Reply-To: <Pine.LNX.4.44L0.1908081027560.1652-100000@iolanthe.rowland.org>
> On Wed, 7 Aug 2019, Oliver Neukum wrote:
>> Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern:
>> > On Wed, 7 Aug 2019, Oliver Neukum wrote:
>> > > technically yes. However in practical terms the straight revert I
>> sent
>> > > out yesterday should fix it.
>> >
>> > I didn't see the revert, and it doesn't appear to have reached the
>> > mailing list archive. Can you post it again?
>> As soon as our VPN server is back up again.
> The revert may not be necessay; a little fix should get rid of the
> locking violation. The key is to avoid calling the registration or
> deregistration routines while holding the rio500_mutex, and to
> recognize that the probe and disconnect routines are both protected by
> the device mutex.
> How does this patch look?
> Alan Stern
> #syz test: https://github.com/google/kasan.git 7f7867ff
This crash does not have a reproducer. I cannot test it.
> Index: usb-devel/drivers/usb/misc/rio500.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/misc/rio500.c
> +++ usb-devel/drivers/usb/misc/rio500.c
> @@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac
> {
> struct usb_device *dev = interface_to_usbdev(intf);
> struct rio_usb_data *rio = &rio_instance;
> - int retval = 0;
> + int retval;
> + char *ibuf, *obuf;
> - mutex_lock(&rio500_mutex);
> if (rio->present) {
> dev_info(&intf->dev, "Second USB Rio at address %d refused\n",
> dev->devnum);
> - retval = -EBUSY;
> - goto bail_out;
> - } else {
> - dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
> + return -EBUSY;
> }
> + dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
> retval = usb_register_dev(intf, &usb_rio_class);
> if (retval) {
> dev_err(&dev->dev,
> "Not able to get a minor for this device.\n");
> - retval = -ENOMEM;
> - goto bail_out;
> + goto err_register;
> }
> - rio->rio_dev = dev;
> -
> - if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
> + obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
> + if (!obuf) {
> dev_err(&dev->dev,
> "probe_rio: Not enough memory for the output buffer\n");
> - usb_deregister_dev(intf, &usb_rio_class);
> - retval = -ENOMEM;
> - goto bail_out;
> + goto err_obuf;
> }
> - dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
> + dev_dbg(&intf->dev, "obuf address: %p\n", obuf);
> - if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
> + ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
> + if (!ibuf) {
> dev_err(&dev->dev,
> "probe_rio: Not enough memory for the input buffer\n");
> - usb_deregister_dev(intf, &usb_rio_class);
> - kfree(rio->obuf);
> - retval = -ENOMEM;
> - goto bail_out;
> + goto err_ibuf;
> }
> - dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
> + dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);
> + mutex_lock(&rio500_mutex);
> + rio->rio_dev = dev;
> + rio->ibuf = ibuf;
> + rio->obuf = obuf;
> usb_set_intfdata (intf, rio);
> rio->present = 1;
> -bail_out:
> mutex_unlock(&rio500_mutex);
> return retval;
> +
> + err_ibuf:
> + kfree(obuf);
> + err_obuf:
> + usb_deregister_dev(intf, &usb_rio_class);
> + err_register:
> + return -ENOMEM;
> }
> static void disconnect_rio(struct usb_interface *intf)
> @@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in
> struct rio_usb_data *rio = usb_get_intfdata (intf);
> usb_set_intfdata (intf, NULL);
> - mutex_lock(&rio500_mutex);
> if (rio) {
> usb_deregister_dev(intf, &usb_rio_class);
> + mutex_lock(&rio500_mutex);
> if (rio->isopen) {
> rio->isopen = 0;
> /* better let it finish - the release will do whats needed */
> @@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in
> dev_info(&intf->dev, "USB Rio disconnected.\n");
> rio->present = 0;
> + mutex_unlock(&rio500_mutex);
> }
> - mutex_unlock(&rio500_mutex);
> }
> static const struct usb_device_id rio_table[] = {
^ permalink raw reply
* Re: possible deadlock in open_rio
From: Alan Stern @ 2019-08-08 14:33 UTC (permalink / raw)
To: Oliver Neukum
Cc: andreyknvl, syzkaller-bugs, gregkh, syzbot,
Kernel development list, USB list
In-Reply-To: <1565187142.15973.3.camel@neukum.org>
On Wed, 7 Aug 2019, Oliver Neukum wrote:
> Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern:
> > On Wed, 7 Aug 2019, Oliver Neukum wrote:
> > > technically yes. However in practical terms the straight revert I sent
> > > out yesterday should fix it.
> >
> > I didn't see the revert, and it doesn't appear to have reached the
> > mailing list archive. Can you post it again?
>
> As soon as our VPN server is back up again.
The revert may not be necessay; a little fix should get rid of the
locking violation. The key is to avoid calling the registration or
deregistration routines while holding the rio500_mutex, and to
recognize that the probe and disconnect routines are both protected by
the device mutex.
How does this patch look?
Alan Stern
#syz test: https://github.com/google/kasan.git 7f7867ff
Index: usb-devel/drivers/usb/misc/rio500.c
===================================================================
--- usb-devel.orig/drivers/usb/misc/rio500.c
+++ usb-devel/drivers/usb/misc/rio500.c
@@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac
{
struct usb_device *dev = interface_to_usbdev(intf);
struct rio_usb_data *rio = &rio_instance;
- int retval = 0;
+ int retval;
+ char *ibuf, *obuf;
- mutex_lock(&rio500_mutex);
if (rio->present) {
dev_info(&intf->dev, "Second USB Rio at address %d refused\n", dev->devnum);
- retval = -EBUSY;
- goto bail_out;
- } else {
- dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
+ return -EBUSY;
}
+ dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
retval = usb_register_dev(intf, &usb_rio_class);
if (retval) {
dev_err(&dev->dev,
"Not able to get a minor for this device.\n");
- retval = -ENOMEM;
- goto bail_out;
+ goto err_register;
}
- rio->rio_dev = dev;
-
- if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
+ obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
+ if (!obuf) {
dev_err(&dev->dev,
"probe_rio: Not enough memory for the output buffer\n");
- usb_deregister_dev(intf, &usb_rio_class);
- retval = -ENOMEM;
- goto bail_out;
+ goto err_obuf;
}
- dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
+ dev_dbg(&intf->dev, "obuf address: %p\n", obuf);
- if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
+ ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
+ if (!ibuf) {
dev_err(&dev->dev,
"probe_rio: Not enough memory for the input buffer\n");
- usb_deregister_dev(intf, &usb_rio_class);
- kfree(rio->obuf);
- retval = -ENOMEM;
- goto bail_out;
+ goto err_ibuf;
}
- dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
+ dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);
+ mutex_lock(&rio500_mutex);
+ rio->rio_dev = dev;
+ rio->ibuf = ibuf;
+ rio->obuf = obuf;
usb_set_intfdata (intf, rio);
rio->present = 1;
-bail_out:
mutex_unlock(&rio500_mutex);
return retval;
+
+ err_ibuf:
+ kfree(obuf);
+ err_obuf:
+ usb_deregister_dev(intf, &usb_rio_class);
+ err_register:
+ return -ENOMEM;
}
static void disconnect_rio(struct usb_interface *intf)
@@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in
struct rio_usb_data *rio = usb_get_intfdata (intf);
usb_set_intfdata (intf, NULL);
- mutex_lock(&rio500_mutex);
if (rio) {
usb_deregister_dev(intf, &usb_rio_class);
+ mutex_lock(&rio500_mutex);
if (rio->isopen) {
rio->isopen = 0;
/* better let it finish - the release will do whats needed */
@@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in
dev_info(&intf->dev, "USB Rio disconnected.\n");
rio->present = 0;
+ mutex_unlock(&rio500_mutex);
}
- mutex_unlock(&rio500_mutex);
}
static const struct usb_device_id rio_table[] = {
^ permalink raw reply
* [PATCH] usb: cdc-acm: make sure a refcount is taken early enough
From: Oliver Neukum @ 2019-08-08 14:21 UTC (permalink / raw)
To: gregKH, linux-usb, stern; +Cc: Oliver Neukum
destroy() will decrement the refcount on the interface, so that
it needs to be taken so early that it never undercounts.
Fixes: 7fb57a019f94e ("USB: cdc-acm: Fix potential deadlock (lockdep warning)")
Reported-and-tested-by: syzbot+1b2449b7b5dc240d107a@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
drivers/usb/class/cdc-acm.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 183b41753c98..62f4fb9b362f 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1301,10 +1301,6 @@ static int acm_probe(struct usb_interface *intf,
tty_port_init(&acm->port);
acm->port.ops = &acm_port_ops;
- minor = acm_alloc_minor(acm);
- if (minor < 0)
- goto alloc_fail1;
-
ctrlsize = usb_endpoint_maxp(epctrl);
readsize = usb_endpoint_maxp(epread) *
(quirks == SINGLE_RX_URB ? 1 : 2);
@@ -1312,6 +1308,13 @@ static int acm_probe(struct usb_interface *intf,
acm->writesize = usb_endpoint_maxp(epwrite) * 20;
acm->control = control_interface;
acm->data = data_interface;
+
+ usb_get_intf(acm->control); /* undone in destruct() */
+
+ minor = acm_alloc_minor(acm);
+ if (minor < 0)
+ goto alloc_fail1;
+
acm->minor = minor;
acm->dev = usb_dev;
if (h.usb_cdc_acm_descriptor)
@@ -1458,7 +1461,6 @@ static int acm_probe(struct usb_interface *intf,
usb_driver_claim_interface(&acm_driver, data_interface, acm);
usb_set_intfdata(data_interface, acm);
- usb_get_intf(control_interface);
tty_dev = tty_port_register_device(&acm->port, acm_tty_driver, minor,
&control_interface->dev);
if (IS_ERR(tty_dev)) {
--
2.16.4
^ permalink raw reply related
* Re: possible deadlock in open_rio
From: Andrey Konovalov @ 2019-08-08 14:44 UTC (permalink / raw)
To: Alan Stern
Cc: Oliver Neukum, syzkaller-bugs, Greg Kroah-Hartman, syzbot,
Kernel development list, USB list
In-Reply-To: <Pine.LNX.4.44L0.1908081027560.1652-100000@iolanthe.rowland.org>
On Thu, Aug 8, 2019 at 4:33 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, 7 Aug 2019, Oliver Neukum wrote:
>
> > Am Mittwoch, den 07.08.2019, 10:07 -0400 schrieb Alan Stern:
> > > On Wed, 7 Aug 2019, Oliver Neukum wrote:
>
> > > > technically yes. However in practical terms the straight revert I sent
> > > > out yesterday should fix it.
> > >
> > > I didn't see the revert, and it doesn't appear to have reached the
> > > mailing list archive. Can you post it again?
> >
> > As soon as our VPN server is back up again.
>
> The revert may not be necessay; a little fix should get rid of the
> locking violation. The key is to avoid calling the registration or
> deregistration routines while holding the rio500_mutex, and to
> recognize that the probe and disconnect routines are both protected by
> the device mutex.
>
> How does this patch look?
>
> Alan Stern
>
>
> #syz test: https://github.com/google/kasan.git 7f7867ff
There's no reproducer yet (it should appear at some point, I've
enabled fuzzing of USB char devices). I've tested your patch manually
and the deadlock report is gone. Thanks!
Tested-by: Andrey Konovalov <andreyknvl@google.com>
>
> Index: usb-devel/drivers/usb/misc/rio500.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/misc/rio500.c
> +++ usb-devel/drivers/usb/misc/rio500.c
> @@ -454,52 +454,54 @@ static int probe_rio(struct usb_interfac
> {
> struct usb_device *dev = interface_to_usbdev(intf);
> struct rio_usb_data *rio = &rio_instance;
> - int retval = 0;
> + int retval;
> + char *ibuf, *obuf;
>
> - mutex_lock(&rio500_mutex);
> if (rio->present) {
> dev_info(&intf->dev, "Second USB Rio at address %d refused\n", dev->devnum);
> - retval = -EBUSY;
> - goto bail_out;
> - } else {
> - dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
> + return -EBUSY;
> }
> + dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
>
> retval = usb_register_dev(intf, &usb_rio_class);
> if (retval) {
> dev_err(&dev->dev,
> "Not able to get a minor for this device.\n");
> - retval = -ENOMEM;
> - goto bail_out;
> + goto err_register;
> }
>
> - rio->rio_dev = dev;
> -
> - if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
> + obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
> + if (!obuf) {
> dev_err(&dev->dev,
> "probe_rio: Not enough memory for the output buffer\n");
> - usb_deregister_dev(intf, &usb_rio_class);
> - retval = -ENOMEM;
> - goto bail_out;
> + goto err_obuf;
> }
> - dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
> + dev_dbg(&intf->dev, "obuf address: %p\n", obuf);
>
> - if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
> + ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
> + if (!ibuf) {
> dev_err(&dev->dev,
> "probe_rio: Not enough memory for the input buffer\n");
> - usb_deregister_dev(intf, &usb_rio_class);
> - kfree(rio->obuf);
> - retval = -ENOMEM;
> - goto bail_out;
> + goto err_ibuf;
> }
> - dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
> + dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);
>
> + mutex_lock(&rio500_mutex);
> + rio->rio_dev = dev;
> + rio->ibuf = ibuf;
> + rio->obuf = obuf;
> usb_set_intfdata (intf, rio);
> rio->present = 1;
> -bail_out:
> mutex_unlock(&rio500_mutex);
>
> return retval;
> +
> + err_ibuf:
> + kfree(obuf);
> + err_obuf:
> + usb_deregister_dev(intf, &usb_rio_class);
> + err_register:
> + return -ENOMEM;
> }
>
> static void disconnect_rio(struct usb_interface *intf)
> @@ -507,10 +509,10 @@ static void disconnect_rio(struct usb_in
> struct rio_usb_data *rio = usb_get_intfdata (intf);
>
> usb_set_intfdata (intf, NULL);
> - mutex_lock(&rio500_mutex);
> if (rio) {
> usb_deregister_dev(intf, &usb_rio_class);
>
> + mutex_lock(&rio500_mutex);
> if (rio->isopen) {
> rio->isopen = 0;
> /* better let it finish - the release will do whats needed */
> @@ -524,8 +526,8 @@ static void disconnect_rio(struct usb_in
> dev_info(&intf->dev, "USB Rio disconnected.\n");
>
> rio->present = 0;
> + mutex_unlock(&rio500_mutex);
> }
> - mutex_unlock(&rio500_mutex);
> }
>
> static const struct usb_device_id rio_table[] = {
>
^ permalink raw reply
* [balbi-usb:testing/next 2/13] drivers/usb/phy/phy-tahvo.c:434:4: error: 'struct device_driver' has no member named 'dev_groups'; did you mean 'groups'?
From: kbuild test robot @ 2019-08-08 15:09 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: kbuild-all, linux-usb, linux-omap, Felipe Balbi
[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]
tree: https://kernel.googlesource.com/pub/scm/linux/kernel/git/balbi/usb.git testing/next
head: d06a2c3f683a591efce9d02b2b60ef346df5ae02
commit: 2a714ea6d90d9d1b510ba424652a2e3dfd547267 [2/13] USB: phy: tahvo: convert platform driver to use dev_groups
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 2a714ea6d90d9d1b510ba424652a2e3dfd547267
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/usb/phy/phy-tahvo.c:434:4: error: 'struct device_driver' has no member named 'dev_groups'; did you mean 'groups'?
.dev_groups = tahvo_groups,
^~~~~~~~~~
groups
>> drivers/usb/phy/phy-tahvo.c:434:17: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.dev_groups = tahvo_groups,
^~~~~~~~~~~~
drivers/usb/phy/phy-tahvo.c:434:17: note: (near initialization for 'tahvo_usb_driver.driver.bus')
cc1: some warnings being treated as errors
vim +434 drivers/usb/phy/phy-tahvo.c
428
429 static struct platform_driver tahvo_usb_driver = {
430 .probe = tahvo_usb_probe,
431 .remove = tahvo_usb_remove,
432 .driver = {
433 .name = "tahvo-usb",
> 434 .dev_groups = tahvo_groups,
435 },
436 };
437 module_platform_driver(tahvo_usb_driver);
438
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51784 bytes --]
^ permalink raw reply
* Re: [PATCH v2] usbfs: Add ioctls for runtime power management
From: Greg KH @ 2019-08-08 15:37 UTC (permalink / raw)
To: Alan Stern; +Cc: Mayuresh Kulkarni, USB list
In-Reply-To: <Pine.LNX.4.44L0.1908071013220.1514-100000@iolanthe.rowland.org>
On Wed, Aug 07, 2019 at 10:29:50AM -0400, Alan Stern wrote:
> It has been requested that usbfs should implement runtime power
> management, instead of forcing the device to remain at full power as
> long as the device file is open. This patch introduces that new
> feature.
>
> It does so by adding three new usbfs ioctls:
>
> USBDEVFS_FORBID_SUSPEND: Prevents the device from going into
> runtime suspend (and causes a resume if the device is already
> suspended).
>
> USBDEVFS_ALLOW_SUSPEND: Allows the device to go into runtime
> suspend. Some time may elapse before the device actually is
> suspended, depending on things like the autosuspend delay.
>
> USBDEVFS_WAIT_FOR_RESUME: Blocks until the call is interrupted
> by a signal or at least one runtime resume has occurred since
> the most recent ALLOW_SUSPEND ioctl call (which may mean
> immediately, even if the device is currently suspended). In
> the latter case, the device is prevented from suspending again
> just as if FORBID_SUSPEND was called before the ioctl returns.
>
> For backward compatibility, when the device file is first opened
> runtime suspends are forbidden. The userspace program can then allow
> suspends whenever it wants, and either resume the device directly (by
> forbidding suspends again) or wait for a resume from some other source
> (such as a remote wakeup). URBs submitted to a suspended device will
> fail or will complete with an appropriate error code.
>
> This combination of ioctls is sufficient for user programs to have
> nearly the same degree of control over a device's runtime power
> behavior as kernel drivers do.
>
> Still lacking is documentation for the new ioctls. I intend to add it
> later, after the existing documentation for the usbfs userspace API is
> straightened out into a reasonable form.
>
> Suggested-by: Mayuresh Kulkarni <mkulkarni@opensource.cirrus.com>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>
> ---
>
> v2: Return -EINTR instead of -ERESTARTSYS when proc_wait_for_resume()
> is interrupted by a signal.
Looks great, thanks for doing this. Now queued up.
greg k-h
^ permalink raw reply
* [PATCH v5 0/2] usbip: Implement SG support
From: Suwan Kim @ 2019-08-08 15:54 UTC (permalink / raw)
To: shuah, valentina.manea.m, gregkh, stern
Cc: linux-usb, linux-kernel, Suwan Kim
There are bugs on vhci with usb 3.0 storage device. In USB, each SG
list entry buffer should be divisible by the bulk max packet size.
But with native SG support, this problem doesn't matter because the
SG buffer is treated as contiguous buffer. But without native SG
support, USB storage driver breaks SG list into several URBs and the
error occurs because of a buffer size of URB that cannot be divided
by the bulk max packet size. The error situation is as follows.
When USB Storage driver requests 31.5 KB data and has SG list which
has 3584 bytes buffer followed by 7 4096 bytes buffer for some
reason. USB Storage driver splits this SG list into several URBs
because VHCI doesn't support SG and sends them separately. So the
first URB buffer size is 3584 bytes. When receiving data from device,
USB 3.0 device sends data packet of 1024 bytes size because the max
packet size of BULK pipe is 1024 bytes. So device sends 4096 bytes.
But the first URB buffer has only 3584 bytes buffer size. So host
controller terminates the transfer even though there is more data to
receive. So, vhci needs to support SG transfer to prevent this error.
In this patch, vhci supports SG regardless of whether the server's
host controller supports SG or not, because stub driver splits SG
list into several URBs if the server's host controller doesn't
support SG.
To support SG, vhci_map_urb_for_dma() sets URB_DMA_MAP_SG flag in
urb->transfer_flags if URB has SG list and this flag will tell stub
driver to use SG list.
vhci sends each SG list entry to stub driver. Then, stub driver sees
the total length of the buffer and allocates SG table and pages
according to the total buffer length calling sgl_alloc(). After stub
driver receives completed URB, it again sends each SG list entry to
vhci.
If the server's host controller doesn't support SG, stub driver
breaks a single SG request into several URBs and submits them to
the server's host controller. When all the split URBs are completed,
stub driver reassembles the URBs into a single return command and
sends it to vhci.
Alan fixed vhci bug with the USB 3.0 storage device by modifying
USB storage driver.
("usb-storage: Set virt_boundary_mask to avoid SG overflows")
But the fundamental solution of it is to add SG support to vhci.
This patch works well with the USB 3.0 storage devices without Alan's
patch, and we can revert Alan's patch if it causes some troubles.
Suwan Kim (2):
usbip: Skip DMA mapping and unmapping for urb at vhci
usbip: Implement SG support to vhci-hcd and stub driver
drivers/usb/usbip/stub.h | 7 +-
drivers/usb/usbip/stub_main.c | 57 ++++++---
drivers/usb/usbip/stub_rx.c | 204 ++++++++++++++++++++++---------
drivers/usb/usbip/stub_tx.c | 99 +++++++++++----
drivers/usb/usbip/usbip_common.c | 59 ++++++---
drivers/usb/usbip/vhci_hcd.c | 34 +++++-
drivers/usb/usbip/vhci_tx.c | 63 ++++++++--
7 files changed, 396 insertions(+), 127 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH v5 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci
From: Suwan Kim @ 2019-08-08 15:54 UTC (permalink / raw)
To: shuah, valentina.manea.m, gregkh, stern
Cc: linux-usb, linux-kernel, Suwan Kim
In-Reply-To: <20190808155435.10050-1-suwan.kim027@gmail.com>
vhci doesn’t do DMA for remote device. Actually, the real DMA
operation is done by network card driver. vhci just passes virtual
address of the buffer to the network stack, so vhci doesn’t use and
need dma address of the buffer of the URB.
But HCD provides DMA mapping and unmapping function by default.
Moreover, it causes unnecessary DMA mapping and unmapping which
will be done again at the NIC driver and it wastes CPU cycles.
So, implement map_urb_for_dma and unmap_urb_for_dma function for
vhci in order to skip the DMA mapping and unmapping procedure.
When it comes to supporting SG for vhci, it is useful to use native
SG list (urb->num_sgs) instead of mapped SG list because DMA mapping
fnuction can adjust the number of SG list (urb->num_mapped_sgs).
And vhci_map_urb_for_dma() prevents isoc pipe from using SG as
hcd_map_urb_for_dma() does.
Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
---
v4 - v5
- Replace pr_err() with dev_err() in the error path.
v3 - v4
- Replace WARN_ON() with pr_err() in the error path.
v2 - v3
- Move setting URB_DMA_MAP_SG flag to the patch 2.
- Prevent isoc pipe from using SG buffer.
v1 - v2
- Add setting URB_DMA_MAP_SG flag in urb->transfer_flags to tell
stub driver to use SG buffer.
---
drivers/usb/usbip/vhci_hcd.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 000ab7225717..ea82b932a2f9 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1288,6 +1288,22 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
return 0;
}
+static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+ gfp_t mem_flags)
+{
+ if (usb_endpoint_xfer_isoc(&urb->ep->desc) && urb->num_sgs) {
+ dev_err(&urb->dev->dev, "SG is not supported for isochronous transfer\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void vhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
+ dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n");
+}
+
static const struct hc_driver vhci_hc_driver = {
.description = driver_name,
.product_desc = driver_desc,
@@ -1304,6 +1320,9 @@ static const struct hc_driver vhci_hc_driver = {
.get_frame_number = vhci_get_frame_number,
+ .map_urb_for_dma = vhci_map_urb_for_dma,
+ .unmap_urb_for_dma = vhci_unmap_urb_for_dma,
+
.hub_status_data = vhci_hub_status,
.hub_control = vhci_hub_control,
.bus_suspend = vhci_bus_suspend,
--
2.20.1
^ permalink raw reply related
* [PATCH v5 2/2] usbip: Implement SG support to vhci-hcd and stub driver
From: Suwan Kim @ 2019-08-08 15:54 UTC (permalink / raw)
To: shuah, valentina.manea.m, gregkh, stern
Cc: linux-usb, linux-kernel, Suwan Kim
In-Reply-To: <20190808155435.10050-1-suwan.kim027@gmail.com>
There are bugs on vhci with usb 3.0 storage device. In USB, each SG
list entry buffer should be divisible by the bulk max packet size.
But with native SG support, this problem doesn't matter because the
SG buffer is treated as contiguous buffer. But without native SG
support, USB storage driver breaks SG list into several URBs and the
error occurs because of a buffer size of URB that cannot be divided
by the bulk max packet size. The error situation is as follows.
When USB Storage driver requests 31.5 KB data and has SG list which
has 3584 bytes buffer followed by 7 4096 bytes buffer for some
reason. USB Storage driver splits this SG list into several URBs
because VHCI doesn't support SG and sends them separately. So the
first URB buffer size is 3584 bytes. When receiving data from device,
USB 3.0 device sends data packet of 1024 bytes size because the max
packet size of BULK pipe is 1024 bytes. So device sends 4096 bytes.
But the first URB buffer has only 3584 bytes buffer size. So host
controller terminates the transfer even though there is more data to
receive. So, vhci needs to support SG transfer to prevent this error.
In this patch, vhci supports SG regardless of whether the server's
host controller supports SG or not, because stub driver splits SG
list into several URBs if the server's host controller doesn't
support SG.
To support SG, vhci_map_urb_for_dma() sets URB_DMA_MAP_SG flag in
urb->transfer_flags if URB has SG list and this flag will tell stub
driver to use SG list.
vhci sends each SG list entry to stub driver. Then, stub driver sees
the total length of the buffer and allocates SG table and pages
according to the total buffer length calling sgl_alloc(). After stub
driver receives completed URB, it again sends each SG list entry to
vhci.
If the server's host controller doesn't support SG, stub driver
breaks a single SG request into several URBs and submits them to
the server's host controller. When all the split URBs are completed,
stub driver reassembles the URBs into a single return command and
sends it to vhci.
Moreover, in the situation where vhci supports SG, but stub driver
does not, or vice versa, usbip works normally. Because there is no
protocol modification, there is no problem in communication between
server and client even if the one has a kernel without SG support.
In the case of vhci supports SG and stub driver doesn't, because
vhci sends only the total length of the buffer to stub driver as
it did before the patch applied, stub driver only needs to allocate
the required length of buffers using only kmalloc() regardless of
whether vhci supports SG or not. But stub driver has to allocate
buffer with kmalloc() as much as the total length of SG buffer which
is quite huge when vhci sends SG request, so it has overhead in
buffer allocation in this situation.
If stub driver needs to send data buffer to vhci because of IN pipe,
stub driver also sends only total length of buffer as metadata and
then sends real data as vhci does. Then vhci receive data from stub
driver and store it to the corresponding buffer of SG list entry.
And for the case of stub driver supports SG and vhci doesn't, since
the USB storage driver checks that vhci doesn't support SG and sends
the request to stub driver by splitting the SG list into multiple
URBs, stub driver allocates a buffer for each URB with kmalloc() as
it did before this patch.
* Test environment
Test uses two difference machines and two different kernel version
to make mismatch situation between the client and the server where
vhci supports SG, but stub driver does not, or vice versa. All tests
are conducted in both full SG support that both vhci and stub support
SG and half SG support that is the mismatch situation.
- Test kernel version
- 5.2-rc6 with SG support
- 5.1.20-200.fc29.x86_64 without SG support
* SG support test
- Test devices
- Super-speed storage device - SanDisk Ultra USB 3.0
- High-speed storage device - SMI corporation USB 2.0 flash drive
- Test description
Test read and write operation of mass storage device that uses the
BULK transfer. In test, the client reads and writes files whose size
is over 1G and it works normally.
* Regression test
- Test devices
- Super-speed device - Logitech Brio webcam
- High-speed device - Logitech C920 HD Pro webcam
- Full-speed device - Logitech bluetooth mouse
- Britz BR-Orion speaker
- Low-speed device - Logitech wired mouse
- Test description
Moving and click test for mouse. To test the webcam, use gnome-cheese.
To test the speaker, play music and video on the client. All works
normally.
* VUDC compatibility test
VUDC also works well with this patch. Tests are done with two USB
gadget created by CONFIGFS USB gadget. Both use the BULK pipe.
1. Serial gadget
2. Mass storage gadget
- Serial gadget test
Serial gadget on the host sends and receives data using cat command
on the /dev/ttyGS<N>. The client uses minicom to communicate with
the serial gadget.
- Mass storage gadget test
After connecting the gadget with vhci, use "dd" to test read and
write operation on the client side.
Read - dd if=/dev/sd<N> iflag=direct of=/dev/null bs=1G count=1
Write - dd if=<my file path> iflag=direct of=/dev/sd<N> bs=1G count=1
Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
---
v4 - v5
- Add the test description about SG support test and regression test
for other USB devices which have various speed.
- Fix list_del() error in stub_device_cleanup_urbs()
- when stub_device_cleanup_urbs() calls stub_pirv_pop() to get a
stub_priv, stub_priv_pop() uses list_del_init() instead of
list_del(). After getting the stub_priv, stub_device_cleanup_urbs()
calls stub_free_priv_and_urb() and it checks if list of the
stub_priv is empty. Because stub_priv_pop() calls list_del_init(),
stub_free_priv_and_urb() doesn't call list_del()
v3 - v4
- Rewrite the description about the vhci bug with USB 3.0 storage
device.
- Add the description about the test with VUDC.
- Fix the error message in stub_recv_cmd_unlink().
v2 - v3
- Rewrite the commit log to make it more clear.
- Add the description about the mismatch situation in commit log.
- Run chechpatch.pl and fix errors with coding style and typos.
- Fix the error path of usbip_recv_xbuff() to stop receiving data
after TCP error occurs.
- Consolidate the duplicated error path in usbip_recv_xbuff() and
vhci_send_cmd_submit().
- Undo the unnecessary changes
* Undo the deletion of empty line in stub_send_ret_submit()
* Move memset() lines in stub_send_ret_submit() to the original
position.
v1 - v2
- Add the logic that splits single SG request into several URBs in
stub driver if server’s HC doesn’t support SG.
---
drivers/usb/usbip/stub.h | 7 +-
drivers/usb/usbip/stub_main.c | 57 ++++++---
drivers/usb/usbip/stub_rx.c | 204 ++++++++++++++++++++++---------
drivers/usb/usbip/stub_tx.c | 99 +++++++++++----
drivers/usb/usbip/usbip_common.c | 59 ++++++---
drivers/usb/usbip/vhci_hcd.c | 15 ++-
drivers/usb/usbip/vhci_tx.c | 63 ++++++++--
7 files changed, 377 insertions(+), 127 deletions(-)
diff --git a/drivers/usb/usbip/stub.h b/drivers/usb/usbip/stub.h
index 35618ceb2791..d11270560c24 100644
--- a/drivers/usb/usbip/stub.h
+++ b/drivers/usb/usbip/stub.h
@@ -52,7 +52,11 @@ struct stub_priv {
unsigned long seqnum;
struct list_head list;
struct stub_device *sdev;
- struct urb *urb;
+ struct urb **urbs;
+ struct scatterlist *sgl;
+ int num_urbs;
+ int completed_urbs;
+ int urb_status;
int unlinking;
};
@@ -86,6 +90,7 @@ extern struct usb_device_driver stub_driver;
struct bus_id_priv *get_busid_priv(const char *busid);
void put_busid_priv(struct bus_id_priv *bid);
int del_match_busid(char *busid);
+void stub_free_priv_and_urb(struct stub_priv *priv);
void stub_device_cleanup_urbs(struct stub_device *sdev);
/* stub_rx.c */
diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index 2e4bfccd4bfc..c1c0bbc9f8b1 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -6,6 +6,7 @@
#include <linux/string.h>
#include <linux/module.h>
#include <linux/device.h>
+#include <linux/scatterlist.h>
#include "usbip_common.h"
#include "stub.h"
@@ -281,13 +282,49 @@ static struct stub_priv *stub_priv_pop_from_listhead(struct list_head *listhead)
struct stub_priv *priv, *tmp;
list_for_each_entry_safe(priv, tmp, listhead, list) {
- list_del(&priv->list);
+ list_del_init(&priv->list);
return priv;
}
return NULL;
}
+void stub_free_priv_and_urb(struct stub_priv *priv)
+{
+ struct urb *urb;
+ int i;
+
+ for (i = 0; i < priv->num_urbs; i++) {
+ urb = priv->urbs[i];
+
+ if (!urb)
+ return;
+
+ kfree(urb->setup_packet);
+ urb->setup_packet = NULL;
+
+
+ if (urb->transfer_buffer && !priv->sgl) {
+ kfree(urb->transfer_buffer);
+ urb->transfer_buffer = NULL;
+ }
+
+ if (urb->num_sgs) {
+ sgl_free(urb->sg);
+ urb->sg = NULL;
+ urb->num_sgs = 0;
+ }
+
+ usb_free_urb(urb);
+ }
+ if (!list_empty(&priv->list))
+ list_del(&priv->list);
+ if (priv->sgl)
+ sgl_free(priv->sgl);
+ kfree(priv->urbs);
+ kmem_cache_free(stub_priv_cache, priv);
+}
+
static struct stub_priv *stub_priv_pop(struct stub_device *sdev)
{
unsigned long flags;
@@ -314,25 +351,15 @@ static struct stub_priv *stub_priv_pop(struct stub_device *sdev)
void stub_device_cleanup_urbs(struct stub_device *sdev)
{
struct stub_priv *priv;
- struct urb *urb;
+ int i;
dev_dbg(&sdev->udev->dev, "Stub device cleaning up urbs\n");
while ((priv = stub_priv_pop(sdev))) {
- urb = priv->urb;
- dev_dbg(&sdev->udev->dev, "free urb seqnum %lu\n",
- priv->seqnum);
- usb_kill_urb(urb);
-
- kmem_cache_free(stub_priv_cache, priv);
+ for (i = 0; i < priv->num_urbs; i++)
+ usb_kill_urb(priv->urbs[i]);
- kfree(urb->transfer_buffer);
- urb->transfer_buffer = NULL;
-
- kfree(urb->setup_packet);
- urb->setup_packet = NULL;
-
- usb_free_urb(urb);
+ stub_free_priv_and_urb(priv);
}
}
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index b0a855acafa3..66edfeea68fe 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -7,6 +7,7 @@
#include <linux/kthread.h>
#include <linux/usb.h>
#include <linux/usb/hcd.h>
+#include <linux/scatterlist.h>
#include "usbip_common.h"
#include "stub.h"
@@ -201,7 +202,7 @@ static void tweak_special_requests(struct urb *urb)
static int stub_recv_cmd_unlink(struct stub_device *sdev,
struct usbip_header *pdu)
{
- int ret;
+ int ret, i;
unsigned long flags;
struct stub_priv *priv;
@@ -246,12 +247,14 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
* so a driver in a client host will know the failure
* of the unlink request ?
*/
- ret = usb_unlink_urb(priv->urb);
- if (ret != -EINPROGRESS)
- dev_err(&priv->urb->dev->dev,
- "failed to unlink a urb # %lu, ret %d\n",
- priv->seqnum, ret);
-
+ for (i = priv->completed_urbs; i < priv->num_urbs; i++) {
+ ret = usb_unlink_urb(priv->urbs[i]);
+ if (ret != -EINPROGRESS)
+ dev_err(&priv->urbs[i]->dev->dev,
+ "failed to unlink %d/%d urb of seqnum %lu, ret %d\n",
+ i + 1, priv->num_urbs,
+ priv->seqnum, ret);
+ }
return 0;
}
@@ -433,14 +436,36 @@ static void masking_bogus_flags(struct urb *urb)
urb->transfer_flags &= allowed;
}
+static int stub_recv_xbuff(struct usbip_device *ud, struct stub_priv *priv)
+{
+ int ret;
+ int i;
+
+ for (i = 0; i < priv->num_urbs; i++) {
+ ret = usbip_recv_xbuff(ud, priv->urbs[i]);
+ if (ret < 0)
+ break;
+ }
+
+ return ret;
+}
+
static void stub_recv_cmd_submit(struct stub_device *sdev,
struct usbip_header *pdu)
{
- int ret;
struct stub_priv *priv;
struct usbip_device *ud = &sdev->ud;
struct usb_device *udev = sdev->udev;
+ struct scatterlist *sgl = NULL, *sg;
+ void *buffer = NULL;
+ unsigned long long buf_len;
+ int nents;
+ int num_urbs = 1;
int pipe = get_pipe(sdev, pdu);
+ int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG;
+ int support_sg = 1;
+ int np = 0;
+ int ret, i;
if (pipe == -1)
return;
@@ -449,76 +474,139 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
if (!priv)
return;
- /* setup a urb */
- if (usb_pipeisoc(pipe))
- priv->urb = usb_alloc_urb(pdu->u.cmd_submit.number_of_packets,
- GFP_KERNEL);
- else
- priv->urb = usb_alloc_urb(0, GFP_KERNEL);
+ buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length;
- if (!priv->urb) {
- usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
- return;
+ /* allocate urb transfer buffer, if needed */
+ if (buf_len) {
+ if (use_sg) {
+ sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);
+ if (!sgl)
+ goto err_malloc;
+ } else {
+ buffer = kzalloc(buf_len, GFP_KERNEL);
+ if (!buffer)
+ goto err_malloc;
+ }
}
- /* allocate urb transfer buffer, if needed */
- if (pdu->u.cmd_submit.transfer_buffer_length > 0) {
- priv->urb->transfer_buffer =
- kzalloc(pdu->u.cmd_submit.transfer_buffer_length,
- GFP_KERNEL);
- if (!priv->urb->transfer_buffer) {
+ /* Check if the server's HCD supports SG */
+ if (use_sg && !udev->bus->sg_tablesize) {
+ /*
+ * If the server's HCD doesn't support SG, break a single SG
+ * request into several URBs and map each SG list entry to
+ * corresponding URB buffer. The previously allocated SG
+ * list is stored in priv->sgl (If the server's HCD support SG,
+ * SG list is stored only in urb->sg) and it is used as an
+ * indicator that the server split single SG request into
+ * several URBs. Later, priv->sgl is used by stub_complete() and
+ * stub_send_ret_submit() to reassemble the divied URBs.
+ */
+ support_sg = 0;
+ num_urbs = nents;
+ priv->completed_urbs = 0;
+ pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG;
+ }
+
+ /* allocate urb array */
+ priv->num_urbs = num_urbs;
+ priv->urbs = kmalloc_array(num_urbs, sizeof(*priv->urbs), GFP_KERNEL);
+ if (!priv->urbs)
+ goto err_urbs;
+
+ /* setup a urb */
+ if (support_sg) {
+ if (usb_pipeisoc(pipe))
+ np = pdu->u.cmd_submit.number_of_packets;
+
+ priv->urbs[0] = usb_alloc_urb(np, GFP_KERNEL);
+ if (!priv->urbs[0])
+ goto err_urb;
+
+ if (buf_len) {
+ if (use_sg) {
+ priv->urbs[0]->sg = sgl;
+ priv->urbs[0]->num_sgs = nents;
+ priv->urbs[0]->transfer_buffer = NULL;
+ } else {
+ priv->urbs[0]->transfer_buffer = buffer;
+ }
+ }
+
+ /* copy urb setup packet */
+ priv->urbs[0]->setup_packet = kmemdup(&pdu->u.cmd_submit.setup,
+ 8, GFP_KERNEL);
+ if (!priv->urbs[0]->setup_packet) {
usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
return;
}
- }
- /* copy urb setup packet */
- priv->urb->setup_packet = kmemdup(&pdu->u.cmd_submit.setup, 8,
- GFP_KERNEL);
- if (!priv->urb->setup_packet) {
- dev_err(&udev->dev, "allocate setup_packet\n");
- usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
- return;
+ usbip_pack_pdu(pdu, priv->urbs[0], USBIP_CMD_SUBMIT, 0);
+ } else {
+ for_each_sg(sgl, sg, nents, i) {
+ priv->urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
+ /* The URBs which is previously allocated will be freed
+ * in stub_device_cleanup_urbs() if error occurs.
+ */
+ if (!priv->urbs[i])
+ goto err_urb;
+
+ usbip_pack_pdu(pdu, priv->urbs[i], USBIP_CMD_SUBMIT, 0);
+ priv->urbs[i]->transfer_buffer = sg_virt(sg);
+ priv->urbs[i]->transfer_buffer_length = sg->length;
+ }
+ priv->sgl = sgl;
}
- /* set other members from the base header of pdu */
- priv->urb->context = (void *) priv;
- priv->urb->dev = udev;
- priv->urb->pipe = pipe;
- priv->urb->complete = stub_complete;
+ for (i = 0; i < num_urbs; i++) {
+ /* set other members from the base header of pdu */
+ priv->urbs[i]->context = (void *) priv;
+ priv->urbs[i]->dev = udev;
+ priv->urbs[i]->pipe = pipe;
+ priv->urbs[i]->complete = stub_complete;
- usbip_pack_pdu(pdu, priv->urb, USBIP_CMD_SUBMIT, 0);
+ /* no need to submit an intercepted request, but harmless? */
+ tweak_special_requests(priv->urbs[i]);
+ masking_bogus_flags(priv->urbs[i]);
+ }
- if (usbip_recv_xbuff(ud, priv->urb) < 0)
+ if (stub_recv_xbuff(ud, priv) < 0)
return;
- if (usbip_recv_iso(ud, priv->urb) < 0)
+ if (usbip_recv_iso(ud, priv->urbs[0]) < 0)
return;
- /* no need to submit an intercepted request, but harmless? */
- tweak_special_requests(priv->urb);
-
- masking_bogus_flags(priv->urb);
/* urb is now ready to submit */
- ret = usb_submit_urb(priv->urb, GFP_KERNEL);
-
- if (ret == 0)
- usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
- pdu->base.seqnum);
- else {
- dev_err(&udev->dev, "submit_urb error, %d\n", ret);
- usbip_dump_header(pdu);
- usbip_dump_urb(priv->urb);
-
- /*
- * Pessimistic.
- * This connection will be discarded.
- */
- usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
+ for (i = 0; i < priv->num_urbs; i++) {
+ ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
+
+ if (ret == 0)
+ usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
+ pdu->base.seqnum);
+ else {
+ dev_err(&udev->dev, "submit_urb error, %d\n", ret);
+ usbip_dump_header(pdu);
+ usbip_dump_urb(priv->urbs[i]);
+
+ /*
+ * Pessimistic.
+ * This connection will be discarded.
+ */
+ usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
+ break;
+ }
}
usbip_dbg_stub_rx("Leave\n");
+ return;
+
+err_urb:
+ kfree(priv->urbs);
+err_urbs:
+ kfree(buffer);
+ sgl_free(sgl);
+err_malloc:
+ usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
}
/* recv a pdu */
diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index f0ec41a50cbc..36010a82b359 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -5,25 +5,11 @@
#include <linux/kthread.h>
#include <linux/socket.h>
+#include <linux/scatterlist.h>
#include "usbip_common.h"
#include "stub.h"
-static void stub_free_priv_and_urb(struct stub_priv *priv)
-{
- struct urb *urb = priv->urb;
-
- kfree(urb->setup_packet);
- urb->setup_packet = NULL;
-
- kfree(urb->transfer_buffer);
- urb->transfer_buffer = NULL;
-
- list_del(&priv->list);
- kmem_cache_free(stub_priv_cache, priv);
- usb_free_urb(urb);
-}
-
/* be in spin_lock_irqsave(&sdev->priv_lock, flags) */
void stub_enqueue_ret_unlink(struct stub_device *sdev, __u32 seqnum,
__u32 status)
@@ -85,6 +71,22 @@ void stub_complete(struct urb *urb)
break;
}
+ /*
+ * If the server breaks single SG request into the several URBs, the
+ * URBs must be reassembled before sending completed URB to the vhci.
+ * Don't wake up the tx thread until all the URBs are completed.
+ */
+ if (priv->sgl) {
+ priv->completed_urbs++;
+
+ /* Only save the first error status */
+ if (urb->status && !priv->urb_status)
+ priv->urb_status = urb->status;
+
+ if (priv->completed_urbs < priv->num_urbs)
+ return;
+ }
+
/* link a urb to the queue of tx. */
spin_lock_irqsave(&sdev->priv_lock, flags);
if (sdev->ud.tcp_socket == NULL) {
@@ -156,18 +158,22 @@ static int stub_send_ret_submit(struct stub_device *sdev)
size_t total_size = 0;
while ((priv = dequeue_from_priv_tx(sdev)) != NULL) {
- int ret;
- struct urb *urb = priv->urb;
+ struct urb *urb = priv->urbs[0];
struct usbip_header pdu_header;
struct usbip_iso_packet_descriptor *iso_buffer = NULL;
struct kvec *iov = NULL;
+ struct scatterlist *sg;
+ u32 actual_length = 0;
int iovnum = 0;
+ int ret;
+ int i;
txsize = 0;
memset(&pdu_header, 0, sizeof(pdu_header));
memset(&msg, 0, sizeof(msg));
- if (urb->actual_length > 0 && !urb->transfer_buffer) {
+ if (urb->actual_length > 0 && !urb->transfer_buffer &&
+ !urb->num_sgs) {
dev_err(&sdev->udev->dev,
"urb: actual_length %d transfer_buffer null\n",
urb->actual_length);
@@ -176,6 +182,11 @@ static int stub_send_ret_submit(struct stub_device *sdev)
if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
iovnum = 2 + urb->number_of_packets;
+ else if (usb_pipein(urb->pipe) && urb->actual_length > 0 &&
+ urb->num_sgs)
+ iovnum = 1 + urb->num_sgs;
+ else if (usb_pipein(urb->pipe) && priv->sgl)
+ iovnum = 1 + priv->num_urbs;
else
iovnum = 2;
@@ -192,6 +203,15 @@ static int stub_send_ret_submit(struct stub_device *sdev)
setup_ret_submit_pdu(&pdu_header, urb);
usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
pdu_header.base.seqnum);
+
+ if (priv->sgl) {
+ for (i = 0; i < priv->num_urbs; i++)
+ actual_length += priv->urbs[i]->actual_length;
+
+ pdu_header.u.ret_submit.status = priv->urb_status;
+ pdu_header.u.ret_submit.actual_length = actual_length;
+ }
+
usbip_header_correct_endian(&pdu_header, 1);
iov[iovnum].iov_base = &pdu_header;
@@ -200,12 +220,47 @@ static int stub_send_ret_submit(struct stub_device *sdev)
txsize += sizeof(pdu_header);
/* 2. setup transfer buffer */
- if (usb_pipein(urb->pipe) &&
+ if (usb_pipein(urb->pipe) && priv->sgl) {
+ /* If the server split a single SG request into several
+ * URBs because the server's HCD doesn't support SG,
+ * reassemble the split URB buffers into a single
+ * return command.
+ */
+ for (i = 0; i < priv->num_urbs; i++) {
+ iov[iovnum].iov_base =
+ priv->urbs[i]->transfer_buffer;
+ iov[iovnum].iov_len =
+ priv->urbs[i]->actual_length;
+ iovnum++;
+ }
+ txsize += actual_length;
+ } else if (usb_pipein(urb->pipe) &&
usb_pipetype(urb->pipe) != PIPE_ISOCHRONOUS &&
urb->actual_length > 0) {
- iov[iovnum].iov_base = urb->transfer_buffer;
- iov[iovnum].iov_len = urb->actual_length;
- iovnum++;
+ if (urb->num_sgs) {
+ unsigned int copy = urb->actual_length;
+ int size;
+
+ for_each_sg(urb->sg, sg, urb->num_sgs, i) {
+ if (copy == 0)
+ break;
+
+ if (copy < sg->length)
+ size = copy;
+ else
+ size = sg->length;
+
+ iov[iovnum].iov_base = sg_virt(sg);
+ iov[iovnum].iov_len = size;
+
+ iovnum++;
+ copy -= size;
+ }
+ } else {
+ iov[iovnum].iov_base = urb->transfer_buffer;
+ iov[iovnum].iov_len = urb->actual_length;
+ iovnum++;
+ }
txsize += urb->actual_length;
} else if (usb_pipein(urb->pipe) &&
usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index 45da3e01c7b0..6532d68e8808 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -680,8 +680,12 @@ EXPORT_SYMBOL_GPL(usbip_pad_iso);
/* some members of urb must be substituted before. */
int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
{
- int ret;
+ struct scatterlist *sg;
+ int ret = 0;
+ int recv;
int size;
+ int copy;
+ int i;
if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
/* the direction of urb must be OUT. */
@@ -701,29 +705,48 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
if (!(size > 0))
return 0;
- if (size > urb->transfer_buffer_length) {
+ if (size > urb->transfer_buffer_length)
/* should not happen, probably malicious packet */
- if (ud->side == USBIP_STUB) {
- usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
- return 0;
- } else {
- usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
- return -EPIPE;
- }
- }
+ goto error;
- ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
- if (ret != size) {
- dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
- if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
- usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
- } else {
- usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
- return -EPIPE;
+ if (urb->num_sgs) {
+ copy = size;
+ for_each_sg(urb->sg, sg, urb->num_sgs, i) {
+ int recv_size;
+
+ if (copy < sg->length)
+ recv_size = copy;
+ else
+ recv_size = sg->length;
+
+ recv = usbip_recv(ud->tcp_socket, sg_virt(sg),
+ recv_size);
+
+ if (recv != recv_size)
+ goto error;
+
+ copy -= recv;
+ ret += recv;
}
+
+ if (ret != size)
+ goto error;
+ } else {
+ ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
+ if (ret != size)
+ goto error;
}
return ret;
+
+error:
+ dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
+ if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC)
+ usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
+ else
+ usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
+
+ return -EPIPE;
}
EXPORT_SYMBOL_GPL(usbip_recv_xbuff);
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index ea82b932a2f9..e64ab50cbe2b 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -697,7 +697,8 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
}
vdev = &vhci_hcd->vdev[portnum-1];
- if (!urb->transfer_buffer && urb->transfer_buffer_length) {
+ if (!urb->transfer_buffer && !urb->num_sgs &&
+ urb->transfer_buffer_length) {
dev_dbg(dev, "Null URB transfer buffer\n");
return -EINVAL;
}
@@ -1143,6 +1144,15 @@ static int vhci_setup(struct usb_hcd *hcd)
hcd->speed = HCD_USB3;
hcd->self.root_hub->speed = USB_SPEED_SUPER;
}
+
+ /*
+ * Support SG.
+ * sg_tablesize is an arbitrary value to alleviate memory pressure
+ * on the host.
+ */
+ hcd->self.sg_tablesize = 32;
+ hcd->self.no_sg_constraint = 1;
+
return 0;
}
@@ -1296,6 +1306,9 @@ static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
return -EINVAL;
}
+ if (urb->num_sgs)
+ urb->transfer_flags |= URB_DMA_MAP_SG;
+
return 0;
}
diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c
index 2fa26d0578d7..865eb1276b6c 100644
--- a/drivers/usb/usbip/vhci_tx.c
+++ b/drivers/usb/usbip/vhci_tx.c
@@ -5,6 +5,7 @@
#include <linux/kthread.h>
#include <linux/slab.h>
+#include <linux/scatterlist.h>
#include "usbip_common.h"
#include "vhci.h"
@@ -50,19 +51,23 @@ static struct vhci_priv *dequeue_from_priv_tx(struct vhci_device *vdev)
static int vhci_send_cmd_submit(struct vhci_device *vdev)
{
+ struct usbip_iso_packet_descriptor *iso_buffer = NULL;
struct vhci_priv *priv = NULL;
+ struct scatterlist *sg;
struct msghdr msg;
- struct kvec iov[3];
+ struct kvec *iov;
size_t txsize;
size_t total_size = 0;
+ int iovnum;
+ int err = -ENOMEM;
+ int i;
while ((priv = dequeue_from_priv_tx(vdev)) != NULL) {
int ret;
struct urb *urb = priv->urb;
struct usbip_header pdu_header;
- struct usbip_iso_packet_descriptor *iso_buffer = NULL;
txsize = 0;
memset(&pdu_header, 0, sizeof(pdu_header));
@@ -72,18 +77,42 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
usbip_dbg_vhci_tx("setup txdata urb seqnum %lu\n",
priv->seqnum);
+ if (urb->num_sgs && usb_pipeout(urb->pipe))
+ iovnum = 2 + urb->num_sgs;
+ else
+ iovnum = 3;
+
+ iov = kcalloc(iovnum, sizeof(*iov), GFP_KERNEL);
+ if (!iov) {
+ usbip_event_add(&vdev->ud, SDEV_EVENT_ERROR_MALLOC);
+ return -ENOMEM;
+ }
+
/* 1. setup usbip_header */
setup_cmd_submit_pdu(&pdu_header, urb);
usbip_header_correct_endian(&pdu_header, 1);
+ iovnum = 0;
- iov[0].iov_base = &pdu_header;
- iov[0].iov_len = sizeof(pdu_header);
+ iov[iovnum].iov_base = &pdu_header;
+ iov[iovnum].iov_len = sizeof(pdu_header);
txsize += sizeof(pdu_header);
+ iovnum++;
/* 2. setup transfer buffer */
if (!usb_pipein(urb->pipe) && urb->transfer_buffer_length > 0) {
- iov[1].iov_base = urb->transfer_buffer;
- iov[1].iov_len = urb->transfer_buffer_length;
+ if (urb->num_sgs &&
+ !usb_endpoint_xfer_isoc(&urb->ep->desc)) {
+ for_each_sg(urb->sg, sg, urb->num_sgs, i) {
+ iov[iovnum].iov_base = sg_virt(sg);
+ iov[iovnum].iov_len = sg->length;
+ iovnum++;
+ }
+ } else {
+ iov[iovnum].iov_base = urb->transfer_buffer;
+ iov[iovnum].iov_len =
+ urb->transfer_buffer_length;
+ iovnum++;
+ }
txsize += urb->transfer_buffer_length;
}
@@ -95,23 +124,26 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
if (!iso_buffer) {
usbip_event_add(&vdev->ud,
SDEV_EVENT_ERROR_MALLOC);
- return -1;
+ goto err_iso_buffer;
}
- iov[2].iov_base = iso_buffer;
- iov[2].iov_len = len;
+ iov[iovnum].iov_base = iso_buffer;
+ iov[iovnum].iov_len = len;
+ iovnum++;
txsize += len;
}
- ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, 3, txsize);
+ ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, iovnum,
+ txsize);
if (ret != txsize) {
pr_err("sendmsg failed!, ret=%d for %zd\n", ret,
txsize);
- kfree(iso_buffer);
usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_TCP);
- return -1;
+ err = -EPIPE;
+ goto err_tx;
}
+ kfree(iov);
kfree(iso_buffer);
usbip_dbg_vhci_tx("send txdata\n");
@@ -119,6 +151,13 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
}
return total_size;
+
+err_tx:
+ kfree(iso_buffer);
+err_iso_buffer:
+ kfree(iov);
+
+ return err;
}
static struct vhci_unlink *dequeue_from_unlink_tx(struct vhci_device *vdev)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] MAINTAINERS: mark wusbcore and UWB as obsolete
From: Joe Perches @ 2019-08-08 16:09 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel, devel
In-Reply-To: <20190808112358.GA25286@kroah.com>
On Thu, 2019-08-08 at 13:23 +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 08, 2019 at 04:15:44AM -0700, Joe Perches wrote:
> > On Thu, 2019-08-08 at 11:41 +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Aug 08, 2019 at 11:25:09AM +0200, Greg Kroah-Hartman wrote:
> > > > Joe rightly points out that we should be using the "Obsolete" status for
> > > > these two subsystems.
> > >
> > > Even with that change, I don't see get_maintainers.pl tell me I
> > > shouldn't be sending a patch in for this area:
> >
> > Nor should you. It's checkpatch that should warn.
>
> Ah, wrong tool. Yes, it does, let's see if anyone actually notices that
> when sending checkpatch changes for these files in the future :)
Maybe mark the isdn block obsolete too.
https://lore.kernel.org/lkml/2ecfbf8dda354fe47912446bf5c3fe30ca905aa0.camel@perches.com/
^ permalink raw reply
* Re: usb zero copy dma handling
From: Christoph Hellwig @ 2019-08-08 16:10 UTC (permalink / raw)
To: yvahkhfo.1df7f8c2; +Cc: linux-usb, linux-arm-kernel, security
In-Reply-To: <20190808084636.GB15080@priv-mua.localdomain>
On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote:
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> usbm->vma_use_count = 1;
> INIT_LIST_HEAD(&usbm->memlist);
>
> +#ifdef CONFIG_X86
> if (remap_pfn_range(vma, vma->vm_start,
> virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> size, vma->vm_page_prot) < 0) {
> +#else /* !CONFIG_X86 */
> + if (dma_mmap_coherent(ps->dev->bus->sysdev,
> + vma, mem, dma_handle, size) < 0) {
> +#endif /* !CONFIG_X86 */
Doing the dma_mmap_coherent unconditionally is the right thing here.
Gavin who is on Cc has been looking into that.
Note that you'll also need this patch which I'm going to send to Linus
this week before it properly works on x86:
http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/197b3e665b82c6027be5c68a143233df7ce5224f
^ permalink raw reply
* Re: usb zero copy dma handling
From: Christoph Hellwig @ 2019-08-08 16:12 UTC (permalink / raw)
To: yvahkhfo.1df7f8c2; +Cc: security, linux-usb, linux-arm-kernel
In-Reply-To: <20190808161015.GA8470@infradead.org>
On Thu, Aug 08, 2019 at 09:10:15AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote:
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> > usbm->vma_use_count = 1;
> > INIT_LIST_HEAD(&usbm->memlist);
> >
> > +#ifdef CONFIG_X86
> > if (remap_pfn_range(vma, vma->vm_start,
> > virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> > size, vma->vm_page_prot) < 0) {
> > +#else /* !CONFIG_X86 */
> > + if (dma_mmap_coherent(ps->dev->bus->sysdev,
> > + vma, mem, dma_handle, size) < 0) {
> > +#endif /* !CONFIG_X86 */
>
> Doing the dma_mmap_coherent unconditionally is the right thing here.
> Gavin who is on Cc has been looking into that.
Ok, tht is assuming it always is dma_alloc_* memory which apparently
it isn't. But the arch ifdef for sure is wrong.
^ permalink raw reply
* Re: [PATCH v5 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci
From: shuah @ 2019-08-08 16:18 UTC (permalink / raw)
To: Suwan Kim, valentina.manea.m, gregkh, stern
Cc: linux-usb, linux-kernel, shuah
In-Reply-To: <20190808155435.10050-2-suwan.kim027@gmail.com>
On 8/8/19 9:54 AM, Suwan Kim wrote:
> vhci doesn’t do DMA for remote device. Actually, the real DMA
> operation is done by network card driver. vhci just passes virtual
> address of the buffer to the network stack, so vhci doesn’t use and
> need dma address of the buffer of the URB.
>
> But HCD provides DMA mapping and unmapping function by default.
> Moreover, it causes unnecessary DMA mapping and unmapping which
> will be done again at the NIC driver and it wastes CPU cycles.
> So, implement map_urb_for_dma and unmap_urb_for_dma function for
> vhci in order to skip the DMA mapping and unmapping procedure.
>
> When it comes to supporting SG for vhci, it is useful to use native
> SG list (urb->num_sgs) instead of mapped SG list because DMA mapping
> fnuction can adjust the number of SG list (urb->num_mapped_sgs).
> And vhci_map_urb_for_dma() prevents isoc pipe from using SG as
> hcd_map_urb_for_dma() does.
>
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
> v4 - v5
> - Replace pr_err() with dev_err() in the error path.
>
> v3 - v4
> - Replace WARN_ON() with pr_err() in the error path.
>
> v2 - v3
> - Move setting URB_DMA_MAP_SG flag to the patch 2.
> - Prevent isoc pipe from using SG buffer.
>
> v1 - v2
> - Add setting URB_DMA_MAP_SG flag in urb->transfer_flags to tell
> stub driver to use SG buffer.
> ---
> drivers/usb/usbip/vhci_hcd.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 000ab7225717..ea82b932a2f9 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -1288,6 +1288,22 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
> return 0;
> }
>
> +static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> + gfp_t mem_flags)
> +{
> + if (usb_endpoint_xfer_isoc(&urb->ep->desc) && urb->num_sgs) {
> + dev_err(&urb->dev->dev, "SG is not supported for isochronous transfer\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static void vhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> +{
> + dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n");
> +}
> +
> static const struct hc_driver vhci_hc_driver = {
> .description = driver_name,
> .product_desc = driver_desc,
> @@ -1304,6 +1320,9 @@ static const struct hc_driver vhci_hc_driver = {
>
> .get_frame_number = vhci_get_frame_number,
>
> + .map_urb_for_dma = vhci_map_urb_for_dma,
> + .unmap_urb_for_dma = vhci_unmap_urb_for_dma,
> +
> .hub_status_data = vhci_hub_status,
> .hub_control = vhci_hub_control,
> .bus_suspend = vhci_bus_suspend,
>
Looks good to me.
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH v5 2/2] usbip: Implement SG support to vhci-hcd and stub driver
From: shuah @ 2019-08-08 16:21 UTC (permalink / raw)
To: Suwan Kim, valentina.manea.m, gregkh, stern
Cc: linux-usb, linux-kernel, shuah
In-Reply-To: <20190808155435.10050-3-suwan.kim027@gmail.com>
On 8/8/19 9:54 AM, Suwan Kim wrote:
> There are bugs on vhci with usb 3.0 storage device. In USB, each SG
> list entry buffer should be divisible by the bulk max packet size.
> But with native SG support, this problem doesn't matter because the
> SG buffer is treated as contiguous buffer. But without native SG
> support, USB storage driver breaks SG list into several URBs and the
> error occurs because of a buffer size of URB that cannot be divided
> by the bulk max packet size. The error situation is as follows.
>
> When USB Storage driver requests 31.5 KB data and has SG list which
> has 3584 bytes buffer followed by 7 4096 bytes buffer for some
> reason. USB Storage driver splits this SG list into several URBs
> because VHCI doesn't support SG and sends them separately. So the
> first URB buffer size is 3584 bytes. When receiving data from device,
> USB 3.0 device sends data packet of 1024 bytes size because the max
> packet size of BULK pipe is 1024 bytes. So device sends 4096 bytes.
> But the first URB buffer has only 3584 bytes buffer size. So host
> controller terminates the transfer even though there is more data to
> receive. So, vhci needs to support SG transfer to prevent this error.
>
> In this patch, vhci supports SG regardless of whether the server's
> host controller supports SG or not, because stub driver splits SG
> list into several URBs if the server's host controller doesn't
> support SG.
>
> To support SG, vhci_map_urb_for_dma() sets URB_DMA_MAP_SG flag in
> urb->transfer_flags if URB has SG list and this flag will tell stub
> driver to use SG list.
>
> vhci sends each SG list entry to stub driver. Then, stub driver sees
> the total length of the buffer and allocates SG table and pages
> according to the total buffer length calling sgl_alloc(). After stub
> driver receives completed URB, it again sends each SG list entry to
> vhci.
>
> If the server's host controller doesn't support SG, stub driver
> breaks a single SG request into several URBs and submits them to
> the server's host controller. When all the split URBs are completed,
> stub driver reassembles the URBs into a single return command and
> sends it to vhci.
>
> Moreover, in the situation where vhci supports SG, but stub driver
> does not, or vice versa, usbip works normally. Because there is no
> protocol modification, there is no problem in communication between
> server and client even if the one has a kernel without SG support.
>
> In the case of vhci supports SG and stub driver doesn't, because
> vhci sends only the total length of the buffer to stub driver as
> it did before the patch applied, stub driver only needs to allocate
> the required length of buffers using only kmalloc() regardless of
> whether vhci supports SG or not. But stub driver has to allocate
> buffer with kmalloc() as much as the total length of SG buffer which
> is quite huge when vhci sends SG request, so it has overhead in
> buffer allocation in this situation.
>
> If stub driver needs to send data buffer to vhci because of IN pipe,
> stub driver also sends only total length of buffer as metadata and
> then sends real data as vhci does. Then vhci receive data from stub
> driver and store it to the corresponding buffer of SG list entry.
>
> And for the case of stub driver supports SG and vhci doesn't, since
> the USB storage driver checks that vhci doesn't support SG and sends
> the request to stub driver by splitting the SG list into multiple
> URBs, stub driver allocates a buffer for each URB with kmalloc() as
> it did before this patch.
>
> * Test environment
>
> Test uses two difference machines and two different kernel version
> to make mismatch situation between the client and the server where
> vhci supports SG, but stub driver does not, or vice versa. All tests
> are conducted in both full SG support that both vhci and stub support
> SG and half SG support that is the mismatch situation.
>
> - Test kernel version
> - 5.2-rc6 with SG support
> - 5.1.20-200.fc29.x86_64 without SG support
>
> * SG support test
>
> - Test devices
> - Super-speed storage device - SanDisk Ultra USB 3.0
> - High-speed storage device - SMI corporation USB 2.0 flash drive
>
> - Test description
>
> Test read and write operation of mass storage device that uses the
> BULK transfer. In test, the client reads and writes files whose size
> is over 1G and it works normally.
>
> * Regression test
>
> - Test devices
> - Super-speed device - Logitech Brio webcam
> - High-speed device - Logitech C920 HD Pro webcam
> - Full-speed device - Logitech bluetooth mouse
> - Britz BR-Orion speaker
> - Low-speed device - Logitech wired mouse
>
> - Test description
>
> Moving and click test for mouse. To test the webcam, use gnome-cheese.
> To test the speaker, play music and video on the client. All works
> normally.
>
> * VUDC compatibility test
>
> VUDC also works well with this patch. Tests are done with two USB
> gadget created by CONFIGFS USB gadget. Both use the BULK pipe.
>
> 1. Serial gadget
> 2. Mass storage gadget
>
> - Serial gadget test
>
> Serial gadget on the host sends and receives data using cat command
> on the /dev/ttyGS<N>. The client uses minicom to communicate with
> the serial gadget.
>
> - Mass storage gadget test
>
> After connecting the gadget with vhci, use "dd" to test read and
> write operation on the client side.
>
> Read - dd if=/dev/sd<N> iflag=direct of=/dev/null bs=1G count=1
> Write - dd if=<my file path> iflag=direct of=/dev/sd<N> bs=1G count=1
>
Thanks for including the detailed test description.
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
> v4 - v5
> - Add the test description about SG support test and regression test
> for other USB devices which have various speed.
> - Fix list_del() error in stub_device_cleanup_urbs()
> - when stub_device_cleanup_urbs() calls stub_pirv_pop() to get a
> stub_priv, stub_priv_pop() uses list_del_init() instead of
> list_del(). After getting the stub_priv, stub_device_cleanup_urbs()
> calls stub_free_priv_and_urb() and it checks if list of the
> stub_priv is empty. Because stub_priv_pop() calls list_del_init(),
> stub_free_priv_and_urb() doesn't call list_del()
>
> v3 - v4
> - Rewrite the description about the vhci bug with USB 3.0 storage
> device.
> - Add the description about the test with VUDC.
> - Fix the error message in stub_recv_cmd_unlink().
>
> v2 - v3
> - Rewrite the commit log to make it more clear.
> - Add the description about the mismatch situation in commit log.
> - Run chechpatch.pl and fix errors with coding style and typos.
> - Fix the error path of usbip_recv_xbuff() to stop receiving data
> after TCP error occurs.
> - Consolidate the duplicated error path in usbip_recv_xbuff() and
> vhci_send_cmd_submit().
> - Undo the unnecessary changes
> * Undo the deletion of empty line in stub_send_ret_submit()
> * Move memset() lines in stub_send_ret_submit() to the original
> position.
>
> v1 - v2
> - Add the logic that splits single SG request into several URBs in
> stub driver if server’s HC doesn’t support SG.
> ---
> drivers/usb/usbip/stub.h | 7 +-
> drivers/usb/usbip/stub_main.c | 57 ++++++---
> drivers/usb/usbip/stub_rx.c | 204 ++++++++++++++++++++++---------
> drivers/usb/usbip/stub_tx.c | 99 +++++++++++----
> drivers/usb/usbip/usbip_common.c | 59 ++++++---
> drivers/usb/usbip/vhci_hcd.c | 15 ++-
> drivers/usb/usbip/vhci_tx.c | 63 ++++++++--
> 7 files changed, 377 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/usb/usbip/stub.h b/drivers/usb/usbip/stub.h
> index 35618ceb2791..d11270560c24 100644
> --- a/drivers/usb/usbip/stub.h
> +++ b/drivers/usb/usbip/stub.h
> @@ -52,7 +52,11 @@ struct stub_priv {
> unsigned long seqnum;
> struct list_head list;
> struct stub_device *sdev;
> - struct urb *urb;
> + struct urb **urbs;
> + struct scatterlist *sgl;
> + int num_urbs;
> + int completed_urbs;
> + int urb_status;
>
> int unlinking;
> };
> @@ -86,6 +90,7 @@ extern struct usb_device_driver stub_driver;
> struct bus_id_priv *get_busid_priv(const char *busid);
> void put_busid_priv(struct bus_id_priv *bid);
> int del_match_busid(char *busid);
> +void stub_free_priv_and_urb(struct stub_priv *priv);
> void stub_device_cleanup_urbs(struct stub_device *sdev);
>
> /* stub_rx.c */
> diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
> index 2e4bfccd4bfc..c1c0bbc9f8b1 100644
> --- a/drivers/usb/usbip/stub_main.c
> +++ b/drivers/usb/usbip/stub_main.c
> @@ -6,6 +6,7 @@
> #include <linux/string.h>
> #include <linux/module.h>
> #include <linux/device.h>
> +#include <linux/scatterlist.h>
>
> #include "usbip_common.h"
> #include "stub.h"
> @@ -281,13 +282,49 @@ static struct stub_priv *stub_priv_pop_from_listhead(struct list_head *listhead)
> struct stub_priv *priv, *tmp;
>
> list_for_each_entry_safe(priv, tmp, listhead, list) {
> - list_del(&priv->list);
> + list_del_init(&priv->list);
> return priv;
> }
>
> return NULL;
> }
>
> +void stub_free_priv_and_urb(struct stub_priv *priv)
> +{
> + struct urb *urb;
> + int i;
> +
> + for (i = 0; i < priv->num_urbs; i++) {
> + urb = priv->urbs[i];
> +
> + if (!urb)
> + return;
> +
> + kfree(urb->setup_packet);
> + urb->setup_packet = NULL;
> +
> +
> + if (urb->transfer_buffer && !priv->sgl) {
> + kfree(urb->transfer_buffer);
> + urb->transfer_buffer = NULL;
> + }
> +
> + if (urb->num_sgs) {
> + sgl_free(urb->sg);
> + urb->sg = NULL;
> + urb->num_sgs = 0;
> + }
> +
> + usb_free_urb(urb);
> + }
> + if (!list_empty(&priv->list))
> + list_del(&priv->list);
> + if (priv->sgl)
> + sgl_free(priv->sgl);
> + kfree(priv->urbs);
> + kmem_cache_free(stub_priv_cache, priv);
> +}
> +
> static struct stub_priv *stub_priv_pop(struct stub_device *sdev)
> {
> unsigned long flags;
> @@ -314,25 +351,15 @@ static struct stub_priv *stub_priv_pop(struct stub_device *sdev)
> void stub_device_cleanup_urbs(struct stub_device *sdev)
> {
> struct stub_priv *priv;
> - struct urb *urb;
> + int i;
>
> dev_dbg(&sdev->udev->dev, "Stub device cleaning up urbs\n");
>
> while ((priv = stub_priv_pop(sdev))) {
> - urb = priv->urb;
> - dev_dbg(&sdev->udev->dev, "free urb seqnum %lu\n",
> - priv->seqnum);
> - usb_kill_urb(urb);
> -
> - kmem_cache_free(stub_priv_cache, priv);
> + for (i = 0; i < priv->num_urbs; i++)
> + usb_kill_urb(priv->urbs[i]);
>
> - kfree(urb->transfer_buffer);
> - urb->transfer_buffer = NULL;
> -
> - kfree(urb->setup_packet);
> - urb->setup_packet = NULL;
> -
> - usb_free_urb(urb);
> + stub_free_priv_and_urb(priv);
> }
> }
>
> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> index b0a855acafa3..66edfeea68fe 100644
> --- a/drivers/usb/usbip/stub_rx.c
> +++ b/drivers/usb/usbip/stub_rx.c
> @@ -7,6 +7,7 @@
> #include <linux/kthread.h>
> #include <linux/usb.h>
> #include <linux/usb/hcd.h>
> +#include <linux/scatterlist.h>
>
> #include "usbip_common.h"
> #include "stub.h"
> @@ -201,7 +202,7 @@ static void tweak_special_requests(struct urb *urb)
> static int stub_recv_cmd_unlink(struct stub_device *sdev,
> struct usbip_header *pdu)
> {
> - int ret;
> + int ret, i;
> unsigned long flags;
> struct stub_priv *priv;
>
> @@ -246,12 +247,14 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
> * so a driver in a client host will know the failure
> * of the unlink request ?
> */
> - ret = usb_unlink_urb(priv->urb);
> - if (ret != -EINPROGRESS)
> - dev_err(&priv->urb->dev->dev,
> - "failed to unlink a urb # %lu, ret %d\n",
> - priv->seqnum, ret);
> -
> + for (i = priv->completed_urbs; i < priv->num_urbs; i++) {
> + ret = usb_unlink_urb(priv->urbs[i]);
> + if (ret != -EINPROGRESS)
> + dev_err(&priv->urbs[i]->dev->dev,
> + "failed to unlink %d/%d urb of seqnum %lu, ret %d\n",
> + i + 1, priv->num_urbs,
> + priv->seqnum, ret);
> + }
> return 0;
> }
>
> @@ -433,14 +436,36 @@ static void masking_bogus_flags(struct urb *urb)
> urb->transfer_flags &= allowed;
> }
>
> +static int stub_recv_xbuff(struct usbip_device *ud, struct stub_priv *priv)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < priv->num_urbs; i++) {
> + ret = usbip_recv_xbuff(ud, priv->urbs[i]);
> + if (ret < 0)
> + break;
> + }
> +
> + return ret;
> +}
> +
> static void stub_recv_cmd_submit(struct stub_device *sdev,
> struct usbip_header *pdu)
> {
> - int ret;
> struct stub_priv *priv;
> struct usbip_device *ud = &sdev->ud;
> struct usb_device *udev = sdev->udev;
> + struct scatterlist *sgl = NULL, *sg;
> + void *buffer = NULL;
> + unsigned long long buf_len;
> + int nents;
> + int num_urbs = 1;
> int pipe = get_pipe(sdev, pdu);
> + int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG;
> + int support_sg = 1;
> + int np = 0;
> + int ret, i;
>
> if (pipe == -1)
> return;
> @@ -449,76 +474,139 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
> if (!priv)
> return;
>
> - /* setup a urb */
> - if (usb_pipeisoc(pipe))
> - priv->urb = usb_alloc_urb(pdu->u.cmd_submit.number_of_packets,
> - GFP_KERNEL);
> - else
> - priv->urb = usb_alloc_urb(0, GFP_KERNEL);
> + buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length;
>
> - if (!priv->urb) {
> - usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> - return;
> + /* allocate urb transfer buffer, if needed */
> + if (buf_len) {
> + if (use_sg) {
> + sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);
> + if (!sgl)
> + goto err_malloc;
> + } else {
> + buffer = kzalloc(buf_len, GFP_KERNEL);
> + if (!buffer)
> + goto err_malloc;
> + }
> }
>
> - /* allocate urb transfer buffer, if needed */
> - if (pdu->u.cmd_submit.transfer_buffer_length > 0) {
> - priv->urb->transfer_buffer =
> - kzalloc(pdu->u.cmd_submit.transfer_buffer_length,
> - GFP_KERNEL);
> - if (!priv->urb->transfer_buffer) {
> + /* Check if the server's HCD supports SG */
> + if (use_sg && !udev->bus->sg_tablesize) {
> + /*
> + * If the server's HCD doesn't support SG, break a single SG
> + * request into several URBs and map each SG list entry to
> + * corresponding URB buffer. The previously allocated SG
> + * list is stored in priv->sgl (If the server's HCD support SG,
> + * SG list is stored only in urb->sg) and it is used as an
> + * indicator that the server split single SG request into
> + * several URBs. Later, priv->sgl is used by stub_complete() and
> + * stub_send_ret_submit() to reassemble the divied URBs.
> + */
> + support_sg = 0;
> + num_urbs = nents;
> + priv->completed_urbs = 0;
> + pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG;
> + }
> +
> + /* allocate urb array */
> + priv->num_urbs = num_urbs;
> + priv->urbs = kmalloc_array(num_urbs, sizeof(*priv->urbs), GFP_KERNEL);
> + if (!priv->urbs)
> + goto err_urbs;
> +
> + /* setup a urb */
> + if (support_sg) {
> + if (usb_pipeisoc(pipe))
> + np = pdu->u.cmd_submit.number_of_packets;
> +
> + priv->urbs[0] = usb_alloc_urb(np, GFP_KERNEL);
> + if (!priv->urbs[0])
> + goto err_urb;
> +
> + if (buf_len) {
> + if (use_sg) {
> + priv->urbs[0]->sg = sgl;
> + priv->urbs[0]->num_sgs = nents;
> + priv->urbs[0]->transfer_buffer = NULL;
> + } else {
> + priv->urbs[0]->transfer_buffer = buffer;
> + }
> + }
> +
> + /* copy urb setup packet */
> + priv->urbs[0]->setup_packet = kmemdup(&pdu->u.cmd_submit.setup,
> + 8, GFP_KERNEL);
> + if (!priv->urbs[0]->setup_packet) {
> usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> return;
> }
> - }
>
> - /* copy urb setup packet */
> - priv->urb->setup_packet = kmemdup(&pdu->u.cmd_submit.setup, 8,
> - GFP_KERNEL);
> - if (!priv->urb->setup_packet) {
> - dev_err(&udev->dev, "allocate setup_packet\n");
> - usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> - return;
> + usbip_pack_pdu(pdu, priv->urbs[0], USBIP_CMD_SUBMIT, 0);
> + } else {
> + for_each_sg(sgl, sg, nents, i) {
> + priv->urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
> + /* The URBs which is previously allocated will be freed
> + * in stub_device_cleanup_urbs() if error occurs.
> + */
> + if (!priv->urbs[i])
> + goto err_urb;
> +
> + usbip_pack_pdu(pdu, priv->urbs[i], USBIP_CMD_SUBMIT, 0);
> + priv->urbs[i]->transfer_buffer = sg_virt(sg);
> + priv->urbs[i]->transfer_buffer_length = sg->length;
> + }
> + priv->sgl = sgl;
> }
>
> - /* set other members from the base header of pdu */
> - priv->urb->context = (void *) priv;
> - priv->urb->dev = udev;
> - priv->urb->pipe = pipe;
> - priv->urb->complete = stub_complete;
> + for (i = 0; i < num_urbs; i++) {
> + /* set other members from the base header of pdu */
> + priv->urbs[i]->context = (void *) priv;
> + priv->urbs[i]->dev = udev;
> + priv->urbs[i]->pipe = pipe;
> + priv->urbs[i]->complete = stub_complete;
>
> - usbip_pack_pdu(pdu, priv->urb, USBIP_CMD_SUBMIT, 0);
> + /* no need to submit an intercepted request, but harmless? */
> + tweak_special_requests(priv->urbs[i]);
>
> + masking_bogus_flags(priv->urbs[i]);
> + }
>
> - if (usbip_recv_xbuff(ud, priv->urb) < 0)
> + if (stub_recv_xbuff(ud, priv) < 0)
> return;
>
> - if (usbip_recv_iso(ud, priv->urb) < 0)
> + if (usbip_recv_iso(ud, priv->urbs[0]) < 0)
> return;
>
> - /* no need to submit an intercepted request, but harmless? */
> - tweak_special_requests(priv->urb);
> -
> - masking_bogus_flags(priv->urb);
> /* urb is now ready to submit */
> - ret = usb_submit_urb(priv->urb, GFP_KERNEL);
> -
> - if (ret == 0)
> - usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> - pdu->base.seqnum);
> - else {
> - dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> - usbip_dump_header(pdu);
> - usbip_dump_urb(priv->urb);
> -
> - /*
> - * Pessimistic.
> - * This connection will be discarded.
> - */
> - usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> + for (i = 0; i < priv->num_urbs; i++) {
> + ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
> +
> + if (ret == 0)
> + usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> + pdu->base.seqnum);
> + else {
> + dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> + usbip_dump_header(pdu);
> + usbip_dump_urb(priv->urbs[i]);
> +
> + /*
> + * Pessimistic.
> + * This connection will be discarded.
> + */
> + usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> + break;
> + }
> }
>
> usbip_dbg_stub_rx("Leave\n");
> + return;
> +
> +err_urb:
> + kfree(priv->urbs);
> +err_urbs:
> + kfree(buffer);
> + sgl_free(sgl);
> +err_malloc:
> + usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> }
>
> /* recv a pdu */
> diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
> index f0ec41a50cbc..36010a82b359 100644
> --- a/drivers/usb/usbip/stub_tx.c
> +++ b/drivers/usb/usbip/stub_tx.c
> @@ -5,25 +5,11 @@
>
> #include <linux/kthread.h>
> #include <linux/socket.h>
> +#include <linux/scatterlist.h>
>
> #include "usbip_common.h"
> #include "stub.h"
>
> -static void stub_free_priv_and_urb(struct stub_priv *priv)
> -{
> - struct urb *urb = priv->urb;
> -
> - kfree(urb->setup_packet);
> - urb->setup_packet = NULL;
> -
> - kfree(urb->transfer_buffer);
> - urb->transfer_buffer = NULL;
> -
> - list_del(&priv->list);
> - kmem_cache_free(stub_priv_cache, priv);
> - usb_free_urb(urb);
> -}
> -
> /* be in spin_lock_irqsave(&sdev->priv_lock, flags) */
> void stub_enqueue_ret_unlink(struct stub_device *sdev, __u32 seqnum,
> __u32 status)
> @@ -85,6 +71,22 @@ void stub_complete(struct urb *urb)
> break;
> }
>
> + /*
> + * If the server breaks single SG request into the several URBs, the
> + * URBs must be reassembled before sending completed URB to the vhci.
> + * Don't wake up the tx thread until all the URBs are completed.
> + */
> + if (priv->sgl) {
> + priv->completed_urbs++;
> +
> + /* Only save the first error status */
> + if (urb->status && !priv->urb_status)
> + priv->urb_status = urb->status;
> +
> + if (priv->completed_urbs < priv->num_urbs)
> + return;
> + }
> +
> /* link a urb to the queue of tx. */
> spin_lock_irqsave(&sdev->priv_lock, flags);
> if (sdev->ud.tcp_socket == NULL) {
> @@ -156,18 +158,22 @@ static int stub_send_ret_submit(struct stub_device *sdev)
> size_t total_size = 0;
>
> while ((priv = dequeue_from_priv_tx(sdev)) != NULL) {
> - int ret;
> - struct urb *urb = priv->urb;
> + struct urb *urb = priv->urbs[0];
> struct usbip_header pdu_header;
> struct usbip_iso_packet_descriptor *iso_buffer = NULL;
> struct kvec *iov = NULL;
> + struct scatterlist *sg;
> + u32 actual_length = 0;
> int iovnum = 0;
> + int ret;
> + int i;
>
> txsize = 0;
> memset(&pdu_header, 0, sizeof(pdu_header));
> memset(&msg, 0, sizeof(msg));
>
> - if (urb->actual_length > 0 && !urb->transfer_buffer) {
> + if (urb->actual_length > 0 && !urb->transfer_buffer &&
> + !urb->num_sgs) {
> dev_err(&sdev->udev->dev,
> "urb: actual_length %d transfer_buffer null\n",
> urb->actual_length);
> @@ -176,6 +182,11 @@ static int stub_send_ret_submit(struct stub_device *sdev)
>
> if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
> iovnum = 2 + urb->number_of_packets;
> + else if (usb_pipein(urb->pipe) && urb->actual_length > 0 &&
> + urb->num_sgs)
> + iovnum = 1 + urb->num_sgs;
> + else if (usb_pipein(urb->pipe) && priv->sgl)
> + iovnum = 1 + priv->num_urbs;
> else
> iovnum = 2;
>
> @@ -192,6 +203,15 @@ static int stub_send_ret_submit(struct stub_device *sdev)
> setup_ret_submit_pdu(&pdu_header, urb);
> usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
> pdu_header.base.seqnum);
> +
> + if (priv->sgl) {
> + for (i = 0; i < priv->num_urbs; i++)
> + actual_length += priv->urbs[i]->actual_length;
> +
> + pdu_header.u.ret_submit.status = priv->urb_status;
> + pdu_header.u.ret_submit.actual_length = actual_length;
> + }
> +
> usbip_header_correct_endian(&pdu_header, 1);
>
> iov[iovnum].iov_base = &pdu_header;
> @@ -200,12 +220,47 @@ static int stub_send_ret_submit(struct stub_device *sdev)
> txsize += sizeof(pdu_header);
>
> /* 2. setup transfer buffer */
> - if (usb_pipein(urb->pipe) &&
> + if (usb_pipein(urb->pipe) && priv->sgl) {
> + /* If the server split a single SG request into several
> + * URBs because the server's HCD doesn't support SG,
> + * reassemble the split URB buffers into a single
> + * return command.
> + */
> + for (i = 0; i < priv->num_urbs; i++) {
> + iov[iovnum].iov_base =
> + priv->urbs[i]->transfer_buffer;
> + iov[iovnum].iov_len =
> + priv->urbs[i]->actual_length;
> + iovnum++;
> + }
> + txsize += actual_length;
> + } else if (usb_pipein(urb->pipe) &&
> usb_pipetype(urb->pipe) != PIPE_ISOCHRONOUS &&
> urb->actual_length > 0) {
> - iov[iovnum].iov_base = urb->transfer_buffer;
> - iov[iovnum].iov_len = urb->actual_length;
> - iovnum++;
> + if (urb->num_sgs) {
> + unsigned int copy = urb->actual_length;
> + int size;
> +
> + for_each_sg(urb->sg, sg, urb->num_sgs, i) {
> + if (copy == 0)
> + break;
> +
> + if (copy < sg->length)
> + size = copy;
> + else
> + size = sg->length;
> +
> + iov[iovnum].iov_base = sg_virt(sg);
> + iov[iovnum].iov_len = size;
> +
> + iovnum++;
> + copy -= size;
> + }
> + } else {
> + iov[iovnum].iov_base = urb->transfer_buffer;
> + iov[iovnum].iov_len = urb->actual_length;
> + iovnum++;
> + }
> txsize += urb->actual_length;
> } else if (usb_pipein(urb->pipe) &&
> usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
> diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
> index 45da3e01c7b0..6532d68e8808 100644
> --- a/drivers/usb/usbip/usbip_common.c
> +++ b/drivers/usb/usbip/usbip_common.c
> @@ -680,8 +680,12 @@ EXPORT_SYMBOL_GPL(usbip_pad_iso);
> /* some members of urb must be substituted before. */
> int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
> {
> - int ret;
> + struct scatterlist *sg;
> + int ret = 0;
> + int recv;
> int size;
> + int copy;
> + int i;
>
> if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
> /* the direction of urb must be OUT. */
> @@ -701,29 +705,48 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
> if (!(size > 0))
> return 0;
>
> - if (size > urb->transfer_buffer_length) {
> + if (size > urb->transfer_buffer_length)
> /* should not happen, probably malicious packet */
> - if (ud->side == USBIP_STUB) {
> - usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> - return 0;
> - } else {
> - usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> - return -EPIPE;
> - }
> - }
> + goto error;
>
> - ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
> - if (ret != size) {
> - dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> - if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
> - usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> - } else {
> - usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> - return -EPIPE;
> + if (urb->num_sgs) {
> + copy = size;
> + for_each_sg(urb->sg, sg, urb->num_sgs, i) {
> + int recv_size;
> +
> + if (copy < sg->length)
> + recv_size = copy;
> + else
> + recv_size = sg->length;
> +
> + recv = usbip_recv(ud->tcp_socket, sg_virt(sg),
> + recv_size);
> +
> + if (recv != recv_size)
> + goto error;
> +
> + copy -= recv;
> + ret += recv;
> }
> +
> + if (ret != size)
> + goto error;
> + } else {
> + ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
> + if (ret != size)
> + goto error;
> }
>
> return ret;
> +
> +error:
> + dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> + if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC)
> + usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> + else
> + usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> +
> + return -EPIPE;
> }
> EXPORT_SYMBOL_GPL(usbip_recv_xbuff);
>
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index ea82b932a2f9..e64ab50cbe2b 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -697,7 +697,8 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
> }
> vdev = &vhci_hcd->vdev[portnum-1];
>
> - if (!urb->transfer_buffer && urb->transfer_buffer_length) {
> + if (!urb->transfer_buffer && !urb->num_sgs &&
> + urb->transfer_buffer_length) {
> dev_dbg(dev, "Null URB transfer buffer\n");
> return -EINVAL;
> }
> @@ -1143,6 +1144,15 @@ static int vhci_setup(struct usb_hcd *hcd)
> hcd->speed = HCD_USB3;
> hcd->self.root_hub->speed = USB_SPEED_SUPER;
> }
> +
> + /*
> + * Support SG.
> + * sg_tablesize is an arbitrary value to alleviate memory pressure
> + * on the host.
> + */
> + hcd->self.sg_tablesize = 32;
> + hcd->self.no_sg_constraint = 1;
> +
> return 0;
> }
>
> @@ -1296,6 +1306,9 @@ static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> return -EINVAL;
> }
>
> + if (urb->num_sgs)
> + urb->transfer_flags |= URB_DMA_MAP_SG;
> +
> return 0;
> }
>
> diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c
> index 2fa26d0578d7..865eb1276b6c 100644
> --- a/drivers/usb/usbip/vhci_tx.c
> +++ b/drivers/usb/usbip/vhci_tx.c
> @@ -5,6 +5,7 @@
>
> #include <linux/kthread.h>
> #include <linux/slab.h>
> +#include <linux/scatterlist.h>
>
> #include "usbip_common.h"
> #include "vhci.h"
> @@ -50,19 +51,23 @@ static struct vhci_priv *dequeue_from_priv_tx(struct vhci_device *vdev)
>
> static int vhci_send_cmd_submit(struct vhci_device *vdev)
> {
> + struct usbip_iso_packet_descriptor *iso_buffer = NULL;
> struct vhci_priv *priv = NULL;
> + struct scatterlist *sg;
>
> struct msghdr msg;
> - struct kvec iov[3];
> + struct kvec *iov;
> size_t txsize;
>
> size_t total_size = 0;
> + int iovnum;
> + int err = -ENOMEM;
> + int i;
>
> while ((priv = dequeue_from_priv_tx(vdev)) != NULL) {
> int ret;
> struct urb *urb = priv->urb;
> struct usbip_header pdu_header;
> - struct usbip_iso_packet_descriptor *iso_buffer = NULL;
>
> txsize = 0;
> memset(&pdu_header, 0, sizeof(pdu_header));
> @@ -72,18 +77,42 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
> usbip_dbg_vhci_tx("setup txdata urb seqnum %lu\n",
> priv->seqnum);
>
> + if (urb->num_sgs && usb_pipeout(urb->pipe))
> + iovnum = 2 + urb->num_sgs;
> + else
> + iovnum = 3;
> +
> + iov = kcalloc(iovnum, sizeof(*iov), GFP_KERNEL);
> + if (!iov) {
> + usbip_event_add(&vdev->ud, SDEV_EVENT_ERROR_MALLOC);
> + return -ENOMEM;
> + }
> +
> /* 1. setup usbip_header */
> setup_cmd_submit_pdu(&pdu_header, urb);
> usbip_header_correct_endian(&pdu_header, 1);
> + iovnum = 0;
>
> - iov[0].iov_base = &pdu_header;
> - iov[0].iov_len = sizeof(pdu_header);
> + iov[iovnum].iov_base = &pdu_header;
> + iov[iovnum].iov_len = sizeof(pdu_header);
> txsize += sizeof(pdu_header);
> + iovnum++;
>
> /* 2. setup transfer buffer */
> if (!usb_pipein(urb->pipe) && urb->transfer_buffer_length > 0) {
> - iov[1].iov_base = urb->transfer_buffer;
> - iov[1].iov_len = urb->transfer_buffer_length;
> + if (urb->num_sgs &&
> + !usb_endpoint_xfer_isoc(&urb->ep->desc)) {
> + for_each_sg(urb->sg, sg, urb->num_sgs, i) {
> + iov[iovnum].iov_base = sg_virt(sg);
> + iov[iovnum].iov_len = sg->length;
> + iovnum++;
> + }
> + } else {
> + iov[iovnum].iov_base = urb->transfer_buffer;
> + iov[iovnum].iov_len =
> + urb->transfer_buffer_length;
> + iovnum++;
> + }
> txsize += urb->transfer_buffer_length;
> }
>
> @@ -95,23 +124,26 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
> if (!iso_buffer) {
> usbip_event_add(&vdev->ud,
> SDEV_EVENT_ERROR_MALLOC);
> - return -1;
> + goto err_iso_buffer;
> }
>
> - iov[2].iov_base = iso_buffer;
> - iov[2].iov_len = len;
> + iov[iovnum].iov_base = iso_buffer;
> + iov[iovnum].iov_len = len;
> + iovnum++;
> txsize += len;
> }
>
> - ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, 3, txsize);
> + ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, iovnum,
> + txsize);
> if (ret != txsize) {
> pr_err("sendmsg failed!, ret=%d for %zd\n", ret,
> txsize);
> - kfree(iso_buffer);
> usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_TCP);
> - return -1;
> + err = -EPIPE;
> + goto err_tx;
> }
>
> + kfree(iov);
> kfree(iso_buffer);
> usbip_dbg_vhci_tx("send txdata\n");
>
> @@ -119,6 +151,13 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
> }
>
> return total_size;
> +
> +err_tx:
> + kfree(iso_buffer);
> +err_iso_buffer:
> + kfree(iov);
> +
> + return err;
> }
>
> static struct vhci_unlink *dequeue_from_unlink_tx(struct vhci_device *vdev)
>
Thanks for doing this work.
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
thanks,
-- Shuah
^ permalink raw reply
* Re: usb zero copy dma handling
From: Russell King - ARM Linux admin @ 2019-08-08 16:57 UTC (permalink / raw)
To: Christoph Hellwig
Cc: yvahkhfo.1df7f8c2, security, linux-usb, linux-arm-kernel
In-Reply-To: <20190808161015.GA8470@infradead.org>
On Thu, Aug 08, 2019 at 09:10:15AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote:
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> > usbm->vma_use_count = 1;
> > INIT_LIST_HEAD(&usbm->memlist);
> >
> > +#ifdef CONFIG_X86
> > if (remap_pfn_range(vma, vma->vm_start,
> > virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> > size, vma->vm_page_prot) < 0) {
> > +#else /* !CONFIG_X86 */
> > + if (dma_mmap_coherent(ps->dev->bus->sysdev,
> > + vma, mem, dma_handle, size) < 0) {
> > +#endif /* !CONFIG_X86 */
>
> Doing the dma_mmap_coherent unconditionally is the right thing here.
So what if usbm->mem is from kmalloc because the host doesn't support DMA?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* [PATCH] USB: rio500: Fix lockdep violation
From: Alan Stern @ 2019-08-08 17:34 UTC (permalink / raw)
To: Greg KH; +Cc: Andrey Konovalov, Oliver Neukum, syzkaller-bugs, USB list
In-Reply-To: <CAAeHK+z7a3R+Lvo_0VeeMYZZqjWXgcX0qBmi0-Gcx=rDQsBPNA@mail.gmail.com>
The syzbot fuzzer found a lockdep violation in the rio500 driver:
======================================================
WARNING: possible circular locking dependency detected
5.3.0-rc2+ #23 Not tainted
------------------------------------------------------
syz-executor.2/20386 is trying to acquire lock:
00000000772249c6 (rio500_mutex){+.+.}, at: open_rio+0x16/0xc0
drivers/usb/misc/rio500.c:64
but task is already holding lock:
00000000d3e8f4b9 (minor_rwsem){++++}, at: usb_open+0x23/0x270
drivers/usb/core/file.c:39
which lock already depends on the new lock.
The problem is that the driver's open_rio() routine is called while
the usbcore's minor_rwsem is locked for reading, and it acquires the
rio500_mutex; whereas conversely, probe_rio() and disconnect_rio()
first acquire the rio500_mutex and then call usb_register_dev() or
usb_deregister_dev(), which lock minor_rwsem for writing.
The correct ordering of acquisition should be: minor_rwsem first, then
rio500_mutex (since the locking in open_rio() cannot be changed).
Thus, the probe and disconnect routines should avoid holding
rio500_mutex while doing their registration and deregistration.
This patch adjusts the code in those two routines to do just that. It
also relies on the fact that the probe and disconnect routines are
protected by the device mutex, so the initial test of rio->present
needs no extra locking.
Reported-by: syzbot+7bbcbe9c9ff0cd49592a@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Fixes: d710734b0677 ("USB: rio500: simplify locking")
CC: Oliver Neukum <oneukum@suse.com>
CC: <stable@vger.kernel.org>
---
This patch is different from the one I posted earlier. I realized that
we don't want to register the device's char file until after the
buffers have been allocated.
[as1906]
drivers/usb/misc/rio500.c | 66 ++++++++++++++++++++++++----------------------
1 file changed, 35 insertions(+), 31 deletions(-)
Index: usb-devel/drivers/usb/misc/rio500.c
===================================================================
--- usb-devel.orig/drivers/usb/misc/rio500.c
+++ usb-devel/drivers/usb/misc/rio500.c
@@ -454,51 +454,55 @@ static int probe_rio(struct usb_interfac
{
struct usb_device *dev = interface_to_usbdev(intf);
struct rio_usb_data *rio = &rio_instance;
- int retval = 0;
+ int retval = -ENOMEM;
+ char *ibuf, *obuf;
- mutex_lock(&rio500_mutex);
if (rio->present) {
dev_info(&intf->dev, "Second USB Rio at address %d refused\n", dev->devnum);
- retval = -EBUSY;
- goto bail_out;
- } else {
- dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
+ return -EBUSY;
}
+ dev_info(&intf->dev, "USB Rio found at address %d\n", dev->devnum);
- retval = usb_register_dev(intf, &usb_rio_class);
- if (retval) {
- dev_err(&dev->dev,
- "Not able to get a minor for this device.\n");
- retval = -ENOMEM;
- goto bail_out;
- }
-
- rio->rio_dev = dev;
-
- if (!(rio->obuf = kmalloc(OBUF_SIZE, GFP_KERNEL))) {
+ obuf = kmalloc(OBUF_SIZE, GFP_KERNEL);
+ if (!obuf) {
dev_err(&dev->dev,
"probe_rio: Not enough memory for the output buffer\n");
- usb_deregister_dev(intf, &usb_rio_class);
- retval = -ENOMEM;
- goto bail_out;
+ goto err_obuf;
}
- dev_dbg(&intf->dev, "obuf address:%p\n", rio->obuf);
+ dev_dbg(&intf->dev, "obuf address: %p\n", obuf);
- if (!(rio->ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL))) {
+ ibuf = kmalloc(IBUF_SIZE, GFP_KERNEL);
+ if (!ibuf) {
dev_err(&dev->dev,
"probe_rio: Not enough memory for the input buffer\n");
- usb_deregister_dev(intf, &usb_rio_class);
- kfree(rio->obuf);
- retval = -ENOMEM;
- goto bail_out;
+ goto err_ibuf;
}
- dev_dbg(&intf->dev, "ibuf address:%p\n", rio->ibuf);
+ dev_dbg(&intf->dev, "ibuf address: %p\n", ibuf);
- usb_set_intfdata (intf, rio);
+ mutex_lock(&rio500_mutex);
+ rio->rio_dev = dev;
+ rio->ibuf = ibuf;
+ rio->obuf = obuf;
rio->present = 1;
-bail_out:
mutex_unlock(&rio500_mutex);
+ retval = usb_register_dev(intf, &usb_rio_class);
+ if (retval) {
+ dev_err(&dev->dev,
+ "Not able to get a minor for this device.\n");
+ goto err_register;
+ }
+
+ usb_set_intfdata(intf, rio);
+ return retval;
+
+ err_register:
+ mutex_lock(&rio500_mutex);
+ rio->present = 0;
+ mutex_unlock(&rio500_mutex);
+ err_ibuf:
+ kfree(obuf);
+ err_obuf:
return retval;
}
@@ -507,10 +511,10 @@ static void disconnect_rio(struct usb_in
struct rio_usb_data *rio = usb_get_intfdata (intf);
usb_set_intfdata (intf, NULL);
- mutex_lock(&rio500_mutex);
if (rio) {
usb_deregister_dev(intf, &usb_rio_class);
+ mutex_lock(&rio500_mutex);
if (rio->isopen) {
rio->isopen = 0;
/* better let it finish - the release will do whats needed */
@@ -524,8 +528,8 @@ static void disconnect_rio(struct usb_in
dev_info(&intf->dev, "USB Rio disconnected.\n");
rio->present = 0;
+ mutex_unlock(&rio500_mutex);
}
- mutex_unlock(&rio500_mutex);
}
static const struct usb_device_id rio_table[] = {
^ permalink raw reply
* Re: possible deadlock in iowarrior
From: Alan Stern @ 2019-08-08 17:43 UTC (permalink / raw)
To: Oliver Neukum; +Cc: andreyknvl, syzkaller-bugs, gregkh, syzbot, USB list
In-Reply-To: <1565187142.15973.3.camel@neukum.org>
On Wed, 7 Aug 2019, Oliver Neukum wrote:
> > PS: syzbot reported a similar lock inversion problem (involving two
> > mutexes rather than just one) in drivers/usb/misc/iowarrior.c.
> > Probably the two drivers need similar fixes.
>
> No, but I got a fix.
>
> Regards
> Oliver
>
> From 973e82b3f583113e4d7fe5cd2f918a16022c4e38 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Tue, 6 Aug 2019 16:17:54 +0200
> Subject: [PATCH] usb: iowarrior: fix deadlock on disconnect
>
> We have to drop the mutex before we close() upon disconnect()
> as close() needs the lock. This is safe to do by dropping the
> mutex as intfdata is already set to NULL, so open() will fail.
>
> Fixes: 03f36e885fc26 ("USB: open disconnect race in iowarrior")
> Reported-by: syzbot+a64a382964bf6c71a9c0@syzkaller.appspotmail.com
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
> drivers/usb/misc/iowarrior.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
> index ba05dd80a020..f5bed9f29e56 100644
> --- a/drivers/usb/misc/iowarrior.c
> +++ b/drivers/usb/misc/iowarrior.c
> @@ -866,19 +866,20 @@ static void iowarrior_disconnect(struct usb_interface *interface)
> dev = usb_get_intfdata(interface);
> mutex_lock(&iowarrior_open_disc_lock);
> usb_set_intfdata(interface, NULL);
> + /* prevent device read, write and ioctl */
> + dev->present = 0;
>
> minor = dev->minor;
> + mutex_unlock(&iowarrior_open_disc_lock);
> + /* give back our minor - this will call close() locks need to be dropped at this point*/
Ungrammatical and untrue: usb_deregister_dev() does not call close().
>
> - /* give back our minor */
> usb_deregister_dev(interface, &iowarrior_class);
>
> mutex_lock(&dev->mutex);
>
> /* prevent device read, write and ioctl */
> - dev->present = 0;
>
> mutex_unlock(&dev->mutex);
> - mutex_unlock(&iowarrior_open_disc_lock);
That part looks weird. The critical section is empty, except for a
comment. Maybe you meant to lock dev->mutex around the assignment to
dev->present above?
In any case, this driver still seems to have at least one unnecessary
mutex. Look at the locking mess in iowarrior_open().
Alan Stern
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox