cfg80211 wext compat w/o wext code changes, rtnl locking

cfg80211 wext compat w/o wext code changes, rtnl locking

[Johannes Berg]

Doh! /me smacks forehead. It could've been so easy.

How about we simply use do_ioctl for cfg80211/wext compat? wext still
calls that "for old devices" and we'll need to take a bit more care than
usual, but it should be much simpler than trying to assign all
callbacks. And less code to boot.

It doesn't really matter that we then seize do_ioctl, the ioctls you can
do aren't interesting for wireless devices.

As for locking issues... Once mac80211 doesn't use the rtnl any more (I
firmly believe it has no business using it) we'll need to make wext not
acquire rtnl or we'll run into ABBA deadlock issues. Below is an
untested patch to achieve this.

For building external modules, we'll have to carry a small patch to make
cfg80211 acquire the rtnl for each call, and a small patch to mac80211
use the locked variants for registering netdevs etc.

But all that once mac80211 actually doesn't use the rtnl any more.

johannes

---
 include/net/iw_handler.h   |    3 +
 net/wireless/wext-common.c |    6 ---
 net/wireless/wext-old.c    |   70 ++++++++++++++++++++++++++++-----------------
 3 files changed, 47 insertions(+), 32 deletions(-)

--- wireless-dev.orig/include/net/iw_handler.h	2007-03-29 00:52:50.965831178 +0200
+++ wireless-dev/include/net/iw_handler.h	2007-03-29 00:53:21.455831178 +0200
@@ -345,6 +345,9 @@ struct iw_handler_def
 	 * The old pointer in struct net_device will be gradually phased
 	 * out, and drivers are encouraged to use this one... */
 	struct iw_statistics*	(*get_wireless_stats)(struct net_device *dev);
+
+	/* can live without rtnl locking? */
+	int no_locking;
 };
 
 /* ---------------------- IOCTL DESCRIPTION ---------------------- */
