[PATCH] mac80211: Add RTNL version of ieee80211_iterate_active_interfaces

[PATCH] mac80211: Add RTNL version of ieee80211_iterate_active_interfaces

[Ivo van Doorn]

Since commit e38bad4766a110b61fa6038f10be16ced8c6cc38
	mac80211: make ieee80211_iterate_active_interfaces not need rtnl
rt2500usb and rt73usb broke down due to attempting register access
in atomic context.

This patch adds a ieee80211_iterate_active_interfaces_rtnl()
function that behaves exactly the same as the original
ieee80211_iterate_active_interfaces() function before the above
mentioned patch.

The add_interface, remove_interface() and config_interface()
functions are already under the RTNL lock by mac80211 so simply
calling  ieee80211_iterate_active_interfaces_rtnl() from within
a RTNL protected area would be sufficient to prevent race
conditions.

In rt2x00 only 1 callsite of ieee80211_iterate_active_interfaces()
needs to be changed, since only rt2x00lib_intf_scheduled() performs
hardware access per given interface.

Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>
---
John please queue for 2.6.26, this bug impacts both rt2500usb and rt73usb
and effectively shuts both interfaces down from proper functioning.. :S

diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index 213adbb..7fdbb84 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -25,6 +25,7 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/rtnetlink.h>
 
 #include "rt2x00.h"
 #include "rt2x00lib.h"
@@ -474,11 +475,15 @@ static void rt2x00lib_intf_scheduled(struct work_struct *work)
 
 	/*
 	 * Iterate over each interface and perform the
-	 * requested configurations.
-	 */
-	ieee80211_iterate_active_interfaces(rt2x00dev->hw,
-					    rt2x00lib_intf_scheduled_iter,
-					    rt2x00dev);
+	 * requested configurations, make sure to use
+	 * the _rtnl version since the regular version
+	 * requires atomic context.
+	 */
+	rtnl_lock();
+	ieee80211_iterate_active_interfaces_rtnl(rt2x00dev->hw,
+						 rt2x00lib_intf_scheduled_iter,
+						 rt2x00dev);
+	rtnl_unlock();
 }
 
 /*
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 7e75516..741a3d6 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1601,6 +1601,24 @@ void ieee80211_iterate_active_interfaces(struct ieee80211_hw *hw,
 					 void *data);
 
 /**
+ * ieee80211_iterate_active_interfaces_rtnl - iterate active interfaces
+ *
+ * This function iterates over the interfaces associated with a given
+ * hardware that are currently active and calls the callback for them.
+ * This function allows the iterator function to sleep, the caller is
+ * responsible for proper locking mechanisms to prevent concurrency.
+ *
+ * @hw: the hardware struct of which the interfaces should be iterated over
+ * @iterator: the iterator function to call
+ * @data: first argument of the iterator function
+ */
+void ieee80211_iterate_active_interfaces_rtnl(struct ieee80211_hw *hw,
+					      void (*iterator)(void *data,
+						u8 *mac,
+						struct ieee80211_vif *vif),
+					      void *data);
+
+/**
  * ieee80211_start_tx_ba_session - Start a tx Block Ack session.
  * @hw: pointer as obtained from ieee80211_alloc_hw().
  * @ra: receiver address of the BA session recipient
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 74c48ef..6980590 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -426,3 +426,36 @@ void ieee80211_iterate_active_interfaces(
 	rcu_read_unlock();
 }
 EXPORT_SYMBOL_GPL(ieee80211_iterate_active_interfaces);
+
+void ieee80211_iterate_active_interfaces_rtnl(
+	struct ieee80211_hw *hw,
+	void (*iterator)(void *data, u8 *mac,
+			 struct ieee80211_vif *vif),
+	void *data)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+	struct ieee80211_sub_if_data *sdata;
+
+	ASSERT_RTNL();
+
+	list_for_each_entry(sdata, &local->interfaces, list) {
+		switch (sdata->vif.type) {
+		case IEEE80211_IF_TYPE_INVALID:
+		case IEEE80211_IF_TYPE_MNTR:
+		case IEEE80211_IF_TYPE_VLAN:
+			continue;
+		case IEEE80211_IF_TYPE_AP:
+		case IEEE80211_IF_TYPE_STA:
+		case IEEE80211_IF_TYPE_IBSS:
+		case IEEE80211_IF_TYPE_WDS:
+		case IEEE80211_IF_TYPE_MESH_POINT:
+			break;
+		}
+		if (sdata->dev == local->mdev)
+			continue;
+		if (netif_running(sdata->dev))
+			iterator(data, sdata->dev->dev_addr,
+				 &sdata->vif);
+	}
+}
+EXPORT_SYMBOL_GPL(ieee80211_iterate_active_interfaces_rtnl);

Re: [PATCH] mac80211: Add RTNL version of ieee80211_iterate_active_interfaces

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]


> +	 * requested configurations, make sure to use
> +	 * the _rtnl version since the regular version
> +	 * requires atomic context.

That's a bit misleading, the regular version requires that the callback
is atomic :)

> +	 */
> +	rtnl_lock();
> +	ieee80211_iterate_active_interfaces_rtnl(rt2x00dev->hw,
> +						 rt2x00lib_intf_scheduled_iter,
> +						 rt2x00dev);
> +	rtnl_unlock();

