[PATCH RFC net-next 00/13] net: bridge: forwarding of unknown IPv4/IPv6/MAC BUM traffic

[PATCH RFC net-next 00/13] net: bridge: forwarding of unknown IPv4/IPv6/MAC BUM traffic

[Joachim Wiberg]

This is a proposal to improve forwarding control of BUM traffic to the
bridge itself.  Another, related, issue regarding loss of function when
an IP multicast router is detected, is also brought up here which can be
a separate series in v2.

First, we add BROPT_BCAST_FLOOD, BROPT_UNICAST_FLOOD, BROPT_MCAST_FLOOD
flags for unknown traffic to the bridge itself, with netlink support and
a selftest.  We ensure backwards compatible forwarding behavior is
preserved by enabling these flags by default.  Please note however,
these flags do not affect the behavior of IFF_PROMISC on the bridge
interface.

Second, and with the above in place, we set out to verify flooding of
unknown *and* known multicast to regular bridge ports, including the
bridge itself.  We use `tcpdump -p` in the tests to ensure we do not
trigger IFF_PROIMISC.  Unknown multicast should be forwarded according
to the MCAST_FLOOD flag, and known multicast according the MDB and to
multicast router ports.

We find that forwarding of unknown IP multicast stops[1] as soon as a
multicast router is known.  Affecting all ports, including the bridge
itself (this series).  The root cause for this is the classification
`mrouters_only` in br_multicast_rcv().

Dropping this classification of unknown IP multicast and moving the
multicast flow handling from br_multicast_flood() to br_flood() fixes
this problem.  The bridge now properly floods all unknown multicast, and
this can now be controlled using the MCAST_FLOOD flag on all ports
including the bridge itself.

The use of br_flood() opens up the need for the multicast 'rport' API,
but that is behind CONFIG_BRIDGE_IGMP_SNOOPING, which is the primary
reason for this being an RFC series.  I'd love some feedback on how to
go about all this, opening up that API and even the take on the issue as
a whole.

Honestly, despite taking great care to not change the bridge's default
behavior the patch series in itself propose quite radical changes that
alone mandate RFC status at this point.  There has been some discussion
already on this in 20220410220324.4c3l3idubwi3w6if@skbuf and I expect
more disucssion here.

Note: this series builds upon my previous patch for host l2 mdb entries,
      20220411084054.298807-1-troglobit@gmail.com, extending the test
      bridge_mdb.sh

Best regards
 /Joachim

[1]: MAC multicast is not affected.

net/bridge/br_device.c                        |   4 +
 net/bridge/br_forward.c                       |  11 +
 net/bridge/br_input.c                         |  11 +-
 net/bridge/br_multicast.c                     |   6 +-
 net/bridge/br_netlink.c                       | 170 +++++++---
 net/bridge/br_private.h                       |   7 +-
 net/bridge/br_switchdev.c                     |   8 +-
 net/bridge/br_sysfs_if.c                      |   2 +-
 .../drivers/net/ocelot/tc_flower_chains.sh    |  24 +-
 .../testing/selftests/net/forwarding/Makefile |   3 +-
 .../selftests/net/forwarding/bridge_flood.sh  | 170 ++++++++++
 .../selftests/net/forwarding/bridge_mdb.sh    | 321 +++++++++++++++++-
 tools/testing/selftests/net/forwarding/lib.sh |  33 +-
 13 files changed, 683 insertions(+), 87 deletions(-)
 create mode 100755 tools/testing/selftests/net/forwarding/bridge_flood.sh
 mode change 100644 => 100755 tools/testing/selftests/net/forwarding/lib.sh

[Bridge] [PATCH RFC net-next 00/13] net: bridge: forwarding of unknown IPv4/IPv6/MAC BUM traffic

[Joachim Wiberg]

This is a proposal to improve forwarding control of BUM traffic to the
bridge itself.  Another, related, issue regarding loss of function when
an IP multicast router is detected, is also brought up here which can be
a separate series in v2.

First, we add BROPT_BCAST_FLOOD, BROPT_UNICAST_FLOOD, BROPT_MCAST_FLOOD
flags for unknown traffic to the bridge itself, with netlink support and
a selftest.  We ensure backwards compatible forwarding behavior is
preserved by enabling these flags by default.  Please note however,
these flags do not affect the behavior of IFF_PROMISC on the bridge
interface.

Second, and with the above in place, we set out to verify flooding of
unknown *and* known multicast to regular bridge ports, including the
bridge itself.  We use `tcpdump -p` in the tests to ensure we do not
trigger IFF_PROIMISC.  Unknown multicast should be forwarded according
to the MCAST_FLOOD flag, and known multicast according the MDB and to
multicast router ports.

We find that forwarding of unknown IP multicast stops[1] as soon as a
multicast router is known.  Affecting all ports, including the bridge
itself (this series).  The root cause for this is the classification
`mrouters_only` in br_multicast_rcv().

Dropping this classification of unknown IP multicast and moving the
multicast flow handling from br_multicast_flood() to br_flood() fixes
this problem.  The bridge now properly floods all unknown multicast, and
this can now be controlled using the MCAST_FLOOD flag on all ports
including the bridge itself.

The use of br_flood() opens up the need for the multicast 'rport' API,
but that is behind CONFIG_BRIDGE_IGMP_SNOOPING, which is the primary
reason for this being an RFC series.  I'd love some feedback on how to
go about all this, opening up that API and even the take on the issue as
a whole.

Honestly, despite taking great care to not change the bridge's default
behavior the patch series in itself propose quite radical changes that
alone mandate RFC status at this point.  There has been some discussion
already on this in 20220410220324.4c3l3idubwi3w6if@skbuf and I expect
more disucssion here.

Note: this series builds upon my previous patch for host l2 mdb entries,
      20220411084054.298807-1-troglobit@gmail.com, extending the test
      bridge_mdb.sh

Best regards
 /Joachim

[1]: MAC multicast is not affected.

net/bridge/br_device.c                        |   4 +
 net/bridge/br_forward.c                       |  11 +
 net/bridge/br_input.c                         |  11 +-
 net/bridge/br_multicast.c                     |   6 +-
 net/bridge/br_netlink.c                       | 170 +++++++---
 net/bridge/br_private.h                       |   7 +-
 net/bridge/br_switchdev.c                     |   8 +-
 net/bridge/br_sysfs_if.c                      |   2 +-
 .../drivers/net/ocelot/tc_flower_chains.sh    |  24 +-
 .../testing/selftests/net/forwarding/Makefile |   3 +-
 .../selftests/net/forwarding/bridge_flood.sh  | 170 ++++++++++
 .../selftests/net/forwarding/bridge_mdb.sh    | 321 +++++++++++++++++-
 tools/testing/selftests/net/forwarding/lib.sh |  33 +-
 13 files changed, 683 insertions(+), 87 deletions(-)
 create mode 100755 tools/testing/selftests/net/forwarding/bridge_flood.sh
 mode change 100644 => 100755 tools/testing/selftests/net/forwarding/lib.sh

[PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Joachim Wiberg]

The bridge itself is also a port, but unfortunately it does not (yet)
have a 'struct net_bridge_port'.  However, in many cases we want to
treat it as a proper port so concessions have been made, e.g., NULL
port or host_joined attributes.

This patch is an attempt to more of the same by adding support for
controlling flooding of unknown broadcast/unicast/multicast to the
bridge.  Something we often also want to control in an offloaded
switching fabric.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_device.c  |  4 ++++
 net/bridge/br_input.c   | 11 ++++++++---
 net/bridge/br_private.h |  3 +++
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 8d6bab244c4a..0aa7d21ac82c 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
 	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
 	dev->max_mtu = ETH_MAX_MTU;
 
+	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);
+	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
+	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);
+
 	br_netfilter_rtable_init(br);
 	br_stp_timer_init(br);
 	br_multicast_init(br);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 196417859c4a..d439b876bdf5 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		/* by definition the broadcast is also a multicast address */
 		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
 			pkt_type = BR_PKT_BROADCAST;
-			local_rcv = true;
+			if (br_opt_get(br, BROPT_BCAST_FLOOD))
+				local_rcv = true;
 		} else {
 			pkt_type = BR_PKT_MULTICAST;
 			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
@@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 			}
 			mcast_hit = true;
 		} else {
-			local_rcv = true;
-			br->dev->stats.multicast++;
+			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
+				local_rcv = true;
+				br->dev->stats.multicast++;
+			}
 		}
 		break;
 	case BR_PKT_UNICAST:
 		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
+		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
+			local_rcv = true;
 		break;
 	default:
 		break;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 18ccc3d5d296..683bd0ee4c64 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -449,6 +449,9 @@ enum net_bridge_opts {
 	BROPT_VLAN_BRIDGE_BINDING,
 	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
 	BROPT_MST_ENABLED,
+	BROPT_UNICAST_FLOOD,
+	BROPT_MCAST_FLOOD,
+	BROPT_BCAST_FLOOD,
 };
 
 struct net_bridge {
-- 
2.25.1

[Bridge] [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Joachim Wiberg]

The bridge itself is also a port, but unfortunately it does not (yet)
have a 'struct net_bridge_port'.  However, in many cases we want to
treat it as a proper port so concessions have been made, e.g., NULL
port or host_joined attributes.

This patch is an attempt to more of the same by adding support for
controlling flooding of unknown broadcast/unicast/multicast to the
bridge.  Something we often also want to control in an offloaded
switching fabric.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_device.c  |  4 ++++
 net/bridge/br_input.c   | 11 ++++++++---
 net/bridge/br_private.h |  3 +++
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 8d6bab244c4a..0aa7d21ac82c 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
 	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
 	dev->max_mtu = ETH_MAX_MTU;
 
+	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);
+	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
+	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);
+
 	br_netfilter_rtable_init(br);
 	br_stp_timer_init(br);
 	br_multicast_init(br);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 196417859c4a..d439b876bdf5 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		/* by definition the broadcast is also a multicast address */
 		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
 			pkt_type = BR_PKT_BROADCAST;
-			local_rcv = true;
+			if (br_opt_get(br, BROPT_BCAST_FLOOD))
+				local_rcv = true;
 		} else {
 			pkt_type = BR_PKT_MULTICAST;
 			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
@@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 			}
 			mcast_hit = true;
 		} else {
-			local_rcv = true;
-			br->dev->stats.multicast++;
+			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
+				local_rcv = true;
+				br->dev->stats.multicast++;
+			}
 		}
 		break;
 	case BR_PKT_UNICAST:
 		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
+		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
+			local_rcv = true;
 		break;
 	default:
 		break;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 18ccc3d5d296..683bd0ee4c64 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -449,6 +449,9 @@ enum net_bridge_opts {
 	BROPT_VLAN_BRIDGE_BINDING,
 	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
 	BROPT_MST_ENABLED,
+	BROPT_UNICAST_FLOOD,
+	BROPT_MCAST_FLOOD,
+	BROPT_BCAST_FLOOD,
 };
 
 struct net_bridge {
-- 
2.25.1

Re: [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Nikolay Aleksandrov]

On 11/04/2022 16:38, Joachim Wiberg wrote:
> The bridge itself is also a port, but unfortunately it does not (yet)
> have a 'struct net_bridge_port'.  However, in many cases we want to
> treat it as a proper port so concessions have been made, e.g., NULL
> port or host_joined attributes.
> 
> This patch is an attempt to more of the same by adding support for
> controlling flooding of unknown broadcast/unicast/multicast to the
> bridge.  Something we often also want to control in an offloaded
> switching fabric.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  net/bridge/br_device.c  |  4 ++++
>  net/bridge/br_input.c   | 11 ++++++++---
>  net/bridge/br_private.h |  3 +++
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 8d6bab244c4a..0aa7d21ac82c 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>  	dev->max_mtu = ETH_MAX_MTU;
>  
> +	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);

This one must be false by default. It changes current default behaviour.
Unknown unicast is not currently passed up to the bridge if the port is
not in promisc mode, this will change it. You'll have to make it consistent
with promisc (e.g. one way would be for promisc always to enable unicast flood
and it won't be possible to be disabled while promisc).

> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);

s/1/true/ for consistency

> +
>  	br_netfilter_rtable_init(br);
>  	br_stp_timer_init(br);
>  	br_multicast_init(br);
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 196417859c4a..d439b876bdf5 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  		/* by definition the broadcast is also a multicast address */
>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
>  			pkt_type = BR_PKT_BROADCAST;
> -			local_rcv = true;
> +			if (br_opt_get(br, BROPT_BCAST_FLOOD))
> +				local_rcv = true;
>  		} else {
>  			pkt_type = BR_PKT_MULTICAST;
>  			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  			}
>  			mcast_hit = true;
>  		} else {
> -			local_rcv = true;
> -			br->dev->stats.multicast++;
> +			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
> +				local_rcv = true;
> +				br->dev->stats.multicast++;
> +			}
>  		}
>  		break;
>  	case BR_PKT_UNICAST:
>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
> +		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
> +			local_rcv = true;
>  		break;

This adds new tests for all fast paths for host traffic,
especially the port - port communication which is the most critical one.
Please at least move the unicast test to the "else" block of "if (dst)" later.

The other tests can be moved to host only code too, but would require bigger changes.
Please try to keep the impact on the fast-path at minimum.

>  	default:
>  		break;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 18ccc3d5d296..683bd0ee4c64 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -449,6 +449,9 @@ enum net_bridge_opts {
>  	BROPT_VLAN_BRIDGE_BINDING,
>  	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
>  	BROPT_MST_ENABLED,
> +	BROPT_UNICAST_FLOOD,
> +	BROPT_MCAST_FLOOD,
> +	BROPT_BCAST_FLOOD,
>  };
>  
>  struct net_bridge {

Re: [Bridge] [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Nikolay Aleksandrov]

On 11/04/2022 16:38, Joachim Wiberg wrote:
> The bridge itself is also a port, but unfortunately it does not (yet)
> have a 'struct net_bridge_port'.  However, in many cases we want to
> treat it as a proper port so concessions have been made, e.g., NULL
> port or host_joined attributes.
> 
> This patch is an attempt to more of the same by adding support for
> controlling flooding of unknown broadcast/unicast/multicast to the
> bridge.  Something we often also want to control in an offloaded
> switching fabric.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  net/bridge/br_device.c  |  4 ++++
>  net/bridge/br_input.c   | 11 ++++++++---
>  net/bridge/br_private.h |  3 +++
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 8d6bab244c4a..0aa7d21ac82c 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>  	dev->max_mtu = ETH_MAX_MTU;
>  
> +	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);

This one must be false by default. It changes current default behaviour.
Unknown unicast is not currently passed up to the bridge if the port is
not in promisc mode, this will change it. You'll have to make it consistent
with promisc (e.g. one way would be for promisc always to enable unicast flood
and it won't be possible to be disabled while promisc).

> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);

s/1/true/ for consistency

> +
>  	br_netfilter_rtable_init(br);
>  	br_stp_timer_init(br);
>  	br_multicast_init(br);
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 196417859c4a..d439b876bdf5 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  		/* by definition the broadcast is also a multicast address */
>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
>  			pkt_type = BR_PKT_BROADCAST;
> -			local_rcv = true;
> +			if (br_opt_get(br, BROPT_BCAST_FLOOD))
> +				local_rcv = true;
>  		} else {
>  			pkt_type = BR_PKT_MULTICAST;
>  			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  			}
>  			mcast_hit = true;
>  		} else {
> -			local_rcv = true;
> -			br->dev->stats.multicast++;
> +			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
> +				local_rcv = true;
> +				br->dev->stats.multicast++;
> +			}
>  		}
>  		break;
>  	case BR_PKT_UNICAST:
>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
> +		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
> +			local_rcv = true;
>  		break;

This adds new tests for all fast paths for host traffic,
especially the port - port communication which is the most critical one.
Please at least move the unicast test to the "else" block of "if (dst)" later.

The other tests can be moved to host only code too, but would require bigger changes.
Please try to keep the impact on the fast-path at minimum.

>  	default:
>  		break;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 18ccc3d5d296..683bd0ee4c64 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -449,6 +449,9 @@ enum net_bridge_opts {
>  	BROPT_VLAN_BRIDGE_BINDING,
>  	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
>  	BROPT_MST_ENABLED,
> +	BROPT_UNICAST_FLOOD,
> +	BROPT_MCAST_FLOOD,
> +	BROPT_BCAST_FLOOD,
>  };
>  
>  struct net_bridge {

Re: [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Nikolay Aleksandrov]

On 12/04/2022 21:27, Nikolay Aleksandrov wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> The bridge itself is also a port, but unfortunately it does not (yet)
>> have a 'struct net_bridge_port'.  However, in many cases we want to
>> treat it as a proper port so concessions have been made, e.g., NULL
>> port or host_joined attributes.
>>
>> This patch is an attempt to more of the same by adding support for
>> controlling flooding of unknown broadcast/unicast/multicast to the
>> bridge.  Something we often also want to control in an offloaded
>> switching fabric.
>>
>> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
>> ---
>>  net/bridge/br_device.c  |  4 ++++
>>  net/bridge/br_input.c   | 11 ++++++++---
>>  net/bridge/br_private.h |  3 +++
>>  3 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>> index 8d6bab244c4a..0aa7d21ac82c 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
>>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>>  	dev->max_mtu = ETH_MAX_MTU;
>>  
>> +	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);
> 
> This one must be false by default. It changes current default behaviour.
> Unknown unicast is not currently passed up to the bridge if the port is

s/port/bridge/ in promisc mode

> not in promisc mode, this will change it. You'll have to make it consistent
> with promisc (e.g. one way would be for promisc always to enable unicast flood
> and it won't be possible to be disabled while promisc).
> 
>> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
>> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);
> 
> s/1/true/ for consistency
> 
>> +
>>  	br_netfilter_rtable_init(br);
>>  	br_stp_timer_init(br);
>>  	br_multicast_init(br);
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index 196417859c4a..d439b876bdf5 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  		/* by definition the broadcast is also a multicast address */
>>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
>>  			pkt_type = BR_PKT_BROADCAST;
>> -			local_rcv = true;
>> +			if (br_opt_get(br, BROPT_BCAST_FLOOD))
>> +				local_rcv = true;
>>  		} else {
>>  			pkt_type = BR_PKT_MULTICAST;
>>  			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
>> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  			}
>>  			mcast_hit = true;
>>  		} else {
>> -			local_rcv = true;
>> -			br->dev->stats.multicast++;
>> +			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
>> +				local_rcv = true;
>> +				br->dev->stats.multicast++;
>> +			}
>>  		}
>>  		break;
>>  	case BR_PKT_UNICAST:
>>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
>> +		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
>> +			local_rcv = true;
>>  		break;
> 
> This adds new tests for all fast paths for host traffic,
> especially the port - port communication which is the most critical one.
> Please at least move the unicast test to the "else" block of "if (dst)" later.
> 
> The other tests can be moved to host only code too, but would require bigger changes.
> Please try to keep the impact on the fast-path at minimum.
> 
>>  	default:
>>  		break;
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 18ccc3d5d296..683bd0ee4c64 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -449,6 +449,9 @@ enum net_bridge_opts {
>>  	BROPT_VLAN_BRIDGE_BINDING,
>>  	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
>>  	BROPT_MST_ENABLED,
>> +	BROPT_UNICAST_FLOOD,
>> +	BROPT_MCAST_FLOOD,
>> +	BROPT_BCAST_FLOOD,
>>  };
>>  
>>  struct net_bridge {
> 

Re: [Bridge] [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Nikolay Aleksandrov]

On 12/04/2022 21:27, Nikolay Aleksandrov wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> The bridge itself is also a port, but unfortunately it does not (yet)
>> have a 'struct net_bridge_port'.  However, in many cases we want to
>> treat it as a proper port so concessions have been made, e.g., NULL
>> port or host_joined attributes.
>>
>> This patch is an attempt to more of the same by adding support for
>> controlling flooding of unknown broadcast/unicast/multicast to the
>> bridge.  Something we often also want to control in an offloaded
>> switching fabric.
>>
>> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
>> ---
>>  net/bridge/br_device.c  |  4 ++++
>>  net/bridge/br_input.c   | 11 ++++++++---
>>  net/bridge/br_private.h |  3 +++
>>  3 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>> index 8d6bab244c4a..0aa7d21ac82c 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
>>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>>  	dev->max_mtu = ETH_MAX_MTU;
>>  
>> +	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);
> 
> This one must be false by default. It changes current default behaviour.
> Unknown unicast is not currently passed up to the bridge if the port is

s/port/bridge/ in promisc mode

> not in promisc mode, this will change it. You'll have to make it consistent
> with promisc (e.g. one way would be for promisc always to enable unicast flood
> and it won't be possible to be disabled while promisc).
> 
>> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
>> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);
> 
> s/1/true/ for consistency
> 
>> +
>>  	br_netfilter_rtable_init(br);
>>  	br_stp_timer_init(br);
>>  	br_multicast_init(br);
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index 196417859c4a..d439b876bdf5 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  		/* by definition the broadcast is also a multicast address */
>>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
>>  			pkt_type = BR_PKT_BROADCAST;
>> -			local_rcv = true;
>> +			if (br_opt_get(br, BROPT_BCAST_FLOOD))
>> +				local_rcv = true;
>>  		} else {
>>  			pkt_type = BR_PKT_MULTICAST;
>>  			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
>> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  			}
>>  			mcast_hit = true;
>>  		} else {
>> -			local_rcv = true;
>> -			br->dev->stats.multicast++;
>> +			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
>> +				local_rcv = true;
>> +				br->dev->stats.multicast++;
>> +			}
>>  		}
>>  		break;
>>  	case BR_PKT_UNICAST:
>>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
>> +		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
>> +			local_rcv = true;
>>  		break;
> 
> This adds new tests for all fast paths for host traffic,
> especially the port - port communication which is the most critical one.
> Please at least move the unicast test to the "else" block of "if (dst)" later.
> 
> The other tests can be moved to host only code too, but would require bigger changes.
> Please try to keep the impact on the fast-path at minimum.
> 
>>  	default:
>>  		break;
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 18ccc3d5d296..683bd0ee4c64 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -449,6 +449,9 @@ enum net_bridge_opts {
>>  	BROPT_VLAN_BRIDGE_BINDING,
>>  	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
>>  	BROPT_MST_ENABLED,
>> +	BROPT_UNICAST_FLOOD,
>> +	BROPT_MCAST_FLOOD,
>> +	BROPT_BCAST_FLOOD,
>>  };
>>  
>>  struct net_bridge {
> 

Re: [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Joachim Wiberg]

On Tue, Apr 12, 2022 at 21:27, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
>>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>>  	dev->max_mtu = ETH_MAX_MTU;
>> +	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);
> This one must be false by default. It changes current default behaviour.
> Unknown unicast is not currently passed up to the bridge if the port is
> not in promisc mode, this will change it. You'll have to make it consistent
> with promisc (e.g. one way would be for promisc always to enable unicast flood
> and it won't be possible to be disabled while promisc).

Ouch, my bad!  Will look into how to let this have as little impact as
possible.  I like your semantics there, promisc should always win.

>> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
>> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);
>
> s/1/true/ for consistency

Of course, thanks!

>> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  		/* by definition the broadcast is also a multicast address */
>>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
>>  			pkt_type = BR_PKT_BROADCAST;
>> -			local_rcv = true;
>> +			if (br_opt_get(br, BROPT_BCAST_FLOOD))
>> +				local_rcv = true;
>>  		} else {
>>  			pkt_type = BR_PKT_MULTICAST;
>>  			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
>> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  			}
>>  			mcast_hit = true;
>>  		} else {
>> -			local_rcv = true;
>> -			br->dev->stats.multicast++;
>> +			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
>> +				local_rcv = true;
>> +				br->dev->stats.multicast++;
>> +			}
>>  		}
>>  		break;
>>  	case BR_PKT_UNICAST:
>>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
>> +		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
>> +			local_rcv = true;
>>  		break;
>
> This adds new tests for all fast paths for host traffic, especially
> the port - port communication which is the most critical one.  Please
> at least move the unicast test to the "else" block of "if (dst)"
> later.

OK, will fix!

> The other tests can be moved to host only code too, but would require
> bigger changes.  Please try to keep the impact on the fast-path at
> minimum.

Interesting, you mean by speculatively setting local_rcv = true and
postpone the decsion to br_pass_frame_up(), right?  Yeah that would
indeed be a bit more work.

Re: [Bridge] [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Joachim Wiberg]

On Tue, Apr 12, 2022 at 21:27, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
>>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>>  	dev->max_mtu = ETH_MAX_MTU;
>> +	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);
> This one must be false by default. It changes current default behaviour.
> Unknown unicast is not currently passed up to the bridge if the port is
> not in promisc mode, this will change it. You'll have to make it consistent
> with promisc (e.g. one way would be for promisc always to enable unicast flood
> and it won't be possible to be disabled while promisc).

Ouch, my bad!  Will look into how to let this have as little impact as
possible.  I like your semantics there, promisc should always win.

>> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
>> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);
>
> s/1/true/ for consistency

Of course, thanks!

>> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  		/* by definition the broadcast is also a multicast address */
>>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
>>  			pkt_type = BR_PKT_BROADCAST;
>> -			local_rcv = true;
>> +			if (br_opt_get(br, BROPT_BCAST_FLOOD))
>> +				local_rcv = true;
>>  		} else {
>>  			pkt_type = BR_PKT_MULTICAST;
>>  			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
>> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  			}
>>  			mcast_hit = true;
>>  		} else {
>> -			local_rcv = true;
>> -			br->dev->stats.multicast++;
>> +			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
>> +				local_rcv = true;
>> +				br->dev->stats.multicast++;
>> +			}
>>  		}
>>  		break;
>>  	case BR_PKT_UNICAST:
>>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
>> +		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
>> +			local_rcv = true;
>>  		break;
>
> This adds new tests for all fast paths for host traffic, especially
> the port - port communication which is the most critical one.  Please
> at least move the unicast test to the "else" block of "if (dst)"
> later.

OK, will fix!

> The other tests can be moved to host only code too, but would require
> bigger changes.  Please try to keep the impact on the fast-path at
> minimum.

Interesting, you mean by speculatively setting local_rcv = true and
postpone the decsion to br_pass_frame_up(), right?  Yeah that would
indeed be a bit more work.

Re: [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Nikolay Aleksandrov]

On 13/04/2022 12:51, Joachim Wiberg wrote:
> On Tue, Apr 12, 2022 at 21:27, Nikolay Aleksandrov <razor@blackwall.org> wrote:
>> On 11/04/2022 16:38, Joachim Wiberg wrote:
>>> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
>>>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>>>  	dev->max_mtu = ETH_MAX_MTU;
>>> +	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);
>> This one must be false by default. It changes current default behaviour.
>> Unknown unicast is not currently passed up to the bridge if the port is
>> not in promisc mode, this will change it. You'll have to make it consistent
>> with promisc (e.g. one way would be for promisc always to enable unicast flood
>> and it won't be possible to be disabled while promisc).
> 
> Ouch, my bad!  Will look into how to let this have as little impact as
> possible.  I like your semantics there, promisc should always win.
> 
>>> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
>>> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);
>>
>> s/1/true/ for consistency
> 
> Of course, thanks!
> 
>>> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>  		/* by definition the broadcast is also a multicast address */
>>>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
>>>  			pkt_type = BR_PKT_BROADCAST;
>>> -			local_rcv = true;
>>> +			if (br_opt_get(br, BROPT_BCAST_FLOOD))
>>> +				local_rcv = true;
>>>  		} else {
>>>  			pkt_type = BR_PKT_MULTICAST;
>>>  			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
>>> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>  			}
>>>  			mcast_hit = true;
>>>  		} else {
>>> -			local_rcv = true;
>>> -			br->dev->stats.multicast++;
>>> +			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
>>> +				local_rcv = true;
>>> +				br->dev->stats.multicast++;
>>> +			}
>>>  		}
>>>  		break;
>>>  	case BR_PKT_UNICAST:
>>>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
>>> +		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
>>> +			local_rcv = true;
>>>  		break;
>>
>> This adds new tests for all fast paths for host traffic, especially
>> the port - port communication which is the most critical one.  Please
>> at least move the unicast test to the "else" block of "if (dst)"
>> later.
> 
> OK, will fix!
> 
>> The other tests can be moved to host only code too, but would require
>> bigger changes.  Please try to keep the impact on the fast-path at
>> minimum.
> 
> Interesting, you mean by speculatively setting local_rcv = true and
> postpone the decsion to br_pass_frame_up(), right?  Yeah that would
> indeed be a bit more work.

Yes, I was thinking maybe local_rcv can become an enum with an exact reason for the
local_rcv, so if it's > 0 do the local_rcv and br_pass_frame_up() then
can make a proper decision without passing all of the vars. I haven't tried it,
so not sure if it's feasible. :)

Re: [Bridge] [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Nikolay Aleksandrov]

