Network Stack Deadlock

[jackburton]

Yesterday I was doing some tests with the network stack, and after a while, I experienced a deadlock. ifconfig was locked and couldn't be killed (even with kill -9). net_server was locked, too. I then entered KDL, and had a look at the threads involved. There was a deadlock between ifconfig and wpa_supplicant. The net_interfaces mutex was held by wpa_supplicant, which was waiting on the mutex of the wireless interface, which in turn was held by ifconfig. So I guess we have a lock inversion somewhere. It's reproducible here 100% of the times by doing: ifconfig /dev/net/<wireless_card> down while wpa_supplicant is scanning the available networks. I have an ipro3945 wireless card.

[jackburton]

From the backtrace of the two threads, the lock inversion is clear: the wpa_supplicant thread calls get_interface() which first locks the "net_interfaces" mutex, then calls CreateDomainDatalinkIfNeeded() tries to acquire the recursive lock of the interface.

The ifconfig thread, on the other hand, first acquires the interface lock in Interface::SetDown(), then (through the ipv6 protocol add-on) calls get_interface() which tries to acquire the net_interfaces mutex.

[jackburton]

Would it be possible to unlock the interface list mutex after getting the interface in get_interface(), before calling CreateDomainDatalinkIfNeeded() ?

I mean here:

Interface*
get_interface(net_domain* domain, const char* name)
{
	MutexLocker locker(sLock);

	Interface* interface = find_interface(name);
	if (interface == NULL)
		return NULL;

+       locker.Unlock();

	if (interface->CreateDomainDatalinkIfNeeded(domain) != B_OK)
		return NULL;

	interface->AcquireReference();
	return interface;
}

At least, that function doesn't seem to mess with the interfaces list, so it would seem to be safe to unlock.

[axeld]

That does not work, unfortunately.

First of all, a second get_interface() for the same name might return an invalid interface (ie. one that hasn't been completely initialized), or CreateDomainDatalinkIfNeeded() might be called twice.

Second of all, if you do not acquire a reference before losing the lock, the interface might be deleted by someone else before you do.

In general, I think the locking order "all interfaces" -> "single interface" is more natural than the other way around. Maybe SetDown() can be changed to not hold the interface lock when calling into the interface code. If that isn't possible at all (I don't remember), another way would be to lock the interfaces lock, too (in the right order).

[jackburton]

I went with the second solution. Here's a patch which converts the net_interfaces mutex to a recursive_lock, so that it can be acquired in Interface::SetDown(). This avoids the deadlock. Please someone review the patch.

[axeld]

Not my preferred solution, but it looks good. Maybe add a TODO comment to SetDown() that suggests that a better solution would be to prevent locking when calling the lower layers?

Thanks for working on this!

[jackburton]

Applied in hrev45240.

[jackburton]

Replying to axeld:

Not my preferred solution, but it looks good. Maybe add a TODO comment to SetDown() that suggests that a better solution would be to prevent locking when calling the lower layers?

How would you avoid locking in this case ?

[axeld]

Hm, why didn't you ask me 19 months ago? :-)

By looking over the code, I noticed that Interface::_SetUp() has the same issue. I think the best solution would be to use a similar approach than the VFS when vnodes are created:

• lock all interfaces
• create the interface, and put it in there
• mark the interface as busy
• unlock all interfaces
• call into the interface protocol layer as needed (ie. interface_up()/interface_down())
• lock all interfaces again
• remove the busy flag
• unlock all interfaces

Making the busy flag atomic would save locking all interfaces twice. Of course, get_interface() would then need to check for the busy flag, and not return those.

If that fails (maybe because the up/down path calls get_interface() on itself, and needs it to succeed), the change will get a bit more complicated, than this, though.

[jackburton]

Replying to axeld:

Hm, why didn't you ask me 19 months ago? :-)

Dunno, maybe I thought it would've been too complicated, and I just wanted a quick fix (for myself).

By looking over the code, I noticed that Interface::_SetUp() has the same issue. I think the best solution would be to use a similar approach than the VFS when vnodes are created:

• lock all interfaces
• create the interface, and put it in there
• mark the interface as busy
• unlock all interfaces
• call into the interface protocol layer as needed (ie. interface_up()/interface_down())
• lock all interfaces again
• remove the busy flag
• unlock all interfaces

Making the busy flag atomic would save locking all interfaces twice. Of course, get_interface() would then need to check for the busy flag, and not return those.

If that fails (maybe because the up/down path calls get_interface() on itself, and needs it to succeed), the change will get a bit more complicated, than this, though.

Okay, thanks for explaining.

[jackburton]

Replying to axeld:

Making the busy flag atomic would save locking all interfaces twice.

Of course, get_interface() would then need to check for the busy flag, and not return those.

get_interface(), but also get_interface_for_device() and get_interface_for_link(), I assume.

[axeld]

Replying to jackburton:

Dunno, maybe I thought it would've been too complicated, and I just wanted a quick fix (for myself).

I just meant because I wouldn't have needed to look at the code again then :-) This way I at least noticed that _SetUp() has the same bug, though.

get_interface(), but also get_interface_for_device() and get_interface_for_link(), I assume.

Yup.

[jackburton]

I attached a patch. Does it look good ?

[axeld]

Almost:

• You still need to hold the all interfaces lock when setting the interface busy, otherwise you might hand out busy interfaces; the lock can be released as soon as the interface is made busy.
• Not sure if SetDown() can be reached in that phase, but if it does, it would need to check for being busy as well (and bail out in that case). Also, I don't know if it's currently given that SetDown() can only be called when there are no other interface users. If it isn't the patch is going to be more involved.
• Indenting seems off in interfaces.h ("inline" must be indented one tab if it isn't) -- if the rest doesn't fit, it's probably not correctly indented either, though (I think the current indentation rule is newer than the network stack).

Have you tried if it works? :-)

I'll try to read through that code again in the next couple of days to make sure that SetDown() works the way it should in this context. It could very well be that setting the interface busy might only be needed in the _SetUp() case.

[jackburton]

Just an update: I didn't forget to update the code, but my haiku development machine (my only usable machine, actually) is currently "exploded" on my desk, waiting for a new fan.

[jackburton]

Second try

[jackburton]

Replying to axeld:

Have you tried if it works? :-)

Yes, it works.

[pulkomandy]

Reopening since there is an updated version of the patch waiting here...

[jscipione]

can somebody please look into this?

[korli]

1. I would check the busy flag with sLock hold in SetDown(), or SetBusy() should return the previous value and return if already busy (with atomic_get_and_set())
2. I would also check for the busy flag in get_interface_address_for_destination(), get_interface_address_for_link().

[kallisti5]

I tried the patch locally.. while it doesn't introduce any new bugs, i'm seeing some odd network issues here... so I can't confirm 100% it works. I'd say we hold off on this one until we fix the current network / DHCP issues.

[jackburton]

I can update the patch with Korli advices, and if there aren't objections, I'd commit it.

[jackburton]

Tested working and applied in hrev49531. If you think something's wrong, feel free to comment or fix it directly.