[PATCH] net: allow interface to be set into vrf if a vif in same vrf

[PATCH] net: allow interface to be set into vrf if a vif in same vrf

[Mike Manning]

Setting an interface into a vrf fails with 'RTNETLINK answers: File
exists' if one of its vifs is already in the same vrf. As the vrf is an
upper device of the vif, it is also showing up as an upper device of
the interface itself. The solution is to restrict this check to devices
other than master. As only one master device can be linked to a device,
in this case the check is for the upper device (vrf) to be linked to as
being the master device rather than any other upper device.

Signed-off-by: Mike Manning <mmanning@att.com>
---
 net/core/dev.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d4362be..2cedf52 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6396,6 +6396,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
 		.linking = true,
 		.upper_info = upper_info,
 	};
+	struct net_device *master_dev;
 	int ret = 0;
 
 	ASSERT_RTNL();
@@ -6407,11 +6408,14 @@ static int __netdev_upper_dev_link(struct net_device *dev,
 	if (netdev_has_upper_dev(upper_dev, dev))
 		return -EBUSY;
 
-	if (netdev_has_upper_dev(dev, upper_dev))
-		return -EEXIST;
-
-	if (master && netdev_master_upper_dev_get(dev))
-		return -EBUSY;
+	if (!master) {
+		if (netdev_has_upper_dev(dev, upper_dev))
+			return -EEXIST;
+	} else {
+		master_dev = netdev_master_upper_dev_get(dev);
+		if (master_dev)
+			return master_dev == upper_dev ? -EEXIST : -EBUSY;
+	}
 
 	ret = call_netdevice_notifiers_info(NETDEV_PRECHANGEUPPER,
 					    &changeupper_info.info);
-- 
2.1.4

Re: [PATCH] net: allow interface to be set into vrf if a vif in same vrf

[David Ahern]

On 2/26/18 9:48 AM, Mike Manning wrote:
> Setting an interface into a vrf fails with 'RTNETLINK answers: File
> exists' if one of its vifs is already in the same vrf. As the vrf is an
> upper device of the vif, it is also showing up as an upper device of
> the interface itself. The solution is to restrict this check to devices
> other than master. As only one master device can be linked to a device,
> in this case the check is for the upper device (vrf) to be linked to as
> being the master device rather than any other upper device.

I'm not understanding what you mean by vif in this context. Can you
elaborate and show an example set of commands?


> 
> Signed-off-by: Mike Manning <mmanning@att.com>
> ---
>  net/core/dev.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d4362be..2cedf52 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6396,6 +6396,7 @@ static int __netdev_upper_dev_link(struct net_device *dev,
>  		.linking = true,
>  		.upper_info = upper_info,
>  	};
> +	struct net_device *master_dev;
>  	int ret = 0;
>  
>  	ASSERT_RTNL();
> @@ -6407,11 +6408,14 @@ static int __netdev_upper_dev_link(struct net_device *dev,
>  	if (netdev_has_upper_dev(upper_dev, dev))
>  		return -EBUSY;
>  
> -	if (netdev_has_upper_dev(dev, upper_dev))
> -		return -EEXIST;
> -
> -	if (master && netdev_master_upper_dev_get(dev))
> -		return -EBUSY;
> +	if (!master) {
> +		if (netdev_has_upper_dev(dev, upper_dev))
> +			return -EEXIST;
> +	} else {
> +		master_dev = netdev_master_upper_dev_get(dev);
> +		if (master_dev)
> +			return master_dev == upper_dev ? -EEXIST : -EBUSY;
> +	}
>  
>  	ret = call_netdevice_notifiers_info(NETDEV_PRECHANGEUPPER,
>  					    &changeupper_info.info);
> 

Re: [PATCH] net: allow interface to be set into vrf if a vif in same vrf

[David Miller]

From: Mike Manning <mmanning@vyatta.mail-att.com>
Date: Mon, 26 Feb 2018 16:48:56 +0000

> Setting an interface into a vrf fails with 'RTNETLINK answers: File
> exists' if one of its vifs is already in the same vrf. As the vrf is an
> upper device of the vif, it is also showing up as an upper device of
> the interface itself. The solution is to restrict this check to devices
> other than master. As only one master device can be linked to a device,
> in this case the check is for the upper device (vrf) to be linked to as
> being the master device rather than any other upper device.
> 
> Signed-off-by: Mike Manning <mmanning@att.com>

David A., please review.

Re: [PATCH] net: allow interface to be set into vrf if a vif in same vrf

[David Ahern]

[ Fixed Mike's email ]

On 3/1/18 11:28 AM, David Miller wrote:
> From: Mike Manning <mmanning@vyatta.mail-att.com>
> Date: Mon, 26 Feb 2018 16:48:56 +0000
> 
>> Setting an interface into a vrf fails with 'RTNETLINK answers: File
>> exists' if one of its vifs is already in the same vrf. As the vrf is an
>> upper device of the vif, it is also showing up as an upper device of
>> the interface itself. The solution is to restrict this check to devices
>> other than master. As only one master device can be linked to a device,
>> in this case the check is for the upper device (vrf) to be linked to as
>> being the master device rather than any other upper device.
>>
>> Signed-off-by: Mike Manning <mmanning@att.com>
> 
> David A., please review.
> 

This one has been superseded by "[PATCH] net: allow interface to be set
into VRF if VLAN interface in same VRF"

Re: [PATCH] net: allow interface to be set into vrf if a vif in same vrf

[David Ahern]

On 2/26/18 11:05 AM, Mike Manning wrote:
> On 2/26/18 9:48 AM, Mike Manning wrote:
> 
>> Setting an interface into a vrf fails with 'RTNETLINK answers: File
>> exists' if one of its vifs is already in the same vrf. As the vrf is an
>> upper device of the vif, it is also showing up as an upper device of
>> the interface itself. The solution is to restrict this check to devices
>> other than master. As only one master device can be linked to a device,
>> in this case the check is for the upper device (vrf) to be linked to as
>> being the master device rather than any other upper device.
> 
> I'm not understanding what you mean by vif in this context. Can you
> elaborate and show an example set of commands?
> 
> 
> Here is an example of a vrf (green), a physical if (ens12) and a virtual if (vif) on vlan 10 (ens12.10):

ok, so by vif you mean a vlan subinterface.


> 
> # ip link show dev vrfgreen
> 14: vrfgreen: <NOARP,MASTER,UP,LOWER_UP> mtu 65536 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>     link/ether b2:9a:92:88:a8:fe brd ff:ff:ff:ff:ff:ff
> # ip link show dev ens12
> 3: ens12: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
>     link/ether 52:54:00:4c:a0:45 brd ff:ff:ff:ff:ff:ff
> # ip link add link ens12 ens12.10 type vlan id 10
> # ip link add link ens12 ens12.20 type vlan id 20
> 
> This works fine:
> 
> # ip link set dev ens12 master vrfgreen
> # ip link show dev ens12
> 3: ens12: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel master vrfgreen state UP mode DEFAULT group default qlen 1000
>     link/ether 52:54:00:4c:a0:45 brd ff:ff:ff:ff:ff:ff
> # ip link set dev ens12 nomaster
> 
> But if one of the vifs is first set into the same vrf, then subsequently setting the parent into the vrf fails:
> 
> # ip link set dev ens12.10 master vrfgreen
> # ip link show dev ens12.10
> 39: ens12.10@ens12: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master vrfgreen state UP mode DEFAULT group default qlen 1000
>     link/ether 52:54:00:4c:a0:45 brd ff:ff:ff:ff:ff:ff
> # ip link set dev ens12 master vrfgreen
> RTNETLINK answers: File exists
> # 
> 
> The workaround is to move the vif back into the default VRF beforehand, but for this one first has to shut the vif so as to avoid the risk of traffic leaking from the VRF.
> 
> This fix is proposed to avoid that messy workaround.

Ok, I get the problem now. I would like to see the above comments and
series of commands added to the commit message.

I need to think about the change with respect to other stacking options.
Somewhere I have commands that cover a lot of permutations.