[PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Mattias Forsblad]

Greetings,

This series implements a new bridge flag 'local_receive' and HW
offloading for Marvell mv88e6xxx.

When using a non-VLAN filtering bridge we want to be able to limit
traffic to the CPU port to lessen the CPU load. This is specially
important when we have disabled learning on user ports.

A sample configuration could be something like this:

       br0
      /   \
   swp0   swp1

ip link add dev br0 type bridge stp_state 0 vlan_filtering 0
ip link set swp0 master br0
ip link set swp1 master br0
ip link set swp0 type bridge_slave learning off
ip link set swp1 type bridge_slave learning off
ip link set swp0 up
ip link set swp1 up
ip link set br0 type bridge local_receive 0
ip link set br0 up

The first part of the series implements the flag for the SW bridge
and the second part the DSA infrastructure. The last part implements
offloading of this flag to HW for mv88e6xxx, which uses the
port vlan table to restrict the ingress from user ports
to the CPU port when this flag is cleared.

Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

Regards,
Mattias Forsblad (3):
  net: bridge: Implement bridge flag local_receive
  dsa: Handle the local_receive flag in the DSA layer.
  mv88e6xxx: Offload the local_receive flag

 drivers/net/dsa/mv88e6xxx/chip.c | 45 ++++++++++++++++++++++++++++++--
 include/linux/if_bridge.h        |  6 +++++
 include/net/dsa.h                |  6 +++++
 include/net/switchdev.h          |  2 ++
 include/uapi/linux/if_bridge.h   |  1 +
 include/uapi/linux/if_link.h     |  1 +
 net/bridge/br.c                  | 18 +++++++++++++
 net/bridge/br_device.c           |  1 +
 net/bridge/br_input.c            |  3 +++
 net/bridge/br_ioctl.c            |  1 +
 net/bridge/br_netlink.c          | 14 +++++++++-
 net/bridge/br_private.h          |  2 ++
 net/bridge/br_sysfs_br.c         | 23 ++++++++++++++++
 net/bridge/br_vlan.c             |  8 ++++++
 net/dsa/dsa_priv.h               |  1 +
 net/dsa/slave.c                  | 16 ++++++++++++
 16 files changed, 145 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Mattias Forsblad]

This patch implements the bridge flag local_receive. When this
flag is cleared packets received on bridge ports will not be forwarded up.
This makes is possible to only forward traffic between the port members
of the bridge.

Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
---
 include/linux/if_bridge.h      |  6 ++++++
 include/net/switchdev.h        |  2 ++
 include/uapi/linux/if_bridge.h |  1 +
 include/uapi/linux/if_link.h   |  1 +
 net/bridge/br.c                | 18 ++++++++++++++++++
 net/bridge/br_device.c         |  1 +
 net/bridge/br_input.c          |  3 +++
 net/bridge/br_ioctl.c          |  1 +
 net/bridge/br_netlink.c        | 14 +++++++++++++-
 net/bridge/br_private.h        |  2 ++
 net/bridge/br_sysfs_br.c       | 23 +++++++++++++++++++++++
 net/bridge/br_vlan.c           |  8 ++++++++
 12 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 3aae023a9353..e6b77d18c1d2 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_local_receive_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_local_receive_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/net/switchdev.h b/include/net/switchdev.h
index 3e424d40fae3..aec5c1f9b5c7 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -25,6 +25,7 @@ enum switchdev_attr_id {
 	SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL,
+	SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RECEIVE,
 	SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
 	SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
 	SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
@@ -50,6 +51,7 @@ struct switchdev_attr {
 		u16 vlan_protocol;			/* BRIDGE_VLAN_PROTOCOL */
 		bool mc_disabled;			/* MC_DISABLED */
 		u8 mrp_port_role;			/* MRP_PORT_ROLE */
+		bool local_receive;			/* BRIDGE_LOCAL_RECEIVE */
 	} u;
 };
 
diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 2711c3522010..fc889b5ccd69 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 local_receive;
 };
 
 struct __port_info {
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index e315e53125f4..bb7c25e1c89c 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -482,6 +482,7 @@ enum {
 	IFLA_BR_VLAN_STATS_PER_PORT,
 	IFLA_BR_MULTI_BOOLOPT,
 	IFLA_BR_MCAST_QUERIER_STATE,
+	IFLA_BR_LOCAL_RECEIVE,
 	__IFLA_BR_MAX,
 };
 
diff --git a/net/bridge/br.c b/net/bridge/br.c
index b1dea3febeea..ff7eb4f269ec 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -325,6 +325,24 @@ void br_boolopt_multi_get(const struct net_bridge *br,
 	bm->optmask = GENMASK((BR_BOOLOPT_MAX - 1), 0);
 }
 
+int br_local_receive_change(struct net_bridge *p,
+			    bool local_receive)
+{
+	struct switchdev_attr attr = {
+		.orig_dev = p->dev,
+		.id = SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RECEIVE,
+		.flags = SWITCHDEV_F_DEFER,
+		.u.local_receive = local_receive,
+	};
+	int ret;
+
+	ret = switchdev_port_attr_set(p->dev, &attr, NULL);
+	if (!ret)
+		br_opt_toggle(p, BROPT_LOCAL_RECEIVE, local_receive);
+
+	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..7cd9c5880d18 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -524,6 +524,7 @@ 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_LOCAL_RECEIVE, 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..5864b61157d3 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		break;
 	}
 
+	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
+		local_rcv = false;
+
 	if (dst) {
 		unsigned long now = jiffies;
 
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index f213ed108361..abe538129290 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -177,6 +177,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq,
 		b.topology_change = br->topology_change;
 		b.topology_change_detected = br->topology_change_detected;
 		b.root_port = br->root_port;
+		b.local_receive = br_opt_get(br, BROPT_LOCAL_RECEIVE) ? 1 : 0;
 
 		b.stp_enabled = (br->stp_enabled != BR_NO_STP);
 		b.ageing_time = jiffies_to_clock_t(br->ageing_time);
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 7d4432ca9a20..5e7f99950195 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1163,6 +1163,7 @@ static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
 	[IFLA_BR_MCAST_IGMP_VERSION] = { .type = NLA_U8 },
 	[IFLA_BR_MCAST_MLD_VERSION] = { .type = NLA_U8 },
 	[IFLA_BR_VLAN_STATS_PER_PORT] = { .type = NLA_U8 },
+	[IFLA_BR_LOCAL_RECEIVE] = { .type = NLA_U8 },
 	[IFLA_BR_MULTI_BOOLOPT] =
 		NLA_POLICY_EXACT_LEN(sizeof(struct br_boolopt_multi)),
 };
@@ -1434,6 +1435,14 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 			return err;
 	}
 
+	if (data[IFLA_BR_LOCAL_RECEIVE]) {
+		u8 val = nla_get_u8(data[IFLA_BR_LOCAL_RECEIVE]);
+
+		err = br_local_receive_change(br, !!val);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
@@ -1514,6 +1523,7 @@ static size_t br_get_size(const struct net_device *brdev)
 	       nla_total_size(sizeof(u8)) +     /* IFLA_BR_NF_CALL_ARPTABLES */
 #endif
 	       nla_total_size(sizeof(struct br_boolopt_multi)) + /* IFLA_BR_MULTI_BOOLOPT */
+	       nla_total_size(sizeof(u8)) +     /* IFLA_BR_LOCAL_RECEIVE */
 	       0;
 }
 
@@ -1527,6 +1537,7 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
 	u32 stp_enabled = br->stp_enabled;
 	u16 priority = (br->bridge_id.prio[0] << 8) | br->bridge_id.prio[1];
 	u8 vlan_enabled = br_vlan_enabled(br->dev);
+	u8 local_receive = br_opt_get(br, BROPT_LOCAL_RECEIVE) ? 1 : 0;
 	struct br_boolopt_multi bm;
 	u64 clockval;
 
@@ -1563,7 +1574,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
 	    nla_put_u8(skb, IFLA_BR_TOPOLOGY_CHANGE_DETECTED,
 		       br->topology_change_detected) ||
 	    nla_put(skb, IFLA_BR_GROUP_ADDR, ETH_ALEN, br->group_addr) ||
