[PATCH net] core: Don't attempt to load the "" driver.

[PATCH net] core: Don't attempt to load the "" driver.

[David Laight]

Requests like 'ifconfig ""' cause dev_load(net, "") be called, since
request_module("") return 0 (success) this generates the error message:
"Loading kernel module for a network device with CAP_SYS_MODULE (deprecated).  Use CAP_NET_ADMIN and alias netdev- instead."

Since dev_load() doesn't have to work, just ignore such names.
---

If you search for the above error message, you'll find a lot of complaints.
While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
the kernel shouldn't be tracing the error either.

Due to the complaints from users, this might be a backport candidate.

 net/core/dev_ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index cf999e0..84edf16 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -358,6 +358,9 @@ void dev_load(struct net *net, const char *name)
 	struct net_device *dev;
 	int no_module;
 
+	if (!name[0])
+		return;
+
 	rcu_read_lock();
 	dev = dev_get_by_name_rcu(net, name);
 	rcu_read_unlock();
-- 
1.8.1.2

Re: [PATCH net] core: Don't attempt to load the "" driver.

[Cong Wang]

On Tue, Sep 2, 2014 at 6:48 AM, David Laight <David.Laight@aculab.com> wrote:
> While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
> the kernel shouldn't be tracing the error either.
>

Why don't we reject this empty string? It doesn't look like a valid one.
I assume this is for compatibility?

RE: [PATCH net] core: Don't attempt to load the "" driver.

[David Laight]

> On Tue, Sep 2, 2014 at 6:48 AM, David Laight <David.Laight@aculab.com> wrote:
> > While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
> > the kernel shouldn't be tracing the error either.
> >
> 
> Why don't we reject this empty string? It doesn't look like a valid one.
> I assume this is for compatibility?

The ioctl code will error it later on - the module load is 'speculative'.
Analysing whether all the ioctls need dev_load() to succeed is another issue.

Indeed I'm not sure anything stops the module being unloaded before the
ioctl action tries to take a real reference on the interface.

Whether request_module("") should be an error is a different question,
probably much harder to analyse.

	David

Re: [PATCH net] core: Don't attempt to load the "" driver.

[Cong Wang]

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

On Wed, Sep 3, 2014 at 2:02 AM, David Laight <David.Laight@aculab.com> wrote:
>> On Tue, Sep 2, 2014 at 6:48 AM, David Laight <David.Laight@aculab.com> wrote:
>> > While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
>> > the kernel shouldn't be tracing the error either.
>> >
>>
>> Why don't we reject this empty string? It doesn't look like a valid one.
>> I assume this is for compatibility?
>
> The ioctl code will error it later on - the module load is 'speculative'.
> Analysing whether all the ioctls need dev_load() to succeed is another issue.
>
> Indeed I'm not sure anything stops the module being unloaded before the
> ioctl action tries to take a real reference on the interface.
>
> Whether request_module("") should be an error is a different question,
> probably much harder to analyse.
>

If an empty string is an invalid name, we definitely should reject it from
the very beginning, so that you would not need to worry about the above
issues.

Something like the attached patch.

[-- Attachment #2: dev_load.diff --]
[-- Type: text/plain, Size: 5202 bytes --]

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5be20a7..6a26d2c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3338,7 +3338,7 @@ void netdev_state_change(struct net_device *dev);
 void netdev_notify_peers(struct net_device *dev);
 void netdev_features_change(struct net_device *dev);
 /* Load a device via the kmod */
-void dev_load(struct net *net, const char *name);
+int dev_load(struct net *net, const char *name);
 struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 					struct rtnl_link_stats64 *storage);
 void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index cf999e0..00a60f8 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -353,11 +353,14 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
  *	available in this kernel then it becomes a nop.
  */
 
-void dev_load(struct net *net, const char *name)
+int dev_load(struct net *net, const char *name)
 {
 	struct net_device *dev;
 	int no_module;
 
+	if (!dev_valid_name(name))
+		return -EINVAL;
+
 	rcu_read_lock();
 	dev = dev_get_by_name_rcu(net, name);
 	rcu_read_unlock();
@@ -366,10 +369,14 @@ void dev_load(struct net *net, const char *name)
 	if (no_module && capable(CAP_NET_ADMIN))
 		no_module = request_module("netdev-%s", name);
 	if (no_module && capable(CAP_SYS_MODULE)) {
-		if (!request_module("%s", name))
+		int err = request_module("%s", name);
+		if (!err)
 			pr_warn("Loading kernel module for a network device with CAP_SYS_MODULE (deprecated).  Use CAP_NET_ADMIN and alias netdev-%s instead.\n",
 				name);
+		return err;
 	}
+
+	return no_module;
 }
 EXPORT_SYMBOL(dev_load);
 
