linux-kernel - Re: [PATCH] Fixes kernel crash generating from bam_dma

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] [day] [month] [year] [list]

Message-ID: <20201207052322.GD8403@vkoul-mobl>
Date:   Mon, 7 Dec 2020 10:53:22 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Parth Y Shah <sparth1292@...il.com>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     agross@...nel.org, bjorn.andersson@...aro.org,
        dan.j.williams@...el.com, linux-arm-msm@...r.kernel.org,
        dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Fixes kernel crash generating from bam_dma_irq()

Hi Parth,

On 06-12-20, 15:51, Parth Y Shah wrote:
> While performing suspend/resume, we were getting below kernel crash.
> 
> [   54.541672] [FTS][Info]gesture suspend...
> [   54.605256] [FTS][Error][GESTURE]Enter into gesture(suspend) failed!
> [   54.605256]
> [   58.345850] irq event 10: bogus return value fffffff3
> ......
> 
> [   58.345966] [<ffff0000080830f0>] el1_irq+0xb0/0x124
> [   58.345971] [<ffff000008085360>] arch_cpu_idle+0x10/0x18
> [   58.345975] [<ffff0000081077f4>] do_idle+0x1ac/0x1e0
> [   58.345979] [<ffff0000081079c8>] cpu_startup_entry+0x20/0x28
> [   58.345983] [<ffff000008a80ed0>] rest_init+0xd0/0xdc
> [   58.345988] [<ffff0000091c0b48>] start_kernel+0x390/0x3a4
> [   58.345990] handlers:
> [   58.345994] [<ffff0000085120d0>] bam_dma_irq
> 
> The reason for the crash we found is, bam_dma_irq() was returning
> negative value when the device resumes in some conditions.
> 
> In addition, the irq handler should have one of the below return values.
> 
> IRQ_NONE            interrupt was not from this device or was not handled
> IRQ_HANDLED         interrupt was handled by this device
> IRQ_WAKE_THREAD     handler requests to wake the handler thread
> 
> Therefore, to resolve this crash, we have changed the return value to
> IRQ_NONE.

The change and explanation look good to me, unfortunately the patch
title is incorrect. It describes the fix it does and not the change in
this patch. Also do add subsystem and driver tags to the patch! git log
would tell you this information

Consider: "dmaengine: bam_dma: fix return of bam_dma_irq()" as a
suggestion.

> 
> Signed-off-by: Parth Y Shah <sparth1292@...il.com>
> ---
>  drivers/dma/qcom/bam_dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 4eeb8bb..d5773d4 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -875,7 +875,7 @@ static irqreturn_t bam_dma_irq(int irq, void *data)
>  
>  	ret = bam_pm_runtime_get_sync(bdev->dev);

Also this looks wrong to me. get_sync() can sleep and we cant invoke
that in an irq. Srini have you seen this issue

>  	if (ret < 0)
> -		return ret;
> +		return IRQ_NONE;
>  
>  	if (srcs & BAM_IRQ) {
>  		clr_mask = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_STTS));
> -- 
> 2.7.4

-- 
~Vinod