-	    nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm))
+	    nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm) ||
+	    nla_put_u8(skb, IFLA_BR_LOCAL_RECEIVE, local_receive))
 		return -EMSGSIZE;
 
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 48bc61ebc211..01fa5426094b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -445,6 +445,7 @@ enum net_bridge_opts {
 	BROPT_NO_LL_LEARN,
 	BROPT_VLAN_BRIDGE_BINDING,
 	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
+	BROPT_LOCAL_RECEIVE,
 };
 
 struct net_bridge {
@@ -720,6 +721,7 @@ int br_boolopt_multi_toggle(struct net_bridge *br,
 void br_boolopt_multi_get(const struct net_bridge *br,
 			  struct br_boolopt_multi *bm);
 void br_opt_toggle(struct net_bridge *br, enum net_bridge_opts opt, bool on);
+int br_local_receive_change(struct net_bridge *p, bool local_receive);
 
 /* br_device.c */
 void br_dev_setup(struct net_device *dev);
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 3f7ca88c2aa3..9af0c2ba929c 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -84,6 +84,28 @@ static ssize_t forward_delay_store(struct device *d,
 }
 static DEVICE_ATTR_RW(forward_delay);
 
+static ssize_t local_receive_show(struct device *d,
+				  struct device_attribute *attr, char *buf)
+{
+	struct net_bridge *br = to_bridge(d);
+
+	return sprintf(buf, "%u\n", br_opt_get(br, BROPT_LOCAL_RECEIVE));
+}
+
+static int set_local_receive(struct net_bridge *br, unsigned long val,
+			     struct netlink_ext_ack *extack)
+{
+	return br_local_receive_change(br, !!val);
+}
+
+static ssize_t local_receive_store(struct device *d,
+				   struct device_attribute *attr,
+				   const char *buf, size_t len)
+{
+	return store_bridge_parm(d, buf, len, set_local_receive);
+}
+static DEVICE_ATTR_RW(local_receive);
+
 static ssize_t hello_time_show(struct device *d, struct device_attribute *attr,
 			       char *buf)
 {
@@ -950,6 +972,7 @@ static struct attribute *bridge_attrs[] = {
 	&dev_attr_group_addr.attr,
 	&dev_attr_flush.attr,
 	&dev_attr_no_linklocal_learn.attr,
+	&dev_attr_local_receive.attr,
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	&dev_attr_multicast_router.attr,
 	&dev_attr_multicast_snooping.attr,
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 7557e90b60e1..57dd14d5e360 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -905,6 +905,14 @@ bool br_vlan_enabled(const struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(br_vlan_enabled);
 
+bool br_local_receive_enabled(const struct net_device *dev)
+{
+	struct net_bridge *br = netdev_priv(dev);
+
+	return br_opt_get(br, BROPT_LOCAL_RECEIVE);
+}
+EXPORT_SYMBOL_GPL(br_local_receive_enabled);
+
 int br_vlan_get_proto(const struct net_device *dev, u16 *p_proto)
 {
 	struct net_bridge *br = netdev_priv(dev);
-- 
2.25.1

[PATCH 2/3] dsa: Handle the local_receive flag in the DSA layer.

[Mattias Forsblad]

Add infrastructure to be able to handle the local_receive
flag to the DSA layer.

Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
---
 include/net/dsa.h  |  6 ++++++
 net/dsa/dsa_priv.h |  1 +
 net/dsa/slave.c    | 16 ++++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index cfedcfb86350..3abd7cfad7a0 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -925,6 +925,12 @@ struct dsa_switch_ops {
 	void	(*get_regs)(struct dsa_switch *ds, int port,
 			    struct ethtool_regs *regs, void *p);
 
+	/*
+	 * Local receive
+	 */
+	int	(*set_local_receive)(struct dsa_switch *ds, int port,
+				     struct net_device *bridge, bool enable);
+
 	/*
 	 * Upper device tracking.
 	 */
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 07c0ad52395a..33e607615e63 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -217,6 +217,7 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			    struct netlink_ext_ack *extack);
 bool dsa_port_skip_vlan_configuration(struct dsa_port *dp);
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock);
+int dsa_port_set_local_receive(struct dsa_port *dp, struct net_device *br, bool enable);
 int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu,
 			bool targeted_match);
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 089616206b11..50f88b0bd851 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -295,6 +295,12 @@ static int dsa_slave_port_attr_set(struct net_device *dev, const void *ctx,
 		ret = dsa_port_vlan_filtering(dp, attr->u.vlan_filtering,
 					      extack);
 		break;
+	case SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RECEIVE:
+		if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev))
+			return -EOPNOTSUPP;
+
+		ret = dsa_port_set_local_receive(dp, attr->orig_dev, attr->u.local_receive);
+		break;
 	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
 		if (!dsa_port_offloads_bridge_dev(dp, attr->orig_dev))
 			return -EOPNOTSUPP;
@@ -671,6 +677,16 @@ dsa_slave_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *_p)
 		ds->ops->get_regs(ds, dp->index, regs, _p);
 }
 
+int dsa_port_set_local_receive(struct dsa_port *dp, struct net_device *br, bool enable)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	if (ds->ops->set_local_receive)
+		return ds->ops->set_local_receive(ds, dp->index, br, enable);
+
+	return 0;
+}
+
 static int dsa_slave_nway_reset(struct net_device *dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-- 
2.25.1

[PATCH 3/3] mv88e6xxx: Offload the local_receive flag

[Mattias Forsblad]

Use the port vlan table to restrict ingressing traffic to the
CPU port if the local_receive flag is cleared.

Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 45 ++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 84b90fc36c58..d5616c7ca46e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1384,6 +1384,7 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 	struct dsa_switch *ds = chip->ds;
 	struct dsa_switch_tree *dst = ds->dst;
 	struct dsa_port *dp, *other_dp;
+	int local_receive = 1;
 	bool found = false;
 	u16 pvlan;
 
@@ -1425,6 +1426,9 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 
 	pvlan = 0;
 
+	if (dp->bridge)
+		local_receive = br_local_receive_enabled(dp->bridge->dev);
+
 	/* Frames from standalone user ports can only egress on the
 	 * upstream port.
 	 */
@@ -1433,10 +1437,11 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 
 	/* Frames from bridged user ports can egress any local DSA
 	 * links and CPU ports, as well as any local member of their
-	 * bridge group.
+	 * as well as any local member of their bridge group. However, CPU ports
+	 * are omitted if local_receive is reset.
 	 */
 	dsa_switch_for_each_port(other_dp, ds)
-		if (other_dp->type == DSA_PORT_TYPE_CPU ||
+		if ((other_dp->type == DSA_PORT_TYPE_CPU && local_receive) ||
 		    other_dp->type == DSA_PORT_TYPE_DSA ||
 		    dsa_port_bridge_same(dp, other_dp))
 			pvlan |= BIT(other_dp->index);
@@ -2718,6 +2723,41 @@ static void mv88e6xxx_crosschip_bridge_leave(struct dsa_switch *ds,
 	mv88e6xxx_reg_unlock(chip);
 }
 
+static int mv88e6xxx_set_local_receive(struct dsa_switch *ds, int port, struct net_device *br,
+				       bool enable)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct dsa_bridge *bridge;
+	struct dsa_port *dp;
+	bool found = false;
+	int err;
+
+	if (!netif_is_bridge_master(br))
+		return 0;
+
+	list_for_each_entry(dp, &ds->dst->ports, list) {
+		if (dp->ds == ds && dp->index == port) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found)
+		return 0;
+
+	bridge = dp->bridge;
+	if (!bridge)
+		return 0;
+
+	mv88e6xxx_reg_lock(chip);
+
+	err = mv88e6xxx_bridge_map(chip, *bridge);
+
+	mv88e6xxx_reg_unlock(chip);
+
+	return err;
+}
+
 static int mv88e6xxx_software_reset(struct mv88e6xxx_chip *chip)
 {
 	if (chip->info->ops->reset)
@@ -6478,6 +6518,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.set_eeprom		= mv88e6xxx_set_eeprom,
 	.get_regs_len		= mv88e6xxx_get_regs_len,
 	.get_regs		= mv88e6xxx_get_regs,
+	.set_local_receive      = mv88e6xxx_set_local_receive,
 	.get_rxnfc		= mv88e6xxx_get_rxnfc,
 	.set_rxnfc		= mv88e6xxx_set_rxnfc,
 	.set_ageing_time	= mv88e6xxx_set_ageing_time,
-- 
2.25.1

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Ido Schimmel]

On Tue, Mar 01, 2022 at 01:31:02PM +0100, Mattias Forsblad wrote:
> This patch implements the bridge flag local_receive. When this
> flag is cleared packets received on bridge ports will not be forwarded up.
> This makes is possible to only forward traffic between the port members
> of the bridge.
> 
> Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
> ---
>  include/linux/if_bridge.h      |  6 ++++++
>  include/net/switchdev.h        |  2 ++

Nik might ask you to split the offload part from the bridge
implementation. Please wait for his feedback as he might be AFK right
now

>  include/uapi/linux/if_bridge.h |  1 +
>  include/uapi/linux/if_link.h   |  1 +
>  net/bridge/br.c                | 18 ++++++++++++++++++
>  net/bridge/br_device.c         |  1 +
>  net/bridge/br_input.c          |  3 +++
>  net/bridge/br_ioctl.c          |  1 +
>  net/bridge/br_netlink.c        | 14 +++++++++++++-
>  net/bridge/br_private.h        |  2 ++
>  net/bridge/br_sysfs_br.c       | 23 +++++++++++++++++++++++

I believe the bridge doesn't implement sysfs for new attributes

>  net/bridge/br_vlan.c           |  8 ++++++++
>  12 files changed, 79 insertions(+), 1 deletion(-)

[...]

> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index e0c13fcc50ed..5864b61157d3 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  		break;
>  	}
>  
> +	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
> +		local_rcv = false;
> +

I don't think the description in the commit message is accurate:
"packets received on bridge ports will not be forwarded up". From the
code it seems that if packets hit a local FDB entry, then they will be
"forwarded up". Instead, it seems that packets will not be flooded
towards the bridge. In which case, why not maintain the same granularity
we have for the rest of the ports and split this into unicast /
multicast / broadcast?

BTW, while the patch honors local FDB entries, it overrides host MDB
entries which seems wrong / inconsistent.

>  	if (dst) {
>  		unsigned long now = jiffies;

Re: [PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Florian Fainelli]

On 3/1/2022 4:31 AM, Mattias Forsblad wrote:
> Greetings,
> 
> This series implements a new bridge flag 'local_receive' and HW
> offloading for Marvell mv88e6xxx.
> 
> When using a non-VLAN filtering bridge we want to be able to limit
> traffic to the CPU port to lessen the CPU load. This is specially
> important when we have disabled learning on user ports.
> 
> A sample configuration could be something like this:
> 
>         br0
>        /   \
>     swp0   swp1
> 
> ip link add dev br0 type bridge stp_state 0 vlan_filtering 0
> ip link set swp0 master br0
> ip link set swp1 master br0
> ip link set swp0 type bridge_slave learning off
> ip link set swp1 type bridge_slave learning off
> ip link set swp0 up
> ip link set swp1 up
> ip link set br0 type bridge local_receive 0
> ip link set br0 up
> 
> The first part of the series implements the flag for the SW bridge
> and the second part the DSA infrastructure. The last part implements
> offloading of this flag to HW for mv88e6xxx, which uses the
> port vlan table to restrict the ingress from user ports
> to the CPU port when this flag is cleared.

Why not use a bridge with VLAN filtering enabled? I cannot quite find it 
right now, but Vladimir recently picked up what I had attempted before 
which was to allow removing the CPU port (via the bridge master device) 
from a specific group of VLANs to achieve that isolation.

> 
> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>

I don't believe this tag has much value since it was presumably carried 
over from an internal review. Might be worth adding it publicly now, though.

> 
> Regards,
> Mattias Forsblad (3):
>    net: bridge: Implement bridge flag local_receive
>    dsa: Handle the local_receive flag in the DSA layer.
>    mv88e6xxx: Offload the local_receive flag
> 
>   drivers/net/dsa/mv88e6xxx/chip.c | 45 ++++++++++++++++++++++++++++++--
>   include/linux/if_bridge.h        |  6 +++++
>   include/net/dsa.h                |  6 +++++
>   include/net/switchdev.h          |  2 ++
>   include/uapi/linux/if_bridge.h   |  1 +
>   include/uapi/linux/if_link.h     |  1 +
>   net/bridge/br.c                  | 18 +++++++++++++
>   net/bridge/br_device.c           |  1 +
>   net/bridge/br_input.c            |  3 +++
>   net/bridge/br_ioctl.c            |  1 +
>   net/bridge/br_netlink.c          | 14 +++++++++-
>   net/bridge/br_private.h          |  2 ++
>   net/bridge/br_sysfs_br.c         | 23 ++++++++++++++++
>   net/bridge/br_vlan.c             |  8 ++++++
>   net/dsa/dsa_priv.h               |  1 +
>   net/dsa/slave.c                  | 16 ++++++++++++
>   16 files changed, 145 insertions(+), 3 deletions(-)
> 

-- 
Florian

Re: [PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Tobias Waldekranz]

On Tue, Mar 01, 2022 at 09:14, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 3/1/2022 4:31 AM, Mattias Forsblad wrote:
>> Greetings,
>> 
>> This series implements a new bridge flag 'local_receive' and HW
>> offloading for Marvell mv88e6xxx.
>> 
>> When using a non-VLAN filtering bridge we want to be able to limit
>> traffic to the CPU port to lessen the CPU load. This is specially
>> important when we have disabled learning on user ports.
>> 
>> A sample configuration could be something like this:
>> 
>>         br0
>>        /   \
>>     swp0   swp1
>> 
>> ip link add dev br0 type bridge stp_state 0 vlan_filtering 0
>> ip link set swp0 master br0
>> ip link set swp1 master br0
>> ip link set swp0 type bridge_slave learning off
>> ip link set swp1 type bridge_slave learning off
>> ip link set swp0 up
>> ip link set swp1 up
>> ip link set br0 type bridge local_receive 0
>> ip link set br0 up
>> 
>> The first part of the series implements the flag for the SW bridge
>> and the second part the DSA infrastructure. The last part implements
>> offloading of this flag to HW for mv88e6xxx, which uses the
>> port vlan table to restrict the ingress from user ports
>> to the CPU port when this flag is cleared.
>
> Why not use a bridge with VLAN filtering enabled? I cannot quite find it 
> right now, but Vladimir recently picked up what I had attempted before 
> which was to allow removing the CPU port (via the bridge master device) 
> from a specific group of VLANs to achieve that isolation.
>

Hi Florian,

Yes we are aware of this work, which is awesome by the way! For anyone
else who is interested, I believe you are referring to this series:

https://lore.kernel.org/netdev/20220215170218.2032432-1-vladimir.oltean@nxp.com/

There are cases though, where you want a TPMR-like setup (or "dumb hub"
mode, if you will) and ignore all tag information.

One application could be to use a pair of ports on a switch as an
ethernet extender/repeater for topologies that span large physical
distances. If this repeater is part of a redundant topology, you'd to
well to disable learning, in order to avoid dropping packets when the
surrounding active topology changes. This, in turn, will mean that all
flows will be classified as unknown unicast. For that reason it is very
important that the CPU be shielded.

You might be tempted to solve this using flooding filters of the
switch's CPU port, but these go out the window if you have another
bridge configured, that requires that flooding of unknown traffic is
enabled.

Another application is to create a similar setup, but with three ports,
and have the third one be used as a TAP.

>> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
>
> I don't believe this tag has much value since it was presumably carried 
> over from an internal review. Might be worth adding it publicly now, though.

I think Mattias meant to replicate this tag on each individual
patch. Aside from that though, are you saying that a tag is never valid
unless there is a public message on the list from the signee? Makes
sense I suppose. Anyway, I will send separate tags for this series.

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Nikolay Aleksandrov]

