[PATCH] mac80211: handle hw roc cancel vs. expiration race

[PATCH] mac80211: handle hw roc cancel vs. expiration race

[Eliad Peller]

mac80211 handles roc expirations (indicated by
ieee80211_remain_on_channel_expired()) with a separate
work (hw_roc_done).

If userspace asks to cancel the ongoing roc before
hw_roc_done was scheduled, mac80211 will cancel
the ongoing roc, while leaving the hw_roc_done work
pending.

This might end up in the next roc being cancelled
by the stale work, causing mac80211 and the low-level
driver to go out of sync.

Fix it by cancelling hw_roc_done on roc cancel.
Since hw_roc_done takes local->mtx, temporarily
release it (before re-acquiring it and starting
the next roc if needed).

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 net/mac80211/cfg.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index dd4ff36..a80f7b3 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2782,7 +2782,22 @@ static int ieee80211_cancel_roc(struct ieee80211_local *local,
 		}
 
 		list_del(&found->list);
+		mutex_unlock(&local->mtx);
+
+		/*
+		 * The driver might have already expired the ROC (by
+		 * calling ieee80211_remain_on_channel_expired()), so
+		 * cancel the pending roc_done work (otherwise, it might
+		 * cancel the upcoming roc).
+		 */
+		cancel_work_sync(&local->hw_roc_done);
 
+		mutex_lock(&local->mtx);
+		/*
+		 * We hold rtnl, so nothing should have been able to
+		 * manipulate the roc_list during the temporary
+		 * lock release.
+		 */
 		if (found->started)
 			ieee80211_start_next_roc(local);
 		mutex_unlock(&local->mtx);
-- 
1.8.5.2.229.g4448466.dirty

Re: [PATCH] mac80211: handle hw roc cancel vs. expiration race

[Johannes Berg]

On Mon, 2015-03-09 at 13:53 +0200, Eliad Peller wrote:

> Fix it by cancelling hw_roc_done on roc cancel.
> Since hw_roc_done takes local->mtx, temporarily
> release it (before re-acquiring it and starting
> the next roc if needed).

I can't say I like this, and I'm not even sure it doesn't leave a race?
After all, the work will not take the RTNL.

Also, I think the whole concept is racy because you acquire "found"
before the unlock, but use it after the unlock, so if the work actually
ran (_sync, perhaps it already started) it may have freed the "found"
pointer.

I think the only way to really fix that is to make the driver return the
roc pointer or so and then we can either queue a work struct per roc
struct, or at least mark the roc struct as driver-destroyed and
double-check that in the work function?

Or perhaps we could flush the work before we even take the lock, but
then it might still race with the driver trying to cancel just after we
flushed I guess.

johannes

Re: [PATCH] mac80211: handle hw roc cancel vs. expiration race

[Eliad Peller]

On Mon, Mar 9, 2015 at 2:52 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2015-03-09 at 13:53 +0200, Eliad Peller wrote:
>
>> Fix it by cancelling hw_roc_done on roc cancel.
>> Since hw_roc_done takes local->mtx, temporarily
>> release it (before re-acquiring it and starting
>> the next roc if needed).
>
> I can't say I like this, and I'm not even sure it doesn't leave a race?
> After all, the work will not take the RTNL.
>
the other way (cancel while hw_roc_done) is protected, since
ieee80211_cancel_roc() looks for a specific roc cookie.

> Also, I think the whole concept is racy because you acquire "found"
> before the unlock, but use it after the unlock, so if the work actually
> ran (_sync, perhaps it already started) it may have freed the "found"
> pointer.
>
hw_roc_done work bails out in case of no started roc, so i think we're
safe here.

> I think the only way to really fix that is to make the driver return the
> roc pointer or so and then we can either queue a work struct per roc
> struct, or at least mark the roc struct as driver-destroyed and
> double-check that in the work function?
>
that was my initial implementation. but then i thought it will be
nicer to implement it in mac80211 and avoid changing all the drivers
(with the need to save the cookie, etc.)
do you prefer adding such cookie/pointer param?

> Or perhaps we could flush the work before we even take the lock, but
> then it might still race with the driver trying to cancel just after we
> flushed I guess.
right.

Eliad.

Re: [PATCH] mac80211: handle hw roc cancel vs. expiration race

[Johannes Berg]

To revive an old thread - sorry...

> >> Fix it by cancelling hw_roc_done on roc cancel.
> >> Since hw_roc_done takes local->mtx, temporarily
> >> release it (before re-acquiring it and starting
> >> the next roc if needed).
> >
> > I can't say I like this, and I'm not even sure it doesn't leave a race?
> > After all, the work will not take the RTNL.
> >
> the other way (cancel while hw_roc_done) is protected, since
> ieee80211_cancel_roc() looks for a specific roc cookie.

But you're saying that we rely on RTNL to not modify the list, and the
hw_roc_done work doesn't use the RTNL? Hmm. Are you saying now that it
also checks that it was started? But then why do we even have this
issue?

Or I can see why we have this issue, but perhaps we could fix it by
flushing the work *after* we cancel, so in case the driver was
cancelling at the same time we'd not yet have started the next one?

> > I think the only way to really fix that is to make the driver return the
> > roc pointer or so and then we can either queue a work struct per roc
> > struct, or at least mark the roc struct as driver-destroyed and
> > double-check that in the work function?
> >
> that was my initial implementation. but then i thought it will be
> nicer to implement it in mac80211 and avoid changing all the drivers
> (with the need to save the cookie, etc.)
> do you prefer adding such cookie/pointer param?

How difficult was it for the driver(s)? It seems it would make the code
far easier to reason about, which can only be a good thing. We'd save
the whole race discussion above :)

johannes

Re: [PATCH] mac80211: handle hw roc cancel vs. expiration race

[Eliad Peller]

On Wed, May 6, 2015 at 4:15 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> To revive an old thread - sorry...
>
>> >> Fix it by cancelling hw_roc_done on roc cancel.
>> >> Since hw_roc_done takes local->mtx, temporarily
>> >> release it (before re-acquiring it and starting
>> >> the next roc if needed).
>> >
>> > I can't say I like this, and I'm not even sure it doesn't leave a race?
>> > After all, the work will not take the RTNL.
>> >
>> the other way (cancel while hw_roc_done) is protected, since
>> ieee80211_cancel_roc() looks for a specific roc cookie.
>
> But you're saying that we rely on RTNL to not modify the list, and the
> hw_roc_done work doesn't use the RTNL? Hmm. Are you saying now that it
> also checks that it was started? But then why do we even have this
> issue?
>
sorry, i'll have to recall the exact details...

> Or I can see why we have this issue, but perhaps we could fix it by
> flushing the work *after* we cancel, so in case the driver was
> cancelling at the same time we'd not yet have started the next one?
>
that's basically what i did - canceled the work after
drv_cancel_remain_on_channel() was called.

>> > I think the only way to really fix that is to make the driver return the
>> > roc pointer or so and then we can either queue a work struct per roc
>> > struct, or at least mark the roc struct as driver-destroyed and
>> > double-check that in the work function?
>> >
>> that was my initial implementation. but then i thought it will be
>> nicer to implement it in mac80211 and avoid changing all the drivers
>> (with the need to save the cookie, etc.)
>> do you prefer adding such cookie/pointer param?
>
> How difficult was it for the driver(s)? It seems it would make the code
> far easier to reason about, which can only be a good thing. We'd save
> the whole race discussion above :)
>
sure. let's forget about this discussion and go with the clearer solution :)

Eliad.