--- wireless-dev.orig/net/wireless/wext-common.c	2007-03-29 00:52:50.995831178 +0200
+++ wireless-dev/net/wireless/wext-common.c	2007-03-29 00:53:21.475831178 +0200
@@ -631,15 +631,9 @@ int wext_ioctl(unsigned int cmd, struct 
 #ifdef CONFIG_WIRELESS_EXT
 	dev_load(ifr->ifr_name);
 
-	/* we could change the code to not hold the rtnl but
-	 * some callees might require it held */
-	rtnl_lock();
-
 	/* Follow me in wext-old.c */
 	ret = wireless_process_ioctl(ifr, cmd);
 
-	rtnl_unlock();
-
 	/* haha, I cheat here by allowing a driver or stack to have both WE and
 	 * CFG80211-WE for a little while during conversion... wext returns
 	 * -EOPNOTSUPP if a handler is not assigned, so we can in that case try
--- wireless-dev.orig/net/wireless/wext-old.c	2007-03-29 00:52:51.105831178 +0200
+++ wireless-dev/net/wireless/wext-old.c	2007-03-29 00:58:25.455831178 +0200
@@ -526,64 +526,82 @@ int wireless_process_ioctl(struct ifreq 
 {
 	struct net_device *dev;
 	iw_handler	handler;
+	int err;
 
 	/* Permissions are already checked in dev_ioctl() before calling us.
 	 * The copy_to/from_user() of ifr is also dealt with in there */
 
 	/* Make sure the device exist */
-	if ((dev = __dev_get_by_name(ifr->ifr_name)) == NULL)
+	if ((dev = dev_get_by_name(ifr->ifr_name)) == NULL)
 		return -ENODEV;
 
 	/* A bunch of special cases, then the generic case...
 	 * Note that 'cmd' is already filtered in dev_ioctl() with
 	 * (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST) */
-	switch(cmd) {
+	switch (cmd) {
 		case SIOCGIWSTATS:
 			/* Get Wireless Stats */
-			return ioctl_standard_call(dev,
-						   ifr,
-						   cmd,
-						   &iw_handler_get_iwstats);
-
+			err = ioctl_standard_call(dev,
+						  ifr,
+						  cmd,
+						  &iw_handler_get_iwstats);
+			break;
 		case SIOCGIWPRIV:
 			/* Check if we have some wireless handlers defined */
-			if(dev->wireless_handlers != NULL) {
+			if (dev->wireless_handlers) {
 				/* We export to user space the definition of
 				 * the private handler ourselves */
-				return ioctl_standard_call(dev,
-							   ifr,
-							   cmd,
-							   &iw_handler_get_private);
+				err = ioctl_standard_call(dev,
+							  ifr,
+							  cmd,
+							  &iw_handler_get_private);
+				break;
 			}
 			// ## Fall-through for old API ##
 		default:
 			/* Generic IOCTL */
 			/* Basic check */
-			if (!netif_device_present(dev))
-				return -ENODEV;
+			if (!netif_device_present(dev)) {
+				err = -ENODEV;
+				break;
+			}
 			/* New driver API : try to find the handler */
 			handler = get_handler(dev, cmd);
-			if(handler != NULL) {
+			if (handler) {
+				if (!dev->wireless_handlers->no_locking)
+					rtnl_lock();
 				/* Standard and private are not the same */
-				if(cmd < SIOCIWFIRSTPRIV)
-					return ioctl_standard_call(dev,
-								   ifr,
-								   cmd,
-								   handler);
-				else
-					return ioctl_private_call(dev,
+				if (cmd < SIOCIWFIRSTPRIV)
+					err = ioctl_standard_call(dev,
 								  ifr,
 								  cmd,
 								  handler);
+				else
+					err = ioctl_private_call(dev,
+								 ifr,
+								 cmd,
+								 handler);
+				if (!dev->wireless_handlers->no_locking)
+					rtnl_unlock();
+				break;
 			}
 			/* Old driver API : call driver ioctl handler */
 			if (dev->do_ioctl) {
-				return dev->do_ioctl(dev, ifr, cmd);
+				if (dev->wireless_handlers &&
+				    dev->wireless_handlers->no_locking)
+					err = dev->do_ioctl(dev, ifr, cmd);
+				else {
+					rtnl_lock();
+					err = dev->do_ioctl(dev, ifr, cmd);
+					rtnl_unlock();
+				}
+				break;
 			}
-			return -EOPNOTSUPP;
+			err = -EOPNOTSUPP;
 	}
-	/* Not reached */
-	return -EINVAL;
+
+	dev_put(dev);
+	return err;
 }
 
 /********************** ENHANCED IWSPY SUPPORT **********************/

Re: cfg80211 wext compat w/o wext code changes, rtnl locking

[Michael Buesch]

On Thursday 29 March 2007 01:00, Johannes Berg wrote:
> -			if(handler != NULL) {
> +			if (handler) {
> +				if (!dev->wireless_handlers->no_locking)
> +					rtnl_lock();

Is sparse OK with this conditional locking?

-- 
Greetings Michael.

Re: cfg80211 wext compat w/o wext code changes, rtnl locking

[Johannes Berg]

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

On Fri, 2007-03-30 at 12:11 +0200, Michael Buesch wrote:
> On Thursday 29 March 2007 01:00, Johannes Berg wrote:
> > -			if(handler != NULL) {
> > +			if (handler) {
> > +				if (!dev->wireless_handlers->no_locking)
> > +					rtnl_lock();
> 
> Is sparse OK with this conditional locking?

I also thought it wasn't but it didn't complain. The only reason why I
didn't just duplicate the code was the deep indentation here...

johannes

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

Re: cfg80211 wext compat w/o wext code changes, rtnl locking

[Pavel Roskin]

On Fri, 2007-03-30 at 12:37 +0200, Johannes Berg wrote:
> On Fri, 2007-03-30 at 12:11 +0200, Michael Buesch wrote:
> > On Thursday 29 March 2007 01:00, Johannes Berg wrote:
> > > -			if(handler != NULL) {
> > > +			if (handler) {
> > > +				if (!dev->wireless_handlers->no_locking)
> > > +					rtnl_lock();
> > 
> > Is sparse OK with this conditional locking?
> 
> I also thought it wasn't but it didn't complain. The only reason why I
> didn't just duplicate the code was the deep indentation here...

Just for your information, rtnl_lock() is actually a mutex.  Neither
rtnl_lock() nor any mutex operation are annotated to give sparse any
idea of what they are doing.

If sparse learns about mutexes, expect it to give a warning.  Please
consider if the code between rtnl_lock() and rtnl_unlock() could be
moved to a separate function so that locking and unlocking would happen
in the same basic block.

-- 
Regards,
Pavel Roskin

Re: cfg80211 wext compat w/o wext code changes, rtnl locking

[Johannes Berg]

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

On Mon, 2007-04-02 at 14:00 -0400, Pavel Roskin wrote:

> Just for your information, rtnl_lock() is actually a mutex. Neither
> rtnl_lock() nor any mutex operation are annotated to give sparse any
> idea of what they are doing.

Oh. And I guess rtnl_lock/unlock would need to be annotated and not just
the mutex operations (or are they inlines? I forgot)

> If sparse learns about mutexes, expect it to give a warning.  Please
> consider if the code between rtnl_lock() and rtnl_unlock() could be
> moved to a separate function so that locking and unlocking would happen
> in the same basic block.

Yeah. The patch is probably not complete anyway. I just floated it to
get some comments...

johannes

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

Re: cfg80211 wext compat w/o wext code changes, rtnl locking

[Pavel Roskin]

On Mon, 2007-04-02 at 20:15 +0200, Johannes Berg wrote:
> On Mon, 2007-04-02 at 14:00 -0400, Pavel Roskin wrote:
> 
> > Just for your information, rtnl_lock() is actually a mutex. Neither
> > rtnl_lock() nor any mutex operation are annotated to give sparse any
> > idea of what they are doing.
> 
> Oh. And I guess rtnl_lock/unlock would need to be annotated and not just
> the mutex operations (or are they inlines? I forgot)

They are not inlines.  Either way, sparse would require annotating them
if mutexes are to be tracked the way it's done for spinlocks.

-- 
Regards,
Pavel Roskin