2011-02-18 08:46:22

by Vivek Natarajan

[permalink] [raw]
Subject: [PATCH 1/2] mac80211: Fix a race on enabling power save.

There is a race on sending a data frame before the tx completion
of nullfunc frame for enabling power save. As the data quickly
follows the nullfunc frame, the AP thinks that the station is out
of power save and continues to send the frames. Whereas in the
station, the nullfunc ack will be processed after the tx completion
of data frame and mac80211 goes to powersave. Thus the power
save state mismatch between the station and the AP causes some
data loss and some applications fail because of that. This patch
fixes this issue.

Signed-off-by: Vivek Natarajan <[email protected]>
---
net/mac80211/mlme.c | 11 ++++++++++-
net/mac80211/status.c | 2 --
2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index d89e878..91006a2 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -738,8 +738,16 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
return;

if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
- (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)))
+ (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
+ netif_tx_stop_all_queues(sdata->dev);
+ /* flush all the frames queued in the driver before
+ * going to power save
+ */
+ drv_flush(local, false);
ieee80211_send_nullfunc(local, sdata, 1);
+ /* flush once again to get the tx status of nullfunc frame */
+ drv_flush(local, false);
+ }

if (!((local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
(local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)) ||
@@ -748,6 +756,7 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
local->hw.conf.flags |= IEEE80211_CONF_PS;
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
+ netif_tx_start_all_queues(sdata->dev);
}

void ieee80211_dynamic_ps_timer(unsigned long data)
diff --git a/net/mac80211/status.c b/net/mac80211/status.c
index 010a559..8651851 100644
--- a/net/mac80211/status.c
+++ b/net/mac80211/status.c
@@ -318,8 +318,6 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
if (info->flags & IEEE80211_TX_STAT_ACK) {
local->ps_sdata->u.mgd.flags |=
IEEE80211_STA_NULLFUNC_ACKED;
- ieee80211_queue_work(&local->hw,
- &local->dynamic_ps_enable_work);
} else
mod_timer(&local->dynamic_ps_timer, jiffies +
msecs_to_jiffies(10));
--
1.6.3.3



2011-02-21 07:21:21

by Vivek Natarajan

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.

On Sat, Feb 19, 2011 at 2:44 PM, Johannes Berg
<[email protected]> wrote:
> On Sat, 2011-02-19 at 11:42 +0530, Rajkumar Manoharan wrote:
>
>> > I know we currently don't allow powersave when there's more than one
>> > interface, but we still allow it while there are monitor interfaces and
>> > a monitor interface could be sending packets, which would violate the
>> > flush() contract with the driver -- the driver may expect not to get any
>> > packets during flush.
>
>> AFIK we also are not stopping tx queue of monitor iface on offchannel.
>> Doing so could help flush().
>
> Hrm, indeed. And we can't even stop the queues there. We can pause them
> for the flush though. In fact, why don't we just stop device queues for
> flush and bypass this problem.

If the device queues are stopped, we have to find some other fix for
this power save issue since null func frame will be queued to pending
frames as I mentioned earlier. Can we have this patch merged and fix
the monitor vif issue for this case and the scanning case later? I
have also sent a RFC patch earlier with stopping device queues and a
new PS_PENDING flag for this
(http://marc.info/?l=linux-wireless&m=129775059910372&w=3) That patch
also addresses this race.

Vivek.

2011-02-19 06:13:06

by Rajkumar Manoharan

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.

On Fri, Feb 18, 2011 at 04:37:25PM +0530, Johannes Berg wrote:
> On Fri, 2011-02-18 at 14:16 +0530, Vivek Natarajan wrote:
> > There is a race on sending a data frame before the tx completion
> > of nullfunc frame for enabling power save. As the data quickly
> > follows the nullfunc frame, the AP thinks that the station is out
> > of power save and continues to send the frames. Whereas in the
> > station, the nullfunc ack will be processed after the tx completion
> > of data frame and mac80211 goes to powersave. Thus the power
> > save state mismatch between the station and the AP causes some
> > data loss and some applications fail because of that. This patch
> > fixes this issue.
> >
> > Signed-off-by: Vivek Natarajan <[email protected]>
> > ---
> > net/mac80211/mlme.c | 11 ++++++++++-
> > net/mac80211/status.c | 2 --
> > 2 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> > index d89e878..91006a2 100644
> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -738,8 +738,16 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
> > return;
> >
> > if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
> > - (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)))
> > + (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
> > + netif_tx_stop_all_queues(sdata->dev);
> > + /* flush all the frames queued in the driver before
> > + * going to power save
> > + */
> > + drv_flush(local, false);
>
> Ok, I should've thought of this earlier -- but can we really do this?
>
> I know we currently don't allow powersave when there's more than one
> interface, but we still allow it while there are monitor interfaces and
> a monitor interface could be sending packets, which would violate the
> flush() contract with the driver -- the driver may expect not to get any
> packets during flush.
AFIK we also are not stopping tx queue of monitor iface on offchannel.
Doing so could help flush().

> I'm not sure. Maybe we should disable powersave when there are monitor
> interfaces after all, even though that would rob us of a way to debug
> things. Thoughts?
>
drivers like ath9k receives all frames from any bss on FIF_PROMISC_IN_BSS,
since there is no specific hw filter for that. So in that case it would be
better to disable power save.

--
Rajkumar

2011-02-19 09:14:55

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.

On Sat, 2011-02-19 at 11:42 +0530, Rajkumar Manoharan wrote:

> > I know we currently don't allow powersave when there's more than one
> > interface, but we still allow it while there are monitor interfaces and
> > a monitor interface could be sending packets, which would violate the
> > flush() contract with the driver -- the driver may expect not to get any
> > packets during flush.

> AFIK we also are not stopping tx queue of monitor iface on offchannel.
> Doing so could help flush().

Hrm, indeed. And we can't even stop the queues there. We can pause them
for the flush though. In fact, why don't we just stop device queues for
flush and bypass this problem. I also think we should flush the status
tasklet when drv_flush() has returned so that all status processing is
also flushed out before we return to regular processing.

> > I'm not sure. Maybe we should disable powersave when there are monitor
> > interfaces after all, even though that would rob us of a way to debug
> > things. Thoughts?

> drivers like ath9k receives all frames from any bss on FIF_PROMISC_IN_BSS,
> since there is no specific hw filter for that. So in that case it would be
> better to disable power save.

Well, I'm not convinced yet -- it will likely also complicate the code
quite a bit when we want to implement NoA powersave etc. Of course we're
trying to get rid of monitor interfaces there anyway, but still ...

johannes


2011-02-23 07:30:18

by Vivek Natarajan

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.

On Tue, Feb 22, 2011 at 6:32 PM, Johannes Berg
<[email protected]> wrote:
>> If the device queues are stopped, we have to find some other fix for
>> this power save issue since null func frame will be queued to pending
>> frames as I mentioned earlier. Can we have this patch merged and fix
>> the monitor vif issue for this case and the scanning case later? I
>> have also sent a RFC patch earlier with stopping device queues and a
>> new PS_PENDING flag for this
>> (http://marc.info/?l=linux-wireless&m=129775059910372&w=3) That patch
>> also addresses this race.
>
> I think for a first solution we can probably get away not worrying about
> monitor injection frames, since typically they'd be used by P2P things
> that disable powersave anyway. Right? And we already have the monitor
> vs. flush problem anyway.

Thanks. I will resend the patch rebased to current wireless-testing.

Vivek.

2011-02-22 13:01:03

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.

On Mon, 2011-02-21 at 12:51 +0530, Vivek Natarajan wrote:
> On Sat, Feb 19, 2011 at 2:44 PM, Johannes Berg
> <[email protected]> wrote:
> > On Sat, 2011-02-19 at 11:42 +0530, Rajkumar Manoharan wrote:
> >
> >> > I know we currently don't allow powersave when there's more than one
> >> > interface, but we still allow it while there are monitor interfaces and
> >> > a monitor interface could be sending packets, which would violate the
> >> > flush() contract with the driver -- the driver may expect not to get any
> >> > packets during flush.
> >
> >> AFIK we also are not stopping tx queue of monitor iface on offchannel.
> >> Doing so could help flush().
> >
> > Hrm, indeed. And we can't even stop the queues there. We can pause them
> > for the flush though. In fact, why don't we just stop device queues for
> > flush and bypass this problem.
>
> If the device queues are stopped, we have to find some other fix for
> this power save issue since null func frame will be queued to pending
> frames as I mentioned earlier. Can we have this patch merged and fix
> the monitor vif issue for this case and the scanning case later? I
> have also sent a RFC patch earlier with stopping device queues and a
> new PS_PENDING flag for this
> (http://marc.info/?l=linux-wireless&m=129775059910372&w=3) That patch
> also addresses this race.

I think for a first solution we can probably get away not worrying about
monitor injection frames, since typically they'd be used by P2P things
that disable powersave anyway. Right? And we already have the monitor
vs. flush problem anyway.

johannes


2011-02-18 11:07:30

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 1/2] mac80211: Fix a race on enabling power save.

On Fri, 2011-02-18 at 14:16 +0530, Vivek Natarajan wrote:
> There is a race on sending a data frame before the tx completion
> of nullfunc frame for enabling power save. As the data quickly
> follows the nullfunc frame, the AP thinks that the station is out
> of power save and continues to send the frames. Whereas in the
> station, the nullfunc ack will be processed after the tx completion
> of data frame and mac80211 goes to powersave. Thus the power
> save state mismatch between the station and the AP causes some
> data loss and some applications fail because of that. This patch
> fixes this issue.
>
> Signed-off-by: Vivek Natarajan <[email protected]>
> ---
> net/mac80211/mlme.c | 11 ++++++++++-
> net/mac80211/status.c | 2 --
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index d89e878..91006a2 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -738,8 +738,16 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
> return;
>
> if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
> - (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)))
> + (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
> + netif_tx_stop_all_queues(sdata->dev);
> + /* flush all the frames queued in the driver before
> + * going to power save
> + */
> + drv_flush(local, false);

Ok, I should've thought of this earlier -- but can we really do this?

I know we currently don't allow powersave when there's more than one
interface, but we still allow it while there are monitor interfaces and
a monitor interface could be sending packets, which would violate the
flush() contract with the driver -- the driver may expect not to get any
packets during flush.

I'm not sure. Maybe we should disable powersave when there are monitor
interfaces after all, even though that would rob us of a way to debug
things. Thoughts?

johannes