>  /**
> + * ieee80211_iterate_active_interfaces_rtnl - iterate active interfaces
> + *
> + * This function iterates over the interfaces associated with a given
> + * hardware that are currently active and calls the callback for them.
> + * This function allows the iterator function to sleep, the caller is
> + * responsible for proper locking mechanisms to prevent concurrency.

That description seems misleading. Also, since the mac80211 internal
list management could possibly change (I don't see it changing but who
knows), I think I'd prefer if this function acquired the rtnl itself
didn't have rtnl in the name. Can't think of a good name right now,
maybe rename the other one to _atomic?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] mac80211: Add RTNL version of ieee80211_iterate_active_interfaces

[Ivo van Doorn]

On Tuesday 06 May 2008, Johannes Berg wrote:
> 
> > +	 * requested configurations, make sure to use
> > +	 * the _rtnl version since the regular version
> > +	 * requires atomic context.
> 
> That's a bit misleading, the regular version requires that the callback
> is atomic :)

Ok, will fix.

> > +	 */
> > +	rtnl_lock();
> > +	ieee80211_iterate_active_interfaces_rtnl(rt2x00dev->hw,
> > +						 rt2x00lib_intf_scheduled_iter,
> > +						 rt2x00dev);
> > +	rtnl_unlock();
> 
> >  /**
> > + * ieee80211_iterate_active_interfaces_rtnl - iterate active interfaces
> > + *
> > + * This function iterates over the interfaces associated with a given
> > + * hardware that are currently active and calls the callback for them.
> > + * This function allows the iterator function to sleep, the caller is
> > + * responsible for proper locking mechanisms to prevent concurrency.
> 
> That description seems misleading. Also, since the mac80211 internal
> list management could possibly change (I don't see it changing but who
> knows), I think I'd prefer if this function acquired the rtnl itself

Ok, that is what I prefered as well, but the patch reverted it to the
original version ;)

> didn't have rtnl in the name. Can't think of a good name right now,
> maybe rename the other one to _atomic?

Ok, will fix.

Ivo

[PATCH v2] mac80211: Add RTNL version of ieee80211_iterate_active_interfaces

[Ivo van Doorn]

Since commit e38bad4766a110b61fa6038f10be16ced8c6cc38
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mac80211: make ieee8021=
1_iterate_active_interfaces not need rtnl
rt2500usb and rt73usb broke down due to attempting register access
in atomic context (which is not possible for USB hardware).

This patch restores ieee80211_iterate_active_interfaces() to use RTNL l=
ock,
and provides the non-RTNL version under a new name:
	ieee80211_iterate_active_interfaces_atomic()

So far only rt2x00 uses ieee80211_iterate_active_interfaces(), and thos=
e
drivers require the RTNL version of ieee80211_iterate_active_interfaces=
().
Since they already call that function directly, this patch will automat=
ically
fix the USB rt2x00 drivers.

v2: Rename ieee80211_iterate_active_interfaces_rtnl

Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>
---
John, this is still 2.6.26 material. ;)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 7e75516..7d6de23 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1586,13 +1586,16 @@ void ieee80211_wake_queues(struct ieee80211_hw =
*hw);
 void ieee80211_scan_completed(struct ieee80211_hw *hw);
=20
 /**
- * ieee80211_iterate_active_interfaces - iterate active interfaces
+ * ieee80211_iterate_active_interfaces- iterate active interfaces
  *
  * This function iterates over the interfaces associated with a given
  * hardware that are currently active and calls the callback for them.
+ * This function allows the iterator function to sleep, when the itera=
tor
+ * function is atomic @ieee80211_iterate_active_interfaces_atomic can
+ * be used.
  *
  * @hw: the hardware struct of which the interfaces should be iterated=
 over
- * @iterator: the iterator function to call, cannot sleep
+ * @iterator: the iterator function to call
  * @data: first argument of the iterator function
  */
 void ieee80211_iterate_active_interfaces(struct ieee80211_hw *hw,
@@ -1601,6 +1604,24 @@ void ieee80211_iterate_active_interfaces(struct =
ieee80211_hw *hw,
 					 void *data);
=20
 /**
+ * ieee80211_iterate_active_interfaces_atomic - iterate active interfa=
ces
+ *
+ * This function iterates over the interfaces associated with a given
+ * hardware that are currently active and calls the callback for them.
+ * This function requires the iterator callback function to be atomic,
+ * if that is not desired, use @ieee80211_iterate_active_interfaces in=
stead.
+ *
+ * @hw: the hardware struct of which the interfaces should be iterated=
 over
+ * @iterator: the iterator function to call, cannot sleep
+ * @data: first argument of the iterator function
+ */
+void ieee80211_iterate_active_interfaces_atomic(struct ieee80211_hw *h=
w,
+						void (*iterator)(void *data,
+						    u8 *mac,
+						    struct ieee80211_vif *vif),
+						void *data);
+
+/**
  * ieee80211_start_tx_ba_session - Start a tx Block Ack session.
  * @hw: pointer as obtained from ieee80211_alloc_hw().
  * @ra: receiver address of the BA session recipient
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 74c48ef..2a8c0c5 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -401,6 +401,41 @@ void ieee80211_iterate_active_interfaces(
 	struct ieee80211_local *local =3D hw_to_local(hw);
 	struct ieee80211_sub_if_data *sdata;
=20
+	rtnl_lock();
+
+	list_for_each_entry(sdata, &local->interfaces, list) {
+		switch (sdata->vif.type) {
+		case IEEE80211_IF_TYPE_INVALID:
+		case IEEE80211_IF_TYPE_MNTR:
+		case IEEE80211_IF_TYPE_VLAN:
+			continue;
+		case IEEE80211_IF_TYPE_AP:
+		case IEEE80211_IF_TYPE_STA:
+		case IEEE80211_IF_TYPE_IBSS:
+		case IEEE80211_IF_TYPE_WDS:
+		case IEEE80211_IF_TYPE_MESH_POINT:
+			break;
+		}
+		if (sdata->dev =3D=3D local->mdev)
+			continue;
+		if (netif_running(sdata->dev))
+			iterator(data, sdata->dev->dev_addr,
+				 &sdata->vif);
+	}
+
+	rtnl_unlock();
+}
+EXPORT_SYMBOL_GPL(ieee80211_iterate_active_interfaces);
+
+void ieee80211_iterate_active_interfaces_atomic(
+	struct ieee80211_hw *hw,
+	void (*iterator)(void *data, u8 *mac,
+			 struct ieee80211_vif *vif),
+	void *data)
+{
+	struct ieee80211_local *local =3D hw_to_local(hw);
+	struct ieee80211_sub_if_data *sdata;
+
 	rcu_read_lock();
=20
 	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
@@ -425,4 +460,4 @@ void ieee80211_iterate_active_interfaces(
=20
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL_GPL(ieee80211_iterate_active_interfaces);
+EXPORT_SYMBOL_GPL(ieee80211_iterate_active_interfaces_atomic);
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] mac80211: Add RTNL version of ieee80211_iterate_active_interfaces

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 4891 bytes --]

On Tue, 2008-05-06 at 23:50 +0200, Ivo van Doorn wrote:
> Since commit e38bad4766a110b61fa6038f10be16ced8c6cc38
>         mac80211: make ieee80211_iterate_active_interfaces not need rtnl
> rt2500usb and rt73usb broke down due to attempting register access
> in atomic context (which is not possible for USB hardware).
> 
> This patch restores ieee80211_iterate_active_interfaces() to use RTNL lock,
> and provides the non-RTNL version under a new name:
> 	ieee80211_iterate_active_interfaces_atomic()
> 
> So far only rt2x00 uses ieee80211_iterate_active_interfaces(), and those
> drivers require the RTNL version of ieee80211_iterate_active_interfaces().
> Since they already call that function directly, this patch will automatically
> fix the USB rt2x00 drivers.
> 
> v2: Rename ieee80211_iterate_active_interfaces_rtnl
> 
> Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>

Acked-by: Johannes Berg <johannes@sipsolutions.net>

Thanks for doing this.


> John, this is still 2.6.26 material. ;)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 7e75516..7d6de23 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1586,13 +1586,16 @@ void ieee80211_wake_queues(struct ieee80211_hw *hw);
>  void ieee80211_scan_completed(struct ieee80211_hw *hw);
>  
>  /**
> - * ieee80211_iterate_active_interfaces - iterate active interfaces
> + * ieee80211_iterate_active_interfaces- iterate active interfaces
>   *
>   * This function iterates over the interfaces associated with a given
>   * hardware that are currently active and calls the callback for them.
> + * This function allows the iterator function to sleep, when the iterator
> + * function is atomic @ieee80211_iterate_active_interfaces_atomic can
> + * be used.
>   *
>   * @hw: the hardware struct of which the interfaces should be iterated over
> - * @iterator: the iterator function to call, cannot sleep
> + * @iterator: the iterator function to call
>   * @data: first argument of the iterator function
>   */
>  void ieee80211_iterate_active_interfaces(struct ieee80211_hw *hw,
> @@ -1601,6 +1604,24 @@ void ieee80211_iterate_active_interfaces(struct ieee80211_hw *hw,
>  					 void *data);
>  
>  /**
> + * ieee80211_iterate_active_interfaces_atomic - iterate active interfaces
> + *
> + * This function iterates over the interfaces associated with a given
> + * hardware that are currently active and calls the callback for them.
> + * This function requires the iterator callback function to be atomic,
> + * if that is not desired, use @ieee80211_iterate_active_interfaces instead.
> + *
> + * @hw: the hardware struct of which the interfaces should be iterated over
> + * @iterator: the iterator function to call, cannot sleep
> + * @data: first argument of the iterator function
> + */
> +void ieee80211_iterate_active_interfaces_atomic(struct ieee80211_hw *hw,
> +						void (*iterator)(void *data,
> +						    u8 *mac,
> +						    struct ieee80211_vif *vif),
> +						void *data);
> +
> +/**
>   * ieee80211_start_tx_ba_session - Start a tx Block Ack session.
>   * @hw: pointer as obtained from ieee80211_alloc_hw().
>   * @ra: receiver address of the BA session recipient
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 74c48ef..2a8c0c5 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -401,6 +401,41 @@ void ieee80211_iterate_active_interfaces(
>  	struct ieee80211_local *local = hw_to_local(hw);
>  	struct ieee80211_sub_if_data *sdata;
>  
> +	rtnl_lock();
> +
> +	list_for_each_entry(sdata, &local->interfaces, list) {
> +		switch (sdata->vif.type) {
> +		case IEEE80211_IF_TYPE_INVALID:
> +		case IEEE80211_IF_TYPE_MNTR:
> +		case IEEE80211_IF_TYPE_VLAN:
> +			continue;
> +		case IEEE80211_IF_TYPE_AP:
> +		case IEEE80211_IF_TYPE_STA:
> +		case IEEE80211_IF_TYPE_IBSS:
> +		case IEEE80211_IF_TYPE_WDS:
> +		case IEEE80211_IF_TYPE_MESH_POINT:
> +			break;
> +		}
> +		if (sdata->dev == local->mdev)
> +			continue;
> +		if (netif_running(sdata->dev))
> +			iterator(data, sdata->dev->dev_addr,
> +				 &sdata->vif);
> +	}
> +
> +	rtnl_unlock();
> +}
> +EXPORT_SYMBOL_GPL(ieee80211_iterate_active_interfaces);
> +
> +void ieee80211_iterate_active_interfaces_atomic(
> +	struct ieee80211_hw *hw,
> +	void (*iterator)(void *data, u8 *mac,
> +			 struct ieee80211_vif *vif),
> +	void *data)
> +{
> +	struct ieee80211_local *local = hw_to_local(hw);
> +	struct ieee80211_sub_if_data *sdata;
> +
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> @@ -425,4 +460,4 @@ void ieee80211_iterate_active_interfaces(
>  
>  	rcu_read_unlock();
>  }
> -EXPORT_SYMBOL_GPL(ieee80211_iterate_active_interfaces);
> +EXPORT_SYMBOL_GPL(ieee80211_iterate_active_interfaces_atomic);
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]