[PATCH 0/3] dev: Fix network device ifindex allocation

[PATCH 0/3] dev: Fix network device ifindex allocation

[Serhey Popovych]

There are some problems with network device ifindex handling in
the core and veth driver.

* Network device index (ifindex) is signed int, but only values > 0
  consodered to be valid. However it is possible for userspace
  to supply negative ifindex value using following command:

    # ip link add dev dummy2 index -100 up type dummy
    # ip link sh dev dummy2
    Device "dummy2" does not exist.
    # ip li sh |tail -n2
    -100: dummy2: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue \
    state UNKNOWN mode DEFAULT group default
        link/ether 7e:77:1b:49:50:de brd ff:ff:ff:ff:ff:ff

* It is possible to hit infinite loop when number of ifindex in
  some network namespace get exchausted.

	# There should be plenty of RAM to create INT_MAX netdev.
	# Hitting OOM is much likely with such huge number, but anyway.
	for ((i = 1; i <= 0x7fffffff; i++)); do
		ifname="dummy$i";
		[ ! -d "/sys/class/net/$ifname" ] || continue
		ip link add dev "$ifname" index "$i" type dummy
	done

* (minor) check for dev->ifindex being greater than zero.

Patches in this series tries to address these problems.

Serhey Popovych (3):
  dev: Prevent creating network devices with negative ifindex
  dev: Avoid infinite loop on network device index exhaustion
  veth: Set ifindex only if given and not negative

 drivers/net/veth.c |  2 +-
 net/core/dev.c     | 30 ++++++++++++++++++++++--------
 2 files changed, 23 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] dev: Prevent creating network devices with negative ifindex

[Serhey Popovych]

Interface index is signed integer, we can pass ifm->ifi_index
from userspace via netlink and create network device with
negative ifindex value.

Fixes: 9c7dafbfab15 ("net: Allow to create links with given ifindex")
Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8658074..dae8010 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7491,7 +7491,7 @@ int register_netdevice(struct net_device *dev)
 	}
 
 	ret = -EBUSY;
-	if (!dev->ifindex)
+	if (dev->ifindex <= 0)
 		dev->ifindex = dev_new_index(net);
 	else if (__dev_get_by_index(net, dev->ifindex))
 		goto err_uninit;
-- 
1.8.3.1

Re: [PATCH 1/3] dev: Prevent creating network devices with negative ifindex

[Stephen Hemminger]

On Fri, 16 Jun 2017 17:23:51 +0300
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> Interface index is signed integer, we can pass ifm->ifi_index
> from userspace via netlink and create network device with
> negative ifindex value.
> 
> Fixes: 9c7dafbfab15 ("net: Allow to create links with given ifindex")
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
> ---
>  net/core/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 8658074..dae8010 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7491,7 +7491,7 @@ int register_netdevice(struct net_device *dev)
>  	}
>  
>  	ret = -EBUSY;
> -	if (!dev->ifindex)
> +	if (dev->ifindex <= 0)
>  		dev->ifindex = dev_new_index(net);
>  	else if (__dev_get_by_index(net, dev->ifindex))
>  		goto err_uninit;

You should fix this by adding error check in the netlink portion
that allows creating devices with given ifindex. Passing < 0
should be an error.

Re: [PATCH 1/3] dev: Prevent creating network devices with negative ifindex

[Serhey Popovych]

> On Fri, 16 Jun 2017 17:23:51 +0300
> Serhey Popovych <serhe.popovych@gmail.com> wrote:
> 
>> Interface index is signed integer, we can pass ifm->ifi_index
>> from userspace via netlink and create network device with
>> negative ifindex value.
>>
>> Fixes: 9c7dafbfab15 ("net: Allow to create links with given ifindex")
>> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
>> ---
>>  net/core/dev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 8658074..dae8010 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -7491,7 +7491,7 @@ int register_netdevice(struct net_device *dev)
>>  	}
>>  
>>  	ret = -EBUSY;
>> -	if (!dev->ifindex)
>> +	if (dev->ifindex <= 0)
>>  		dev->ifindex = dev_new_index(net);
>>  	else if (__dev_get_by_index(net, dev->ifindex))
>>  		goto err_uninit;
> 
> You should fix this by adding error check in the netlink portion
> that allows creating devices with given ifindex. Passing < 0
> should be an error.But should this break some setups if I add such check to netlink
portion? In my opinion it is better to choose silently different
ifindex rather than reporting failure. That's why I prefer doing
this in register_netdevice().

