batman-adv: Use forw_bcast_list_lock always in disabled interrupt context
Commit Message
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(-)
Comments
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 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
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.
>
> 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.
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
@@ -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);