* [Patch net-next 1/3] ipv4: fail early when creating netdev named all or default
2014-07-23 22:17 [Patch net-next 0/3] net: forbid net devices named "all" "default" or "config" Cong Wang
@ 2014-07-23 22:17 ` Cong Wang
2014-07-24 1:05 ` Florian Fainelli
2014-07-23 22:17 ` [Patch net-next 2/3] ipv6: " Cong Wang
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2014-07-23 22:17 UTC (permalink / raw)
To: netdev; +Cc: stephane.chazelas, davem, Cong Wang
We create a proc dir for each network device, this will cause
conflicts when the devices have name "all" or "default".
Rather than emitting an ugly kernel warning, we could just
fail earlier by checking the device name.
Reported-by: Stephane Chazelas <stephane.chazelas@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/ipv4/devinet.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e944937..fbd8b04 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -180,11 +180,12 @@ static BLOCKING_NOTIFIER_HEAD(inetaddr_chain);
static void inet_del_ifa(struct in_device *in_dev, struct in_ifaddr **ifap,
int destroy);
#ifdef CONFIG_SYSCTL
-static void devinet_sysctl_register(struct in_device *idev);
+static int devinet_sysctl_register(struct in_device *idev);
static void devinet_sysctl_unregister(struct in_device *idev);
#else
-static void devinet_sysctl_register(struct in_device *idev)
+static int devinet_sysctl_register(struct in_device *idev)
{
+ return 0;
}
static void devinet_sysctl_unregister(struct in_device *idev)
{
@@ -232,6 +233,7 @@ EXPORT_SYMBOL(in_dev_finish_destroy);
static struct in_device *inetdev_init(struct net_device *dev)
{
struct in_device *in_dev;
+ int err = -ENOMEM;
ASSERT_RTNL();
@@ -252,7 +254,13 @@ static struct in_device *inetdev_init(struct net_device *dev)
/* Account for reference dev->ip_ptr (below) */
in_dev_hold(in_dev);
- devinet_sysctl_register(in_dev);
+ err = devinet_sysctl_register(in_dev);
+ if (err) {
+ in_dev->dead = 1;
+ in_dev_put(in_dev);
+ in_dev = NULL;
+ goto out;
+ }
ip_mc_init_dev(in_dev);
if (dev->flags & IFF_UP)
ip_mc_up(in_dev);
@@ -260,7 +268,7 @@ static struct in_device *inetdev_init(struct net_device *dev)
/* we can receive as soon as ip_ptr is set -- do this last */
rcu_assign_pointer(dev->ip_ptr, in_dev);
out:
- return in_dev;
+ return in_dev ?: ERR_PTR(err);
out_kfree:
kfree(in_dev);
in_dev = NULL;
@@ -1347,8 +1355,8 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
if (!in_dev) {
if (event == NETDEV_REGISTER) {
in_dev = inetdev_init(dev);
- if (!in_dev)
- return notifier_from_errno(-ENOMEM);
+ if (IS_ERR(in_dev))
+ return notifier_from_errno(PTR_ERR(in_dev));
if (dev->flags & IFF_LOOPBACK) {
IN_DEV_CONF_SET(in_dev, NOXFRM, 1);
IN_DEV_CONF_SET(in_dev, NOPOLICY, 1);
@@ -2182,11 +2190,22 @@ static void __devinet_sysctl_unregister(struct ipv4_devconf *cnf)
kfree(t);
}
-static void devinet_sysctl_register(struct in_device *idev)
+static int devinet_sysctl_register(struct in_device *idev)
{
- neigh_sysctl_register(idev->dev, idev->arp_parms, NULL);
- __devinet_sysctl_register(dev_net(idev->dev), idev->dev->name,
+ int err;
+
+ if (!strcmp(idev->dev->name, "default") ||
+ !strcmp(idev->dev->name, "all"))
+ return -EINVAL;
+
+ err = neigh_sysctl_register(idev->dev, idev->arp_parms, NULL);
+ if (err)
+ return err;
+ err = __devinet_sysctl_register(dev_net(idev->dev), idev->dev->name,
&idev->cnf);
+ if (err)
+ neigh_sysctl_unregister(idev->arp_parms);
+ return err;
}
static void devinet_sysctl_unregister(struct in_device *idev)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Patch net-next 1/3] ipv4: fail early when creating netdev named all or default
2014-07-23 22:17 ` [Patch net-next 1/3] ipv4: fail early when creating netdev named all or default Cong Wang
@ 2014-07-24 1:05 ` Florian Fainelli
2014-07-24 3:00 ` Andy Gospodarek
0 siblings, 1 reply; 8+ messages in thread
From: Florian Fainelli @ 2014-07-24 1:05 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, stephane.chazelas, David Miller
2014-07-23 15:17 GMT-07:00 Cong Wang <xiyou.wangcong@gmail.com>:
> We create a proc dir for each network device, this will cause
> conflicts when the devices have name "all" or "default".
>
> Rather than emitting an ugly kernel warning, we could just
> fail earlier by checking the device name.
>
> Reported-by: Stephane Chazelas <stephane.chazelas@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
[snip]
>
> -static void devinet_sysctl_register(struct in_device *idev)
> +static int devinet_sysctl_register(struct in_device *idev)
> {
> - neigh_sysctl_register(idev->dev, idev->arp_parms, NULL);
> - __devinet_sysctl_register(dev_net(idev->dev), idev->dev->name,
> + int err;
> +
> + if (!strcmp(idev->dev->name, "default") ||
> + !strcmp(idev->dev->name, "all"))
> + return -EINVAL;
Since this exact same check is done in the ipv6 counterpart, how about
moving this to a helper function like: sysctl_dev_name_is_allowed()
such that you centralize where the naming policy is enforced? In case
we need to blacklist some other interface names, there should be one
place to update.
> +
> + err = neigh_sysctl_register(idev->dev, idev->arp_parms, NULL);
> + if (err)
> + return err;
> + err = __devinet_sysctl_register(dev_net(idev->dev), idev->dev->name,
> &idev->cnf);
> + if (err)
> + neigh_sysctl_unregister(idev->arp_parms);
> + return err;
> }
>
> static void devinet_sysctl_unregister(struct in_device *idev)
> --
> 1.8.3.1
>
> --
> 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
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch net-next 1/3] ipv4: fail early when creating netdev named all or default
2014-07-24 1:05 ` Florian Fainelli
@ 2014-07-24 3:00 ` Andy Gospodarek
2014-07-24 17:41 ` Cong Wang
0 siblings, 1 reply; 8+ messages in thread
From: Andy Gospodarek @ 2014-07-24 3:00 UTC (permalink / raw)
To: Cong Wang, Florian Fainelli; +Cc: netdev, stephane.chazelas, David Miller
On Wed, Jul 23, 2014 at 06:05:07PM -0700, Florian Fainelli wrote:
> 2014-07-23 15:17 GMT-07:00 Cong Wang <xiyou.wangcong@gmail.com>:
> > We create a proc dir for each network device, this will cause
> > conflicts when the devices have name "all" or "default".
> >
> > Rather than emitting an ugly kernel warning, we could just
> > fail earlier by checking the device name.
> >
> > Reported-by: Stephane Chazelas <stephane.chazelas@gmail.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
>
> [snip]
>
> >
> > -static void devinet_sysctl_register(struct in_device *idev)
> > +static int devinet_sysctl_register(struct in_device *idev)
> > {
> > - neigh_sysctl_register(idev->dev, idev->arp_parms, NULL);
> > - __devinet_sysctl_register(dev_net(idev->dev), idev->dev->name,
> > + int err;
> > +
> > + if (!strcmp(idev->dev->name, "default") ||
> > + !strcmp(idev->dev->name, "all"))
> > + return -EINVAL;
>
> Since this exact same check is done in the ipv6 counterpart, how about
> moving this to a helper function like: sysctl_dev_name_is_allowed()
> such that you centralize where the naming policy is enforced? In case
> we need to blacklist some other interface names, there should be one
> place to update.
Agree that a common function would be useful.
There also might be cases where one would want to impose more limits
than just what is limited based on /proc, so this may be something to consider
in core networking code.
>
> > +
> > + err = neigh_sysctl_register(idev->dev, idev->arp_parms, NULL);
> > + if (err)
> > + return err;
> > + err = __devinet_sysctl_register(dev_net(idev->dev), idev->dev->name,
> > &idev->cnf);
> > + if (err)
> > + neigh_sysctl_unregister(idev->arp_parms);
> > + return err;
> > }
> >
> > static void devinet_sysctl_unregister(struct in_device *idev)
> > --
> > 1.8.3.1
> >
> > --
> > 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
>
>
>
> --
> Florian
> --
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch net-next 1/3] ipv4: fail early when creating netdev named all or default
2014-07-24 3:00 ` Andy Gospodarek
@ 2014-07-24 17:41 ` Cong Wang
0 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2014-07-24 17:41 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: Florian Fainelli, netdev, Stephane Chazelas, David Miller
On Wed, Jul 23, 2014 at 8:00 PM, Andy Gospodarek
<gospo@cumulusnetworks.com> wrote:
> On Wed, Jul 23, 2014 at 06:05:07PM -0700, Florian Fainelli wrote:
>>
>> Since this exact same check is done in the ipv6 counterpart, how about
>> moving this to a helper function like: sysctl_dev_name_is_allowed()
>> such that you centralize where the naming policy is enforced? In case
>> we need to blacklist some other interface names, there should be one
>> place to update.
>
> Agree that a common function would be useful.
>
> There also might be cases where one would want to impose more limits
> than just what is limited based on /proc, so this may be something to consider
> in core networking code.
I agree generally, just that I think each protocol should be free to define
its own limit on names, not just "all" and "default".
Anyway, I will wait for other feedback, especially from David,
and update the patch.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Patch net-next 2/3] ipv6: fail early when creating netdev named all or default
2014-07-23 22:17 [Patch net-next 0/3] net: forbid net devices named "all" "default" or "config" Cong Wang
2014-07-23 22:17 ` [Patch net-next 1/3] ipv4: fail early when creating netdev named all or default Cong Wang
@ 2014-07-23 22:17 ` Cong Wang
2014-07-23 22:17 ` [Patch net-next 3/3] vlan: fail early when creating netdev named config Cong Wang
2014-07-25 6:32 ` [Patch net-next 0/3] net: forbid net devices named "all" "default" or "config" David Miller
3 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2014-07-23 22:17 UTC (permalink / raw)
To: netdev; +Cc: stephane.chazelas, davem, Cong Wang
Same for ipv6.
Reported-by: Stephane Chazelas <stephane.chazelas@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/ipv6/addrconf.c | 67 +++++++++++++++++++++++++++++++++++------------------
1 file changed, 44 insertions(+), 23 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 4c03c28..cd9cfd3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -108,11 +108,12 @@ static inline u32 cstamp_delta(unsigned long cstamp)
}
#ifdef CONFIG_SYSCTL
-static void addrconf_sysctl_register(struct inet6_dev *idev);
+static int addrconf_sysctl_register(struct inet6_dev *idev);
static void addrconf_sysctl_unregister(struct inet6_dev *idev);
#else
-static inline void addrconf_sysctl_register(struct inet6_dev *idev)
+static inline int addrconf_sysctl_register(struct inet6_dev *idev)
{
+ return 0;
}
static inline void addrconf_sysctl_unregister(struct inet6_dev *idev)
@@ -310,16 +311,16 @@ err_ip:
static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
{
struct inet6_dev *ndev;
+ int err = -ENOMEM;
ASSERT_RTNL();
if (dev->mtu < IPV6_MIN_MTU)
- return NULL;
+ return ERR_PTR(-EINVAL);
ndev = kzalloc(sizeof(struct inet6_dev), GFP_KERNEL);
-
if (ndev == NULL)
- return NULL;
+ return ERR_PTR(err);
rwlock_init(&ndev->lock);
ndev->dev = dev;
@@ -332,7 +333,7 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
ndev->nd_parms = neigh_parms_alloc(dev, &nd_tbl);
if (ndev->nd_parms == NULL) {
kfree(ndev);
- return NULL;
+ return ERR_PTR(err);
}
if (ndev->cnf.forwarding)
dev_disable_lro(dev);
@@ -346,17 +347,14 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
neigh_parms_release(&nd_tbl, ndev->nd_parms);
dev_put(dev);
kfree(ndev);
- return NULL;
+ return ERR_PTR(err);
}
if (snmp6_register_dev(ndev) < 0) {
ADBG(KERN_WARNING
"%s: cannot create /proc/net/dev_snmp6/%s\n",
__func__, dev->name);
- neigh_parms_release(&nd_tbl, ndev->nd_parms);
- ndev->dead = 1;
- in6_dev_finish_destroy(ndev);
- return NULL;
+ goto err_release;
}
/* One reference from device. We must do this before
@@ -394,7 +392,12 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
ipv6_mc_init_dev(ndev);
ndev->tstamp = jiffies;
- addrconf_sysctl_register(ndev);
+ err = addrconf_sysctl_register(ndev);
+ if (err) {
+ ipv6_mc_destroy_dev(ndev);
+ del_timer(&ndev->regen_timer);
+ goto err_release;
+ }
/* protected by rtnl_lock */
rcu_assign_pointer(dev->ip6_ptr, ndev);
@@ -409,6 +412,12 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
return ndev;
+
+err_release:
+ neigh_parms_release(&nd_tbl, ndev->nd_parms);
+ ndev->dead = 1;
+ in6_dev_finish_destroy(ndev);
+ return ERR_PTR(err);
}
static struct inet6_dev *ipv6_find_idev(struct net_device *dev)
@@ -420,7 +429,7 @@ static struct inet6_dev *ipv6_find_idev(struct net_device *dev)
idev = __in6_dev_get(dev);
if (!idev) {
idev = ipv6_add_dev(dev);
- if (!idev)
+ if (IS_ERR(idev))
return NULL;
}
@@ -2830,8 +2839,8 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
case NETDEV_REGISTER:
if (!idev && dev->mtu >= IPV6_MIN_MTU) {
idev = ipv6_add_dev(dev);
- if (!idev)
- return notifier_from_errno(-ENOMEM);
+ if (IS_ERR(idev))
+ return notifier_from_errno(PTR_ERR(idev));
}
break;
@@ -2851,7 +2860,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
if (!idev && dev->mtu >= IPV6_MIN_MTU)
idev = ipv6_add_dev(dev);
- if (idev) {
+ if (!IS_ERR_OR_NULL(idev)) {
idev->if_flags |= IF_READY;
run_pending = 1;
}
@@ -2894,7 +2903,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
break;
}
- if (idev) {
+ if (!IS_ERR_OR_NULL(idev)) {
if (run_pending)
addrconf_dad_run(idev);
@@ -2929,7 +2938,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
if (!idev && dev->mtu >= IPV6_MIN_MTU) {
idev = ipv6_add_dev(dev);
- if (idev)
+ if (!IS_ERR(idev))
break;
}
@@ -5248,12 +5257,24 @@ static void __addrconf_sysctl_unregister(struct ipv6_devconf *p)
kfree(t);
}
-static void addrconf_sysctl_register(struct inet6_dev *idev)
+static int addrconf_sysctl_register(struct inet6_dev *idev)
{
- neigh_sysctl_register(idev->dev, idev->nd_parms,
- &ndisc_ifinfo_sysctl_change);
- __addrconf_sysctl_register(dev_net(idev->dev), idev->dev->name,
- idev, &idev->cnf);
+ int err;
+
+ if (!strcmp(idev->dev->name, "default") ||
+ !strcmp(idev->dev->name, "all"))
+ return -EINVAL;
+
+ err = neigh_sysctl_register(idev->dev, idev->nd_parms,
+ &ndisc_ifinfo_sysctl_change);
+ if (err)
+ return err;
+ err = __addrconf_sysctl_register(dev_net(idev->dev), idev->dev->name,
+ idev, &idev->cnf);
+ if (err)
+ neigh_sysctl_unregister(idev->nd_parms);
+
+ return err;
}
static void addrconf_sysctl_unregister(struct inet6_dev *idev)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Patch net-next 3/3] vlan: fail early when creating netdev named config
2014-07-23 22:17 [Patch net-next 0/3] net: forbid net devices named "all" "default" or "config" Cong Wang
2014-07-23 22:17 ` [Patch net-next 1/3] ipv4: fail early when creating netdev named all or default Cong Wang
2014-07-23 22:17 ` [Patch net-next 2/3] ipv6: " Cong Wang
@ 2014-07-23 22:17 ` Cong Wang
2014-07-25 6:32 ` [Patch net-next 0/3] net: forbid net devices named "all" "default" or "config" David Miller
3 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2014-07-23 22:17 UTC (permalink / raw)
To: netdev; +Cc: stephane.chazelas, davem, Cong Wang
Similarly, vlan will create /proc/net/vlan/<dev>, so when we
create dev with name "config", it will confict with
/proc/net/vlan/config.
Reported-by: Stephane Chazelas <stephane.chazelas@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/8021q/vlan.c | 21 +++++++++++++--------
net/8021q/vlanproc.c | 2 ++
2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index cba9c21..64c6bed 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -325,23 +325,24 @@ static void vlan_transfer_features(struct net_device *dev,
netdev_update_features(vlandev);
}
-static void __vlan_device_event(struct net_device *dev, unsigned long event)
+static int __vlan_device_event(struct net_device *dev, unsigned long event)
{
+ int err = 0;
+
switch (event) {
case NETDEV_CHANGENAME:
vlan_proc_rem_dev(dev);
- if (vlan_proc_add_dev(dev) < 0)
- pr_warn("failed to change proc name for %s\n",
- dev->name);
+ err = vlan_proc_add_dev(dev);
break;
case NETDEV_REGISTER:
- if (vlan_proc_add_dev(dev) < 0)
- pr_warn("failed to add proc entry for %s\n", dev->name);
+ err = vlan_proc_add_dev(dev);
break;
case NETDEV_UNREGISTER:
vlan_proc_rem_dev(dev);
break;
}
+
+ return err;
}
static int vlan_device_event(struct notifier_block *unused, unsigned long event,
@@ -356,8 +357,12 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
bool last = false;
LIST_HEAD(list);
- if (is_vlan_dev(dev))
- __vlan_device_event(dev, event);
+ if (is_vlan_dev(dev)) {
+ int err = __vlan_device_event(dev, event);
+
+ if (err)
+ return notifier_from_errno(err);
+ }
if ((event == NETDEV_UP) &&
(dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)) {
diff --git a/net/8021q/vlanproc.c b/net/8021q/vlanproc.c
index 1d0e8921..ae63cf7 100644
--- a/net/8021q/vlanproc.c
+++ b/net/8021q/vlanproc.c
@@ -171,6 +171,8 @@ int vlan_proc_add_dev(struct net_device *vlandev)
struct vlan_dev_priv *vlan = vlan_dev_priv(vlandev);
struct vlan_net *vn = net_generic(dev_net(vlandev), vlan_net_id);
+ if (!strcmp(vlandev->name, name_conf))
+ return -EINVAL;
vlan->dent =
proc_create_data(vlandev->name, S_IFREG|S_IRUSR|S_IWUSR,
vn->proc_vlan_dir, &vlandev_fops, vlandev);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Patch net-next 0/3] net: forbid net devices named "all" "default" or "config"
2014-07-23 22:17 [Patch net-next 0/3] net: forbid net devices named "all" "default" or "config" Cong Wang
` (2 preceding siblings ...)
2014-07-23 22:17 ` [Patch net-next 3/3] vlan: fail early when creating netdev named config Cong Wang
@ 2014-07-25 6:32 ` David Miller
3 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2014-07-25 6:32 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, stephane.chazelas
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 23 Jul 2014 15:17:29 -0700
> /proc/sys/net/ipv[46]/conf/<dev> could conflict with
> /proc/sys/net/ipv[46]/conf/(all|default). And /proc/net/vlan/<dev>
> could conflict with /proc/net/vlan/config. Besides kernel warnings,
> undefined behavior such as duplicated proc files also appears, therefore
> we should forbid these names.
>
> Cong Wang (3):
> ipv4: fail early when creating netdev named all or default
> ipv6: fail early when creating netdev named all or default
> vlan: fail early when creating netdev named config
As suggested, please make a common helper for ipv4/ipv6, the restrictions
are identical currently.
^ permalink raw reply [flat|nested] 8+ messages in thread