Also there is similar problem for drivers/net/veth.c, it might
happen that other places will be added later that setup
dev->ifindex and then call register_netdevice().

What do you think?

Thanks for quick review!

> 

-- 
Thanks, Serhey

Re: [PATCH 1/3] dev: Prevent creating network devices with negative ifindex

[Stephen Hemminger]

On Fri, 16 Jun 2017 19:44:45 +0300
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> > On Fri, 16 Jun 2017 17:23:51 +0300
> > Serhey Popovych <serhe.popovych@gmail.com> wrote:
> >   
> >> Interface index is signed integer, we can pass ifm->ifi_index
> >> from userspace via netlink and create network device with
> >> negative ifindex value.
> >>
> >> Fixes: 9c7dafbfab15 ("net: Allow to create links with given ifindex")
> >> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
> >> ---
> >>  net/core/dev.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 8658074..dae8010 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -7491,7 +7491,7 @@ int register_netdevice(struct net_device *dev)
> >>  	}
> >>  
> >>  	ret = -EBUSY;
> >> -	if (!dev->ifindex)
> >> +	if (dev->ifindex <= 0)
> >>  		dev->ifindex = dev_new_index(net);
> >>  	else if (__dev_get_by_index(net, dev->ifindex))
> >>  		goto err_uninit;  
> > 
> > You should fix this by adding error check in the netlink portion
> > that allows creating devices with given ifindex. Passing < 0
> > should be an error.But should this break some setups if I add such check to netlink  
> portion? In my opinion it is better to choose silently different
> ifindex rather than reporting failure. That's why I prefer doing
> this in register_netdevice().
> 
> Also there is similar problem for drivers/net/veth.c, it might
> happen that other places will be added later that setup
> dev->ifindex and then call register_netdevice().
> 
> What do you think?

Passing -1 is an error, it doesn't make sense  to try and be
helpful to buggy userland.

Re: [PATCH 1/3] dev: Prevent creating network devices with negative ifindex

[Serhey Popovych]

>> What do you think?
>
> Passing -1 is an error, it doesn't make sense  to try and be
> helpful to buggy userland.

Here is commit I actually change/fix:

commit 9c7dafbfab15 ("net: Allow to create links with given ifindex")

In this change done the opposite: check for ifm->ifi_index moved to
register_netdevice() from rtnl_newlink().

Let me understand finally: why do I need to reverse and move check
for dev->ifindex from register_netdevice() to rtnl_newlink()? That
will partially revert commit I note above.

-- 
Thanks,  Serhey

[PATCH 2/3] dev: Avoid infinite loop on network device index exhaustion

[Serhey Popovych]

If network device indexes exhaust in namespace dev_new_index()
can loop indefinitely since there is no condition to exit
except case where free index is found.

Since all it's caller hold RTNL mutex this may completely
lock down network subsystem configuration operations.

Instead of retrying with ifindex == 1 (LOOPBACK_IFINDEX)
in dev_new_index() we should fail and return invalid
index value (0).

Adjust callers to correctly handle error case of dev_new_index().

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 net/core/dev.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index dae8010..6f573f7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7014,7 +7014,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
  *	@net: the applicable net namespace
  *
  *	Returns a suitable unique value for a new device interface
- *	number.  The caller must hold the rtnl semaphore or the
+ *	number or zero in case of no free indexes in net namespace.
+ *
+ *	The caller must hold the rtnl mutex or the
  *	dev_base_lock to be sure it remains unique.
  */
 static int dev_new_index(struct net *net)
@@ -7023,7 +7025,7 @@ static int dev_new_index(struct net *net)
 
 	for (;;) {
 		if (++ifindex <= 0)
-			ifindex = 1;
+			return 0;
 		if (!__dev_get_by_index(net, ifindex))
 			return net->ifindex = ifindex;
 	}
@@ -7491,9 +7493,12 @@ int register_netdevice(struct net_device *dev)
 	}
 
 	ret = -EBUSY;
