[B.A.T.M.A.N.] [PATCH] batman-adv: Use forw_bcast_list_lock always in disabled interrupt context
forw_bcast_list_lock is spin_locked in both process and softirq context. SoftIRQ calls the spinlock with disabled IRQ and normal process context with enabled IRQs.
When process context is inside an spin_locked area protected by forw_bcast_list_lock and gets interrupted by an IRQ, it could happen that something tries to lock forw_bcast_list_lock again in SoftIRQ context. It cannot proceed further since the lock is already taken somewhere else, but no reschedule will happen inside the SoftIRQ context. This leads to an complete kernel hang without any chance of resurrection.
All functions called in process context must disable IRQs when they try to get get that lock to to prevent any reschedule due to IRQs.
Signed-off-by: Sven Eckelmann sven.eckelmann@gmx.de --- batman-adv-kernelland/send.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/batman-adv-kernelland/send.c b/batman-adv-kernelland/send.c index fc4953f..49b1534 100644 --- a/batman-adv-kernelland/send.c +++ b/batman-adv-kernelland/send.c @@ -338,12 +338,13 @@ static void forw_packet_free(struct forw_packet *forw_packet) static void _add_bcast_packet_to_list(struct forw_packet *forw_packet, unsigned long send_time) { + unsigned long flags; INIT_HLIST_NODE(&forw_packet->list);
/* add new packet to packet list */ - spin_lock(&forw_bcast_list_lock); + spin_lock_irqsave(&forw_bcast_list_lock, flags); hlist_add_head(&forw_packet->list, &forw_bcast_list); - spin_unlock(&forw_bcast_list_lock); + spin_unlock_irqrestore(&forw_bcast_list_lock, flags);
/* start timer for this packet */ INIT_DELAYED_WORK(&forw_packet->delayed_work, @@ -382,10 +383,11 @@ void send_outstanding_bcast_packet(struct work_struct *work) container_of(work, struct delayed_work, work); struct forw_packet *forw_packet = container_of(delayed_work, struct forw_packet, delayed_work); + unsigned long flags;
- spin_lock(&forw_bcast_list_lock); + spin_lock_irqsave(&forw_bcast_list_lock, flags); hlist_del(&forw_packet->list); - spin_unlock(&forw_bcast_list_lock); + spin_unlock_irqrestore(&forw_bcast_list_lock, flags);
/* rebroadcast packet */ rcu_read_lock(); @@ -436,24 +438,25 @@ void purge_outstanding_packets(void) { struct forw_packet *forw_packet; struct hlist_node *tmp_node, *safe_tmp_node; + unsigned long flags;
bat_dbg(DBG_BATMAN, "purge_outstanding_packets()\n");
/* free bcast list */ - spin_lock(&forw_bcast_list_lock); + spin_lock_irqsave(&forw_bcast_list_lock, flags); hlist_for_each_entry_safe(forw_packet, tmp_node, safe_tmp_node, &forw_bcast_list, list) {
- spin_unlock(&forw_bcast_list_lock); + spin_unlock_irqrestore(&forw_bcast_list_lock, flags);
/** * send_outstanding_bcast_packet() will lock the list to * delete the item from the list */ cancel_delayed_work_sync(&forw_packet->delayed_work); - spin_lock(&forw_bcast_list_lock); + spin_lock_irqsave(&forw_bcast_list_lock, flags); } - spin_unlock(&forw_bcast_list_lock); + spin_unlock_irqrestore(&forw_bcast_list_lock, flags);
/* free batman packet list */ spin_lock(&forw_bat_list_lock);
Hi Sven
This reminds me of something i keep intending to do, but never get around to.
It would be nice to have a LOCKING.TXT document, with the following Table of Contents.
1) What locks we have and what they protect.
2) What different contexts different parts of the code run in.
These two sections provide the basis for the following sections.
3) Which locking primitives, ie spin_lock() or spin_lock_irqsave() should be used for each lock.
4) A list of what order locks should be taken in, when taking multiple locks, so as to avoid deadlocks.
When finding this bug, did you take any notes etc, which could contribute to such a document?
Thanks Andrew
On Thu, Dec 17, 2009 at 01:25:44PM +0100, Andrew Lunn wrote:
This reminds me of something i keep intending to do, but never get around to.
It would be nice to have a LOCKING.TXT document, with the following Table of Contents.
What locks we have and what they protect.
What different contexts different parts of the code run in.
These two sections provide the basis for the following sections.
- Which locking primitives, ie spin_lock() or spin_lock_irqsave()
should be used for each lock.
- A list of what order locks should be taken in, when taking multiple
locks, so as to avoid deadlocks.
Yes, would be nice to have something like that, but Simon will change some of the stuff with the next (maybe not deadlocking) patch to remove the big b.a.t.m.a.n. lock. And I heard that Simon also wanted to change the way the packets get send... which probably changes context of each lock.
When finding this bug, did you take any notes etc, which could contribute to such a document?
Sry, I have no notes. I was just sitting next to Marek while discussing what we can do to provide a better working situation for you. This patch was just used to test the changes to git-svn and the pre-commit hooks of svn. The patch was only produced using the help of much to less sleep and some developer tea.
The documentation about the context in which each lock/function is used isn't really there for the kernel. As there are some upcoming changes we should wait until they are submitted. They should be generated while checking for new deadlocks introduced by them.
Best regards, Sven
On Thu, Dec 17, 2009 at 03:34:47PM +0100, Sven Eckelmann wrote: [...]
When finding this bug, did you take any notes etc, which could contribute to such a document?
[...]
The documentation about the context in which each lock/function is used isn't really there for the kernel. As there are some upcoming changes we should wait until they are submitted. They should be generated while checking for new deadlocks introduced by them.
Sorry, wanted to say that there is no real documentation inside the kernel in which situation/context a specific hook/function is called or can be called. Some parts like the vm has such a document, but this is more an exception and not common in many other subsystems.
Best regards, Sven
On Thursday 17 December 2009 20:11:55 Sven Eckelmann wrote:
forw_bcast_list_lock is spin_locked in both process and softirq context. SoftIRQ calls the spinlock with disabled IRQ and normal process context with enabled IRQs.
When process context is inside an spin_locked area protected by forw_bcast_list_lock and gets interrupted by an IRQ, it could happen that something tries to lock forw_bcast_list_lock again in SoftIRQ context. It cannot proceed further since the lock is already taken somewhere else, but no reschedule will happen inside the SoftIRQ context. This leads to an complete kernel hang without any chance of resurrection.
All functions called in process context must disable IRQs when they try to get get that lock to to prevent any reschedule due to IRQs.
Thanks - nice catch (applied in rev 1504)!
Regards, Marek
b.a.t.m.a.n@lists.open-mesh.org
-
Andrew Lunn -
Marek Lindner -
Sven Eckelmann