@@ -438,7 +445,9 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 	case SIOCGIFMAP:
 	case SIOCGIFINDEX:
 	case SIOCGIFTXQLEN:
-		dev_load(net, ifr.ifr_name);
+		ret = dev_load(net, ifr.ifr_name);
+		if (ret)
+			return ret;
 		rcu_read_lock();
 		ret = dev_ifsioc_locked(net, &ifr, cmd);
 		rcu_read_unlock();
@@ -452,7 +461,9 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 		return ret;
 
 	case SIOCETHTOOL:
-		dev_load(net, ifr.ifr_name);
+		ret = dev_load(net, ifr.ifr_name);
+		if (ret)
+			return ret;
 		rtnl_lock();
 		ret = dev_ethtool(net, &ifr);
 		rtnl_unlock();
@@ -476,7 +487,9 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 	case SIOCSIFNAME:
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
-		dev_load(net, ifr.ifr_name);
+		ret = dev_load(net, ifr.ifr_name);
+		if (ret)
+			return ret;
 		rtnl_lock();
 		ret = dev_ifsioc(net, &ifr, cmd);
 		rtnl_unlock();
@@ -527,7 +540,9 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 		/* fall through */
 	case SIOCBONDSLAVEINFOQUERY:
 	case SIOCBONDINFOQUERY:
-		dev_load(net, ifr.ifr_name);
+		ret = dev_load(net, ifr.ifr_name);
+		if (ret)
+			return ret;
 		rtnl_lock();
 		ret = dev_ifsioc(net, &ifr, cmd);
 		rtnl_unlock();
