mac80211: workqueue & RTNL lock

That function calls flush_workqueue()

Quoted 1 line · On Monday 23 June 2008 00:42:41 Ivo van Doorn wrote:

That function calls flush_workqueue()

Uh wait. It shouldn't call that. Where exactly is this function called?

Quoted 4 lines · Michael Buesch

Quoted 1 line · On Monday 23 June 2008 00:42:41 Ivo van Doorn wrote:

That function calls flush_workqueue()

Uh wait. It shouldn't call that. Where exactly is this function called?

It is called right before local->ops->remove_interface() which means the workqueue is flushed when it isn't even guaranteed that all interfaces are gone.

net/mac80211/main.c:553

static int ieee80211_stop(struct net_device *dev)
{
<snip>

  switch (sdata->vif.type) {
	case IEEE80211_IF_TYPE_MESH_POINT:
	case IEEE80211_IF_TYPE_STA:
	case IEEE80211_IF_TYPE_IBSS:
        <snip>
        flush_workqueue(local->hw.workqueue);
       <snip>

}
<snip>
}

Quoted 22 lines · On Monday 23 June 2008 10:30:47 Ivo Van Doorn wrote:

Quoted 4 lines · Michael Buesch

Quoted 1 line · On Monday 23 June 2008 00:42:41 Ivo van Doorn wrote:

That function calls flush_workqueue()

Uh wait. It shouldn't call that. Where exactly is this function called?

It is called right before local->ops->remove_interface() which means the workqueue is flushed when it isn't even guaranteed that all interfaces are gone.

net/mac80211/main.c:553

static int ieee80211_stop(struct net_device *dev)
{
<snip>

  switch (sdata->vif.type) {
	case IEEE80211_IF_TYPE_MESH_POINT:
	case IEEE80211_IF_TYPE_STA:
	case IEEE80211_IF_TYPE_IBSS:
        <snip>
        flush_workqueue(local->hw.workqueue);
       <snip>

Quoted 3 lines

}
<snip>
}

Oh right. I see. This is actually OK. I thought it would flush the global wq.
The local mac80211 wq was introduced to workaround this rtnl deadlock, so I'm surprised it happens again.

Well it was caused by the recent introduction of the interface iterator. Question now is if we need to restrict the usage, remove the flush_workqueue, or change the locking in the interface iterator. :S

Quoted 4 lines · Ivo Van Doorn

Well it was caused by the recent introduction of the interface iterator. Question now is if we need to restrict the usage, remove the flush_workqueue,
or change the locking in the interface iterator. :S

Ouch. Any way you can use the atomic iterator there?

Or if not, can you use schedule_work and cancel_work_sync to take care of things?

I would like to keep the mac80211 workqueue rtnl-free because otherwise locking is going to get ugly.

Quoted 1 line

Ouch. Any way you can use the atomic iterator there?

No because of rt2500usb and rt73usb who need scheduling,
and this bug is in exactly the same function as before where we had the scheduling while atomic problem when you introduced the atomic iterator. ;)

Quoted 2 lines

Or if not, can you use schedule_work and cancel_work_sync to take care of things?

I thought about that it cancel_work_sync will cause exactly the same problem, stop() holds the rtnl lock, rt2x00 work waits for rtnl lock, mac80211 calls stop(), rt2x00 waits until tasks are completed but one of those is still waiting on the rtnl lock.

However I could use the non-synchronized version for cancelling the work and add a check to see if the work should be cancelled within the rtnl-protected area.

Quoted 2 lines

I would like to keep the mac80211 workqueue rtnl-free because otherwise locking is going to get ugly.

We need a big warning in mac80211.h then that tells that the non-atomic iterator or any other rtnl locking cannot be used in the mac80211 workqueue.

Quoted 9 lines

Quoted 2 lines

Or if not, can you use schedule_work and cancel_work_sync to take care of things?

I thought about that it cancel_work_sync will cause exactly the same problem,
stop() holds the rtnl lock, rt2x00 work waits for rtnl lock, mac80211 calls stop(), rt2x00 waits until tasks are completed but one of those is still waiting on the rtnl lock.

Quoted 3 lines

However I could use the non-synchronized version for cancelling the work and add a check to see if the work should be cancelled within the rtnl-protected area.

Oh, right, your work is actually taking the RTNL. We will not get the ->stop callback rtnl-free no matter what we do. And we can probably not even get it free of whatever lock we need for the netdev list either way...

However, the interface iterator will simply not run at all if ->stop has already succeeded since no interfaces would be UP, so you don't really have to protect it against that, you only have to make sure it's not running when you unregister your hardware struct, and _that_ doesn't have the rtnl taken so you can use cancel_work_sync() there.

Quoted 6 lines

Quoted 2 lines

I would like to keep the mac80211 workqueue rtnl-free because otherwise locking is going to get ugly.

We need a big warning in mac80211.h then that tells that the non-atomic iterator
or any other rtnl locking cannot be used in the mac80211 workqueue.

Agreed, care to prepare a patch?

Quoted 6 lines

However, the interface iterator will simply not run at all if ->stop has already succeeded since no interfaces would be UP, so you don't really have to protect it against that, you only have to make sure it's not running when you unregister your hardware struct, and _that_ doesn't have the rtnl taken so you can use cancel_work_sync() there.

True, but the race condition happens when ieee80211_stop() is called, and the rt2x00 work is triggered before before rt2x00->remove_interface() or rt2x00>stop() is called. when stop() is then called it cannot use cancel_work_sync() because
the work structure is waiting until stop() has completed which will not end until cancel_work_sync() has completed.

Quoted 6 lines

Quoted 5 lines

However, the interface iterator will simply not run at all if ->stop has already succeeded since no interfaces would be UP, so you don't really have to protect it against that, you only have to make sure it's not running when you unregister your hardware struct, and _that_ doesn't have the rtnl taken so you can use cancel_work_sync() there.

Quoted 6 lines

True, but the race condition happens when ieee80211_stop() is called, and the rt2x00 work is triggered before before rt2x00->remove_interface() or rt2x00>stop() is called. when stop() is then called it cannot use cancel_work_sync() because
the work structure is waiting until stop() has completed which will not end until cancel_work_sync() has completed.

No, but I'm saying that ->stop doesn't need to cancel_work_sync(). You should have a work struct that you exclusively use for whatever you need to do with interfaces, so the code looks something like this:

void work(...)

{
	iterate_interfaces();
}

maybe with some additional locking. You also use schedule_work() and not the mac80211-provided workqueue.

This work will do exactly nothing if triggered before ->stop and running after ->stop because all interfaces will be gone. Hence, you don't need to cancel it in ->stop, you don't care whether it'll run or not.

Hence, the only thing you need to take care of is that it's canceled before unregister_hw(), and for that you can use cancel_work_sync().

Am I missing something?