* [PATCH] dma-debug: Fix deadlock with netconsole or other drivers that use the DMA API
@ 2012-04-19 18:12 Roland Dreier
2012-04-19 18:48 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Roland Dreier @ 2012-04-19 18:12 UTC (permalink / raw)
To: Joerg Roedel, Andrew Morton; +Cc: linux-kernel
From: Roland Dreier <roland@purestorage.com>
If we exhaust the free_entries list, then we print the error message
DMA-API: debugging out of memory - disabling
to the kernel log, while holding free_entries_lock. Unfortunately, if
the console driver ends up calling back into the DMA API to map a
buffer, as eg a NIC driver is quite likely to for the packet netconsole
asks it to send, this will deadlock on free_entries_lock.
A fix is pretty simple: if we flip the order of setting global_disable
to be before we print the error message, then the nested call into the
DMA API will bail out before trying to get free_entries_lock.
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
lib/dma-debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 13ef233..f198b4e 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -436,8 +436,8 @@ static struct dma_debug_entry *dma_entry_alloc(void)
spin_lock_irqsave(&free_entries_lock, flags);
if (list_empty(&free_entries)) {
- pr_err("DMA-API: debugging out of memory - disabling\n");
global_disable = true;
+ pr_err("DMA-API: debugging out of memory - disabling\n");
goto out;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-debug: Fix deadlock with netconsole or other drivers that use the DMA API
2012-04-19 18:12 [PATCH] dma-debug: Fix deadlock with netconsole or other drivers that use the DMA API Roland Dreier
@ 2012-04-19 18:48 ` Andrew Morton
2012-04-19 23:36 ` Roland Dreier
2012-04-20 11:22 ` Joerg Roedel
0 siblings, 2 replies; 5+ messages in thread
From: Andrew Morton @ 2012-04-19 18:48 UTC (permalink / raw)
To: Roland Dreier; +Cc: Joerg Roedel, linux-kernel
On Thu, 19 Apr 2012 11:12:53 -0700
Roland Dreier <roland@kernel.org> wrote:
> From: Roland Dreier <roland@purestorage.com>
>
> If we exhaust the free_entries list, then we print the error message
>
> DMA-API: debugging out of memory - disabling
>
> to the kernel log, while holding free_entries_lock. Unfortunately, if
> the console driver ends up calling back into the DMA API to map a
> buffer, as eg a NIC driver is quite likely to for the packet netconsole
> asks it to send, this will deadlock on free_entries_lock.
>
> A fix is pretty simple: if we flip the order of setting global_disable
> to be before we print the error message, then the nested call into the
> DMA API will bail out before trying to get free_entries_lock.
>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
> lib/dma-debug.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index 13ef233..f198b4e 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -436,8 +436,8 @@ static struct dma_debug_entry *dma_entry_alloc(void)
> spin_lock_irqsave(&free_entries_lock, flags);
>
> if (list_empty(&free_entries)) {
> - pr_err("DMA-API: debugging out of memory - disabling\n");
> global_disable = true;
> + pr_err("DMA-API: debugging out of memory - disabling\n");
> goto out;
> }
So *any* printk can deadlock if free_entries_lock is held and
global_disable==false?
In that case we're going to need much sterner fixes. Any list_head
operation can do a printk if list_head debugging is enabled.
dma_debug_resize_entries() does a kfree() under free_entries_lock(!).
Methinks we need a more general fix?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-debug: Fix deadlock with netconsole or other drivers that use the DMA API
2012-04-19 18:48 ` Andrew Morton
@ 2012-04-19 23:36 ` Roland Dreier
2012-04-19 23:50 ` Andrew Morton
2012-04-20 11:22 ` Joerg Roedel
1 sibling, 1 reply; 5+ messages in thread
From: Roland Dreier @ 2012-04-19 23:36 UTC (permalink / raw)
To: Andrew Morton; +Cc: Joerg Roedel, linux-kernel
On Thu, Apr 19, 2012 at 11:48 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> So *any* printk can deadlock if free_entries_lock is held and
> global_disable==false?
apparently.
> In that case we're going to need much sterner fixes. Any list_head
> operation can do a printk if list_head debugging is enabled.
> dma_debug_resize_entries() does a kfree() under free_entries_lock(!).
>
> Methinks we need a more general fix?
sigh... no good deed goes unpunished.
OK, will look at it. Just to make things even more fun, all the
err_printk() stuff can potentially deadlock on the hash bucket
lock, although that requires enough bad luck a collision to happen.
- R.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-debug: Fix deadlock with netconsole or other drivers that use the DMA API
2012-04-19 23:36 ` Roland Dreier
@ 2012-04-19 23:50 ` Andrew Morton
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Morton @ 2012-04-19 23:50 UTC (permalink / raw)
To: Roland Dreier; +Cc: Joerg Roedel, linux-kernel
On Thu, 19 Apr 2012 16:36:56 -0700
Roland Dreier <roland@kernel.org> wrote:
> On Thu, Apr 19, 2012 at 11:48 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > So *any* printk can deadlock if free_entries_lock is held and
> > global_disable==false?
>
> apparently.
>
> > In that case we're going to need much sterner fixes. __Any list_head
> > operation can do a printk if list_head debugging is enabled.
> > dma_debug_resize_entries() does a kfree() under free_entries_lock(!).
> >
> > Methinks we need a more general fix?
>
> sigh... no good deed goes unpunished.
>
> OK, will look at it. Just to make things even more fun, all the
> err_printk() stuff can potentially deadlock on the hash bucket
> lock, although that requires enough bad luck a collision to happen.
I suppose one could do something like
static DEFINE_SPINLOCK(lock);
static struct task_struct *owner;
static unsigned depth;
/*
* Nice comments go here
*/
static unsigned long free_entries_lock(void)
{
unsigned long flags = 0;
if (owner == current) {
depth++;
} else {
/* Permit recursive locking */
spin_lock_irqsave(&lock, flags);
BUG_ON(depth != 0);
BUG_ON(owner != NULL);
owner = current;
}
return flags;
}
static void free_entries_unlock(unsigned long flags)
{
BUG_ON(owner != current);
if (!--depth) {
owner = NULL;
spin_lock_irqrestore(&lock, flags);
}
}
After removing the bugs, I think that's safe wrt interrupts?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-debug: Fix deadlock with netconsole or other drivers that use the DMA API
2012-04-19 18:48 ` Andrew Morton
2012-04-19 23:36 ` Roland Dreier
@ 2012-04-20 11:22 ` Joerg Roedel
1 sibling, 0 replies; 5+ messages in thread
From: Joerg Roedel @ 2012-04-20 11:22 UTC (permalink / raw)
To: Andrew Morton; +Cc: Roland Dreier, linux-kernel
On Thu, Apr 19, 2012 at 11:48:11AM -0700, Andrew Morton wrote:
> On Thu, 19 Apr 2012 11:12:53 -0700
> Roland Dreier <roland@kernel.org> wrote:
> > ---
> > lib/dma-debug.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> > index 13ef233..f198b4e 100644
> > --- a/lib/dma-debug.c
> > +++ b/lib/dma-debug.c
> > @@ -436,8 +436,8 @@ static struct dma_debug_entry *dma_entry_alloc(void)
> > spin_lock_irqsave(&free_entries_lock, flags);
> >
> > if (list_empty(&free_entries)) {
> > - pr_err("DMA-API: debugging out of memory - disabling\n");
> > global_disable = true;
> > + pr_err("DMA-API: debugging out of memory - disabling\n");
> > goto out;
> > }
>
> So *any* printk can deadlock if free_entries_lock is held and
> global_disable==false?
>
> In that case we're going to need much sterner fixes. Any list_head
> operation can do a printk if list_head debugging is enabled.
> dma_debug_resize_entries() does a kfree() under free_entries_lock(!).
>
> Methinks we need a more general fix?
Hmm, I think the best way to fix it is to switch the hash-bucket lists
to rcu. Then the lock is only needed in hash_bucket_add/del and we
should be fine.
But that is probably only a fix for the next merge window.
Joerg
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-04-20 11:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-19 18:12 [PATCH] dma-debug: Fix deadlock with netconsole or other drivers that use the DMA API Roland Dreier
2012-04-19 18:48 ` Andrew Morton
2012-04-19 23:36 ` Roland Dreier
2012-04-19 23:50 ` Andrew Morton
2012-04-20 11:22 ` Joerg Roedel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.