| Message ID | CAHA+R7OoBp=ZAJeg_hXRjNhT6_n2siVQidJK6hx8EtHW7iRwqA@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Stephen Hemminger > 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. Or the interface isn't generated buy a module etc. Also ISTR seeing dev_load() being used as an initialiser for something. So it isn't easy to change the prototype - otherwise I'd have suggested tracing the failing ioctl cmd to make it easier to determine where the invalid request was coming from. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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();