dma-debug: Fix deadlock with netconsole or other drivers that use the DMA API
PATCH 1/1 dma-debug: Fix deadlock with netconsole or other drivers that use the DMA API
From: Roland Dreier <[email protected]>
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.
--- lib/dma-debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/dma-debug.c +1 -1
@@ -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; }
Signature · 1 line
1.7.9.5
Andrew Morton
maintainerQuoted 25 lines · On Thu, 19 Apr 2012 11:12:53 -0700 Roland Dreier <[email protected]> wrote:
From: Roland Dreier <[email protected]>
If we exhaust the
free_entrieslist, then we print the error messageDMA-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 onfree_entries_lock.A fix is pretty simple: if we flip the order of setting
global_disableto be before we print the error message, then the nested call into the DMA API will bail out before trying to getfree_entries_lock.Signed-off-by Roland Dreier <[email protected]>--- lib/dma-debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)lib/dma-debug.c +0
-0
lib/dma-debug.c static struct dma_debug_entry *dma_entry_alloc(void) · line 442
Quoted 8 lines
lib/dma-debug.c +1
-1@@ -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?
Quoted 2 lines · Andrew Morton
So *any* printk can deadlock if
free_entries_lockis held and global_disable==false?
apparently.
Quoted 5 lines
In that case we're going to need much sterner fixes. Any
list_headoperation can do a printk iflist_headdebugging is enabled.dma_debug_resize_entries()does akfree()underfree_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.
Andrew Morton
maintainerQuoted 11 lines · On Thu, 19 Apr 2012 16:36:56 -0700 Roland Dreier <[email protected]> wrote:
Quoted 2 lines · Andrew Morton
So *any* printk can deadlock if
free_entries_lockis held and global_disable==false?apparently.
Quoted 4 lines
In that case we're going to need much sterner fixes.
__Anylist_headoperation can do a printk iflist_headdebugging is enabled.dma_debug_resize_entries()does akfree()underfree_entries_lock(!).
Quoted 7 lines
Quoted 1 line
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?
Quoted 23 lines · Andrew Morton
Quoted 9 lines · On Thu, 19 Apr 2012 11:12:53 -0700 Roland Dreier <[email protected]> wrote:
--- lib/dma-debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)lib/dma-debug.c +0
-0lib/dma-debug.c static struct dma_debug_entry *dma_entry_alloc(void) · line 442
Quoted 8 lines
lib/dma-debug.c +1
-1@@ -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_lockis held and global_disable==false?
Quoted 5 lines
In that case we're going to need much sterner fixes. Any
list_headoperation can do a printk iflist_headdebugging is enabled.dma_debug_resize_entries()does akfree()underfree_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
Signature · 5 lines
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