Vladimir Oltean
2023-Jan-09 11:56 UTC
[Bridge] [PATCH v5 net-next 01/15] net: bridge: mst: Multiple Spanning Tree (MST) mode
On Mon, Jan 09, 2023 at 01:43:46PM +0200, Ido Schimmel wrote:> OK, thanks for confirming. Will send a patch later this week if Tobias > won't take care of it by then. First patch will probably be [1] to make > sure we dump the correct MST state to user space. It will also make it > easier to show the problem and validate the fix. > > [1] > diff --git a/net/bridge/br.c b/net/bridge/br.c > index 4f5098d33a46..f02a1ad589de 100644 > --- a/net/bridge/br.c > +++ b/net/bridge/br.c > @@ -286,7 +286,7 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt) > case BR_BOOLOPT_MCAST_VLAN_SNOOPING: > return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED); > case BR_BOOLOPT_MST_ENABLE: > - return br_opt_get(br, BROPT_MST_ENABLED); > + return br_mst_is_enabled(br);Well, this did report the correct MST state despite the incorrect static branch state, no? The users of br_mst_is_enabled(br) are broken, not those of br_opt_get(br, BROPT_MST_ENABLED). Anyway, I see there's a br_mst_is_enabled() and also a br_mst_enabled()?! One is used in the fast path and the other in the slow path. They should probably be merged, I guess. They both exist probably because somebody thought that the "if (!netif_is_bridge_master(dev))" test is redundant in the fast path.> default: > /* shouldn't be called with unsupported options */ > WARN_ON(1); > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 75aff9bbf17e..7f0475f62d45 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -1827,7 +1827,7 @@ static inline bool br_vlan_state_allowed(u8 state, bool learn_allow) > /* br_mst.c */ > #ifdef CONFIG_BRIDGE_VLAN_FILTERING > DECLARE_STATIC_KEY_FALSE(br_mst_used); > -static inline bool br_mst_is_enabled(struct net_bridge *br) > +static inline bool br_mst_is_enabled(const struct net_bridge *br) > { > return static_branch_unlikely(&br_mst_used) && > br_opt_get(br, BROPT_MST_ENABLED); > @@ -1845,7 +1845,7 @@ int br_mst_fill_info(struct sk_buff *skb, > int br_mst_process(struct net_bridge_port *p, const struct nlattr *mst_attr, > struct netlink_ext_ack *extack); > #else > -static inline bool br_mst_is_enabled(struct net_bridge *br) > +static inline bool br_mst_is_enabled(const struct net_bridge *br) > { > return false; > }
Ido Schimmel
2023-Jan-09 12:20 UTC
[Bridge] [PATCH v5 net-next 01/15] net: bridge: mst: Multiple Spanning Tree (MST) mode
On Mon, Jan 09, 2023 at 01:56:53PM +0200, Vladimir Oltean wrote:> On Mon, Jan 09, 2023 at 01:43:46PM +0200, Ido Schimmel wrote: > > OK, thanks for confirming. Will send a patch later this week if Tobias > > won't take care of it by then. First patch will probably be [1] to make > > sure we dump the correct MST state to user space. It will also make it > > easier to show the problem and validate the fix. > > > > [1] > > diff --git a/net/bridge/br.c b/net/bridge/br.c > > index 4f5098d33a46..f02a1ad589de 100644 > > --- a/net/bridge/br.c > > +++ b/net/bridge/br.c > > @@ -286,7 +286,7 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt) > > case BR_BOOLOPT_MCAST_VLAN_SNOOPING: > > return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED); > > case BR_BOOLOPT_MST_ENABLE: > > - return br_opt_get(br, BROPT_MST_ENABLED); > > + return br_mst_is_enabled(br); > > Well, this did report the correct MST state despite the incorrect static > branch state, no? The users of br_mst_is_enabled(br) are broken, not > those of br_opt_get(br, BROPT_MST_ENABLED).I should have said "actual"/"effective" instead of "correct". IMO, it's better to use the same conditional in the both the data and control paths to eliminate discrepancies. Without the patch, a user will see that MST is supposedly enabled when it is actually disabled in the data path.> > Anyway, I see there's a br_mst_is_enabled() and also a br_mst_enabled()?! > One is used in the fast path and the other in the slow path. They should > probably be merged, I guess. They both exist probably because somebody > thought that the "if (!netif_is_bridge_master(dev))" test is redundant > in the fast path.The single user of br_mst_enabled() (DSA) is not affected by the bug (only the SW data path is), so I suggest making this consolidation in net-next after the bug is fixed. OK?> > > default: > > /* shouldn't be called with unsupported options */ > > WARN_ON(1); > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > > index 75aff9bbf17e..7f0475f62d45 100644 > > --- a/net/bridge/br_private.h > > +++ b/net/bridge/br_private.h > > @@ -1827,7 +1827,7 @@ static inline bool br_vlan_state_allowed(u8 state, bool learn_allow) > > /* br_mst.c */ > > #ifdef CONFIG_BRIDGE_VLAN_FILTERING > > DECLARE_STATIC_KEY_FALSE(br_mst_used); > > -static inline bool br_mst_is_enabled(struct net_bridge *br) > > +static inline bool br_mst_is_enabled(const struct net_bridge *br) > > { > > return static_branch_unlikely(&br_mst_used) && > > br_opt_get(br, BROPT_MST_ENABLED); > > @@ -1845,7 +1845,7 @@ int br_mst_fill_info(struct sk_buff *skb, > > int br_mst_process(struct net_bridge_port *p, const struct nlattr *mst_attr, > > struct netlink_ext_ack *extack); > > #else > > -static inline bool br_mst_is_enabled(struct net_bridge *br) > > +static inline bool br_mst_is_enabled(const struct net_bridge *br) > > { > > return false; > > }