| 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
| ||
|
Date: Fri, 7 Dec 2018 14:13:18 +0000
From: Minas Harutyunyan <minas.harutyunyan@...opsys.com>
To: Dan Carpenter <dan.carpenter@...cle.com>,
Minas Harutyunyan <minas.harutyunyan@...opsys.com>
CC: Marek Szyprowski <m.szyprowski@...sung.com>,
Maynard CABIENTE <maynard.cabiente@...itan.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Felipe Balbi <felipe.balbi@...ux.intel.com>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on
disconnect"
Hi Dan,
On 12/7/2018 3:20 PM, Minas Harutyunyan wrote:
> Hi Dan,
>
> On 12/7/2018 2:16 PM, Dan Carpenter wrote:
>> On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:
>>> Hi,
>>>
>>> On 12/4/2018 5:29 PM, Dan Carpenter wrote:
>>>> On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:
>>>>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>>>>> hsotg->connected = 0;
>>>>> hsotg->test_mode = 0;
>>>>>
>>>>> - /* all endpoints should be shutdown */
>>>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>>>> if (hsotg->eps_in[ep])
>>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>>>>> + -ESHUTDOWN);
>>>>> if (hsotg->eps_out[ep])
>>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>>>>> + -ESHUTDOWN);
>>>>
>>>>
>>>> Should this part be in a separate patch?
>>>>
>>>> I'm not trying to be rhetorical at all. I literally don't know the
>>>> code very well. Hopefully the full commit message will explain it.
>>>>
>>>
>>> Actually, this fragment of patch revert changes from V2 and keep
>>> untouched dwc2_hsotg_disconnect() function.
>>>
>>
>> To me it feels like there are two issues. The first is this change, and
>> the second is fixing the lockdep warning.
>>
>>
>>>>> }
>>>>>
>>>>> call_gadget(hsotg, disconnect);
>>>>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>>>>> dwc2_hsotg *hsotg, bool periodic)
>>>>> GINTSTS_PTXFEMP | \
>>>>> GINTSTS_RXFLVL)
>>>>>
>>>>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>>>>> +
>>>>> /**
>>>>> * dwc2_hsotg_core_init - issue softreset to the core
>>>>> * @hsotg: The device state
>>>>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>>>>> dwc2_hsotg *hsotg,
>>>>> return;
>>>>> } else {
>>>>> /* all endpoints should be shutdown */
>>>>> + spin_unlock(&hsotg->lock);
>>>>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>>>>> if (hsotg->eps_in[ep])
>>>>>
>>>>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>>> if (hsotg->eps_out[ep])
>>>>>
>>>>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>>> }
>>>>> + spin_lock(&hsotg->lock);
>>>>> }
>>>>>
>>>>> /*
>>>>
>>>> The idea here is that this is the only caller which is holding the
>>>> lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
>>>> I don't know the code very well and can't totally swear that this
>>>> doesn't introduce a small race condition...
>>>>
>>> Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()
>>> function also, without changing spin_lock/_unlock stuff inside function.
>>>
>>> My approach here minimally update code to add any races. Just in
>>> dwc2_hsotg_core_init_disconnected() function on USB reset interrupt
>>> perform disabling all EP's. Because on USB reset interrupt, called from interrupt
>>> handler with acquired lock and dwc2_hsotg_ep_disble() function (without
>>> changes) acquire lock, just need to unlock lock to avoid any troubles.
>>>
>>
>> Yes. I understand that. I just don't like it.
>>
>> Although your patch is more "minimal" in that it touches fewer lines of
>> code it's actually more complicated because we have to verify that it's
>> safe to drop the lock.
>>
>>
>>>> Another option would be to introduce a new function which takes the lock
>>>> and change all the other callers instead. To me that would be easier to
>>>> review... See below for how it might look:
>>>>
>>>> regards,
>>>> dan carpenter
>>>>
>>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>>> index 94f3ba995580..b17a5dbefd5f 100644
>>>> --- a/drivers/usb/dwc2/gadget.c
>>>> +++ b/drivers/usb/dwc2/gadget.c
>>>> @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
>>>> }
>>>>
>>>> static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>>>> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
>>>>
>>>> /**
>>>> * dwc2_hsotg_disconnect - disconnect service
>>>> @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>>>> /* all endpoints should be shutdown */
>>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>>> if (hsotg->eps_in[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>>>> if (hsotg->eps_out[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>>>> }
>>>>
>>>> call_gadget(hsotg, disconnect);
>>>> @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>>> struct dwc2_hsotg *hsotg = hs_ep->parent;
>>>> int dir_in = hs_ep->dir_in;
>>>> int index = hs_ep->index;
>>>> - unsigned long flags;
>>>> u32 epctrl_reg;
>>>> u32 ctrl;
>>>> - int locked;
>>>>
>>>> dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>>>>
>>>> @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>>>
>>>> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>>>>
>>>> - locked = spin_is_locked(&hsotg->lock);
>>>> - if (!locked)
>>>> - spin_lock_irqsave(&hsotg->lock, flags);
>>>> -
>>>> ctrl = dwc2_readl(hsotg, epctrl_reg);
>>>>
>>>> if (ctrl & DXEPCTL_EPENA)
>>>> @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>>> hs_ep->fifo_index = 0;
>>>> hs_ep->fifo_size = 0;
>>>>
>>>> - if (!locked)
>>>> - spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> -
>>>> return 0;
>>>> }
>>>>
>>>> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
>>>> +{
>>>> + struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
>>>> + struct dwc2_hsotg *hsotg = hs_ep->parent;
>>>> + unsigned long flags;
>>>> + int ret;
>>>> +
>>>> + spin_lock_irqsave(&hsotg->lock, flags);
>>>> + ret = dwc2_hsotg_ep_disable(ep);
>>>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> /**
>>>> * on_list - check request is on the given endpoint
>>>> * @ep: The endpoint to check.
>>>> @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value)
>>>>
>>>> static const struct usb_ep_ops dwc2_hsotg_ep_ops = {
>>>> .enable = dwc2_hsotg_ep_enable,
>>>> - .disable = dwc2_hsotg_ep_disable,
>>>> + .disable = dwc2_hsotg_ep_disable_lock,
>>>> .alloc_request = dwc2_hsotg_ep_alloc_request,
>>>> .free_request = dwc2_hsotg_ep_free_request,
>>>> .queue = dwc2_hsotg_ep_queue_lock,
>>>> @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget)
>>>> /* all endpoints should be shutdown */
>>>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>>>> if (hsotg->eps_in[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>>>> if (hsotg->eps_out[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>>>> }
>>>>
>>>> spin_lock_irqsave(&hsotg->lock, flags);
>>>> @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
>>>>
>>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>>> if (hsotg->eps_in[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
>>>> if (hsotg->eps_out[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
>>>> }
>>>> }
>>>>
>>>>
>>>
>>> Your code doesn't take care about fifo_map warnings from
>>> dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo()
>>> from dwc2_hsotg_core_init_disconnected() function all Ep's should
>>> disabled and fifo bitmap should be cleared.
>>>
>>
>> Correct. I am only trying to fix the locking. I hope you can fix the
>> rest in a separate patch.
>>
> Yeah. I'll try deeper investigate driver locking flow and fix it later.
> Actually, I like your idea with introducing dwc2_hsotg_ep_disable_lock()
> function. Maybe you yourself will submit new patch for safe locking
> fixes? But please just after my patch will applied :-)
> Currently there are 2-3 high priority issues reported by community and I
> should find solutions/fixes.
> Thank you very much for your time and useful feedback.
>
> Thanks,
> Minas
>
>
>> regards,
>> dan carpenter
>>
>>
>
>
My patch doesn't pass sparse checking: "warning: context imbalance in
'dwc2_hsotg_core_init_disconnected' - unexpected unlock". Sparse persist!
So, I need to re-work patch. Can I use your idea with
dwc2_hsotg_ep_disable_lock() function to prepare new one?
Thanks,
Minas
Powered by blists - more mailing lists