All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: Fix device name resolving crash in default_device_exit()
@ 2018-06-14 12:38 Kirill Tkhai
  2018-06-14 17:11 ` David Ahern
  0 siblings, 1 reply; 11+ messages in thread
From: Kirill Tkhai @ 2018-06-14 12:38 UTC (permalink / raw)
  To: netdev
  Cc: davem, daniel, jakub.kicinski, ktkhai, ast, linux,
	john.fastabend, brouer, dsahern

The following script makes kernel to crash since it can't obtain
a name for a device, when the name is occupied by another device:

#!/bin/bash
ifconfig eth0 down
ifconfig eth1 down
index=`cat /sys/class/net/eth1/ifindex`
ip link set eth1 name dev$index
unshare -n sleep 1h &
pid=$!
while [[ "`readlink /proc/self/ns/net`" == "`readlink /proc/$pid/ns/net`" ]]; do continue; done
ip link set dev$index netns $pid
ip link set eth0 name dev$index
kill -9 $pid

Kernel messages:

virtio_net virtio1 dev3: renamed from eth1
virtio_net virtio0 dev3: renamed from eth0
default_device_exit: failed to move dev3 to init_net: -17
------------[ cut here ]------------
kernel BUG at net/core/dev.c:8978!
invalid opcode: 0000 [#1] PREEMPT SMP
CPU: 1 PID: 276 Comm: kworker/u8:3 Not tainted 4.17.0+ #292
Workqueue: netns cleanup_net
RIP: 0010:default_device_exit+0x9c/0xb0
[stack trace snipped]

This patch gives more variability during choosing new name
of device and fixes the problem.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 net/core/dev.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6e18242a1cae..6c9b9303ded6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8959,7 +8959,6 @@ static void __net_exit default_device_exit(struct net *net)
 	rtnl_lock();
 	for_each_netdev_safe(net, dev, aux) {
 		int err;
-		char fb_name[IFNAMSIZ];
 
 		/* Ignore unmoveable devices (i.e. loopback) */
 		if (dev->features & NETIF_F_NETNS_LOCAL)
@@ -8970,8 +8969,7 @@ static void __net_exit default_device_exit(struct net *net)
 			continue;
 
 		/* Push remaining network devices to init_net */
-		snprintf(fb_name, IFNAMSIZ, "dev%d", dev->ifindex);
-		err = dev_change_net_namespace(dev, &init_net, fb_name);
+		err = dev_change_net_namespace(dev, &init_net, "dev%d");
 		if (err) {
 			pr_emerg("%s: failed to move %s to init_net: %d\n",
 				 __func__, dev->name, err);

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-14 12:38 [PATCH] net: Fix device name resolving crash in default_device_exit() Kirill Tkhai
@ 2018-06-14 17:11 ` David Ahern
  2018-06-15  9:44   ` Kirill Tkhai
  0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2018-06-14 17:11 UTC (permalink / raw)
  To: Kirill Tkhai, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

On 6/14/18 6:38 AM, Kirill Tkhai wrote:
> The following script makes kernel to crash since it can't obtain
> a name for a device, when the name is occupied by another device:
> 
> #!/bin/bash
> ifconfig eth0 down
> ifconfig eth1 down
> index=`cat /sys/class/net/eth1/ifindex`
> ip link set eth1 name dev$index
> unshare -n sleep 1h &
> pid=$!
> while [[ "`readlink /proc/self/ns/net`" == "`readlink /proc/$pid/ns/net`" ]]; do continue; done
> ip link set dev$index netns $pid
> ip link set eth0 name dev$index
> kill -9 $pid
> 
> Kernel messages:
> 
> virtio_net virtio1 dev3: renamed from eth1
> virtio_net virtio0 dev3: renamed from eth0
> default_device_exit: failed to move dev3 to init_net: -17
> ------------[ cut here ]------------
> kernel BUG at net/core/dev.c:8978!
> invalid opcode: 0000 [#1] PREEMPT SMP
> CPU: 1 PID: 276 Comm: kworker/u8:3 Not tainted 4.17.0+ #292
> Workqueue: netns cleanup_net
> RIP: 0010:default_device_exit+0x9c/0xb0
> [stack trace snipped]
> 
> This patch gives more variability during choosing new name
> of device and fixes the problem.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  net/core/dev.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6e18242a1cae..6c9b9303ded6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8959,7 +8959,6 @@ static void __net_exit default_device_exit(struct net *net)
>  	rtnl_lock();
>  	for_each_netdev_safe(net, dev, aux) {
>  		int err;
> -		char fb_name[IFNAMSIZ];
>  
>  		/* Ignore unmoveable devices (i.e. loopback) */
>  		if (dev->features & NETIF_F_NETNS_LOCAL)
> @@ -8970,8 +8969,7 @@ static void __net_exit default_device_exit(struct net *net)
>  			continue;
>  
>  		/* Push remaining network devices to init_net */
> -		snprintf(fb_name, IFNAMSIZ, "dev%d", dev->ifindex);
> -		err = dev_change_net_namespace(dev, &init_net, fb_name);
> +		err = dev_change_net_namespace(dev, &init_net, "dev%d");
>  		if (err) {
>  			pr_emerg("%s: failed to move %s to init_net: %d\n",
>  				 __func__, dev->name, err);
> 

This could cause repeated looping over __dev_alloc_name. If init_net has
a large number of devices, it is going to be a performance bottleneck.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-14 17:11 ` David Ahern
@ 2018-06-15  9:44   ` Kirill Tkhai
  2018-06-17 18:58     ` David Ahern
  0 siblings, 1 reply; 11+ messages in thread
