[PATCH] core: Correct an over-stringent device loop detection.

[PATCH] core: Correct an over-stringent device loop detection.

[Vladislav Yasevich]

The code in __netdev_upper_dev_link() has an over-stringent
loop detection logic that actually prevents valid configurations
from working correctly.

In particular, the logic returns an error if an upper device
is already in the list of all upper devices for a given dev.
This particular check seems to be a overzealous as it disallows
perfectly valid configurations.  For example:
  # ip l a link eth0 name eth0.10 type vlan id 10
  # ip l a dev br0 typ bridge
  # ip l s eth0.10 master br0
  # ip l s eth0 master br0  <--- Will fail

If you switch the last two commands (add eth0 first), then both
will succeed.  If after that, you remove eth0 and try to re-add
it, it will fail!

It appears to be enough to simply check adj_list to keeps things
safe.

I've tried stacking multiple devices multiple times in all different
combinations, and either rx_handler registration prevented the stacking
of the device linking cought the error.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.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 c7ba038..2c1c67f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5209,7 +5209,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
 	if (__netdev_find_adj(upper_dev, dev, &upper_dev->all_adj_list.upper))
 		return -EBUSY;
 
-	if (__netdev_find_adj(dev, upper_dev, &dev->all_adj_list.upper))
+	if (__netdev_find_adj(dev, upper_dev, &dev->adj_list.upper))
 		return -EEXIST;
 
 	if (master && netdev_master_upper_dev_get(dev))
-- 
1.9.3

Re: [PATCH] core: Correct an over-stringent device loop detection.

[Jiri Pirko]

Sun, May 03, 2015 at 03:33:44AM CEST, vyasevich@gmail.com wrote:
>The code in __netdev_upper_dev_link() has an over-stringent
>loop detection logic that actually prevents valid configurations
>from working correctly.
>
>In particular, the logic returns an error if an upper device
>is already in the list of all upper devices for a given dev.
>This particular check seems to be a overzealous as it disallows
>perfectly valid configurations.  For example:
>  # ip l a link eth0 name eth0.10 type vlan id 10
>  # ip l a dev br0 typ bridge
>  # ip l s eth0.10 master br0
>  # ip l s eth0 master br0  <--- Will fail
>
>If you switch the last two commands (add eth0 first), then both
>will succeed.  If after that, you remove eth0 and try to re-add
>it, it will fail!
>
>It appears to be enough to simply check adj_list to keeps things
>safe.
>
>I've tried stacking multiple devices multiple times in all different
>combinations, and either rx_handler registration prevented the stacking
>of the device linking cought the error.
>
>Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>

Acked-by: Jiri Pirko <jiri@resnulli.us>

Re: [PATCH] core: Correct an over-stringent device loop detection.

[Veaceslav Falico]

On Sat, May 02, 2015 at 09:33:44PM -0400, Vladislav Yasevich wrote:
>The code in __netdev_upper_dev_link() has an over-stringent
>loop detection logic that actually prevents valid configurations
>from working correctly.
>
>In particular, the logic returns an error if an upper device
>is already in the list of all upper devices for a given dev.
>This particular check seems to be a overzealous as it disallows
>perfectly valid configurations.  For example:
>  # ip l a link eth0 name eth0.10 type vlan id 10
>  # ip l a dev br0 typ bridge
>  # ip l s eth0.10 master br0
>  # ip l s eth0 master br0  <--- Will fail
>
>If you switch the last two commands (add eth0 first), then both
>will succeed.  If after that, you remove eth0 and try to re-add
>it, it will fail!
>
>It appears to be enough to simply check adj_list to keeps things
>safe.
>
>I've tried stacking multiple devices multiple times in all different
>combinations, and either rx_handler registration prevented the stacking
>of the device linking cought the error.
>
>Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>

Good catch.

Acked-by: Veaceslav Falico <vfalico@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 c7ba038..2c1c67f 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -5209,7 +5209,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
> 	if (__netdev_find_adj(upper_dev, dev, &upper_dev->all_adj_list.upper))
> 		return -EBUSY;
>
>-	if (__netdev_find_adj(dev, upper_dev, &dev->all_adj_list.upper))
>+	if (__netdev_find_adj(dev, upper_dev, &dev->adj_list.upper))
> 		return -EEXIST;
>
> 	if (master && netdev_master_upper_dev_get(dev))
>-- 
>1.9.3
>

Re: [PATCH] core: Correct an over-stringent device loop detection.

[David Miller]

From: Vladislav Yasevich <vyasevich@gmail.com>
Date: Sat,  2 May 2015 21:33:44 -0400

> The code in __netdev_upper_dev_link() has an over-stringent
> loop detection logic that actually prevents valid configurations
> from working correctly.
> 
> In particular, the logic returns an error if an upper device
> is already in the list of all upper devices for a given dev.
> This particular check seems to be a overzealous as it disallows
> perfectly valid configurations.  For example:
>   # ip l a link eth0 name eth0.10 type vlan id 10
>   # ip l a dev br0 typ bridge
>   # ip l s eth0.10 master br0
>   # ip l s eth0 master br0  <--- Will fail
> 
> If you switch the last two commands (add eth0 first), then both
> will succeed.  If after that, you remove eth0 and try to re-add
> it, it will fail!
> 
> It appears to be enough to simply check adj_list to keeps things
> safe.
> 
> I've tried stacking multiple devices multiple times in all different
> combinations, and either rx_handler registration prevented the stacking
> of the device linking cought the error.
> 
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>

Applied, thanks Vlad.

Re: [PATCH] core: Correct an over-stringent device loop detection.

[Vlad Yasevich]

On 05/04/2015 02:58 PM, David Miller wrote:
> From: Vladislav Yasevich <vyasevich@gmail.com>
> Date: Sat,  2 May 2015 21:33:44 -0400
> 
>> The code in __netdev_upper_dev_link() has an over-stringent
>> loop detection logic that actually prevents valid configurations
>> from working correctly.
>>
>> In particular, the logic returns an error if an upper device
>> is already in the list of all upper devices for a given dev.
>> This particular check seems to be a overzealous as it disallows
>> perfectly valid configurations.  For example:
>>   # ip l a link eth0 name eth0.10 type vlan id 10
>>   # ip l a dev br0 typ bridge
>>   # ip l s eth0.10 master br0
>>   # ip l s eth0 master br0  <--- Will fail
>>
>> If you switch the last two commands (add eth0 first), then both
>> will succeed.  If after that, you remove eth0 and try to re-add
>> it, it will fail!
>>
>> It appears to be enough to simply check adj_list to keeps things
>> safe.
>>
>> I've tried stacking multiple devices multiple times in all different
>> combinations, and either rx_handler registration prevented the stacking
>> of the device linking cought the error.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> 
> Applied, thanks Vlad.
> 

Hi Dave

Can you also queue it for stable.  This has been broken for a while.
Thanks
-vlad

Re: [PATCH] core: Correct an over-stringent device loop detection.

[David Miller]

From: Vlad Yasevich <vyasevic@redhat.com>
Date: Mon, 04 May 2015 15:55:48 -0400

> Can you also queue it for stable.  This has been broken for a while.

Done.