Channel context refcount is protected by
chanctx_mtx. Accessing the value without holding
the mutex is racy. RCU section didn't guarantee
anything here.
Theoretically ieee80211_channel_switch() could
fail to see refcount change and read "1" instead
of, e.g. "2". This means mac80211 could accept CSA
even though it shouldn't have.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/cfg.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index aaa59d7..a79875c 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3233,23 +3233,23 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
&sdata->vif.bss_conf.chandef))
return -EINVAL;
- rcu_read_lock();
+ mutex_lock(&local->chanctx_mtx);
chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
if (!chanctx_conf) {
- rcu_read_unlock();
+ mutex_unlock(&local->chanctx_mtx);
return -EBUSY;
}
/* don't handle for multi-VIF cases */
chanctx = container_of(chanctx_conf, struct ieee80211_chanctx, conf);
if (chanctx->refcount > 1) {
- rcu_read_unlock();
+ mutex_unlock(&local->chanctx_mtx);
return -EBUSY;
}
num_chanctx = 0;
list_for_each_entry_rcu(chanctx, &local->chanctx_list, list)
num_chanctx++;
- rcu_read_unlock();
+ mutex_unlock(&local->chanctx_mtx);
if (num_chanctx > 1)
return -EBUSY;
--
1.8.5.3
On Tue, 2014-03-11 at 14:33 +0100, Michal Kazior wrote:
> >> I'm planning on adding `struct list_head` to sdata and chanctx to be
> >> able to iterate over assigned (and reserved, in the future) chanctx.
> >> What do you think? Do you have other ideas?
> >
> > I guess that seems reasonable - not sure why you'd want the reserved
> > one? Some of the maintenance there might be tricky?
>
> For multi-vif we need to be able to track list of interfaces that have
> a reservation. This is needed for example for chandef calculation
> (e.g. to see if you can start a new vif on given chanctx that is part
> of a reservation already, etc).
>
> It's also necessary so you can synchronize CSA across interfaces with
> the chanctx reservation that happen "in place" when you're out of
> max_num_different_channels. In that case you must unassign all
> interfaces, free chanctx, create new chanctx, and then assign the
> reservation interfaces.
>
> Actually I think there's also one thing that might benefit from
> chanctx-vif list tracking: ieee80211_recalc_chanctx_chantype(). It
> seems to complain with WARN_ON splat when I reload driver when more
> than 1 vif is running because it doesn't find a non-NULL `compat`
> chandef (rightfully so as local->interfaces is empty at that point due
> to ieee80211_remove_interfaces).
Ok, makes sense. I guess I don't see any easier solution either.
johannes
It doesn't make much sense to have a manually
managed chanctx refcount. It isn't performance
critical and it can't be even used outside the
protection of chanctx_mtx.
With future enhancements to channel contenxt
(namely reservations) refcount accounting will
become more complex and having manually managed
refcount is going to be error-prone and confusing.
The refcount is now equal to the number of vifs it
is assigned to. However in the future this will
change so keep the refcount/num_assigned_vifs
functions split now.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/cfg.c | 2 +-
net/mac80211/chan.c | 62 +++++++++++++++++++++++++++++++++++++---------
net/mac80211/ieee80211_i.h | 5 +++-
net/mac80211/mlme.c | 2 +-
4 files changed, 56 insertions(+), 15 deletions(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index a79875c..b5c9a77 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3242,7 +3242,7 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
/* don't handle for multi-VIF cases */
chanctx = container_of(chanctx_conf, struct ieee80211_chanctx, conf);
- if (chanctx->refcount > 1) {
+ if (ieee80211_chanctx_refcount(chanctx) > 1) {
mutex_unlock(&local->chanctx_mtx);
return -EBUSY;
}
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 5b946e8..a320dfd 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -9,6 +9,45 @@
#include "ieee80211_i.h"
#include "driver-ops.h"
+struct ieee80211_chanctx *
+ieee80211_vif_assigned_chanctx(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_chanctx_conf *conf;
+
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+ lockdep_is_held(&local->chanctx_mtx));
+ if (!conf)
+ return NULL;
+
+ return container_of(conf, struct ieee80211_chanctx, conf);
+}
+
+int ieee80211_chanctx_num_assigned_vifs(struct ieee80211_chanctx *ctx)
+{
+ struct ieee80211_local *local = ctx->local;
+ struct ieee80211_sub_if_data *sdata;
+ int num = 0;
+
+ lockdep_assert_held(&local->chanctx_mtx);
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list)
+ if (ieee80211_vif_assigned_chanctx(sdata) == ctx)
+ num++;
+ rcu_read_unlock();
+
+ return num;
+}
+
+int ieee80211_chanctx_refcount(struct ieee80211_chanctx *ctx)
+{
+ lockdep_assert_held(&ctx->local->chanctx_mtx);
+ return ieee80211_chanctx_num_assigned_vifs(ctx);
+}
+
static enum nl80211_chan_width ieee80211_get_sta_bw(struct ieee80211_sta *sta)
{
switch (sta->bandwidth) {
@@ -266,7 +305,7 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
bool check_single_channel = false;
lockdep_assert_held(&local->chanctx_mtx);
- WARN_ON_ONCE(ctx->refcount != 0);
+ WARN_ON_ONCE(ieee80211_chanctx_refcount(ctx) != 0);
if (!local->use_chanctx) {
struct cfg80211_chan_def *chandef = &local->_oper_chandef;
@@ -308,7 +347,6 @@ static int ieee80211_assign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
return ret;
rcu_assign_pointer(sdata->vif.chanctx_conf, &ctx->conf);
- ctx->refcount++;
ieee80211_recalc_txpower(sdata);
ieee80211_recalc_chanctx_min_def(local, ctx);
@@ -386,7 +424,6 @@ static void ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
lockdep_assert_held(&local->chanctx_mtx);
- ctx->refcount--;
rcu_assign_pointer(sdata->vif.chanctx_conf, NULL);
sdata->vif.bss_conf.idle = true;
@@ -397,12 +434,13 @@ static void ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
drv_unassign_vif_chanctx(local, sdata, ctx);
- if (ctx->refcount > 0) {
- ieee80211_recalc_chanctx_chantype(sdata->local, ctx);
- ieee80211_recalc_smps_chanctx(local, ctx);
- ieee80211_recalc_radar_chanctx(local, ctx);
- ieee80211_recalc_chanctx_min_def(local, ctx);
- }
+ if (ieee80211_chanctx_num_assigned_vifs(ctx) == 0)
+ return;
+
+ ieee80211_recalc_chanctx_chantype(sdata->local, ctx);
+ ieee80211_recalc_smps_chanctx(local, ctx);
+ ieee80211_recalc_radar_chanctx(local, ctx);
+ ieee80211_recalc_chanctx_min_def(local, ctx);
}
static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
@@ -421,7 +459,7 @@ static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
ctx = container_of(conf, struct ieee80211_chanctx, conf);
ieee80211_unassign_vif_chanctx(sdata, ctx);
- if (ctx->refcount == 0)
+ if (ieee80211_chanctx_refcount(ctx) == 0)
ieee80211_free_chanctx(local, ctx);
}
@@ -536,7 +574,7 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
ret = ieee80211_assign_vif_chanctx(sdata, ctx);
if (ret) {
/* if assign fails refcount stays the same */
- if (ctx->refcount == 0)
+ if (ieee80211_chanctx_refcount(ctx) == 0)
ieee80211_free_chanctx(local, ctx);
goto out;
}
@@ -577,7 +615,7 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
}
ctx = container_of(conf, struct ieee80211_chanctx, conf);
- if (ctx->refcount != 1) {
+ if (ieee80211_chanctx_refcount(ctx) != 1) {
ret = -EINVAL;
goto out;
}
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c21b0c3..11d70b5 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -693,7 +693,6 @@ struct ieee80211_chanctx {
struct ieee80211_local *local;
enum ieee80211_chanctx_mode mode;
- int refcount;
bool driver_present;
struct ieee80211_chanctx_conf conf;
@@ -1788,6 +1787,10 @@ void ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata);
void ieee80211_vif_vlan_copy_chanctx(struct ieee80211_sub_if_data *sdata);
void ieee80211_vif_copy_chanctx_to_vlans(struct ieee80211_sub_if_data *sdata,
bool clear);
+struct ieee80211_chanctx *
+ieee80211_vif_assigned_chanctx(struct ieee80211_sub_if_data *sdata);
+int ieee80211_chanctx_num_assigned_vifs(struct ieee80211_chanctx *ctx);
+int ieee80211_chanctx_refcount(struct ieee80211_chanctx *ctx);
void ieee80211_recalc_smps_chanctx(struct ieee80211_local *local,
struct ieee80211_chanctx *chanctx);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 46b62bb..c0b2fb0 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1066,7 +1066,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
}
chanctx = container_of(rcu_access_pointer(sdata->vif.chanctx_conf),
struct ieee80211_chanctx, conf);
- if (chanctx->refcount > 1) {
+ if (ieee80211_chanctx_refcount(chanctx) > 1) {
sdata_info(sdata,
"channel switch with multiple interfaces on the same channel, disconnecting\n");
ieee80211_queue_work(&local->hw,
--
1.8.5.3
On Sun, Mar 9, 2014 at 11:47 AM, Eliad Peller <[email protected]> wrote:
> On Fri, Mar 7, 2014 at 1:19 PM, Michal Kazior <[email protected]> wrote:
>> It doesn't make much sense to have a manually
>> managed chanctx refcount. It isn't performance
>> critical and it can't be even used outside the
>> protection of chanctx_mtx.
>>
>> With future enhancements to channel contenxt
>> (namely reservations) refcount accounting will
>> become more complex and having manually managed
>> refcount is going to be error-prone and confusing.
>>
>> The refcount is now equal to the number of vifs it
>> is assigned to. However in the future this will
>> change so keep the refcount/num_assigned_vifs
>> functions split now.
>>
>> Signed-off-by: Michal Kazior <[email protected]>
>> ---
> [...]
>
>> +int ieee80211_chanctx_num_assigned_vifs(struct ieee80211_chanctx *ctx)
>> +{
>> + struct ieee80211_local *local = ctx->local;
>> + struct ieee80211_sub_if_data *sdata;
>> + int num = 0;
>> +
>> + lockdep_assert_held(&local->chanctx_mtx);
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(sdata, &local->interfaces, list)
>> + if (ieee80211_vif_assigned_chanctx(sdata) == ctx)
>> + num++;
>> + rcu_read_unlock();
>
> i don't think you need rcu_read_lock if you already hold chanctx_mtx
>
just realized this for the interface iteration, not for the chanctx.
please ignore :)
Eliad.
On Tue, 2014-03-11 at 08:46 +0100, Michal Kazior wrote:
> Self-NACK.
Heh.
> I totally missed how ieee80211_remove_interfaces() works. It removes
> all interfaces from the list in one go before each is actually
> stopped. This means refcount computes 0. IOW If you unload a driver
> with multiple interfaces running you get a nice general protection
> fault (multiple ieee80211_free_chanctx calls).
Ok, that seems like a problem :)
> I'm planning on adding `struct list_head` to sdata and chanctx to be
> able to iterate over assigned (and reserved, in the future) chanctx.
> What do you think? Do you have other ideas?
I guess that seems reasonable - not sure why you'd want the reserved
one? Some of the maintenance there might be tricky?
johannes
On Sun, 2014-03-09 at 11:40 +0200, Eliad Peller wrote:
> > @@ -3233,23 +3233,23 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
> > &sdata->vif.bss_conf.chandef))
> > return -EINVAL;
> >
> > - rcu_read_lock();
> > + mutex_lock(&local->chanctx_mtx);
> > chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
> this should probably be rcu_dereference_protected now?
Yes, in fact, lockdep ought to warn about this usage now, please enable
it :)
johannes
On Fri, 2014-03-07 at 12:19 +0100, Michal Kazior wrote:
> It doesn't make much sense to have a manually
> managed chanctx refcount. It isn't performance
> critical and it can't be even used outside the
> protection of chanctx_mtx.
>
> With future enhancements to channel contenxt
> (namely reservations) refcount accounting will
> become more complex and having manually managed
> refcount is going to be error-prone and confusing.
>
> The refcount is now equal to the number of vifs it
> is assigned to. However in the future this will
> change so keep the refcount/num_assigned_vifs
> functions split now.
Alright, all of this looks fine, but we have a ton of churn in this area
right now, and I've been sick and not as attentive as usual. Somebody
also recently posted patches to refactor the recalc_* functions for
channel contexts, which is sure to conflict with this.
With Luca and myself both also travelling for the next ~week, what's
your preference on how to work this out? I likely won't be merging much
while I'm not at home.
At least one of you will probably have to rebase the patches, and while
I don't really care who does it or in which order they go in, I didn't
really want to say that only in over a week from now.
Maybe one of you can just put a git tree somewhere in the meantime
(forked from mac80211-next) and collect those CSA/chanctx-related
patches in there? So far, I think most patches I've seen fly by looked
fine (from their description, I haven't had a chance to look into them
in too much detail.)
Of course I don't mind if you want to just wait for me to be back, I'm
happy to do the merging and then complain about patches that need
rebasing, just be warned that that would likely only happen after the
17th.
johannes
On Fri, 2014-03-07 at 12:19 +0100, Michal Kazior wrote:
> Channel contexts are implicitly bound to hw
> already so make it possible to dereference it
> through the channel context itself.
>
> This will make it possible to reduce number of
> arguments for chanctx-related functions.
I don't really see much point in this, is there ever anything where we
really get only a channel context and don't already have the local
pointer?
With interfaces, at least, we need it because we can get an interface
through e.g. do_stop or do_open, so we don't have an easy way to
reference local, but here? I don't think our functions have so many
arguments that it's worth putting a pointer into the struct instead.
johannes
On 7 March 2014 12:19, Michal Kazior <[email protected]> wrote:
> It doesn't make much sense to have a manually
> managed chanctx refcount. It isn't performance
> critical and it can't be even used outside the
> protection of chanctx_mtx.
>
> With future enhancements to channel contenxt
> (namely reservations) refcount accounting will
> become more complex and having manually managed
> refcount is going to be error-prone and confusing.
>
> The refcount is now equal to the number of vifs it
> is assigned to. However in the future this will
> change so keep the refcount/num_assigned_vifs
> functions split now.
>
> Signed-off-by: Michal Kazior <[email protected]>
Self-NACK.
I totally missed how ieee80211_remove_interfaces() works. It removes
all interfaces from the list in one go before each is actually
stopped. This means refcount computes 0. IOW If you unload a driver
with multiple interfaces running you get a nice general protection
fault (multiple ieee80211_free_chanctx calls).
I'm planning on adding `struct list_head` to sdata and chanctx to be
able to iterate over assigned (and reserved, in the future) chanctx.
What do you think? Do you have other ideas?
Michał
On Fri, Mar 7, 2014 at 1:19 PM, Michal Kazior <[email protected]> wrote:
> It doesn't make much sense to have a manually
> managed chanctx refcount. It isn't performance
> critical and it can't be even used outside the
> protection of chanctx_mtx.
>
> With future enhancements to channel contenxt
> (namely reservations) refcount accounting will
> become more complex and having manually managed
> refcount is going to be error-prone and confusing.
>
> The refcount is now equal to the number of vifs it
> is assigned to. However in the future this will
> change so keep the refcount/num_assigned_vifs
> functions split now.
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
[...]
> +int ieee80211_chanctx_num_assigned_vifs(struct ieee80211_chanctx *ctx)
> +{
> + struct ieee80211_local *local = ctx->local;
> + struct ieee80211_sub_if_data *sdata;
> + int num = 0;
> +
> + lockdep_assert_held(&local->chanctx_mtx);
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(sdata, &local->interfaces, list)
> + if (ieee80211_vif_assigned_chanctx(sdata) == ctx)
> + num++;
> + rcu_read_unlock();
i don't think you need rcu_read_lock if you already hold chanctx_mtx
Eliad.
On 11 March 2014 14:16, Johannes Berg <[email protected]> wrote:
> On Tue, 2014-03-11 at 08:46 +0100, Michal Kazior wrote:
>
>> Self-NACK.
>
> Heh.
>
>> I totally missed how ieee80211_remove_interfaces() works. It removes
>> all interfaces from the list in one go before each is actually
>> stopped. This means refcount computes 0. IOW If you unload a driver
>> with multiple interfaces running you get a nice general protection
>> fault (multiple ieee80211_free_chanctx calls).
>
> Ok, that seems like a problem :)
>
>> I'm planning on adding `struct list_head` to sdata and chanctx to be
>> able to iterate over assigned (and reserved, in the future) chanctx.
>> What do you think? Do you have other ideas?
>
> I guess that seems reasonable - not sure why you'd want the reserved
> one? Some of the maintenance there might be tricky?
For multi-vif we need to be able to track list of interfaces that have
a reservation. This is needed for example for chandef calculation
(e.g. to see if you can start a new vif on given chanctx that is part
of a reservation already, etc).
It's also necessary so you can synchronize CSA across interfaces with
the chanctx reservation that happen "in place" when you're out of
max_num_different_channels. In that case you must unassign all
interfaces, free chanctx, create new chanctx, and then assign the
reservation interfaces.
Actually I think there's also one thing that might benefit from
chanctx-vif list tracking: ieee80211_recalc_chanctx_chantype(). It
seems to complain with WARN_ON splat when I reload driver when more
than 1 vif is running because it doesn't find a non-NULL `compat`
chandef (rightfully so as local->interfaces is empty at that point due
to ieee80211_remove_interfaces).
Michał
On 10 March 2014 21:27, Johannes Berg <[email protected]> wrote:
> On Fri, 2014-03-07 at 12:19 +0100, Michal Kazior wrote:
>> Channel contexts are implicitly bound to hw
>> already so make it possible to dereference it
>> through the channel context itself.
>>
>> This will make it possible to reduce number of
>> arguments for chanctx-related functions.
>
> I don't really see much point in this, is there ever anything where we
> really get only a channel context and don't already have the local
> pointer?
>
> With interfaces, at least, we need it because we can get an interface
> through e.g. do_stop or do_open, so we don't have an easy way to
> reference local, but here? I don't think our functions have so many
> arguments that it's worth putting a pointer into the struct instead.
It just seems silly to me to pass local+chanctx all the time while
chanctx is already implicitly bound to a single hw.
I'm okay to drop this though. It's not really important.
Michał
On Fri, Mar 7, 2014 at 1:19 PM, Michal Kazior <[email protected]> wrote:
> Channel context refcount is protected by
> chanctx_mtx. Accessing the value without holding
> the mutex is racy. RCU section didn't guarantee
> anything here.
>
> Theoretically ieee80211_channel_switch() could
> fail to see refcount change and read "1" instead
> of, e.g. "2". This means mac80211 could accept CSA
> even though it shouldn't have.
>
> Signed-off-by: Michal Kazior <[email protected]>
> ---
[...]
> @@ -3233,23 +3233,23 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
> &sdata->vif.bss_conf.chandef))
> return -EINVAL;
>
> - rcu_read_lock();
> + mutex_lock(&local->chanctx_mtx);
> chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
this should probably be rcu_dereference_protected now?
Eliad.
On Fri, 2014-03-07 at 22:44:27 +0800, Johannes Berg wrote:
[...]
> Of course I don't mind if you want to just wait for me to be back, I'm
> happy to do the merging and then complain about patches that need
> rebasing, just be warned that that would likely only happen after the
> 17th.
My patch "mac80211: refactor recalculate channel context functions" can
wait, and I can rebase them if needed.
>
> johannes
Channel contexts are implicitly bound to hw
already so make it possible to dereference it
through the channel context itself.
This will make it possible to reduce number of
arguments for chanctx-related functions.
Signed-off-by: Michal Kazior <[email protected]>
---
net/mac80211/chan.c | 1 +
net/mac80211/ieee80211_i.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 42c6592..5b946e8 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -228,6 +228,7 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
ctx->conf.def = *chandef;
ctx->conf.rx_chains_static = 1;
ctx->conf.rx_chains_dynamic = 1;
+ ctx->local = local;
ctx->mode = mode;
ctx->conf.radar_enabled = ieee80211_is_radar_required(local);
ieee80211_recalc_chanctx_min_def(local, ctx);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 8603dfb..c21b0c3 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -691,6 +691,7 @@ struct ieee80211_chanctx {
struct list_head list;
struct rcu_head rcu_head;
+ struct ieee80211_local *local;
enum ieee80211_chanctx_mode mode;
int refcount;
bool driver_present;
--
1.8.5.3