On 1 March 2022 17:43:27 CET, Ido Schimmel <idosch@idosch.org> wrote:
>On Tue, Mar 01, 2022 at 01:31:02PM +0100, Mattias Forsblad wrote:
>> This patch implements the bridge flag local_receive. When this
>> flag is cleared packets received on bridge ports will not be forwarded up.
>> This makes is possible to only forward traffic between the port members
>> of the bridge.
>> 
>> Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
>> ---
>>  include/linux/if_bridge.h      |  6 ++++++
>>  include/net/switchdev.h        |  2 ++
>
>Nik might ask you to split the offload part from the bridge
>implementation. Please wait for his feedback as he might be AFK right
>now
>

Indeed, I'm traveling and won't have pc access until end of week (Sun). 
I'll try to review the patches through my phoneas much as I can.
Ack on the split.

>>  include/uapi/linux/if_bridge.h |  1 +
>>  include/uapi/linux/if_link.h   |  1 +
>>  net/bridge/br.c                | 18 ++++++++++++++++++
>>  net/bridge/br_device.c         |  1 +
>>  net/bridge/br_input.c          |  3 +++
>>  net/bridge/br_ioctl.c          |  1 +
>>  net/bridge/br_netlink.c        | 14 +++++++++++++-
>>  net/bridge/br_private.h        |  2 ++
>>  net/bridge/br_sysfs_br.c       | 23 +++++++++++++++++++++++
>
>I believe the bridge doesn't implement sysfs for new attributes
>

Right, no new sysfs please.

>>  net/bridge/br_vlan.c           |  8 ++++++++
>>  12 files changed, 79 insertions(+), 1 deletion(-)
>
>[...]
>
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index e0c13fcc50ed..5864b61157d3 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  		break;
>>  	}
>>  
>> +	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
>> +		local_rcv = false;
>> +
>
>I don't think the description in the commit message is accurate:
>"packets received on bridge ports will not be forwarded up". From the
>code it seems that if packets hit a local FDB entry, then they will be
>"forwarded up". Instead, it seems that packets will not be flooded
>towards the bridge. In which case, why not maintain the same granularity
>we have for the rest of the ports and split this into unicast /
>multicast / broadcast?
>

Exactly my first thought - why not implement the same control for the bridge?
Also try to minimize the fast-path hit, you can keep the needed changes 
localized only to the cases where they are needed.
I'll send a few more comments in a reply to the patch.

>BTW, while the patch honors local FDB entries, it overrides host MDB
>entries which seems wrong / inconsistent.
>
>>  	if (dst) {
>>  		unsigned long now = jiffies;

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Nikolay Aleksandrov]

On 1 March 2022 13:31:02 CET, Mattias Forsblad <mattias.forsblad@gmail.com> wrote:
>This patch implements the bridge flag local_receive. When this
>flag is cleared packets received on bridge ports will not be forwarded up.
>This makes is possible to only forward traffic between the port members
>of the bridge.
>
>Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
>---
> include/linux/if_bridge.h      |  6 ++++++
> include/net/switchdev.h        |  2 ++
> include/uapi/linux/if_bridge.h |  1 +
> include/uapi/linux/if_link.h   |  1 +
> net/bridge/br.c                | 18 ++++++++++++++++++
> net/bridge/br_device.c         |  1 +
> net/bridge/br_input.c          |  3 +++
> net/bridge/br_ioctl.c          |  1 +
> net/bridge/br_netlink.c        | 14 +++++++++++++-
> net/bridge/br_private.h        |  2 ++
> net/bridge/br_sysfs_br.c       | 23 +++++++++++++++++++++++
> net/bridge/br_vlan.c           |  8 ++++++++
> 12 files changed, 79 insertions(+), 1 deletion(-)
>
>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>index 3aae023a9353..e6b77d18c1d2 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_local_receive_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_local_receive_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/net/switchdev.h b/include/net/switchdev.h
>index 3e424d40fae3..aec5c1f9b5c7 100644
>--- a/include/net/switchdev.h
>+++ b/include/net/switchdev.h
>@@ -25,6 +25,7 @@ enum switchdev_attr_id {
> 	SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
> 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL,
>+	SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RECEIVE,
> 	SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
> 	SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
> 	SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
>@@ -50,6 +51,7 @@ struct switchdev_attr {
> 		u16 vlan_protocol;			/* BRIDGE_VLAN_PROTOCOL */
> 		bool mc_disabled;			/* MC_DISABLED */
> 		u8 mrp_port_role;			/* MRP_PORT_ROLE */
>+		bool local_receive;			/* BRIDGE_LOCAL_RECEIVE */
> 	} u;
> };
> 
>diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
>index 2711c3522010..fc889b5ccd69 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 local_receive;
> };
> 
> struct __port_info {
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index e315e53125f4..bb7c25e1c89c 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -482,6 +482,7 @@ enum {
> 	IFLA_BR_VLAN_STATS_PER_PORT,
> 	IFLA_BR_MULTI_BOOLOPT,
> 	IFLA_BR_MCAST_QUERIER_STATE,
>+	IFLA_BR_LOCAL_RECEIVE,

Please use the boolopt api for new boolean options
We're trying to limit the nl options expansion as the bridge is the
largest user.

> 	__IFLA_BR_MAX,
> };
> 
>diff --git a/net/bridge/br.c b/net/bridge/br.c
>index b1dea3febeea..ff7eb4f269ec 100644
>--- a/net/bridge/br.c
>+++ b/net/bridge/br.c
>@@ -325,6 +325,24 @@ void br_boolopt_multi_get(const struct net_bridge *br,
> 	bm->optmask = GENMASK((BR_BOOLOPT_MAX - 1), 0);
> }
> 
>+int br_local_receive_change(struct net_bridge *p,
>+			    bool local_receive)
>+{
>+	struct switchdev_attr attr = {
>+		.orig_dev = p->dev,
>+		.id = SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RECEIVE,
>+		.flags = SWITCHDEV_F_DEFER,
>+		.u.local_receive = local_receive,
>+	};
>+	int ret;
>+
>+	ret = switchdev_port_attr_set(p->dev, &attr, NULL);
>+	if (!ret)
>+		br_opt_toggle(p, BROPT_LOCAL_RECEIVE, local_receive);
>+
>+	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..7cd9c5880d18 100644
>--- a/net/bridge/br_device.c
>+++ b/net/bridge/br_device.c
>@@ -524,6 +524,7 @@ 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_LOCAL_RECEIVE, 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..5864b61157d3 100644
>--- a/net/bridge/br_input.c
>+++ b/net/bridge/br_input.c
>@@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> 		break;
> 	}
> 
>+	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
>+		local_rcv = false;
>+

this affects the whole fast path, it can be better localized to make sure
it will not affect all use cases

> 	if (dst) {
> 		unsigned long now = jiffies;
> 
>diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
>index f213ed108361..abe538129290 100644
>--- a/net/bridge/br_ioctl.c
>+++ b/net/bridge/br_ioctl.c
>@@ -177,6 +177,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq,
> 		b.topology_change = br->topology_change;
> 		b.topology_change_detected = br->topology_change_detected;
> 		b.root_port = br->root_port;
>+		b.local_receive = br_opt_get(br, BROPT_LOCAL_RECEIVE) ? 1 : 0;

ioctl is not being extended anymore, please drop it

> 
> 		b.stp_enabled = (br->stp_enabled != BR_NO_STP);
> 		b.ageing_time = jiffies_to_clock_t(br->ageing_time);
>diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>index 7d4432ca9a20..5e7f99950195 100644
>--- a/net/bridge/br_netlink.c
>+++ b/net/bridge/br_netlink.c
>@@ -1163,6 +1163,7 @@ static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
> 	[IFLA_BR_MCAST_IGMP_VERSION] = { .type = NLA_U8 },
> 	[IFLA_BR_MCAST_MLD_VERSION] = { .type = NLA_U8 },
> 	[IFLA_BR_VLAN_STATS_PER_PORT] = { .type = NLA_U8 },
>+	[IFLA_BR_LOCAL_RECEIVE] = { .type = NLA_U8 },
> 	[IFLA_BR_MULTI_BOOLOPT] =
> 		NLA_POLICY_EXACT_LEN(sizeof(struct br_boolopt_multi)),
> };
>@@ -1434,6 +1435,14 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
> 			return err;
> 	}
> 
>+	if (data[IFLA_BR_LOCAL_RECEIVE]) {
>+		u8 val = nla_get_u8(data[IFLA_BR_LOCAL_RECEIVE]);
>+
>+		err = br_local_receive_change(br, !!val);
>+		if (err)
>+			return err;
>+	}
>+
> 	return 0;
> }
> 
>@@ -1514,6 +1523,7 @@ static size_t br_get_size(const struct net_device *brdev)
> 	       nla_total_size(sizeof(u8)) +     /* IFLA_BR_NF_CALL_ARPTABLES */
> #endif
> 	       nla_total_size(sizeof(struct br_boolopt_multi)) + /* IFLA_BR_MULTI_BOOLOPT */
>+	       nla_total_size(sizeof(u8)) +     /* IFLA_BR_LOCAL_RECEIVE */
> 	       0;
> }
> 
>@@ -1527,6 +1537,7 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
> 	u32 stp_enabled = br->stp_enabled;
> 	u16 priority = (br->bridge_id.prio[0] << 8) | br->bridge_id.prio[1];
> 	u8 vlan_enabled = br_vlan_enabled(br->dev);
>+	u8 local_receive = br_opt_get(br, BROPT_LOCAL_RECEIVE) ? 1 : 0;
> 	struct br_boolopt_multi bm;
> 	u64 clockval;
> 
>@@ -1563,7 +1574,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
> 	    nla_put_u8(skb, IFLA_BR_TOPOLOGY_CHANGE_DETECTED,
> 		       br->topology_change_detected) ||
> 	    nla_put(skb, IFLA_BR_GROUP_ADDR, ETH_ALEN, br->group_addr) ||
>-	    nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm))
>+	    nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm) ||
>+	    nla_put_u8(skb, IFLA_BR_LOCAL_RECEIVE, local_receive))
> 		return -EMSGSIZE;
> 
> #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>index 48bc61ebc211..01fa5426094b 100644
>--- a/net/bridge/br_private.h
>+++ b/net/bridge/br_private.h
>@@ -445,6 +445,7 @@ enum net_bridge_opts {
> 	BROPT_NO_LL_LEARN,
> 	BROPT_VLAN_BRIDGE_BINDING,
> 	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
>+	BROPT_LOCAL_RECEIVE,
> };
> 
> struct net_bridge {
>@@ -720,6 +721,7 @@ int br_boolopt_multi_toggle(struct net_bridge *br,
> void br_boolopt_multi_get(const struct net_bridge *br,
> 			  struct br_boolopt_multi *bm);
> void br_opt_toggle(struct net_bridge *br, enum net_bridge_opts opt, bool on);
>+int br_local_receive_change(struct net_bridge *p, bool local_receive);
> 
> /* br_device.c */
> void br_dev_setup(struct net_device *dev);
>diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>index 3f7ca88c2aa3..9af0c2ba929c 100644
>--- a/net/bridge/br_sysfs_br.c
>+++ b/net/bridge/br_sysfs_br.c
>@@ -84,6 +84,28 @@ static ssize_t forward_delay_store(struct device *d,
> }
> static DEVICE_ATTR_RW(forward_delay);
> 
>+static ssize_t local_receive_show(struct device *d,
>+				  struct device_attribute *attr, char *buf)
>+{
>+	struct net_bridge *br = to_bridge(d);
>+
>+	return sprintf(buf, "%u\n", br_opt_get(br, BROPT_LOCAL_RECEIVE));
>+}
>+
>+static int set_local_receive(struct net_bridge *br, unsigned long val,
>+			     struct netlink_ext_ack *extack)
>+{
>+	return br_local_receive_change(br, !!val);
>+}
>+
>+static ssize_t local_receive_store(struct device *d,
>+				   struct device_attribute *attr,
>+				   const char *buf, size_t len)
>+{
>+	return store_bridge_parm(d, buf, len, set_local_receive);
>+}
>+static DEVICE_ATTR_RW(local_receive);
>+

Drop sysfs too, netlink only

> static ssize_t hello_time_show(struct device *d, struct device_attribute *attr,
> 			       char *buf)
> {
>@@ -950,6 +972,7 @@ static struct attribute *bridge_attrs[] = {
> 	&dev_attr_group_addr.attr,
> 	&dev_attr_flush.attr,
> 	&dev_attr_no_linklocal_learn.attr,
>+	&dev_attr_local_receive.attr,
> #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> 	&dev_attr_multicast_router.attr,
> 	&dev_attr_multicast_snooping.attr,
>diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>index 7557e90b60e1..57dd14d5e360 100644
>--- a/net/bridge/br_vlan.c
>+++ b/net/bridge/br_vlan.c
>@@ -905,6 +905,14 @@ bool br_vlan_enabled(const struct net_device *dev)
> }
> EXPORT_SYMBOL_GPL(br_vlan_enabled);
> 
>+bool br_local_receive_enabled(const struct net_device *dev)
>+{
>+	struct net_bridge *br = netdev_priv(dev);
>+
>+	return br_opt_get(br, BROPT_LOCAL_RECEIVE);
>+}
>+EXPORT_SYMBOL_GPL(br_local_receive_enabled);
>+

What the hell is this doing in br_vlan.c???

> int br_vlan_get_proto(const struct net_device *dev, u16 *p_proto)
> {
> 	struct net_bridge *br = netdev_priv(dev);

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Roopa Prabhu]

On 3/1/22 08:43, Ido Schimmel wrote:
> On Tue, Mar 01, 2022 at 01:31:02PM +0100, Mattias Forsblad wrote:
>> This patch implements the bridge flag local_receive. When this
>> flag is cleared packets received on bridge ports will not be forwarded up.
>> This makes is possible to only forward traffic between the port members
>> of the bridge.
>>
>> Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
>> ---
>>   include/linux/if_bridge.h      |  6 ++++++
>>   include/net/switchdev.h        |  2 ++
> Nik might ask you to split the offload part from the bridge
> implementation. Please wait for his feedback as he might be AFK right
> now
>
>>   include/uapi/linux/if_bridge.h |  1 +
>>   include/uapi/linux/if_link.h   |  1 +
>>   net/bridge/br.c                | 18 ++++++++++++++++++
>>   net/bridge/br_device.c         |  1 +
>>   net/bridge/br_input.c          |  3 +++
>>   net/bridge/br_ioctl.c          |  1 +
>>   net/bridge/br_netlink.c        | 14 +++++++++++++-
>>   net/bridge/br_private.h        |  2 ++
>>   net/bridge/br_sysfs_br.c       | 23 +++++++++++++++++++++++
> I believe the bridge doesn't implement sysfs for new attributes
>
>>   net/bridge/br_vlan.c           |  8 ++++++++
>>   12 files changed, 79 insertions(+), 1 deletion(-)
> [...]
>
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index e0c13fcc50ed..5864b61157d3 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>   		break;
>>   	}
>>   
>> +	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
>> +		local_rcv = false;
>> +
> I don't think the description in the commit message is accurate:
> "packets received on bridge ports will not be forwarded up". From the
> code it seems that if packets hit a local FDB entry, then they will be
> "forwarded up". Instead, it seems that packets will not be flooded
> towards the bridge. In which case, why not maintain the same granularity
> we have for the rest of the ports and split this into unicast /
> multicast / broadcast?
>
> BTW, while the patch honors local FDB entries, it overrides host MDB
> entries which seems wrong / inconsistent.

+1

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Mattias Forsblad]

On 2022-03-01 23:36, Nikolay Aleksandrov wrote:
> On 1 March 2022 17:43:27 CET, Ido Schimmel <idosch@idosch.org> wrote:
>> On Tue, Mar 01, 2022 at 01:31:02PM +0100, Mattias Forsblad wrote:
>>> This patch implements the bridge flag local_receive. When this
>>> flag is cleared packets received on bridge ports will not be forwarded up.
>>> This makes is possible to only forward traffic between the port members
>>> of the bridge.
>>>
>>> Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
>>> ---
>>>  include/linux/if_bridge.h      |  6 ++++++
>>>  include/net/switchdev.h        |  2 ++
>>
>> Nik might ask you to split the offload part from the bridge
>> implementation. Please wait for his feedback as he might be AFK right
>> now
>>
> 
> Indeed, I'm traveling and won't have pc access until end of week (Sun). 
> I'll try to review the patches through my phoneas much as I can.
> Ack on the split.
> 

I'll split the patch, thanks!

>>>  include/uapi/linux/if_bridge.h |  1 +
>>>  include/uapi/linux/if_link.h   |  1 +
>>>  net/bridge/br.c                | 18 ++++++++++++++++++
>>>  net/bridge/br_device.c         |  1 +
>>>  net/bridge/br_input.c          |  3 +++
>>>  net/bridge/br_ioctl.c          |  1 +
>>>  net/bridge/br_netlink.c        | 14 +++++++++++++-
>>>  net/bridge/br_private.h        |  2 ++
>>>  net/bridge/br_sysfs_br.c       | 23 +++++++++++++++++++++++
>>
>> I believe the bridge doesn't implement sysfs for new attributes
>>
> 
> Right, no new sysfs please.
> 

Ok, I wasn't aware of that. I'll drop that part, thanks!

>>>  net/bridge/br_vlan.c           |  8 ++++++++
>>>  12 files changed, 79 insertions(+), 1 deletion(-)
>>
>> [...]
>>
>>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>>> index e0c13fcc50ed..5864b61157d3 100644
>>> --- a/net/bridge/br_input.c
>>> +++ b/net/bridge/br_input.c
>>> @@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>  		break;
>>>  	}
>>>  
>>> +	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
>>> +		local_rcv = false;
>>> +
>>
>> I don't think the description in the commit message is accurate:
>> "packets received on bridge ports will not be forwarded up". From the
>> code it seems that if packets hit a local FDB entry, then they will be
>> "forwarded up". Instead, it seems that packets will not be flooded
>> towards the bridge. In which case, why not maintain the same granularity
>> we have for the rest of the ports and split this into unicast /
>> multicast / broadcast?
>>
> 
> Exactly my first thought - why not implement the same control for the bridge?
> Also try to minimize the fast-path hit, you can keep the needed changes 
> localized only to the cases where they are needed.
> I'll send a few more comments in a reply to the patch.
> 

