linux-kernel - Re: [PATCH] usb: dwc3: Potential fix of possible dwc3 interrupt storm

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

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

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

Message-ID: <20240807003806.5owtgwgw2lczg4u5@synopsys.com>
Date: Wed, 7 Aug 2024 00:38:11 +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, 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