bridge: fix regression in ageing time
Subsystems (3)
PATCH 1/1 bridge: fix regression in ageing time
This fixes a regression in the bridge ageing time caused by:
commit c62987bbd8a1 ("bridge: push bridge setting ageing_time down to switchdev")
There are users of Linux bridge which use the feature that if ageing time is set to 0 it causes entries to never expire.
This feature is even listed on the web page:
https://www.linuxfoundation.org/collaborate/workgroups/networking/bridge
For a pure software bridge, it is unnecessary for the code to have arbitrary restrictions on what values are allowable.
---
Please apply to 4.4 for stable as well.
include/linux/if_bridge.h | 5 ----- net/bridge/br_stp.c | 9 ++++++--- 2 files changed, 6 insertions(+), 8 deletions(-)
include/linux/if_bridge.h +0 -5
@@ -45,11 +45,6 @@ struct br_ip_list {#define BR_PROXYARP BIT(8)#define BR_LEARNING_SYNC BIT(9)#define BR_PROXYARP_WIFI BIT(10) - -/* values as per ieee8021QBridgeFdbAgingTime */ -#define BR_MIN_AGEING_TIME (10 * HZ) -#define BR_MAX_AGEING_TIME (1000000 * HZ) -#define BR_DEFAULT_AGEING_TIME (300 * HZ)extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *));
net/bridge/br_stp.c +6 -3
@@ -568,6 +568,12 @@ int br_set_max_age(struct net_bridge *br } +/* Set the how long dynamic forwarding database entries live. +* The Linux bridge allows values outside the standard 802.1 +* specification to allow for special cases: +* 0 - means entry never age. +* 1 - means entries disappear in next clock tick +*/ int br_set_ageing_time (struct net_bridge *br,u32 ageing_time) {struct switchdev_attr attr = {
@@ -579,9 +585,6 @@ int br_set_ageing_time(struct net_bridgeunsigned long t =clock_t_to_jiffies (ageing_time);int err; -if (t <BR_MIN_AGEING_TIME || t >BR_MAX_AGEING_TIME ) -return -ERANGE ; - err =switchdev_port_attr_set (br->dev, &attr);if (err)return err;
Fri, Feb 12, 2016 at 11:31:09PM IST, [email protected] wrote:
Hi,
Quoted 24 lines
This fixes a regression in the bridge ageing time caused by:
commit
c62987bbd8a1("bridge: push bridge settingageing_timedown to switchdev")There are users of Linux bridge which use the feature that if ageing time is set to 0 it causes entries to never expire.
This feature is even listed on the web page:
https://www.linuxfoundation.org/collaborate/workgroups/networking/bridgeFor a pure software bridge, it is unnecessary for the code to have arbitrary restrictions on what values are allowable.
Signed-off-by Stephen Hemminger <[email protected]>---
Please apply to 4.4 for stable as well.include/linux/if_bridge.h | 5 ----- net/bridge/br_stp.c | 9 ++++++--- 2 files changed, 6 insertions(+), 8 deletions(-)
include/linux/if_bridge.h struct br_ip_list { · line 50
Quoted 6 lines
include/linux/if_bridge.h +0
-3@@ -45,11 +45,6 @@ struct br_ip_list {#define BR_PROXYARP BIT(8)#define BR_LEARNING_SYNC BIT(9)#define BR_PROXYARP_WIFI BIT(10) - -/* values as per ieee8021QBridgeFdbAgingTime */ -#define BR_MIN_AGEING_TIME (10 * HZ)
rocker is using this value:
vi drivers/net/ethernet/rocker/rocker.c +3707
net/bridge/br_stp.c int br_set_max_age(struct net_bridge *br · line 571
Quoted 12 lines
include/linux/if_bridge.h +0
-2-#define BR_MAX_AGEING_TIME (1000000 * HZ) -#define BR_DEFAULT_AGEING_TIME (300 * HZ)extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *));net/bridge/br_stp.c +1
-0@@ -568,6 +568,12 @@ int br_set_max_age(struct net_bridge *br } +/* Set the how long dynamic forwarding database entries live.
Maybe drop the 'the'?
net/bridge/br_stp.c int br_set_max_age(struct net_bridge *br · line 574
Quoted 3 lines
net/bridge/br_stp.c +3
-0+* The Linux bridge allows values outside the standard 802.1 +* specification to allow for special cases: +* 0 - means entry never age.
s/entry/entries/ ?
net/bridge/br_stp.c int br_set_max_age(struct net_bridge *br · line 575
Quoted 1 line
net/bridge/br_stp.c +1
-0+* 1 - means entries disappear in next clock tick
Missing dot. ;)
net/bridge/br_stp.c int br_set_ageing_time(struct net_bridge · line 583
Quoted 10 lines
net/bridge/br_stp.c +1
-2+*/ int br_set_ageing_time (struct net_bridge *br,u32 ageing_time) {struct switchdev_attr attr = {@@ -579,9 +585,6 @@ int br_set_ageing_time(struct net_bridgeunsigned long t =clock_t_to_jiffies (ageing_time);int err; -if (t <BR_MIN_AGEING_TIME || t >BR_MAX_AGEING_TIME ) -return -ERANGE ;
Removing this and trying to set ageing time to 0 will cause mlxsw driver to error out (firmware is checking for min value). I will send you a patch for both rocker and mlxsw tomorrow, so that you can squash to yours and do this change atomically.
Thanks for looking into this.
Quoted 4 lines
net/bridge/br_stp.c +0
-1- err =switchdev_port_attr_set (br->dev, &attr);if (err)return err;
Quoted 38 lines · Fri, Feb 12, 2016 at 11:31:09PM IST, [email protected] wrote:
This fixes a regression in the bridge ageing time caused by:
commit
c62987bbd8a1("bridge: push bridge settingageing_timedown to switchdev")There are users of Linux bridge which use the feature that if ageing time is set to 0 it causes entries to never expire.
This feature is even listed on the web page:
https://www.linuxfoundation.org/collaborate/workgroups/networking/bridgeFor a pure software bridge, it is unnecessary for the code to have arbitrary restrictions on what values are allowable.
Signed-off-by Stephen Hemminger <[email protected]>---
Please apply to 4.4 for stable as well.include/linux/if_bridge.h | 5 ----- net/bridge/br_stp.c | 9 ++++++--- 2 files changed, 6 insertions(+), 8 deletions(-)include/linux/if_bridge.h +0
-5@@ -45,11 +45,6 @@ struct br_ip_list {#define BR_PROXYARP BIT(8)#define BR_LEARNING_SYNC BIT(9)#define BR_PROXYARP_WIFI BIT(10) - -/* values as per ieee8021QBridgeFdbAgingTime */ -#define BR_MIN_AGEING_TIME (10 * HZ) -#define BR_MAX_AGEING_TIME (1000000 * HZ) -#define BR_DEFAULT_AGEING_TIME (300 * HZ)extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *));
net/bridge/br_stp.c int br_set_max_age(struct net_bridge *br · line 574
Quoted 7 lines
net/bridge/br_stp.c +4
-0@@ -568,6 +568,12 @@ int br_set_max_age(struct net_bridge *br } +/* Set the how long dynamic forwarding database entries live. +* The Linux bridge allows values outside the standard 802.1 +* specification to allow for special cases: +* 0 - means entry never age.
Looking at the code I didn't quite understand how this could happen and indeed with your patch I see that the FDB is never populated. Maybe that's the expected behavior? I also found the following, which seems to support this:
https://lists.linuxfoundation.org/pipermail/bridge/2010-November/007410.html
Thanks.
Quoted 15 lines
net/bridge/br_stp.c +2
-3+* 1 - means entries disappear in next clock tick +*/ int br_set_ageing_time (struct net_bridge *br,u32 ageing_time) {struct switchdev_attr attr = {@@ -579,9 +585,6 @@ int br_set_ageing_time(struct net_bridgeunsigned long t =clock_t_to_jiffies (ageing_time);int err; -if (t <BR_MIN_AGEING_TIME || t >BR_MAX_AGEING_TIME ) -return -ERANGE ; - err =switchdev_port_attr_set (br->dev, &attr);if (err)return err;
Quoted 51 lines · Fri, Feb 12, 2016 at 10:31:09PM CET, [email protected] wrote:
This fixes a regression in the bridge ageing time caused by:
commit
c62987bbd8a1("bridge: push bridge settingageing_timedown to switchdev")There are users of Linux bridge which use the feature that if ageing time is set to 0 it causes entries to never expire.
This feature is even listed on the web page:
https://www.linuxfoundation.org/collaborate/workgroups/networking/bridgeFor a pure software bridge, it is unnecessary for the code to have arbitrary restrictions on what values are allowable.
Signed-off-by Stephen Hemminger <[email protected]>---
Please apply to 4.4 for stable as well.include/linux/if_bridge.h | 5 ----- net/bridge/br_stp.c | 9 ++++++--- 2 files changed, 6 insertions(+), 8 deletions(-)include/linux/if_bridge.h +0
-5@@ -45,11 +45,6 @@ struct br_ip_list {#define BR_PROXYARP BIT(8)#define BR_LEARNING_SYNC BIT(9)#define BR_PROXYARP_WIFI BIT(10) - -/* values as per ieee8021QBridgeFdbAgingTime */ -#define BR_MIN_AGEING_TIME (10 * HZ) -#define BR_MAX_AGEING_TIME (1000000 * HZ) -#define BR_DEFAULT_AGEING_TIME (300 * HZ)extern void brioctl_set (int (*ioctl_hook)(struct net *,unsigned int ,void __user *));net/bridge/br_stp.c +6
-0@@ -568,6 +568,12 @@ int br_set_max_age(struct net_bridge *br } +/* Set the how long dynamic forwarding database entries live. +* The Linux bridge allows values outside the standard 802.1 +* specification to allow for special cases: +* 0 - means entry never age. +* 1 - means entries disappear in next clock tick +*/ int br_set_ageing_time (struct net_bridge *br,u32 ageing_time) {struct switchdev_attr attr = {
net/bridge/br_stp.c int br_set_ageing_time(struct net_bridge · line 584
Quoted 6 lines
net/bridge/br_stp.c +0
-3@@ -579,9 +585,6 @@ int br_set_ageing_time(struct net_bridgeunsigned long t =clock_t_to_jiffies (ageing_time);int err; -if (t <BR_MIN_AGEING_TIME || t >BR_MAX_AGEING_TIME ) -return -ERANGE ; -
rocker and mlxsw rely on this check. If you remove it here, you have to add it there.
Quoted 3 lines
net/bridge/br_stp.c +0
-0err =switchdev_port_attr_set (br->dev, &attr);if (err)return err;