linux-kernel - Re: [RFC] dmaengine: pl330: fix a race condition in case of threaded irqs

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 9 Jan 2018 18:12:09 +0800
From:   qhou <qi.hou@...driver.com>
To:     Jassi Brar <jassisinghbrar@...il.com>
CC:     Vinod Koul <vinod.koul@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        <paul.gortmaker@...driver.com>, <xiao.zhang@...driver.com>,
        Russell King <rmk+kernel@....linux.org.uk>,
        <dmaengine@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] dmaengine: pl330: fix a race condition in case of threaded
 irqs

Hi, Jassi Brar

On 2018年01月09日 16:29, Jassi Brar wrote:
> On Mon, Dec 25, 2017 at 7:50 AM, Qi Hou <qi.hou@...driver.com> wrote:
> > I found this problem below, and I now understand why it happens, but I'm not
> >  100% sure what is the best way to fix it.
> >
> > When booting up with "threadirqs" in command line, all irq handlers of the DMA
> > controller pl330 will be threaded forcedly. These threads will race for the same
> > list, pl330->req_done.
> >
> > Before the callback, the spinlock was released. And after it, the spinlock was
> > taken. This opened an race window where another threaded irq handler could steal
> > the spinlock and be permitted to delete entries of the list, pl330->req_done.
> >
> The locking has been recently modified beyond recognition, so I can't
> tell why that part of code is the way it is.
> The safest and cleanest solution seems to be to not drop and re-aquire the lock.

The terrible thing is that the lock can not be held all the time during 
doing the callbacks.

If that, there will be a hidden deadlock. As to irq handler of pl330, 
get pl330->lock first,
then pch->lock. As to tasklet of PL330, pch->lockfirst, then 
pl330->lock. It have been
verified when I enabled the kernel lock debug option.

And after tracing the changelogs of the driver of the DMA controller 
PL330, I have to stay back.
The pair of unlock/lock staffs is there since the original commit.  It 
seems that the author
intent to do things like that.

If it's hard to trace the author's intention, maybe the advised fix 
below could be taken, since
there is no side effect.  And it achieves the same goal as the original 
wants to.


diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index d7327fd..de1fd59 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1510,7 +1510,7 @@ static void pl330_dotask(unsigned long data)
  /* Returns 1 if state was updated, 0 otherwise */
  static int pl330_update(struct pl330_dmac *pl330)
  {
-       struct dma_pl330_desc *descdone, *tmp;
+       struct dma_pl330_desc *descdone;
         unsigned long flags;
         void __iomem *regs;
         u32 val;
@@ -1588,7 +1588,9 @@ static int pl330_update(struct pl330_dmac *pl330)
         }

         /* Now that we are in no hurry, do the callbacks */
-       list_for_each_entry_safe(descdone, tmp, &pl330->req_done, rqd) {
+       while (!list_empty(&pl330->req_done)) {
+               descdone = list_first_entry(&pl330->req_done,
+                                           struct dma_pl330_desc, rqd);
                 list_del(&descdone->rqd);
                 spin_unlock_irqrestore(&pl330->lock, flags);
                 dma_pl330_rqcb(descdone, PL330_ERR_NONE);

--
best regards,
Qi Hou

>
> Cheers!

-- 
Best regards,
Qi Hou
Phone number: +86-10-8477-8608
Address: Floor 15, Building B, Wangjing Plaza, No.9 Zhong-Huan Nanlu, Chaoyang District