Soo, if I understand you correctly, you want to have three different options?
local_receive_unicast
local_receive_multicast
local_receive_broadcast

>> BTW, while the patch honors local FDB entries, it overrides host MDB
>> entries which seems wrong / inconsistent.
>>
>>>  	if (dst) {
>>>  		unsigned long now = jiffies;
> 

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Mattias Forsblad]

On 2022-03-01 23:43, Nikolay Aleksandrov wrote:
> On 1 March 2022 13:31:02 CET, Mattias Forsblad <mattias.forsblad@gmail.com> wrote:
>> This patch implements the bridge flag local_receive. When this
>> flag is cleared packets received on bridge ports will not be forwarded up.
>> This makes is possible to only forward traffic between the port members
>> of the bridge.
>>
>> Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
>> ---
>> include/linux/if_bridge.h      |  6 ++++++
>> include/net/switchdev.h        |  2 ++
>> include/uapi/linux/if_bridge.h |  1 +
>> include/uapi/linux/if_link.h   |  1 +
>> net/bridge/br.c                | 18 ++++++++++++++++++
>> net/bridge/br_device.c         |  1 +
>> net/bridge/br_input.c          |  3 +++
>> net/bridge/br_ioctl.c          |  1 +
>> net/bridge/br_netlink.c        | 14 +++++++++++++-
>> net/bridge/br_private.h        |  2 ++
>> net/bridge/br_sysfs_br.c       | 23 +++++++++++++++++++++++
>> net/bridge/br_vlan.c           |  8 ++++++++
>> 12 files changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>> index 3aae023a9353..e6b77d18c1d2 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_local_receive_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_local_receive_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/net/switchdev.h b/include/net/switchdev.h
>> index 3e424d40fae3..aec5c1f9b5c7 100644
>> --- a/include/net/switchdev.h
>> +++ b/include/net/switchdev.h
>> @@ -25,6 +25,7 @@ enum switchdev_attr_id {
>> 	SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING,
>> 	SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL,
>> +	SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RECEIVE,
>> 	SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
>> 	SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
>> 	SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
>> @@ -50,6 +51,7 @@ struct switchdev_attr {
>> 		u16 vlan_protocol;			/* BRIDGE_VLAN_PROTOCOL */
>> 		bool mc_disabled;			/* MC_DISABLED */
>> 		u8 mrp_port_role;			/* MRP_PORT_ROLE */
>> +		bool local_receive;			/* BRIDGE_LOCAL_RECEIVE */
>> 	} u;
>> };
>>
>> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
>> index 2711c3522010..fc889b5ccd69 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 local_receive;
>> };
>>
>> struct __port_info {
>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> index e315e53125f4..bb7c25e1c89c 100644
>> --- a/include/uapi/linux/if_link.h
>> +++ b/include/uapi/linux/if_link.h
>> @@ -482,6 +482,7 @@ enum {
>> 	IFLA_BR_VLAN_STATS_PER_PORT,
>> 	IFLA_BR_MULTI_BOOLOPT,
>> 	IFLA_BR_MCAST_QUERIER_STATE,
>> +	IFLA_BR_LOCAL_RECEIVE,
> 
> Please use the boolopt api for new boolean options
> We're trying to limit the nl options expansion as the bridge is the
> largest user.
> 

I'll move to that api, thanks!

>> 	__IFLA_BR_MAX,
>> };
>>
>> diff --git a/net/bridge/br.c b/net/bridge/br.c
>> index b1dea3febeea..ff7eb4f269ec 100644
>> --- a/net/bridge/br.c
>> +++ b/net/bridge/br.c
>> @@ -325,6 +325,24 @@ void br_boolopt_multi_get(const struct net_bridge *br,
>> 	bm->optmask = GENMASK((BR_BOOLOPT_MAX - 1), 0);
>> }
>>
>> +int br_local_receive_change(struct net_bridge *p,
>> +			    bool local_receive)
>> +{
>> +	struct switchdev_attr attr = {
>> +		.orig_dev = p->dev,
>> +		.id = SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RECEIVE,
>> +		.flags = SWITCHDEV_F_DEFER,
>> +		.u.local_receive = local_receive,
>> +	};
>> +	int ret;
>> +
>> +	ret = switchdev_port_attr_set(p->dev, &attr, NULL);
>> +	if (!ret)
>> +		br_opt_toggle(p, BROPT_LOCAL_RECEIVE, local_receive);
>> +
>> +	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..7cd9c5880d18 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -524,6 +524,7 @@ 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_LOCAL_RECEIVE, 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..5864b61157d3 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>> 		break;
>> 	}
>>
>> +	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
>> +		local_rcv = false;
>> +
> 
> this affects the whole fast path, it can be better localized to make sure
> it will not affect all use cases
> 
>> 	if (dst) {
>> 		unsigned long now = jiffies;
>>
>> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
>> index f213ed108361..abe538129290 100644
>> --- a/net/bridge/br_ioctl.c
>> +++ b/net/bridge/br_ioctl.c
>> @@ -177,6 +177,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq,
>> 		b.topology_change = br->topology_change;
>> 		b.topology_change_detected = br->topology_change_detected;
>> 		b.root_port = br->root_port;
>> +		b.local_receive = br_opt_get(br, BROPT_LOCAL_RECEIVE) ? 1 : 0;
> 
> ioctl is not being extended anymore, please drop it
> 

I'll drop it, thanks!

>>
>> 		b.stp_enabled = (br->stp_enabled != BR_NO_STP);
>> 		b.ageing_time = jiffies_to_clock_t(br->ageing_time);
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 7d4432ca9a20..5e7f99950195 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -1163,6 +1163,7 @@ static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
>> 	[IFLA_BR_MCAST_IGMP_VERSION] = { .type = NLA_U8 },
>> 	[IFLA_BR_MCAST_MLD_VERSION] = { .type = NLA_U8 },
>> 	[IFLA_BR_VLAN_STATS_PER_PORT] = { .type = NLA_U8 },
>> +	[IFLA_BR_LOCAL_RECEIVE] = { .type = NLA_U8 },
>> 	[IFLA_BR_MULTI_BOOLOPT] =
>> 		NLA_POLICY_EXACT_LEN(sizeof(struct br_boolopt_multi)),
>> };
>> @@ -1434,6 +1435,14 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
>> 			return err;
>> 	}
>>
>> +	if (data[IFLA_BR_LOCAL_RECEIVE]) {
>> +		u8 val = nla_get_u8(data[IFLA_BR_LOCAL_RECEIVE]);
>> +
>> +		err = br_local_receive_change(br, !!val);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> 	return 0;
>> }
>>
>> @@ -1514,6 +1523,7 @@ static size_t br_get_size(const struct net_device *brdev)
>> 	       nla_total_size(sizeof(u8)) +     /* IFLA_BR_NF_CALL_ARPTABLES */
>> #endif
>> 	       nla_total_size(sizeof(struct br_boolopt_multi)) + /* IFLA_BR_MULTI_BOOLOPT */
>> +	       nla_total_size(sizeof(u8)) +     /* IFLA_BR_LOCAL_RECEIVE */
>> 	       0;
>> }
>>
>> @@ -1527,6 +1537,7 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
>> 	u32 stp_enabled = br->stp_enabled;
>> 	u16 priority = (br->bridge_id.prio[0] << 8) | br->bridge_id.prio[1];
>> 	u8 vlan_enabled = br_vlan_enabled(br->dev);
>> +	u8 local_receive = br_opt_get(br, BROPT_LOCAL_RECEIVE) ? 1 : 0;
>> 	struct br_boolopt_multi bm;
>> 	u64 clockval;
>>
>> @@ -1563,7 +1574,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
>> 	    nla_put_u8(skb, IFLA_BR_TOPOLOGY_CHANGE_DETECTED,
>> 		       br->topology_change_detected) ||
>> 	    nla_put(skb, IFLA_BR_GROUP_ADDR, ETH_ALEN, br->group_addr) ||
>> -	    nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm))
>> +	    nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm) ||
>> +	    nla_put_u8(skb, IFLA_BR_LOCAL_RECEIVE, local_receive))
>> 		return -EMSGSIZE;
>>
>> #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 48bc61ebc211..01fa5426094b 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -445,6 +445,7 @@ enum net_bridge_opts {
>> 	BROPT_NO_LL_LEARN,
>> 	BROPT_VLAN_BRIDGE_BINDING,
>> 	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
>> +	BROPT_LOCAL_RECEIVE,
>> };
>>
>> struct net_bridge {
>> @@ -720,6 +721,7 @@ int br_boolopt_multi_toggle(struct net_bridge *br,
>> void br_boolopt_multi_get(const struct net_bridge *br,
>> 			  struct br_boolopt_multi *bm);
>> void br_opt_toggle(struct net_bridge *br, enum net_bridge_opts opt, bool on);
>> +int br_local_receive_change(struct net_bridge *p, bool local_receive);
>>
>> /* br_device.c */
>> void br_dev_setup(struct net_device *dev);
>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>> index 3f7ca88c2aa3..9af0c2ba929c 100644
>> --- a/net/bridge/br_sysfs_br.c
>> +++ b/net/bridge/br_sysfs_br.c
>> @@ -84,6 +84,28 @@ static ssize_t forward_delay_store(struct device *d,
>> }
>> static DEVICE_ATTR_RW(forward_delay);
>>
>> +static ssize_t local_receive_show(struct device *d,
>> +				  struct device_attribute *attr, char *buf)
>> +{
>> +	struct net_bridge *br = to_bridge(d);
>> +
>> +	return sprintf(buf, "%u\n", br_opt_get(br, BROPT_LOCAL_RECEIVE));
>> +}
>> +
>> +static int set_local_receive(struct net_bridge *br, unsigned long val,
>> +			     struct netlink_ext_ack *extack)
>> +{
>> +	return br_local_receive_change(br, !!val);
>> +}
>> +
>> +static ssize_t local_receive_store(struct device *d,
>> +				   struct device_attribute *attr,
>> +				   const char *buf, size_t len)
>> +{
>> +	return store_bridge_parm(d, buf, len, set_local_receive);
>> +}
>> +static DEVICE_ATTR_RW(local_receive);
>> +
> 
> Drop sysfs too, netlink only
> 

Yes, will do, thanks!

>> static ssize_t hello_time_show(struct device *d, struct device_attribute *attr,
>> 			       char *buf)
>> {
>> @@ -950,6 +972,7 @@ static struct attribute *bridge_attrs[] = {
>> 	&dev_attr_group_addr.attr,
>> 	&dev_attr_flush.attr,
>> 	&dev_attr_no_linklocal_learn.attr,
>> +	&dev_attr_local_receive.attr,
>> #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>> 	&dev_attr_multicast_router.attr,
>> 	&dev_attr_multicast_snooping.attr,
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 7557e90b60e1..57dd14d5e360 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -905,6 +905,14 @@ bool br_vlan_enabled(const struct net_device *dev)
>> }
>> EXPORT_SYMBOL_GPL(br_vlan_enabled);
>>
>> +bool br_local_receive_enabled(const struct net_device *dev)
>> +{
>> +	struct net_bridge *br = netdev_priv(dev);
>> +
>> +	return br_opt_get(br, BROPT_LOCAL_RECEIVE);
>> +}
>> +EXPORT_SYMBOL_GPL(br_local_receive_enabled);
>> +
> 
> What the hell is this doing in br_vlan.c???
> 
>> int br_vlan_get_proto(const struct net_device *dev, u16 *p_proto)
>> {
>> 	struct net_bridge *br = netdev_priv(dev);
> 

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Mattias Forsblad]

On 2022-03-01 23:43, Nikolay Aleksandrov wrote:
> On 1 March 2022 13:31:02 CET, Mattias Forsblad <mattias.forsblad@gmail.com> wrote:
>> This patch implements the bridge flag local_receive. When this
>> flag is cleared packets received on bridge ports will not be forwarded up.
>> This makes is possible to only forward traffic between the port members
>> of the bridge.
>>
>> Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@gmail.com>
>> ---
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 7557e90b60e1..57dd14d5e360 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -905,6 +905,14 @@ bool br_vlan_enabled(const struct net_device *dev)
>> }
>> EXPORT_SYMBOL_GPL(br_vlan_enabled);
>>
>> +bool br_local_receive_enabled(const struct net_device *dev)
>> +{
>> +	struct net_bridge *br = netdev_priv(dev);
>> +
>> +	return br_opt_get(br, BROPT_LOCAL_RECEIVE);
>> +}
>> +EXPORT_SYMBOL_GPL(br_local_receive_enabled);
>> +
> 
> What the hell is this doing in br_vlan.c???
> 

I'm truly sorry to have made an error, might I inqire for a better approach?

>> int br_vlan_get_proto(const struct net_device *dev, u16 *p_proto)
>> {
>> 	struct net_bridge *br = netdev_priv(dev);
> 

Re: [PATCH 3/3] mv88e6xxx: Offload the local_receive flag

[kernel test robot]

Hi Mattias,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Mattias-Forsblad/bridge-dsa-switchdev-mv88e6xxx-Implement-local_receive-bridge-flag/20220301-203159
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 1e385c08249e4822e0f425efde1896d3933d1471
config: x86_64-randconfig-a002-20220124 (https://download.01.org/0day-ci/archive/20220302/202203022050.4XOWNDtx-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/fcf70df585fafd4afbec12c1597bfc62c3245c32
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mattias-Forsblad/bridge-dsa-switchdev-mv88e6xxx-Implement-local_receive-bridge-flag/20220301-203159
        git checkout fcf70df585fafd4afbec12c1597bfc62c3245c32
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "br_local_receive_enabled" [drivers/net/dsa/mv88e6xxx/mv88e6xxx.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 3/3] mv88e6xxx: Offload the local_receive flag

[kernel test robot]

Hi Mattias,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Mattias-Forsblad/bridge-dsa-switchdev-mv88e6xxx-Implement-local_receive-bridge-flag/20220301-203159
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 1e385c08249e4822e0f425efde1896d3933d1471
config: parisc-randconfig-r012-20220302 (https://download.01.org/0day-ci/archive/20220302/202203022121.C5ab64dX-lkp@intel.com/config)
compiler: hppa64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/fcf70df585fafd4afbec12c1597bfc62c3245c32
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mattias-Forsblad/bridge-dsa-switchdev-mv88e6xxx-Implement-local_receive-bridge-flag/20220301-203159
        git checkout fcf70df585fafd4afbec12c1597bfc62c3245c32
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=parisc64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   hppa64-linux-ld: drivers/net/dsa/mv88e6xxx/chip.o: in function `.LC292':
>> (.data.rel.ro+0x7c8): undefined reference to `br_local_receive_enabled'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Ido Schimmel]

On Wed, Mar 02, 2022 at 07:27:25AM +0100, Mattias Forsblad wrote:
> On 2022-03-01 23:36, Nikolay Aleksandrov wrote:
> > On 1 March 2022 17:43:27 CET, Ido Schimmel <idosch@idosch.org> wrote:
> >> On Tue, Mar 01, 2022 at 01:31:02PM +0100, Mattias Forsblad wrote:
> >>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> >>> index e0c13fcc50ed..5864b61157d3 100644
> >>> --- a/net/bridge/br_input.c
> >>> +++ b/net/bridge/br_input.c
> >>> @@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >>>  		break;
> >>>  	}
> >>>  
> >>> +	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
> >>> +		local_rcv = false;
> >>> +
> >>
> >> I don't think the description in the commit message is accurate:
> >> "packets received on bridge ports will not be forwarded up". From the
> >> code it seems that if packets hit a local FDB entry, then they will be
> >> "forwarded up". Instead, it seems that packets will not be flooded
> >> towards the bridge. In which case, why not maintain the same granularity
> >> we have for the rest of the ports and split this into unicast /
> >> multicast / broadcast?
> >>
> > 
> > Exactly my first thought - why not implement the same control for the bridge?
> > Also try to minimize the fast-path hit, you can keep the needed changes 
> > localized only to the cases where they are needed.
> > I'll send a few more comments in a reply to the patch.
> > 
> 
> Soo, if I understand you correctly, you want to have three different options?
> local_receive_unicast
> local_receive_multicast
> local_receive_broadcast

My understanding of the feature is that you want to prevent flooding
towards the bridge. In which case, it makes sense to keep the same
granularity as for regular bridge ports and also name the options
similarly. We already have several options that are applicable to both
the bridge and bridge ports (e.g., 'mcast_router').

I suggest:

$ ip link help bridge
Usage: ... bridge [ fdb_flush ]
                  ...
                  [ flood {on | off} ]
                  [ mcast_flood {on | off} ]
                  [ bcast_flood {on | off} ]

This is consistent with "bridge_slave".

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Ido Schimmel]

On Mon, Mar 14, 2022 at 06:29:58PM +0200, Ido Schimmel wrote:
> On Wed, Mar 02, 2022 at 07:27:25AM +0100, Mattias Forsblad wrote:
> > On 2022-03-01 23:36, Nikolay Aleksandrov wrote:
> > > On 1 March 2022 17:43:27 CET, Ido Schimmel <idosch@idosch.org> wrote:
> > >> On Tue, Mar 01, 2022 at 01:31:02PM +0100, Mattias Forsblad wrote:
> > >>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > >>> index e0c13fcc50ed..5864b61157d3 100644
> > >>> --- a/net/bridge/br_input.c
> > >>> +++ b/net/bridge/br_input.c
> > >>> @@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> > >>>  		break;
> > >>>  	}
> > >>>  
> > >>> +	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
> > >>> +		local_rcv = false;
> > >>> +
> > >>
> > >> I don't think the description in the commit message is accurate:
> > >> "packets received on bridge ports will not be forwarded up". From the
> > >> code it seems that if packets hit a local FDB entry, then they will be
> > >> "forwarded up". Instead, it seems that packets will not be flooded
> > >> towards the bridge. In which case, why not maintain the same granularity
> > >> we have for the rest of the ports and split this into unicast /
> > >> multicast / broadcast?
> > >>
> > > 
> > > Exactly my first thought - why not implement the same control for the bridge?
> > > Also try to minimize the fast-path hit, you can keep the needed changes 
> > > localized only to the cases where they are needed.
> > > I'll send a few more comments in a reply to the patch.
> > > 
> > 
> > Soo, if I understand you correctly, you want to have three different options?
> > local_receive_unicast
> > local_receive_multicast
> > local_receive_broadcast
> 
> My understanding of the feature is that you want to prevent flooding
> towards the bridge. In which case, it makes sense to keep the same
> granularity as for regular bridge ports and also name the options
> similarly. We already have several options that are applicable to both
> the bridge and bridge ports (e.g., 'mcast_router').
> 
> I suggest:
> 
> $ ip link help bridge
> Usage: ... bridge [ fdb_flush ]
>                   ...
>                   [ flood {on | off} ]
>                   [ mcast_flood {on | off} ]
>                   [ bcast_flood {on | off} ]
> 
> This is consistent with "bridge_slave".

And please add a selftest. See this commit for reference:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=b2b681a412517bf477238de62b1d227361fa04fe

It should allow you to test both the software data path (using veth
pairs) and the hardware data path (using physical loopbacks).

Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

[Mattias Forsblad]

On 2022-03-14 17:29, Ido Schimmel wrote:
> On Wed, Mar 02, 2022 at 07:27:25AM +0100, Mattias Forsblad wrote:
>> On 2022-03-01 23:36, Nikolay Aleksandrov wrote:
>>> On 1 March 2022 17:43:27 CET, Ido Schimmel <idosch@idosch.org> wrote:
>>>> On Tue, Mar 01, 2022 at 01:31:02PM +0100, Mattias Forsblad wrote:
>>>>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>>>>> index e0c13fcc50ed..5864b61157d3 100644
>>>>> --- a/net/bridge/br_input.c
>>>>> +++ b/net/bridge/br_input.c
>>>>> @@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>>>>  		break;
>>>>>  	}
>>>>>  
>>>>> +	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
>>>>> +		local_rcv = false;
>>>>> +
>>>>
>>>> I don't think the description in the commit message is accurate:
>>>> "packets received on bridge ports will not be forwarded up". From the
>>>> code it seems that if packets hit a local FDB entry, then they will be
>>>> "forwarded up". Instead, it seems that packets will not be flooded
>>>> towards the bridge. In which case, why not maintain the same granularity
>>>> we have for the rest of the ports and split this into unicast /
>>>> multicast / broadcast?
>>>>
>>>
>>> Exactly my first thought - why not implement the same control for the bridge?
>>> Also try to minimize the fast-path hit, you can keep the needed changes 
>>> localized only to the cases where they are needed.
>>> I'll send a few more comments in a reply to the patch.
>>>
>>
>> Soo, if I understand you correctly, you want to have three different options?
>> local_receive_unicast
>> local_receive_multicast
>> local_receive_broadcast
> 
> My understanding of the feature is that you want to prevent flooding
> towards the bridge. In which case, it makes sense to keep the same
> granularity as for regular bridge ports and also name the options
> similarly. We already have several options that are applicable to both
> the bridge and bridge ports (e.g., 'mcast_router').
> 
> I suggest:
> 
> $ ip link help bridge
> Usage: ... bridge [ fdb_flush ]
>                   ...
>                   [ flood {on | off} ]
>                   [ mcast_flood {on | off} ]
>                   [ bcast_flood {on | off} ]
> 
> This is consistent with "bridge_slave".

Many thanks for your input. I'll have a go at a V2.

BR
Mattias Forsblad

Re: [PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Vladimir Oltean]

Hello Tobias,

On Tue, Mar 01, 2022 at 10:04:09PM +0100, Tobias Waldekranz wrote:
> On Tue, Mar 01, 2022 at 09:14, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > On 3/1/2022 4:31 AM, Mattias Forsblad wrote:
> >> Greetings,
> >> 
> >> This series implements a new bridge flag 'local_receive' and HW
> >> offloading for Marvell mv88e6xxx.
> >> 
> >> When using a non-VLAN filtering bridge we want to be able to limit
> >> traffic to the CPU port to lessen the CPU load. This is specially
> >> important when we have disabled learning on user ports.
> >> 
> >> A sample configuration could be something like this:
> >> 
> >>         br0
> >>        /   \
> >>     swp0   swp1
> >> 
> >> ip link add dev br0 type bridge stp_state 0 vlan_filtering 0
> >> ip link set swp0 master br0
> >> ip link set swp1 master br0
> >> ip link set swp0 type bridge_slave learning off
> >> ip link set swp1 type bridge_slave learning off
> >> ip link set swp0 up
> >> ip link set swp1 up
> >> ip link set br0 type bridge local_receive 0
> >> ip link set br0 up
> >> 
> >> The first part of the series implements the flag for the SW bridge
> >> and the second part the DSA infrastructure. The last part implements
> >> offloading of this flag to HW for mv88e6xxx, which uses the
> >> port vlan table to restrict the ingress from user ports
> >> to the CPU port when this flag is cleared.
> >
> > Why not use a bridge with VLAN filtering enabled? I cannot quite find it 
> > right now, but Vladimir recently picked up what I had attempted before 
> > which was to allow removing the CPU port (via the bridge master device) 
> > from a specific group of VLANs to achieve that isolation.
> >
> 
> Hi Florian,
> 
> Yes we are aware of this work, which is awesome by the way! For anyone
> else who is interested, I believe you are referring to this series:
> 
> https://lore.kernel.org/netdev/20220215170218.2032432-1-vladimir.oltean@nxp.com/
> 
> There are cases though, where you want a TPMR-like setup (or "dumb hub"
> mode, if you will) and ignore all tag information.
> 
> One application could be to use a pair of ports on a switch as an
> ethernet extender/repeater for topologies that span large physical
> distances. If this repeater is part of a redundant topology, you'd to
> well to disable learning, in order to avoid dropping packets when the
> surrounding active topology changes. This, in turn, will mean that all
> flows will be classified as unknown unicast. For that reason it is very
> important that the CPU be shielded.

So have you seriously considered making the bridge ports that operate in
'dumb hub' mode have a pvid which isn't installed as a 'self' entry on
the bridge device?

> You might be tempted to solve this using flooding filters of the
> switch's CPU port, but these go out the window if you have another
> bridge configured, that requires that flooding of unknown traffic is
> enabled.

Not if CPU flooding can be managed on a per-user-port basis.

> Another application is to create a similar setup, but with three ports,
> and have the third one be used as a TAP.

Could you expand more on this use case?

> >> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
> >
> > I don't believe this tag has much value since it was presumably carried 
> > over from an internal review. Might be worth adding it publicly now, though.
> 
> I think Mattias meant to replicate this tag on each individual
> patch. Aside from that though, are you saying that a tag is never valid
> unless there is a public message on the list from the signee? Makes
> sense I suppose. Anyway, I will send separate tags for this series.

Re: [PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Tobias Waldekranz]

On Thu, Mar 17, 2022 at 16:05, Vladimir Oltean <olteanv@gmail.com> wrote:
> Hello Tobias,
>
> On Tue, Mar 01, 2022 at 10:04:09PM +0100, Tobias Waldekranz wrote:
>> On Tue, Mar 01, 2022 at 09:14, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> > On 3/1/2022 4:31 AM, Mattias Forsblad wrote:
>> >> Greetings,
>> >> 
>> >> This series implements a new bridge flag 'local_receive' and HW
>> >> offloading for Marvell mv88e6xxx.
>> >> 
>> >> When using a non-VLAN filtering bridge we want to be able to limit
>> >> traffic to the CPU port to lessen the CPU load. This is specially
>> >> important when we have disabled learning on user ports.
>> >> 
>> >> A sample configuration could be something like this:
>> >> 
>> >>         br0
>> >>        /   \
>> >>     swp0   swp1
>> >> 
>> >> ip link add dev br0 type bridge stp_state 0 vlan_filtering 0
>> >> ip link set swp0 master br0
>> >> ip link set swp1 master br0
>> >> ip link set swp0 type bridge_slave learning off
>> >> ip link set swp1 type bridge_slave learning off
>> >> ip link set swp0 up
>> >> ip link set swp1 up
>> >> ip link set br0 type bridge local_receive 0
>> >> ip link set br0 up
>> >> 
>> >> The first part of the series implements the flag for the SW bridge
>> >> and the second part the DSA infrastructure. The last part implements
>> >> offloading of this flag to HW for mv88e6xxx, which uses the
>> >> port vlan table to restrict the ingress from user ports
>> >> to the CPU port when this flag is cleared.
>> >
>> > Why not use a bridge with VLAN filtering enabled? I cannot quite find it 
>> > right now, but Vladimir recently picked up what I had attempted before 
>> > which was to allow removing the CPU port (via the bridge master device) 
>> > from a specific group of VLANs to achieve that isolation.
>> >
>> 
>> Hi Florian,
>> 
>> Yes we are aware of this work, which is awesome by the way! For anyone
>> else who is interested, I believe you are referring to this series:
>> 
>> https://lore.kernel.org/netdev/20220215170218.2032432-1-vladimir.oltean@nxp.com/
>> 
>> There are cases though, where you want a TPMR-like setup (or "dumb hub"
>> mode, if you will) and ignore all tag information.
>> 
>> One application could be to use a pair of ports on a switch as an
>> ethernet extender/repeater for topologies that span large physical
>> distances. If this repeater is part of a redundant topology, you'd to
>> well to disable learning, in order to avoid dropping packets when the
>> surrounding active topology changes. This, in turn, will mean that all
>> flows will be classified as unknown unicast. For that reason it is very
>> important that the CPU be shielded.
>
> So have you seriously considered making the bridge ports that operate in
> 'dumb hub' mode have a pvid which isn't installed as a 'self' entry on
> the bridge device?

Just so there's no confusion, you mean something like...

    ip link add dev br0 type bridge vlan_filtering 1 vlan_default_pvid 0

    for p in swp0 swp1; do
        ip link set dev $p master br0
        bridge vlan add dev $p vid 1 pvid untagged
    done

... right?

In that case, the repeater is no longer transparent with respect to
tagged packets, which the application requires.

>> You might be tempted to solve this using flooding filters of the
>> switch's CPU port, but these go out the window if you have another
>> bridge configured, that requires that flooding of unknown traffic is
>> enabled.
>
> Not if CPU flooding can be managed on a per-user-port basis.

True, but we aren't lucky enough to have hardware that can do that :)

>> Another application is to create a similar setup, but with three ports,
>> and have the third one be used as a TAP.
>
> Could you expand more on this use case?

Its just the standard use-case for a TAP really. You have some link of
interest that you want to snoop, but for some reason there is no way of
getting a PCAP from the station on either side:

   Link of interest
          |
.-------. v .-------.
| Alice +---+  Bob  |
'-------'   '-------'

So you insert a hub in the middle, and listen on a third port:

.-------.   .-----.   .-------.
| Alice +---+ TAP +---+  Bob  |
'-------'   '--+--'   '-------'
               |
 PC running tcpdump/wireshark

The nice thing about being able to set this up in Linux is that if your
hardware comes with a mix of media types, you can dynamically create the
TAP for the job at hand. E.g. if Alice and Bob are communicating over a
fiber, but your PC only has a copper interface, you can bridge to fiber
ports with one copper; if you need to monitor a copper link 5min later,
you just swap out the fiber ports for two copper ports.

>> >> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
>> >
>> > I don't believe this tag has much value since it was presumably carried 
>> > over from an internal review. Might be worth adding it publicly now, though.
>> 
>> I think Mattias meant to replicate this tag on each individual
>> patch. Aside from that though, are you saying that a tag is never valid
>> unless there is a public message on the list from the signee? Makes
>> sense I suppose. Anyway, I will send separate tags for this series.

Re: [PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Vladimir Oltean]

On Fri, Mar 18, 2022 at 08:58:11AM +0100, Tobias Waldekranz wrote:
> On Thu, Mar 17, 2022 at 16:05, Vladimir Oltean <olteanv@gmail.com> wrote:
> > Hello Tobias,
> >
> > On Tue, Mar 01, 2022 at 10:04:09PM +0100, Tobias Waldekranz wrote:
> >> On Tue, Mar 01, 2022 at 09:14, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >> > On 3/1/2022 4:31 AM, Mattias Forsblad wrote:
> >> >> Greetings,
> >> >> 
> >> >> This series implements a new bridge flag 'local_receive' and HW
> >> >> offloading for Marvell mv88e6xxx.
> >> >> 
> >> >> When using a non-VLAN filtering bridge we want to be able to limit
> >> >> traffic to the CPU port to lessen the CPU load. This is specially
> >> >> important when we have disabled learning on user ports.
> >> >> 
> >> >> A sample configuration could be something like this:
> >> >> 
> >> >>         br0
> >> >>        /   \
> >> >>     swp0   swp1
> >> >> 
> >> >> ip link add dev br0 type bridge stp_state 0 vlan_filtering 0
> >> >> ip link set swp0 master br0
> >> >> ip link set swp1 master br0
> >> >> ip link set swp0 type bridge_slave learning off
> >> >> ip link set swp1 type bridge_slave learning off
> >> >> ip link set swp0 up
> >> >> ip link set swp1 up
> >> >> ip link set br0 type bridge local_receive 0
> >> >> ip link set br0 up
> >> >> 
> >> >> The first part of the series implements the flag for the SW bridge
> >> >> and the second part the DSA infrastructure. The last part implements
> >> >> offloading of this flag to HW for mv88e6xxx, which uses the
> >> >> port vlan table to restrict the ingress from user ports
> >> >> to the CPU port when this flag is cleared.
> >> >
> >> > Why not use a bridge with VLAN filtering enabled? I cannot quite find it 
> >> > right now, but Vladimir recently picked up what I had attempted before 
> >> > which was to allow removing the CPU port (via the bridge master device) 
> >> > from a specific group of VLANs to achieve that isolation.
> >> >
> >> 
> >> Hi Florian,
> >> 
> >> Yes we are aware of this work, which is awesome by the way! For anyone
> >> else who is interested, I believe you are referring to this series:
> >> 
> >> https://lore.kernel.org/netdev/20220215170218.2032432-1-vladimir.oltean@nxp.com/
> >> 
> >> There are cases though, where you want a TPMR-like setup (or "dumb hub"
> >> mode, if you will) and ignore all tag information.
> >> 
> >> One application could be to use a pair of ports on a switch as an
> >> ethernet extender/repeater for topologies that span large physical
> >> distances. If this repeater is part of a redundant topology, you'd to
> >> well to disable learning, in order to avoid dropping packets when the
> >> surrounding active topology changes. This, in turn, will mean that all
> >> flows will be classified as unknown unicast. For that reason it is very
> >> important that the CPU be shielded.
> >
> > So have you seriously considered making the bridge ports that operate in
> > 'dumb hub' mode have a pvid which isn't installed as a 'self' entry on
> > the bridge device?
> 
> Just so there's no confusion, you mean something like...
> 
>     ip link add dev br0 type bridge vlan_filtering 1 vlan_default_pvid 0
> 
>     for p in swp0 swp1; do
>         ip link set dev $p master br0
>         bridge vlan add dev $p vid 1 pvid untagged
>     done
> 
> ... right?
> 
> In that case, the repeater is no longer transparent with respect to
> tagged packets, which the application requires.

If you are sure that there exists one VLAN ID which is never used (like
4094), what you could do is you could set the port pvids to that VID
instead of 1, and add the entire VLAN_N_VID range sans that VID in the
membership list of the two ports, as egress-tagged.

This is 'practical transparency' - if true transparency is required then
yes, this doesn't work.

> >> You might be tempted to solve this using flooding filters of the
> >> switch's CPU port, but these go out the window if you have another
> >> bridge configured, that requires that flooding of unknown traffic is
> >> enabled.
> >
> > Not if CPU flooding can be managed on a per-user-port basis.
> 
> True, but we aren't lucky enough to have hardware that can do that :)
> 
> >> Another application is to create a similar setup, but with three ports,
> >> and have the third one be used as a TAP.
> >
> > Could you expand more on this use case?
> 
> Its just the standard use-case for a TAP really. You have some link of
> interest that you want to snoop, but for some reason there is no way of
> getting a PCAP from the station on either side:
> 
>    Link of interest
>           |
> .-------. v .-------.
> | Alice +---+  Bob  |
> '-------'   '-------'
> 
> So you insert a hub in the middle, and listen on a third port:
> 
> .-------.   .-----.   .-------.
> | Alice +---+ TAP +---+  Bob  |
> '-------'   '--+--'   '-------'
>                |
>  PC running tcpdump/wireshark
> 
> The nice thing about being able to set this up in Linux is that if your
> hardware comes with a mix of media types, you can dynamically create the
> TAP for the job at hand. E.g. if Alice and Bob are communicating over a
> fiber, but your PC only has a copper interface, you can bridge to fiber
> ports with one copper; if you need to monitor a copper link 5min later,
> you just swap out the fiber ports for two copper ports.
> 
> >> >> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
> >> >
> >> > I don't believe this tag has much value since it was presumably carried 
> >> > over from an internal review. Might be worth adding it publicly now, though.
> >> 
> >> I think Mattias meant to replicate this tag on each individual
> >> patch. Aside from that though, are you saying that a tag is never valid
> >> unless there is a public message on the list from the signee? Makes
> >> sense I suppose. Anyway, I will send separate tags for this series.

Re: [PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Tobias Waldekranz]

On Fri, Mar 18, 2022 at 13:11, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Fri, Mar 18, 2022 at 08:58:11AM +0100, Tobias Waldekranz wrote:
>> On Thu, Mar 17, 2022 at 16:05, Vladimir Oltean <olteanv@gmail.com> wrote:
>> > Hello Tobias,
>> >
>> > On Tue, Mar 01, 2022 at 10:04:09PM +0100, Tobias Waldekranz wrote:
>> >> On Tue, Mar 01, 2022 at 09:14, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> >> > On 3/1/2022 4:31 AM, Mattias Forsblad wrote:
>> >> >> Greetings,
>> >> >> 
>> >> >> This series implements a new bridge flag 'local_receive' and HW
>> >> >> offloading for Marvell mv88e6xxx.
>> >> >> 
>> >> >> When using a non-VLAN filtering bridge we want to be able to limit
>> >> >> traffic to the CPU port to lessen the CPU load. This is specially
>> >> >> important when we have disabled learning on user ports.
>> >> >> 
>> >> >> A sample configuration could be something like this:
>> >> >> 
>> >> >>         br0
>> >> >>        /   \
>> >> >>     swp0   swp1
>> >> >> 
>> >> >> ip link add dev br0 type bridge stp_state 0 vlan_filtering 0
>> >> >> ip link set swp0 master br0
>> >> >> ip link set swp1 master br0
>> >> >> ip link set swp0 type bridge_slave learning off
>> >> >> ip link set swp1 type bridge_slave learning off
>> >> >> ip link set swp0 up
>> >> >> ip link set swp1 up
>> >> >> ip link set br0 type bridge local_receive 0
>> >> >> ip link set br0 up
>> >> >> 
>> >> >> The first part of the series implements the flag for the SW bridge
>> >> >> and the second part the DSA infrastructure. The last part implements
>> >> >> offloading of this flag to HW for mv88e6xxx, which uses the
>> >> >> port vlan table to restrict the ingress from user ports
>> >> >> to the CPU port when this flag is cleared.
>> >> >
>> >> > Why not use a bridge with VLAN filtering enabled? I cannot quite find it 
>> >> > right now, but Vladimir recently picked up what I had attempted before 
>> >> > which was to allow removing the CPU port (via the bridge master device) 
>> >> > from a specific group of VLANs to achieve that isolation.
>> >> >
>> >> 
>> >> Hi Florian,
>> >> 
>> >> Yes we are aware of this work, which is awesome by the way! For anyone
>> >> else who is interested, I believe you are referring to this series:
>> >> 
>> >> https://lore.kernel.org/netdev/20220215170218.2032432-1-vladimir.oltean@nxp.com/
>> >> 
>> >> There are cases though, where you want a TPMR-like setup (or "dumb hub"
>> >> mode, if you will) and ignore all tag information.
>> >> 
>> >> One application could be to use a pair of ports on a switch as an
>> >> ethernet extender/repeater for topologies that span large physical
>> >> distances. If this repeater is part of a redundant topology, you'd to
>> >> well to disable learning, in order to avoid dropping packets when the
>> >> surrounding active topology changes. This, in turn, will mean that all
>> >> flows will be classified as unknown unicast. For that reason it is very
>> >> important that the CPU be shielded.
>> >
>> > So have you seriously considered making the bridge ports that operate in
>> > 'dumb hub' mode have a pvid which isn't installed as a 'self' entry on
>> > the bridge device?
>> 
>> Just so there's no confusion, you mean something like...
>> 
>>     ip link add dev br0 type bridge vlan_filtering 1 vlan_default_pvid 0
>> 
>>     for p in swp0 swp1; do
>>         ip link set dev $p master br0
>>         bridge vlan add dev $p vid 1 pvid untagged
>>     done
>> 
>> ... right?
>> 
>> In that case, the repeater is no longer transparent with respect to
>> tagged packets, which the application requires.
>
> If you are sure that there exists one VLAN ID which is never used (like
> 4094), what you could do is you could set the port pvids to that VID
> instead of 1, and add the entire VLAN_N_VID range sans that VID in the
> membership list of the two ports, as egress-tagged.

Yeah, I've thought about this too. If the device's only role is to act
as a repeater, then you can get away with it. But you will have consumed
all rows in the VTU and half of the rows in the ATU (we add an entry for
the broadcast address in every FID). So if you want to use your other
ports for regular bridging you're left with a very limited feature set.

> This is 'practical transparency' - if true transparency is required then
> yes, this doesn't work.
>
>> >> You might be tempted to solve this using flooding filters of the
>> >> switch's CPU port, but these go out the window if you have another
>> >> bridge configured, that requires that flooding of unknown traffic is
>> >> enabled.
>> >
>> > Not if CPU flooding can be managed on a per-user-port basis.
>> 
>> True, but we aren't lucky enough to have hardware that can do that :)
>> 
>> >> Another application is to create a similar setup, but with three ports,
>> >> and have the third one be used as a TAP.
>> >
>> > Could you expand more on this use case?
>> 
>> Its just the standard use-case for a TAP really. You have some link of
>> interest that you want to snoop, but for some reason there is no way of
>> getting a PCAP from the station on either side:
>> 
>>    Link of interest
>>           |
>> .-------. v .-------.
>> | Alice +---+  Bob  |
>> '-------'   '-------'
>> 
>> So you insert a hub in the middle, and listen on a third port:
>> 
>> .-------.   .-----.   .-------.
>> | Alice +---+ TAP +---+  Bob  |
>> '-------'   '--+--'   '-------'
>>                |
>>  PC running tcpdump/wireshark
>> 
>> The nice thing about being able to set this up in Linux is that if your
>> hardware comes with a mix of media types, you can dynamically create the
>> TAP for the job at hand. E.g. if Alice and Bob are communicating over a
>> fiber, but your PC only has a copper interface, you can bridge to fiber
>> ports with one copper; if you need to monitor a copper link 5min later,
>> you just swap out the fiber ports for two copper ports.
>> 
>> >> >> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
>> >> >
>> >> > I don't believe this tag has much value since it was presumably carried 
>> >> > over from an internal review. Might be worth adding it publicly now, though.
>> >> 
>> >> I think Mattias meant to replicate this tag on each individual
>> >> patch. Aside from that though, are you saying that a tag is never valid
>> >> unless there is a public message on the list from the signee? Makes
>> >> sense I suppose. Anyway, I will send separate tags for this series.

Re: [PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Vladimir Oltean]

On Fri, Mar 18, 2022 at 01:09:08PM +0100, Tobias Waldekranz wrote:
> >> > So have you seriously considered making the bridge ports that operate in
> >> > 'dumb hub' mode have a pvid which isn't installed as a 'self' entry on
> >> > the bridge device?
> >> 
> >> Just so there's no confusion, you mean something like...
> >> 
> >>     ip link add dev br0 type bridge vlan_filtering 1 vlan_default_pvid 0
> >> 
> >>     for p in swp0 swp1; do
> >>         ip link set dev $p master br0
> >>         bridge vlan add dev $p vid 1 pvid untagged
> >>     done
> >> 
> >> ... right?
> >> 
> >> In that case, the repeater is no longer transparent with respect to
> >> tagged packets, which the application requires.
> >
> > If you are sure that there exists one VLAN ID which is never used (like
> > 4094), what you could do is you could set the port pvids to that VID
> > instead of 1, and add the entire VLAN_N_VID range sans that VID in the
> > membership list of the two ports, as egress-tagged.
> 
> Yeah, I've thought about this too. If the device's only role is to act
> as a repeater, then you can get away with it. But you will have consumed
> all rows in the VTU and half of the rows in the ATU (we add an entry for
> the broadcast address in every FID). So if you want to use your other
> ports for regular bridging you're left with a very limited feature set.

But VLANs in other bridges would reuse the same FIDs, at least in the
current mv88e6xxx implementation with no FDB isolation, no? So even
though the VTU is maxed out, it wouldn't get 'more' maxed out.

As for the broadcast address needing to be present in the ATU, honestly
I don't know too much about that. I see that some switches have a
FloodBC bit, wouldn't that be useful?

> > This is 'practical transparency' - if true transparency is required then
> > yes, this doesn't work.
> >
> >> >> You might be tempted to solve this using flooding filters of the
> >> >> switch's CPU port, but these go out the window if you have another
> >> >> bridge configured, that requires that flooding of unknown traffic is
> >> >> enabled.
> >> >
> >> > Not if CPU flooding can be managed on a per-user-port basis.
> >> 
> >> True, but we aren't lucky enough to have hardware that can do that :)
> >> 
> >> >> Another application is to create a similar setup, but with three ports,
> >> >> and have the third one be used as a TAP.
> >> >
> >> > Could you expand more on this use case?
> >> 
> >> Its just the standard use-case for a TAP really. You have some link of
> >> interest that you want to snoop, but for some reason there is no way of
> >> getting a PCAP from the station on either side:
> >> 
> >>    Link of interest
> >>           |
> >> .-------. v .-------.
> >> | Alice +---+  Bob  |
> >> '-------'   '-------'
> >> 
> >> So you insert a hub in the middle, and listen on a third port:
> >> 
> >> .-------.   .-----.   .-------.
> >> | Alice +---+ TAP +---+  Bob  |
> >> '-------'   '--+--'   '-------'
> >>                |
> >>  PC running tcpdump/wireshark
> >> 
> >> The nice thing about being able to set this up in Linux is that if your
> >> hardware comes with a mix of media types, you can dynamically create the
> >> TAP for the job at hand. E.g. if Alice and Bob are communicating over a
> >> fiber, but your PC only has a copper interface, you can bridge to fiber
> >> ports with one copper; if you need to monitor a copper link 5min later,
> >> you just swap out the fiber ports for two copper ports.
> >> 
> >> >> >> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
> >> >> >
> >> >> > I don't believe this tag has much value since it was presumably carried 
> >> >> > over from an internal review. Might be worth adding it publicly now, though.
> >> >> 
> >> >> I think Mattias meant to replicate this tag on each individual
> >> >> patch. Aside from that though, are you saying that a tag is never valid
> >> >> unless there is a public message on the list from the signee? Makes
> >> >> sense I suppose. Anyway, I will send separate tags for this series.

Re: [PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Tobias Waldekranz]

On Fri, Mar 18, 2022 at 14:44, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Fri, Mar 18, 2022 at 01:09:08PM +0100, Tobias Waldekranz wrote:
>> >> > So have you seriously considered making the bridge ports that operate in
>> >> > 'dumb hub' mode have a pvid which isn't installed as a 'self' entry on
>> >> > the bridge device?
>> >> 
>> >> Just so there's no confusion, you mean something like...
>> >> 
>> >>     ip link add dev br0 type bridge vlan_filtering 1 vlan_default_pvid 0
>> >> 
>> >>     for p in swp0 swp1; do
>> >>         ip link set dev $p master br0
>> >>         bridge vlan add dev $p vid 1 pvid untagged
>> >>     done
>> >> 
>> >> ... right?
>> >> 
>> >> In that case, the repeater is no longer transparent with respect to
>> >> tagged packets, which the application requires.
>> >
>> > If you are sure that there exists one VLAN ID which is never used (like
>> > 4094), what you could do is you could set the port pvids to that VID
>> > instead of 1, and add the entire VLAN_N_VID range sans that VID in the
>> > membership list of the two ports, as egress-tagged.
>> 
>> Yeah, I've thought about this too. If the device's only role is to act
>> as a repeater, then you can get away with it. But you will have consumed
>> all rows in the VTU and half of the rows in the ATU (we add an entry for
>> the broadcast address in every FID). So if you want to use your other
>> ports for regular bridging you're left with a very limited feature set.
>
> But VLANs in other bridges would reuse the same FIDs, at least in the
> current mv88e6xxx implementation with no FDB isolation, no? So even
> though the VTU is maxed out, it wouldn't get 'more' maxed out.

I'm pretty sure that mv88e6xxx won't allow the same VID to be configured
on multiple bridges. A quick test seems to support that:

   root@coronet:~# ip link add dev br0 type bridge vlan_filtering 1
   root@coronet:~# ip link add dev br1 type bridge vlan_filtering 1
   root@coronet:~# ip link set dev br0 up
   root@coronet:~# ip link set dev br1 up
   root@coronet:~# ip link set dev swp1 master br0
   root@coronet:~# ip link set dev swp2 master br1
   RTNETLINK answers: Operation not supported

> As for the broadcast address needing to be present in the ATU, honestly
> I don't know too much about that. I see that some switches have a
> FloodBC bit, wouldn't that be useful?

mv88e6xxx can handle broadcast in two ways:

1. Always flood broadcast, independent of all other settings.

2. Treat broadcast as multicast, only allow flooding if unknown
   multicast is allowed on the port, or if there's an entry in the ATU
   (making it known) that allows it.

The kernel driver uses (2), because that is the only way (I know of)
that we can support the BCAST_FLOOD flag. In order to make BCAST_FLOOD
independent of MCAST_FLOOD, we have to load an entry allowing bc to
egress on all ports by default. De Morgan comes back to guide us once
more :)