-	if (dev->ifindex <= 0)
-		dev->ifindex = dev_new_index(net);
-	else if (__dev_get_by_index(net, dev->ifindex))
+	if (dev->ifindex <= 0) {
+		int ifindex = dev_new_index(net);
+		if (!ifindex)
+			goto err_uninit;
+		dev->ifindex = ifindex;
+	} else if (__dev_get_by_index(net, dev->ifindex))
 		goto err_uninit;
 
 	/* Transfer changeable features to wanted_features and enable
@@ -8174,6 +8179,7 @@ void unregister_netdev(struct net_device *dev)
 
 int dev_change_net_namespace(struct net_device *dev, struct net *net, const char *pat)
 {
+	int ifindex = 0;
 	int err;
 
 	ASSERT_RTNL();
@@ -8192,6 +8198,14 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	if (net_eq(dev_net(dev), net))
 		goto out;
 
+	/* If there is an ifindex conflict assign a new one */
+	err = -EBUSY;
+	if (__dev_get_by_index(net, dev->ifindex)) {
+		ifindex = dev_new_index(net);
+		if (!ifindex)
+			goto out;
+	}
+
 	/* Pick the destination device name, and ensure
 	 * we can use it in the destination network namespace.
 	 */
@@ -8245,9 +8259,9 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	/* Actually switch the network namespace */
 	dev_net_set(dev, net);
 
-	/* If there is an ifindex conflict assign a new one */
-	if (__dev_get_by_index(net, dev->ifindex))
-		dev->ifindex = dev_new_index(net);
+	/* Actually change the ifindex */
+	if (ifindex)
+		dev->ifindex = ifindex;
 
 	/* Send a netdev-add uevent to the new namespace */
 	kobject_uevent(&dev->dev.kobj, KOBJ_ADD);
-- 
1.8.3.1

Re: [PATCH 2/3] dev: Avoid infinite loop on network device index exhaustion

[Stephen Hemminger]

On Fri, 16 Jun 2017 17:23:52 +0300
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> If network device indexes exhaust in namespace dev_new_index()
> can loop indefinitely since there is no condition to exit
> except case where free index is found.
> 
> Since all it's caller hold RTNL mutex this may completely
> lock down network subsystem configuration operations.
> 
> Instead of retrying with ifindex == 1 (LOOPBACK_IFINDEX)
> in dev_new_index() we should fail and return invalid
> index value (0).
> 
> Adjust callers to correctly handle error case of dev_new_index().
> 
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
 
This breaks existing semantics.

Today on Linux the ifindex allocator intentionally wraps around back to 1.
This is to handle the case of long running system with things like VPN's
that create and destroy lots of devices.

Re: [PATCH 2/3] dev: Avoid infinite loop on network device index exhaustion

[Serhey Popovych]

> On Fri, 16 Jun 2017 17:23:52 +0300
> Serhey Popovych <serhe.popovych@gmail.com> wrote:
> 
>> If network device indexes exhaust in namespace dev_new_index()
>> can loop indefinitely since there is no condition to exit
>> except case where free index is found.
>>
>> Since all it's caller hold RTNL mutex this may completely
>> lock down network subsystem configuration operations.
>>
>> Instead of retrying with ifindex == 1 (LOOPBACK_IFINDEX)
>> in dev_new_index() we should fail and return invalid
>> index value (0).
>>
>> Adjust callers to correctly handle error case of dev_new_index().
>>
>> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
>  
> This breaks existing semantics.
> 
> Today on Linux the ifindex allocator intentionally wraps around back to 1.
> This is to handle the case of long running system with things like VPN's
> that create and destroy lots of devices.
> 
Ok, got it. Maybe we can change allocation mechanism?

That what actually I did.
What do you think?

I will show POC patch doing this.

-- 
Thanks, Serhey

dev: Reclaim network device indexes

[Serhey Popovych]

While making dev_new_index() return zero on overrun prevents
from infinite loop, there is no way to recovery mechanisms
since namespace ifindex only increases and never reused
from released network devices.

To address this we introduce dev_free_index() helper which
is used to reclaim released network device index when it is
smaller than last allocated index in namespace.

This also has positive side effect for equal distribution
of network devices per buckets in index hash table. That
positively affects performance of dev_get_by_index() family.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 net/core/dev.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6f573f7..e1714e7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7031,6 +7031,24 @@ static int dev_new_index(struct net *net)
 	}
 }
 
+/**
+ *	dev_free_index	-	free an ifindex
+ *	@dev: the network device whose index to free
+ *
+ *	Sets network namespace last allocated index to the value
+ *	proceed to the ifindex we want to free.  The caller
+ *	must hold the rtnl semaphore or the dev_base_lock to be
+ *	sure it remains unique.
+ */
+static void dev_free_index(struct net_device *dev)
+{
+	struct net *net = dev_net(dev);
+	int ifindex = dev->ifindex;
+
+	if (--ifindex < net->ifindex)
+		net->ifindex = ifindex;
+}
+
 /* Delayed registration/unregisteration */
 static LIST_HEAD(net_todo_list);
 DECLARE_WAIT_QUEUE_HEAD(netdev_unregistering_wq);