@@ -550,7 +565,9 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 		    cmd == SIOCGHWTSTAMP ||
 		    (cmd >= SIOCDEVPRIVATE &&
 		     cmd <= SIOCDEVPRIVATE + 15)) {
-			dev_load(net, ifr.ifr_name);
+			ret = dev_load(net, ifr.ifr_name);
+			if (ret)
+				return ret;
 			rtnl_lock();
 			ret = dev_ifsioc(net, &ifr, cmd);
 			rtnl_unlock();
diff --git a/net/decnet/dn_dev.c b/net/decnet/dn_dev.c
index 4400da7..484a5de 100644
--- a/net/decnet/dn_dev.c
+++ b/net/decnet/dn_dev.c
@@ -424,7 +424,9 @@ int dn_dev_ioctl(unsigned int cmd, void __user *arg)
 		return -EFAULT;
 	ifr->ifr_name[IFNAMSIZ-1] = 0;
 
-	dev_load(&init_net, ifr->ifr_name);
+	ret = dev_load(&init_net, ifr->ifr_name);
+	if (ret)
+		return ret;
 
 	switch (cmd) {
 	case SIOCGIFADDR:
diff --git a/net/ieee802154/af_ieee802154.c b/net/ieee802154/af_ieee802154.c
index 29e0de6..6c525a8 100644
--- a/net/ieee802154/af_ieee802154.c
+++ b/net/ieee802154/af_ieee802154.c
@@ -148,7 +148,9 @@ static int ieee802154_dev_ioctl(struct sock *sk, struct ifreq __user *arg,
 
 	ifr.ifr_name[IFNAMSIZ-1] = 0;
 
-	dev_load(sock_net(sk), ifr.ifr_name);
+	ret = dev_load(sock_net(sk), ifr.ifr_name);
+	if (ret)
+		return ret;
 	dev = dev_get_by_name(sock_net(sk), ifr.ifr_name);
 
 	if (!dev)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 214882e..ec48f1e 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -909,7 +909,9 @@ int devinet_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 	if (colon)
 		*colon = 0;
 
-	dev_load(net, ifr.ifr_name);
+	ret = dev_load(net, ifr.ifr_name);
+	if (ret)
+		return ret;
 
 	switch (cmd) {
 	case SIOCGIFADDR:	/* Get interface address */
diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index c8717c1..8fea5fe 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -954,7 +954,9 @@ static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
 	if (ret)
 		return ret;
 
-	dev_load(net, ifr->ifr_name);
+	ret = dev_load(net, ifr->ifr_name);
+	if (ret)
+		return ret;
 	rtnl_lock();
 	ret = wireless_process_ioctl(net, ifr, cmd, info, standard, private);
 	rtnl_unlock();

Re: [PATCH net] core: Don't attempt to load the "" driver.

[Stephen Hemminger]

On Wed, 3 Sep 2014 10:02:26 -0700
Cong Wang <cwang@twopensource.com> wrote:

> On Wed, Sep 3, 2014 at 2:02 AM, David Laight <David.Laight@aculab.com> wrote:
> >> On Tue, Sep 2, 2014 at 6:48 AM, David Laight <David.Laight@aculab.com> wrote:
> >> > While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
> >> > the kernel shouldn't be tracing the error either.
> >> >
> >>
> >> Why don't we reject this empty string? It doesn't look like a valid one.
> >> I assume this is for compatibility?
> >
> > The ioctl code will error it later on - the module load is 'speculative'.
> > Analysing whether all the ioctls need dev_load() to succeed is another issue.
> >
> > Indeed I'm not sure anything stops the module being unloaded before the
> > ioctl action tries to take a real reference on the interface.
> >
> > Whether request_module("") should be an error is a different question,
> > probably much harder to analyse.
> >
> 
> If an empty string is an invalid name, we definitely should reject it from
> the very beginning, so that you would not need to worry about the above
> issues.
> 
> Something like the attached patch.

This will break for many things where the code randomly tries
to load something based on name, but the module is already there.

Re: [PATCH net] core: Don't attempt to load the "" driver.

[David Miller]

From: David Laight <David.Laight@ACULAB.COM>
Date: Tue, 2 Sep 2014 13:48:39 +0000

> Requests like 'ifconfig ""' cause dev_load(net, "") be called, since
> request_module("") return 0 (success) this generates the error message:
> "Loading kernel module for a network device with CAP_SYS_MODULE (deprecated).  Use CAP_NET_ADMIN and alias netdev- instead."
> 
> Since dev_load() doesn't have to work, just ignore such names.

Patch submissions require a signoff.

Please freshly resubmit this with an appropriate signoff, thanks.

[PATCH V2 net] core: Don't attempt to load the "" driver.

[David Laight]

Requests like 'ifconfig ""' cause dev_load(net, "") be called, since
request_module("") return 0 (success) this generates the error message:
"Loading kernel module for a network device with CAP_SYS_MODULE (deprecated).  Use CAP_NET_ADMIN and alias netdev- instead."

Since dev_load() doesn't have to work, just ignore such names.

Signed-off-by: David Laight <david.laight@aculab.com>
---

V2: Added sign off.

If you search for the above error message, you'll find a lot of complaints.
While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
the kernel shouldn't be tracing the error either.

Due to the complaints from users, this might be a backport candidate.

net/core/dev_ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index cf999e0..84edf16 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -358,6 +358,9 @@ void dev_load(struct net *net, const char *name)
 	struct net_device *dev;
 	int no_module;
 
+	if (!name[0])
+		return;
+
 	rcu_read_lock();
 	dev = dev_get_by_name_rcu(net, name);
 	rcu_read_unlock();
-- 
1.8.1.2

Re: [PATCH V2 net] core: Don't attempt to load the "" driver.

[David Miller]

From: David Laight <David.Laight@ACULAB.COM>
Date: Wed, 3 Sep 2014 08:55:21 +0000

> Requests like 'ifconfig ""' cause dev_load(net, "") be called, since
> request_module("") return 0 (success) this generates the error message:
> "Loading kernel module for a network device with CAP_SYS_MODULE (deprecated).  Use CAP_NET_ADMIN and alias netdev- instead."
> 
> Since dev_load() doesn't have to work, just ignore such names.
> 
> Signed-off-by: David Laight <david.laight@aculab.com>
> ---
> 
> V2: Added sign off.
> 
> If you search for the above error message, you'll find a lot of complaints.
> While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
> the kernel shouldn't be tracing the error either.
> 
> Due to the complaints from users, this might be a backport candidate.

I have to say that I don't like this.

Why is request_module("") signalled with success unconditionally?
A module didn't get loaded, so semantically this behavior makes
no sense at all.

I would rather than simply request_module() signal an error in this
case, and then _NOBODY_ has to have all of this duplicated logic to
check for name[0]=='\0' throughout all of the call chains leading to
request_module().

RE: [PATCH V2 net] core: Don't attempt to load the "" driver.

[David Laight]

From: David Miller
> From: David Laight <David.Laight@ACULAB.COM>
> Date: Wed, 3 Sep 2014 08:55:21 +0000
> 
> > Requests like 'ifconfig ""' cause dev_load(net, "") be called, since
> > request_module("") return 0 (success) this generates the error message:
> > "Loading kernel module for a network device with CAP_SYS_MODULE (deprecated).  Use CAP_NET_ADMIN and
> alias netdev- instead."
> >
> > Since dev_load() doesn't have to work, just ignore such names.
> >
> > Signed-off-by: David Laight <david.laight@aculab.com>
> > ---
> >
> > V2: Added sign off.
> >
> > If you search for the above error message, you'll find a lot of complaints.
> > While the applications shouldn't be calling an SIOCxxx ioctl with ifr_name[0] == 0
> > the kernel shouldn't be tracing the error either.
> >
> > Due to the complaints from users, this might be a backport candidate.
> 
> I have to say that I don't like this.
> 
> Why is request_module("") signalled with success unconditionally?
> A module didn't get loaded, so semantically this behavior makes
> no sense at all.
> 
> I would rather than simply request_module() signal an error in this
> case, and then _NOBODY_ has to have all of this duplicated logic to
> check for name[0]=='\0' throughout all of the call chains leading to
> request_module().

Maybe, but the SIOCxxx ioctl code is only doing a speculative module load.
Possibly it should error out the ioctl request earlier if ifr_name is "",
but maybe it is sometimes valid?

In this case you also want to stop the attempt to load "netdev-".
Hmmm... that really sucks...
This whole 'try to load the module' path should be predicated on
'interface not found'.
I presume that the locking rules prohibit the module load attempt
at that time.
Maybe the ioctl code should do a retry if it gets the 'interface not found'
error - instead of assuming that the module needs loading.

At the moment I think that if you run 'ifconfig eth0' it will try to
load "netdev-eth0" and "eth0" (maybe without the unit suffix) before
attempting to do anything else.

I didn't do any 'software archaeology' on the SIOCxxx ioctl code.

	David

Re: [PATCH V2 net] core: Don't attempt to load the "" driver.

[David Miller]

From: David Laight <David.Laight@ACULAB.COM>
Date: Mon, 8 Sep 2014 13:23:33 +0000

> This whole 'try to load the module' path should be predicated on
> 'interface not found'.

And that's the first thing dev_load() does, it tries to look up the
device by name, and only does the module load attempt if that fails.

Nobody cares if the module load itself actually fails, dev_load()
returns void after all.

RE: [PATCH V2 net] core: Don't attempt to load the "" driver.

[David Laight]

From: David Miller
> From: David Laight <David.Laight@ACULAB.COM>
> Date: Mon, 8 Sep 2014 13:23:33 +0000
> 
> > This whole 'try to load the module' path should be predicated on
> > 'interface not found'.
> 
> And that's the first thing dev_load() does, it tries to look up the
> device by name, and only does the module load attempt if that fails.

OK, I'd not looked back at the code before making that comment.

> Nobody cares if the module load itself actually fails, dev_load()
> returns void after all.

The actual code path is typically:
	dev_load(net, ifr.ifr_name);
	rtnl_lock()
	ret = dev_xxx(net, &ifr, cmd);
	rtnl_unlock()

where the first thing dev_xxx() does is to call __dev_get_by_name()
(or the equivalent dev_get_by_name_rcu()) again.
So the code 'sort of' cares whether the module loaded.

I actually wonder when the module load is useful - must be useful sometimes,
otherwise it wouldn't have been added.

I thought ifr.ifr_name was likely to be (say) "eth2" - but the module
won't be called that.
Looks like some hack to get something like the 'bond' driver loaded,
but even then surely ifr.ifr_name would be "bond0" not "bond".

I've had a quick look with 'git blame', but dev_ioctl.c was split from
somewhere and doesn't contain the history.

	David.

Re: [PATCH V2 net] core: Don't attempt to load the "" driver.

[David Miller]

From: David Laight <David.Laight@ACULAB.COM>
Date: Wed, 10 Sep 2014 08:50:50 +0000

> I actually wonder when the module load is useful - must be useful
> sometimes, otherwise it wouldn't have been added.

Just grep for things like MODULE_ALIAS_NETDEV(...) for the legacy cases
where this can still be used.  Largely it's for tunneling devices.