* patch to fix potential problem with wiphy_update_regulatory
@ 2011-07-12 7:43 Sven Neumann
2011-07-12 7:43 ` [PATCH] cfg80211: hold reg_mutex when updating regulatory Sven Neumann
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Sven Neumann @ 2011-07-12 7:43 UTC (permalink / raw)
To: libertas-dev; +Cc: daniel, linux-wireless
While investigating the crashes I found that wiphy_update_regulatory
is from one place being called without the lock held that protects
the static variable it uses. I am sending you a patch that corrects
this issue. Unfortunately this change does not fix the crashes that
I reported. I am sending you the patch for review nevertheless.
Regards,
Sven
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH] cfg80211: hold reg_mutex when updating regulatory 2011-07-12 7:43 patch to fix potential problem with wiphy_update_regulatory Sven Neumann @ 2011-07-12 7:43 ` Sven Neumann 2011-07-12 7:52 ` patch to fix potential problem with wiphy_update_regulatory (update) Sven Neumann 2011-07-12 7:52 ` [PATCH] cfg80211: hold reg_mutex when updating regulatory Sven Neumann 2 siblings, 0 replies; 13+ messages in thread From: Sven Neumann @ 2011-07-12 7:43 UTC (permalink / raw) To: libertas-dev; +Cc: daniel, linux-wireless, Sven Neumann The function wiphy_update_regulatory() uses the static variable last_request and thus needs to be called with reg_mutex held. This is the case for all users in reg.c, but the function was exported for use by wiphy_register(), from where it is called without the lock being held. Fix this by making wiphy_update_regulatory() private and introducing regulatory_update() as a wrapper that acquires and holds the lock. Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> --- net/wireless/core.c | 2 +- net/wireless/core.h | 2 -- net/wireless/reg.c | 14 ++++++++++++-- net/wireless/reg.h | 2 ++ 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/net/wireless/core.c b/net/wireless/core.c index c22ef34..179921e 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -577,7 +577,7 @@ int wiphy_register(struct wiphy *wiphy) } /* set up regulatory info */ - wiphy_update_regulatory(wiphy, NL80211_REGDOM_SET_BY_CORE); + regulatory_update(wiphy, NL80211_REGDOM_SET_BY_CORE); list_add_rcu(&rdev->list, &cfg80211_rdev_list); cfg80211_rdev_list_generation++; diff --git a/net/wireless/core.h b/net/wireless/core.h index 3dce1f1..0517d03 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -277,8 +277,6 @@ extern int cfg80211_dev_rename(struct cfg80211_registered_device *rdev, char *newname); void ieee80211_set_bitrate_flags(struct wiphy *wiphy); -void wiphy_update_regulatory(struct wiphy *wiphy, - enum nl80211_reg_initiator setby); void cfg80211_bss_expire(struct cfg80211_registered_device *dev); void cfg80211_bss_age(struct cfg80211_registered_device *dev, diff --git a/net/wireless/reg.c b/net/wireless/reg.c index 1ad0f39..c61fd89 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -1119,11 +1119,13 @@ static void reg_process_ht_flags(struct wiphy *wiphy) } -void wiphy_update_regulatory(struct wiphy *wiphy, - enum nl80211_reg_initiator initiator) +static void wiphy_update_regulatory(struct wiphy *wiphy, + enum nl80211_reg_initiator initiator) { enum ieee80211_band band; + assert_reg_lock(); + if (ignore_reg_update(wiphy, initiator)) goto out; for (band = 0; band < IEEE80211_NUM_BANDS; band++) { @@ -1137,6 +1139,14 @@ out: wiphy->reg_notifier(wiphy, last_request); } +void regulatory_update(struct wiphy *wiphy, + enum nl80211_reg_initiator setby) +{ + mutex_lock(®_mutex); + wiphy_update_regulatory(wiphy, setby); + mutex_unlock(®_mutex); +} + static void handle_channel_custom(struct wiphy *wiphy, enum ieee80211_band band, unsigned int chan_idx, diff --git a/net/wireless/reg.h b/net/wireless/reg.h index b67d1c3..4a56799 100644 --- a/net/wireless/reg.h +++ b/net/wireless/reg.h @@ -16,6 +16,8 @@ void regulatory_exit(void); int set_regdom(const struct ieee80211_regdomain *rd); +void regulatory_update(struct wiphy *wiphy, enum nl80211_reg_initiator setby); + /** * regulatory_hint_found_beacon - hints a beacon was found on a channel * @wiphy: the wireless device where the beacon was found on -- 1.7.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* patch to fix potential problem with wiphy_update_regulatory (update) 2011-07-12 7:43 patch to fix potential problem with wiphy_update_regulatory Sven Neumann 2011-07-12 7:43 ` [PATCH] cfg80211: hold reg_mutex when updating regulatory Sven Neumann @ 2011-07-12 7:52 ` Sven Neumann 2011-07-12 7:52 ` [PATCH] cfg80211: hold reg_mutex when updating regulatory Sven Neumann 2 siblings, 0 replies; 13+ messages in thread From: Sven Neumann @ 2011-07-12 7:52 UTC (permalink / raw) To: libertas-dev; +Cc: daniel, linux-wireless Hi, I am sorry, I've picked up the wrong version of the patch. In order to compile the functions need to be reordered a little. Here's an updated version that actually compiles (and works for me). Regards, Sven ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] cfg80211: hold reg_mutex when updating regulatory 2011-07-12 7:43 patch to fix potential problem with wiphy_update_regulatory Sven Neumann 2011-07-12 7:43 ` [PATCH] cfg80211: hold reg_mutex when updating regulatory Sven Neumann 2011-07-12 7:52 ` patch to fix potential problem with wiphy_update_regulatory (update) Sven Neumann @ 2011-07-12 7:52 ` Sven Neumann 2011-07-15 17:32 ` John W. Linville 2 siblings, 1 reply; 13+ messages in thread From: Sven Neumann @ 2011-07-12 7:52 UTC (permalink / raw) To: libertas-dev; +Cc: daniel, linux-wireless, Sven Neumann The function wiphy_update_regulatory() uses the static variable last_request and thus needs to be called with reg_mutex held. This is the case for all users in reg.c, but the function was exported for use by wiphy_register(), from where it is called without the lock being held. Fix this by making wiphy_update_regulatory() private and introducing regulatory_update() as a wrapper that acquires and holds the lock. Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> --- net/wireless/core.c | 2 +- net/wireless/core.h | 2 - net/wireless/reg.c | 62 +++++++++++++++++++++++++++++--------------------- net/wireless/reg.h | 2 + 4 files changed, 39 insertions(+), 29 deletions(-) diff --git a/net/wireless/core.c b/net/wireless/core.c index c22ef34..179921e 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -577,7 +577,7 @@ int wiphy_register(struct wiphy *wiphy) } /* set up regulatory info */ - wiphy_update_regulatory(wiphy, NL80211_REGDOM_SET_BY_CORE); + regulatory_update(wiphy, NL80211_REGDOM_SET_BY_CORE); list_add_rcu(&rdev->list, &cfg80211_rdev_list); cfg80211_rdev_list_generation++; diff --git a/net/wireless/core.h b/net/wireless/core.h index 3dce1f1..0517d03 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -277,8 +277,6 @@ extern int cfg80211_dev_rename(struct cfg80211_registered_device *rdev, char *newname); void ieee80211_set_bitrate_flags(struct wiphy *wiphy); -void wiphy_update_regulatory(struct wiphy *wiphy, - enum nl80211_reg_initiator setby); void cfg80211_bss_expire(struct cfg80211_registered_device *dev); void cfg80211_bss_age(struct cfg80211_registered_device *dev, diff --git a/net/wireless/reg.c b/net/wireless/reg.c index 1ad0f39..a10dc34 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -912,14 +912,6 @@ static bool ignore_reg_update(struct wiphy *wiphy, return false; } -static void update_all_wiphy_regulatory(enum nl80211_reg_initiator initiator) -{ - struct cfg80211_registered_device *rdev; - - list_for_each_entry(rdev, &cfg80211_rdev_list, list) - wiphy_update_regulatory(&rdev->wiphy, initiator); -} - static void handle_reg_beacon(struct wiphy *wiphy, unsigned int chan_idx, struct reg_beacon *reg_beacon) @@ -1119,24 +1111,6 @@ static void reg_process_ht_flags(struct wiphy *wiphy) } -void wiphy_update_regulatory(struct wiphy *wiphy, - enum nl80211_reg_initiator initiator) -{ - enum ieee80211_band band; - - if (ignore_reg_update(wiphy, initiator)) - goto out; - for (band = 0; band < IEEE80211_NUM_BANDS; band++) { - if (wiphy->bands[band]) - handle_band(wiphy, band, initiator); - } -out: - reg_process_beacons(wiphy); - reg_process_ht_flags(wiphy); - if (wiphy->reg_notifier) - wiphy->reg_notifier(wiphy, last_request); -} - static void handle_channel_custom(struct wiphy *wiphy, enum ieee80211_band band, unsigned int chan_idx, @@ -1423,6 +1397,42 @@ new_request: return call_crda(last_request->alpha2); } +static void wiphy_update_regulatory(struct wiphy *wiphy, + enum nl80211_reg_initiator initiator) +{ + enum ieee80211_band band; + + assert_reg_lock(); + + if (ignore_reg_update(wiphy, initiator)) + goto out; + for (band = 0; band < IEEE80211_NUM_BANDS; band++) { + if (wiphy->bands[band]) + handle_band(wiphy, band, initiator); + } +out: + reg_process_beacons(wiphy); + reg_process_ht_flags(wiphy); + if (wiphy->reg_notifier) + wiphy->reg_notifier(wiphy, last_request); +} + +static void update_all_wiphy_regulatory(enum nl80211_reg_initiator initiator) +{ + struct cfg80211_registered_device *rdev; + + list_for_each_entry(rdev, &cfg80211_rdev_list, list) + wiphy_update_regulatory(&rdev->wiphy, initiator); +} + +void regulatory_update(struct wiphy *wiphy, + enum nl80211_reg_initiator setby) +{ + mutex_lock(®_mutex); + wiphy_update_regulatory(wiphy, setby); + mutex_unlock(®_mutex); +} + /* This processes *all* regulatory hints */ static void reg_process_hint(struct regulatory_request *reg_request) { diff --git a/net/wireless/reg.h b/net/wireless/reg.h index b67d1c3..4a56799 100644 --- a/net/wireless/reg.h +++ b/net/wireless/reg.h @@ -16,6 +16,8 @@ void regulatory_exit(void); int set_regdom(const struct ieee80211_regdomain *rd); +void regulatory_update(struct wiphy *wiphy, enum nl80211_reg_initiator setby); + /** * regulatory_hint_found_beacon - hints a beacon was found on a channel * @wiphy: the wireless device where the beacon was found on -- 1.7.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] cfg80211: hold reg_mutex when updating regulatory 2011-07-12 7:52 ` [PATCH] cfg80211: hold reg_mutex when updating regulatory Sven Neumann @ 2011-07-15 17:32 ` John W. Linville 2011-07-22 20:37 ` Sven Neumann 0 siblings, 1 reply; 13+ messages in thread From: John W. Linville @ 2011-07-15 17:32 UTC (permalink / raw) To: Sven Neumann; +Cc: libertas-dev, daniel, linux-wireless, mcgrof Luis, any comment on this? On Tue, Jul 12, 2011 at 09:52:39AM +0200, Sven Neumann wrote: > The function wiphy_update_regulatory() uses the static variable > last_request and thus needs to be called with reg_mutex held. > This is the case for all users in reg.c, but the function was > exported for use by wiphy_register(), from where it is called > without the lock being held. > > Fix this by making wiphy_update_regulatory() private and introducing > regulatory_update() as a wrapper that acquires and holds the lock. > > Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> > --- > net/wireless/core.c | 2 +- > net/wireless/core.h | 2 - > net/wireless/reg.c | 62 +++++++++++++++++++++++++++++--------------------- > net/wireless/reg.h | 2 + > 4 files changed, 39 insertions(+), 29 deletions(-) > > diff --git a/net/wireless/core.c b/net/wireless/core.c > index c22ef34..179921e 100644 > --- a/net/wireless/core.c > +++ b/net/wireless/core.c > @@ -577,7 +577,7 @@ int wiphy_register(struct wiphy *wiphy) > } > > /* set up regulatory info */ > - wiphy_update_regulatory(wiphy, NL80211_REGDOM_SET_BY_CORE); > + regulatory_update(wiphy, NL80211_REGDOM_SET_BY_CORE); > > list_add_rcu(&rdev->list, &cfg80211_rdev_list); > cfg80211_rdev_list_generation++; > diff --git a/net/wireless/core.h b/net/wireless/core.h > index 3dce1f1..0517d03 100644 > --- a/net/wireless/core.h > +++ b/net/wireless/core.h > @@ -277,8 +277,6 @@ extern int cfg80211_dev_rename(struct cfg80211_registered_device *rdev, > char *newname); > > void ieee80211_set_bitrate_flags(struct wiphy *wiphy); > -void wiphy_update_regulatory(struct wiphy *wiphy, > - enum nl80211_reg_initiator setby); > > void cfg80211_bss_expire(struct cfg80211_registered_device *dev); > void cfg80211_bss_age(struct cfg80211_registered_device *dev, > diff --git a/net/wireless/reg.c b/net/wireless/reg.c > index 1ad0f39..a10dc34 100644 > --- a/net/wireless/reg.c > +++ b/net/wireless/reg.c > @@ -912,14 +912,6 @@ static bool ignore_reg_update(struct wiphy *wiphy, > return false; > } > > -static void update_all_wiphy_regulatory(enum nl80211_reg_initiator initiator) > -{ > - struct cfg80211_registered_device *rdev; > - > - list_for_each_entry(rdev, &cfg80211_rdev_list, list) > - wiphy_update_regulatory(&rdev->wiphy, initiator); > -} > - > static void handle_reg_beacon(struct wiphy *wiphy, > unsigned int chan_idx, > struct reg_beacon *reg_beacon) > @@ -1119,24 +1111,6 @@ static void reg_process_ht_flags(struct wiphy *wiphy) > > } > > -void wiphy_update_regulatory(struct wiphy *wiphy, > - enum nl80211_reg_initiator initiator) > -{ > - enum ieee80211_band band; > - > - if (ignore_reg_update(wiphy, initiator)) > - goto out; > - for (band = 0; band < IEEE80211_NUM_BANDS; band++) { > - if (wiphy->bands[band]) > - handle_band(wiphy, band, initiator); > - } > -out: > - reg_process_beacons(wiphy); > - reg_process_ht_flags(wiphy); > - if (wiphy->reg_notifier) > - wiphy->reg_notifier(wiphy, last_request); > -} > - > static void handle_channel_custom(struct wiphy *wiphy, > enum ieee80211_band band, > unsigned int chan_idx, > @@ -1423,6 +1397,42 @@ new_request: > return call_crda(last_request->alpha2); > } > > +static void wiphy_update_regulatory(struct wiphy *wiphy, > + enum nl80211_reg_initiator initiator) > +{ > + enum ieee80211_band band; > + > + assert_reg_lock(); > + > + if (ignore_reg_update(wiphy, initiator)) > + goto out; > + for (band = 0; band < IEEE80211_NUM_BANDS; band++) { > + if (wiphy->bands[band]) > + handle_band(wiphy, band, initiator); > + } > +out: > + reg_process_beacons(wiphy); > + reg_process_ht_flags(wiphy); > + if (wiphy->reg_notifier) > + wiphy->reg_notifier(wiphy, last_request); > +} > + > +static void update_all_wiphy_regulatory(enum nl80211_reg_initiator initiator) > +{ > + struct cfg80211_registered_device *rdev; > + > + list_for_each_entry(rdev, &cfg80211_rdev_list, list) > + wiphy_update_regulatory(&rdev->wiphy, initiator); > +} > + > +void regulatory_update(struct wiphy *wiphy, > + enum nl80211_reg_initiator setby) > +{ > + mutex_lock(®_mutex); > + wiphy_update_regulatory(wiphy, setby); > + mutex_unlock(®_mutex); > +} > + > /* This processes *all* regulatory hints */ > static void reg_process_hint(struct regulatory_request *reg_request) > { > diff --git a/net/wireless/reg.h b/net/wireless/reg.h > index b67d1c3..4a56799 100644 > --- a/net/wireless/reg.h > +++ b/net/wireless/reg.h > @@ -16,6 +16,8 @@ void regulatory_exit(void); > > int set_regdom(const struct ieee80211_regdomain *rd); > > +void regulatory_update(struct wiphy *wiphy, enum nl80211_reg_initiator setby); > + > /** > * regulatory_hint_found_beacon - hints a beacon was found on a channel > * @wiphy: the wireless device where the beacon was found on > -- > 1.7.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- John W. Linville Someday the world will need a hero, and you linville@tuxdriver.com might be all we have. Be ready. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cfg80211: hold reg_mutex when updating regulatory 2011-07-15 17:32 ` John W. Linville @ 2011-07-22 20:37 ` Sven Neumann 2011-07-25 20:21 ` Luis R. Rodriguez 0 siblings, 1 reply; 13+ messages in thread From: Sven Neumann @ 2011-07-22 20:37 UTC (permalink / raw) To: John W. Linville; +Cc: libertas-dev, daniel, linux-wireless, mcgrof Hi, On Fri, 2011-07-15 at 13:32 -0400, John W. Linville wrote: > Luis, any comment on this? > > On Tue, Jul 12, 2011 at 09:52:39AM +0200, Sven Neumann wrote: > > The function wiphy_update_regulatory() uses the static variable > > last_request and thus needs to be called with reg_mutex held. > > This is the case for all users in reg.c, but the function was > > exported for use by wiphy_register(), from where it is called > > without the lock being held. > > > > Fix this by making wiphy_update_regulatory() private and introducing > > regulatory_update() as a wrapper that acquires and holds the lock. > > > > Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> I'd appreciate if someone would review this patch. But probably this is not really an issue except that it's somewhat ugly to export a function that should be called with a lock held and that lock is actually private. But in this particular case it is not a problem, as far as I can see, since the only user of wiphy_update_regulatory() outside net/wireless/reg.c is initialization code. So there is not likely going to be a race condition here. Sven ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cfg80211: hold reg_mutex when updating regulatory 2011-07-22 20:37 ` Sven Neumann @ 2011-07-25 20:21 ` Luis R. Rodriguez 2011-07-31 8:24 ` Sven Neumann 0 siblings, 1 reply; 13+ messages in thread From: Luis R. Rodriguez @ 2011-07-25 20:21 UTC (permalink / raw) To: Sven Neumann; +Cc: John W. Linville, libertas-dev, daniel, linux-wireless On Fri, Jul 22, 2011 at 1:37 PM, Sven Neumann <s.neumann@raumfeld.com> wrote: > Hi, > > On Fri, 2011-07-15 at 13:32 -0400, John W. Linville wrote: >> Luis, any comment on this? >> >> On Tue, Jul 12, 2011 at 09:52:39AM +0200, Sven Neumann wrote: >> > The function wiphy_update_regulatory() uses the static variable >> > last_request and thus needs to be called with reg_mutex held. >> > This is the case for all users in reg.c, but the function was >> > exported for use by wiphy_register(), from where it is called >> > without the lock being held. >> > >> > Fix this by making wiphy_update_regulatory() private and introducing >> > regulatory_update() as a wrapper that acquires and holds the lock. >> > >> > Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> > > I'd appreciate if someone would review this patch. But probably this is > not really an issue except that it's somewhat ugly to export a function > that should be called with a lock held and that lock is actually > private. But in this particular case it is not a problem, as far as I > can see, since the only user of wiphy_update_regulatory() outside > net/wireless/reg.c is initialization code. So there is not likely going > to be a race condition here. Apologies for the delay and thanks for the patch. The patch seems good except for the fact that there are so many changes reflected on the patch itself and this can be avoided by splitting the work into a few more patches so that the actual code changes required are reflected cleanly in one patch. Can you perhaps split up your changes so that moves of code are just that and actual code changes are reflected elsewhere? Luis ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cfg80211: hold reg_mutex when updating regulatory 2011-07-25 20:21 ` Luis R. Rodriguez @ 2011-07-31 8:24 ` Sven Neumann 2011-08-30 19:14 ` John W. Linville 0 siblings, 1 reply; 13+ messages in thread From: Sven Neumann @ 2011-07-31 8:24 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: John W. Linville, libertas-dev, daniel, linux-wireless On Mon, 2011-07-25 at 13:21 -0700, Luis R. Rodriguez wrote: > On Fri, Jul 22, 2011 at 1:37 PM, Sven Neumann <s.neumann@raumfeld.com> wrote: > > Hi, > > > > On Fri, 2011-07-15 at 13:32 -0400, John W. Linville wrote: > >> Luis, any comment on this? > >> > >> On Tue, Jul 12, 2011 at 09:52:39AM +0200, Sven Neumann wrote: > >> > The function wiphy_update_regulatory() uses the static variable > >> > last_request and thus needs to be called with reg_mutex held. > >> > This is the case for all users in reg.c, but the function was > >> > exported for use by wiphy_register(), from where it is called > >> > without the lock being held. > >> > > >> > Fix this by making wiphy_update_regulatory() private and introducing > >> > regulatory_update() as a wrapper that acquires and holds the lock. > >> > > >> > Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> > > > > I'd appreciate if someone would review this patch. But probably this is > > not really an issue except that it's somewhat ugly to export a function > > that should be called with a lock held and that lock is actually > > private. But in this particular case it is not a problem, as far as I > > can see, since the only user of wiphy_update_regulatory() outside > > net/wireless/reg.c is initialization code. So there is not likely going > > to be a race condition here. > > Apologies for the delay and thanks for the patch. The patch seems good > except for the fact that there are so many changes reflected on the > patch itself and this can be avoided by splitting the work into a few > more patches so that the actual code changes required are reflected > cleanly in one patch. Can you perhaps split up your changes so that > moves of code are just that and actual code changes are reflected > elsewhere? Sure, I can do that. But I won't get to it before next week as I am currently on vacation. Thanks for the review, Sven ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cfg80211: hold reg_mutex when updating regulatory 2011-07-31 8:24 ` Sven Neumann @ 2011-08-30 19:14 ` John W. Linville 2011-08-30 21:38 ` [PATCH 1/2] " Sven Neumann 0 siblings, 1 reply; 13+ messages in thread From: John W. Linville @ 2011-08-30 19:14 UTC (permalink / raw) To: Sven Neumann; +Cc: Luis R. Rodriguez, libertas-dev, daniel, linux-wireless On Sun, Jul 31, 2011 at 10:24:21AM +0200, Sven Neumann wrote: > On Mon, 2011-07-25 at 13:21 -0700, Luis R. Rodriguez wrote: > > On Fri, Jul 22, 2011 at 1:37 PM, Sven Neumann <s.neumann@raumfeld.com> wrote: > > > Hi, > > > > > > On Fri, 2011-07-15 at 13:32 -0400, John W. Linville wrote: > > >> Luis, any comment on this? > > >> > > >> On Tue, Jul 12, 2011 at 09:52:39AM +0200, Sven Neumann wrote: > > >> > The function wiphy_update_regulatory() uses the static variable > > >> > last_request and thus needs to be called with reg_mutex held. > > >> > This is the case for all users in reg.c, but the function was > > >> > exported for use by wiphy_register(), from where it is called > > >> > without the lock being held. > > >> > > > >> > Fix this by making wiphy_update_regulatory() private and introducing > > >> > regulatory_update() as a wrapper that acquires and holds the lock. > > >> > > > >> > Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> > > > > > > I'd appreciate if someone would review this patch. But probably this is > > > not really an issue except that it's somewhat ugly to export a function > > > that should be called with a lock held and that lock is actually > > > private. But in this particular case it is not a problem, as far as I > > > can see, since the only user of wiphy_update_regulatory() outside > > > net/wireless/reg.c is initialization code. So there is not likely going > > > to be a race condition here. > > > > Apologies for the delay and thanks for the patch. The patch seems good > > except for the fact that there are so many changes reflected on the > > patch itself and this can be avoided by splitting the work into a few > > more patches so that the actual code changes required are reflected > > cleanly in one patch. Can you perhaps split up your changes so that > > moves of code are just that and actual code changes are reflected > > elsewhere? > > Sure, I can do that. But I won't get to it before next week as I am > currently on vacation. Did you have a nice vacation? Are you ready to work on Linux again? :-) John -- John W. Linville Someday the world will need a hero, and you linville@tuxdriver.com might be all we have. Be ready. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] cfg80211: hold reg_mutex when updating regulatory 2011-08-30 19:14 ` John W. Linville @ 2011-08-30 21:38 ` Sven Neumann 2011-08-30 21:38 ` [PATCH 2/2] cfg80211: reorder code to obsolete forward declaration Sven Neumann 2011-08-30 22:24 ` [PATCH 1/2] cfg80211: hold reg_mutex when updating regulatory Luis R. Rodriguez 0 siblings, 2 replies; 13+ messages in thread From: Sven Neumann @ 2011-08-30 21:38 UTC (permalink / raw) To: linux-wireless Cc: Sven Neumann, John W. Linville, Luis R. Rodriguez, Daniel Mack The function wiphy_update_regulatory() uses the static variable last_request and thus needs to be called with reg_mutex held. This is the case for all users in reg.c, but the function was exported for use by wiphy_register(), from where it is called without the lock being held. Fix this by making wiphy_update_regulatory() private and introducing regulatory_update() as a wrapper that acquires and holds the lock. Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> Cc: John W. Linville <linville@tuxdriver.com> Cc: Luis R. Rodriguez <mcgrof@gmail.com> Cc: Daniel Mack <daniel@zonque.org> Cc: linux-wireless@vger.kernel.org --- net/wireless/core.c | 2 +- net/wireless/core.h | 2 -- net/wireless/reg.c | 17 +++++++++++++++-- net/wireless/reg.h | 2 ++ 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/net/wireless/core.c b/net/wireless/core.c index c148651..220f3bd 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -582,7 +582,7 @@ int wiphy_register(struct wiphy *wiphy) } /* set up regulatory info */ - wiphy_update_regulatory(wiphy, NL80211_REGDOM_SET_BY_CORE); + regulatory_update(wiphy, NL80211_REGDOM_SET_BY_CORE); list_add_rcu(&rdev->list, &cfg80211_rdev_list); cfg80211_rdev_list_generation++; diff --git a/net/wireless/core.h b/net/wireless/core.h index 8672e02..796a4bd 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -279,8 +279,6 @@ extern int cfg80211_dev_rename(struct cfg80211_registered_device *rdev, char *newname); void ieee80211_set_bitrate_flags(struct wiphy *wiphy); -void wiphy_update_regulatory(struct wiphy *wiphy, - enum nl80211_reg_initiator setby); void cfg80211_bss_expire(struct cfg80211_registered_device *dev); void cfg80211_bss_age(struct cfg80211_registered_device *dev, diff --git a/net/wireless/reg.c b/net/wireless/reg.c index 02751db..c50016e 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -103,6 +103,9 @@ struct reg_beacon { struct ieee80211_channel chan; }; +static void wiphy_update_regulatory(struct wiphy *wiphy, + enum nl80211_reg_initiator initiator); + static void reg_todo(struct work_struct *work); static DECLARE_WORK(reg_work, reg_todo); @@ -1119,11 +1122,13 @@ static void reg_process_ht_flags(struct wiphy *wiphy) } -void wiphy_update_regulatory(struct wiphy *wiphy, - enum nl80211_reg_initiator initiator) +static void wiphy_update_regulatory(struct wiphy *wiphy, + enum nl80211_reg_initiator initiator) { enum ieee80211_band band; + assert_reg_lock(); + if (ignore_reg_update(wiphy, initiator)) return; @@ -1138,6 +1143,14 @@ void wiphy_update_regulatory(struct wiphy *wiphy, wiphy->reg_notifier(wiphy, last_request); } +void regulatory_update(struct wiphy *wiphy, + enum nl80211_reg_initiator setby) +{ + mutex_lock(®_mutex); + wiphy_update_regulatory(wiphy, setby); + mutex_unlock(®_mutex); +} + static void handle_channel_custom(struct wiphy *wiphy, enum ieee80211_band band, unsigned int chan_idx, diff --git a/net/wireless/reg.h b/net/wireless/reg.h index b67d1c3..4a56799 100644 --- a/net/wireless/reg.h +++ b/net/wireless/reg.h @@ -16,6 +16,8 @@ void regulatory_exit(void); int set_regdom(const struct ieee80211_regdomain *rd); +void regulatory_update(struct wiphy *wiphy, enum nl80211_reg_initiator setby); + /** * regulatory_hint_found_beacon - hints a beacon was found on a channel * @wiphy: the wireless device where the beacon was found on -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] cfg80211: reorder code to obsolete forward declaration 2011-08-30 21:38 ` [PATCH 1/2] " Sven Neumann @ 2011-08-30 21:38 ` Sven Neumann 2011-08-30 22:25 ` Luis R. Rodriguez 2011-08-30 22:24 ` [PATCH 1/2] cfg80211: hold reg_mutex when updating regulatory Luis R. Rodriguez 1 sibling, 1 reply; 13+ messages in thread From: Sven Neumann @ 2011-08-30 21:38 UTC (permalink / raw) To: linux-wireless Cc: Sven Neumann, John W. Linville, Luis R. Rodriguez, Daniel Mack Reorder functions to remove the need for a forward declaration introduced by the last commit. Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> Cc: John W. Linville <linville@tuxdriver.com> Cc: Luis R. Rodriguez <mcgrof@gmail.com> Cc: Daniel Mack <daniel@zonque.org> Cc: linux-wireless@vger.kernel.org --- net/wireless/reg.c | 19 ++++++++----------- 1 files changed, 8 insertions(+), 11 deletions(-) diff --git a/net/wireless/reg.c b/net/wireless/reg.c index c50016e..c77928b 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -103,9 +103,6 @@ struct reg_beacon { struct ieee80211_channel chan; }; -static void wiphy_update_regulatory(struct wiphy *wiphy, - enum nl80211_reg_initiator initiator); - static void reg_todo(struct work_struct *work); static DECLARE_WORK(reg_work, reg_todo); @@ -915,14 +912,6 @@ static bool ignore_reg_update(struct wiphy *wiphy, return false; } -static void update_all_wiphy_regulatory(enum nl80211_reg_initiator initiator) -{ - struct cfg80211_registered_device *rdev; - - list_for_each_entry(rdev, &cfg80211_rdev_list, list) - wiphy_update_regulatory(&rdev->wiphy, initiator); -} - static void handle_reg_beacon(struct wiphy *wiphy, unsigned int chan_idx, struct reg_beacon *reg_beacon) @@ -1151,6 +1140,14 @@ void regulatory_update(struct wiphy *wiphy, mutex_unlock(®_mutex); } +static void update_all_wiphy_regulatory(enum nl80211_reg_initiator initiator) +{ + struct cfg80211_registered_device *rdev; + + list_for_each_entry(rdev, &cfg80211_rdev_list, list) + wiphy_update_regulatory(&rdev->wiphy, initiator); +} + static void handle_channel_custom(struct wiphy *wiphy, enum ieee80211_band band, unsigned int chan_idx, -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] cfg80211: reorder code to obsolete forward declaration 2011-08-30 21:38 ` [PATCH 2/2] cfg80211: reorder code to obsolete forward declaration Sven Neumann @ 2011-08-30 22:25 ` Luis R. Rodriguez 0 siblings, 0 replies; 13+ messages in thread From: Luis R. Rodriguez @ 2011-08-30 22:25 UTC (permalink / raw) To: Sven Neumann; +Cc: linux-wireless, John W. Linville, Daniel Mack On Tue, Aug 30, 2011 at 2:38 PM, Sven Neumann <s.neumann@raumfeld.com> wrote: > Reorder functions to remove the need for a forward declaration > introduced by the last commit. > > Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> > Cc: John W. Linville <linville@tuxdriver.com> > Cc: Luis R. Rodriguez <mcgrof@gmail.com> > Cc: Daniel Mack <daniel@zonque.org> > Cc: linux-wireless@vger.kernel.org Acked-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com> Luis ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] cfg80211: hold reg_mutex when updating regulatory 2011-08-30 21:38 ` [PATCH 1/2] " Sven Neumann 2011-08-30 21:38 ` [PATCH 2/2] cfg80211: reorder code to obsolete forward declaration Sven Neumann @ 2011-08-30 22:24 ` Luis R. Rodriguez 1 sibling, 0 replies; 13+ messages in thread From: Luis R. Rodriguez @ 2011-08-30 22:24 UTC (permalink / raw) To: Sven Neumann; +Cc: linux-wireless, John W. Linville, Daniel Mack On Tue, Aug 30, 2011 at 2:38 PM, Sven Neumann <s.neumann@raumfeld.com> wrote: > The function wiphy_update_regulatory() uses the static variable > last_request and thus needs to be called with reg_mutex held. > This is the case for all users in reg.c, but the function was > exported for use by wiphy_register(), from where it is called > without the lock being held. > > Fix this by making wiphy_update_regulatory() private and introducing > regulatory_update() as a wrapper that acquires and holds the lock. > > Signed-off-by: Sven Neumann <s.neumann@raumfeld.com> > Cc: John W. Linville <linville@tuxdriver.com> > Cc: Luis R. Rodriguez <mcgrof@gmail.com> > Cc: Daniel Mack <daniel@zonque.org> > Cc: linux-wireless@vger.kernel.org Acked-by: Luis R. Rodriguez <mcgrof@qca.qualcomm.com> Luis ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-08-30 22:26 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-07-12 7:43 patch to fix potential problem with wiphy_update_regulatory Sven Neumann 2011-07-12 7:43 ` [PATCH] cfg80211: hold reg_mutex when updating regulatory Sven Neumann 2011-07-12 7:52 ` patch to fix potential problem with wiphy_update_regulatory (update) Sven Neumann 2011-07-12 7:52 ` [PATCH] cfg80211: hold reg_mutex when updating regulatory Sven Neumann 2011-07-15 17:32 ` John W. Linville 2011-07-22 20:37 ` Sven Neumann 2011-07-25 20:21 ` Luis R. Rodriguez 2011-07-31 8:24 ` Sven Neumann 2011-08-30 19:14 ` John W. Linville 2011-08-30 21:38 ` [PATCH 1/2] " Sven Neumann 2011-08-30 21:38 ` [PATCH 2/2] cfg80211: reorder code to obsolete forward declaration Sven Neumann 2011-08-30 22:25 ` Luis R. Rodriguez 2011-08-30 22:24 ` [PATCH 1/2] cfg80211: hold reg_mutex when updating regulatory Luis R. Rodriguez
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).