linux-kernel - Re: usb: dwc2: Disable all EP's on disconnect

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Sun, 28 Oct 2018 17:23:42 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Minas.Harutyunyan@...opsys.com
Cc:     Felipe Balbi <balbi@...nel.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        USB list <linux-usb@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: usb: dwc2: Disable all EP's on disconnect

Hi Minas,

On Fri, Oct 26, 2018 at 6:59 PM Linux Kernel Mailing List
<linux-kernel@...r.kernel.org> wrote:
> Commit:     dccf1bad4be7eaa096c1f3697bd37883f9a08ecb
> Parent:     4c19cc14064d99ef0a20fb5ba0d45c94dbedb13c
> Refname:    refs/heads/master
> Web:        https://git.kernel.org/torvalds/c/dccf1bad4be7eaa096c1f3697bd37883f9a08ecb
> Author:     Minas Harutyunyan <Minas.Harutyunyan@...opsys.com>
> AuthorDate: Wed Sep 19 18:13:52 2018 +0400
> Committer:  Felipe Balbi <felipe.balbi@...ux.intel.com>
> CommitDate: Tue Oct 2 10:33:15 2018 +0300
>
>     usb: dwc2: Disable all EP's on disconnect
>
>     Disabling all EP's allow to reset EP's to initial state.
>     On disconnect disable all EP's instead of just killing
>     all requests. Because of some platform didn't catch
>     disconnect event, same stuff added to
>     dwc2_hsotg_core_init_disconnected() function when USB
>     reset detected on the bus.
>
>     Changed from version 1:
>     Changed lock acquire flow in dwc2_hsotg_ep_disable()
>     function.

Dan had some comments on v1, and requested to document why the locking
games you're playing are safe, cfr.
https://lists.01.org/pipermail/kbuild/2018-September/002945.html

Unfortunately you didn't CC him for v2...

> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c

> @@ -3993,6 +4004,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>         unsigned long flags;
>         u32 epctrl_reg;
>         u32 ctrl;
> +       int locked;
>
>         dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>
> @@ -4008,7 +4020,9 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>
>         epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>
> -       spin_lock_irqsave(&hsotg->lock, flags);
> +       locked = spin_is_locked(&hsotg->lock);
> +       if (!locked)
> +               spin_lock_irqsave(&hsotg->lock, flags);

This looks really fishy to me.
What if another CPU takes the lock in between?

Can you please explain, and fix if necessary?

Thanks!

>
>         ctrl = dwc2_readl(hsotg, epctrl_reg);
>
> @@ -4032,7 +4046,9 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>         hs_ep->fifo_index = 0;
>         hs_ep->fifo_size = 0;
>
> -       spin_unlock_irqrestore(&hsotg->lock, flags);
> +       if (!locked)
> +               spin_unlock_irqrestore(&hsotg->lock, flags);
> +
>         return 0;
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds