[PATCH] wext: allow wext users without rtnl

[PATCH] wext: allow wext users without rtnl

[Johannes Berg]

This patch modifies wext adding a flag that allows drivers to specify that
they do not wish their wext calls made under rtnl. The default is to still
acquire the rtnl.

Warning: a device reference is held for the new no_locking API,
therefore it is not safe to call unregister_netdev on the passed device!

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

---
 include/net/iw_handler.h   |    3 +
 net/wireless/wext-common.c |    6 --
 net/wireless/wext-old.c    |  127 ++++++++++++++++++++++++---------------------
 3 files changed, 72 insertions(+), 64 deletions(-)

--- wireless-dev.orig/include/net/iw_handler.h	2007-04-13 23:20:56.755356880 +0200
+++ wireless-dev/include/net/iw_handler.h	2007-04-13 23:36:48.635356880 +0200
@@ -349,6 +349,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-04-13 23:20:56.785356880 +0200
+++ wireless-dev/net/wireless/wext-common.c	2007-04-13 23:36:48.635356880 +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-04-13 23:20:56.925356880 +0200
+++ wireless-dev/net/wireless/wext-old.c	2007-04-17 17:45:57.752028444 +0200
@@ -516,74 +516,85 @@ static inline int ioctl_private_call(str
 	return ret;
 }
 
-/* ---------------------------------------------------------------- */
+static int ioctl_call(struct net_device *dev, struct ifreq *ifr,
+		      unsigned int cmd, iw_handler handler)
+{
+	/* Standard and private are not the same */
+	if (cmd >= SIOCIWFIRSTPRIV)
+		return ioctl_private_call(dev, ifr, cmd, handler);
+
+	return ioctl_standard_call(dev, ifr, cmd, handler);
+}
+
+static int dev_process_ioctl(struct net_device *dev, struct ifreq *ifr,
+			     unsigned int cmd)
+{
+	iw_handler handler;
+
+	if (cmd == SIOCGIWSTATS) {
+		/* Get Wireless Stats */
+		return ioctl_standard_call(dev, ifr, cmd,
+					   &iw_handler_get_iwstats);
+	}
+
+	if (cmd == SIOCGIWPRIV && 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);
+	}
+
+	/* Basic check */
+	if (!netif_device_present(dev))
+		return -ENODEV;
+
+	/* New driver API : try to find the handler */
+	handler = get_handler(dev, cmd);
+	if (handler)
+		return ioctl_call(dev, ifr, cmd, handler);
+
+	/* Old driver API : call driver ioctl handler */
+	if (dev->do_ioctl)
+		return dev->do_ioctl(dev, ifr, cmd);
+
+	return -EOPNOTSUPP;
+}
+
 /*
- * Main IOCTl dispatcher. Called from the main networking code
- * (dev_ioctl() in net/core/dev.c).
- * Check the type of IOCTL and call the appropriate wrapper...
+ * Main ioctl dispatcher. Called from the common wireless code
+ * in net/wireless/wext-common.c.
  */
 int wireless_process_ioctl(struct ifreq *ifr, unsigned int cmd)
 {
 	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 */
+	/* Get device reference so it doesn't go away */
+	dev = dev_get_by_name(ifr->ifr_name);
 
-	/* Make sure the device exist */
-	if ((dev = __dev_get_by_name(ifr->ifr_name)) == NULL)
+	if (!dev)
 		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) {
-		case SIOCGIWSTATS:
-			/* Get Wireless Stats */
-			return ioctl_standard_call(dev,
-						   ifr,
-						   cmd,
-						   &iw_handler_get_iwstats);
-
-		case SIOCGIWPRIV:
-			/* Check if we have some wireless handlers defined */
-			if(dev->wireless_handlers != NULL) {
-				/* We export to user space the definition of
-				 * the private handler ourselves */
-				return ioctl_standard_call(dev,
-							   ifr,
-							   cmd,
-							   &iw_handler_get_private);
-			}
-			// ## Fall-through for old API ##
-		default:
-			/* Generic IOCTL */
-			/* Basic check */
-			if (!netif_device_present(dev))
-				return -ENODEV;
-			/* New driver API : try to find the handler */
-			handler = get_handler(dev, cmd);
-			if(handler != NULL) {
-				/* Standard and private are not the same */
-				if(cmd < SIOCIWFIRSTPRIV)
-					return ioctl_standard_call(dev,
-								   ifr,
-								   cmd,
-								   handler);
-				else
-					return ioctl_private_call(dev,
-								  ifr,
-								  cmd,
-								  handler);
-			}
-			/* Old driver API : call driver ioctl handler */
-			if (dev->do_ioctl) {
-				return dev->do_ioctl(dev, ifr, cmd);
-			}
-			return -EOPNOTSUPP;
+	/*
+	 * Check for the new no_locking API and if no locking is wanted then
+	 * call the handler without ever touching rtnl.
+	 * It needs to be this way to avoid ABBA type deadlocks.
+	 */
+	if (dev->wireless_handlers && dev->wireless_handlers->no_locking) {
+		err = dev_process_ioctl(dev, ifr, cmd);
+		dev_put(dev);
+	} else {
+		rtnl_lock();
+		/*
+		 * put device before calling handler, it might expect a
+		 * device with no held references in its own call path.
+		 */
+		dev_put(dev);
+		err = dev_process_ioctl(dev, ifr, cmd);
+		rtnl_unlock();
 	}
-	/* Not reached */
-	return -EINVAL;
+
+	return err;
 }
 
 /********************** ENHANCED IWSPY SUPPORT **********************/

Re: [PATCH] wext: allow wext users without rtnl

[Stephen Hemminger]

On Tue, 17 Apr 2007 17:50:10 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> This patch modifies wext adding a flag that allows drivers to specify that
> they do not wish their wext calls made under rtnl. The default is to still
> acquire the rtnl.
> 
> Warning: a device reference is held for the new no_locking API,
> therefore it is not safe to call unregister_netdev on the passed device!
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> 
>

NACK Conditional locking is bad design.
Either lock or not, make up your mind and fix the devices.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH] wext: allow wext users without rtnl

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Wed, 18 Apr 2007 14:04:51 -0700

> On Tue, 17 Apr 2007 17:50:10 +0200
> Johannes Berg <johannes@sipsolutions.net> wrote:
> 
> > This patch modifies wext adding a flag that allows drivers to specify that
> > they do not wish their wext calls made under rtnl. The default is to still
> > acquire the rtnl.
> > 
> > Warning: a device reference is held for the new no_locking API,
> > therefore it is not safe to call unregister_netdev on the passed device!
> > 
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > 
> >
> 
> NACK Conditional locking is bad design.
> Either lock or not, make up your mind and fix the devices.

It is also wrong because all entities in the kernel assume
that network configuration changes all occur under the RTNL
therefore it must be held consistently everywhere.

I don't know what kind of deadlock or other issues are being
worked around here, but you'll have to find another way.

Re: [PATCH] wext: allow wext users without rtnl

[Johannes Berg]

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

On Wed, 2007-04-18 at 14:09 -0700, David Miller wrote:

> > NACK Conditional locking is bad design.
> > Either lock or not, make up your mind and fix the devices.

That was because I don't think we can go fixing all current wext users
which might even be out of tree.

> It is also wrong because all entities in the kernel assume
> that network configuration changes all occur under the RTNL
> therefore it must be held consistently everywhere.

So you're saying that cfg80211 should hold rtnl too? I don't see any
point in holding rtnl (for example) for setting the SSID of a wireless
device.

johannes

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

Re: [PATCH] wext: allow wext users without rtnl

[Johannes Berg]

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

On Wed, 2007-04-18 at 14:04 -0700, Stephen Hemminger wrote:

> NACK Conditional locking is bad design.
> Either lock or not, make up your mind and fix the devices.

Ok looking at this again.

We essentially have the problem that all wext users assume that the rtnl
is held during wext calls, which is mostly required right now because
they are being passed a netdev pointer and no reference is taken. Also,
there shouldn't be multiple wext calls in progress at the same time.

In cfg80211, I decided that holding the rtnl would be mostly pointless
since configuration of a wireless device can't race against the other
config ioctls that could be done. I'd have to look at the details to see
if changing the interface mac address and setting a BSSID on it could
clash.

Hence, since cfg80211 doesn't hold rtnl, to avoid ABBA style deadlocks
wext entry points to the code need to be changed to not hold rtnl
either. Or cfg80211 needs to be changed to hold rtnl.

I'm not entirely opposed to making cfg80211 hold rtnl for configuration,
it just seems pointless to block configuration of different devices
against each other when even a for a single item it shouldn't matter.
Then again, holding rtnl in cfg80211 would simplify some things because
we could pass in a netdev pointer instead of the ifindex for example.

If you think I can justify holding rtnl in cfg80211 I'm certainly
willing to change that. Global locks are just too suspicious to me ;)

johannes

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

Re: [PATCH] wext: allow wext users without rtnl

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 20 Apr 2007 21:48:41 +0200

> I'm not entirely opposed to making cfg80211 hold rtnl for configuration,
> it just seems pointless to block configuration of different devices
> against each other when even a for a single item it shouldn't matter.
> Then again, holding rtnl in cfg80211 would simplify some things because
> we could pass in a netdev pointer instead of the ifindex for example.
> 
> If you think I can justify holding rtnl in cfg80211 I'm certainly
> willing to change that. Global locks are just too suspicious to me ;)

It is true that things like IPSEC configuration via xfrm_user.c
uses it's own mutex, so there is precedence for splitting up
the locking where things are truly seperate.

But changing things like MAC addresse changes, which you mention, do
fall into the existing scope of the RTNL semaphore.

If it simplifies things, all the reason more to use it for cfg80211.

Re: [PATCH] wext: allow wext users without rtnl

[Johannes Berg]

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

On Fri, 2007-04-20 at 13:32 -0700, David Miller wrote:

> > Then again, holding rtnl in cfg80211 would simplify some things because
> > we could pass in a netdev pointer instead of the ifindex for example.
> > 
> > If you think I can justify holding rtnl in cfg80211 I'm certainly
> > willing to change that. Global locks are just too suspicious to me ;)
> 
> It is true that things like IPSEC configuration via xfrm_user.c
> uses it's own mutex, so there is precedence for splitting up
> the locking where things are truly seperate.

:)

> But changing things like MAC addresse changes, which you mention, do
> fall into the existing scope of the RTNL semaphore.

Actually, just thought about mac address change again, it is only
permitted when devices are down anyway and at that point nothing can
race since wireless devices that are down just store configuration for
when they're up (or ignore it)

> If it simplifies things, all the reason more to use it for cfg80211.

A few places could use a netdev pointer instead of ifindex but I don't
feel it's much of a simplification for callees. This is only an issue
for two or three calls that cannot hold a netdev reference because the
device will be removed by the driver/stack.

Since the discussion came up due to dropping the rtnl from wext I think
that it may be beneficial if we added rtnl locking to cfg80211 now to
simplify migration away from wext which holds rtnl and then revisit the
locking when we can drop wext completely from the mac80211 code. That
would allow us to revisit rtnl locking for wext when we have completely
migrated to cfg80211 and wext is only handled through cfg80211.

Does that sound like an acceptable way forward?

johannes

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

Re: [PATCH] wext: allow wext users without rtnl

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 20 Apr 2007 22:53:42 +0200

> Since the discussion came up due to dropping the rtnl from wext I think
> that it may be beneficial if we added rtnl locking to cfg80211 now to
> simplify migration away from wext which holds rtnl and then revisit the
> locking when we can drop wext completely from the mac80211 code. That
> would allow us to revisit rtnl locking for wext when we have completely
> migrated to cfg80211 and wext is only handled through cfg80211.
> 
> Does that sound like an acceptable way forward?

Sure.