On 13/04/2022 12:51, Joachim Wiberg wrote:
> On Tue, Apr 12, 2022 at 21:27, Nikolay Aleksandrov <razor@blackwall.org> wrote:
>> On 11/04/2022 16:38, Joachim Wiberg wrote:
>>> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
>>>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>>>  	dev->max_mtu = ETH_MAX_MTU;
>>> +	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);
>> This one must be false by default. It changes current default behaviour.
>> Unknown unicast is not currently passed up to the bridge if the port is
>> not in promisc mode, this will change it. You'll have to make it consistent
>> with promisc (e.g. one way would be for promisc always to enable unicast flood
>> and it won't be possible to be disabled while promisc).
> 
> Ouch, my bad!  Will look into how to let this have as little impact as
> possible.  I like your semantics there, promisc should always win.
> 
>>> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
>>> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);
>>
>> s/1/true/ for consistency
> 
> Of course, thanks!
> 
>>> @@ -118,7 +118,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>  		/* by definition the broadcast is also a multicast address */
>>>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
>>>  			pkt_type = BR_PKT_BROADCAST;
>>> -			local_rcv = true;
>>> +			if (br_opt_get(br, BROPT_BCAST_FLOOD))
>>> +				local_rcv = true;
>>>  		} else {
>>>  			pkt_type = BR_PKT_MULTICAST;
>>>  			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
>>> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>  			}
>>>  			mcast_hit = true;
>>>  		} else {
>>> -			local_rcv = true;
>>> -			br->dev->stats.multicast++;
>>> +			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
>>> +				local_rcv = true;
>>> +				br->dev->stats.multicast++;
>>> +			}
>>>  		}
>>>  		break;
>>>  	case BR_PKT_UNICAST:
>>>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
>>> +		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
>>> +			local_rcv = true;
>>>  		break;
>>
>> This adds new tests for all fast paths for host traffic, especially
>> the port - port communication which is the most critical one.  Please
>> at least move the unicast test to the "else" block of "if (dst)"
>> later.
> 
> OK, will fix!
> 
>> The other tests can be moved to host only code too, but would require
>> bigger changes.  Please try to keep the impact on the fast-path at
>> minimum.
> 
> Interesting, you mean by speculatively setting local_rcv = true and
> postpone the decsion to br_pass_frame_up(), right?  Yeah that would
> indeed be a bit more work.

Yes, I was thinking maybe local_rcv can become an enum with an exact reason for the
local_rcv, so if it's > 0 do the local_rcv and br_pass_frame_up() then
can make a proper decision without passing all of the vars. I haven't tried it,
so not sure if it's feasible. :)

Re: [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Joachim Wiberg]

On Wed, Apr 13, 2022 at 12:58, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 13/04/2022 12:51, Joachim Wiberg wrote:
>> Interesting, you mean by speculatively setting local_rcv = true and
>> postpone the decsion to br_pass_frame_up(), right?  Yeah that would
>> indeed be a bit more work.
> Yes, I was thinking maybe local_rcv can become an enum with an exact reason for the
> local_rcv, so if it's > 0 do the local_rcv and br_pass_frame_up() then
> can make a proper decision without passing all of the vars. I haven't tried it,
> so not sure if it's feasible. :)

Ah, yeah that could definitely work. Thanks, I'll keep that in mind! :)

