Linux Ethernet Bridging - Mar 2018 - [Bridge] [PATCH net-next 0/2] net: bridge: MTU handling changes

Hi,
As previously discussed the recent changes break some setups and could lead
to packet drops. Thus the first patch reverts the behaviour for the bridge
to follow the minimum MTU but also keeps the ability to set the MTU to the
maximum (out of all ports) if vlan filtering is enabled. Patch 02 is the
bigger change in behaviour - we've always had trouble when configuring
bridges and their MTU which is auto tuning on port events
(add/del/changemtu), which means config software needs to chase it and fix
it after each such event, after patch 02 we allow the user to configure any
MTU (ETH_MIN/MAX limited) but once that is done the bridge stops auto
tuning and relies on the user to keep the MTU correct.
This should be compatible with cases that don't touch the MTU (or set it
to the same value), while allowing to configure the MTU and not worry
about it changing afterwards.

The patches are intentionally split like this, so that if they get accepted
and there are any complaints patch 02 can be reverted.

Thanks,
 Nik

Nikolay Aleksandrov (2):
  net: bridge: set min MTU on port events and allow user to set max
  net: bridge: disable bridge MTU auto tuning if it was set manually

 net/bridge/br.c         |  2 +-
 net/bridge/br_device.c  |  4 ++--
 net/bridge/br_if.c      | 49 ++++++++++++++++++++-----------------------------
 net/bridge/br_private.h |  3 ++-
 4 files changed, 25 insertions(+), 33 deletions(-)

-- 
2.11.0

Recently the bridge was changed to automatically set maximum MTU on port
events (add/del/changemtu) when vlan filtering is enabled, but that
actually changes behaviour in a way which breaks some setups and can lead
to packet drops. In order to still allow that maximum to be set while being
compatible, we add the ability for the user to tune the bridge MTU up to
the maximum when vlan filtering is enabled, but that has to be done
explicitly and all port events (add/del/changemtu) lead to resetting that
MTU to the minimum as before.

Suggested-by: Roopa Prabhu <roopa at cumulusnetworks.com>
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
 net/bridge/br.c         |  2 +-
 net/bridge/br_device.c  |  3 ++-
 net/bridge/br_if.c      | 43 ++++++++++++++-----------------------------
 net/bridge/br_private.h |  2 +-
 4 files changed, 18 insertions(+), 32 deletions(-)

