| 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
| ||
|
Message-ID: <20240809234540.wyanxgos7j4d7cu2@synopsys.com>
Date: Fri, 9 Aug 2024 23:45:48 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Selvarasu Ganesan <selvarasu.g@...sung.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jh0801.jung@...sung.com" <jh0801.jung@...sung.com>,
"dh10.jung@...sung.com" <dh10.jung@...sung.com>,
"naushad@...sung.com" <naushad@...sung.com>,
"akash.m5@...sung.com" <akash.m5@...sung.com>,
"rc93.raju@...sung.com" <rc93.raju@...sung.com>,
"taehyun.cho@...sung.com" <taehyun.cho@...sung.com>,
"hongpooh.kim@...sung.com" <hongpooh.kim@...sung.com>,
"eomji.oh@...sung.com" <eomji.oh@...sung.com>,
"shijie.cai@...sung.com" <shijie.cai@...sung.com>
Subject: Re: [PATCH] usb: dwc3: Potential fix of possible dwc3 interrupt storm
On Fri, Aug 09, 2024, Thinh Nguyen wrote:
> On Thu, Aug 08, 2024, Selvarasu Ganesan wrote:
> >
> > On 8/8/2024 6:45 AM, Thinh Nguyen wrote:
> > > On Wed, Aug 07, 2024, Selvarasu Ganesan wrote:
> > >> On 8/7/2024 6:08 AM, Thinh Nguyen wrote:
> > >>> On Fri, Jul 19, 2024, Selvarasu Ganesan wrote:
> > >>>> In certain scenarios, there is a chance that the CPU may not be
> > >>>> scheduled the bottom half of dwc3 interrupt. This is because the CPU
> > >>>> may hang up where any work queue lockup has happened for the same CPU
> > >>>> that is trying to schedule the dwc3 thread interrupt. In this scenario,
> > >>>> the USB can enter runtime suspend as the bus may idle for a longer time
> > >>>> , or user can reconnect the USB cable. Then, the dwc3 event interrupt
> > >>>> can be enabled when runtime resume is happening with regardless of the
> > >>>> previous event status. This can lead to a dwc3 IRQ storm due to the
> > >>>> return from the interrupt handler by checking only the evt->flags as
> > >>>> DWC3_EVENT_PENDING, where the same flag was set as DWC3_EVENT_PENDING
> > >>>> in previous work queue lockup.
> > >>>> Let's consider the following sequences in this scenario,
> > >>>>
> > >>>> Call trace of dwc3 IRQ after workqueue lockup scenario
> > >>>> ======================================================
> > >>>> IRQ #1:
> > >>>> ->dwc3_interrupt()
> > >>>> ->dwc3_check_event_buf()
> > >>>> ->if (evt->flags & DWC3_EVENT_PENDING)
> > >>>> return IRQ_HANDLED;
> > >>>> ->evt->flags |= DWC3_EVENT_PENDING;
> > >>>> ->/* Disable interrupt by setting DWC3_GEVNTSIZ_INTMASK in
> > >>>> DWC3_GEVNTSIZ
> > >>>> ->return IRQ_WAKE_THREAD; // No workqueue scheduled for dwc3
> > >>>> thread_fu due to workqueue lockup
> > >>>> even after return IRQ_WAKE_THREAD
> > >>>> from top-half.
> > >>>>
> > >>>> Thread #2:
> > >>>> ->dwc3_runtime_resume()
> > >>>> ->dwc3_resume_common()
> > >>>> ->dwc3_gadget_resume()
> > >>>> ->dwc3_gadget_soft_connect()
> > >>>> ->dwc3_event_buffers_setup()
> > >>>> ->/*Enable interrupt by clearing DWC3_GEVNTSIZ_INTMASK in
> > >>>> DWC3_GEVNTSIZ*/
> > >>>>
> > >>>> Start IRQ Storming after enable dwc3 event in resume path
> > >>>> =========================================================
> > >>>> CPU0: IRQ
> > >>>> dwc3_interrupt()
> > >>>> dwc3_check_event_buf()
> > >>>> if (evt->flags & DWC3_EVENT_PENDING)
> > >>>> return IRQ_HANDLED;
> > >>>>
> > >>>> CPU0: IRQ
> > >>>> dwc3_interrupt()
> > >>>> dwc3_check_event_buf()
> > >>>> if (evt->flags & DWC3_EVENT_PENDING)
> > >>>> return IRQ_HANDLED;
> > >>>> ..
> > >>>> ..
> > >>>>
> > >>>> To fix this issue by avoiding enabling of the dwc3 event interrupt in
> > >>>> the runtime resume path if dwc3 event processing is in progress.
> > >>>>
> > >>>> Signed-off-by: Selvarasu Ganesan <selvarasu.g@...sung.com>
> > >>>> ---
> > >>>> drivers/usb/dwc3/core.c | 8 ++++++--
> > >>>> 1 file changed, 6 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > >>>> index cb82557678dd..610792a70805 100644
> > >>>> --- a/drivers/usb/dwc3/core.c
> > >>>> +++ b/drivers/usb/dwc3/core.c
> > >>>> @@ -549,8 +549,12 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc)
> > >>>> lower_32_bits(evt->dma));
> > >>>> dwc3_writel(dwc->regs, DWC3_GEVNTADRHI(0),
> > >>>> upper_32_bits(evt->dma));
> > >>>> - dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> > >>>> - DWC3_GEVNTSIZ_SIZE(evt->length));
> > >>>> +
> > >>>> + /* Skip enable dwc3 event interrupt if event is processing in middle */
> > >>>> + if (!(evt->flags & DWC3_EVENT_PENDING))
> > >>>> + dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> > >>>> + DWC3_GEVNTSIZ_SIZE(evt->length));
> > >>>> +
> > >>>> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 0);
> > >>>>
> > >>>> return 0;
> > >>>> --
> > >>>> 2.17.1
> > >>>>
> > >>> We're not waking up from a hibernation. So after a soft-reset and
> > >>> resume, the events that weren't processed are stale. They should be
> > >>> processed prior to entering suspend or be discarded before resume.
> > >>>
> > >>> The synchronize_irq() during suspend() was not sufficient to prevent
> > >>> this? What are we missing here.
> > >>>
> > >>> Thanks,
> > >>> Thinh
> > >> I don’t think the triggering of interrupt would not be stopped even if
> > >> do soft reset. It's because of event count is may be valid .
> > > Ok. I think I see what you're referring to when you say "event is
> > > processing in the middle" now.
> > >
> > > What you want to check is probably this in dwc3_event_buffers_setup().
> > > Please confirm:
> > >
> > > if (dwc->pending_events)
> > > dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> > > DWC3_GEVNTSIZ_INTMASK | DWC3_GEVNTSIZ_SIZE(evt->length));
> > > else
> > > dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), DWC3_GEVNTSIZ_SIZE(evt->length));
> >
> > Yes, we are expecting the same. But, we must verify the status of
> > evt->flags, which will indicate whether the event is currently
> > processing in middle or not. The below code is for the reference.
> >
> > if (!(evt->flags & DWC3_EVENT_PENDING))
> > dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> > DWC3_GEVNTSIZ_SIZE(evt->length));
> > else
> > dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> > DWC3_GEVNTSIZ_INTMASK | DWC3_GEVNTSIZ_SIZE(evt->length));
>
> So, this happens while pending_events is set right? I need to review
> this runtime suspend flow next week. Something doesn't look right. When
> there's a suspend/resume runtime or not, there's a soft disconnect. We
> shouldn't be processing any event prior to going into suspend. Also, we
Clarification: I mean we shouldn't process any event that happened prior
to suspend on resume because there was a disconnect.
> shouldn't be doing soft-disconnect while connected and in operation
> unless we specifically tell it to.
Thinh
Powered by blists - more mailing lists