Re: [Bridge] [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

[Joachim Wiberg]

On Wed, Apr 13, 2022 at 12:58, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 13/04/2022 12:51, Joachim Wiberg wrote:
>> Interesting, you mean by speculatively setting local_rcv = true and
>> postpone the decsion to br_pass_frame_up(), right?  Yeah that would
>> indeed be a bit more work.
> Yes, I was thinking maybe local_rcv can become an enum with an exact reason for the
> local_rcv, so if it's > 0 do the local_rcv and br_pass_frame_up() then
> can make a proper decision without passing all of the vars. I haven't tried it,
> so not sure if it's feasible. :)

Ah, yeah that could definitely work. Thanks, I'll keep that in mind! :)

[PATCH RFC net-next 02/13] net: bridge: rename br_switchdev_set_port_flag() to .._dev_flag()

[Joachim Wiberg]

The br_switchdev_set_port_flag() function only uses the `p->dev` member
of the port, and we also want to reuse it for the bridge itself. Change
the first argument and the name of the function to match.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_netlink.c   | 2 +-
 net/bridge/br_private.h   | 4 ++--
 net/bridge/br_switchdev.c | 8 ++++----
 net/bridge/br_sysfs_if.c  | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 200ad05b296f..7fca8ff13ec7 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -942,7 +942,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
 
 	changed_mask = old_flags ^ p->flags;
 
-	err = br_switchdev_set_port_flag(p, p->flags, changed_mask, extack);
+	err = br_switchdev_set_dev_flag(p->dev, p->flags, changed_mask, extack);
 	if (err) {
 		p->flags = old_flags;
 		return err;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 683bd0ee4c64..6c230be6fbf5 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -2038,7 +2038,7 @@ void nbp_switchdev_frame_mark(const struct net_bridge_port *p,
 			      struct sk_buff *skb);
 bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
 				  const struct sk_buff *skb);
-int br_switchdev_set_port_flag(struct net_bridge_port *p,
+int br_switchdev_set_dev_flag(struct net_device *dev,
 			       unsigned long flags,
 			       unsigned long mask,
 			       struct netlink_ext_ack *extack);
@@ -2108,7 +2108,7 @@ static inline bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
 	return true;
 }
 
-static inline int br_switchdev_set_port_flag(struct net_bridge_port *p,
+static inline int br_switchdev_set_dev_flag(struct net_device *dev,
 					     unsigned long flags,
 					     unsigned long mask,
 					     struct netlink_ext_ack *extack)
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 8cc44c367231..3b4df63e96a6 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -74,13 +74,13 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
 #define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \
 				  BR_MCAST_FLOOD | BR_BCAST_FLOOD | BR_PORT_LOCKED)
 
-int br_switchdev_set_port_flag(struct net_bridge_port *p,
+int br_switchdev_set_dev_flag(struct net_device *dev,
 			       unsigned long flags,
 			       unsigned long mask,
 			       struct netlink_ext_ack *extack)
 {
 	struct switchdev_attr attr = {
-		.orig_dev = p->dev,
+		.orig_dev = dev,
 	};
 	struct switchdev_notifier_port_attr_info info = {
 		.attr = &attr,
@@ -96,7 +96,7 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
 	attr.u.brport_flags.mask = mask;
 
 	/* We run from atomic context here */
-	err = call_switchdev_notifiers(SWITCHDEV_PORT_ATTR_SET, p->dev,
+	err = call_switchdev_notifiers(SWITCHDEV_PORT_ATTR_SET, dev,
 				       &info.info, extack);
 	err = notifier_to_errno(err);
 	if (err == -EOPNOTSUPP)
@@ -112,7 +112,7 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
 	attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS;
 	attr.flags = SWITCHDEV_F_DEFER;
 
-	err = switchdev_port_attr_set(p->dev, &attr, extack);
+	err = switchdev_port_attr_set(dev, &attr, extack);
 	if (err) {
 		if (extack && !extack->_msg)
 			NL_SET_ERR_MSG_MOD(extack,
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 07fa76080512..c222c68105f1 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -69,7 +69,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v,
 		flags &= ~mask;
 
 	if (flags != p->flags) {
-		err = br_switchdev_set_port_flag(p, flags, mask, &extack);
+		err = br_switchdev_set_dev_flag(p->dev, flags, mask, &extack);
 		if (err) {
 			netdev_err(p->dev, "%s\n", extack._msg);
 			return err;
-- 
2.25.1

[Bridge] [PATCH RFC net-next 02/13] net: bridge: rename br_switchdev_set_port_flag() to .._dev_flag()

[Joachim Wiberg]

The br_switchdev_set_port_flag() function only uses the `p->dev` member
of the port, and we also want to reuse it for the bridge itself. Change
the first argument and the name of the function to match.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_netlink.c   | 2 +-
 net/bridge/br_private.h   | 4 ++--
 net/bridge/br_switchdev.c | 8 ++++----
 net/bridge/br_sysfs_if.c  | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 200ad05b296f..7fca8ff13ec7 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -942,7 +942,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
 
 	changed_mask = old_flags ^ p->flags;
 
-	err = br_switchdev_set_port_flag(p, p->flags, changed_mask, extack);
+	err = br_switchdev_set_dev_flag(p->dev, p->flags, changed_mask, extack);
 	if (err) {
 		p->flags = old_flags;
 		return err;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 683bd0ee4c64..6c230be6fbf5 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -2038,7 +2038,7 @@ void nbp_switchdev_frame_mark(const struct net_bridge_port *p,
 			      struct sk_buff *skb);
 bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
 				  const struct sk_buff *skb);
-int br_switchdev_set_port_flag(struct net_bridge_port *p,
+int br_switchdev_set_dev_flag(struct net_device *dev,
 			       unsigned long flags,
 			       unsigned long mask,
 			       struct netlink_ext_ack *extack);
@@ -2108,7 +2108,7 @@ static inline bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
 	return true;
 }
 
-static inline int br_switchdev_set_port_flag(struct net_bridge_port *p,
+static inline int br_switchdev_set_dev_flag(struct net_device *dev,
 					     unsigned long flags,
 					     unsigned long mask,
 					     struct netlink_ext_ack *extack)
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 8cc44c367231..3b4df63e96a6 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -74,13 +74,13 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
 #define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \
 				  BR_MCAST_FLOOD | BR_BCAST_FLOOD | BR_PORT_LOCKED)
 
-int br_switchdev_set_port_flag(struct net_bridge_port *p,
+int br_switchdev_set_dev_flag(struct net_device *dev,
 			       unsigned long flags,
 			       unsigned long mask,
 			       struct netlink_ext_ack *extack)
 {
 	struct switchdev_attr attr = {
-		.orig_dev = p->dev,
+		.orig_dev = dev,
 	};
 	struct switchdev_notifier_port_attr_info info = {
 		.attr = &attr,
@@ -96,7 +96,7 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
 	attr.u.brport_flags.mask = mask;
 
 	/* We run from atomic context here */
-	err = call_switchdev_notifiers(SWITCHDEV_PORT_ATTR_SET, p->dev,
+	err = call_switchdev_notifiers(SWITCHDEV_PORT_ATTR_SET, dev,
 				       &info.info, extack);
 	err = notifier_to_errno(err);
 	if (err == -EOPNOTSUPP)
@@ -112,7 +112,7 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
 	attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS;
 	attr.flags = SWITCHDEV_F_DEFER;
 
-	err = switchdev_port_attr_set(p->dev, &attr, extack);
+	err = switchdev_port_attr_set(dev, &attr, extack);
 	if (err) {
 		if (extack && !extack->_msg)
 			NL_SET_ERR_MSG_MOD(extack,
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 07fa76080512..c222c68105f1 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -69,7 +69,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v,
 		flags &= ~mask;
 
 	if (flags != p->flags) {
-		err = br_switchdev_set_port_flag(p, flags, mask, &extack);
+		err = br_switchdev_set_dev_flag(p->dev, flags, mask, &extack);
 		if (err) {
 			netdev_err(p->dev, "%s\n", extack._msg);
 			return err;
-- 
2.25.1

[PATCH RFC net-next 03/13] net: bridge: minor refactor of br_setlink() for readability

[Joachim Wiberg]

The br_setlink() function extracts the struct net_bridge pointer a bit
sloppy.  It's easy to interpret the code wrong.  This patch attempts to
clear things up a bit.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_netlink.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 7fca8ff13ec7..8f4297287b32 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1040,6 +1040,8 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
 		return 0;
 
 	p = br_port_get_rtnl(dev);
+	if (p)
+		br = p->br;
 	/* We want to accept dev as bridge itself if the AF_SPEC
 	 * is set to see if someone is setting vlan info on the bridge
 	 */
@@ -1055,17 +1057,17 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
 			if (err)
 				return err;
 
-			spin_lock_bh(&p->br->lock);
+			spin_lock_bh(&br->lock);
 			err = br_setport(p, tb, extack);
-			spin_unlock_bh(&p->br->lock);
+			spin_unlock_bh(&br->lock);
 		} else {
 			/* Binary compatibility with old RSTP */
 			if (nla_len(protinfo) < sizeof(u8))
 				return -EINVAL;
 
-			spin_lock_bh(&p->br->lock);
+			spin_lock_bh(&br->lock);
 			err = br_set_port_state(p, nla_get_u8(protinfo));
-			spin_unlock_bh(&p->br->lock);
+			spin_unlock_bh(&br->lock);
 		}
 		if (err)
 			goto out;
-- 
2.25.1

[Bridge] [PATCH RFC net-next 03/13] net: bridge: minor refactor of br_setlink() for readability

[Joachim Wiberg]

The br_setlink() function extracts the struct net_bridge pointer a bit
sloppy.  It's easy to interpret the code wrong.  This patch attempts to
clear things up a bit.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_netlink.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 7fca8ff13ec7..8f4297287b32 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1040,6 +1040,8 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
 		return 0;
 
 	p = br_port_get_rtnl(dev);
+	if (p)
+		br = p->br;
 	/* We want to accept dev as bridge itself if the AF_SPEC
 	 * is set to see if someone is setting vlan info on the bridge
 	 */
@@ -1055,17 +1057,17 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
 			if (err)
 				return err;
 
-			spin_lock_bh(&p->br->lock);
+			spin_lock_bh(&br->lock);
 			err = br_setport(p, tb, extack);
-			spin_unlock_bh(&p->br->lock);
+			spin_unlock_bh(&br->lock);
 		} else {
 			/* Binary compatibility with old RSTP */
 			if (nla_len(protinfo) < sizeof(u8))
 				return -EINVAL;
 
-			spin_lock_bh(&p->br->lock);
+			spin_lock_bh(&br->lock);
 			err = br_set_port_state(p, nla_get_u8(protinfo));
-			spin_unlock_bh(&p->br->lock);
+			spin_unlock_bh(&br->lock);
 		}
 		if (err)
 			goto out;
-- 
2.25.1

Re: [PATCH RFC net-next 03/13] net: bridge: minor refactor of br_setlink() for readability

[Nikolay Aleksandrov]

On 11/04/2022 16:38, Joachim Wiberg wrote:
> The br_setlink() function extracts the struct net_bridge pointer a bit
> sloppy.  It's easy to interpret the code wrong.  This patch attempts to
> clear things up a bit.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  net/bridge/br_netlink.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 

I think you can make it more straight-forward, remove the first br = netdev_priv
and do something like (completely untested):
...
struct net_bridge_port *p = NULL;
...
if (netif_is_bridge_master(dev)) {
 br = netdev_priv(dev);
} else {
 p = br_port_get_rtnl(dev);
 if (WARN_ON(!p))
	return -EINVAL;
 br = p->br;
}

So br is always and only set in this block.

> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 7fca8ff13ec7..8f4297287b32 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1040,6 +1040,8 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>  		return 0;
>  
>  	p = br_port_get_rtnl(dev);
> +	if (p)
> +		br = p->br;
>  	/* We want to accept dev as bridge itself if the AF_SPEC
>  	 * is set to see if someone is setting vlan info on the bridge
>  	 */
> @@ -1055,17 +1057,17 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>  			if (err)
>  				return err;
>  
> -			spin_lock_bh(&p->br->lock);
> +			spin_lock_bh(&br->lock);
>  			err = br_setport(p, tb, extack);
> -			spin_unlock_bh(&p->br->lock);
> +			spin_unlock_bh(&br->lock);
>  		} else {
>  			/* Binary compatibility with old RSTP */
>  			if (nla_len(protinfo) < sizeof(u8))
>  				return -EINVAL;
>  
> -			spin_lock_bh(&p->br->lock);
> +			spin_lock_bh(&br->lock);
>  			err = br_set_port_state(p, nla_get_u8(protinfo));
> -			spin_unlock_bh(&p->br->lock);
> +			spin_unlock_bh(&br->lock);
>  		}
>  		if (err)
>  			goto out;

Re: [Bridge] [PATCH RFC net-next 03/13] net: bridge: minor refactor of br_setlink() for readability

[Nikolay Aleksandrov]

On 11/04/2022 16:38, Joachim Wiberg wrote:
> The br_setlink() function extracts the struct net_bridge pointer a bit
> sloppy.  It's easy to interpret the code wrong.  This patch attempts to
> clear things up a bit.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  net/bridge/br_netlink.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 

I think you can make it more straight-forward, remove the first br = netdev_priv
and do something like (completely untested):
...
struct net_bridge_port *p = NULL;
...
if (netif_is_bridge_master(dev)) {
 br = netdev_priv(dev);
} else {
 p = br_port_get_rtnl(dev);
 if (WARN_ON(!p))
	return -EINVAL;
 br = p->br;
}

So br is always and only set in this block.

> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 7fca8ff13ec7..8f4297287b32 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1040,6 +1040,8 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>  		return 0;
>  
>  	p = br_port_get_rtnl(dev);
> +	if (p)
> +		br = p->br;
>  	/* We want to accept dev as bridge itself if the AF_SPEC
>  	 * is set to see if someone is setting vlan info on the bridge
>  	 */
> @@ -1055,17 +1057,17 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>  			if (err)
>  				return err;
>  
> -			spin_lock_bh(&p->br->lock);
> +			spin_lock_bh(&br->lock);
>  			err = br_setport(p, tb, extack);
> -			spin_unlock_bh(&p->br->lock);
> +			spin_unlock_bh(&br->lock);
>  		} else {
>  			/* Binary compatibility with old RSTP */
>  			if (nla_len(protinfo) < sizeof(u8))
>  				return -EINVAL;
>  
> -			spin_lock_bh(&p->br->lock);
> +			spin_lock_bh(&br->lock);
>  			err = br_set_port_state(p, nla_get_u8(protinfo));
> -			spin_unlock_bh(&p->br->lock);
> +			spin_unlock_bh(&br->lock);
>  		}
>  		if (err)
>  			goto out;

Re: [PATCH RFC net-next 03/13] net: bridge: minor refactor of br_setlink() for readability

[Joachim Wiberg]

On Tue, Apr 12, 2022 at 21:36, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> The br_setlink() function extracts the struct net_bridge pointer a bit
>> sloppy.  It's easy to interpret the code wrong.  This patch attempts to
>> clear things up a bit.
> I think you can make it more straight-forward, remove the first br = netdev_priv
> and do something like (completely untested):
> ...
> struct net_bridge_port *p = NULL;
> ...
> if (netif_is_bridge_master(dev)) {
>  br = netdev_priv(dev);
> } else {
>  p = br_port_get_rtnl(dev);
>  if (WARN_ON(!p))
> 	return -EINVAL;
>  br = p->br;
> }
>
> So br is always and only set in this block.

Yes, this is much better, thank you!  I took the misguided approach of
minmizing my change.  I'll update and include in the non-RFC patch
series I send next.

Re: [Bridge] [PATCH RFC net-next 03/13] net: bridge: minor refactor of br_setlink() for readability

[Joachim Wiberg]

On Tue, Apr 12, 2022 at 21:36, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> The br_setlink() function extracts the struct net_bridge pointer a bit
>> sloppy.  It's easy to interpret the code wrong.  This patch attempts to
>> clear things up a bit.
> I think you can make it more straight-forward, remove the first br = netdev_priv
> and do something like (completely untested):
> ...
> struct net_bridge_port *p = NULL;
> ...
> if (netif_is_bridge_master(dev)) {
>  br = netdev_priv(dev);
> } else {
>  p = br_port_get_rtnl(dev);
>  if (WARN_ON(!p))
> 	return -EINVAL;
>  br = p->br;
> }
>
> So br is always and only set in this block.

Yes, this is much better, thank you!  I took the misguided approach of
minmizing my change.  I'll update and include in the non-RFC patch
series I send next.

[PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge

[Joachim Wiberg]

This patch adds netlink support for controlling the new broadcast,
unicast, and multicast flooding flags to the bridge itself.

The messy part is in br_setport(), which re-indents a large block of
code for the port settings.  To reduce code duplication a few new
variables have been added; new_flags and dev.  The latter is used for
the recently renamed br_switchdev_set_dev_flag(), which can now be used
by underlying switching fabric drivers as another source of information
when controlling flooding of unknown BUM traffic to the CPU port.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_netlink.c | 160 ++++++++++++++++++++++++++++++----------
 1 file changed, 123 insertions(+), 37 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 8f4297287b32..68bbf703b31a 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -225,13 +225,29 @@ static inline size_t br_nlmsg_size(struct net_device *dev, u32 filter_mask)
 		+ nla_total_size(4); /* IFLA_BRPORT_BACKUP_PORT */
 }
 
-static int br_port_fill_attrs(struct sk_buff *skb,
+static int br_port_fill_attrs(struct sk_buff *skb, const struct net_bridge *br,
 			      const struct net_bridge_port *p)
 {
-	u8 mode = !!(p->flags & BR_HAIRPIN_MODE);
 	struct net_bridge_port *backup_p;
 	u64 timerval;
+	u8 mode;
 
+	if (!p) {
+		if (!br)
+			return -EINVAL;
+
+		if (nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD,
+			       br_opt_get(br, BROPT_UNICAST_FLOOD)) ||
+		    nla_put_u8(skb, IFLA_BRPORT_MCAST_FLOOD,
+			       br_opt_get(br, BROPT_MCAST_FLOOD)) ||
+		    nla_put_u8(skb, IFLA_BRPORT_BCAST_FLOOD,
+			       br_opt_get(br, BROPT_BCAST_FLOOD)))
+			return -EMSGSIZE;
+
+		return 0;
+	}
+
+	mode = !!(p->flags & BR_HAIRPIN_MODE);
 	if (nla_put_u8(skb, IFLA_BRPORT_STATE, p->state) ||
 	    nla_put_u16(skb, IFLA_BRPORT_PRIORITY, p->priority) ||
 	    nla_put_u32(skb, IFLA_BRPORT_COST, p->path_cost) ||
@@ -475,11 +491,11 @@ static int br_fill_ifinfo(struct sk_buff *skb,
 	     nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev))))
 		goto nla_put_failure;
 
-	if (event == RTM_NEWLINK && port) {
+	if (event == RTM_NEWLINK) {
 		struct nlattr *nest;
 
 		nest = nla_nest_start(skb, IFLA_PROTINFO);
-		if (nest == NULL || br_port_fill_attrs(skb, port) < 0)
+		if (!nest || br_port_fill_attrs(skb, br, port) < 0)
 			goto nla_put_failure;
 		nla_nest_end(skb, nest);
 	}
@@ -911,43 +927,113 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
 		p->flags &= ~mask;
 }
 
+/* Map bridge options to brport flags */
+static unsigned long br_boolopt_map_flags(struct br_boolopt_multi *bm)
+{
+	unsigned long bitmap = bm->optmask;
+	unsigned long bitmask = 0;
+	int opt_id;
+
+	for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) {
+		if (!(bm->optval & BIT(opt_id)))
+			continue;
+
+		switch (opt_id) {
+		case BROPT_UNICAST_FLOOD:
+			bitmask |= BR_FLOOD;
+			break;
+		case BROPT_MCAST_FLOOD:
+			bitmask |= BR_MCAST_FLOOD;
+			break;
+		case BROPT_BCAST_FLOOD:
+			bitmask |= BR_BCAST_FLOOD;
+			break;
+		}
+	}
+
+	return bitmask;
+}
+
+static void br_set_bropt(struct net_bridge *br, struct nlattr *tb[],
+			 int attrtype, enum net_bridge_opts opt)
+{
+	if (!tb[attrtype])
+		return;
+
+	br_opt_toggle(br, opt, !!nla_get_u8(tb[attrtype]));
+}
+
+#define BROPT_MASK (BROPT_UNICAST_FLOOD | BROPT_MCAST_FLOOD | BROPT_MCAST_FLOOD)
+
 /* Process bridge protocol info on port */
-static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
-		      struct netlink_ext_ack *extack)
+static int br_setport(struct net_bridge *br, struct net_bridge_port *p,
+		      struct nlattr *tb[], struct netlink_ext_ack *extack)
 {
-	unsigned long old_flags, changed_mask;
+	unsigned long old_flags, new_flags, changed_mask;
+	struct br_boolopt_multi old_opts = {
+		.optmask = BROPT_MASK
+	};
 	bool br_vlan_tunnel_old;
+	struct net_device *dev;
 	int err;
 
-	old_flags = p->flags;
-	br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
-
-	br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
-	br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE,
-			 BR_MULTICAST_FAST_LEAVE);
-	br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
-	br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
-	br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST,
-			 BR_MULTICAST_TO_UNICAST);
-	br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
-	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
-	br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
-	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
-	br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
-	br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
-
-	changed_mask = old_flags ^ p->flags;
-
-	err = br_switchdev_set_dev_flag(p->dev, p->flags, changed_mask, extack);
+	if (p) {
+		old_flags = p->flags;
+		br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
+
+		br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
+		br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
+		br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE,
+				 BR_MULTICAST_FAST_LEAVE);
+		br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
+		br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
+		br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
+		br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
+		br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST,
+				 BR_MULTICAST_TO_UNICAST);
+		br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
+		br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
+		br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
+		br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
+		br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
+		br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
+		br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
+
+		new_flags = p->flags;
+		dev = p->dev;
+	} else {
+		struct br_boolopt_multi opts = {
+			.optmask = BROPT_MASK
+		};
+
+		br_boolopt_multi_get(br, &old_opts);
+		old_flags = br_boolopt_map_flags(&old_opts);
+
+		br_set_bropt(br, tb, IFLA_BRPORT_UNICAST_FLOOD, BROPT_UNICAST_FLOOD);
+		br_set_bropt(br, tb, IFLA_BRPORT_MCAST_FLOOD, BROPT_MCAST_FLOOD);
+		br_set_bropt(br, tb, IFLA_BRPORT_BCAST_FLOOD, BROPT_BCAST_FLOOD);
+
+		br_boolopt_multi_get(br, &opts);
+		new_flags = br_boolopt_map_flags(&opts);
+		dev = br->dev;
+	}
+
+	changed_mask = old_flags ^ new_flags;
+
+	err = br_switchdev_set_dev_flag(dev, new_flags, changed_mask, extack);
 	if (err) {
-		p->flags = old_flags;
+		if (!p)
+			br_boolopt_multi_toggle(br, &old_opts, extack);
+		else
+			p->flags = old_flags;
+
 		return err;
 	}
 
+	/* Skip the rest for the bridge itself, for now */
+	if (!p)
+		return 0;
+
 	if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL))
 		nbp_vlan_tunnel_info_flush(p);
 
@@ -1048,7 +1134,7 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
 	if (!p && !afspec)
 		return -EINVAL;
 
-	if (p && protinfo) {
+	if (protinfo) {
 		if (protinfo->nla_type & NLA_F_NESTED) {
 			err = nla_parse_nested_deprecated(tb, IFLA_BRPORT_MAX,
 							  protinfo,
@@ -1058,9 +1144,9 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
 				return err;
 
 			spin_lock_bh(&br->lock);
-			err = br_setport(p, tb, extack);
+			err = br_setport(br, p, tb, extack);
 			spin_unlock_bh(&br->lock);
-		} else {
+		} else if (p) {
 			/* Binary compatibility with old RSTP */
 			if (nla_len(protinfo) < sizeof(u8))
 				return -EINVAL;
@@ -1153,7 +1239,7 @@ static int br_port_slave_changelink(struct net_device *brdev,
 		return 0;
 
 	spin_lock_bh(&br->lock);
-	ret = br_setport(br_port_get_rtnl(dev), data, extack);
+	ret = br_setport(br, br_port_get_rtnl(dev), data, extack);
 	spin_unlock_bh(&br->lock);
 
 	return ret;
@@ -1163,7 +1249,7 @@ static int br_port_fill_slave_info(struct sk_buff *skb,
 				   const struct net_device *brdev,
 				   const struct net_device *dev)
 {
-	return br_port_fill_attrs(skb, br_port_get_rtnl(dev));
+	return br_port_fill_attrs(skb, NULL, br_port_get_rtnl(dev));
 }
 
 static size_t br_port_get_slave_size(const struct net_device *brdev,
-- 
2.25.1

[Bridge] [PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge

[Joachim Wiberg]

This patch adds netlink support for controlling the new broadcast,
unicast, and multicast flooding flags to the bridge itself.

The messy part is in br_setport(), which re-indents a large block of
code for the port settings.  To reduce code duplication a few new
variables have been added; new_flags and dev.  The latter is used for
the recently renamed br_switchdev_set_dev_flag(), which can now be used
by underlying switching fabric drivers as another source of information
when controlling flooding of unknown BUM traffic to the CPU port.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_netlink.c | 160 ++++++++++++++++++++++++++++++----------
 1 file changed, 123 insertions(+), 37 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 8f4297287b32..68bbf703b31a 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -225,13 +225,29 @@ static inline size_t br_nlmsg_size(struct net_device *dev, u32 filter_mask)
 		+ nla_total_size(4); /* IFLA_BRPORT_BACKUP_PORT */
 }
 
-static int br_port_fill_attrs(struct sk_buff *skb,
+static int br_port_fill_attrs(struct sk_buff *skb, const struct net_bridge *br,
 			      const struct net_bridge_port *p)
 {
-	u8 mode = !!(p->flags & BR_HAIRPIN_MODE);
 	struct net_bridge_port *backup_p;
 	u64 timerval;
+	u8 mode;
 
+	if (!p) {
+		if (!br)
+			return -EINVAL;
+
+		if (nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD,
+			       br_opt_get(br, BROPT_UNICAST_FLOOD)) ||
+		    nla_put_u8(skb, IFLA_BRPORT_MCAST_FLOOD,
+			       br_opt_get(br, BROPT_MCAST_FLOOD)) ||
+		    nla_put_u8(skb, IFLA_BRPORT_BCAST_FLOOD,
+			       br_opt_get(br, BROPT_BCAST_FLOOD)))
+			return -EMSGSIZE;
+
+		return 0;
+	}
+
+	mode = !!(p->flags & BR_HAIRPIN_MODE);
 	if (nla_put_u8(skb, IFLA_BRPORT_STATE, p->state) ||
 	    nla_put_u16(skb, IFLA_BRPORT_PRIORITY, p->priority) ||
 	    nla_put_u32(skb, IFLA_BRPORT_COST, p->path_cost) ||
@@ -475,11 +491,11 @@ static int br_fill_ifinfo(struct sk_buff *skb,
 	     nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev))))
 		goto nla_put_failure;
 
-	if (event == RTM_NEWLINK && port) {
+	if (event == RTM_NEWLINK) {
 		struct nlattr *nest;
 
 		nest = nla_nest_start(skb, IFLA_PROTINFO);
-		if (nest == NULL || br_port_fill_attrs(skb, port) < 0)
+		if (!nest || br_port_fill_attrs(skb, br, port) < 0)
 			goto nla_put_failure;
 		nla_nest_end(skb, nest);
 	}
@@ -911,43 +927,113 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
 		p->flags &= ~mask;
 }
 
+/* Map bridge options to brport flags */
+static unsigned long br_boolopt_map_flags(struct br_boolopt_multi *bm)
+{
+	unsigned long bitmap = bm->optmask;
+	unsigned long bitmask = 0;
+	int opt_id;
+
+	for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) {
+		if (!(bm->optval & BIT(opt_id)))
+			continue;
+
+		switch (opt_id) {
+		case BROPT_UNICAST_FLOOD:
+			bitmask |= BR_FLOOD;
+			break;
+		case BROPT_MCAST_FLOOD:
+			bitmask |= BR_MCAST_FLOOD;
+			break;
+		case BROPT_BCAST_FLOOD:
+			bitmask |= BR_BCAST_FLOOD;
+			break;
+		}
+	}
+
+	return bitmask;
+}
+
+static void br_set_bropt(struct net_bridge *br, struct nlattr *tb[],
+			 int attrtype, enum net_bridge_opts opt)
+{
+	if (!tb[attrtype])
+		return;
+
+	br_opt_toggle(br, opt, !!nla_get_u8(tb[attrtype]));
+}
+
+#define BROPT_MASK (BROPT_UNICAST_FLOOD | BROPT_MCAST_FLOOD | BROPT_MCAST_FLOOD)
+
 /* Process bridge protocol info on port */
-static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
-		      struct netlink_ext_ack *extack)
+static int br_setport(struct net_bridge *br, struct net_bridge_port *p,
+		      struct nlattr *tb[], struct netlink_ext_ack *extack)
 {
-	unsigned long old_flags, changed_mask;
+	unsigned long old_flags, new_flags, changed_mask;
+	struct br_boolopt_multi old_opts = {
+		.optmask = BROPT_MASK
+	};
 	bool br_vlan_tunnel_old;
+	struct net_device *dev;
 	int err;
 
-	old_flags = p->flags;
-	br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
-
-	br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
-	br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE,
-			 BR_MULTICAST_FAST_LEAVE);
-	br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
-	br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
-	br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST,
-			 BR_MULTICAST_TO_UNICAST);
-	br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
-	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
-	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
-	br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
-	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
-	br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
-	br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
-
-	changed_mask = old_flags ^ p->flags;
-
-	err = br_switchdev_set_dev_flag(p->dev, p->flags, changed_mask, extack);
+	if (p) {
+		old_flags = p->flags;
+		br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
+
+		br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
+		br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
+		br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE,
+				 BR_MULTICAST_FAST_LEAVE);
+		br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
+		br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
+		br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
+		br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
+		br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST,
+				 BR_MULTICAST_TO_UNICAST);
+		br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
+		br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
+		br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
+		br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
+		br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
+		br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
+		br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
+
+		new_flags = p->flags;
+		dev = p->dev;
+	} else {
+		struct br_boolopt_multi opts = {
+			.optmask = BROPT_MASK
+		};
+
+		br_boolopt_multi_get(br, &old_opts);
+		old_flags = br_boolopt_map_flags(&old_opts);
+
+		br_set_bropt(br, tb, IFLA_BRPORT_UNICAST_FLOOD, BROPT_UNICAST_FLOOD);
+		br_set_bropt(br, tb, IFLA_BRPORT_MCAST_FLOOD, BROPT_MCAST_FLOOD);
+		br_set_bropt(br, tb, IFLA_BRPORT_BCAST_FLOOD, BROPT_BCAST_FLOOD);
+
+		br_boolopt_multi_get(br, &opts);
+		new_flags = br_boolopt_map_flags(&opts);
+		dev = br->dev;
+	}
+
+	changed_mask = old_flags ^ new_flags;
+
+	err = br_switchdev_set_dev_flag(dev, new_flags, changed_mask, extack);
 	if (err) {
-		p->flags = old_flags;
+		if (!p)
+			br_boolopt_multi_toggle(br, &old_opts, extack);
+		else
+			p->flags = old_flags;
+
 		return err;
 	}
 
+	/* Skip the rest for the bridge itself, for now */
+	if (!p)
+		return 0;
+
 	if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL))
 		nbp_vlan_tunnel_info_flush(p);
 
@@ -1048,7 +1134,7 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
 	if (!p && !afspec)
 		return -EINVAL;
 
-	if (p && protinfo) {
+	if (protinfo) {
 		if (protinfo->nla_type & NLA_F_NESTED) {
 			err = nla_parse_nested_deprecated(tb, IFLA_BRPORT_MAX,
 							  protinfo,
@@ -1058,9 +1144,9 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
 				return err;
 
 			spin_lock_bh(&br->lock);
-			err = br_setport(p, tb, extack);
+			err = br_setport(br, p, tb, extack);
 			spin_unlock_bh(&br->lock);
-		} else {
+		} else if (p) {
 			/* Binary compatibility with old RSTP */
 			if (nla_len(protinfo) < sizeof(u8))
 				return -EINVAL;
@@ -1153,7 +1239,7 @@ static int br_port_slave_changelink(struct net_device *brdev,
 		return 0;
 
 	spin_lock_bh(&br->lock);
-	ret = br_setport(br_port_get_rtnl(dev), data, extack);
+	ret = br_setport(br, br_port_get_rtnl(dev), data, extack);
 	spin_unlock_bh(&br->lock);
 
 	return ret;
@@ -1163,7 +1249,7 @@ static int br_port_fill_slave_info(struct sk_buff *skb,
 				   const struct net_device *brdev,
 				   const struct net_device *dev)
 {
-	return br_port_fill_attrs(skb, br_port_get_rtnl(dev));
+	return br_port_fill_attrs(skb, NULL, br_port_get_rtnl(dev));
 }
 
 static size_t br_port_get_slave_size(const struct net_device *brdev,
-- 
2.25.1

Re: [PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge

[Nikolay Aleksandrov]

On 11/04/2022 16:38, Joachim Wiberg wrote:
> This patch adds netlink support for controlling the new broadcast,
> unicast, and multicast flooding flags to the bridge itself.
> 
> The messy part is in br_setport(), which re-indents a large block of
> code for the port settings.  To reduce code duplication a few new
> variables have been added; new_flags and dev.  The latter is used for
> the recently renamed br_switchdev_set_dev_flag(), which can now be used
> by underlying switching fabric drivers as another source of information
> when controlling flooding of unknown BUM traffic to the CPU port.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---

Absolutely not. This is just wrong on a few levels and way too hacky.

Please separate the bridge handling altogether and make it clean.
No need to integrate it with the port handling, also I think you've missed
a few things about bool options, more below...

For boolopts examples you can check BR_BOOLOPT_NO_LL_LEARN,
BR_BOOLOPT_MCAST_VLAN_SNOOPING and BR_BOOLOPT_MST_ENABLE.

>  net/bridge/br_netlink.c | 160 ++++++++++++++++++++++++++++++----------
>  1 file changed, 123 insertions(+), 37 deletions(-)
> 
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 8f4297287b32..68bbf703b31a 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -225,13 +225,29 @@ static inline size_t br_nlmsg_size(struct net_device *dev, u32 filter_mask)
>  		+ nla_total_size(4); /* IFLA_BRPORT_BACKUP_PORT */
>  }
>  
> -static int br_port_fill_attrs(struct sk_buff *skb,
> +static int br_port_fill_attrs(struct sk_buff *skb, const struct net_bridge *br,
>  			      const struct net_bridge_port *p)
>  {
> -	u8 mode = !!(p->flags & BR_HAIRPIN_MODE);
>  	struct net_bridge_port *backup_p;
>  	u64 timerval;
> +	u8 mode;
>  
> +	if (!p) {
> +		if (!br)
> +			return -EINVAL;
> +
> +		if (nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD,
> +			       br_opt_get(br, BROPT_UNICAST_FLOOD)) ||
> +		    nla_put_u8(skb, IFLA_BRPORT_MCAST_FLOOD,
> +			       br_opt_get(br, BROPT_MCAST_FLOOD)) ||
> +		    nla_put_u8(skb, IFLA_BRPORT_BCAST_FLOOD,
> +			       br_opt_get(br, BROPT_BCAST_FLOOD)))
> +			return -EMSGSIZE;

No. Bool opts are already exposed through IFLA_BR_MULTI_BOOLOPT.

> +
> +		return 0;
> +	}
> +
> +	mode = !!(p->flags & BR_HAIRPIN_MODE);
>  	if (nla_put_u8(skb, IFLA_BRPORT_STATE, p->state) ||
>  	    nla_put_u16(skb, IFLA_BRPORT_PRIORITY, p->priority) ||
>  	    nla_put_u32(skb, IFLA_BRPORT_COST, p->path_cost) ||
> @@ -475,11 +491,11 @@ static int br_fill_ifinfo(struct sk_buff *skb,
>  	     nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev))))
>  		goto nla_put_failure;
>  
> -	if (event == RTM_NEWLINK && port) {
> +	if (event == RTM_NEWLINK) {
>  		struct nlattr *nest;
>  
>  		nest = nla_nest_start(skb, IFLA_PROTINFO);
> -		if (nest == NULL || br_port_fill_attrs(skb, port) < 0)
> +		if (!nest || br_port_fill_attrs(skb, br, port) < 0)
>  			goto nla_put_failure;
>  		nla_nest_end(skb, nest);
>  	}
> @@ -911,43 +927,113 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
>  		p->flags &= ~mask;
>  }
>  
> +/* Map bridge options to brport flags */
> +static unsigned long br_boolopt_map_flags(struct br_boolopt_multi *bm)
> +{
> +	unsigned long bitmap = bm->optmask;
> +	unsigned long bitmask = 0;
> +	int opt_id;
> +
> +	for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) {
> +		if (!(bm->optval & BIT(opt_id)))
> +			continue;
> +
> +		switch (opt_id) {
> +		case BROPT_UNICAST_FLOOD:
> +			bitmask |= BR_FLOOD;
> +			break;
> +		case BROPT_MCAST_FLOOD:
> +			bitmask |= BR_MCAST_FLOOD;
> +			break;
> +		case BROPT_BCAST_FLOOD:
> +			bitmask |= BR_BCAST_FLOOD;
> +			break;
> +		}
> +	}
> +
> +	return bitmask;
> +}
> +
> +static void br_set_bropt(struct net_bridge *br, struct nlattr *tb[],
> +			 int attrtype, enum net_bridge_opts opt)
> +{
> +	if (!tb[attrtype])
> +		return;
> +
> +	br_opt_toggle(br, opt, !!nla_get_u8(tb[attrtype]));
> +}

These must be controlled via the boolopt api and attributes, not through
additional nl attributes.

> +
> +#define BROPT_MASK (BROPT_UNICAST_FLOOD | BROPT_MCAST_FLOOD | BROPT_MCAST_FLOOD)
> +
>  /* Process bridge protocol info on port */
> -static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
> -		      struct netlink_ext_ack *extack)
> +static int br_setport(struct net_bridge *br, struct net_bridge_port *p,
> +		      struct nlattr *tb[], struct netlink_ext_ack *extack)
>  {
> -	unsigned long old_flags, changed_mask;
> +	unsigned long old_flags, new_flags, changed_mask;
> +	struct br_boolopt_multi old_opts = {
> +		.optmask = BROPT_MASK
> +	};
>  	bool br_vlan_tunnel_old;
> +	struct net_device *dev;
>  	int err;
>  
> -	old_flags = p->flags;
> -	br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
> -
> -	br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE,
> -			 BR_MULTICAST_FAST_LEAVE);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST,
> -			 BR_MULTICAST_TO_UNICAST);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
> -
> -	changed_mask = old_flags ^ p->flags;
> -
> -	err = br_switchdev_set_dev_flag(p->dev, p->flags, changed_mask, extack);
> +	if (p) {
> +		old_flags = p->flags;
> +		br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
> +
> +		br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE,
> +				 BR_MULTICAST_FAST_LEAVE);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST,
> +				 BR_MULTICAST_TO_UNICAST);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
> +
> +		new_flags = p->flags;
> +		dev = p->dev;
> +	} else {
> +		struct br_boolopt_multi opts = {
> +			.optmask = BROPT_MASK
> +		};
> +
> +		br_boolopt_multi_get(br, &old_opts);
> +		old_flags = br_boolopt_map_flags(&old_opts);
> +
> +		br_set_bropt(br, tb, IFLA_BRPORT_UNICAST_FLOOD, BROPT_UNICAST_FLOOD);
> +		br_set_bropt(br, tb, IFLA_BRPORT_MCAST_FLOOD, BROPT_MCAST_FLOOD);
> +		br_set_bropt(br, tb, IFLA_BRPORT_BCAST_FLOOD, BROPT_BCAST_FLOOD);
> +
> +		br_boolopt_multi_get(br, &opts);
> +		new_flags = br_boolopt_map_flags(&opts);
> +		dev = br->dev;
> +	}
> +
> +	changed_mask = old_flags ^ new_flags;
> +
> +	err = br_switchdev_set_dev_flag(dev, new_flags, changed_mask, extack);
>  	if (err) {
> -		p->flags = old_flags;
> +		if (!p)
> +			br_boolopt_multi_toggle(br, &old_opts, extack);
> +		else
> +			p->flags = old_flags;
> +
>  		return err;
>  	}
>  
> +	/* Skip the rest for the bridge itself, for now */
> +	if (!p)
> +		return 0;
> +
>  	if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL))
>  		nbp_vlan_tunnel_info_flush(p);
>  
> @@ -1048,7 +1134,7 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>  	if (!p && !afspec)
>  		return -EINVAL;
>  
> -	if (p && protinfo) {
> +	if (protinfo) {
>  		if (protinfo->nla_type & NLA_F_NESTED) {
>  			err = nla_parse_nested_deprecated(tb, IFLA_BRPORT_MAX,
>  							  protinfo,
> @@ -1058,9 +1144,9 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>  				return err;
>  
>  			spin_lock_bh(&br->lock);
> -			err = br_setport(p, tb, extack);
> +			err = br_setport(br, p, tb, extack);

setport is for *port* only...

>  			spin_unlock_bh(&br->lock);
> -		} else {
> +		} else if (p) {
>  			/* Binary compatibility with old RSTP */
>  			if (nla_len(protinfo) < sizeof(u8))
>  				return -EINVAL;
> @@ -1153,7 +1239,7 @@ static int br_port_slave_changelink(struct net_device *brdev,
>  		return 0;
>  
>  	spin_lock_bh(&br->lock);
> -	ret = br_setport(br_port_get_rtnl(dev), data, extack);
> +	ret = br_setport(br, br_port_get_rtnl(dev), data, extack);
>  	spin_unlock_bh(&br->lock);
>  
>  	return ret;
> @@ -1163,7 +1249,7 @@ static int br_port_fill_slave_info(struct sk_buff *skb,
>  				   const struct net_device *brdev,
>  				   const struct net_device *dev)
>  {
> -	return br_port_fill_attrs(skb, br_port_get_rtnl(dev));
> +	return br_port_fill_attrs(skb, NULL, br_port_get_rtnl(dev));
>  }
>  
>  static size_t br_port_get_slave_size(const struct net_device *brdev,

Re: [Bridge] [PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge

[Nikolay Aleksandrov]

On 11/04/2022 16:38, Joachim Wiberg wrote:
> This patch adds netlink support for controlling the new broadcast,
> unicast, and multicast flooding flags to the bridge itself.
> 
> The messy part is in br_setport(), which re-indents a large block of
> code for the port settings.  To reduce code duplication a few new
> variables have been added; new_flags and dev.  The latter is used for
> the recently renamed br_switchdev_set_dev_flag(), which can now be used
> by underlying switching fabric drivers as another source of information
> when controlling flooding of unknown BUM traffic to the CPU port.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---

Absolutely not. This is just wrong on a few levels and way too hacky.

Please separate the bridge handling altogether and make it clean.
No need to integrate it with the port handling, also I think you've missed
a few things about bool options, more below...

For boolopts examples you can check BR_BOOLOPT_NO_LL_LEARN,
BR_BOOLOPT_MCAST_VLAN_SNOOPING and BR_BOOLOPT_MST_ENABLE.

>  net/bridge/br_netlink.c | 160 ++++++++++++++++++++++++++++++----------
>  1 file changed, 123 insertions(+), 37 deletions(-)
> 
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 8f4297287b32..68bbf703b31a 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -225,13 +225,29 @@ static inline size_t br_nlmsg_size(struct net_device *dev, u32 filter_mask)
>  		+ nla_total_size(4); /* IFLA_BRPORT_BACKUP_PORT */
>  }
>  
> -static int br_port_fill_attrs(struct sk_buff *skb,
> +static int br_port_fill_attrs(struct sk_buff *skb, const struct net_bridge *br,
>  			      const struct net_bridge_port *p)
>  {
> -	u8 mode = !!(p->flags & BR_HAIRPIN_MODE);
>  	struct net_bridge_port *backup_p;
>  	u64 timerval;
> +	u8 mode;
>  
> +	if (!p) {
> +		if (!br)
> +			return -EINVAL;
> +
> +		if (nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD,
> +			       br_opt_get(br, BROPT_UNICAST_FLOOD)) ||
> +		    nla_put_u8(skb, IFLA_BRPORT_MCAST_FLOOD,
> +			       br_opt_get(br, BROPT_MCAST_FLOOD)) ||
> +		    nla_put_u8(skb, IFLA_BRPORT_BCAST_FLOOD,
> +			       br_opt_get(br, BROPT_BCAST_FLOOD)))
> +			return -EMSGSIZE;

No. Bool opts are already exposed through IFLA_BR_MULTI_BOOLOPT.

> +
> +		return 0;
> +	}
> +
> +	mode = !!(p->flags & BR_HAIRPIN_MODE);
>  	if (nla_put_u8(skb, IFLA_BRPORT_STATE, p->state) ||
>  	    nla_put_u16(skb, IFLA_BRPORT_PRIORITY, p->priority) ||
>  	    nla_put_u32(skb, IFLA_BRPORT_COST, p->path_cost) ||
> @@ -475,11 +491,11 @@ static int br_fill_ifinfo(struct sk_buff *skb,
>  	     nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev))))
>  		goto nla_put_failure;
>  
> -	if (event == RTM_NEWLINK && port) {
> +	if (event == RTM_NEWLINK) {
>  		struct nlattr *nest;
>  
>  		nest = nla_nest_start(skb, IFLA_PROTINFO);
> -		if (nest == NULL || br_port_fill_attrs(skb, port) < 0)
> +		if (!nest || br_port_fill_attrs(skb, br, port) < 0)
>  			goto nla_put_failure;
>  		nla_nest_end(skb, nest);
>  	}
> @@ -911,43 +927,113 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
>  		p->flags &= ~mask;
>  }
>  
> +/* Map bridge options to brport flags */
> +static unsigned long br_boolopt_map_flags(struct br_boolopt_multi *bm)
> +{
> +	unsigned long bitmap = bm->optmask;
> +	unsigned long bitmask = 0;
> +	int opt_id;
> +
> +	for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) {
> +		if (!(bm->optval & BIT(opt_id)))
> +			continue;
> +
> +		switch (opt_id) {
> +		case BROPT_UNICAST_FLOOD:
> +			bitmask |= BR_FLOOD;
> +			break;
> +		case BROPT_MCAST_FLOOD:
> +			bitmask |= BR_MCAST_FLOOD;
> +			break;
> +		case BROPT_BCAST_FLOOD:
> +			bitmask |= BR_BCAST_FLOOD;
> +			break;
> +		}
> +	}
> +
> +	return bitmask;
> +}
> +
> +static void br_set_bropt(struct net_bridge *br, struct nlattr *tb[],
> +			 int attrtype, enum net_bridge_opts opt)
> +{
> +	if (!tb[attrtype])
> +		return;
> +
> +	br_opt_toggle(br, opt, !!nla_get_u8(tb[attrtype]));
> +}

These must be controlled via the boolopt api and attributes, not through
additional nl attributes.

> +
> +#define BROPT_MASK (BROPT_UNICAST_FLOOD | BROPT_MCAST_FLOOD | BROPT_MCAST_FLOOD)
> +
>  /* Process bridge protocol info on port */
> -static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
> -		      struct netlink_ext_ack *extack)
> +static int br_setport(struct net_bridge *br, struct net_bridge_port *p,
> +		      struct nlattr *tb[], struct netlink_ext_ack *extack)
>  {
> -	unsigned long old_flags, changed_mask;
> +	unsigned long old_flags, new_flags, changed_mask;
> +	struct br_boolopt_multi old_opts = {
> +		.optmask = BROPT_MASK
> +	};
>  	bool br_vlan_tunnel_old;
> +	struct net_device *dev;
>  	int err;
>  
> -	old_flags = p->flags;
> -	br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
> -
> -	br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE,
> -			 BR_MULTICAST_FAST_LEAVE);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST,
> -			 BR_MULTICAST_TO_UNICAST);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
> -	br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
> -
> -	changed_mask = old_flags ^ p->flags;
> -
> -	err = br_switchdev_set_dev_flag(p->dev, p->flags, changed_mask, extack);
> +	if (p) {
> +		old_flags = p->flags;
> +		br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false;
> +
> +		br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE,
> +				 BR_MULTICAST_FAST_LEAVE);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST,
> +				 BR_MULTICAST_TO_UNICAST);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
> +		br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
> +
> +		new_flags = p->flags;
> +		dev = p->dev;
> +	} else {
> +		struct br_boolopt_multi opts = {
> +			.optmask = BROPT_MASK
> +		};
> +
> +		br_boolopt_multi_get(br, &old_opts);
> +		old_flags = br_boolopt_map_flags(&old_opts);
> +
> +		br_set_bropt(br, tb, IFLA_BRPORT_UNICAST_FLOOD, BROPT_UNICAST_FLOOD);
> +		br_set_bropt(br, tb, IFLA_BRPORT_MCAST_FLOOD, BROPT_MCAST_FLOOD);
> +		br_set_bropt(br, tb, IFLA_BRPORT_BCAST_FLOOD, BROPT_BCAST_FLOOD);
> +
> +		br_boolopt_multi_get(br, &opts);
> +		new_flags = br_boolopt_map_flags(&opts);
> +		dev = br->dev;
> +	}
> +
> +	changed_mask = old_flags ^ new_flags;
> +
> +	err = br_switchdev_set_dev_flag(dev, new_flags, changed_mask, extack);
>  	if (err) {
> -		p->flags = old_flags;
> +		if (!p)
> +			br_boolopt_multi_toggle(br, &old_opts, extack);
> +		else
> +			p->flags = old_flags;
> +
>  		return err;
>  	}
>  
> +	/* Skip the rest for the bridge itself, for now */
> +	if (!p)
> +		return 0;
> +
>  	if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL))
>  		nbp_vlan_tunnel_info_flush(p);
>  
> @@ -1048,7 +1134,7 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>  	if (!p && !afspec)
>  		return -EINVAL;
>  
> -	if (p && protinfo) {
> +	if (protinfo) {
>  		if (protinfo->nla_type & NLA_F_NESTED) {
>  			err = nla_parse_nested_deprecated(tb, IFLA_BRPORT_MAX,
>  							  protinfo,
> @@ -1058,9 +1144,9 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>  				return err;
>  
>  			spin_lock_bh(&br->lock);
> -			err = br_setport(p, tb, extack);
> +			err = br_setport(br, p, tb, extack);

setport is for *port* only...

>  			spin_unlock_bh(&br->lock);
> -		} else {
> +		} else if (p) {
>  			/* Binary compatibility with old RSTP */
>  			if (nla_len(protinfo) < sizeof(u8))
>  				return -EINVAL;
> @@ -1153,7 +1239,7 @@ static int br_port_slave_changelink(struct net_device *brdev,
>  		return 0;
>  
>  	spin_lock_bh(&br->lock);
> -	ret = br_setport(br_port_get_rtnl(dev), data, extack);
> +	ret = br_setport(br, br_port_get_rtnl(dev), data, extack);
>  	spin_unlock_bh(&br->lock);
>  
>  	return ret;
> @@ -1163,7 +1249,7 @@ static int br_port_fill_slave_info(struct sk_buff *skb,
>  				   const struct net_device *brdev,
>  				   const struct net_device *dev)
>  {
> -	return br_port_fill_attrs(skb, br_port_get_rtnl(dev));
> +	return br_port_fill_attrs(skb, NULL, br_port_get_rtnl(dev));
>  }
>  
>  static size_t br_port_get_slave_size(const struct net_device *brdev,

Re: [PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge

[Joachim Wiberg]

On Tue, Apr 12, 2022 at 21:24, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> The messy part is in br_setport(), which re-indents a large block of
>> code for the port settings.  To reduce code duplication a few new
>> variables have been added; new_flags and dev.  The latter is used for
>> the recently renamed br_switchdev_set_dev_flag(), which can now be used
>> by underlying switching fabric drivers as another source of information
>> when controlling flooding of unknown BUM traffic to the CPU port.
> Absolutely not. This is just wrong on a few levels and way too hacky.
> Please separate the bridge handling altogether and make it clean.
> No need to integrate it with the port handling,

OK, I'll have a go at it.

> also I think you've missed a few things about bool options, more
> below...
>
> For boolopts examples you can check BR_BOOLOPT_NO_LL_LEARN,
> BR_BOOLOPT_MCAST_VLAN_SNOOPING and BR_BOOLOPT_MST_ENABLE.

Ah yes, will read up on those, thanks!

>> +		if (nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD,
>> +			       br_opt_get(br, BROPT_UNICAST_FLOOD)) ||
>> +		    nla_put_u8(skb, IFLA_BRPORT_MCAST_FLOOD,
>> +			       br_opt_get(br, BROPT_MCAST_FLOOD)) ||
>> +		    nla_put_u8(skb, IFLA_BRPORT_BCAST_FLOOD,
>> +			       br_opt_get(br, BROPT_BCAST_FLOOD)))
>> +			return -EMSGSIZE;
> No. Bool opts are already exposed through IFLA_BR_MULTI_BOOLOPT.

Aha, there it is, awesome!

>> +static void br_set_bropt(struct net_bridge *br, struct nlattr *tb[],
>> +			 int attrtype, enum net_bridge_opts opt)
>> +{
>> +	if (!tb[attrtype])
>> +		return;
>> +
>> +	br_opt_toggle(br, opt, !!nla_get_u8(tb[attrtype]));
>> +}
> These must be controlled via the boolopt api and attributes, not through
> additional nl attributes.

Understood.

>> @@ -1058,9 +1144,9 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>>  				return err;
>>  
>>  			spin_lock_bh(&br->lock);
>> -			err = br_setport(p, tb, extack);
>> +			err = br_setport(br, p, tb, extack);
> setport is for *port* only...

A-firm.

Thank you for the honest review.  Netlink is still much of a mystery in
many ways to me.

Best regards
 /Joachim

Re: [Bridge] [PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge

[Joachim Wiberg]

On Tue, Apr 12, 2022 at 21:24, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> The messy part is in br_setport(), which re-indents a large block of
>> code for the port settings.  To reduce code duplication a few new
>> variables have been added; new_flags and dev.  The latter is used for
>> the recently renamed br_switchdev_set_dev_flag(), which can now be used
>> by underlying switching fabric drivers as another source of information
>> when controlling flooding of unknown BUM traffic to the CPU port.
> Absolutely not. This is just wrong on a few levels and way too hacky.
> Please separate the bridge handling altogether and make it clean.
> No need to integrate it with the port handling,

OK, I'll have a go at it.

> also I think you've missed a few things about bool options, more
> below...
>
> For boolopts examples you can check BR_BOOLOPT_NO_LL_LEARN,
> BR_BOOLOPT_MCAST_VLAN_SNOOPING and BR_BOOLOPT_MST_ENABLE.

Ah yes, will read up on those, thanks!

>> +		if (nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD,
>> +			       br_opt_get(br, BROPT_UNICAST_FLOOD)) ||
>> +		    nla_put_u8(skb, IFLA_BRPORT_MCAST_FLOOD,
>> +			       br_opt_get(br, BROPT_MCAST_FLOOD)) ||
>> +		    nla_put_u8(skb, IFLA_BRPORT_BCAST_FLOOD,
>> +			       br_opt_get(br, BROPT_BCAST_FLOOD)))
>> +			return -EMSGSIZE;
> No. Bool opts are already exposed through IFLA_BR_MULTI_BOOLOPT.

Aha, there it is, awesome!

>> +static void br_set_bropt(struct net_bridge *br, struct nlattr *tb[],
>> +			 int attrtype, enum net_bridge_opts opt)
>> +{
>> +	if (!tb[attrtype])
>> +		return;
>> +
>> +	br_opt_toggle(br, opt, !!nla_get_u8(tb[attrtype]));
>> +}
> These must be controlled via the boolopt api and attributes, not through
> additional nl attributes.

Understood.

>> @@ -1058,9 +1144,9 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
>>  				return err;
>>  
>>  			spin_lock_bh(&br->lock);
>> -			err = br_setport(p, tb, extack);
>> +			err = br_setport(br, p, tb, extack);
> setport is for *port* only...

A-firm.

Thank you for the honest review.  Netlink is still much of a mystery in
many ways to me.

Best regards
 /Joachim

[PATCH RFC net-next 05/13] selftests: forwarding: add TCPDUMP_EXTRA_FLAGS to lib.sh

[Joachim Wiberg]

For some use-cases we may want to change the tcpdump flags used in
tcpdump_start().  For instance, observing interfaces without the PROMISC
flag, e.g. to see what's really being forwarded to the bridge interface.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 tools/testing/selftests/net/forwarding/lib.sh

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
old mode 100644
new mode 100755
index 664b9ecaf228..00cdcab7accf
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1369,7 +1369,13 @@ tcpdump_start()
 		capuser="-Z $SUDO_USER"
 	fi
 
-	$ns_cmd tcpdump -e -n -Q in -i $if_name \
+	if [ -z $TCPDUMP_EXTRA_FLAGS ]; then
+		extra_flags=""
+	else
+		extra_flags="$TCPDUMP_EXTRA_FLAGS"
+	fi
+
+	$ns_cmd tcpdump $extra_flags -e -n -Q in -i $if_name \
 		-s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 &
 	cappid=$!
 
-- 
2.25.1

[Bridge] [PATCH RFC net-next 05/13] selftests: forwarding: add TCPDUMP_EXTRA_FLAGS to lib.sh

[Joachim Wiberg]

For some use-cases we may want to change the tcpdump flags used in
tcpdump_start().  For instance, observing interfaces without the PROMISC
flag, e.g. to see what's really being forwarded to the bridge interface.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 tools/testing/selftests/net/forwarding/lib.sh

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
old mode 100644
new mode 100755
index 664b9ecaf228..00cdcab7accf
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1369,7 +1369,13 @@ tcpdump_start()
 		capuser="-Z $SUDO_USER"
 	fi
 
-	$ns_cmd tcpdump -e -n -Q in -i $if_name \
+	if [ -z $TCPDUMP_EXTRA_FLAGS ]; then
+		extra_flags=""
+	else
+		extra_flags="$TCPDUMP_EXTRA_FLAGS"
+	fi
+
+	$ns_cmd tcpdump $extra_flags -e -n -Q in -i $if_name \
 		-s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 &
 	cappid=$!
 
-- 
2.25.1

Re: [PATCH RFC net-next 05/13] selftests: forwarding: add TCPDUMP_EXTRA_FLAGS to lib.sh

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 03:38:29PM +0200, Joachim Wiberg wrote:
> For some use-cases we may want to change the tcpdump flags used in
> tcpdump_start().  For instance, observing interfaces without the PROMISC
> flag, e.g. to see what's really being forwarded to the bridge interface.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  tools/testing/selftests/net/forwarding/lib.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>  mode change 100644 => 100755 tools/testing/selftests/net/forwarding/lib.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> old mode 100644
> new mode 100755
> index 664b9ecaf228..00cdcab7accf
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -1369,7 +1369,13 @@ tcpdump_start()
>  		capuser="-Z $SUDO_USER"
>  	fi
>  
> -	$ns_cmd tcpdump -e -n -Q in -i $if_name \
> +	if [ -z $TCPDUMP_EXTRA_FLAGS ]; then
> +		extra_flags=""
> +	else
> +		extra_flags="$TCPDUMP_EXTRA_FLAGS"
> +	fi
> +
> +	$ns_cmd tcpdump $extra_flags -e -n -Q in -i $if_name \

Could you call directly "$ns_cmd tcpdump $TCPDUMP_EXTRA_FLAGS ..." here,
without an intermediary "extra_flags" global variable which holds the
same content?

You could initialize it just like the way other variables are
initialized, at the beginning of lib.sh:

TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}

>  		-s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 &
>  	cappid=$!
>  
> -- 
> 2.25.1
>

Re: [Bridge] [PATCH RFC net-next 05/13] selftests: forwarding: add TCPDUMP_EXTRA_FLAGS to lib.sh

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 03:38:29PM +0200, Joachim Wiberg wrote:
> For some use-cases we may want to change the tcpdump flags used in
> tcpdump_start().  For instance, observing interfaces without the PROMISC
> flag, e.g. to see what's really being forwarded to the bridge interface.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  tools/testing/selftests/net/forwarding/lib.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>  mode change 100644 => 100755 tools/testing/selftests/net/forwarding/lib.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> old mode 100644
> new mode 100755
> index 664b9ecaf228..00cdcab7accf
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -1369,7 +1369,13 @@ tcpdump_start()
>  		capuser="-Z $SUDO_USER"
>  	fi
>  
> -	$ns_cmd tcpdump -e -n -Q in -i $if_name \
> +	if [ -z $TCPDUMP_EXTRA_FLAGS ]; then
> +		extra_flags=""
> +	else
> +		extra_flags="$TCPDUMP_EXTRA_FLAGS"
> +	fi
> +
> +	$ns_cmd tcpdump $extra_flags -e -n -Q in -i $if_name \

Could you call directly "$ns_cmd tcpdump $TCPDUMP_EXTRA_FLAGS ..." here,
without an intermediary "extra_flags" global variable which holds the
same content?

You could initialize it just like the way other variables are
initialized, at the beginning of lib.sh:

TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}

>  		-s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 &
>  	cappid=$!
>  
> -- 
> 2.25.1
>

Re: [PATCH RFC net-next 05/13] selftests: forwarding: add TCPDUMP_EXTRA_FLAGS to lib.sh

[Joachim Wiberg]

On Mon, Apr 11, 2022 at 17:20, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Mon, Apr 11, 2022 at 03:38:29PM +0200, Joachim Wiberg wrote:
>> -	$ns_cmd tcpdump -e -n -Q in -i $if_name \
>> +	if [ -z $TCPDUMP_EXTRA_FLAGS ]; then
>> +		extra_flags=""
>> +	else
>> +		extra_flags="$TCPDUMP_EXTRA_FLAGS"
>> +	fi
>> +
>> +	$ns_cmd tcpdump $extra_flags -e -n -Q in -i $if_name \
>
> Could you call directly "$ns_cmd tcpdump $TCPDUMP_EXTRA_FLAGS ..." here,
> without an intermediary "extra_flags" global variable which holds the
> same content?
>
> You could initialize it just like the way other variables are
> initialized, at the beginning of lib.sh:
>
> TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}

Ah, yes of course.  Will fix in the next drop!

 /J
 

Re: [Bridge] [PATCH RFC net-next 05/13] selftests: forwarding: add TCPDUMP_EXTRA_FLAGS to lib.sh

[Joachim Wiberg]

On Mon, Apr 11, 2022 at 17:20, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Mon, Apr 11, 2022 at 03:38:29PM +0200, Joachim Wiberg wrote:
>> -	$ns_cmd tcpdump -e -n -Q in -i $if_name \
>> +	if [ -z $TCPDUMP_EXTRA_FLAGS ]; then
>> +		extra_flags=""
>> +	else
>> +		extra_flags="$TCPDUMP_EXTRA_FLAGS"
>> +	fi
>> +
>> +	$ns_cmd tcpdump $extra_flags -e -n -Q in -i $if_name \
>
> Could you call directly "$ns_cmd tcpdump $TCPDUMP_EXTRA_FLAGS ..." here,
> without an intermediary "extra_flags" global variable which holds the
> same content?
>
> You could initialize it just like the way other variables are
> initialized, at the beginning of lib.sh:
>
> TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}

Ah, yes of course.  Will fix in the next drop!

 /J
 

[PATCH RFC net-next 06/13] selftests: forwarding: multiple instances in tcpdump helper

[Joachim Wiberg]

Extend tcpdump_start() & C:o to handle multiple instances.  Useful when
observing bridge operation, e.g., unicast learning/flooding, and any
case of multicast distribution (to these ports but not that one ...).

This means the interface argument is now a mandatory argument to all
tcpdump_*() functions, hence the changes to the ocelot flower test.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../drivers/net/ocelot/tc_flower_chains.sh    | 24 +++++++++---------
 tools/testing/selftests/net/forwarding/lib.sh | 25 +++++++++++++------
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh b/tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh
index eaf8a04a7ca5..7e684e27a682 100755
--- a/tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh
+++ b/tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh
@@ -215,15 +215,15 @@ test_vlan_pop()
 
 	sleep 1
 
-	tcpdump_stop
+	tcpdump_stop $eth2
 
-	if tcpdump_show | grep -q "$eth3_mac > $eth2_mac, ethertype IPv4"; then
+	if tcpdump_show $eth2 | grep -q "$eth3_mac > $eth2_mac, ethertype IPv4"; then
 		echo "OK"
 	else
 		echo "FAIL"
 	fi
 
-	tcpdump_cleanup
+	tcpdump_cleanup $eth2
 }
 
 test_vlan_push()
@@ -236,15 +236,15 @@ test_vlan_push()
 
 	sleep 1
 
-	tcpdump_stop
+	tcpdump_stop $eth3.100
 
-	if tcpdump_show | grep -q "$eth2_mac > $eth3_mac"; then
+	if tcpdump_show $eth3.100 | grep -q "$eth2_mac > $eth3_mac"; then
 		echo "OK"
 	else
 		echo "FAIL"
 	fi
 
-	tcpdump_cleanup
+	tcpdump_cleanup $eth3.100
 }
 
 test_vlan_ingress_modify()
