linux-kernel - Re: [PATCH 3/8] dmaengine: hisilicon: Add multi-thread support for a DMA channel

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]

Message-ID: <91566735-d576-9c3d-88f1-9e0f3bc94395@huawei.com>
Date:   Mon, 27 Jun 2022 21:14:02 +0800
From:   haijie <haijie1@...wei.com>
To:     Vinod Koul <vkoul@...nel.org>
CC:     "Wangzhou (B)" <wangzhou1@...ilicon.com>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/8] dmaengine: hisilicon: Add multi-thread support for a
 DMA channel


在 2022/6/27 14:51, - 写道:
> -----Original Message-----
> From: Vinod Koul [mailto:vkoul@...nel.org]
> Sent: Monday, June 27, 2022 2:21 PM
> To: haijie <haijie1@...wei.com>
> Cc: Wangzhou (B) <wangzhou1@...ilicon.com>; dmaengine@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH 3/8] dmaengine: hisilicon: Add multi-thread support for a DMA channel
>
> On 25-06-22, 15:44, Jie Hai wrote:
>> When we get a DMA channel and try to use it in multiple threads it
>> will cause oops and hanging the system.
>>
>> % echo 100 > /sys/module/dmatest/parameters/threads_per_chan
>> % echo 100 > /sys/module/dmatest/parameters/iterations
>> % echo 1 > /sys/module/dmatest/parameters/run
>> [383493.327077] Unable to handle kernel paging request at virtual
>> 		address dead000000000108
>> [383493.335103] Mem abort info:
>> [383493.335103]   ESR = 0x96000044
>> [383493.335105]   EC = 0x25: DABT (current EL), IL = 32 bits
>> [383493.335107]   SET = 0, FnV = 0
>> [383493.335108]   EA = 0, S1PTW = 0
>> [383493.335109]   FSC = 0x04: level 0 translation fault
>> [383493.335110] Data abort info:
>> [383493.335111]   ISV = 0, ISS = 0x00000044
>> [383493.364739]   CM = 0, WnR = 1
>> [383493.367793] [dead000000000108] address between user and kernel
>> 		address ranges
>> [383493.375021] Internal error: Oops: 96000044 [#1] PREEMPT SMP
>> [383493.437574] CPU: 63 PID: 27895 Comm: dma0chan0-copy2 Kdump:
>> 		loaded Tainted: GO 5.17.0-rc4+ #2
>> [383493.457851] pstate: 204000c9 (nzCv daIF +PAN -UAO -TCO -DIT
>> 		-SSBS BTYPE=--)
>> [383493.465331] pc : vchan_tx_submit+0x64/0xa0 [383493.469957] lr :
>> vchan_tx_submit+0x34/0xa0
>>
>> This happens because of data race. Each thread rewrite channels's
>> descriptor as soon as device_issue_pending is called. It leads to the
>> situation that the driver thinks that it uses the right descriptor in
>> interrupt handler while channels's descriptor has been changed by
>> other thread.
>>
>> With current fixes channels's descriptor changes it's value only when
>> it has been used. A new descriptor is acquired from
>> vc->desc_issued queue that is already filled with descriptors
>> that are ready to be sent. Threads have no direct access to DMA
>> channel descriptor. Now it is just possible to queue a descriptor for
>> further processing.
>>
>> Fixes: e9f08b65250d ("dmaengine: hisilicon: Add Kunpeng DMA engine
>> support")
>> Signed-off-by: Jie Hai <haijie1@...wei.com>
>> ---
>>   drivers/dma/hisi_dma.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/hisi_dma.c b/drivers/dma/hisi_dma.c index
>> 0a0f8a4d168a..0385419be8d5 100644
>> --- a/drivers/dma/hisi_dma.c
>> +++ b/drivers/dma/hisi_dma.c
>> @@ -271,7 +271,6 @@ static void hisi_dma_start_transfer(struct
>> hisi_dma_chan *chan)
>>   
>>   	vd = vchan_next_desc(&chan->vc);
>>   	if (!vd) {
>> -		dev_err(&hdma_dev->pdev->dev, "no issued task!\n");
> how is this a fix?

> With current fixes, hisi_dma_start_transfer may be called twice for one desc.

> If channel's desc is NULL, When hisi_dma_issue_pending
>>   		chan->desc = NULL;
>>   		return;
>>   	}
>> @@ -303,7 +302,7 @@ static void hisi_dma_issue_pending(struct dma_chan
>> *c)
>>   
>>   	spin_lock_irqsave(&chan->vc.lock, flags);
>>   
>> -	if (vchan_issue_pending(&chan->vc))
>> +	if (vchan_issue_pending(&chan->vc) && !chan->desc)
> This looks good
>
>>   		hisi_dma_start_transfer(chan);
>>   
>>   	spin_unlock_irqrestore(&chan->vc.lock, flags); @@ -442,11 +441,10 @@
>> static irqreturn_t hisi_dma_irq(int irq, void *data)
>>   				    chan->qp_num, chan->cq_head);
>>   		if (FIELD_GET(STATUS_MASK, cqe->w0) == STATUS_SUCC) {
>>   			vchan_cookie_complete(&desc->vd);
>> +			hisi_dma_start_transfer(chan);
> Why should this fix the error reported?
>
>>   		} else {
>>   			dev_err(&hdma_dev->pdev->dev, "task error!\n");
>>   		}
>> -
>> -		chan->desc = NULL;
>>   	}
>>   
>>   	spin_unlock(&chan->vc.lock);
>> --
>> 2.33.0
> --
> ~Vinod
> .