[PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

With drivers that support multi-channel concurrency,
make sure that the correct context is in use before
sending a deauth frame. The driver could be in a different
context, in which case the frame could be dropped.

Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 net/mac80211/mlme.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 2de8870..43332ce 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4606,6 +4606,7 @@ int ieee80211_mgd_deauth(struct ieee80211_sub_if_data *sdata,
 			   req->bssid, req->reason_code,
 			   ieee80211_get_reason_code_string(req->reason_code));
 
+		drv_mgd_prepare_tx(sdata->local, sdata);
 		ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
 				       req->reason_code, tx, frame_buf);
 		cfg80211_tx_mlme_mgmt(sdata->dev, frame_buf,
-- 
2.1.0

Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Sujith Manoharan]

Sujith Manoharan wrote:
> From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
> 
> With drivers that support multi-channel concurrency,
> make sure that the correct context is in use before
> sending a deauth frame. The driver could be in a different
> context, in which case the frame could be dropped.

The behavior appears to have been changed in:

commit 8c7d857c4a4a552d8d3e1b2e24e1864ec2989285
Author: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Date:   Wed Jul 25 01:42:36 2012 +0300

    mac80211: don't call mgd_prepare_tx when associated


I am not sure how it can be guaranteed that the driver is
on the right context when trying to send a mgmt frame.
ath9k uses mgd_prepare_tx() to switch to the required context,
so I think this call is necessary even if we are associated.

Sujith

RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Grumbach, Emmanuel]

> 
> Sujith Manoharan wrote:
> > From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
> >
> > With drivers that support multi-channel concurrency, make sure that
> > the correct context is in use before sending a deauth frame. The
> > driver could be in a different context, in which case the frame could
> > be dropped.
> 
> The behavior appears to have been changed in:
> 
> commit 8c7d857c4a4a552d8d3e1b2e24e1864ec2989285
> Author: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> Date:   Wed Jul 25 01:42:36 2012 +0300
> 
>     mac80211: don't call mgd_prepare_tx when associated
> 
> 
> I am not sure how it can be guaranteed that the driver is on the right context
> when trying to send a mgmt frame.
> ath9k uses mgd_prepare_tx() to switch to the required context, so I think
> this call is necessary even if we are associated.
> 

Why are mgmt frames different from data frames with respect to that?
You have a separated queue for mgmt frames that is common to all the contexts?
Just like an "offchannel" queue?

RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Sujith Manoharan]

Grumbach, Emmanuel wrote:
> Why are mgmt frames different from data frames with respect to that?
> You have a separated queue for mgmt frames that is common to all the contexts?
> Just like an "offchannel" queue?

No, ath9k treats them the same, but if the current, active context is
different from the incoming frame's assigned context, then it is added
to a software queue for deferred transmission. But, before a switch
to the next context can happen, mac80211 proceeds to tear down the
BSS/stations etc. and the VIF/context is also removed.

Sujith

RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Grumbach, Emmanuel]

> 
> Grumbach, Emmanuel wrote:
> > Why are mgmt frames different from data frames with respect to that?
> > You have a separated queue for mgmt frames that is common to all the
> contexts?
> > Just like an "offchannel" queue?
> 
> No, ath9k treats them the same, but if the current, active context is different
> from the incoming frame's assigned context, then it is added to a software
> queue for deferred transmission. But, before a switch to the next context
> can happen, mac80211 proceeds to tear down the BSS/stations etc. and the
> VIF/context is also removed.
> 

I see - we also had this problem and because of this, we implemented the flush callback that allows to wait until all the queues are empty.
So mac80211 will send the deauth, flush and only then it'd remove the VIF. This should allow the frame to make it to the air.
Note that API explicitly states that mgd_prepare_tx is before assoc only.

RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Sujith Manoharan]

Grumbach, Emmanuel wrote:
> I see - we also had this problem and because of this, we implemented the flush
> callback that allows to wait until all the queues are empty.  So mac80211 will
> send the deauth, flush and only then it'd remove the VIF. This should allow
> the frame to make it to the air.  Note that API explicitly states that
> mgd_prepare_tx is before assoc only.

flush() doesn't force a context switch, no ?

For example, consider a P2P-GO in freq 2412 and STA in 5180 and both are active.
The current context is GO (2412) and at this time if a local deauth for the STA comes
from userspace, mac80211 sends a deauth frame to the driver and calls flush(),
but the context in the driver is still in channel 2412, so how will this work ?

Sujith

RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Grumbach, Emmanuel]

> 
> Grumbach, Emmanuel wrote:
> > I see - we also had this problem and because of this, we implemented
> > the flush callback that allows to wait until all the queues are empty.
> > So mac80211 will send the deauth, flush and only then it'd remove the
> > VIF. This should allow the frame to make it to the air.  Note that API
> > explicitly states that mgd_prepare_tx is before assoc only.
> 
> flush() doesn't force a context switch, no ?

No it doesn't.