@@ -267,15 +267,15 @@ test_vlan_ingress_modify()
 
 	sleep 1
 
-	tcpdump_stop
+	tcpdump_stop $eth2
 
-	if tcpdump_show | grep -q "$eth3_mac > $eth2_mac, .* vlan 300"; then
+	if tcpdump_show $eth2 | grep -q "$eth3_mac > $eth2_mac, .* vlan 300"; then
 		echo "OK"
 	else
 		echo "FAIL"
 	fi
 
-	tcpdump_cleanup
+	tcpdump_cleanup $eth2
 
 	tc filter del dev $eth0 ingress chain $(IS1 2) pref 3
 
@@ -305,15 +305,15 @@ test_vlan_egress_modify()
 
 	sleep 1
 
-	tcpdump_stop
+	tcpdump_stop $eth2
 
-	if tcpdump_show | grep -q "$eth3_mac > $eth2_mac, .* vlan 300"; then
+	if tcpdump_show $eth2 | grep -q "$eth3_mac > $eth2_mac, .* vlan 300"; then
 		echo "OK"
 	else
 		echo "FAIL"
 	fi
 
-	tcpdump_cleanup
+	tcpdump_cleanup $eth2
 
 	tc filter del dev $eth1 egress chain $(ES0) pref 3
 	tc qdisc del dev $eth1 clsact
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 00cdcab7accf..20a6d6b2f389 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1349,13 +1349,17 @@ stop_traffic()
 	{ kill %% && wait %%; } 2>/dev/null
 }
 
+declare -A cappid
+declare -A capfile
+declare -A capout
+
 tcpdump_start()
 {
 	local if_name=$1; shift
 	local ns=$1; shift
 
-	capfile=$(mktemp)
-	capout=$(mktemp)
+	capfile[$if_name]=$(mktemp)
+	capout[$if_name]=$(mktemp)
 
 	if [ -z $ns ]; then
 		ns_cmd=""
@@ -1376,26 +1380,33 @@ tcpdump_start()
 	fi
 
 	$ns_cmd tcpdump $extra_flags -e -n -Q in -i $if_name \
-		-s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 &
-	cappid=$!
+		-s 65535 -B 32768 $capuser -w ${capfile[$if_name]} > "${capout[$if_name]}" 2>&1 &
+	cappid[$if_name]=$!
 
 	sleep 1
 }
 
 tcpdump_stop()
 {
-	$ns_cmd kill $cappid
+	local if_name=$1
+	local pid=${cappid[$if_name]}
+
+	$ns_cmd kill "$pid" && wait "$pid"
 	sleep 1
 }
 
 tcpdump_cleanup()
 {
-	rm $capfile $capout
+	local if_name=$1
+
+	rm ${capfile[$if_name]} ${capout[$if_name]}
 }
 
 tcpdump_show()
 {
-	tcpdump -e -n -r $capfile 2>&1
+	local if_name=$1
+
+	tcpdump -e -n -r ${capfile[$if_name]} 2>&1
 }
 
 # return 0 if the packet wasn't seen on host2_if or 1 if it was
-- 
2.25.1

[Bridge] [PATCH RFC net-next 06/13] selftests: forwarding: multiple instances in tcpdump helper

[Joachim Wiberg]

Extend tcpdump_start() & C:o to handle multiple instances.  Useful when
observing bridge operation, e.g., unicast learning/flooding, and any
case of multicast distribution (to these ports but not that one ...).

This means the interface argument is now a mandatory argument to all
tcpdump_*() functions, hence the changes to the ocelot flower test.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../drivers/net/ocelot/tc_flower_chains.sh    | 24 +++++++++---------
 tools/testing/selftests/net/forwarding/lib.sh | 25 +++++++++++++------
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh b/tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh
index eaf8a04a7ca5..7e684e27a682 100755
--- a/tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh
+++ b/tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh
@@ -215,15 +215,15 @@ test_vlan_pop()
 
 	sleep 1
 
-	tcpdump_stop
+	tcpdump_stop $eth2
 
-	if tcpdump_show | grep -q "$eth3_mac > $eth2_mac, ethertype IPv4"; then
+	if tcpdump_show $eth2 | grep -q "$eth3_mac > $eth2_mac, ethertype IPv4"; then
 		echo "OK"
 	else
 		echo "FAIL"
 	fi
 
-	tcpdump_cleanup
+	tcpdump_cleanup $eth2
 }
 
 test_vlan_push()
@@ -236,15 +236,15 @@ test_vlan_push()
 
 	sleep 1
 
-	tcpdump_stop
+	tcpdump_stop $eth3.100
 
-	if tcpdump_show | grep -q "$eth2_mac > $eth3_mac"; then
+	if tcpdump_show $eth3.100 | grep -q "$eth2_mac > $eth3_mac"; then
 		echo "OK"
 	else
 		echo "FAIL"
 	fi
 
-	tcpdump_cleanup
+	tcpdump_cleanup $eth3.100
 }
 
 test_vlan_ingress_modify()
@@ -267,15 +267,15 @@ test_vlan_ingress_modify()
 
 	sleep 1
 
-	tcpdump_stop
+	tcpdump_stop $eth2
 
-	if tcpdump_show | grep -q "$eth3_mac > $eth2_mac, .* vlan 300"; then
+	if tcpdump_show $eth2 | grep -q "$eth3_mac > $eth2_mac, .* vlan 300"; then
 		echo "OK"
 	else
 		echo "FAIL"
 	fi
 
-	tcpdump_cleanup
+	tcpdump_cleanup $eth2
 
 	tc filter del dev $eth0 ingress chain $(IS1 2) pref 3
 
@@ -305,15 +305,15 @@ test_vlan_egress_modify()
 
 	sleep 1
 
-	tcpdump_stop
+	tcpdump_stop $eth2
 
-	if tcpdump_show | grep -q "$eth3_mac > $eth2_mac, .* vlan 300"; then
+	if tcpdump_show $eth2 | grep -q "$eth3_mac > $eth2_mac, .* vlan 300"; then
 		echo "OK"
 	else
 		echo "FAIL"
 	fi
 
-	tcpdump_cleanup
+	tcpdump_cleanup $eth2
 
 	tc filter del dev $eth1 egress chain $(ES0) pref 3
 	tc qdisc del dev $eth1 clsact
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 00cdcab7accf..20a6d6b2f389 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1349,13 +1349,17 @@ stop_traffic()
 	{ kill %% && wait %%; } 2>/dev/null
 }
 
+declare -A cappid
+declare -A capfile
+declare -A capout
+
 tcpdump_start()
 {
 	local if_name=$1; shift
 	local ns=$1; shift
 
-	capfile=$(mktemp)
-	capout=$(mktemp)
+	capfile[$if_name]=$(mktemp)
+	capout[$if_name]=$(mktemp)
 
 	if [ -z $ns ]; then
 		ns_cmd=""
@@ -1376,26 +1380,33 @@ tcpdump_start()
 	fi
 
 	$ns_cmd tcpdump $extra_flags -e -n -Q in -i $if_name \
-		-s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 &
-	cappid=$!
+		-s 65535 -B 32768 $capuser -w ${capfile[$if_name]} > "${capout[$if_name]}" 2>&1 &
+	cappid[$if_name]=$!
 
 	sleep 1
 }
 
 tcpdump_stop()
 {
-	$ns_cmd kill $cappid
+	local if_name=$1
+	local pid=${cappid[$if_name]}
+
+	$ns_cmd kill "$pid" && wait "$pid"
 	sleep 1
 }
 
 tcpdump_cleanup()
 {
-	rm $capfile $capout
+	local if_name=$1
+
+	rm ${capfile[$if_name]} ${capout[$if_name]}
 }
 
 tcpdump_show()
 {
-	tcpdump -e -n -r $capfile 2>&1
+	local if_name=$1
+
+	tcpdump -e -n -r ${capfile[$if_name]} 2>&1
 }
 
 # return 0 if the packet wasn't seen on host2_if or 1 if it was
-- 
2.25.1

Re: [PATCH RFC net-next 06/13] selftests: forwarding: multiple instances in tcpdump helper

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 03:38:30PM +0200, Joachim Wiberg wrote:
> Extend tcpdump_start() & C:o to handle multiple instances.  Useful when
> observing bridge operation, e.g., unicast learning/flooding, and any
> case of multicast distribution (to these ports but not that one ...).
> 
> This means the interface argument is now a mandatory argument to all
> tcpdump_*() functions, hence the changes to the ocelot flower test.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [Bridge] [PATCH RFC net-next 06/13] selftests: forwarding: multiple instances in tcpdump helper

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 03:38:30PM +0200, Joachim Wiberg wrote:
> Extend tcpdump_start() & C:o to handle multiple instances.  Useful when
> observing bridge operation, e.g., unicast learning/flooding, and any
> case of multicast distribution (to these ports but not that one ...).
> 
> This means the interface argument is now a mandatory argument to all
> tcpdump_*() functions, hence the changes to the ocelot flower test.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH RFC net-next 07/13] selftests: forwarding: new test, verify bridge flood flags

[Joachim Wiberg]

Test per-port flood control flags of unknown BUM traffic by injecting
bc/uc/mc on one bridge port and verifying it being forwarded to both
the bridge itself and another regular bridge port.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../testing/selftests/net/forwarding/Makefile |   3 +-
 .../selftests/net/forwarding/bridge_flood.sh  | 170 ++++++++++++++++++
 2 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/forwarding/bridge_flood.sh

diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index ae80c2aef577..873fa61d1ee1 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0+ OR MIT
 
-TEST_PROGS = bridge_igmp.sh \
+TEST_PROGS = bridge_flood.sh \
+	bridge_igmp.sh \
 	bridge_locked_port.sh \
 	bridge_mdb.sh \
 	bridge_port_isolation.sh \
diff --git a/tools/testing/selftests/net/forwarding/bridge_flood.sh b/tools/testing/selftests/net/forwarding/bridge_flood.sh
new file mode 100755
index 000000000000..1966c960d705
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/bridge_flood.sh
@@ -0,0 +1,170 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Verify per-port flood control flags of unknown BUM traffic.
+#
+#                     br0
+#                    /   \
+#                  h1     h2
+#
+# We inject bc/uc/mc on h1, toggle the three flood flags for
+# both br0 and h2, then verify that traffic is flooded as per
+# the flags, and nowhere else.
+#
+#set -x
+
+ALL_TESTS="br_flood_unknown_bc_test br_flood_unknown_uc_test br_flood_unknown_mc_test"
+NUM_NETIFS=4
+
+SRC_MAC="00:de:ad:be:ef:00"
+GRP_IP4="225.1.2.3"
+GRP_MAC="01:00:01:c0:ff:ee"
+GRP_IP6="ff02::42"
+
+BC_PKT="ff:ff:ff:ff:ff:ff $SRC_MAC 00:04 48:45:4c:4f"
+UC_PKT="02:00:01:c0:ff:ee $SRC_MAC 00:04 48:45:4c:4f"
+MC_PKT="01:00:5e:01:02:03 $SRC_MAC 08:00 45:00 00:20 c2:10 00:00 ff 11 12:b2 01:02:03:04 e1:01:02:03 04:d2 10:e1 00:0c 6e:84 48:45:4c:4f"
+
+# Disable promisc to ensure we only receive flooded frames
+export TCPDUMP_EXTRA_FLAGS="-pl"
+
+source lib.sh
+
+h1=${NETIFS[p1]}
+h2=${NETIFS[p3]}
+swp1=${NETIFS[p2]}
+swp2=${NETIFS[p4]}
+
+#
+# Port mappings and flood flag pattern to set/detect
+#
+declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2)
+declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off)
+declare -A flag2=([$swp1]=off [$swp2]=on  [br0]=off)
+declare -A flag3=([$swp1]=off [$swp2]=on  [br0]=on )
+declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on )
+
+switch_create()
+{
+	ip link add dev br0 type bridge
+
+	for port in ${!ports[@]}; do
+		[ "$port" != "br0" ] && ip link set dev $port master br0
+		ip link set dev $port up
+	done
+}
+
+switch_destroy()
+{
+	for port in ${!ports[@]}; do
+		ip link set dev $port down
+	done
+	ip link del dev br0
+}
+
+setup_prepare()
+{
+	vrf_prepare
+
+	let i=1
+	for iface in ${ports[@]}; do
+		[ "$iface" = "br0" ] && continue
+		simple_if_init $iface 192.0.2.$i/24 2001:db8:1::$i/64
+		let i=$((i + 1))
+	done
+
+	switch_create
+}
+
+cleanup()
+{
+	pre_cleanup
+	switch_destroy
+
+	let i=1
+	for iface in ${ports[@]}; do
+		[ "$iface" = "br0" ] && continue
+		simple_if_fini $iface 192.0.2.$i/24 2001:db8:1::$i/64
+		let i=$((i + 1))
+	done
+
+	vrf_cleanup
+}
+
+do_flood_unknown()
+{
+	local type=$1
+	local pass=$2
+	local flag=$3
+	local pkt=$4
+	local -n flags=$5
+
+	RET=0
+	for port in ${!ports[@]}; do
+		if [ "$port" = "br0" ]; then
+			self="self"
+		else
+			self=""
+		fi
+		bridge link set dev $port $flag ${flags[$port]} $self
+		check_err $? "Failed setting $port $flag ${flags[$port]}"
+	done
+
+	for iface in ${ports[@]}; do
+		tcpdump_start $iface
+	done
+
+	$MZ -q $h1 "$pkt"
+	sleep 1
+
+	for iface in ${ports[@]}; do
+		tcpdump_stop $iface
+	done
+
+	for port in ${!ports[@]}; do
+		iface=${ports[$port]}
+
+#		echo "Dumping PCAP from $iface, expecting ${flags[$port]}:"
+#		tcpdump_show $iface
+		tcpdump_show $iface |grep -q "$SRC_MAC"
+		rc=$?
+
+		check_err_fail "${flags[$port]} = on"  $? "failed flooding from $h1 to port $port"
+		check_err_fail "${flags[$port]} = off" $? "flooding from $h1 to port $port"
+	done
+
+	log_test "flood unknown $type pass $pass/4"
+}
+
+br_flood_unknown_bc_test()
+{
+	do_flood_unknown BC 1 bcast_flood "$BC_PKT" flag1
+	do_flood_unknown BC 2 bcast_flood "$BC_PKT" flag2
+	do_flood_unknown BC 3 bcast_flood "$BC_PKT" flag3
+	do_flood_unknown BC 4 bcast_flood "$BC_PKT" flag4
+}
+
+br_flood_unknown_uc_test()
+{
+	do_flood_unknown UC 1 flood "$UC_PKT" flag1
+	do_flood_unknown UC 2 flood "$UC_PKT" flag2
+	do_flood_unknown UC 3 flood "$UC_PKT" flag3
+	do_flood_unknown UC 4 flood "$UC_PKT" flag4
+}
+
+br_flood_unknown_mc_test()
+{
+	do_flood_unknown MC 1 mcast_flood "$MC_PKT" flag1
+	do_flood_unknown MC 2 mcast_flood "$MC_PKT" flag2
+	do_flood_unknown MC 3 mcast_flood "$MC_PKT" flag3
+	do_flood_unknown MC 4 mcast_flood "$MC_PKT" flag4
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
-- 
2.25.1

[Bridge] [PATCH RFC net-next 07/13] selftests: forwarding: new test, verify bridge flood flags

[Joachim Wiberg]

Test per-port flood control flags of unknown BUM traffic by injecting
bc/uc/mc on one bridge port and verifying it being forwarded to both
the bridge itself and another regular bridge port.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../testing/selftests/net/forwarding/Makefile |   3 +-
 .../selftests/net/forwarding/bridge_flood.sh  | 170 ++++++++++++++++++
 2 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/forwarding/bridge_flood.sh

diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index ae80c2aef577..873fa61d1ee1 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0+ OR MIT
 
-TEST_PROGS = bridge_igmp.sh \
+TEST_PROGS = bridge_flood.sh \
+	bridge_igmp.sh \
 	bridge_locked_port.sh \
 	bridge_mdb.sh \
 	bridge_port_isolation.sh \
diff --git a/tools/testing/selftests/net/forwarding/bridge_flood.sh b/tools/testing/selftests/net/forwarding/bridge_flood.sh
new file mode 100755
index 000000000000..1966c960d705
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/bridge_flood.sh
@@ -0,0 +1,170 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Verify per-port flood control flags of unknown BUM traffic.
+#
+#                     br0
+#                    /   \
+#                  h1     h2
+#
+# We inject bc/uc/mc on h1, toggle the three flood flags for
+# both br0 and h2, then verify that traffic is flooded as per
+# the flags, and nowhere else.
+#
+#set -x
+
+ALL_TESTS="br_flood_unknown_bc_test br_flood_unknown_uc_test br_flood_unknown_mc_test"
+NUM_NETIFS=4
+
+SRC_MAC="00:de:ad:be:ef:00"
+GRP_IP4="225.1.2.3"
+GRP_MAC="01:00:01:c0:ff:ee"
+GRP_IP6="ff02::42"
+
+BC_PKT="ff:ff:ff:ff:ff:ff $SRC_MAC 00:04 48:45:4c:4f"
+UC_PKT="02:00:01:c0:ff:ee $SRC_MAC 00:04 48:45:4c:4f"
+MC_PKT="01:00:5e:01:02:03 $SRC_MAC 08:00 45:00 00:20 c2:10 00:00 ff 11 12:b2 01:02:03:04 e1:01:02:03 04:d2 10:e1 00:0c 6e:84 48:45:4c:4f"
+
+# Disable promisc to ensure we only receive flooded frames
+export TCPDUMP_EXTRA_FLAGS="-pl"
+
+source lib.sh
+
+h1=${NETIFS[p1]}
+h2=${NETIFS[p3]}
+swp1=${NETIFS[p2]}
+swp2=${NETIFS[p4]}
+
+#
+# Port mappings and flood flag pattern to set/detect
+#
+declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2)
+declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off)
+declare -A flag2=([$swp1]=off [$swp2]=on  [br0]=off)
+declare -A flag3=([$swp1]=off [$swp2]=on  [br0]=on )
+declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on )
+
+switch_create()
+{
+	ip link add dev br0 type bridge
+
+	for port in ${!ports[@]}; do
+		[ "$port" != "br0" ] && ip link set dev $port master br0
+		ip link set dev $port up
+	done
+}
+
+switch_destroy()
+{
+	for port in ${!ports[@]}; do
+		ip link set dev $port down
+	done
+	ip link del dev br0
+}
+
+setup_prepare()
+{
+	vrf_prepare
+
+	let i=1
+	for iface in ${ports[@]}; do
+		[ "$iface" = "br0" ] && continue
+		simple_if_init $iface 192.0.2.$i/24 2001:db8:1::$i/64
+		let i=$((i + 1))
+	done
+
+	switch_create
+}
+
+cleanup()
+{
+	pre_cleanup
+	switch_destroy
+
+	let i=1
+	for iface in ${ports[@]}; do
+		[ "$iface" = "br0" ] && continue
+		simple_if_fini $iface 192.0.2.$i/24 2001:db8:1::$i/64
+		let i=$((i + 1))
+	done
+
+	vrf_cleanup
+}
+
+do_flood_unknown()
+{
+	local type=$1
+	local pass=$2
+	local flag=$3
+	local pkt=$4
+	local -n flags=$5
+
+	RET=0
+	for port in ${!ports[@]}; do
+		if [ "$port" = "br0" ]; then
+			self="self"
+		else
+			self=""
+		fi
+		bridge link set dev $port $flag ${flags[$port]} $self
+		check_err $? "Failed setting $port $flag ${flags[$port]}"
+	done
+
+	for iface in ${ports[@]}; do
+		tcpdump_start $iface
+	done
+
+	$MZ -q $h1 "$pkt"
+	sleep 1
+
+	for iface in ${ports[@]}; do
+		tcpdump_stop $iface
+	done
+
+	for port in ${!ports[@]}; do
+		iface=${ports[$port]}
+
+#		echo "Dumping PCAP from $iface, expecting ${flags[$port]}:"
+#		tcpdump_show $iface
+		tcpdump_show $iface |grep -q "$SRC_MAC"
+		rc=$?
+
+		check_err_fail "${flags[$port]} = on"  $? "failed flooding from $h1 to port $port"
+		check_err_fail "${flags[$port]} = off" $? "flooding from $h1 to port $port"
+	done
+
+	log_test "flood unknown $type pass $pass/4"
+}
+
+br_flood_unknown_bc_test()
+{
+	do_flood_unknown BC 1 bcast_flood "$BC_PKT" flag1
+	do_flood_unknown BC 2 bcast_flood "$BC_PKT" flag2
+	do_flood_unknown BC 3 bcast_flood "$BC_PKT" flag3
+	do_flood_unknown BC 4 bcast_flood "$BC_PKT" flag4
+}
+
+br_flood_unknown_uc_test()
+{
+	do_flood_unknown UC 1 flood "$UC_PKT" flag1
+	do_flood_unknown UC 2 flood "$UC_PKT" flag2
+	do_flood_unknown UC 3 flood "$UC_PKT" flag3
+	do_flood_unknown UC 4 flood "$UC_PKT" flag4
+}
+
+br_flood_unknown_mc_test()
+{
+	do_flood_unknown MC 1 mcast_flood "$MC_PKT" flag1
+	do_flood_unknown MC 2 mcast_flood "$MC_PKT" flag2
+	do_flood_unknown MC 3 mcast_flood "$MC_PKT" flag3
+	do_flood_unknown MC 4 mcast_flood "$MC_PKT" flag4
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
-- 
2.25.1

Re: [PATCH RFC net-next 07/13] selftests: forwarding: new test, verify bridge flood flags

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote:
> Test per-port flood control flags of unknown BUM traffic by injecting
> bc/uc/mc on one bridge port and verifying it being forwarded to both
> the bridge itself and another regular bridge port.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  .../testing/selftests/net/forwarding/Makefile |   3 +-
>  .../selftests/net/forwarding/bridge_flood.sh  | 170 ++++++++++++++++++
>  2 files changed, 172 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/net/forwarding/bridge_flood.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
> index ae80c2aef577..873fa61d1ee1 100644
> --- a/tools/testing/selftests/net/forwarding/Makefile
> +++ b/tools/testing/selftests/net/forwarding/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+ OR MIT
>  
> -TEST_PROGS = bridge_igmp.sh \
> +TEST_PROGS = bridge_flood.sh \
> +	bridge_igmp.sh \
>  	bridge_locked_port.sh \
>  	bridge_mdb.sh \
>  	bridge_port_isolation.sh \
> diff --git a/tools/testing/selftests/net/forwarding/bridge_flood.sh b/tools/testing/selftests/net/forwarding/bridge_flood.sh
> new file mode 100755
> index 000000000000..1966c960d705
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/bridge_flood.sh
> @@ -0,0 +1,170 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Verify per-port flood control flags of unknown BUM traffic.
> +#
> +#                     br0
> +#                    /   \
> +#                  h1     h2

I think the picture is slightly inaccurate. From it I understand that h1
and h2 are bridge ports, but they are stations attached to the real
bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces.

> +#
> +# We inject bc/uc/mc on h1, toggle the three flood flags for
> +# both br0 and h2, then verify that traffic is flooded as per
> +# the flags, and nowhere else.
> +#
> +#set -x

stray debug line

> +
> +ALL_TESTS="br_flood_unknown_bc_test br_flood_unknown_uc_test br_flood_unknown_mc_test"
> +NUM_NETIFS=4
> +
> +SRC_MAC="00:de:ad:be:ef:00"
> +GRP_IP4="225.1.2.3"
> +GRP_MAC="01:00:01:c0:ff:ee"
> +GRP_IP6="ff02::42"
> +
> +BC_PKT="ff:ff:ff:ff:ff:ff $SRC_MAC 00:04 48:45:4c:4f"

HELO to you too

> +UC_PKT="02:00:01:c0:ff:ee $SRC_MAC 00:04 48:45:4c:4f"
> +MC_PKT="01:00:5e:01:02:03 $SRC_MAC 08:00 45:00 00:20 c2:10 00:00 ff 11 12:b2 01:02:03:04 e1:01:02:03 04:d2 10:e1 00:0c 6e:84 48:45:4c:4f"
> +
> +# Disable promisc to ensure we only receive flooded frames
> +export TCPDUMP_EXTRA_FLAGS="-pl"

Exporting should be required only for sub-shells, doesn't apply when you
source a script.

> +
> +source lib.sh
> +
> +h1=${NETIFS[p1]}
> +h2=${NETIFS[p3]}
> +swp1=${NETIFS[p2]}
> +swp2=${NETIFS[p4]}
> +
> +#
> +# Port mappings and flood flag pattern to set/detect
> +#
> +declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2)