@@ -7454,8 +7472,9 @@ void netif_tx_stop_all_queues(struct net_device *dev)
 
 int register_netdevice(struct net_device *dev)
 {
-	int ret;
 	struct net *net = dev_net(dev);
+	int ifindex = 0;
+	int ret;
 
 	BUG_ON(dev_boot_phase);
 	ASSERT_RTNL();
@@ -7494,7 +7513,7 @@ int register_netdevice(struct net_device *dev)
 
 	ret = -EBUSY;
 	if (dev->ifindex <= 0) {
-		int ifindex = dev_new_index(net);
+		ifindex = dev_new_index(net);
 		if (!ifindex)
 			goto err_uninit;
 		dev->ifindex = ifindex;
@@ -7573,10 +7592,9 @@ int register_netdevice(struct net_device *dev)
 	/* Notify protocols, that a new device appeared. */
 	ret = call_netdevice_notifiers(NETDEV_REGISTER, dev);
 	ret = notifier_to_errno(ret);
-	if (ret) {
-		rollback_registered(dev);
-		dev->reg_state = NETREG_UNREGISTERED;
-	}
+	if (ret)
+		goto err_rollback;
+
 	/*
 	 *	Prevent userspace races by waiting until the network
 	 *	device is fully setup before sending notifications.
@@ -7593,6 +7611,14 @@ int register_netdevice(struct net_device *dev)
 		dev->netdev_ops->ndo_uninit(dev);
 	if (dev->priv_destructor)
 		dev->priv_destructor(dev);
+	goto err_free_index;
+
+err_rollback:
+	rollback_registered(dev);
+	dev->reg_state = NETREG_UNREGISTERED;
+err_free_index:
+	if (ifindex)
+		dev_free_index(dev);
 	goto out;
 }
 EXPORT_SYMBOL(register_netdevice);
@@ -7805,9 +7831,11 @@ void netdev_run_todo(void)
 		if (dev->needs_free_netdev)
 			free_netdev(dev);
 
-		/* Report a network device has been unregistered */
 		rtnl_lock();
+		/* Report a network device has been unregistered */
 		dev_net(dev)->dev_unreg_count--;
+		/* Reclaim a network device index */
+		dev_free_index(dev);
 		__rtnl_unlock();
 		wake_up(&netdev_unregistering_wq);
 
@@ -8256,6 +8284,9 @@ int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	kobject_uevent(&dev->dev.kobj, KOBJ_REMOVE);
 	netdev_adjacent_del_links(dev);
 
+	/* Free network device index */
+	dev_free_index(dev);
+
 	/* Actually switch the network namespace */
 	dev_net_set(dev, net);
 
-- 
1.8.3.1

Re: dev: Reclaim network device indexes

[David Miller]

From: Serhey Popovych <serhe.popovych@gmail.com>
Date: Fri, 16 Jun 2017 19:39:34 +0300

> While making dev_new_index() return zero on overrun prevents
> from infinite loop, there is no way to recovery mechanisms
> since namespace ifindex only increases and never reused
> from released network devices.
> 
> To address this we introduce dev_free_index() helper which
> is used to reclaim released network device index when it is
> smaller than last allocated index in namespace.
> 
> This also has positive side effect for equal distribution
> of network devices per buckets in index hash table. That
> positively affects performance of dev_get_by_index() family.
> 
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>

You haven't explained why the current behavior is undesirable,
and why we want reuse.

I don't think we want at all for anything to rely on ifindexes
being allocated one way or another.

Yes, I understand that hash table argument, but that can be solved
other ways.  And what you're talking about is more of an error case
not a normal case.

[PATCH 3/3] veth: Set ifindex only if given and not negative

[Serhey Popovych]

There is already check for ifindex being non negative in
register_netdevice(). Do the same here for convenience.

Fixes: e6f8f1a739b6 ("veth: Allow to create peer link with given ifindex")
Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 drivers/net/veth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 0156fe8..0689433 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -405,7 +405,7 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	if (tbp[IFLA_ADDRESS] == NULL)
 		eth_hw_addr_random(peer);
 
-	if (ifmp && (dev->ifindex != 0))
+	if (ifmp && dev->ifindex > 0)
 		peer->ifindex = ifmp->ifi_index;
 
 	err = register_netdevice(peer);
-- 
1.8.3.1