>> > This is 'practical transparency' - if true transparency is required then
>> > yes, this doesn't work.
>> >
>> >> >> You might be tempted to solve this using flooding filters of the
>> >> >> switch's CPU port, but these go out the window if you have another
>> >> >> bridge configured, that requires that flooding of unknown traffic is
>> >> >> enabled.
>> >> >
>> >> > Not if CPU flooding can be managed on a per-user-port basis.
>> >> 
>> >> True, but we aren't lucky enough to have hardware that can do that :)
>> >> 
>> >> >> Another application is to create a similar setup, but with three ports,
>> >> >> and have the third one be used as a TAP.
>> >> >
>> >> > Could you expand more on this use case?
>> >> 
>> >> Its just the standard use-case for a TAP really. You have some link of
>> >> interest that you want to snoop, but for some reason there is no way of
>> >> getting a PCAP from the station on either side:
>> >> 
>> >>    Link of interest
>> >>           |
>> >> .-------. v .-------.
>> >> | Alice +---+  Bob  |
>> >> '-------'   '-------'
>> >> 
>> >> So you insert a hub in the middle, and listen on a third port:
>> >> 
>> >> .-------.   .-----.   .-------.
>> >> | Alice +---+ TAP +---+  Bob  |
>> >> '-------'   '--+--'   '-------'
>> >>                |
>> >>  PC running tcpdump/wireshark
>> >> 
>> >> The nice thing about being able to set this up in Linux is that if your
>> >> hardware comes with a mix of media types, you can dynamically create the
>> >> TAP for the job at hand. E.g. if Alice and Bob are communicating over a
>> >> fiber, but your PC only has a copper interface, you can bridge to fiber
>> >> ports with one copper; if you need to monitor a copper link 5min later,
>> >> you just swap out the fiber ports for two copper ports.
>> >> 
>> >> >> >> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
>> >> >> >
>> >> >> > I don't believe this tag has much value since it was presumably carried 
>> >> >> > over from an internal review. Might be worth adding it publicly now, though.
>> >> >> 
>> >> >> I think Mattias meant to replicate this tag on each individual
>> >> >> patch. Aside from that though, are you saying that a tag is never valid
>> >> >> unless there is a public message on the list from the signee? Makes
>> >> >> sense I suppose. Anyway, I will send separate tags for this series.