Maybe you could populate the "ports" and the "flagN" arrays in the same
order, i.e. bridge first for all?

Also, to be honest, a generic name like "ports" is hard to digest,
especially since you have another generic variable name "iface".
Maybe "brports" and "station" is a little bit more specific?

> +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off)
> +declare -A flag2=([$swp1]=off [$swp2]=on  [br0]=off)
> +declare -A flag3=([$swp1]=off [$swp2]=on  [br0]=on )
> +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on )

If it's not too much, maybe these could be called "flags_pass1", etc.
Again, it was a bit hard to digest on first read.

> +
> +switch_create()
> +{
> +	ip link add dev br0 type bridge
> +
> +	for port in ${!ports[@]}; do
> +		[ "$port" != "br0" ] && ip link set dev $port master br0
> +		ip link set dev $port up
> +	done
> +}
> +
> +switch_destroy()
> +{
> +	for port in ${!ports[@]}; do
> +		ip link set dev $port down
> +	done
> +	ip link del dev br0
> +}
> +
> +setup_prepare()
> +{
> +	vrf_prepare
> +
> +	let i=1
> +	for iface in ${ports[@]}; do
> +		[ "$iface" = "br0" ] && continue
> +		simple_if_init $iface 192.0.2.$i/24 2001:db8:1::$i/64
> +		let i=$((i + 1))
> +	done
> +
> +	switch_create
> +}
> +
> +cleanup()
> +{
> +	pre_cleanup
> +	switch_destroy
> +
> +	let i=1
> +	for iface in ${ports[@]}; do
> +		[ "$iface" = "br0" ] && continue
> +		simple_if_fini $iface 192.0.2.$i/24 2001:db8:1::$i/64
> +		let i=$((i + 1))
> +	done
> +
> +	vrf_cleanup
> +}
> +
> +do_flood_unknown()
> +{
> +	local type=$1
> +	local pass=$2
> +	local flag=$3
> +	local pkt=$4
> +	local -n flags=$5

I find it slightly less confusing if "flag" and "flags" are next to each
other in the parameter list, since they're related.

> +
> +	RET=0
> +	for port in ${!ports[@]}; do
> +		if [ "$port" = "br0" ]; then
> +			self="self"
> +		else
> +			self=""
> +		fi
> +		bridge link set dev $port $flag ${flags[$port]} $self
> +		check_err $? "Failed setting $port $flag ${flags[$port]}"
> +	done
> +
> +	for iface in ${ports[@]}; do
> +		tcpdump_start $iface
> +	done
> +
> +	$MZ -q $h1 "$pkt"
> +	sleep 1
> +
> +	for iface in ${ports[@]}; do
> +		tcpdump_stop $iface
> +	done
> +
> +	for port in ${!ports[@]}; do
> +		iface=${ports[$port]}
> +
> +#		echo "Dumping PCAP from $iface, expecting ${flags[$port]}:"
> +#		tcpdump_show $iface

Do something about the commented lines.

> +		tcpdump_show $iface |grep -q "$SRC_MAC"

Space between pipe and grep.

> +		rc=$?
> +
> +		check_err_fail "${flags[$port]} = on"  $? "failed flooding from $h1 to port $port"

I think the "failed" word here is superfluous, since check_err_fail
already says "$what succeeded, but should have failed".

> +		check_err_fail "${flags[$port]} = off" $? "flooding from $h1 to port $port"
> +	done
> +
> +	log_test "flood unknown $type pass $pass/4"
> +}
> +
> +br_flood_unknown_bc_test()
> +{
> +	do_flood_unknown BC 1 bcast_flood "$BC_PKT" flag1
> +	do_flood_unknown BC 2 bcast_flood "$BC_PKT" flag2
> +	do_flood_unknown BC 3 bcast_flood "$BC_PKT" flag3
> +	do_flood_unknown BC 4 bcast_flood "$BC_PKT" flag4
> +}
> +
> +br_flood_unknown_uc_test()
> +{
> +	do_flood_unknown UC 1 flood "$UC_PKT" flag1
> +	do_flood_unknown UC 2 flood "$UC_PKT" flag2
> +	do_flood_unknown UC 3 flood "$UC_PKT" flag3
> +	do_flood_unknown UC 4 flood "$UC_PKT" flag4
> +}
> +
> +br_flood_unknown_mc_test()
> +{
> +	do_flood_unknown MC 1 mcast_flood "$MC_PKT" flag1
> +	do_flood_unknown MC 2 mcast_flood "$MC_PKT" flag2
> +	do_flood_unknown MC 3 mcast_flood "$MC_PKT" flag3
> +	do_flood_unknown MC 4 mcast_flood "$MC_PKT" flag4
> +}
> +
> +trap cleanup EXIT
> +
> +setup_prepare
> +setup_wait
> +
> +tests_run
> +
> +exit $EXIT_STATUS
> -- 
> 2.25.1
>

Re: [Bridge] [PATCH RFC net-next 07/13] selftests: forwarding: new test, verify bridge flood flags

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote:
> Test per-port flood control flags of unknown BUM traffic by injecting
> bc/uc/mc on one bridge port and verifying it being forwarded to both
> the bridge itself and another regular bridge port.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  .../testing/selftests/net/forwarding/Makefile |   3 +-
>  .../selftests/net/forwarding/bridge_flood.sh  | 170 ++++++++++++++++++
>  2 files changed, 172 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/net/forwarding/bridge_flood.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
> index ae80c2aef577..873fa61d1ee1 100644
> --- a/tools/testing/selftests/net/forwarding/Makefile
> +++ b/tools/testing/selftests/net/forwarding/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+ OR MIT
>  
> -TEST_PROGS = bridge_igmp.sh \
> +TEST_PROGS = bridge_flood.sh \
> +	bridge_igmp.sh \
>  	bridge_locked_port.sh \
>  	bridge_mdb.sh \
>  	bridge_port_isolation.sh \
> diff --git a/tools/testing/selftests/net/forwarding/bridge_flood.sh b/tools/testing/selftests/net/forwarding/bridge_flood.sh
> new file mode 100755
> index 000000000000..1966c960d705
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/bridge_flood.sh
> @@ -0,0 +1,170 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Verify per-port flood control flags of unknown BUM traffic.
> +#
> +#                     br0
> +#                    /   \
> +#                  h1     h2

I think the picture is slightly inaccurate. From it I understand that h1
and h2 are bridge ports, but they are stations attached to the real
bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces.

> +#
> +# We inject bc/uc/mc on h1, toggle the three flood flags for
> +# both br0 and h2, then verify that traffic is flooded as per
> +# the flags, and nowhere else.
> +#
> +#set -x

stray debug line

> +
> +ALL_TESTS="br_flood_unknown_bc_test br_flood_unknown_uc_test br_flood_unknown_mc_test"
> +NUM_NETIFS=4
> +
> +SRC_MAC="00:de:ad:be:ef:00"
> +GRP_IP4="225.1.2.3"
> +GRP_MAC="01:00:01:c0:ff:ee"
> +GRP_IP6="ff02::42"
> +
> +BC_PKT="ff:ff:ff:ff:ff:ff $SRC_MAC 00:04 48:45:4c:4f"

HELO to you too

> +UC_PKT="02:00:01:c0:ff:ee $SRC_MAC 00:04 48:45:4c:4f"
> +MC_PKT="01:00:5e:01:02:03 $SRC_MAC 08:00 45:00 00:20 c2:10 00:00 ff 11 12:b2 01:02:03:04 e1:01:02:03 04:d2 10:e1 00:0c 6e:84 48:45:4c:4f"
> +
> +# Disable promisc to ensure we only receive flooded frames
> +export TCPDUMP_EXTRA_FLAGS="-pl"

Exporting should be required only for sub-shells, doesn't apply when you
source a script.

> +
> +source lib.sh
> +
> +h1=${NETIFS[p1]}
> +h2=${NETIFS[p3]}
> +swp1=${NETIFS[p2]}
> +swp2=${NETIFS[p4]}
> +
> +#
> +# Port mappings and flood flag pattern to set/detect
> +#
> +declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2)

Maybe you could populate the "ports" and the "flagN" arrays in the same
order, i.e. bridge first for all?

Also, to be honest, a generic name like "ports" is hard to digest,
especially since you have another generic variable name "iface".
Maybe "brports" and "station" is a little bit more specific?

> +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off)
> +declare -A flag2=([$swp1]=off [$swp2]=on  [br0]=off)
> +declare -A flag3=([$swp1]=off [$swp2]=on  [br0]=on )
> +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on )

If it's not too much, maybe these could be called "flags_pass1", etc.
Again, it was a bit hard to digest on first read.

> +
> +switch_create()
> +{
> +	ip link add dev br0 type bridge
> +
> +	for port in ${!ports[@]}; do
> +		[ "$port" != "br0" ] && ip link set dev $port master br0
> +		ip link set dev $port up
> +	done
> +}
> +
> +switch_destroy()
> +{
> +	for port in ${!ports[@]}; do
> +		ip link set dev $port down
> +	done
> +	ip link del dev br0
> +}
> +
> +setup_prepare()
> +{
> +	vrf_prepare
> +
> +	let i=1
> +	for iface in ${ports[@]}; do
> +		[ "$iface" = "br0" ] && continue
> +		simple_if_init $iface 192.0.2.$i/24 2001:db8:1::$i/64
> +		let i=$((i + 1))
> +	done
> +
> +	switch_create
> +}
> +
> +cleanup()
> +{
> +	pre_cleanup
> +	switch_destroy
> +
> +	let i=1
> +	for iface in ${ports[@]}; do
> +		[ "$iface" = "br0" ] && continue
> +		simple_if_fini $iface 192.0.2.$i/24 2001:db8:1::$i/64
> +		let i=$((i + 1))
> +	done
> +
> +	vrf_cleanup
> +}
> +
> +do_flood_unknown()
> +{
> +	local type=$1
> +	local pass=$2
> +	local flag=$3
> +	local pkt=$4
> +	local -n flags=$5

I find it slightly less confusing if "flag" and "flags" are next to each
other in the parameter list, since they're related.

> +
> +	RET=0
> +	for port in ${!ports[@]}; do
> +		if [ "$port" = "br0" ]; then
> +			self="self"
> +		else
> +			self=""
> +		fi
> +		bridge link set dev $port $flag ${flags[$port]} $self
> +		check_err $? "Failed setting $port $flag ${flags[$port]}"
> +	done
> +
> +	for iface in ${ports[@]}; do
> +		tcpdump_start $iface
> +	done
> +
> +	$MZ -q $h1 "$pkt"
> +	sleep 1
> +
> +	for iface in ${ports[@]}; do
> +		tcpdump_stop $iface
> +	done
> +
> +	for port in ${!ports[@]}; do
> +		iface=${ports[$port]}
> +
> +#		echo "Dumping PCAP from $iface, expecting ${flags[$port]}:"
> +#		tcpdump_show $iface

Do something about the commented lines.

> +		tcpdump_show $iface |grep -q "$SRC_MAC"

Space between pipe and grep.

> +		rc=$?
> +
> +		check_err_fail "${flags[$port]} = on"  $? "failed flooding from $h1 to port $port"

I think the "failed" word here is superfluous, since check_err_fail
already says "$what succeeded, but should have failed".

> +		check_err_fail "${flags[$port]} = off" $? "flooding from $h1 to port $port"
> +	done
> +
> +	log_test "flood unknown $type pass $pass/4"
> +}
> +
> +br_flood_unknown_bc_test()
> +{
> +	do_flood_unknown BC 1 bcast_flood "$BC_PKT" flag1
> +	do_flood_unknown BC 2 bcast_flood "$BC_PKT" flag2
> +	do_flood_unknown BC 3 bcast_flood "$BC_PKT" flag3
> +	do_flood_unknown BC 4 bcast_flood "$BC_PKT" flag4
> +}
> +
> +br_flood_unknown_uc_test()
> +{
> +	do_flood_unknown UC 1 flood "$UC_PKT" flag1
> +	do_flood_unknown UC 2 flood "$UC_PKT" flag2
> +	do_flood_unknown UC 3 flood "$UC_PKT" flag3
> +	do_flood_unknown UC 4 flood "$UC_PKT" flag4
> +}
> +
> +br_flood_unknown_mc_test()
> +{
> +	do_flood_unknown MC 1 mcast_flood "$MC_PKT" flag1
> +	do_flood_unknown MC 2 mcast_flood "$MC_PKT" flag2
> +	do_flood_unknown MC 3 mcast_flood "$MC_PKT" flag3
> +	do_flood_unknown MC 4 mcast_flood "$MC_PKT" flag4
> +}
> +
> +trap cleanup EXIT
> +
> +setup_prepare
> +setup_wait
> +
> +tests_run
> +
> +exit $EXIT_STATUS
> -- 
> 2.25.1
>

Re: [PATCH RFC net-next 07/13] selftests: forwarding: new test, verify bridge flood flags

[Joachim Wiberg]

On Mon, Apr 11, 2022 at 20:21, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote:
>> +# Verify per-port flood control flags of unknown BUM traffic.
>> +#
>> +#                     br0
>> +#                    /   \
>> +#                  h1     h2
>
> I think the picture is slightly inaccurate. From it I understand that h1
> and h2 are bridge ports, but they are stations attached to the real
> bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces.

Hmm, yeah either that or drop it entirely.  I sort of assumed everyone
knew about the h<-[veth]->swp (or actual cable) setup, but you're right
this is a bit unclear.  Me and Tobias have internally used h<-->p (for
host<-->bridge-port) and other similar nomenclature.  Finding a good
name that fits easily, and is still readable, in ASCII drawings is hard.
I'll give it a go in the next drop, thanks!

>> +#set -x
> stray debug line

thx

>> +# Disable promisc to ensure we only receive flooded frames
>> +export TCPDUMP_EXTRA_FLAGS="-pl"
> Exporting should be required only for sub-shells, doesn't apply when you
> source a script.

Ah thanks, will fix!

>> +# Port mappings and flood flag pattern to set/detect
>> +declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2)
> Maybe you could populate the "ports" and the "flagN" arrays in the same
> order, i.e. bridge first for all?

Good point, thanks!

> Also, to be honest, a generic name like "ports" is hard to digest,
> especially since you have another generic variable name "iface".
> Maybe "brports" and "station" is a little bit more specific?

Is there a common naming standard between bridge tests, or is it more
important to be consistent the test overview (test heading w/ picture)?

Anyway, I'll have a look at the naming for the next drop.

>> +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off)
>> +declare -A flag2=([$swp1]=off [$swp2]=on  [br0]=off)
>> +declare -A flag3=([$swp1]=off [$swp2]=on  [br0]=on )
>> +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on )
> If it's not too much, maybe these could be called "flags_pass1", etc.
> Again, it was a bit hard to digest on first read.

More like flags_pass_fail, but since its the flooding flags, maybe
flood_patternN would be better?

>> +do_flood_unknown()
>> +{
>> +	local type=$1
>> +	local pass=$2
>> +	local flag=$3
>> +	local pkt=$4
>> +	local -n flags=$5
> I find it slightly less confusing if "flag" and "flags" are next to each
> other in the parameter list, since they're related.

Hmm, OK.

>> +#		echo "Dumping PCAP from $iface, expecting ${flags[$port]}:"
>> +#		tcpdump_show $iface
> Do something about the commented lines.

Oups, thanks!

>> +		tcpdump_show $iface |grep -q "$SRC_MAC"
> Space between pipe and grep.

Will fix!

>> +		check_err_fail "${flags[$port]} = on"  $? "failed flooding from $h1 to port $port"
> I think the "failed" word here is superfluous, since check_err_fail
> already says "$what succeeded, but should have failed".

Ah, good point!

Thank you for the review! <3

 /J
 

Re: [Bridge] [PATCH RFC net-next 07/13] selftests: forwarding: new test, verify bridge flood flags

[Joachim Wiberg]

On Mon, Apr 11, 2022 at 20:21, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote:
>> +# Verify per-port flood control flags of unknown BUM traffic.
>> +#
>> +#                     br0
>> +#                    /   \
>> +#                  h1     h2
>
> I think the picture is slightly inaccurate. From it I understand that h1
> and h2 are bridge ports, but they are stations attached to the real
> bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces.

Hmm, yeah either that or drop it entirely.  I sort of assumed everyone
knew about the h<-[veth]->swp (or actual cable) setup, but you're right
this is a bit unclear.  Me and Tobias have internally used h<-->p (for
host<-->bridge-port) and other similar nomenclature.  Finding a good
name that fits easily, and is still readable, in ASCII drawings is hard.
I'll give it a go in the next drop, thanks!

>> +#set -x
> stray debug line

thx

>> +# Disable promisc to ensure we only receive flooded frames
>> +export TCPDUMP_EXTRA_FLAGS="-pl"
> Exporting should be required only for sub-shells, doesn't apply when you
> source a script.

Ah thanks, will fix!

>> +# Port mappings and flood flag pattern to set/detect
>> +declare -A ports=([br0]=br0 [$swp1]=$h1 [$swp2]=$h2)
> Maybe you could populate the "ports" and the "flagN" arrays in the same
> order, i.e. bridge first for all?

Good point, thanks!

> Also, to be honest, a generic name like "ports" is hard to digest,
> especially since you have another generic variable name "iface".
> Maybe "brports" and "station" is a little bit more specific?

Is there a common naming standard between bridge tests, or is it more
important to be consistent the test overview (test heading w/ picture)?

Anyway, I'll have a look at the naming for the next drop.

>> +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off)
>> +declare -A flag2=([$swp1]=off [$swp2]=on  [br0]=off)
>> +declare -A flag3=([$swp1]=off [$swp2]=on  [br0]=on )
>> +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on )
> If it's not too much, maybe these could be called "flags_pass1", etc.
> Again, it was a bit hard to digest on first read.

More like flags_pass_fail, but since its the flooding flags, maybe
flood_patternN would be better?

>> +do_flood_unknown()
>> +{
>> +	local type=$1
>> +	local pass=$2
>> +	local flag=$3
>> +	local pkt=$4
>> +	local -n flags=$5
> I find it slightly less confusing if "flag" and "flags" are next to each
> other in the parameter list, since they're related.

Hmm, OK.

>> +#		echo "Dumping PCAP from $iface, expecting ${flags[$port]}:"
>> +#		tcpdump_show $iface
> Do something about the commented lines.

Oups, thanks!

>> +		tcpdump_show $iface |grep -q "$SRC_MAC"
> Space between pipe and grep.

Will fix!

>> +		check_err_fail "${flags[$port]} = on"  $? "failed flooding from $h1 to port $port"
> I think the "failed" word here is superfluous, since check_err_fail
> already says "$what succeeded, but should have failed".

Ah, good point!

Thank you for the review! <3

 /J
 

Re: [PATCH RFC net-next 07/13] selftests: forwarding: new test, verify bridge flood flags

[Vladimir Oltean]

On Tue, Apr 12, 2022 at 09:55:31AM +0200, Joachim Wiberg wrote:
> On Mon, Apr 11, 2022 at 20:21, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> > On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote:
> >> +# Verify per-port flood control flags of unknown BUM traffic.
> >> +#
> >> +#                     br0
> >> +#                    /   \
> >> +#                  h1     h2
> >
> > I think the picture is slightly inaccurate. From it I understand that h1
> > and h2 are bridge ports, but they are stations attached to the real
> > bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces.
> 
> Hmm, yeah either that or drop it entirely.  I sort of assumed everyone
> knew about the h<-[veth]->swp (or actual cable) setup, but you're right
> this is a bit unclear.  Me and Tobias have internally used h<-->p (for
> host<-->bridge-port) and other similar nomenclature.  Finding a good
> name that fits easily, and is still readable, in ASCII drawings is hard.
> I'll give it a go in the next drop, thanks!

I wasn't thinking of anything too fancy, this would do I guess.

             br0
            /   \
 h1 --- swp1     swp2 --- h2

> > Also, to be honest, a generic name like "ports" is hard to digest,
> > especially since you have another generic variable name "iface".
> > Maybe "brports" and "station" is a little bit more specific?
> 
> Is there a common naming standard between bridge tests, or is it more
> important to be consistent the test overview (test heading w/ picture)?
> 
> Anyway, I'll have a look at the naming for the next drop.

Even if there is a common naming standard in the selftests I wouldn't
know it. I just found the naming here to be vague enough that it is
confusing. If there are other examples of "port" + "iface" please feel
free to disregard.

> >> +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off)
> >> +declare -A flag2=([$swp1]=off [$swp2]=on  [br0]=off)
> >> +declare -A flag3=([$swp1]=off [$swp2]=on  [br0]=on )
> >> +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on )
> > If it's not too much, maybe these could be called "flags_pass1", etc.
> > Again, it was a bit hard to digest on first read.
> 
> More like flags_pass_fail, but since its the flooding flags, maybe
> flood_patternN would be better?

This works.

Re: [Bridge] [PATCH RFC net-next 07/13] selftests: forwarding: new test, verify bridge flood flags

[Vladimir Oltean]

On Tue, Apr 12, 2022 at 09:55:31AM +0200, Joachim Wiberg wrote:
> On Mon, Apr 11, 2022 at 20:21, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> > On Mon, Apr 11, 2022 at 03:38:31PM +0200, Joachim Wiberg wrote:
> >> +# Verify per-port flood control flags of unknown BUM traffic.
> >> +#
> >> +#                     br0
> >> +#                    /   \
> >> +#                  h1     h2
> >
> > I think the picture is slightly inaccurate. From it I understand that h1
> > and h2 are bridge ports, but they are stations attached to the real
> > bridge ports, swp1 and swp2. Maybe it would be good to draw all interfaces.
> 
> Hmm, yeah either that or drop it entirely.  I sort of assumed everyone
> knew about the h<-[veth]->swp (or actual cable) setup, but you're right
> this is a bit unclear.  Me and Tobias have internally used h<-->p (for
> host<-->bridge-port) and other similar nomenclature.  Finding a good
> name that fits easily, and is still readable, in ASCII drawings is hard.
> I'll give it a go in the next drop, thanks!