From: Kirill Tkhai @ 2018-06-15  9:44 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

On 14.06.2018 20:11, David Ahern wrote:
> On 6/14/18 6:38 AM, Kirill Tkhai wrote:
>> The following script makes kernel to crash since it can't obtain
>> a name for a device, when the name is occupied by another device:
>>
>> #!/bin/bash
>> ifconfig eth0 down
>> ifconfig eth1 down
>> index=`cat /sys/class/net/eth1/ifindex`
>> ip link set eth1 name dev$index
>> unshare -n sleep 1h &
>> pid=$!
>> while [[ "`readlink /proc/self/ns/net`" == "`readlink /proc/$pid/ns/net`" ]]; do continue; done
>> ip link set dev$index netns $pid
>> ip link set eth0 name dev$index
>> kill -9 $pid
>>
>> Kernel messages:
>>
>> virtio_net virtio1 dev3: renamed from eth1
>> virtio_net virtio0 dev3: renamed from eth0
>> default_device_exit: failed to move dev3 to init_net: -17
>> ------------[ cut here ]------------
>> kernel BUG at net/core/dev.c:8978!
>> invalid opcode: 0000 [#1] PREEMPT SMP
>> CPU: 1 PID: 276 Comm: kworker/u8:3 Not tainted 4.17.0+ #292
>> Workqueue: netns cleanup_net
>> RIP: 0010:default_device_exit+0x9c/0xb0
>> [stack trace snipped]
>>
>> This patch gives more variability during choosing new name
>> of device and fixes the problem.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  net/core/dev.c |    4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 6e18242a1cae..6c9b9303ded6 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -8959,7 +8959,6 @@ static void __net_exit default_device_exit(struct net *net)
>>  	rtnl_lock();
>>  	for_each_netdev_safe(net, dev, aux) {
>>  		int err;
>> -		char fb_name[IFNAMSIZ];
>>  
>>  		/* Ignore unmoveable devices (i.e. loopback) */
>>  		if (dev->features & NETIF_F_NETNS_LOCAL)
>> @@ -8970,8 +8969,7 @@ static void __net_exit default_device_exit(struct net *net)
>>  			continue;
>>  
>>  		/* Push remaining network devices to init_net */
>> -		snprintf(fb_name, IFNAMSIZ, "dev%d", dev->ifindex);
>> -		err = dev_change_net_namespace(dev, &init_net, fb_name);
>> +		err = dev_change_net_namespace(dev, &init_net, "dev%d");
>>  		if (err) {
>>  			pr_emerg("%s: failed to move %s to init_net: %d\n",
>>  				 __func__, dev->name, err);
>>
> 
> This could cause repeated looping over __dev_alloc_name. If init_net has
> a large number of devices, it is going to be a performance bottleneck.

Hm, but is this a likely case, when real device is moved to net ns, so it
requires moving to init_net back? It seems the most devices moved to !init_net
are virtual and they just destroyed in default_device_exit_batch(). Or we have
more devices to care here?

I don't much want to insert here something like below:

	if (__dev_get_by_name(&init_net, dev->name))
		snprintf(fb_name, IFNAMSIZ, "dev%d", dev->ifindex);
	err = dev_change_net_namespace(dev, &init_net, "dev%d");

because dev_change_net_namespace() is generic interface and it's used not only here,
and this will crumble the code in corner cases.

Maybe you have better ideas about this?

Kirill

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-15  9:44   ` Kirill Tkhai
@ 2018-06-17 18:58     ` David Ahern
  2018-06-18 11:21       ` Kirill Tkhai
  0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2018-06-17 18:58 UTC (permalink / raw)
  To: Kirill Tkhai, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

On 6/15/18 3:44 AM, Kirill Tkhai wrote:
> Hm, but is this a likely case, when real device is moved to net ns, so it
> requires moving to init_net back? It seems the most devices moved to !init_net
> are virtual and they just destroyed in default_device_exit_batch(). Or we have
> more devices to care here?
> 
> I don't much want to insert here something like below:
> 
> 	if (__dev_get_by_name(&init_net, dev->name))
> 		snprintf(fb_name, IFNAMSIZ, "dev%d", dev->ifindex);
> 	err = dev_change_net_namespace(dev, &init_net, "dev%d");
> 
> because dev_change_net_namespace() is generic interface and it's used not only here,
> and this will crumble the code in corner cases.
> 
> Maybe you have better ideas about this?

There are a lot of use cases these days (e.g., switch NOS) with 1000's
(10's of 1000's) of netdevices. On top of that support for port netdevs
in a namespace to create virtual switches needs to happen (and I suspect
will happen in the next few years). That becomes one example where
netdevices representing physical ports can be pushed back to init_net.

That said, not many easy options at the moment for the bug you are fixing.

Further, panic'ing a node because the move back to init_net fails is
just wrong.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-17 18:58     ` David Ahern
@ 2018-06-18 11:21       ` Kirill Tkhai
  2018-06-18 11:26         ` Kirill Tkhai
  0 siblings, 1 reply; 11+ messages in thread
From: Kirill Tkhai @ 2018-06-18 11:21 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

On 17.06.2018 21:58, David Ahern wrote:
> On 6/15/18 3:44 AM, Kirill Tkhai wrote:
>> Hm, but is this a likely case, when real device is moved to net ns, so it
>> requires moving to init_net back? It seems the most devices moved to !init_net
>> are virtual and they just destroyed in default_device_exit_batch(). Or we have
>> more devices to care here?
>>
>> I don't much want to insert here something like below:
>>
>> 	if (__dev_get_by_name(&init_net, dev->name))
>> 		snprintf(fb_name, IFNAMSIZ, "dev%d", dev->ifindex);
>> 	err = dev_change_net_namespace(dev, &init_net, "dev%d");
>>
>> because dev_change_net_namespace() is generic interface and it's used not only here,
>> and this will crumble the code in corner cases.
>>
>> Maybe you have better ideas about this?
> 
> There are a lot of use cases these days (e.g., switch NOS) with 1000's
> (10's of 1000's) of netdevices. On top of that support for port netdevs
> in a namespace to create virtual switches needs to happen (and I suspect
> will happen in the next few years). That becomes one example where
> netdevices representing physical ports can be pushed back to init_net.

Oh, then we really need to do something with rtnl_mutex. Otherwise
this will stop working at all.

> That said, not many easy options at the moment for the bug you are fixing.
> 
> Further, panic'ing a node because the move back to init_net fails is
> just wrong.

So, let's fix it for now like in the patch to avoid the panic. Then we
can rework this in generic way to make the generic fallback name for moved
devices. Maybe, something like to give all moved device a fallback name
like "__moved-<hash of jiffies>-<generated id>".

Kirill

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-18 11:21       ` Kirill Tkhai
@ 2018-06-18 11:26         ` Kirill Tkhai
  2018-06-20  8:57           ` Kirill Tkhai
  0 siblings, 1 reply; 11+ messages in thread
From: Kirill Tkhai @ 2018-06-18 11:26 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

On 18.06.2018 14:21, Kirill Tkhai wrote:
> On 17.06.2018 21:58, David Ahern wrote:
>> On 6/15/18 3:44 AM, Kirill Tkhai wrote:
>>> Hm, but is this a likely case, when real device is moved to net ns, so it
>>> requires moving to init_net back? It seems the most devices moved to !init_net
>>> are virtual and they just destroyed in default_device_exit_batch(). Or we have
>>> more devices to care here?
>>>
>>> I don't much want to insert here something like below:
>>>
>>> 	if (__dev_get_by_name(&init_net, dev->name))
>>> 		snprintf(fb_name, IFNAMSIZ, "dev%d", dev->ifindex);
>>> 	err = dev_change_net_namespace(dev, &init_net, "dev%d");
>>>
>>> because dev_change_net_namespace() is generic interface and it's used not only here,
>>> and this will crumble the code in corner cases.
>>>
>>> Maybe you have better ideas about this?
>>
>> There are a lot of use cases these days (e.g., switch NOS) with 1000's
>> (10's of 1000's) of netdevices. On top of that support for port netdevs
>> in a namespace to create virtual switches needs to happen (and I suspect
>> will happen in the next few years). That becomes one example where
>> netdevices representing physical ports can be pushed back to init_net.
> 
> Oh, then we really need to do something with rtnl_mutex. Otherwise
> this will stop working at all.
> 
>> That said, not many easy options at the moment for the bug you are fixing.
>>
>> Further, panic'ing a node because the move back to init_net fails is
>> just wrong.
> 
> So, let's fix it for now like in the patch to avoid the panic. Then we
> can rework this in generic way to make the generic fallback name for moved
> devices. Maybe, something like to give all moved device a fallback name
> like "__moved-<hash of jiffies>-<generated id>".

Just to clarify, I mean that this small fix will be easy to backport to stable.

Kirill

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-18 11:26         ` Kirill Tkhai
@ 2018-06-20  8:57           ` Kirill Tkhai
  2018-06-20 17:15             ` David Ahern
  0 siblings, 1 reply; 11+ messages in thread
From: Kirill Tkhai @ 2018-06-20  8:57 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

From: Kirill Tkhai <ktkhai@virtuozzo.com>

The following script makes kernel to crash since it can't obtain
a name for a device, when the name is occupied by another device:

#!/bin/bash
ifconfig eth0 down
ifconfig eth1 down
index=`cat /sys/class/net/eth1/ifindex`
ip link set eth1 name dev$index
unshare -n sleep 1h &
pid=$!
while [[ "`readlink /proc/self/ns/net`" == "`readlink /proc/$pid/ns/net`" ]]; do continue; done
ip link set dev$index netns $pid
ip link set eth0 name dev$index
kill -9 $pid

Kernel messages:

virtio_net virtio1 dev3: renamed from eth1
virtio_net virtio0 dev3: renamed from eth0
default_device_exit: failed to move dev3 to init_net: -17
------------[ cut here ]------------
kernel BUG at net/core/dev.c:8978!
invalid opcode: 0000 [#1] PREEMPT SMP
CPU: 1 PID: 276 Comm: kworker/u8:3 Not tainted 4.17.0+ #292
Workqueue: netns cleanup_net
RIP: 0010:default_device_exit+0x9c/0xb0
[stack trace snipped]

This patch gives more variability during choosing new name
of device and fixes the problem.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---

Since there is no suggestions how to fix this in another way, I'm resending the patch.

 net/core/dev.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6e18242a1cae..6c9b9303ded6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8959,7 +8959,6 @@ static void __net_exit default_device_exit(struct net *net)
 	rtnl_lock();
 	for_each_netdev_safe(net, dev, aux) {
 		int err;
-		char fb_name[IFNAMSIZ];
 
 		/* Ignore unmoveable devices (i.e. loopback) */
 		if (dev->features & NETIF_F_NETNS_LOCAL)
@@ -8970,8 +8969,7 @@ static void __net_exit default_device_exit(struct net *net)
 			continue;
 
 		/* Push remaining network devices to init_net */
-		snprintf(fb_name, IFNAMSIZ, "dev%d", dev->ifindex);
-		err = dev_change_net_namespace(dev, &init_net, fb_name);
+		err = dev_change_net_namespace(dev, &init_net, "dev%d");
 		if (err) {
 			pr_emerg("%s: failed to move %s to init_net: %d\n",
 				 __func__, dev->name, err);

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-20  8:57           ` Kirill Tkhai
@ 2018-06-20 17:15             ` David Ahern
  2018-06-21 10:03               ` Kirill Tkhai
  0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2018-06-20 17:15 UTC (permalink / raw)
  To: Kirill Tkhai, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

On 6/20/18 2:57 AM, Kirill Tkhai wrote:
> From: Kirill Tkhai <ktkhai@virtuozzo.com>
> 
> The following script makes kernel to crash since it can't obtain
> a name for a device, when the name is occupied by another device:
> 
> #!/bin/bash
> ifconfig eth0 down
> ifconfig eth1 down
> index=`cat /sys/class/net/eth1/ifindex`
> ip link set eth1 name dev$index
> unshare -n sleep 1h &
> pid=$!
> while [[ "`readlink /proc/self/ns/net`" == "`readlink /proc/$pid/ns/net`" ]]; do continue; done
> ip link set dev$index netns $pid
> ip link set eth0 name dev$index
> kill -9 $pid
> 
> Kernel messages:
> 
> virtio_net virtio1 dev3: renamed from eth1
> virtio_net virtio0 dev3: renamed from eth0
> default_device_exit: failed to move dev3 to init_net: -17
> ------------[ cut here ]------------
> kernel BUG at net/core/dev.c:8978!
> invalid opcode: 0000 [#1] PREEMPT SMP
> CPU: 1 PID: 276 Comm: kworker/u8:3 Not tainted 4.17.0+ #292
> Workqueue: netns cleanup_net
> RIP: 0010:default_device_exit+0x9c/0xb0
> [stack trace snipped]
> 
> This patch gives more variability during choosing new name
> of device and fixes the problem.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
> 
> Since there is no suggestions how to fix this in another way, I'm resending the patch.

This patch does not remove the BUG, so does not really solve the
problem. ie., it is fairly trivial to write a script (32k dev%d named
devices in init_net) that triggers it again, so your commit subject and
commit log are not correct with the references to 'fixing the problem'.

The change does provide more variability in naming and reduces the
likelihood of not being able to push a device back to init_net.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-20 17:15             ` David Ahern
@ 2018-06-21 10:03               ` Kirill Tkhai
  2018-06-21 15:28                 ` David Ahern
  0 siblings, 1 reply; 11+ messages in thread
From: Kirill Tkhai @ 2018-06-21 10:03 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

On 20.06.2018 20:15, David Ahern wrote:
> On 6/20/18 2:57 AM, Kirill Tkhai wrote:
>> From: Kirill Tkhai <ktkhai@virtuozzo.com>
>>
>> The following script makes kernel to crash since it can't obtain
>> a name for a device, when the name is occupied by another device:
>>
>> #!/bin/bash
>> ifconfig eth0 down
>> ifconfig eth1 down
>> index=`cat /sys/class/net/eth1/ifindex`
>> ip link set eth1 name dev$index
>> unshare -n sleep 1h &
>> pid=$!
>> while [[ "`readlink /proc/self/ns/net`" == "`readlink /proc/$pid/ns/net`" ]]; do continue; done
>> ip link set dev$index netns $pid
>> ip link set eth0 name dev$index
>> kill -9 $pid
>>
>> Kernel messages:
>>
>> virtio_net virtio1 dev3: renamed from eth1
>> virtio_net virtio0 dev3: renamed from eth0
>> default_device_exit: failed to move dev3 to init_net: -17
>> ------------[ cut here ]------------
>> kernel BUG at net/core/dev.c:8978!
>> invalid opcode: 0000 [#1] PREEMPT SMP
>> CPU: 1 PID: 276 Comm: kworker/u8:3 Not tainted 4.17.0+ #292
>> Workqueue: netns cleanup_net
>> RIP: 0010:default_device_exit+0x9c/0xb0
>> [stack trace snipped]
>>
>> This patch gives more variability during choosing new name
>> of device and fixes the problem.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>
>> Since there is no suggestions how to fix this in another way, I'm resending the patch.
> 
> This patch does not remove the BUG, so does not really solve the
> problem. ie., it is fairly trivial to write a script (32k dev%d named
> devices in init_net) that triggers it again, so your commit subject and
> commit log are not correct with the references to 'fixing the problem'.

1)I'm not agree with you and I don't think removing the BUG() is a good idea.
This function is called from the place, where it must not fail. But it can
fail, and the problem with name is not the only reason of this happens.
We can't continue further pernet_operations in case of a problem happened
in default_device_exit(), and we can't remove the BUG() before this function
becomes of void type. But we are not going to make it of void type. So
we can't remove the BUG().

2)In case of the script is trivial, can't you just post it here to show
what type of devices you mean? Is there real problem or this is
a theoretical thinking?

All virtual devices I see have rtnl_link_ops, so that they just destroyed
in default_device_exit_batch(). According to physical devices, it's difficult
to imagine a node with 32k physical devices, and if someone tried to deploy
them it may meet problems not only in this place.

> The change does provide more variability in naming and reduces the
> likelihood of not being able to push a device back to init_net.

No, it provides. With the patch one may move real device to a container,
and allow to do with the device anything including changing of device
index. Then, the destruction of the container does not resilt a kernel
panic just because of two devices have the same index.

Kirill

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-21 10:03               ` Kirill Tkhai
@ 2018-06-21 15:28                 ` David Ahern
  2018-06-22  8:36                   ` Kirill Tkhai
  0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2018-06-21 15:28 UTC (permalink / raw)
  To: Kirill Tkhai, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

On 6/21/18 4:03 AM, Kirill Tkhai wrote:
>> This patch does not remove the BUG, so does not really solve the
>> problem. ie., it is fairly trivial to write a script (32k dev%d named
>> devices in init_net) that triggers it again, so your commit subject and
>> commit log are not correct with the references to 'fixing the problem'.
> 
> 1)I'm not agree with you and I don't think removing the BUG() is a good idea.
> This function is called from the place, where it must not fail. But it can
> fail, and the problem with name is not the only reason of this happens.
> We can't continue further pernet_operations in case of a problem happened
> in default_device_exit(), and we can't remove the BUG() before this function
> becomes of void type. But we are not going to make it of void type. So
> we can't remove the BUG().

You missed my point: that the function can still fail means you are not
"fixing" the problem, only delaying it.

> 
> 2)In case of the script is trivial, can't you just post it here to show
> what type of devices you mean? Is there real problem or this is
> a theoretical thinking?

Current code:

# ip li sh dev eth2
4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP
mode DEFAULT group default qlen 1000
    link/ether 02:e0:f9:46:64:80 brd ff:ff:ff:ff:ff:ff
# ip netns add fubar
# ip li set eth2 netns fubar
# ip li add eth2 type dummy
# ip li add dev4 type dummy
# ip netns del fubar
--> BUG
kernel:[78079.127748] default_device_exit: failed to move eth2 to
init_net: -17


With your patch:

# ip li sh dev eth2
4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP
mode DEFAULT group default qlen 1000
    link/ether 02:e0:f9:46:64:80 brd ff:ff:ff:ff:ff:ff
# ip netns add fubar
# ip li set eth2 netns fubar
# ip li add eth2 type dummy
# for n in $(seq 0 $((32*1024))); do
  echo "li add dev${n} type dummy"
  done > ip.batch
# ip -batch ip.batch
# ip netns del fubar
--> BUG
kernel:[   25.800024] default_device_exit: failed to move eth2 to
init_net: -17


> 
> All virtual devices I see have rtnl_link_ops, so that they just destroyed
> in default_device_exit_batch(). According to physical devices, it's difficult
> to imagine a node with 32k physical devices, and if someone tried to deploy
> them it may meet problems not only in this place.

Nothing says it has to be a physical device. It is only checking for a name.

> 
>> The change does provide more variability in naming and reduces the
>> likelihood of not being able to push a device back to init_net.
> 
> No, it provides. With the patch one may move real device to a container,
> and allow to do with the device anything including changing of device
> index. Then, the destruction of the container does not resilt a kernel
> panic just because of two devices have the same index.
> 
> Kirill
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: Fix device name resolving crash in default_device_exit()
  2018-06-21 15:28                 ` David Ahern
@ 2018-06-22  8:36                   ` Kirill Tkhai
  0 siblings, 0 replies; 11+ messages in thread
From: Kirill Tkhai @ 2018-06-22  8:36 UTC (permalink / raw)
  To: David Ahern, netdev
  Cc: davem, daniel, jakub.kicinski, ast, linux, john.fastabend, brouer

On 21.06.2018 18:28, David Ahern wrote:
> On 6/21/18 4:03 AM, Kirill Tkhai wrote:
>>> This patch does not remove the BUG, so does not really solve the
>>> problem. ie., it is fairly trivial to write a script (32k dev%d named
>>> devices in init_net) that triggers it again, so your commit subject and
>>> commit log are not correct with the references to 'fixing the problem'.
>>
>> 1)I'm not agree with you and I don't think removing the BUG() is a good idea.
>> This function is called from the place, where it must not fail. But it can
>> fail, and the problem with name is not the only reason of this happens.
>> We can't continue further pernet_operations in case of a problem happened
>> in default_device_exit(), and we can't remove the BUG() before this function
>> becomes of void type. But we are not going to make it of void type. So
>> we can't remove the BUG().
> 
> You missed my point: that the function can still fail means you are not
> "fixing" the problem, only delaying it.

Till the function is of int type and it can fail, we can't remove the BUG().
And this does not connected with name resolution.

>>
>> 2)In case of the script is trivial, can't you just post it here to show
>> what type of devices you mean? Is there real problem or this is
>> a theoretical thinking?
> 
> Current code:
> 
> # ip li sh dev eth2
> 4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP
> mode DEFAULT group default qlen 1000
>     link/ether 02:e0:f9:46:64:80 brd ff:ff:ff:ff:ff:ff
> # ip netns add fubar
> # ip li set eth2 netns fubar
> # ip li add eth2 type dummy
> # ip li add dev4 type dummy
> # ip netns del fubar
> --> BUG
> kernel:[78079.127748] default_device_exit: failed to move eth2 to
> init_net: -17
> 
> 
> With your patch:
> 
> # ip li sh dev eth2
> 4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP
> mode DEFAULT group default qlen 1000
>     link/ether 02:e0:f9:46:64:80 brd ff:ff:ff:ff:ff:ff
> # ip netns add fubar
> # ip li set eth2 netns fubar
> # ip li add eth2 type dummy
> # for n in $(seq 0 $((32*1024))); do
>   echo "li add dev${n} type dummy"
>   done > ip.batch
> # ip -batch ip.batch
> # ip netns del fubar
> --> BUG
> kernel:[   25.800024] default_device_exit: failed to move eth2 to
> init_net: -17

Yeah, this has a sense.

>>
>> All virtual devices I see have rtnl_link_ops, so that they just destroyed
>> in default_device_exit_batch(). According to physical devices, it's difficult
>> to imagine a node with 32k physical devices, and if someone tried to deploy
>> them it may meet problems not only in this place.
> 
> Nothing says it has to be a physical device. It is only checking for a name.
> 
>>
>>> The change does provide more variability in naming and reduces the
>>> likelihood of not being able to push a device back to init_net.
>>
>> No, it provides. With the patch one may move real device to a container,
>> and allow to do with the device anything including changing of device
>> index. Then, the destruction of the container does not resilt a kernel
>> panic just because of two devices have the same index.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-06-22  8:36 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-14 12:38 [PATCH] net: Fix device name resolving crash in default_device_exit() Kirill Tkhai
2018-06-14 17:11 ` David Ahern
2018-06-15  9:44   ` Kirill Tkhai
2018-06-17 18:58     ` David Ahern
2018-06-18 11:21       ` Kirill Tkhai
2018-06-18 11:26         ` Kirill Tkhai
2018-06-20  8:57           ` Kirill Tkhai
2018-06-20 17:15             ` David Ahern
2018-06-21 10:03               ` Kirill Tkhai
2018-06-21 15:28                 ` David Ahern
2018-06-22  8:36                   ` Kirill Tkhai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.