[PATCH net-next v2] macvlan: Support interface operstate properly

[PATCH net-next v2] macvlan: Support interface operstate properly

[Debabrata Banerjee]

Set appropriate macvlan interface status based on lower device and our
status. Can be up, down, or lowerlayerdown.

de7d244d0 improved operstate by setting it from unknown to up, however
it did not handle transferring down or lowerlayerdown.

Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
---
v2: Fix locking and update commit message

 drivers/net/macvlan.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 2bcf1f3..306124ba 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -91,6 +91,7 @@ static struct macvlan_port *macvlan_port_get_rtnl(const struct net_device *dev)
 }
 
 #define macvlan_port_exists(dev) (dev->priv_flags & IFF_MACVLAN_PORT)
+#define is_macvlan(dev) (dev->priv_flags & IFF_MACVLAN)
 
 static struct macvlan_dev *macvlan_hash_lookup(const struct macvlan_port *port,
 					       const unsigned char *addr)
@@ -1242,6 +1243,28 @@ static int macvlan_changelink_sources(struct macvlan_dev *vlan, u32 mode,
 	return 0;
 }
 
+static void macvlan_set_operstate(struct net_device *lowerdev,
+				  struct net_device *dev)
+{
+	unsigned char newstate = dev->operstate;
+
+	if (!(dev->flags & IFF_UP))
+		newstate = IF_OPER_DOWN;
+	else if ((lowerdev->flags & IFF_UP) && netif_oper_up(lowerdev))
+		newstate = IF_OPER_UP;
+	else
+		newstate = IF_OPER_LOWERLAYERDOWN;
+
+	write_lock_bh(&dev_base_lock);
+	if (dev->operstate != newstate) {
+		dev->operstate = newstate;
+		write_unlock_bh(&dev_base_lock);
+		netdev_state_change(dev);
+	} else {
+		write_unlock_bh(&dev_base_lock);
+	}
+}
+
 int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 			   struct nlattr *tb[], struct nlattr *data[])
 {
@@ -1324,6 +1347,7 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 
 	list_add_tail_rcu(&vlan->list, &port->vlans);
 	netif_stacked_transfer_operstate(lowerdev, dev);
+	macvlan_set_operstate(lowerdev, dev);
 	linkwatch_fire_event(dev);
 
 	return 0;
@@ -1518,17 +1542,36 @@ static int macvlan_device_event(struct notifier_block *unused,
 	struct macvlan_port *port;
 	LIST_HEAD(list_kill);
 
-	if (!macvlan_port_exists(dev))
+	if (!macvlan_port_exists(dev) && !is_macvlan(dev))
+		return NOTIFY_DONE;
+
+	if (is_macvlan(dev)) {
+		vlan = netdev_priv(dev);
+
+		switch (event) {
+		case NETDEV_UP:
+		case NETDEV_DOWN:
+		case NETDEV_CHANGE:
+			netif_stacked_transfer_operstate(vlan->lowerdev,
+							 vlan->dev);
+			macvlan_set_operstate(vlan->lowerdev, vlan->dev);
+			break;
+		}
+
 		return NOTIFY_DONE;
+	}
 
 	port = macvlan_port_get_rtnl(dev);
 
 	switch (event) {
 	case NETDEV_UP:
+	case NETDEV_DOWN:
 	case NETDEV_CHANGE:
-		list_for_each_entry(vlan, &port->vlans, list)
+		list_for_each_entry(vlan, &port->vlans, list) {
 			netif_stacked_transfer_operstate(vlan->lowerdev,
 							 vlan->dev);
+			macvlan_set_operstate(vlan->lowerdev, vlan->dev);
+		}
 		break;
 	case NETDEV_FEAT_CHANGE:
 		list_for_each_entry(vlan, &port->vlans, list) {
-- 
2.8.0

Re: [PATCH net-next v2] macvlan: Support interface operstate properly

[Nikolay Aleksandrov]

On 04/07/2016 12:36 AM, Debabrata Banerjee wrote:
> Set appropriate macvlan interface status based on lower device and our
> status. Can be up, down, or lowerlayerdown.
What about dormant ?

> 
> de7d244d0 improved operstate by setting it from unknown to up, however
> it did not handle transferring down or lowerlayerdown.
No, this is not correct. It did not set it to "up", lowerlayerdown is currently
being set when the lower device is carrier-off, the only thing not handled is
the lower device going to admin down state. Up until this patch lowerlayerdown is
correctly propagated and set based on lower and macvlan device's carrier states,
after this patch you also include the device flags which makes things inconsistent
because you can overwrite the operstate set by link_watch (or user) afterwards, and
this is especially true for the dormant case. In fact you won't be able to set dormant
on a macvlan device, it'll always get overwritten.
Also another (minor) inconsistency is that user-space will no longer get IFF_RUNNING in the
flags by dev_get_flags() so when the lower device is down but carrier-on and the macvlan
is now in LOWERLAYERDOWN, so it will also show up as NO-CARRIER:
6: mac1@eth2: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000

But you can also see packets going through the macvlan device while it is in
the state above which is confusing.
# tcpdump -e -n -i mac1
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on mac1, link-type EN10MB (Ethernet), capture size 262144 bytes
12:08:37.330771 3a:83:f3:52:47:3b > ff:ff:ff:ff:ff:ff, ethertype ARP (0x0806), length 42: Request who-has 192.168.99.2 tell 192.168.99.1, length 28
12:08:38.332846 3a:83:f3:52:47:3b > ff:ff:ff:ff:ff:ff, ethertype ARP (0x0806), length 42: Request who-has 192.168.99.2 tell 192.168.99.1, length 28

That being said I understand the need to switch to lowerlayerdown when the lower
device is in "down", which is basically the most important change of this patch.
The rest is already handled by link watch based on carrier state. By now people
are used to having lowerlayerdown when there's no carrier, now it can also mean
that the lower device has been brought admin down.

Here's another interesting state:
6: mac1@eth2: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP,DORMANT,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
Prior to this patch the macvlan would stay in dormant state and it will also propagate
to devices stacked on top of it.

> 
> Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
> ---
> v2: Fix locking and update commit message
> 
>  drivers/net/macvlan.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 

Re: [PATCH net-next v2] macvlan: Support interface operstate properly

[Banerjee, Debabrata]

On 4/7/16, 7:05 AM, "Nikolay Aleksandrov" <nikolay@cumulusnetworks.com> wrote:



>On 04/07/2016 12:36 AM, Debabrata Banerjee wrote:
>> Set appropriate macvlan interface status based on lower device and our
>> status. Can be up, down, or lowerlayerdown.
>What about dormant ?
>
>That being said I understand the need to switch to lowerlayerdown when the lower
>device is in "down", which is basically the most important change of this patch.
>The rest is already handled by link watch based on carrier state. By now people
>are used to having lowerlayerdown when there's no carrier, now it can also mean
>that the lower device has been brought admin down.
>
>Here's another interesting state:
>6: mac1@eth2: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP,DORMANT,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
>Prior to this patch the macvlan would stay in dormant state and it will also propagate
>to devices stacked on top of it.

The no carrier issue gave me an idea. What if we just set no carrier on the macvlan device?
This seems appropriate, but it doesn't directly address the dormant issue, although that could
be a separate patch. Will this cause any new issues? It's very simple, new patch incoming.

[PATCH net-next] macvlan: Set nocarrier when lowerdev admin down

[Debabrata Banerjee]

When the lowerdev is set administratively down disable carrier on the
macvlan interface. This means operstate gets set properly instead of
still being "up".

Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
---
 drivers/net/macvlan.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 2bcf1f3..16d0e56 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1525,10 +1525,14 @@ static int macvlan_device_event(struct notifier_block *unused,
 
 	switch (event) {
 	case NETDEV_UP:
+	case NETDEV_DOWN:
 	case NETDEV_CHANGE:
-		list_for_each_entry(vlan, &port->vlans, list)
+		list_for_each_entry(vlan, &port->vlans, list) {
 			netif_stacked_transfer_operstate(vlan->lowerdev,
 							 vlan->dev);
+			if (!(vlan->lowerdev->flags & IFF_UP))
+				netif_carrier_off(vlan->dev);
+		}
 		break;
 	case NETDEV_FEAT_CHANGE:
 		list_for_each_entry(vlan, &port->vlans, list) {
-- 
2.8.0