I wasn't thinking of anything too fancy, this would do I guess.

             br0
            /   \
 h1 --- swp1     swp2 --- h2

> > Also, to be honest, a generic name like "ports" is hard to digest,
> > especially since you have another generic variable name "iface".
> > Maybe "brports" and "station" is a little bit more specific?
> 
> Is there a common naming standard between bridge tests, or is it more
> important to be consistent the test overview (test heading w/ picture)?
> 
> Anyway, I'll have a look at the naming for the next drop.

Even if there is a common naming standard in the selftests I wouldn't
know it. I just found the naming here to be vague enough that it is
confusing. If there are other examples of "port" + "iface" please feel
free to disregard.

> >> +declare -A flag1=([$swp1]=off [$swp2]=off [br0]=off)
> >> +declare -A flag2=([$swp1]=off [$swp2]=on  [br0]=off)
> >> +declare -A flag3=([$swp1]=off [$swp2]=on  [br0]=on )
> >> +declare -A flag4=([$swp1]=off [$swp2]=off [br0]=on )
> > If it's not too much, maybe these could be called "flags_pass1", etc.
> > Again, it was a bit hard to digest on first read.
> 
> More like flags_pass_fail, but since its the flooding flags, maybe
> flood_patternN would be better?

This works.

[PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Joachim Wiberg]

Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to
the per-port mcast_flood setting, as well as to detected and configured
mcast_router ports.

