On 2022-03-17 12:59, Vladimir Oltean wrote:

On Thu, Mar 17, 2022 at 01:57:03PM +0200, Vladimir Oltean wrote:

quoted

On Thu, Mar 17, 2022 at 01:15:45PM +0200, Vladimir Oltean wrote:

quoted

On Thu, Mar 17, 2022 at 12:11:40PM +0200, Nikolay Aleksandrov wrote:

quoted

On 17/03/2022 08:50, Mattias Forsblad wrote:

quoted

This patch implements the bridge flood flags. There are three different
flags matching unicast, multicast and broadcast. When the corresponding
flag is cleared packets received on bridge ports will not be flooded
towards the bridge.
This makes is possible to only forward selected traffic between the
port members of the bridge.

Signed-off-by: Mattias Forsblad <redacted>
---
 include/linux/if_bridge.h      |  6 +++++
 include/uapi/linux/if_bridge.h |  9 ++++++-
 net/bridge/br.c                | 45 ++++++++++++++++++++++++++++++++++
 net/bridge/br_device.c         |  3 +++
 net/bridge/br_input.c          | 23 ++++++++++++++---
 net/bridge/br_private.h        |  4 +++
 6 files changed, 85 insertions(+), 5 deletions(-)

Please always CC bridge maintainers for bridge patches.
I almost missed this one. I'll add my reply to Joachim's notes
which are pretty spot on.

And DSA maintainers for DSA patches ;) I was aimlessly scrolling through
patchwork when I happened to notice these, and the series is already at v3.

As a matter of fact, I downloaded these patches from the mailing list
with the intention of giving them a spin on mv88e6xxx to see what
they're about, and to my surprise, this particular patch (I haven't even
reached the offloading part) breaks DHCP on my bridge, so it can no
longer get an IP address. I haven't toggled any bridge flag through
netlink, just booted the board with systemd-networkd. The same thing
happens with my LS1028A board. Further investigation to come, but this
isn't off to a good start, I'm afraid...

quoted

quoted

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 3aae023a9353..fa8e000a6fb9 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h