diff --git a/net/bridge/br.c b/net/bridge/br.c
index 26e1616b2c90..565ff055813b 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -52,7 +52,7 @@ static int br_device_event(struct notifier_block *unused,
unsigned long event, v
 
 	switch (event) {
 	case NETDEV_CHANGEMTU:
-		dev_set_mtu(br->dev, br_mtu(br));
+		dev_set_mtu(br->dev, br_mtu(br, false));
 		break;
 
 	case NETDEV_CHANGEADDR:
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 278fc999d355..edb9967eb165 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -224,7 +224,8 @@ static void br_get_stats64(struct net_device *dev,
 static int br_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct net_bridge *br = netdev_priv(dev);
-	if (new_mtu > br_mtu(br))
+
+	if (new_mtu > br_mtu(br, br_vlan_enabled(dev)))
 		return -EINVAL;
 
 	dev->mtu = new_mtu;
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 87b2afd455c7..7d5dc5a91084 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -424,41 +424,26 @@ int br_del_bridge(struct net *net, const char *name)
 	return ret;
 }
 
-static bool min_mtu(int a, int b)
-{
-	return a < b ? 1 : 0;
-}
-
-static bool max_mtu(int a, int b)
-{
-	return a > b ? 1 : 0;
-}
-
-/* MTU of the bridge pseudo-device: ETH_DATA_LEN or the minimum of the ports */
-static int __br_mtu(const struct net_bridge *br, bool (compare_fn)(int, int))
+/* MTU of the bridge pseudo-device: ETH_DATA_LEN if there are no ports, the
+ * minimum of the ports if @max is false or the maximum if it's true
+ */
+int br_mtu(const struct net_bridge *br, bool max)
 {
 	const struct net_bridge_port *p;
-	int mtu = 0;
+	int ret_mtu = 0;
 
 	ASSERT_RTNL();
 
-	if (list_empty(&br->port_list))
-		mtu = ETH_DATA_LEN;
-	else {
-		list_for_each_entry(p, &br->port_list, list) {
-			if (!mtu || compare_fn(p->dev->mtu, mtu))
-				mtu = p->dev->mtu;
+	list_for_each_entry(p, &br->port_list, list) {
+		if (!max) {
+			if (!ret_mtu || ret_mtu > p->dev->mtu)
+				ret_mtu = p->dev->mtu;
+		} else if (p->dev->mtu > ret_mtu) {
+			ret_mtu = p->dev->mtu;
 		}
 	}
-	return mtu;
-}
 
-int br_mtu(const struct net_bridge *br)
-{
-	if (br_vlan_enabled(br->dev))
-		return __br_mtu(br, max_mtu);
-	else
-		return __br_mtu(br, min_mtu);
+	return ret_mtu ? ret_mtu : ETH_DATA_LEN;
 }
 
 static void br_set_gso_limits(struct net_bridge *br)
@@ -612,7 +597,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 	if (changed_addr)
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
 
-	dev_set_mtu(br->dev, br_mtu(br));
+	dev_set_mtu(br->dev, br_mtu(br, false));
 	br_set_gso_limits(br);
 
 	kobject_uevent(&p->kobj, KOBJ_ADD);
@@ -659,7 +644,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	 */
 	del_nbp(p);
 
-	dev_set_mtu(br->dev, br_mtu(br));
+	dev_set_mtu(br->dev, br_mtu(br, false));
 	br_set_gso_limits(br);
 
 	spin_lock_bh(&br->lock);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 048d5b51813b..586f84b9670d 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -578,7 +578,7 @@ int br_del_bridge(struct net *net, const char *name);
 int br_add_if(struct net_bridge *br, struct net_device *dev,
 	      struct netlink_ext_ack *extack);
 int br_del_if(struct net_bridge *br, struct net_device *dev);
-int br_mtu(const struct net_bridge *br);
+int br_mtu(const struct net_bridge *br, bool max);
 netdev_features_t br_features_recompute(struct net_bridge *br,
 					netdev_features_t features);
 void br_port_flags_change(struct net_bridge_port *port, unsigned long mask);
-- 
2.11.0

As Roopa noted today the biggest source of problems when configuring
bridge and ports is that the bridge MTU keeps changing automatically on
port events (add/del/changemtu). That leads to inconsistent behaviour
and network config software needs to chase the MTU and fix it on each
such event. Let's improve on that situation and allow for the user to
set any MTU within ETH_MIN/MAX limits, but once manually configured it
is the user's responsibility to keep it correct afterwards.

In case the MTU isn't manually set - the behaviour reverts to the
previous and the bridge follows the minimum MTU.

Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
 net/bridge/br.c         |  2 +-
 net/bridge/br_device.c  |  5 ++---
 net/bridge/br_if.c      | 36 +++++++++++++++++++++---------------
 net/bridge/br_private.h |  3 ++-
 4 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/net/bridge/br.c b/net/bridge/br.c
index 565ff055813b..671d13c10f6f 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -52,7 +52,7 @@ static int br_device_event(struct notifier_block *unused,
unsigned long event, v
 
 	switch (event) {
 	case NETDEV_CHANGEMTU:
-		dev_set_mtu(br->dev, br_mtu(br, false));
+		br_mtu_auto_adjust(br);
 		break;
 
 	case NETDEV_CHANGEADDR:
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index edb9967eb165..e682a668ce57 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -225,11 +225,10 @@ static int br_change_mtu(struct net_device *dev, int
new_mtu)
 {
 	struct net_bridge *br = netdev_priv(dev);
 
-	if (new_mtu > br_mtu(br, br_vlan_enabled(dev)))
-		return -EINVAL;
-
 	dev->mtu = new_mtu;
 
+	/* this flag will be cleared if the MTU was automatically adjusted */
+	br->mtu_set_by_user = true;
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	/* remember the MTU in the rtable for PMTU */
 	dst_metric_set(&br->fake_rtable.dst, RTAX_MTU, new_mtu);
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 7d5dc5a91084..82c1a6f430b3 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -424,28 +424,34 @@ int br_del_bridge(struct net *net, const char *name)
 	return ret;
 }
 
-/* MTU of the bridge pseudo-device: ETH_DATA_LEN if there are no ports, the
- * minimum of the ports if @max is false or the maximum if it's true
- */
-int br_mtu(const struct net_bridge *br, bool max)
+/* MTU of the bridge pseudo-device: ETH_DATA_LEN or the minimum of the ports */
+static int br_mtu_min(const struct net_bridge *br)
 {
 	const struct net_bridge_port *p;
 	int ret_mtu = 0;
 
-	ASSERT_RTNL();
-
-	list_for_each_entry(p, &br->port_list, list) {
-		if (!max) {
-			if (!ret_mtu || ret_mtu > p->dev->mtu)
-				ret_mtu = p->dev->mtu;
-		} else if (p->dev->mtu > ret_mtu) {
+	list_for_each_entry(p, &br->port_list, list)
+		if (!ret_mtu || ret_mtu > p->dev->mtu)
 			ret_mtu = p->dev->mtu;
-		}
-	}
 
 	return ret_mtu ? ret_mtu : ETH_DATA_LEN;
 }
 
+void br_mtu_auto_adjust(struct net_bridge *br)
+{
+	ASSERT_RTNL();
+
+	/* if the bridge MTU was manually configured don't mess with it */
+	if (br->mtu_set_by_user)
+		return;
+
+	/* change to the minimum MTU and clear the flag which was set by
+	 * the bridge ndo_change_mtu callback
+	 */
+	dev_set_mtu(br->dev, br_mtu_min(br));
+	br->mtu_set_by_user = false;
+}
+
 static void br_set_gso_limits(struct net_bridge *br)
 {
 	unsigned int gso_max_size = GSO_MAX_SIZE;
@@ -597,7 +603,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 	if (changed_addr)
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev);
 
-	dev_set_mtu(br->dev, br_mtu(br, false));
+	br_mtu_auto_adjust(br);
 	br_set_gso_limits(br);
 
 	kobject_uevent(&p->kobj, KOBJ_ADD);
@@ -644,7 +650,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	 */
 	del_nbp(p);
 
-	dev_set_mtu(br->dev, br_mtu(br, false));
+	br_mtu_auto_adjust(br);
 	br_set_gso_limits(br);
 
 	spin_lock_bh(&br->lock);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 586f84b9670d..a7cb3ece5031 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -410,6 +410,7 @@ struct net_bridge {
 	int offload_fwd_mark;
 #endif
 	bool				neigh_suppress_enabled;
+	bool				mtu_set_by_user;
 	struct hlist_head		fdb_list;
 };
 
@@ -578,7 +579,7 @@ int br_del_bridge(struct net *net, const char *name);
 int br_add_if(struct net_bridge *br, struct net_device *dev,
 	      struct netlink_ext_ack *extack);
 int br_del_if(struct net_bridge *br, struct net_device *dev);
-int br_mtu(const struct net_bridge *br, bool max);
+void br_mtu_auto_adjust(struct net_bridge *br);
 netdev_features_t br_features_recompute(struct net_bridge *br,
 					netdev_features_t features);
 void br_port_flags_change(struct net_bridge_port *port, unsigned long mask);
-- 
2.11.0

From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
Date: Fri, 30 Mar 2018 13:46:17 +0300
> As previously discussed the recent changes break some setups and could lead
> to packet drops. Thus the first patch reverts the behaviour for the bridge
> to follow the minimum MTU but also keeps the ability to set the MTU to the
> maximum (out of all ports) if vlan filtering is enabled. Patch 02 is the
> bigger change in behaviour - we've always had trouble when configuring
> bridges and their MTU which is auto tuning on port events
> (add/del/changemtu), which means config software needs to chase it and fix
> it after each such event, after patch 02 we allow the user to configure any
> MTU (ETH_MIN/MAX limited) but once that is done the bridge stops auto
> tuning and relies on the user to keep the MTU correct.
> This should be compatible with cases that don't touch the MTU (or set
it
> to the same value), while allowing to configure the MTU and not worry
> about it changing afterwards.
> 
> The patches are intentionally split like this, so that if they get accepted
> and there are any complaints patch 02 can be reverted.
Series applied, thanks.