2015-06-01 13:59:52

by Wojciech Dubowik

Subject: [PATCH] mac80211: Fix double locking on CSA counter update

We call rcu locked ieee80211_csa_update_counter from
already locked section. Fix it by decrementing counter
directly instead of calling ieee80211_csa_update_counter.

Signed-off-by: Wojciech Dubowik <[email protected]>
---
net/mac80211/tx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 8df1342..9233559 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3338,7 +3338,7 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw,
if (beacon) {
if (beacon->csa_counter_offsets[0]) {
if (!is_template)
- ieee80211_csa_update_counter(vif);
+ WARN_ON_ONCE(beacon->csa_current_counter--);

ieee80211_set_csa(sdata, beacon);
}
@@ -3384,7 +3384,7 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw,

if (beacon->csa_counter_offsets[0]) {
if (!is_template)
- ieee80211_csa_update_counter(vif);
+ WARN_ON_ONCE(beacon->csa_current_counter--);

ieee80211_set_csa(sdata, beacon);
}
@@ -3414,7 +3414,7 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw,
* for now we leave it consistent with overall
* mac80211's behavior.
*/
- ieee80211_csa_update_counter(vif);
+ WARN_ON_ONCE(beacon->csa_current_counter--);

ieee80211_set_csa(sdata, beacon);
}
--
1.9.1

2015-06-01 14:23:42

by Wojciech Dubowik

Subject: Re: [PATCH] mac80211: Fix double locking on CSA counter update

On 01/06/15 16:13, Julian Calaby wrote:
> Hi Wojciech,
>
> On Mon, Jun 1, 2015 at 11:54 PM, Wojciech Dubowik
> <[email protected]> wrote:
>> We call rcu locked ieee80211_csa_update_counter from
>> already locked section. Fix it by decrementing counter
>> directly instead of calling ieee80211_csa_update_counter.
> Stupid question: wouldn't it be better to split the work from
> ieee80211_csa_update_counter() into a separate function without
> locking and call that instead?
Yes. It would be better for maintenance. It's just that they will
have different input parameters

ieee80211_csa_update_counter(struct ieee80211_vif *
__ieee80211_csa_update_counter(struct beacon_data *

as it doesn't make sense to dereference beacon twice.

I guess it's not a problem?
Wojtek
>
> Thanks,
>

2015-06-01 14:40:59

by Johannes Berg

Subject: Re: [PATCH] mac80211: Fix double locking on CSA counter update

On Mon, 2015-06-01 at 16:23 +0200, Wojciech Dubowik wrote:
> On 01/06/15 16:13, Julian Calaby wrote:
> > Hi Wojciech,
> >
> > On Mon, Jun 1, 2015 at 11:54 PM, Wojciech Dubowik
> > <[email protected]> wrote:
> >> We call rcu locked ieee80211_csa_update_counter from
> >> already locked section. Fix it by decrementing counter
> >> directly instead of calling ieee80211_csa_update_counter.
> > Stupid question: wouldn't it be better to split the work from
> > ieee80211_csa_update_counter() into a separate function without
> > locking and call that instead?
> Yes. It would be better for maintenance. It's just that they will
> have different input parameters
>
> ieee80211_csa_update_counter(struct ieee80211_vif *
> __ieee80211_csa_update_counter(struct beacon_data *
>
> as it doesn't make sense to dereference beacon twice.
>
> I guess it's not a problem?

Seems fine to me.

johannes

2015-06-01 14:13:47

by Julian Calaby

Subject: Re: [PATCH] mac80211: Fix double locking on CSA counter update

Hi Wojciech,

On Mon, Jun 1, 2015 at 11:54 PM, Wojciech Dubowik
<[email protected]> wrote:
> We call rcu locked ieee80211_csa_update_counter from
> already locked section. Fix it by decrementing counter
> directly instead of calling ieee80211_csa_update_counter.

Stupid question: wouldn't it be better to split the work from
ieee80211_csa_update_counter() into a separate function without
locking and call that instead?

Thanks,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/