@@ -157,6 +157,7 @@ static inline int br_vlan_get_info_rcu(const struct net_device *dev, u16 vid,
 struct net_device *br_fdb_find_port(const struct net_device *br_dev,
 				    const unsigned char *addr,
 				    __u16 vid);
+bool br_flood_enabled(const struct net_device *dev);
 void br_fdb_clear_offload(const struct net_device *dev, u16 vid);
 bool br_port_flag_is_set(const struct net_device *dev, unsigned long flag);
 u8 br_port_get_stp_state(const struct net_device *dev);

@@ -170,6 +171,11 @@ br_fdb_find_port(const struct net_device *br_dev,
 	return NULL;
 }
 
+static inline bool br_flood_enabled(const struct net_device *dev)
+{
+	return true;
+}
+
 static inline void br_fdb_clear_offload(const struct net_device *dev, u16 vid)
 {
 }

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 2711c3522010..765ed70c9b28 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h

@@ -72,6 +72,7 @@ struct __bridge_info {
 	__u32 tcn_timer_value;
 	__u32 topology_change_timer_value;
 	__u32 gc_timer_value;
+	__u8 flood;
 };
 
 struct __port_info {

@@ -752,13 +753,19 @@ struct br_mcast_stats {
 /* bridge boolean options
  * BR_BOOLOPT_NO_LL_LEARN - disable learning from link-local packets
  * BR_BOOLOPT_MCAST_VLAN_SNOOPING - control vlan multicast snooping
+ * BR_BOOLOPT_FLOOD - control bridge flood flag
+ * BR_BOOLOPT_MCAST_FLOOD - control bridge multicast flood flag
+ * BR_BOOLOPT_BCAST_FLOOD - control bridge broadcast flood flag
  *
  * IMPORTANT: if adding a new option do not forget to handle
- *            it in br_boolopt_toggle/get and bridge sysfs
+ *            it in br_boolopt_toggle/get
  */
 enum br_boolopt_id {
 	BR_BOOLOPT_NO_LL_LEARN,
 	BR_BOOLOPT_MCAST_VLAN_SNOOPING,
+	BR_BOOLOPT_FLOOD,
+	BR_BOOLOPT_MCAST_FLOOD,
+	BR_BOOLOPT_BCAST_FLOOD,
 	BR_BOOLOPT_MAX
 };
 

diff --git a/net/bridge/br.c b/net/bridge/br.c
index b1dea3febeea..63a17bed6c63 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c

@@ -265,6 +265,11 @@ int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id opt, bool on,
 	case BR_BOOLOPT_MCAST_VLAN_SNOOPING:
 		err = br_multicast_toggle_vlan_snooping(br, on, extack);
 		break;
+	case BR_BOOLOPT_FLOOD:
+	case BR_BOOLOPT_MCAST_FLOOD:
+	case BR_BOOLOPT_BCAST_FLOOD:
+		err = br_flood_toggle(br, opt, on);
+		break;
 	default:
 		/* shouldn't be called with unsupported options */
 		WARN_ON(1);

@@ -281,6 +286,12 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt)
 		return br_opt_get(br, BROPT_NO_LL_LEARN);
 	case BR_BOOLOPT_MCAST_VLAN_SNOOPING:
 		return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED);
+	case BR_BOOLOPT_FLOOD:
+		return br_opt_get(br, BROPT_FLOOD);
+	case BR_BOOLOPT_MCAST_FLOOD:
+		return br_opt_get(br, BROPT_MCAST_FLOOD);
+	case BR_BOOLOPT_BCAST_FLOOD:
+		return br_opt_get(br, BROPT_BCAST_FLOOD);
 	default:
 		/* shouldn't be called with unsupported options */
 		WARN_ON(1);

@@ -325,6 +336,40 @@ void br_boolopt_multi_get(const struct net_bridge *br,
 	bm->optmask = GENMASK((BR_BOOLOPT_MAX - 1), 0);
 }
 
+int br_flood_toggle(struct net_bridge *br, enum br_boolopt_id opt,
+		    bool on)
+{
+	struct switchdev_attr attr = {
+		.orig_dev = br->dev,
+		.id = SWITCHDEV_ATTR_ID_BRIDGE_FLOOD,
+		.flags = SWITCHDEV_F_DEFER,
+	};
+	enum net_bridge_opts bropt;
+	int ret;
+
+	switch (opt) {
+	case BR_BOOLOPT_FLOOD:
+		bropt = BROPT_FLOOD;
+		break;
+	case BR_BOOLOPT_MCAST_FLOOD:
+		bropt = BROPT_MCAST_FLOOD;
+		break;
+	case BR_BOOLOPT_BCAST_FLOOD:
+		bropt = BROPT_BCAST_FLOOD;
+		break;
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
+	br_opt_toggle(br, bropt, on);
+
+	attr.u.brport_flags.mask = BIT(bropt);
+	attr.u.brport_flags.val = on << bropt;
+	ret = switchdev_port_attr_set(br->dev, &attr, NULL);
+
+	return ret;
+}
+
 /* private bridge options, controlled by the kernel */
 void br_opt_toggle(struct net_bridge *br, enum net_bridge_opts opt, bool on)
 {

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 8d6bab244c4a..fafaef9d4b3a 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c

@@ -524,6 +524,9 @@ void br_dev_setup(struct net_device *dev)
 	br->bridge_hello_time = br->hello_time = 2 * HZ;
 	br->bridge_forward_delay = br->forward_delay = 15 * HZ;
 	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
+	br_opt_toggle(br, BROPT_FLOOD, true);
+	br_opt_toggle(br, BROPT_MCAST_FLOOD, true);
+	br_opt_toggle(br, BROPT_BCAST_FLOOD, true);
 	dev->max_mtu = ETH_MAX_MTU;
 
 	br_netfilter_rtable_init(br);

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index e0c13fcc50ed..fcb0757bfdcc 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -109,11 +109,12 @@ 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;
+			local_rcv = true && br_opt_get(br, BROPT_BCAST_FLOOD);
 		} else {
 			pkt_type = BR_PKT_MULTICAST;
-			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
-				goto drop;
+			if (br_opt_get(br, BROPT_MCAST_FLOOD))
+				if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
+					goto drop;
 		}
 	}
 

@@ -155,9 +156,13 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 			local_rcv = true;
 			br->dev->stats.multicast++;
 		}
+		if (!br_opt_get(br, BROPT_MCAST_FLOOD))
+			local_rcv = false;
 		break;
 	case BR_PKT_UNICAST:
 		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
+		if (!br_opt_get(br, BROPT_FLOOD))
+			local_rcv = false;
 		break;
 	default:
 		break;

@@ -166,7 +171,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	if (dst) {
 		unsigned long now = jiffies;
 
-		if (test_bit(BR_FDB_LOCAL, &dst->flags))
+		if (test_bit(BR_FDB_LOCAL, &dst->flags) && local_rcv)

So this is the line that breaks local termination. Could you explain
the reasoning for this change? For unicast packets matching a local FDB
entry, local_rcv used to be irrelevant (and wasn't even set to true,
unless the bridge device was promiscuous).

Sorry, it wasn't obvious from the To: field, but the question was
targeted to Mattias and not to Nikolay.

It might as simple be that I've misunderstood that flow.

/Mattias