Re: [PATCH net-next 0/3] bridge: dsa: switchdev: mv88e6xxx: Implement local_receive bridge flag

[Vladimir Oltean]

On Fri, Mar 18, 2022 at 05:03:31PM +0100, Tobias Waldekranz wrote:
> On Fri, Mar 18, 2022 at 14:44, Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Fri, Mar 18, 2022 at 01:09:08PM +0100, Tobias Waldekranz wrote:
> >> >> > So have you seriously considered making the bridge ports that operate in
> >> >> > 'dumb hub' mode have a pvid which isn't installed as a 'self' entry on
> >> >> > the bridge device?
> >> >> 
> >> >> Just so there's no confusion, you mean something like...
> >> >> 
> >> >>     ip link add dev br0 type bridge vlan_filtering 1 vlan_default_pvid 0
> >> >> 
> >> >>     for p in swp0 swp1; do
> >> >>         ip link set dev $p master br0
> >> >>         bridge vlan add dev $p vid 1 pvid untagged
> >> >>     done
> >> >> 
> >> >> ... right?
> >> >> 
> >> >> In that case, the repeater is no longer transparent with respect to
> >> >> tagged packets, which the application requires.
> >> >
> >> > If you are sure that there exists one VLAN ID which is never used (like
> >> > 4094), what you could do is you could set the port pvids to that VID
> >> > instead of 1, and add the entire VLAN_N_VID range sans that VID in the
> >> > membership list of the two ports, as egress-tagged.
> >> 
> >> Yeah, I've thought about this too. If the device's only role is to act
> >> as a repeater, then you can get away with it. But you will have consumed
> >> all rows in the VTU and half of the rows in the ATU (we add an entry for
> >> the broadcast address in every FID). So if you want to use your other
> >> ports for regular bridging you're left with a very limited feature set.
> >
> > But VLANs in other bridges would reuse the same FIDs, at least in the
> > current mv88e6xxx implementation with no FDB isolation, no? So even
> > though the VTU is maxed out, it wouldn't get 'more' maxed out.
> 
> I'm pretty sure that mv88e6xxx won't allow the same VID to be configured
> on multiple bridges. A quick test seems to support that:
> 
>    root@coronet:~# ip link add dev br0 type bridge vlan_filtering 1
>    root@coronet:~# ip link add dev br1 type bridge vlan_filtering 1
>    root@coronet:~# ip link set dev br0 up
>    root@coronet:~# ip link set dev br1 up
>    root@coronet:~# ip link set dev swp1 master br0
>    root@coronet:~# ip link set dev swp2 master br1
>    RTNETLINK answers: Operation not supported

Ok, I forgot about mv88e6xxx_port_check_hw_vlan() even though I was
there on multiple occasions. Thanks for reminding me.

> > As for the broadcast address needing to be present in the ATU, honestly
> > I don't know too much about that. I see that some switches have a
> > FloodBC bit, wouldn't that be useful?
> 
> mv88e6xxx can handle broadcast in two ways:
> 
> 1. Always flood broadcast, independent of all other settings.
> 
> 2. Treat broadcast as multicast, only allow flooding if unknown
>    multicast is allowed on the port, or if there's an entry in the ATU
>    (making it known) that allows it.
> 
> The kernel driver uses (2), because that is the only way (I know of)
> that we can support the BCAST_FLOOD flag. In order to make BCAST_FLOOD
> independent of MCAST_FLOOD, we have to load an entry allowing bc to
> egress on all ports by default. De Morgan comes back to guide us once
> more :)

Ok, so this alternative falls flat on its face due to excessive resource
usage. Next...

Does your application require bridged foreign interfaces with the other
switch ports? In other words, is there a reason to keep the CPU port in
the flood domain of the switch, other than current software limitations?