> 
> For example, consider a P2P-GO in freq 2412 and STA in 5180 and both are
> active.
> The current context is GO (2412) and at this time if a local deauth for the STA
> comes from userspace, mac80211 sends a deauth frame to the driver and
> calls flush(), but the context in the driver is still in channel 2412, so how will
> this work ?
> 

It can wait until a context switch will occur in ath9k - you should switch from time
to time anyway to avoid starvation, and this is not under the radar of mac80211.

RE: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Sujith Manoharan]

Grumbach, Emmanuel wrote:
> It can wait until a context switch will occur in ath9k - you should switch
> from time to time anyway to avoid starvation, and this is not under the radar
> of mac80211.

But mac80211 doesn't wait for a switch to happen. It removes the BSS and station
info immediately after sending the deauth frame to the driver and the driver doesn't
get a chance to process the pending frames for the other context. I think
this race could be avoided by making sure the driver switches to the
required context before sending deauth.

Sujith

Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Emmanuel Grumbach]

On Wed, Sep 17, 2014 at 9:17 AM, Sujith Manoharan <sujith@msujith.org> wrote:
> Grumbach, Emmanuel wrote:
>> It can wait until a context switch will occur in ath9k - you should switch
>> from time to time anyway to avoid starvation, and this is not under the radar
>> of mac80211.
>
> But mac80211 doesn't wait for a switch to happen. It removes the BSS and station
> info immediately after sending the deauth frame to the driver and the driver doesn't
> get a chance to process the pending frames for the other context. I think
> this race could be avoided by making sure the driver switches to the
> required context before sending deauth.
>

mac80211 will not wait until the context switch happen specifically,
but if you can wait
in the driver in the flush() callback until the deauth is being sent -
whatever you need to
happen to have that done, context switch, TX queues to move etc... -
then you should be good?
IIRC, mac80211 sends the deauth to the driver and then calls flush() -
only then, it'll remove the
contexts.

So I guess that we agree - the solution for you I guess is to wait
until you have context switch
in the flush() callback.

Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Sujith Manoharan]

Emmanuel Grumbach wrote:
> mac80211 will not wait until the context switch happen specifically,
> but if you can wait
> in the driver in the flush() callback until the deauth is being sent -
> whatever you need to
> happen to have that done, context switch, TX queues to move etc... -
> then you should be good?
> IIRC, mac80211 sends the deauth to the driver and then calls flush() -
> only then, it'll remove the
> contexts.
> 
> So I guess that we agree - the solution for you I guess is to wait
> until you have context switch
> in the flush() callback.

I don't think trying to switch channel contexts in the flush() callback
is a good idea. We would be changing the meaning of the callback if
we do that. Moreover flush() is called from many other places too, like the
PS code.

It seems appropriate to instruct a driver to make the necessary
preparations before sending a deauth frame. Do you see any problems
if mgd_prepare_tx() is called before initiating deauth ?

Sujith

Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Sujith Manoharan]

Sujith Manoharan wrote:
> I don't think trying to switch channel contexts in the flush() callback
> is a good idea. We would be changing the meaning of the callback if
> we do that. Moreover flush() is called from many other places too, like the
> PS code.
> 
> It seems appropriate to instruct a driver to make the necessary
> preparations before sending a deauth frame. Do you see any problems
> if mgd_prepare_tx() is called before initiating deauth ?

Calling mgd_prepare_tx() in ieee80211_send_deauth_disassoc(), which
was the case originally, is probably more correct, since there are other
situations where this will be a problem.

CSA is one example. The driver can receive a beacon, mac80211 begins to
process the CSA and if multiple contexts are active, initiates a disconnect
operation. But, in the meantime, if the driver has switched to a different
context, things will break.

Sujith

Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Emmanuel Grumbach]

Hi,

On Thu, Sep 18, 2014 at 7:13 AM, Sujith Manoharan <sujith@msujith.org> wrote:
> Sujith Manoharan wrote:
>> I don't think trying to switch channel contexts in the flush() callback
>> is a good idea. We would be changing the meaning of the callback if
>> we do that. Moreover flush() is called from many other places too, like the
>> PS code.
>>
>> It seems appropriate to instruct a driver to make the necessary
>> preparations before sending a deauth frame. Do you see any problems
>> if mgd_prepare_tx() is called before initiating deauth ?
>
> Calling mgd_prepare_tx() in ieee80211_send_deauth_disassoc(), which
> was the case originally, is probably more correct, since there are other
> situations where this will be a problem.
>
> CSA is one example. The driver can receive a beacon, mac80211 begins to
> process the CSA and if multiple contexts are active, initiates a disconnect
> operation. But, in the meantime, if the driver has switched to a different
> context, things will break.
>

I still think it does not match the original intent of mgd_prepare_tx().
The way I see it, mgd_prepare_tx() ensures that the Tx *will* happen
in cases the driver has no specific reason to be on channel, mainly
because the context is not associated yet. Note that after association
mac80211 does not interfere with the context switches. The driver is in
charge of all this - why would it be different in this case?
flush() makes sure that the Tx *has* happened before destroying things.
The context switches from association until destruction are under the driver's
responsibility.

