[PATCH net] net: bridge: mst: Check vlan state for egress decision

[PATCH net] net: bridge: mst: Check vlan state for egress decision

[Elliot Ayrey]

If a port is blocking in the common instance but forwarding in an MST
instance, traffic egressing the bridge will be dropped because the
state of the common instance is overriding that of the MST instance.

Fix this by temporarily forcing the port state to forwarding when in
MST mode to allow checking the vlan state via br_allowed_egress().
This is similar to what happens in br_handle_frame_finish() when
checking ingress traffic, which was introduced in the change below.

Fixes: ec7328b59176 ("net: bridge: mst: Multiple Spanning Tree (MST) mode")
Signed-off-by: Elliot Ayrey <elliot.ayrey@alliedtelesis.co.nz>
---
 net/bridge/br_forward.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index d97064d460dc..911b37a38a32 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -22,10 +22,16 @@ static inline int should_deliver(const struct net_bridge_port *p,
 				 const struct sk_buff *skb)
 {
 	struct net_bridge_vlan_group *vg;
+	u8 state;
+
+	if (br_mst_is_enabled(p->br))
+		state = BR_STATE_FORWARDING;
+	else
+		state = p->state;
 
 	vg = nbp_vlan_group_rcu(p);
 	return ((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
-		p->state == BR_STATE_FORWARDING && br_allowed_egress(vg, skb) &&
+		state == BR_STATE_FORWARDING && br_allowed_egress(vg, skb) &&
 		nbp_switchdev_allowed_egress(p, skb) &&
 		!br_skb_isolated(p, skb);
 }

Re: [PATCH net] net: bridge: mst: Check vlan state for egress decision

[Nikolay Aleksandrov]

On 05/07/2024 06:00, Elliot Ayrey wrote:
> If a port is blocking in the common instance but forwarding in an MST
> instance, traffic egressing the bridge will be dropped because the
> state of the common instance is overriding that of the MST instance.
> 
> Fix this by temporarily forcing the port state to forwarding when in
> MST mode to allow checking the vlan state via br_allowed_egress().
> This is similar to what happens in br_handle_frame_finish() when
> checking ingress traffic, which was introduced in the change below.
> 
> Fixes: ec7328b59176 ("net: bridge: mst: Multiple Spanning Tree (MST) mode")
> Signed-off-by: Elliot Ayrey <elliot.ayrey@alliedtelesis.co.nz>
> ---
>  net/bridge/br_forward.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index d97064d460dc..911b37a38a32 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -22,10 +22,16 @@ static inline int should_deliver(const struct net_bridge_port *p,
>  				 const struct sk_buff *skb)
>  {
>  	struct net_bridge_vlan_group *vg;
> +	u8 state;

state = p->state
...

> +
> +	if (br_mst_is_enabled(p->br))
> +		state = BR_STATE_FORWARDING;

...
and you can drop the else clause

> +	else
> +		state = p->state;
>  
>  	vg = nbp_vlan_group_rcu(p);
>  	return ((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
> -		p->state == BR_STATE_FORWARDING && br_allowed_egress(vg, skb) &&
> +		state == BR_STATE_FORWARDING && br_allowed_egress(vg, skb) &&
>  		nbp_switchdev_allowed_egress(p, skb) &&
>  		!br_skb_isolated(p, skb);
>  }

Re: [PATCH net] net: bridge: mst: Check vlan state for egress decision

[Tobias Waldekranz]

On fre, jul 05, 2024 at 15:00, Elliot Ayrey <elliot.ayrey@alliedtelesis.co.nz> wrote:
> If a port is blocking in the common instance but forwarding in an MST
> instance, traffic egressing the bridge will be dropped because the
> state of the common instance is overriding that of the MST instance.

Can't believe I missed this - thanks!

> Fix this by temporarily forcing the port state to forwarding when in
> MST mode to allow checking the vlan state via br_allowed_egress().
> This is similar to what happens in br_handle_frame_finish() when
> checking ingress traffic, which was introduced in the change below.
>
> Fixes: ec7328b59176 ("net: bridge: mst: Multiple Spanning Tree (MST) mode")
> Signed-off-by: Elliot Ayrey <elliot.ayrey@alliedtelesis.co.nz>
> ---
>  net/bridge/br_forward.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index d97064d460dc..911b37a38a32 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -22,10 +22,16 @@ static inline int should_deliver(const struct net_bridge_port *p,
>  				 const struct sk_buff *skb)
>  {
>  	struct net_bridge_vlan_group *vg;
> +	u8 state;
> +
> +	if (br_mst_is_enabled(p->br))
> +		state = BR_STATE_FORWARDING;
> +	else
> +		state = p->state;
>  
>  	vg = nbp_vlan_group_rcu(p);
>  	return ((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&

I think it might read a bit better if we model it like the hairpin check
above. I.e. (special_mode || regular_condition)

It's not really that the state is forwarding when mst is enabled, we
simply ignore the port-global state in that case.

> -		p->state == BR_STATE_FORWARDING && br_allowed_egress(vg, skb) &&
> +		state == BR_STATE_FORWARDING && br_allowed_egress(vg, skb) &&

so something like:

    ...
    (br_mst_is_enabled(p->br) || p->state == BR_STATE_FORWARDING) &&
    br_allowed_egress(vg, skb) && nbp_switchdev_allowed_egress(p, skb) &&
    ...

>  		nbp_switchdev_allowed_egress(p, skb) &&
>  		!br_skb_isolated(p, skb);
>  }

Re: [PATCH net] net: bridge: mst: Check vlan state for egress decision

[Elliot Ayrey]

On Sat, 2024-07-06 at 00:00 +0200, Tobias Waldekranz wrote:
> I think it might read a bit better if we model it like the hairpin check
> above. I.e. (special_mode || regular_condition)
> 
> It's not really that the state is forwarding when mst is enabled, we
> simply ignore the port-global state in that case.
> 
> > -		p->state == BR_STATE_FORWARDING && br_allowed_egress(vg, skb) &&
> > +		state == BR_STATE_FORWARDING && br_allowed_egress(vg, skb) &&
> 
> so something like:
> 
>     ...
>     (br_mst_is_enabled(p->br) || p->state == BR_STATE_FORWARDING) &&
>     br_allowed_egress(vg, skb) && nbp_switchdev_allowed_egress(p, skb) &&
>     ...

Yes you're right, that's much clearer. I'll go with that.