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 <[email protected]>
---
drivers/dma/plx_dma.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
index 34b6416c3287..46068b8d83f9 100644
--- a/drivers/dma/plx_dma.c
+++ b/drivers/dma/plx_dma.c
@@ -135,9 +135,10 @@ static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
{
struct dmaengine_result res;
struct plx_dma_desc *desc;
+ unsigned long lock_flags;
u32 flags;
- spin_lock(&plxdev->ring_lock);
+ spin_lock_irqsave(&plxdev->ring_lock, lock_flags);
while (plxdev->tail != plxdev->head) {
desc = plx_dma_get_desc(plxdev, plxdev->tail);
@@ -165,7 +166,7 @@ static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
plxdev->tail++;
}
- spin_unlock(&plxdev->ring_lock);
+ spin_unlock_irqrestore(&plxdev->ring_lock, lock_flags);
}
static void plx_dma_abort_desc(struct plx_dma_dev *plxdev)
--
2.17.1
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 <[email protected]>
Makes sense. Thanks!
Reviewed-by: Logan Gunthorpe <[email protected]>
Logan
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 <[email protected]>
>
> Makes sense. Thanks!
>
> Reviewed-by: Logan Gunthorpe <[email protected]>
>
> Logan
>
Hi,
as explained in another reply [1], would spin_lock_bh() be enough in
such a case?
CJ
[1]:
https://lore.kernel.org/all/[email protected]/
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 <[email protected]>
>>
>> Makes sense. Thanks!
>>
>> Reviewed-by: Logan Gunthorpe <[email protected]>
>>
>> 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
Hi Logan and Christophe,
Thanks much for the reply and reminder, and yes, spin_lock_bh() should
be better.
When I wrote the patch I thought the spin_lock_bh() cannot be nested,
and afraid that if some outside callers called .dma_tx_status() callback
with softirq already disable, then spin_unlock_bh() would unintentionally
re-enable softirq(). spin_lock_irqsave() is always safer in general thus I
used it.
But I just check the document [1] about these API and found that _bh()
can be nested. Then use spin_lock_bh() should be better due to
performance concern.
> So perhaps we should just revert 1d05a0bdb420?
Then for this one I think revert 1d05a0bdb420 should be enough. May I
ask to revert that patch, should I do anything further? (like sending
a new patch).
> as explained in another reply [1], would spin_lock_bh() be enough in
> such a case?
For the another one [2], I would send a v2 patch to change to spin_lock_bh()
[1] http://books.gigatux.nl/mirror/kerneldevelopment/0672327201/ch07lev1sec6.html
[2] https://lore.kernel.org/all/[email protected]/
Thanks again,
Chengfeng
Hello,
Am 27.07.2023 um 08:48 schrieb Chengfeng Ye:
> Hi Logan and Christophe,
>
> Thanks much for the reply and reminder, and yes, spin_lock_bh() should
> be better.
>
> When I wrote the patch I thought the spin_lock_bh() cannot be nested,
> and afraid that if some outside callers called .dma_tx_status() callback
> with softirq already disable, then spin_unlock_bh() would unintentionally
> re-enable softirq(). spin_lock_irqsave() is always safer in general thus I
> used it.
>
> But I just check the document [1] about these API and found that _bh()
> can be nested. Then use spin_lock_bh() should be better due to
> performance concern.
>
>
>> So perhaps we should just revert 1d05a0bdb420?
> Then for this one I think revert 1d05a0bdb420 should be enough. May I
> ask to revert that patch, should I do anything further? (like sending
> a new patch).
>
>> as explained in another reply [1], would spin_lock_bh() be enough in
>> such a case?
> For the another one [2], I would send a v2 patch to change to spin_lock_bh()
>
> [1] http://books.gigatux.nl/mirror/kerneldevelopment/0672327201/ch07lev1sec6.html
> [2] https://lore.kernel.org/all/[email protected]/
For uniformity reason across drivers and also that not something else
gets missed please compare your requirements and solution to the
implementation of the "altera-msgdma" driver (altera-msgdma.c).
W/ special emphasis on commit edf10919e5fc ("dmaengine: altera: fix
spinlock usage")
spin_lock_bh was changed to spin_lock_irqsave w/ this patch.
Cheers
Eric
Hi Eric,
> W/ special emphasis on commit edf10919e5fc ("dmaengine: altera: fix
> spinlock usage")
> spin_lock_bh was changed to spin_lock_irqsave w/ this patch.
Yeah, that patch is to fix the deadlock between irq and kernel thread
thus use spin_lock_irqsave(), exactly the kind of bug that I am reporting
about. But slight difference is the deadlock on that commit concerns
hardirq and thus fixed by spin_lock_irqsave(), but this one concerns softirq.
> For uniformity reason across drivers and also that not something else
> gets missed please compare your requirements and solution to the
> implementation of the "altera-msgdma" driver (altera-msgdma.c).
Maybe for our case spin_lock_bh() seems to be enough since we are handling
deadlock between tasklet and kernel thread. It's truth that uniformity
is an important concern, also as I glance at other DMA driver, in general
spin_lock_irqsave() is more frequently used than spin_lock_bh(). But this driver
already consistently uses spin_lock_bh() on &plxdev->ring_lock, only change to
spin_lock_irqsave() at this function could instead make the code seems
a bit weird.
Even though my original patch did use spin_lock_irqsave(), now I think at least
for this bug fix just revert back to spin_lock_bh() may be better and
will not complicate
things much.
Thanks,
Chengfeng
On 7/27/23 00:48, Chengfeng Ye wrote:
> Hi Logan and Christophe,
>
> Thanks much for the reply and reminder, and yes, spin_lock_bh() should
> be better.
>
> When I wrote the patch I thought the spin_lock_bh() cannot be nested,
> and afraid that if some outside callers called .dma_tx_status() callback
> with softirq already disable, then spin_unlock_bh() would unintentionally
> re-enable softirq(). spin_lock_irqsave() is always safer in general thus I
> used it.
>
> But I just check the document [1] about these API and found that _bh()
> can be nested. Then use spin_lock_bh() should be better due to
> performance concern.
>
>
>> So perhaps we should just revert 1d05a0bdb420?
> Then for this one I think revert 1d05a0bdb420 should be enough. May I
> ask to revert that patch, should I do anything further? (like sending
> a new patch).
Yes, I think you can just send a revert patch explaining the reasoning
further in a commit message.
Thanks,
Logan
> Yes, I think you can just send a revert patch explaining the reasoning
> further in a commit message.
v2 patch is just sent to address the problem.
Best Regards,
Chengfeng