This patch drops the mrouters_only classifier of unknown IP multicast
and moves the flow handling from br_multicast_flood() to br_flood().
This in turn means br_flood() must know about multicast router ports.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_forward.c   | 11 +++++++++++
 net/bridge/br_multicast.c |  6 +-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 02bb620d3b8d..ab5b97a8c12e 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver(
 void br_flood(struct net_bridge *br, struct sk_buff *skb,
 	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
 {
+	struct net_bridge_mcast *brmctx = &br->multicast_ctx;
+	struct net_bridge_port *rport = NULL;
 	struct net_bridge_port *prev = NULL;
+	struct hlist_node *rp = NULL;
 	struct net_bridge_port *p;
 
+	if (pkt_type == BR_PKT_MULTICAST)
+		rp = br_multicast_get_first_rport_node(brmctx, skb);
+
 	list_for_each_entry_rcu(p, &br->port_list, list) {
 		/* Do not flood unicast traffic to ports that turn it off, nor
 		 * other traffic if flood off, except for traffic we originate
@@ -212,6 +218,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
 				continue;
 			break;
 		case BR_PKT_MULTICAST:
+			rport = br_multicast_rport_from_node_skb(rp, skb);
+			if (rport == p) {
+				rp = rcu_dereference(hlist_next_rcu(rp));
+				break;
+			}
 			if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev)
 				continue;
 			break;
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index db4f2641d1cd..c57e3bbb00ad 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -3643,9 +3643,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge_mcast *brmctx,
 	err = ip_mc_check_igmp(skb);
 
 	if (err == -ENOMSG) {
-		if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) {
-			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
-		} else if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) {
+		if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) {
 			if (ip_hdr(skb)->protocol == IPPROTO_PIM)
 				br_multicast_pim(brmctx, pmctx, skb);
 		} else if (ipv4_is_all_snoopers(ip_hdr(skb)->daddr)) {
@@ -3712,8 +3710,6 @@ static int br_multicast_ipv6_rcv(struct net_bridge_mcast *brmctx,
 	err = ipv6_mc_check_mld(skb);
 
 	if (err == -ENOMSG || err == -ENODATA) {
-		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
-			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
 		if (err == -ENODATA &&
 		    ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr))
 			br_ip6_multicast_mrd_rcv(brmctx, pmctx, skb);
-- 
2.25.1

[Bridge] [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Joachim Wiberg]

Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to
the per-port mcast_flood setting, as well as to detected and configured
mcast_router ports.

This patch drops the mrouters_only classifier of unknown IP multicast
and moves the flow handling from br_multicast_flood() to br_flood().
This in turn means br_flood() must know about multicast router ports.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 net/bridge/br_forward.c   | 11 +++++++++++
 net/bridge/br_multicast.c |  6 +-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 02bb620d3b8d..ab5b97a8c12e 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver(
 void br_flood(struct net_bridge *br, struct sk_buff *skb,
 	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
 {
+	struct net_bridge_mcast *brmctx = &br->multicast_ctx;
+	struct net_bridge_port *rport = NULL;
 	struct net_bridge_port *prev = NULL;
+	struct hlist_node *rp = NULL;
 	struct net_bridge_port *p;
 
+	if (pkt_type == BR_PKT_MULTICAST)
+		rp = br_multicast_get_first_rport_node(brmctx, skb);
+
 	list_for_each_entry_rcu(p, &br->port_list, list) {
 		/* Do not flood unicast traffic to ports that turn it off, nor
 		 * other traffic if flood off, except for traffic we originate
@@ -212,6 +218,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
 				continue;
 			break;
 		case BR_PKT_MULTICAST:
+			rport = br_multicast_rport_from_node_skb(rp, skb);
+			if (rport == p) {
+				rp = rcu_dereference(hlist_next_rcu(rp));
+				break;
+			}
 			if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev)
 				continue;
 			break;
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index db4f2641d1cd..c57e3bbb00ad 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -3643,9 +3643,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge_mcast *brmctx,
 	err = ip_mc_check_igmp(skb);
 
 	if (err == -ENOMSG) {
-		if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) {
-			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
-		} else if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) {
+		if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) {
 			if (ip_hdr(skb)->protocol == IPPROTO_PIM)
 				br_multicast_pim(brmctx, pmctx, skb);
 		} else if (ipv4_is_all_snoopers(ip_hdr(skb)->daddr)) {
@@ -3712,8 +3710,6 @@ static int br_multicast_ipv6_rcv(struct net_bridge_mcast *brmctx,
 	err = ipv6_mc_check_mld(skb);
 
 	if (err == -ENOMSG || err == -ENODATA) {
-		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
-			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
 		if (err == -ENODATA &&
 		    ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr))
 			br_ip6_multicast_mrd_rcv(brmctx, pmctx, skb);
-- 
2.25.1

Re: [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Nikolay Aleksandrov]

On 11/04/2022 16:38, Joachim Wiberg wrote:
> Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to
> the per-port mcast_flood setting, as well as to detected and configured
> mcast_router ports.
> 
> This patch drops the mrouters_only classifier of unknown IP multicast
> and moves the flow handling from br_multicast_flood() to br_flood().
> This in turn means br_flood() must know about multicast router ports.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  net/bridge/br_forward.c   | 11 +++++++++++
>  net/bridge/br_multicast.c |  6 +-----
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 

If you'd like to flood unknown mcast traffic when a router is present please add
a new option which defaults to the current state (disabled).

> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index 02bb620d3b8d..ab5b97a8c12e 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver(
>  void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
>  {
> +	struct net_bridge_mcast *brmctx = &br->multicast_ctx;

Note this breaks per-vlan mcast. You have to use the inferred mctx.

> +	struct net_bridge_port *rport = NULL;
>  	struct net_bridge_port *prev = NULL;
> +	struct hlist_node *rp = NULL;
>  	struct net_bridge_port *p;
>  
> +	if (pkt_type == BR_PKT_MULTICAST)
> +		rp = br_multicast_get_first_rport_node(brmctx, skb);
> +
>  	list_for_each_entry_rcu(p, &br->port_list, list) {
>  		/* Do not flood unicast traffic to ports that turn it off, nor
>  		 * other traffic if flood off, except for traffic we originate
> @@ -212,6 +218,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  				continue;
>  			break;
>  		case BR_PKT_MULTICAST:
> +			rport = br_multicast_rport_from_node_skb(rp, skb);
> +			if (rport == p) {
> +				rp = rcu_dereference(hlist_next_rcu(rp));
> +				break;
> +			}
>  			if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev)
>  				continue;
>  			break;
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index db4f2641d1cd..c57e3bbb00ad 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -3643,9 +3643,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge_mcast *brmctx,
>  	err = ip_mc_check_igmp(skb);
>  
>  	if (err == -ENOMSG) {
> -		if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) {
> -			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> -		} else if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) {
> +		if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) {
>  			if (ip_hdr(skb)->protocol == IPPROTO_PIM)
>  				br_multicast_pim(brmctx, pmctx, skb);
>  		} else if (ipv4_is_all_snoopers(ip_hdr(skb)->daddr)) {
> @@ -3712,8 +3710,6 @@ static int br_multicast_ipv6_rcv(struct net_bridge_mcast *brmctx,
>  	err = ipv6_mc_check_mld(skb);
>  
>  	if (err == -ENOMSG || err == -ENODATA) {
> -		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
> -			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
>  		if (err == -ENODATA &&
>  		    ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr))
>  			br_ip6_multicast_mrd_rcv(brmctx, pmctx, skb);

Re: [Bridge] [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Nikolay Aleksandrov]

On 11/04/2022 16:38, Joachim Wiberg wrote:
> Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to
> the per-port mcast_flood setting, as well as to detected and configured
> mcast_router ports.
> 
> This patch drops the mrouters_only classifier of unknown IP multicast
> and moves the flow handling from br_multicast_flood() to br_flood().
> This in turn means br_flood() must know about multicast router ports.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  net/bridge/br_forward.c   | 11 +++++++++++
>  net/bridge/br_multicast.c |  6 +-----
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 

If you'd like to flood unknown mcast traffic when a router is present please add
a new option which defaults to the current state (disabled).

> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index 02bb620d3b8d..ab5b97a8c12e 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver(
>  void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
>  {
> +	struct net_bridge_mcast *brmctx = &br->multicast_ctx;

Note this breaks per-vlan mcast. You have to use the inferred mctx.

> +	struct net_bridge_port *rport = NULL;
>  	struct net_bridge_port *prev = NULL;
> +	struct hlist_node *rp = NULL;
>  	struct net_bridge_port *p;
>  
> +	if (pkt_type == BR_PKT_MULTICAST)
> +		rp = br_multicast_get_first_rport_node(brmctx, skb);
> +
>  	list_for_each_entry_rcu(p, &br->port_list, list) {
>  		/* Do not flood unicast traffic to ports that turn it off, nor
>  		 * other traffic if flood off, except for traffic we originate
> @@ -212,6 +218,11 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  				continue;
>  			break;
>  		case BR_PKT_MULTICAST:
> +			rport = br_multicast_rport_from_node_skb(rp, skb);
> +			if (rport == p) {
> +				rp = rcu_dereference(hlist_next_rcu(rp));
> +				break;
> +			}
>  			if (!(p->flags & BR_MCAST_FLOOD) && skb->dev != br->dev)
>  				continue;
>  			break;
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index db4f2641d1cd..c57e3bbb00ad 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -3643,9 +3643,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge_mcast *brmctx,
>  	err = ip_mc_check_igmp(skb);
>  
>  	if (err == -ENOMSG) {
> -		if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr)) {
> -			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> -		} else if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) {
> +		if (pim_ipv4_all_pim_routers(ip_hdr(skb)->daddr)) {
>  			if (ip_hdr(skb)->protocol == IPPROTO_PIM)
>  				br_multicast_pim(brmctx, pmctx, skb);
>  		} else if (ipv4_is_all_snoopers(ip_hdr(skb)->daddr)) {
> @@ -3712,8 +3710,6 @@ static int br_multicast_ipv6_rcv(struct net_bridge_mcast *brmctx,
>  	err = ipv6_mc_check_mld(skb);
>  
>  	if (err == -ENOMSG || err == -ENODATA) {
> -		if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
> -			BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
>  		if (err == -ENODATA &&
>  		    ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr))
>  			br_ip6_multicast_mrd_rcv(brmctx, pmctx, skb);

Re: [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Joachim Wiberg]

Hi Nik,

and thank you for taking the time to respond!

On Tue, Apr 12, 2022 at 16:59, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to
>> the per-port mcast_flood setting, as well as to detected and configured
>> mcast_router ports.

I realize I should've included a reference to RFC4541 here.  Will add
that in the non-RFC patch.

>> This patch drops the mrouters_only classifier of unknown IP multicast
>> and moves the flow handling from br_multicast_flood() to br_flood().
>> This in turn means br_flood() must know about multicast router ports.
> If you'd like to flood unknown mcast traffic when a router is present please add
> a new option which defaults to the current state (disabled).

I don't think we have to add another option, because according to the
snooping RFC[1], section 2.1.2 Data Forwarding Rules:

 "3) [..] If a switch receives an unregistered packet, it must forward
  that packet on all ports to which an IGMP[2] router is attached.  A
  switch may default to forwarding unregistered packets on all ports.
  Switches that do not forward unregistered packets to all ports must
  include a configuration option to force the flooding of unregistered
  packets on specified ports. [..]"

From this I'd like to argue that our current behavior in the bridge is
wrong.  To me it's clear that, since we have a confiugration option, we
should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
the router ports).

Also, and more critically, the current behavior of offloaded switches do
forwarding like this already.  So there is a discrepancy currently
between how the bridge forwards unknown multicast and how any underlying
switchcore does it.

Sure, we'll break bridge behavior slightly by forwarding to more ports
than previous (until the group becomes known/registered), but we'd be
standards compliant, and the behavior can still be controlled per-port.

[1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
[2]: Section 3 goes on to explain how this is similar also for MLD

>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
>> index 02bb620d3b8d..ab5b97a8c12e 100644
>> --- a/net/bridge/br_forward.c
>> +++ b/net/bridge/br_forward.c
>> @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver(
>>  void br_flood(struct net_bridge *br, struct sk_buff *skb,
>>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
>>  {
>> +	struct net_bridge_mcast *brmctx = &br->multicast_ctx;
> Note this breaks per-vlan mcast. You have to use the inferred mctx.

Thank you, this was one of the things I was really unsure about since
the introduction of per-VLAN support.  I'll extend the prototype and
include the brmctx from br_handle_frame_finish().  Thanks!

Best regards
 /Joachim

Re: [Bridge] [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Joachim Wiberg]

Hi Nik,

and thank you for taking the time to respond!

On Tue, Apr 12, 2022 at 16:59, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to
>> the per-port mcast_flood setting, as well as to detected and configured
>> mcast_router ports.

I realize I should've included a reference to RFC4541 here.  Will add
that in the non-RFC patch.

>> This patch drops the mrouters_only classifier of unknown IP multicast
>> and moves the flow handling from br_multicast_flood() to br_flood().
>> This in turn means br_flood() must know about multicast router ports.
> If you'd like to flood unknown mcast traffic when a router is present please add
> a new option which defaults to the current state (disabled).

I don't think we have to add another option, because according to the
snooping RFC[1], section 2.1.2 Data Forwarding Rules:

 "3) [..] If a switch receives an unregistered packet, it must forward
  that packet on all ports to which an IGMP[2] router is attached.  A
  switch may default to forwarding unregistered packets on all ports.
  Switches that do not forward unregistered packets to all ports must
  include a configuration option to force the flooding of unregistered
  packets on specified ports. [..]"

From this I'd like to argue that our current behavior in the bridge is
wrong.  To me it's clear that, since we have a confiugration option, we
should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
the router ports).

Also, and more critically, the current behavior of offloaded switches do
forwarding like this already.  So there is a discrepancy currently
between how the bridge forwards unknown multicast and how any underlying
switchcore does it.

Sure, we'll break bridge behavior slightly by forwarding to more ports
than previous (until the group becomes known/registered), but we'd be
standards compliant, and the behavior can still be controlled per-port.

[1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
[2]: Section 3 goes on to explain how this is similar also for MLD

>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
>> index 02bb620d3b8d..ab5b97a8c12e 100644
>> --- a/net/bridge/br_forward.c
>> +++ b/net/bridge/br_forward.c
>> @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver(
>>  void br_flood(struct net_bridge *br, struct sk_buff *skb,
>>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
>>  {
>> +	struct net_bridge_mcast *brmctx = &br->multicast_ctx;
> Note this breaks per-vlan mcast. You have to use the inferred mctx.

Thank you, this was one of the things I was really unsure about since
the introduction of per-VLAN support.  I'll extend the prototype and
include the brmctx from br_handle_frame_finish().  Thanks!

Best regards
 /Joachim

Re: [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Nikolay Aleksandrov]

On 12/04/2022 20:27, Joachim Wiberg wrote:
> 
> Hi Nik,
> 
> and thank you for taking the time to respond!
> 
> On Tue, Apr 12, 2022 at 16:59, Nikolay Aleksandrov <razor@blackwall.org> wrote:
>> On 11/04/2022 16:38, Joachim Wiberg wrote:
>>> Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to
>>> the per-port mcast_flood setting, as well as to detected and configured
>>> mcast_router ports.
> 
> I realize I should've included a reference to RFC4541 here.  Will add
> that in the non-RFC patch.
> 
>>> This patch drops the mrouters_only classifier of unknown IP multicast
>>> and moves the flow handling from br_multicast_flood() to br_flood().
>>> This in turn means br_flood() must know about multicast router ports.
>> If you'd like to flood unknown mcast traffic when a router is present please add
>> a new option which defaults to the current state (disabled).
> 
> I don't think we have to add another option, because according to the
> snooping RFC[1], section 2.1.2 Data Forwarding Rules:
> 
>  "3) [..] If a switch receives an unregistered packet, it must forward
>   that packet on all ports to which an IGMP[2] router is attached.  A
>   switch may default to forwarding unregistered packets on all ports.
>   Switches that do not forward unregistered packets to all ports must
>   include a configuration option to force the flooding of unregistered
>   packets on specified ports. [..]"
> 
> From this I'd like to argue that our current behavior in the bridge is
> wrong.  To me it's clear that, since we have a confiugration option, we
> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
> the router ports).

Definitely not wrong. In fact:
"Switches that do not forward unregistered packets to all ports must
 include a configuration option to force the flooding of unregistered
 packets on specified ports. [..]"

is already implemented because the admin can mark any port as a router and
enable flooding to it.

> 
> Also, and more critically, the current behavior of offloaded switches do
> forwarding like this already.  So there is a discrepancy currently
> between how the bridge forwards unknown multicast and how any underlying
> switchcore does it.
> 
> Sure, we'll break bridge behavior slightly by forwarding to more ports
> than previous (until the group becomes known/registered), but we'd be
> standards compliant, and the behavior can still be controlled per-port.
> 
> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
> [2]: Section 3 goes on to explain how this is similar also for MLD
> 

RFC4541 is only recommending, it's not a mandatory behaviour. This default has been placed
for a very long time and a lot of users and tests take it into consideration.
We cannot break such assumptions and start suddenly flooding packets, but we can
leave it up to the admin or distribution/network software to configure it as default.

>>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
>>> index 02bb620d3b8d..ab5b97a8c12e 100644
>>> --- a/net/bridge/br_forward.c
>>> +++ b/net/bridge/br_forward.c
>>> @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver(
>>>  void br_flood(struct net_bridge *br, struct sk_buff *skb,
>>>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
>>>  {
>>> +	struct net_bridge_mcast *brmctx = &br->multicast_ctx;
>> Note this breaks per-vlan mcast. You have to use the inferred mctx.
> 
> Thank you, this was one of the things I was really unsure about since
> the introduction of per-VLAN support.  I'll extend the prototype and
> include the brmctx from br_handle_frame_finish().  Thanks!
> 
> Best regards
>  /Joachim

Re: [Bridge] [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Nikolay Aleksandrov]

On 12/04/2022 20:27, Joachim Wiberg wrote:
> 
> Hi Nik,
> 
> and thank you for taking the time to respond!
> 
> On Tue, Apr 12, 2022 at 16:59, Nikolay Aleksandrov <razor@blackwall.org> wrote:
>> On 11/04/2022 16:38, Joachim Wiberg wrote:
>>> Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to
>>> the per-port mcast_flood setting, as well as to detected and configured
>>> mcast_router ports.
> 
> I realize I should've included a reference to RFC4541 here.  Will add
> that in the non-RFC patch.
> 
>>> This patch drops the mrouters_only classifier of unknown IP multicast
>>> and moves the flow handling from br_multicast_flood() to br_flood().
>>> This in turn means br_flood() must know about multicast router ports.
>> If you'd like to flood unknown mcast traffic when a router is present please add
>> a new option which defaults to the current state (disabled).
> 
> I don't think we have to add another option, because according to the
> snooping RFC[1], section 2.1.2 Data Forwarding Rules:
> 
>  "3) [..] If a switch receives an unregistered packet, it must forward
>   that packet on all ports to which an IGMP[2] router is attached.  A
>   switch may default to forwarding unregistered packets on all ports.
>   Switches that do not forward unregistered packets to all ports must
>   include a configuration option to force the flooding of unregistered
>   packets on specified ports. [..]"
> 
> From this I'd like to argue that our current behavior in the bridge is
> wrong.  To me it's clear that, since we have a confiugration option, we
> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
> the router ports).

Definitely not wrong. In fact:
"Switches that do not forward unregistered packets to all ports must
 include a configuration option to force the flooding of unregistered
 packets on specified ports. [..]"

is already implemented because the admin can mark any port as a router and
enable flooding to it.

> 
> Also, and more critically, the current behavior of offloaded switches do
> forwarding like this already.  So there is a discrepancy currently
> between how the bridge forwards unknown multicast and how any underlying
> switchcore does it.
> 
> Sure, we'll break bridge behavior slightly by forwarding to more ports
> than previous (until the group becomes known/registered), but we'd be
> standards compliant, and the behavior can still be controlled per-port.
> 
> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
> [2]: Section 3 goes on to explain how this is similar also for MLD
> 

RFC4541 is only recommending, it's not a mandatory behaviour. This default has been placed
for a very long time and a lot of users and tests take it into consideration.
We cannot break such assumptions and start suddenly flooding packets, but we can
leave it up to the admin or distribution/network software to configure it as default.

>>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
>>> index 02bb620d3b8d..ab5b97a8c12e 100644
>>> --- a/net/bridge/br_forward.c
>>> +++ b/net/bridge/br_forward.c
>>> @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver(
>>>  void br_flood(struct net_bridge *br, struct sk_buff *skb,
>>>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
>>>  {
>>> +	struct net_bridge_mcast *brmctx = &br->multicast_ctx;
>> Note this breaks per-vlan mcast. You have to use the inferred mctx.
> 
> Thank you, this was one of the things I was really unsure about since
> the introduction of per-VLAN support.  I'll extend the prototype and
> include the brmctx from br_handle_frame_finish().  Thanks!
> 
> Best regards
>  /Joachim

Re: [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Joachim Wiberg]

On Tue, Apr 12, 2022 at 20:37, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 12/04/2022 20:27, Joachim Wiberg wrote:
>> [snip]
>> From this I'd like to argue that our current behavior in the bridge is
>> wrong.  To me it's clear that, since we have a confiugration option, we
>> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
>> the router ports).
> Definitely not wrong. In fact:
> "Switches that do not forward unregistered packets to all ports must
>  include a configuration option to force the flooding of unregistered
>  packets on specified ports. [..]"
> is already implemented because the admin can mark any port as a router and
> enable flooding to it.

Hmm, I understand your point (here and below), and won't drive this
point further.  Instead I'll pick up on what you said in your first
reply ... (below, last)

Btw, thank you for taking the time to reply and explain your standpoint,
really helps my understanding of how we can develop the bridge further,
without breaking userspace! :)

>> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
> RFC4541 is only recommending, it's not a mandatory behaviour. This
> default has been placed for a very long time and a lot of users and
> tests take it into consideration.

Noted.

> We cannot break such assumptions and start suddenly flooding packets,
> but we can leave it up to the admin or distribution/network software
> to configure it as default.

So, if I add a bridge flag, default off as you mentioned out earlier,
which changes the default behavior of MCAST_FLOOD, then you'd be OK with
that?  Something cheeky like this perhaps:

    if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr))
       	BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541);


Best regards
 /Joachim

Re: [Bridge] [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Joachim Wiberg]

On Tue, Apr 12, 2022 at 20:37, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 12/04/2022 20:27, Joachim Wiberg wrote:
>> [snip]
>> From this I'd like to argue that our current behavior in the bridge is
>> wrong.  To me it's clear that, since we have a confiugration option, we
>> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
>> the router ports).
> Definitely not wrong. In fact:
> "Switches that do not forward unregistered packets to all ports must
>  include a configuration option to force the flooding of unregistered
>  packets on specified ports. [..]"
> is already implemented because the admin can mark any port as a router and
> enable flooding to it.

Hmm, I understand your point (here and below), and won't drive this
point further.  Instead I'll pick up on what you said in your first
reply ... (below, last)

Btw, thank you for taking the time to reply and explain your standpoint,
really helps my understanding of how we can develop the bridge further,
without breaking userspace! :)

>> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
> RFC4541 is only recommending, it's not a mandatory behaviour. This
> default has been placed for a very long time and a lot of users and
> tests take it into consideration.

Noted.

> We cannot break such assumptions and start suddenly flooding packets,
> but we can leave it up to the admin or distribution/network software
> to configure it as default.

So, if I add a bridge flag, default off as you mentioned out earlier,
which changes the default behavior of MCAST_FLOOD, then you'd be OK with
that?  Something cheeky like this perhaps:

    if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr))
       	BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541);


Best regards
 /Joachim

Re: [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Nikolay Aleksandrov]

On 13/04/2022 11:51, Joachim Wiberg wrote:
> On Tue, Apr 12, 2022 at 20:37, Nikolay Aleksandrov <razor@blackwall.org> wrote:
>> On 12/04/2022 20:27, Joachim Wiberg wrote:
>>> [snip]
>>> From this I'd like to argue that our current behavior in the bridge is
>>> wrong.  To me it's clear that, since we have a confiugration option, we
>>> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
>>> the router ports).
>> Definitely not wrong. In fact:
>> "Switches that do not forward unregistered packets to all ports must
>>  include a configuration option to force the flooding of unregistered
>>  packets on specified ports. [..]"
>> is already implemented because the admin can mark any port as a router and
>> enable flooding to it.
> 
> Hmm, I understand your point (here and below), and won't drive this
> point further.  Instead I'll pick up on what you said in your first
> reply ... (below, last)
> 
> Btw, thank you for taking the time to reply and explain your standpoint,
> really helps my understanding of how we can develop the bridge further,
> without breaking userspace! :)
> 
>>> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
>> RFC4541 is only recommending, it's not a mandatory behaviour. This
>> default has been placed for a very long time and a lot of users and
>> tests take it into consideration.
> 
> Noted.
> 
>> We cannot break such assumptions and start suddenly flooding packets,
>> but we can leave it up to the admin or distribution/network software
>> to configure it as default.
> 
> So, if I add a bridge flag, default off as you mentioned out earlier,
> which changes the default behavior of MCAST_FLOOD, then you'd be OK with
> that?  Something cheeky like this perhaps:
> 
>     if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr))
>        	BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541);

Exactly! And that is exactly what I had in mind when I wrote it. :)

Thanks,
 Nik

> 
> 
> Best regards
>  /Joachim

Re: [Bridge] [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Nikolay Aleksandrov]

On 13/04/2022 11:51, Joachim Wiberg wrote:
> On Tue, Apr 12, 2022 at 20:37, Nikolay Aleksandrov <razor@blackwall.org> wrote:
>> On 12/04/2022 20:27, Joachim Wiberg wrote:
>>> [snip]
>>> From this I'd like to argue that our current behavior in the bridge is
>>> wrong.  To me it's clear that, since we have a confiugration option, we
>>> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
>>> the router ports).
>> Definitely not wrong. In fact:
>> "Switches that do not forward unregistered packets to all ports must
>>  include a configuration option to force the flooding of unregistered
>>  packets on specified ports. [..]"
>> is already implemented because the admin can mark any port as a router and
>> enable flooding to it.
> 
> Hmm, I understand your point (here and below), and won't drive this
> point further.  Instead I'll pick up on what you said in your first
> reply ... (below, last)
> 
> Btw, thank you for taking the time to reply and explain your standpoint,
> really helps my understanding of how we can develop the bridge further,
> without breaking userspace! :)
> 
>>> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
>> RFC4541 is only recommending, it's not a mandatory behaviour. This
>> default has been placed for a very long time and a lot of users and
>> tests take it into consideration.
> 
> Noted.
> 
>> We cannot break such assumptions and start suddenly flooding packets,
>> but we can leave it up to the admin or distribution/network software
>> to configure it as default.
> 
> So, if I add a bridge flag, default off as you mentioned out earlier,
> which changes the default behavior of MCAST_FLOOD, then you'd be OK with
> that?  Something cheeky like this perhaps:
> 
>     if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr))
>        	BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541);

Exactly! And that is exactly what I had in mind when I wrote it. :)

Thanks,
 Nik

> 
> 
> Best regards
>  /Joachim

Re: [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Nikolay Aleksandrov]

On 13/04/2022 11:55, Nikolay Aleksandrov wrote:
> On 13/04/2022 11:51, Joachim Wiberg wrote:
>> On Tue, Apr 12, 2022 at 20:37, Nikolay Aleksandrov <razor@blackwall.org> wrote:
>>> On 12/04/2022 20:27, Joachim Wiberg wrote:
>>>> [snip]
>>>> From this I'd like to argue that our current behavior in the bridge is
>>>> wrong.  To me it's clear that, since we have a confiugration option, we
>>>> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
>>>> the router ports).
>>> Definitely not wrong. In fact:
>>> "Switches that do not forward unregistered packets to all ports must
>>>  include a configuration option to force the flooding of unregistered
>>>  packets on specified ports. [..]"
>>> is already implemented because the admin can mark any port as a router and
>>> enable flooding to it.
>>
>> Hmm, I understand your point (here and below), and won't drive this
>> point further.  Instead I'll pick up on what you said in your first
>> reply ... (below, last)
>>
>> Btw, thank you for taking the time to reply and explain your standpoint,
>> really helps my understanding of how we can develop the bridge further,
>> without breaking userspace! :)
>>
>>>> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
>>> RFC4541 is only recommending, it's not a mandatory behaviour. This
>>> default has been placed for a very long time and a lot of users and
>>> tests take it into consideration.
>>
>> Noted.
>>
>>> We cannot break such assumptions and start suddenly flooding packets,
>>> but we can leave it up to the admin or distribution/network software
>>> to configure it as default.
>>
>> So, if I add a bridge flag, default off as you mentioned out earlier,
>> which changes the default behavior of MCAST_FLOOD, then you'd be OK with
>> that?  Something cheeky like this perhaps:
>>
>>     if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr))
>>        	BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541);
> 
> Exactly! And that is exactly what I had in mind when I wrote it. :)
> 

Just please use a different option name that better suggests what it does.

> Thanks,
>  Nik
> 
>>
>>
>> Best regards
>>  /Joachim
> 

Re: [Bridge] [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Nikolay Aleksandrov]

On 13/04/2022 11:55, Nikolay Aleksandrov wrote:
> On 13/04/2022 11:51, Joachim Wiberg wrote:
>> On Tue, Apr 12, 2022 at 20:37, Nikolay Aleksandrov <razor@blackwall.org> wrote:
>>> On 12/04/2022 20:27, Joachim Wiberg wrote:
>>>> [snip]
>>>> From this I'd like to argue that our current behavior in the bridge is
>>>> wrong.  To me it's clear that, since we have a confiugration option, we
>>>> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
>>>> the router ports).
>>> Definitely not wrong. In fact:
>>> "Switches that do not forward unregistered packets to all ports must
>>>  include a configuration option to force the flooding of unregistered
>>>  packets on specified ports. [..]"
>>> is already implemented because the admin can mark any port as a router and
>>> enable flooding to it.
>>
>> Hmm, I understand your point (here and below), and won't drive this
>> point further.  Instead I'll pick up on what you said in your first
>> reply ... (below, last)
>>
>> Btw, thank you for taking the time to reply and explain your standpoint,
>> really helps my understanding of how we can develop the bridge further,
>> without breaking userspace! :)
>>
>>>> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
>>> RFC4541 is only recommending, it's not a mandatory behaviour. This
>>> default has been placed for a very long time and a lot of users and
>>> tests take it into consideration.
>>
>> Noted.
>>
>>> We cannot break such assumptions and start suddenly flooding packets,
>>> but we can leave it up to the admin or distribution/network software
>>> to configure it as default.
>>
>> So, if I add a bridge flag, default off as you mentioned out earlier,
>> which changes the default behavior of MCAST_FLOOD, then you'd be OK with
>> that?  Something cheeky like this perhaps:
>>
>>     if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr))
>>        	BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541);
> 
> Exactly! And that is exactly what I had in mind when I wrote it. :)
> 

Just please use a different option name that better suggests what it does.

> Thanks,
>  Nik
> 
>>
>>
>> Best regards
>>  /Joachim
> 

Re: [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Joachim Wiberg]

On Wed, Apr 13, 2022 at 12:00, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 13/04/2022 11:55, Nikolay Aleksandrov wrote:
>> On 13/04/2022 11:51, Joachim Wiberg wrote:
>>> So, if I add a bridge flag, default off as you mentioned out earlier,
>>> which changes the default behavior of MCAST_FLOOD, then you'd be OK with
>>> that?  Something cheeky like this perhaps:
>>>     if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr))
>>>        	BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541);
>> Exactly! And that is exactly what I had in mind when I wrote it. :)

Awesome, thank you! :)

> Just please use a different option name that better suggests what it does.

Heh, yeah spent a good while with my colleague (Tobias) thinking about
how to name this one.  I'll see what I can come up with, but whatever
shows up in the next patch iteration will be very open for discussion.

Cheers
 /Joachim
 

Re: [Bridge] [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters_only

[Joachim Wiberg]

On Wed, Apr 13, 2022 at 12:00, Nikolay Aleksandrov <razor@blackwall.org> wrote:
> On 13/04/2022 11:55, Nikolay Aleksandrov wrote:
>> On 13/04/2022 11:51, Joachim Wiberg wrote:
>>> So, if I add a bridge flag, default off as you mentioned out earlier,
>>> which changes the default behavior of MCAST_FLOOD, then you'd be OK with
>>> that?  Something cheeky like this perhaps:
>>>     if (!ipv4_is_local_multicast(ip_hdr(skb)->daddr))
>>>        	BR_INPUT_SKB_CB(skb)->mrouters_only = !br_opt_get(br, BROPT_MCAST_FLOOD_RFC4541);
>> Exactly! And that is exactly what I had in mind when I wrote it. :)

Awesome, thank you! :)

> Just please use a different option name that better suggests what it does.

Heh, yeah spent a good while with my colleague (Tobias) thinking about
how to name this one.  I'll see what I can come up with, but whatever
shows up in the next patch iteration will be very open for discussion.

Cheers
 /Joachim
 

[PATCH RFC net-next 09/13] selftests: forwarding: rename test groups for next bridge mdb tests

[Joachim Wiberg]

Rename test groups to PASS and FAIL, respectively, for upcoming changes
to test suite.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh     | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index b1ba6876dd86..c0b84b7d4364 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -7,9 +7,15 @@
 ALL_TESTS="mdb_add_del_test"
 NUM_NETIFS=2
 
-TEST_GROUP_IP4="225.1.2.3"
-TEST_GROUP_IP6="ff02::42"
-TEST_GROUP_MAC="01:00:01:c0:ff:ee"
+PASS_GRP_IP4="225.1.2.3"
+FAIL_GRP_IP4="225.1.2.4"
+
+PASS_GRP_MAC="01:00:01:c0:ff:ee"
+FAIL_GRP_MAC="01:00:01:c0:ff:ef"
+
+PASS_GRP_IP6="ff02::42"
+FAIL_GRP_IP6="ff02::43"
+
 
 source lib.sh
 
@@ -88,9 +94,9 @@ do_mdb_add_del()
 
 mdb_add_del_test()
 {
-	do_mdb_add_del $TEST_GROUP_MAC permanent
-	do_mdb_add_del $TEST_GROUP_IP4
-	do_mdb_add_del $TEST_GROUP_IP6
+	do_mdb_add_del $PASS_GRP_MAC permanent
+	do_mdb_add_del $PASS_GRP_IP4
+	do_mdb_add_del $PASS_GRP_IP6
 }
 
 trap cleanup EXIT
-- 
2.25.1

[Bridge] [PATCH RFC net-next 09/13] selftests: forwarding: rename test groups for next bridge mdb tests

[Joachim Wiberg]

Rename test groups to PASS and FAIL, respectively, for upcoming changes
to test suite.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh     | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index b1ba6876dd86..c0b84b7d4364 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -7,9 +7,15 @@
 ALL_TESTS="mdb_add_del_test"
 NUM_NETIFS=2
 
-TEST_GROUP_IP4="225.1.2.3"
-TEST_GROUP_IP6="ff02::42"
-TEST_GROUP_MAC="01:00:01:c0:ff:ee"
+PASS_GRP_IP4="225.1.2.3"
+FAIL_GRP_IP4="225.1.2.4"
+
+PASS_GRP_MAC="01:00:01:c0:ff:ee"
+FAIL_GRP_MAC="01:00:01:c0:ff:ef"
+
+PASS_GRP_IP6="ff02::42"
+FAIL_GRP_IP6="ff02::43"
+
 
 source lib.sh
 
@@ -88,9 +94,9 @@ do_mdb_add_del()
 
 mdb_add_del_test()
 {
-	do_mdb_add_del $TEST_GROUP_MAC permanent
-	do_mdb_add_del $TEST_GROUP_IP4
-	do_mdb_add_del $TEST_GROUP_IP6
+	do_mdb_add_del $PASS_GRP_MAC permanent
+	do_mdb_add_del $PASS_GRP_IP4
+	do_mdb_add_del $PASS_GRP_IP6
 }
 
 trap cleanup EXIT
-- 
2.25.1

Re: [PATCH RFC net-next 09/13] selftests: forwarding: rename test groups for next bridge mdb tests

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 03:38:33PM +0200, Joachim Wiberg wrote:
> Rename test groups to PASS and FAIL, respectively, for upcoming changes
> to test suite.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  .../selftests/net/forwarding/bridge_mdb.sh     | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> index b1ba6876dd86..c0b84b7d4364 100755
> --- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> +++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> @@ -7,9 +7,15 @@
>  ALL_TESTS="mdb_add_del_test"
>  NUM_NETIFS=2
>  
> -TEST_GROUP_IP4="225.1.2.3"
> -TEST_GROUP_IP6="ff02::42"
> -TEST_GROUP_MAC="01:00:01:c0:ff:ee"
> +PASS_GRP_IP4="225.1.2.3"
> +FAIL_GRP_IP4="225.1.2.4"

This is more than just the advertised rename, the fail groups are new
and not used in this change.

> +
> +PASS_GRP_MAC="01:00:01:c0:ff:ee"
> +FAIL_GRP_MAC="01:00:01:c0:ff:ef"
> +
> +PASS_GRP_IP6="ff02::42"
> +FAIL_GRP_IP6="ff02::43"
> +
>  
>  source lib.sh
>  
> @@ -88,9 +94,9 @@ do_mdb_add_del()
>  
>  mdb_add_del_test()
>  {
> -	do_mdb_add_del $TEST_GROUP_MAC permanent
> -	do_mdb_add_del $TEST_GROUP_IP4
> -	do_mdb_add_del $TEST_GROUP_IP6
> +	do_mdb_add_del $PASS_GRP_MAC permanent
> +	do_mdb_add_del $PASS_GRP_IP4
> +	do_mdb_add_del $PASS_GRP_IP6
>  }
>  
>  trap cleanup EXIT
> -- 
> 2.25.1
>

Re: [Bridge] [PATCH RFC net-next 09/13] selftests: forwarding: rename test groups for next bridge mdb tests

[Vladimir Oltean]

On Mon, Apr 11, 2022 at 03:38:33PM +0200, Joachim Wiberg wrote:
> Rename test groups to PASS and FAIL, respectively, for upcoming changes
> to test suite.
> 
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
> ---
>  .../selftests/net/forwarding/bridge_mdb.sh     | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> index b1ba6876dd86..c0b84b7d4364 100755
> --- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> +++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> @@ -7,9 +7,15 @@
>  ALL_TESTS="mdb_add_del_test"
>  NUM_NETIFS=2
>  
> -TEST_GROUP_IP4="225.1.2.3"
> -TEST_GROUP_IP6="ff02::42"
> -TEST_GROUP_MAC="01:00:01:c0:ff:ee"
> +PASS_GRP_IP4="225.1.2.3"
> +FAIL_GRP_IP4="225.1.2.4"

This is more than just the advertised rename, the fail groups are new
and not used in this change.

> +
> +PASS_GRP_MAC="01:00:01:c0:ff:ee"
> +FAIL_GRP_MAC="01:00:01:c0:ff:ef"
> +
> +PASS_GRP_IP6="ff02::42"
> +FAIL_GRP_IP6="ff02::43"
> +
>  
>  source lib.sh
>  
> @@ -88,9 +94,9 @@ do_mdb_add_del()
>  
>  mdb_add_del_test()
>  {
> -	do_mdb_add_del $TEST_GROUP_MAC permanent
> -	do_mdb_add_del $TEST_GROUP_IP4
> -	do_mdb_add_del $TEST_GROUP_IP6
> +	do_mdb_add_del $PASS_GRP_MAC permanent
> +	do_mdb_add_del $PASS_GRP_IP4
> +	do_mdb_add_del $PASS_GRP_IP6
>  }
>  
>  trap cleanup EXIT
> -- 
> 2.25.1
>

Re: [PATCH RFC net-next 09/13] selftests: forwarding: rename test groups for next bridge mdb tests

[Joachim Wiberg]

On Mon, Apr 11, 2022 at 20:23, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Mon, Apr 11, 2022 at 03:38:33PM +0200, Joachim Wiberg wrote:
>> -TEST_GROUP_IP4="225.1.2.3"
>> -TEST_GROUP_IP6="ff02::42"
>> -TEST_GROUP_MAC="01:00:01:c0:ff:ee"
>> +PASS_GRP_IP4="225.1.2.3"
>> +FAIL_GRP_IP4="225.1.2.4"
>> +PASS_GRP_MAC="01:00:01:c0:ff:ee"
>> +FAIL_GRP_MAC="01:00:01:c0:ff:ef"
>> +
>> +PASS_GRP_IP6="ff02::42"
>> +FAIL_GRP_IP6="ff02::43"
> This is more than just the advertised rename, the fail groups are new
> and not used in this change.

Yeah I rushed the set out to get feedback on the overall take, sorry
about this.  I'll see to fixing this for the non-RFC drop.  Thanks!

Re: [Bridge] [PATCH RFC net-next 09/13] selftests: forwarding: rename test groups for next bridge mdb tests

[Joachim Wiberg]

On Mon, Apr 11, 2022 at 20:23, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Mon, Apr 11, 2022 at 03:38:33PM +0200, Joachim Wiberg wrote:
>> -TEST_GROUP_IP4="225.1.2.3"
>> -TEST_GROUP_IP6="ff02::42"
>> -TEST_GROUP_MAC="01:00:01:c0:ff:ee"
>> +PASS_GRP_IP4="225.1.2.3"
>> +FAIL_GRP_IP4="225.1.2.4"
>> +PASS_GRP_MAC="01:00:01:c0:ff:ee"
>> +FAIL_GRP_MAC="01:00:01:c0:ff:ef"
>> +
>> +PASS_GRP_IP6="ff02::42"
>> +FAIL_GRP_IP6="ff02::43"
> This is more than just the advertised rename, the fail groups are new
> and not used in this change.

Yeah I rushed the set out to get feedback on the overall take, sorry
about this.  I'll see to fixing this for the non-RFC drop.  Thanks!

[PATCH RFC net-next 10/13] selftests: forwarding: verify flooding of unknown multicast

[Joachim Wiberg]

This patch adds forwarding test to bridge_mdb.sh, using groups and group
names that make little sense right now.

Ensure flooding of unknown multicast works as intended after the initial
grace period when we don't know if we are the querier or if someone else
on the LAN is.  Hence the reduced timers.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh    | 135 +++++++++++++++++-
 1 file changed, 131 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index c0b84b7d4364..6de98c59a620 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -3,22 +3,50 @@
 #
 # Verify that adding host mdb entries work as intended for all types of
 # multicast filters: ipv4, ipv6, and mac
+#
+# Verify forwarding (default flooding behavior) to all ports of unknown
+# multicast: MAC, IPv4, IPv6.
+#
+# Note: this test completely disables IPv6 auto-configuration to avoid
+#       any type of dynamic behavior outside of MLD and IGMP protocols.
+#       Static IPv6 addresses are used to ensure consistent behavior,
+#       even in the startup phase when multicast snooping is enabled.
+#
 
-ALL_TESTS="mdb_add_del_test"
-NUM_NETIFS=2
+ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test"
+NUM_NETIFS=4
 
+SRC_PORT="1234"
+DST_PORT="4321"
+
+SRC_ADDR_IP4="1.2.3.4"
 PASS_GRP_IP4="225.1.2.3"
 FAIL_GRP_IP4="225.1.2.4"
 
+SRC_ADDR_MAC="00:de:ad:be:ef:00"
 PASS_GRP_MAC="01:00:01:c0:ff:ee"
 FAIL_GRP_MAC="01:00:01:c0:ff:ef"
 
+PASS_PKT_MAC="$PASS_GRP_MAC $SRC_ADDR_MAC 00:04 48:45:4c:4f"
+FAIL_PKT_MAC="$FAIL_GRP_MAC $SRC_ADDR_MAC 00:04 46:41:49:4c"
+
+PASS_PKT_IP4="01:00:5e:01:02:03 $SRC_ADDR_MAC 08:00 45:00 00:20 c2:10 00:00 ff 11 12:b2 01:02:03:04 e1:01:02:03 04:d2 10:e1 00:0c 6e:84 48:45:4c:4f"
+FAIL_PKT_IP4="01:00:5e:01:02:04 $SRC_ADDR_MAC 08:00 45:00 00:20 dc:e4 00:00 ff 11 f7:dc 01:02:03:04 e1:01:02:04 04:d2 10:e1 00:0c 73:8a 46:41:49:4c"
+
+SRC_ADDR_IP6="ff2e::42"
 PASS_GRP_IP6="ff02::42"
 FAIL_GRP_IP6="ff02::43"
 
+PASS_PKT_IP6="33 33 00 00 00 42 36 1e b4 04 cd e8 86 dd 60 00 01 01 00 08 11 ff ff 2e 00 00 00 00 00 00 00 00 00 00 00 00 00 42 ff 02 00 00 00 00 00 00 00 00 00 00 00 00 00 42 04 d2 10 e1 00 08 eb 75"
+FAIL_PKT_IP6="33 33 00 00 00 43 36 1e b4 04 cd e8 86 dd 60 00 01 01 00 08 11 ff ff 2e 00 00 00 00 00 00 00 00 00 00 00 00 00 42 ff 02 00 00 00 00 00 00 00 00 00 00 00 00 00 43 04 d2 10 e1 00 08 eb 74"
+
+# Disable promisc to ensure we only receive $TEST_GROUP*
+export TCPDUMP_EXTRA_FLAGS="-pl"
 
 source lib.sh
 
+require_command tcpdump
+
 h1_create()
 {
 	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
@@ -29,20 +57,51 @@ h1_destroy()
 	simple_if_fini $h1 192.0.2.1/24 2001:db8:1::1/64
 }
 
+h2_create()
+{
+	simple_if_init $h2 192.0.2.2/24 2001:db8:1::2/64
+}
+
+h2_destroy()
+{
+	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
+}
+
 switch_create()
 {
-	# Enable multicast filtering
-	ip link add dev br0 type bridge mcast_snooping 1
+	# Enable multicast filtering w/ querier, reduce query response
+	# and startup interval to speed up test a bit.
+	ip link add dev br0 type bridge mcast_snooping 1 \
+	   mcast_startup_query_interval 400 mcast_query_response_interval 200
+
+	# Set static IPv6 address before we enable the multicast querier
+	# function.  This, along with disabling IPv6 address auto config
+	# (previously), ensures correct forwarding according to the MDB
+	# even when per-port flooding is disabled, *after* the initial
+	# startup phase when the bridge floods all multicast (according
+	# to its per-port mcast_flood settings.
+	ip addr add 2001:db8:1::42/64 dev br0
+	ip link set br0 type bridge mcast_querier 1
 
 	ip link set dev $swp1 master br0
+	ip link set dev $swp2 master br0
 
 	ip link set dev br0 up
 	ip link set dev $swp1 up
+	ip link set dev $swp2 up
+
+	# Initial delay, when bridge floods all mcast, is set to 200
+	# above (2 sec.)  We wait 3 sec to handle the case when a single
+	# strict fwd test is run directly after the initial setup, e.g.,
+	# TESTS=mdb_ip6_fwd_test
+	sleep 3
 }
 
 switch_destroy()
 {
+	ip link set dev $swp2 down
 	ip link set dev $swp1 down
+
 	ip link del dev br0
 }
 
@@ -51,9 +110,17 @@ setup_prepare()
 	h1=${NETIFS[p1]}
 	swp1=${NETIFS[p2]}
 
+	swp2=${NETIFS[p3]}
+	h2=${NETIFS[p4]}
+
+	# Disable all IPv6 autoconfiguration during test, we want
+	# full control of when MLD queries start etc.
+	sysctl_set net.ipv6.conf.default.accept_ra 0
 	vrf_prepare
 
 	h1_create
+	h2_create
+
 	switch_create
 }
 
@@ -62,9 +129,12 @@ cleanup()
 	pre_cleanup
 
 	switch_destroy
+
+	h2_destroy
 	h1_destroy
 
 	vrf_cleanup
+	sysctl_restore net.ipv6.conf.default.accept_ra
 }
 
 do_mdb_add_del()
@@ -99,6 +169,63 @@ mdb_add_del_test()
 	do_mdb_add_del $PASS_GRP_IP6
 }
 
+do_compat_fwd()
+{
+	port=$1
+	RET=0
+
+	# Ensure default settings, regardless of test start order
+	bridge link set dev "$swp1" mcast_flood on
+	bridge link set dev "$swp2" mcast_flood on
+	bridge link set dev "br0"   mcast_flood on self
+
+	tcpdump_start "$port"
+
+	$MZ -q $h1 "$PASS_PKT_MAC"
+	$MZ -q $h1 "$FAIL_PKT_MAC"
+
+	$MZ -q $h1 "$PASS_PKT_IP4"
+	$MZ -q $h1 "$FAIL_PKT_IP4"
+
+	$MZ -q $h1 "$PASS_PKT_IP6"
+	$MZ -q $h1 "$FAIL_PKT_IP6"
+
+	sleep 1
+	tcpdump_stop "$port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_MAC} > ${PASS_GRP_MAC}"
+	check_err $? "Failed forwarding multicast group $PASS_GRP_MAC from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_MAC} > ${FAIL_GRP_MAC}"
+	check_err $? "Failed forwarding multicast group ${FAIL_GRP_MAC} from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_IP4}.${SRC_PORT} > ${PASS_GRP_IP4}.${DST_PORT}"
+	check_err $? "Failed forwarding multicast group $PASS_GRP_IP4 from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_IP4}.${SRC_PORT} > ${FAIL_GRP_IP4}.${DST_PORT}"
+	check_err $? "Failed forwarding multicast group ${FAIL_GRP_IP4} from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_IP6}.${SRC_PORT} > ${PASS_GRP_IP6}.${DST_PORT}"
+	check_err $? "Failed forwarding multicast group $PASS_GRP_IP6 from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_IP6}.${SRC_PORT} > ${FAIL_GRP_IP6}.${DST_PORT}"
+	check_err $? "Failed forwarding multicast group ${FAIL_GRP_IP6} from $h1 to port $port"
+
+	log_test "MDB forward unknown MAC/IPv4/IPv6 multicast to port $port"
+	tcpdump_cleanup "$port"
+}
+
+#
+# Verify default behavior, unknown multicast is flooded, to both
+# regular bridge ports and the bridge itself (also a port).
+#
+mdb_compat_fwd_test()
+{
+	do_compat_fwd "$h2"
+	do_compat_fwd "br0"
+}
+}
+
 trap cleanup EXIT
 
 setup_prepare
-- 
2.25.1

[Bridge] [PATCH RFC net-next 10/13] selftests: forwarding: verify flooding of unknown multicast

[Joachim Wiberg]

This patch adds forwarding test to bridge_mdb.sh, using groups and group
names that make little sense right now.

Ensure flooding of unknown multicast works as intended after the initial
grace period when we don't know if we are the querier or if someone else
on the LAN is.  Hence the reduced timers.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh    | 135 +++++++++++++++++-
 1 file changed, 131 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index c0b84b7d4364..6de98c59a620 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -3,22 +3,50 @@
 #
 # Verify that adding host mdb entries work as intended for all types of
 # multicast filters: ipv4, ipv6, and mac
+#
+# Verify forwarding (default flooding behavior) to all ports of unknown
+# multicast: MAC, IPv4, IPv6.
+#
+# Note: this test completely disables IPv6 auto-configuration to avoid
+#       any type of dynamic behavior outside of MLD and IGMP protocols.
+#       Static IPv6 addresses are used to ensure consistent behavior,
+#       even in the startup phase when multicast snooping is enabled.
+#
 
-ALL_TESTS="mdb_add_del_test"
-NUM_NETIFS=2
+ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test"
+NUM_NETIFS=4
 
+SRC_PORT="1234"
+DST_PORT="4321"
+
+SRC_ADDR_IP4="1.2.3.4"
 PASS_GRP_IP4="225.1.2.3"
 FAIL_GRP_IP4="225.1.2.4"
 
+SRC_ADDR_MAC="00:de:ad:be:ef:00"
 PASS_GRP_MAC="01:00:01:c0:ff:ee"
 FAIL_GRP_MAC="01:00:01:c0:ff:ef"
 
+PASS_PKT_MAC="$PASS_GRP_MAC $SRC_ADDR_MAC 00:04 48:45:4c:4f"
+FAIL_PKT_MAC="$FAIL_GRP_MAC $SRC_ADDR_MAC 00:04 46:41:49:4c"
+
+PASS_PKT_IP4="01:00:5e:01:02:03 $SRC_ADDR_MAC 08:00 45:00 00:20 c2:10 00:00 ff 11 12:b2 01:02:03:04 e1:01:02:03 04:d2 10:e1 00:0c 6e:84 48:45:4c:4f"
+FAIL_PKT_IP4="01:00:5e:01:02:04 $SRC_ADDR_MAC 08:00 45:00 00:20 dc:e4 00:00 ff 11 f7:dc 01:02:03:04 e1:01:02:04 04:d2 10:e1 00:0c 73:8a 46:41:49:4c"
+
+SRC_ADDR_IP6="ff2e::42"
 PASS_GRP_IP6="ff02::42"
 FAIL_GRP_IP6="ff02::43"
 
+PASS_PKT_IP6="33 33 00 00 00 42 36 1e b4 04 cd e8 86 dd 60 00 01 01 00 08 11 ff ff 2e 00 00 00 00 00 00 00 00 00 00 00 00 00 42 ff 02 00 00 00 00 00 00 00 00 00 00 00 00 00 42 04 d2 10 e1 00 08 eb 75"
+FAIL_PKT_IP6="33 33 00 00 00 43 36 1e b4 04 cd e8 86 dd 60 00 01 01 00 08 11 ff ff 2e 00 00 00 00 00 00 00 00 00 00 00 00 00 42 ff 02 00 00 00 00 00 00 00 00 00 00 00 00 00 43 04 d2 10 e1 00 08 eb 74"
+
+# Disable promisc to ensure we only receive $TEST_GROUP*
+export TCPDUMP_EXTRA_FLAGS="-pl"
 
 source lib.sh
 
+require_command tcpdump
+
 h1_create()
 {
 	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
@@ -29,20 +57,51 @@ h1_destroy()
 	simple_if_fini $h1 192.0.2.1/24 2001:db8:1::1/64
 }
 
+h2_create()
+{
+	simple_if_init $h2 192.0.2.2/24 2001:db8:1::2/64
+}
+
+h2_destroy()
+{
+	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
+}
+
 switch_create()
 {
-	# Enable multicast filtering
-	ip link add dev br0 type bridge mcast_snooping 1
+	# Enable multicast filtering w/ querier, reduce query response
+	# and startup interval to speed up test a bit.
+	ip link add dev br0 type bridge mcast_snooping 1 \
+	   mcast_startup_query_interval 400 mcast_query_response_interval 200
+
+	# Set static IPv6 address before we enable the multicast querier
+	# function.  This, along with disabling IPv6 address auto config
+	# (previously), ensures correct forwarding according to the MDB
+	# even when per-port flooding is disabled, *after* the initial
+	# startup phase when the bridge floods all multicast (according
+	# to its per-port mcast_flood settings.
+	ip addr add 2001:db8:1::42/64 dev br0
+	ip link set br0 type bridge mcast_querier 1
 
 	ip link set dev $swp1 master br0
+	ip link set dev $swp2 master br0
 
 	ip link set dev br0 up
 	ip link set dev $swp1 up
+	ip link set dev $swp2 up
+
+	# Initial delay, when bridge floods all mcast, is set to 200
+	# above (2 sec.)  We wait 3 sec to handle the case when a single
+	# strict fwd test is run directly after the initial setup, e.g.,
+	# TESTS=mdb_ip6_fwd_test
+	sleep 3
 }
 
 switch_destroy()
 {
+	ip link set dev $swp2 down
 	ip link set dev $swp1 down
+
 	ip link del dev br0
 }
 
@@ -51,9 +110,17 @@ setup_prepare()
 	h1=${NETIFS[p1]}
 	swp1=${NETIFS[p2]}
 
+	swp2=${NETIFS[p3]}
+	h2=${NETIFS[p4]}
+
+	# Disable all IPv6 autoconfiguration during test, we want
+	# full control of when MLD queries start etc.
+	sysctl_set net.ipv6.conf.default.accept_ra 0
 	vrf_prepare
 
 	h1_create
+	h2_create
+
 	switch_create
 }
 
@@ -62,9 +129,12 @@ cleanup()
 	pre_cleanup
 
 	switch_destroy
+
+	h2_destroy
 	h1_destroy
 
 	vrf_cleanup
+	sysctl_restore net.ipv6.conf.default.accept_ra
 }
 
 do_mdb_add_del()
@@ -99,6 +169,63 @@ mdb_add_del_test()
 	do_mdb_add_del $PASS_GRP_IP6
 }
 
+do_compat_fwd()
+{
+	port=$1
+	RET=0
+
+	# Ensure default settings, regardless of test start order
+	bridge link set dev "$swp1" mcast_flood on
+	bridge link set dev "$swp2" mcast_flood on
+	bridge link set dev "br0"   mcast_flood on self
+
+	tcpdump_start "$port"
+
+	$MZ -q $h1 "$PASS_PKT_MAC"
+	$MZ -q $h1 "$FAIL_PKT_MAC"
+
+	$MZ -q $h1 "$PASS_PKT_IP4"
+	$MZ -q $h1 "$FAIL_PKT_IP4"
+
+	$MZ -q $h1 "$PASS_PKT_IP6"
+	$MZ -q $h1 "$FAIL_PKT_IP6"
+
+	sleep 1
+	tcpdump_stop "$port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_MAC} > ${PASS_GRP_MAC}"
+	check_err $? "Failed forwarding multicast group $PASS_GRP_MAC from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_MAC} > ${FAIL_GRP_MAC}"
+	check_err $? "Failed forwarding multicast group ${FAIL_GRP_MAC} from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_IP4}.${SRC_PORT} > ${PASS_GRP_IP4}.${DST_PORT}"
+	check_err $? "Failed forwarding multicast group $PASS_GRP_IP4 from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_IP4}.${SRC_PORT} > ${FAIL_GRP_IP4}.${DST_PORT}"
+	check_err $? "Failed forwarding multicast group ${FAIL_GRP_IP4} from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_IP6}.${SRC_PORT} > ${PASS_GRP_IP6}.${DST_PORT}"
+	check_err $? "Failed forwarding multicast group $PASS_GRP_IP6 from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "${SRC_ADDR_IP6}.${SRC_PORT} > ${FAIL_GRP_IP6}.${DST_PORT}"
+	check_err $? "Failed forwarding multicast group ${FAIL_GRP_IP6} from $h1 to port $port"
+
+	log_test "MDB forward unknown MAC/IPv4/IPv6 multicast to port $port"
+	tcpdump_cleanup "$port"
+}
+
+#
+# Verify default behavior, unknown multicast is flooded, to both
+# regular bridge ports and the bridge itself (also a port).
+#
+mdb_compat_fwd_test()
+{
+	do_compat_fwd "$h2"
+	do_compat_fwd "br0"
+}
+}
+
 trap cleanup EXIT
 
 setup_prepare
-- 
2.25.1

[PATCH RFC net-next 11/13] selftests: forwarding: verify strict mdb fwd of known multicast

[Joachim Wiberg]

When mcast_flood is disabled forwarding of multicast should strictly
follow the mdb, and mcast_router ports, dedicated test added later.

This patch updates bridge_mdb.sh with MAC, IPv4 and IPv6 strict MDB
forwarding tests.  The bulk of the work is done by do_mdb_fwd(); one MC
packet to a known group is verified to reach its destination port, and
one MC packet to an unknown group is verified to not be forwarded.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh    | 88 ++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 6de98c59a620..4e3bb950263f 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -7,13 +7,16 @@
 # Verify forwarding (default flooding behavior) to all ports of unknown
 # multicast: MAC, IPv4, IPv6.
 #
+# Verify selective multicast forwarding (strict mdb), when bridge port
+# mcast_flood is disabled, of known MAC, IPv4, IPv6 traffic.
+#
 # Note: this test completely disables IPv6 auto-configuration to avoid
 #       any type of dynamic behavior outside of MLD and IGMP protocols.
 #       Static IPv6 addresses are used to ensure consistent behavior,
 #       even in the startup phase when multicast snooping is enabled.
 #
 
-ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test"
+ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test mdb_mac_fwd_test mdb_ip4_fwd_test mdb_ip6_fwd_test"
 NUM_NETIFS=4
 
 SRC_PORT="1234"
@@ -224,6 +227,89 @@ mdb_compat_fwd_test()
 	do_compat_fwd "$h2"
 	do_compat_fwd "br0"
 }
+
+do_mdb_fwd()
+{
+	type=$1
+	port=$2
+	swp=$port
+	src=$3
+	pass_grp=$4
+	fail_grp=$5
+	pass_pkt=$6
+	fail_pkt=$7
+	RET=0
+
+	if [ "$type" = "MAC" ]; then
+		flag="permanent"
+	else
+		flag=""
+		spt=".$SRC_PORT"
+		dpt=".$DST_PORT"
+	fi
+	if [ "$port" = "$h2" ]; then
+		swp=$swp2
+	fi
+
+	# Disable flooding of unknown multicast, strict MDB forwarding
+	bridge link set dev "$swp1" mcast_flood off
+	bridge link set dev "$swp2" mcast_flood off
+	bridge link set dev "br0"   mcast_flood off self
+
+	# Static filter only to this port
+	bridge mdb add dev br0 port "$swp" grp "$pass_grp" $flag
+	check_err $? "Failed adding $type multicast group $pass_grp to bridge port $swp"
+
+	tcpdump_start "$port"
+
+	# Real data we're expecting
+	$MZ -q "$h1" "$pass_pkt"
+	# This should not pass
+	$MZ -q "$h1" "$fail_pkt"
+
+	sleep 1
+	tcpdump_stop "$port"
+
+	tcpdump_show "$port" |grep -q "$src$spt > $pass_grp$dpt"
+	check_err $? "Failed forwarding $type multicast $pass_grp from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "$src$spt > $fail_grp$dpt"
+	check_err_fail 1 $? "Received $type multicast group $fail_grp from $h1 to port $port"
+
+	bridge mdb del dev br0 port "$swp" grp "$pass_grp"
+
+	log_test "MDB forward $type multicast to bridge port $port"
+	tcpdump_cleanup "$port"
+}
+
+#
+# Forwarding of known MAC multicast according to mdb, verify blocking
+# unknown MAC multicast (flood off)
+#
+mdb_mac_fwd_test()
+{
+	do_mdb_fwd MAC "br0" $SRC_ADDR_MAC $PASS_GRP_MAC $FAIL_GRP_MAC "$PASS_PKT_MAC" "$FAIL_PKT_MAC"
+	do_mdb_fwd MAC "$h2" $SRC_ADDR_MAC $PASS_GRP_MAC $FAIL_GRP_MAC "$PASS_PKT_MAC" "$FAIL_PKT_MAC"
+}
+
+#
+# Forwarding of known IPv4 UDP multicast according to mdb, verify
+# blocking unknown IPv4 UDP multicast (flood off)
+#
+mdb_ip4_fwd_test()
+{
+	do_mdb_fwd IPv4 br0 $SRC_ADDR_IP4 $PASS_GRP_IP4 $FAIL_GRP_IP4 "$PASS_PKT_IP4" "$FAIL_PKT_IP4"
+	do_mdb_fwd IPv4 $h2 $SRC_ADDR_IP4 $PASS_GRP_IP4 $FAIL_GRP_IP4 "$PASS_PKT_IP4" "$FAIL_PKT_IP4"
+}
+
+#
+# Forwarding of known IPv6 UDP multicast according to mdb, verify
+# blocking unknown IPv6 UDP multicast (flood off)
+#
+mdb_ip6_fwd_test()
+{
+	do_mdb_fwd IPv6 br0 $SRC_ADDR_IP6 $PASS_GRP_IP6 $FAIL_GRP_IP6 "$PASS_PKT_IP6" "$FAIL_PKT_IP6"
+	do_mdb_fwd IPv6 $h2 $SRC_ADDR_IP6 $PASS_GRP_IP6 $FAIL_GRP_IP6 "$PASS_PKT_IP6" "$FAIL_PKT_IP6"
 }
 
 trap cleanup EXIT
-- 
2.25.1

[Bridge] [PATCH RFC net-next 11/13] selftests: forwarding: verify strict mdb fwd of known multicast

[Joachim Wiberg]

When mcast_flood is disabled forwarding of multicast should strictly
follow the mdb, and mcast_router ports, dedicated test added later.

This patch updates bridge_mdb.sh with MAC, IPv4 and IPv6 strict MDB
forwarding tests.  The bulk of the work is done by do_mdb_fwd(); one MC
packet to a known group is verified to reach its destination port, and
one MC packet to an unknown group is verified to not be forwarded.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh    | 88 ++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 6de98c59a620..4e3bb950263f 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -7,13 +7,16 @@
 # Verify forwarding (default flooding behavior) to all ports of unknown
 # multicast: MAC, IPv4, IPv6.
 #
+# Verify selective multicast forwarding (strict mdb), when bridge port
+# mcast_flood is disabled, of known MAC, IPv4, IPv6 traffic.
+#
 # Note: this test completely disables IPv6 auto-configuration to avoid
 #       any type of dynamic behavior outside of MLD and IGMP protocols.
 #       Static IPv6 addresses are used to ensure consistent behavior,
 #       even in the startup phase when multicast snooping is enabled.
 #
 
-ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test"
+ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test mdb_mac_fwd_test mdb_ip4_fwd_test mdb_ip6_fwd_test"
 NUM_NETIFS=4
 
 SRC_PORT="1234"
@@ -224,6 +227,89 @@ mdb_compat_fwd_test()
 	do_compat_fwd "$h2"
 	do_compat_fwd "br0"
 }
+
+do_mdb_fwd()
+{
+	type=$1
+	port=$2
+	swp=$port
+	src=$3
+	pass_grp=$4
+	fail_grp=$5
+	pass_pkt=$6
+	fail_pkt=$7
+	RET=0
+
+	if [ "$type" = "MAC" ]; then
+		flag="permanent"
+	else
+		flag=""
+		spt=".$SRC_PORT"
+		dpt=".$DST_PORT"
+	fi
+	if [ "$port" = "$h2" ]; then
+		swp=$swp2
+	fi
+
+	# Disable flooding of unknown multicast, strict MDB forwarding
+	bridge link set dev "$swp1" mcast_flood off
+	bridge link set dev "$swp2" mcast_flood off
+	bridge link set dev "br0"   mcast_flood off self
+
+	# Static filter only to this port
+	bridge mdb add dev br0 port "$swp" grp "$pass_grp" $flag
+	check_err $? "Failed adding $type multicast group $pass_grp to bridge port $swp"
+
+	tcpdump_start "$port"
+
+	# Real data we're expecting
+	$MZ -q "$h1" "$pass_pkt"
+	# This should not pass
+	$MZ -q "$h1" "$fail_pkt"
+
+	sleep 1
+	tcpdump_stop "$port"
+
+	tcpdump_show "$port" |grep -q "$src$spt > $pass_grp$dpt"
+	check_err $? "Failed forwarding $type multicast $pass_grp from $h1 to port $port"
+
+	tcpdump_show "$port" |grep -q "$src$spt > $fail_grp$dpt"
+	check_err_fail 1 $? "Received $type multicast group $fail_grp from $h1 to port $port"
+
+	bridge mdb del dev br0 port "$swp" grp "$pass_grp"
+
+	log_test "MDB forward $type multicast to bridge port $port"
+	tcpdump_cleanup "$port"
+}
+
+#
+# Forwarding of known MAC multicast according to mdb, verify blocking
+# unknown MAC multicast (flood off)
+#
+mdb_mac_fwd_test()
+{
+	do_mdb_fwd MAC "br0" $SRC_ADDR_MAC $PASS_GRP_MAC $FAIL_GRP_MAC "$PASS_PKT_MAC" "$FAIL_PKT_MAC"
+	do_mdb_fwd MAC "$h2" $SRC_ADDR_MAC $PASS_GRP_MAC $FAIL_GRP_MAC "$PASS_PKT_MAC" "$FAIL_PKT_MAC"
+}
+
+#
+# Forwarding of known IPv4 UDP multicast according to mdb, verify
+# blocking unknown IPv4 UDP multicast (flood off)
+#
+mdb_ip4_fwd_test()
+{
+	do_mdb_fwd IPv4 br0 $SRC_ADDR_IP4 $PASS_GRP_IP4 $FAIL_GRP_IP4 "$PASS_PKT_IP4" "$FAIL_PKT_IP4"
+	do_mdb_fwd IPv4 $h2 $SRC_ADDR_IP4 $PASS_GRP_IP4 $FAIL_GRP_IP4 "$PASS_PKT_IP4" "$FAIL_PKT_IP4"
+}
+
+#
+# Forwarding of known IPv6 UDP multicast according to mdb, verify
+# blocking unknown IPv6 UDP multicast (flood off)
+#
+mdb_ip6_fwd_test()
+{
+	do_mdb_fwd IPv6 br0 $SRC_ADDR_IP6 $PASS_GRP_IP6 $FAIL_GRP_IP6 "$PASS_PKT_IP6" "$FAIL_PKT_IP6"
+	do_mdb_fwd IPv6 $h2 $SRC_ADDR_IP6 $PASS_GRP_IP6 $FAIL_GRP_IP6 "$PASS_PKT_IP6" "$FAIL_PKT_IP6"
 }
 
 trap cleanup EXIT
-- 
2.25.1

[PATCH RFC net-next 12/13] selftests: forwarding: verify strict filtering doesn't leak

[Joachim Wiberg]

Add a another veth pair to get a dedicated canary port.  This to help us
verify that multicast is received by only a subset of the ports in the
MDB filter.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh    | 32 ++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 4e3bb950263f..137bc79fd677 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -17,7 +17,7 @@
 #
 
 ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test mdb_mac_fwd_test mdb_ip4_fwd_test mdb_ip6_fwd_test"
-NUM_NETIFS=4
+NUM_NETIFS=6
 
 SRC_PORT="1234"
 DST_PORT="4321"
@@ -70,6 +70,16 @@ h2_destroy()
 	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
 }
 
+h3_create()
+{
+	simple_if_init $h3 192.0.2.3/24 2001:db8:1::3/64
+}
+
+h3_destroy()
+{
+	simple_if_fini $h3 192.0.2.3/24 2001:db8:1::3/64
+}
+
 switch_create()
 {
 	# Enable multicast filtering w/ querier, reduce query response
@@ -88,10 +98,12 @@ switch_create()
 
 	ip link set dev $swp1 master br0
 	ip link set dev $swp2 master br0
+	ip link set dev $swp3 master br0
 
 	ip link set dev br0 up
 	ip link set dev $swp1 up
 	ip link set dev $swp2 up
+	ip link set dev $swp3 up
 
 	# Initial delay, when bridge floods all mcast, is set to 200
 	# above (2 sec.)  We wait 3 sec to handle the case when a single
@@ -102,6 +114,7 @@ switch_create()
 
 switch_destroy()
 {
+	ip link set dev $swp3 down
 	ip link set dev $swp2 down
 	ip link set dev $swp1 down
 
@@ -116,6 +129,9 @@ setup_prepare()
 	swp2=${NETIFS[p3]}
 	h2=${NETIFS[p4]}
 
+	swp3=${NETIFS[p5]}
+	h3=${NETIFS[p6]}
+
 	# Disable all IPv6 autoconfiguration during test, we want
 	# full control of when MLD queries start etc.
 	sysctl_set net.ipv6.conf.default.accept_ra 0
@@ -123,6 +139,7 @@ setup_prepare()
 
 	h1_create
 	h2_create
+	h3_create
 
 	switch_create
 }
@@ -133,6 +150,7 @@ cleanup()
 
 	switch_destroy
 
+	h3_destroy
 	h2_destroy
 	h1_destroy
 
@@ -249,11 +267,15 @@ do_mdb_fwd()
 	fi
 	if [ "$port" = "$h2" ]; then
 		swp=$swp2
+		nop="$h3"
+	else
+		nop="$h2"
 	fi
 
 	# Disable flooding of unknown multicast, strict MDB forwarding
 	bridge link set dev "$swp1" mcast_flood off
 	bridge link set dev "$swp2" mcast_flood off
+	bridge link set dev "$swp3" mcast_flood off
 	bridge link set dev "br0"   mcast_flood off self
 
 	# Static filter only to this port
@@ -261,6 +283,7 @@ do_mdb_fwd()
 	check_err $? "Failed adding $type multicast group $pass_grp to bridge port $swp"
 
 	tcpdump_start "$port"
+	tcpdump_start "$nop"
 
 	# Real data we're expecting
 	$MZ -q "$h1" "$pass_pkt"
@@ -268,6 +291,7 @@ do_mdb_fwd()
 	$MZ -q "$h1" "$fail_pkt"
 
 	sleep 1
+	tcpdump_stop "$nop"
 	tcpdump_stop "$port"
 
 	tcpdump_show "$port" |grep -q "$src$spt > $pass_grp$dpt"
@@ -276,6 +300,12 @@ do_mdb_fwd()
 	tcpdump_show "$port" |grep -q "$src$spt > $fail_grp$dpt"
 	check_err_fail 1 $? "Received $type multicast group $fail_grp from $h1 to port $port"
 
+	# Verify we don't get multicast to the canary port
+	tcpdump_show "$nop" |grep -q "$src$spt > $pass_grp$dpt"
+	check_err_fail 1 $? "Received $type multicast group $pass_grp from $h1 to port $nop"
+	tcpdump_show "$nop" |grep -q "$src$spt > $fail_grp$dpt"
+	check_err_fail 1 $? "Received $type multicast group $fail_grp from $h1 to port $nop"
+
 	bridge mdb del dev br0 port "$swp" grp "$pass_grp"
 
 	log_test "MDB forward $type multicast to bridge port $port"
-- 
2.25.1

[Bridge] [PATCH RFC net-next 12/13] selftests: forwarding: verify strict filtering doesn't leak

[Joachim Wiberg]

Add a another veth pair to get a dedicated canary port.  This to help us
verify that multicast is received by only a subset of the ports in the
MDB filter.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh    | 32 ++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 4e3bb950263f..137bc79fd677 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -17,7 +17,7 @@
 #
 
 ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test mdb_mac_fwd_test mdb_ip4_fwd_test mdb_ip6_fwd_test"
-NUM_NETIFS=4
+NUM_NETIFS=6
 
 SRC_PORT="1234"
 DST_PORT="4321"
@@ -70,6 +70,16 @@ h2_destroy()
 	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
 }
 
+h3_create()
+{
+	simple_if_init $h3 192.0.2.3/24 2001:db8:1::3/64
+}
+
+h3_destroy()
+{
+	simple_if_fini $h3 192.0.2.3/24 2001:db8:1::3/64
+}
+
 switch_create()
 {
 	# Enable multicast filtering w/ querier, reduce query response
@@ -88,10 +98,12 @@ switch_create()
 
 	ip link set dev $swp1 master br0
 	ip link set dev $swp2 master br0
+	ip link set dev $swp3 master br0
 
 	ip link set dev br0 up
 	ip link set dev $swp1 up
 	ip link set dev $swp2 up
+	ip link set dev $swp3 up
 
 	# Initial delay, when bridge floods all mcast, is set to 200
 	# above (2 sec.)  We wait 3 sec to handle the case when a single
@@ -102,6 +114,7 @@ switch_create()
 
 switch_destroy()
 {
+	ip link set dev $swp3 down
 	ip link set dev $swp2 down
 	ip link set dev $swp1 down
 
@@ -116,6 +129,9 @@ setup_prepare()
 	swp2=${NETIFS[p3]}
 	h2=${NETIFS[p4]}
 
+	swp3=${NETIFS[p5]}
+	h3=${NETIFS[p6]}
+
 	# Disable all IPv6 autoconfiguration during test, we want
 	# full control of when MLD queries start etc.
 	sysctl_set net.ipv6.conf.default.accept_ra 0
@@ -123,6 +139,7 @@ setup_prepare()
 
 	h1_create
 	h2_create
+	h3_create
 
 	switch_create
 }
@@ -133,6 +150,7 @@ cleanup()
 
 	switch_destroy
 
+	h3_destroy
 	h2_destroy
 	h1_destroy
 
@@ -249,11 +267,15 @@ do_mdb_fwd()
 	fi
 	if [ "$port" = "$h2" ]; then
 		swp=$swp2
+		nop="$h3"
+	else
+		nop="$h2"
 	fi
 
 	# Disable flooding of unknown multicast, strict MDB forwarding
 	bridge link set dev "$swp1" mcast_flood off
 	bridge link set dev "$swp2" mcast_flood off
+	bridge link set dev "$swp3" mcast_flood off
 	bridge link set dev "br0"   mcast_flood off self
 
 	# Static filter only to this port
@@ -261,6 +283,7 @@ do_mdb_fwd()
 	check_err $? "Failed adding $type multicast group $pass_grp to bridge port $swp"
 
 	tcpdump_start "$port"
+	tcpdump_start "$nop"
 
 	# Real data we're expecting
 	$MZ -q "$h1" "$pass_pkt"
@@ -268,6 +291,7 @@ do_mdb_fwd()
 	$MZ -q "$h1" "$fail_pkt"
 
 	sleep 1
+	tcpdump_stop "$nop"
 	tcpdump_stop "$port"
 
 	tcpdump_show "$port" |grep -q "$src$spt > $pass_grp$dpt"
@@ -276,6 +300,12 @@ do_mdb_fwd()
 	tcpdump_show "$port" |grep -q "$src$spt > $fail_grp$dpt"
 	check_err_fail 1 $? "Received $type multicast group $fail_grp from $h1 to port $port"
 
+	# Verify we don't get multicast to the canary port
+	tcpdump_show "$nop" |grep -q "$src$spt > $pass_grp$dpt"
+	check_err_fail 1 $? "Received $type multicast group $pass_grp from $h1 to port $nop"
+	tcpdump_show "$nop" |grep -q "$src$spt > $fail_grp$dpt"
+	check_err_fail 1 $? "Received $type multicast group $fail_grp from $h1 to port $nop"
+
 	bridge mdb del dev br0 port "$swp" grp "$pass_grp"
 
 	log_test "MDB forward $type multicast to bridge port $port"
-- 
2.25.1

[PATCH RFC net-next 13/13] selftests: forwarding: verify flood of known mc on mcast_router port

[Joachim Wiberg]

This test verifies that both known (in mdb) and unknown IP multicast is
forwarded to a mcast_router port.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh    | 54 ++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 137bc79fd677..3fd7d68bca09 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -10,13 +10,16 @@
 # Verify selective multicast forwarding (strict mdb), when bridge port
 # mcast_flood is disabled, of known MAC, IPv4, IPv6 traffic.
 #
+# Verify flooding towards mcast_router ports of known IP multicast.
+#
 # Note: this test completely disables IPv6 auto-configuration to avoid
 #       any type of dynamic behavior outside of MLD and IGMP protocols.
 #       Static IPv6 addresses are used to ensure consistent behavior,
 #       even in the startup phase when multicast snooping is enabled.
 #
 
-ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test mdb_mac_fwd_test mdb_ip4_fwd_test mdb_ip6_fwd_test"
+ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test mdb_rport_fwd_test \
+	   mdb_mac_fwd_test mdb_ip4_fwd_test mdb_ip6_fwd_test"
 NUM_NETIFS=6
 
 SRC_PORT="1234"
@@ -246,6 +249,55 @@ mdb_compat_fwd_test()
 	do_compat_fwd "br0"
 }
 
+#
+# Verify fwd of IP multicast to router ports.  A detected multicast
+# router should always receive both known and unknown multicast.
+#
+mdb_rport_fwd_test()
+{
+	pass_grp=$PASS_GRP_IP4
+	fail_grp=$FAIL_GRP_IP4
+	pass_pkt=$PASS_PKT_IP4
+	fail_pkt=$FAIL_PKT_IP4
+	decoy="br0"
+	port=$h1
+	type="IPv4"
+
+	# Disable flooding of unknown multicast, strict MDB forwarding
+	bridge link set dev "$swp1" mcast_flood off
+	bridge link set dev "$swp2" mcast_flood off
+	bridge link set dev "br0"   mcast_flood off self
+
+	# Let h2 act as a multicast router
+	ip link set dev "$swp1" type bridge_slave mcast_router 2
+
+	# Static filter only to this decoy port
+	bridge mdb add dev br0 port $decoy grp "$pass_grp"
+	check_err $? "Failed adding multicast group $pass_grp to bridge port $decoy"
+
+	tcpdump_start "$port"
+
+	# Real data we're expecting
+	$MZ -q "$h2" "$pass_pkt"
+	# This should not pass
+	$MZ -q "$h2" "$fail_pkt"
+
+	sleep 1
+	tcpdump_stop "$port"
+
+	tcpdump_show "$port" |grep -q "$src$spt > $pass_grp$dpt"
+	check_err $? "Failed forwarding $type multicast $pass_grp from $h2 to port $port"
+
+	tcpdump_show "$port" |grep -q "$src$spt > $fail_grp$dpt"
+	check_err $? "Failed forwarding $type multicast $fail_grp from $h2 to port $port"
+
+	bridge mdb del dev br0 port br0 grp "$pass_grp"
+	ip link set dev "$swp1" type bridge_slave mcast_router 1
+
+	log_test "MDB forward all $type multicast to multicast router on $port"
+	tcpdump_cleanup "$port"
+}
+
 do_mdb_fwd()
 {
 	type=$1
-- 
2.25.1

[Bridge] [PATCH RFC net-next 13/13] selftests: forwarding: verify flood of known mc on mcast_router port

[Joachim Wiberg]

This test verifies that both known (in mdb) and unknown IP multicast is
forwarded to a mcast_router port.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 .../selftests/net/forwarding/bridge_mdb.sh    | 54 ++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 137bc79fd677..3fd7d68bca09 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -10,13 +10,16 @@
 # Verify selective multicast forwarding (strict mdb), when bridge port
 # mcast_flood is disabled, of known MAC, IPv4, IPv6 traffic.
 #
+# Verify flooding towards mcast_router ports of known IP multicast.
+#
 # Note: this test completely disables IPv6 auto-configuration to avoid
 #       any type of dynamic behavior outside of MLD and IGMP protocols.
 #       Static IPv6 addresses are used to ensure consistent behavior,
 #       even in the startup phase when multicast snooping is enabled.
 #
 
-ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test mdb_mac_fwd_test mdb_ip4_fwd_test mdb_ip6_fwd_test"
+ALL_TESTS="mdb_add_del_test mdb_compat_fwd_test mdb_rport_fwd_test \
+	   mdb_mac_fwd_test mdb_ip4_fwd_test mdb_ip6_fwd_test"
 NUM_NETIFS=6
 
 SRC_PORT="1234"
@@ -246,6 +249,55 @@ mdb_compat_fwd_test()
 	do_compat_fwd "br0"
 }
 
+#
+# Verify fwd of IP multicast to router ports.  A detected multicast
+# router should always receive both known and unknown multicast.
+#
+mdb_rport_fwd_test()
+{
+	pass_grp=$PASS_GRP_IP4
+	fail_grp=$FAIL_GRP_IP4
+	pass_pkt=$PASS_PKT_IP4
+	fail_pkt=$FAIL_PKT_IP4
+	decoy="br0"
+	port=$h1
+	type="IPv4"
+
+	# Disable flooding of unknown multicast, strict MDB forwarding
+	bridge link set dev "$swp1" mcast_flood off
+	bridge link set dev "$swp2" mcast_flood off
+	bridge link set dev "br0"   mcast_flood off self
+
+	# Let h2 act as a multicast router
+	ip link set dev "$swp1" type bridge_slave mcast_router 2
+
+	# Static filter only to this decoy port
+	bridge mdb add dev br0 port $decoy grp "$pass_grp"
+	check_err $? "Failed adding multicast group $pass_grp to bridge port $decoy"
+
+	tcpdump_start "$port"
+
+	# Real data we're expecting
+	$MZ -q "$h2" "$pass_pkt"
+	# This should not pass
+	$MZ -q "$h2" "$fail_pkt"
+
+	sleep 1
+	tcpdump_stop "$port"
+
+	tcpdump_show "$port" |grep -q "$src$spt > $pass_grp$dpt"
+	check_err $? "Failed forwarding $type multicast $pass_grp from $h2 to port $port"
+
+	tcpdump_show "$port" |grep -q "$src$spt > $fail_grp$dpt"
+	check_err $? "Failed forwarding $type multicast $fail_grp from $h2 to port $port"
+
+	bridge mdb del dev br0 port br0 grp "$pass_grp"
+	ip link set dev "$swp1" type bridge_slave mcast_router 1
+
+	log_test "MDB forward all $type multicast to multicast router on $port"
+	tcpdump_cleanup "$port"
+}
+
 do_mdb_fwd()
 {
 	type=$1
-- 
2.25.1