At least this is the way I see it.
We (Intel) will not break if you add this call, but in my eyes this does not
comply with the design. I guess you can always send a patch to add it and
see what others will say, but Johannes is away right now.
If you choose to do that, please add a reason as a parameter to mgd_prepare_tx()
so that we can ignore it when it is called before sending a frame that
is flush()'ed.
I still prefer to rely on flush(). And I agree that flush() isn't a
trivial implementation.

Note that the only reason I implemented flush() is because of 11w that forces us
to make sure that the deauth is sent, otherwise you'd be kicked by the AP after
3 cycles of association / disassociation.
So not sending the deauth from time to time isn't really a big deal in
my eyes, but
one can argue about that (just like about pretty much everything) :)

Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Sujith Manoharan]

Emmanuel Grumbach wrote:
> I still think it does not match the original intent of mgd_prepare_tx().
> The way I see it, mgd_prepare_tx() ensures that the Tx *will* happen
> in cases the driver has no specific reason to be on channel, mainly
> because the context is not associated yet. Note that after association
> mac80211 does not interfere with the context switches. The driver is in
> charge of all this - why would it be different in this case?
> flush() makes sure that the Tx *has* happened before destroying things.
> The context switches from association until destruction are under the driver's
> responsibility.
> 
> At least this is the way I see it.
> We (Intel) will not break if you add this call, but in my eyes this does not
> comply with the design. I guess you can always send a patch to add it and
> see what others will say, but Johannes is away right now.
> If you choose to do that, please add a reason as a parameter to mgd_prepare_tx()
> so that we can ignore it when it is called before sending a frame that
> is flush()'ed.
> I still prefer to rely on flush(). And I agree that flush() isn't a
> trivial implementation.

I can see your point and agree that the original intention of the mgd_prepare_tx()
callback is as described in the documentation. But with multiple active
contexts, flush() becomes a big hammer rather than a simple emptying of
the queues. :-)

I think we can extend the intention of the API and use mgd_prepare_tx()
for deauth/disassoc too. And this was done in the original patch, anyway.
This would greatly reduce driver complexity.

I'll send a patch including your suggestion about adding a parameter
to mgd_prepare_tx() which iwlwifi can use.

Sujith

Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Johannes Berg]

On Thu, 2014-09-18 at 11:19 +0530, Sujith Manoharan wrote:

> I can see your point and agree that the original intention of the mgd_prepare_tx()
> callback is as described in the documentation. But with multiple active
> contexts, flush() becomes a big hammer rather than a simple emptying of
> the queues. :-)

I'm a bit conflicted about this. If we add mgd_prepare_tx() here, what's
to say that we don't need to also do this for a variety of other frames?
Like sending delBA action frames? I guess the difference would be that
we don't disassoc immediately in those case.

However, if you consider deauth/flush while you're on a different
context, then mgd_prepare_tx() would potentially not really be
sufficient either. If that forces or waits for a context switch, you
really have no guarantee that the deauth frame went out because it might
be waiting behind other frames, and there's nothing that guarantees
enough time in the context slice to actually send out all of them ...

In that sense, I think you still have to implement flush.

But I also think you see it wrongly - you don't have to force a context
switch in flush, flush can essentially be "wait for queue to be empty"
and the context switching etc. happens in the background.

mgd_prepare_tx() is, the way I see it, really more for the case where
you really have no context scheduling *at all* and have to somehow send
a frame anyway. In the case we're talking about you have regular context
scheduling, and there shouldn't be a need to add a special scheduling on
top.

johannes

Re: [PATCH] mac80211: Call mgd_prepare_tx before deauthentication

[Sujith Manoharan]

Johannes Berg wrote:
> I'm a bit conflicted about this. If we add mgd_prepare_tx() here, what's
> to say that we don't need to also do this for a variety of other frames?
> Like sending delBA action frames? I guess the difference would be that
> we don't disassoc immediately in those case.

Deauth was the main problem with ath9k, I didn't check delba or other
mgmt frames.

> However, if you consider deauth/flush while you're on a different
> context, then mgd_prepare_tx() would potentially not really be
> sufficient either. If that forces or waits for a context switch, you
> really have no guarantee that the deauth frame went out because it might
> be waiting behind other frames, and there's nothing that guarantees
> enough time in the context slice to actually send out all of them ...

It works with ath9k now because all non-data frames are sent immediately,
without being added to the internal SW queue.

> In that sense, I think you still have to implement flush.

Yes, I think we might have to. :)

> But I also think you see it wrongly - you don't have to force a context
> switch in flush, flush can essentially be "wait for queue to be empty"
> and the context switching etc. happens in the background.

flush() implementation in ath9k currently waits for the HW queues
and the SW queues assigned to the current channel context to become empty.

A context switch can be forced to expedite the process of draining
the queues in all contexts and we probably have to send out a new
NoA if a GO is active to accommodate the longer absence period
when flushing. The fact that, in ath9k,  the same flush routine is called
from the main context scheduler and the mac80211 callback makes things
hairy. :)

But, I agree (and Emmanuel was right), flush() in ath9k needs
to be reworked. Please drop this patch, I'll carry it internally until
flush is fixed.

Sujith