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: <0e4caa6c-d5bd-61e7-2ef6-300973cd2db6@deltatee.com>
Date:   Wed, 26 Jul 2023 15:10:43 -0600
From:   Logan Gunthorpe <logang@...tatee.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Chengfeng Ye <dg573847474@...il.com>, vkoul@...nel.org,
        Yunbo Yu <yuyunbo519@...il.com>
Cc:     dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dmaengine: plx_dma: Fix potential deadlock on
 &plxdev->ring_lock



On 2023-07-26 15:00, Christophe JAILLET wrote:
> Le 26/07/2023 à 17:57, Logan Gunthorpe a écrit :
>>
>>
>> On 2023-07-26 04:48, Chengfeng Ye wrote:
>>> As plx_dma_process_desc() is invoked by both tasklet plx_dma_desc_task()
>>> under softirq context and plx_dma_tx_status() callback that executed
>>> under
>>> process context, the lock aquicision of &plxdev->ring_lock inside
>>> plx_dma_process_desc() should disable irq otherwise deadlock could
>>> happen
>>> if the irq preempts the execution of process context code while the lock
>>> is held in process context on the same CPU.
>>>
>>> Possible deadlock scenario:
>>> plx_dma_tx_status()
>>>      -> plx_dma_process_desc()
>>>      -> spin_lock(&plxdev->ring_lock)
>>>          <tasklet softirq>
>>>          -> plx_dma_desc_task()
>>>          -> plx_dma_process_desc()
>>>          -> spin_lock(&plxdev->ring_lock) (deadlock here)
>>>
>>> This flaw was found by an experimental static analysis tool I am
>>> developing
>>> for irq-related deadlock.
>>>
>>> The tentative patch fixes the potential deadlock by
>>> spin_lock_irqsave() in
>>> plx_dma_process_desc() to disable irq while lock is held.
>>>
>>> Signed-off-by: Chengfeng Ye <dg573847474@...il.com>
>>
>> Makes sense. Thanks!
>>
>> Reviewed-by: Logan Gunthorpe <logang@...tatee.com>
>>
>> Logan
>>
> 
> Hi,
> 
> as explained in another reply [1], would spin_lock_bh() be enough in
> such a case?

The driver originally used spin_lock_bh(). It was removed by Yunbo Yu in
2022 who said that it was unnecessary to be used with a tasklet:

1d05a0bdb420 ("dmaengine: plx_dma: Move spin_lock_bh() to spin_lock() ")

If spin_lock_bh() is correct (which is what I originally thought when I
wrote the driver, though I'm a bit confused now) then I guess that
Yunbo's change was just incorrect. It sounded sensible at the time, but
it looks like there are two call sites of plx_dma_desc_task(): one in
the tasklet and one not in the tasklet. The one not in the tasklet needs
to use the bh version.

So perhaps we should just revert 1d05a0bdb420?

Logan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