[Bridge] [PATCH net-next 0/2] net/bridge: Support learning_sync on master

[Bridge] [PATCH net-next 0/2] net/bridge: Support learning_sync on master

[Alexandra Winter]

The following patches added support for 'learning_sync on/off self' to
the qeth device driver:
521c65b64916 s390/qeth: implement ndo_bridge_setlink for learning_sync
780b6e7db25e s390/qeth: implement ndo_bridge_getlink for learning_sync
10a6cfc0fc82 s390/qeth: Translate address events into switchdev notifiers
The 'learning_sync self' attribute is used to enable syncing from the
HW fdb to the software bridge's fdb via SWITCHDEV_FDB_ADD/DEL_TO_BRIDGE
notifications.

The current patchset now adds support for 'learning_sync on/off [master]'
to synchronize from the software bridge's fdb to a hardware fdb.

Alexandra Winter (2):
  net/bridge: Support learning_sync on master
  net/bridge: Update uc addr on LEARNING_SYNC bp

 include/uapi/linux/if_link.h |  2 +-
 net/bridge/br_fdb.c          | 28 ++++++++++++++++++++++++++++
 net/bridge/br_netlink.c      |  5 +++++
 3 files changed, 34 insertions(+), 1 deletion(-)

-- 
2.25.1

[Bridge] [PATCH net-next 1/2] net/bridge: Support learning_sync on master

[Alexandra Winter]

Add support to set and get the 'learning_sync [master]' attribute of a
bridgeport. A following patch adds support to synchronize the software
bridge's fdb changes to the hardware fdb of this bridgeport.

Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
---
 include/uapi/linux/if_link.h | 2 +-
 net/bridge/br_netlink.c      | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index cd5b382a4138..4d8e4c9b803c 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -499,7 +499,7 @@ enum {
 	IFLA_BRPORT_LEARNING,	/* mac learning */
 	IFLA_BRPORT_UNICAST_FLOOD, /* flood unicast traffic */
 	IFLA_BRPORT_PROXYARP,	/* proxy ARP */
-	IFLA_BRPORT_LEARNING_SYNC, /* mac learning sync from device */
+	IFLA_BRPORT_LEARNING_SYNC, /* mac learning sync from/to device */
 	IFLA_BRPORT_PROXYARP_WIFI, /* proxy ARP for Wi-Fi */
 	IFLA_BRPORT_ROOT_ID,	/* designated root */
 	IFLA_BRPORT_BRIDGE_ID,	/* designated bridge */
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index e4e6e991313e..d91a5a319a4b 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -180,6 +180,7 @@ static inline size_t br_port_info_size(void)
 		+ nla_total_size(1)	/* IFLA_BRPORT_MCAST_FLOOD */
 		+ nla_total_size(1)	/* IFLA_BRPORT_BCAST_FLOOD */
 		+ nla_total_size(1)	/* IFLA_BRPORT_PROXYARP */
+		+ nla_total_size(1)	/* IFLA_BRPORT_LEARNING_SYNC */
 		+ nla_total_size(1)	/* IFLA_BRPORT_PROXYARP_WIFI */
 		+ nla_total_size(1)	/* IFLA_BRPORT_VLAN_TUNNEL */
 		+ nla_total_size(1)	/* IFLA_BRPORT_NEIGH_SUPPRESS */
@@ -247,6 +248,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
 	    nla_put_u8(skb, IFLA_BRPORT_BCAST_FLOOD,
 		       !!(p->flags & BR_BCAST_FLOOD)) ||
 	    nla_put_u8(skb, IFLA_BRPORT_PROXYARP, !!(p->flags & BR_PROXYARP)) ||
+	    nla_put_u8(skb, IFLA_BRPORT_LEARNING_SYNC,
+		       !!(p->flags & BR_LEARNING_SYNC)) ||
 	    nla_put_u8(skb, IFLA_BRPORT_PROXYARP_WIFI,
 		       !!(p->flags & BR_PROXYARP_WIFI)) ||
 	    nla_put(skb, IFLA_BRPORT_ROOT_ID, sizeof(struct ifla_bridge_id),
@@ -818,6 +821,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
 	[IFLA_BRPORT_LEARNING]	= { .type = NLA_U8 },
 	[IFLA_BRPORT_UNICAST_FLOOD] = { .type = NLA_U8 },
 	[IFLA_BRPORT_PROXYARP]	= { .type = NLA_U8 },
+	[IFLA_BRPORT_LEARNING_SYNC] = { .type = NLA_U8 },
 	[IFLA_BRPORT_PROXYARP_WIFI] = { .type = NLA_U8 },
 	[IFLA_BRPORT_MULTICAST_ROUTER] = { .type = NLA_U8 },
 	[IFLA_BRPORT_MCAST_TO_UCAST] = { .type = NLA_U8 },
@@ -889,6 +893,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
 			 BR_MULTICAST_TO_UNICAST);
 	br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD);
 	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
+	br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING_SYNC, BR_LEARNING_SYNC);
 	br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
 	br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
 	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
-- 
2.25.1

[Bridge] [PATCH net-next 2/2] net/bridge: Update uc addr on LEARNING_SYNC bp

[Alexandra Winter]

Whenever a unicast fdb entry is added or deleted in the software
bridge's fdb, synchronize it to the hardware fdb of a bridgeport
device, if the bridgeport has the attribute LEARNING_SYNC and is not
isolated from the target of the changed fdb entry.

To inform HW, that messages with a specific unicast target address
should be sent to the software bridge via this bridgeport, simply
register this address with the device.

Without this patch smart NICs attached to a bridgeport of a software
bridge can already do their own learning on the messages that the
SW bridge sends out via this port. And otherwise accept/flood all
unknown target messages to the SW bridge (promiscuous port).
This patch gives the attached HW the chance to update its fdb, even
when it does not see the respective message, because it is forwarded
to another piece of HW attached to another bridgeport. Or when the NIC
is not capable of learning or flooding.

An alternative solution would be to subscribe to the
SWITCHDEV_FDB_ADD/DEL_TO_DEVICE switchdev notifiers in the respective
device drivers. But as there's no HW-specific part in this
implementation, it was felt that this should rather be implemented in
the common layer of the bridge code.

Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
---
 net/bridge/br_fdb.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 698b79747d32..2075b5da6db3 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -567,6 +567,32 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 	return ret;
 }
 
+static void br_fdb_learning_sync(struct net_bridge *br,
+				 const struct net_bridge_fdb_entry *fdb,
+				 int type)
+{
+	struct net_bridge_port *p;
+
+	if (!fdb->dst)
+		return;
+	list_for_each_entry(p, &br->port_list, list) {
+		if ((p->flags & BR_LEARNING_SYNC) && p != fdb->dst &&
+		    (!(p->flags & BR_ISOLATED) ||
+		     !(fdb->dst->flags & BR_ISOLATED))) {
+			switch (type) {
+			case RTM_DELNEIGH:
+				dev_uc_del(p->dev, fdb->key.addr.addr);
+				break;
+			case RTM_NEWNEIGH:
+				dev_uc_add(p->dev, fdb->key.addr.addr);
+				break;
+			default:
+				break;
+			}
+		}
+	}
+}
+
 /* returns true if the fdb was modified */
 static bool __fdb_mark_active(struct net_bridge_fdb_entry *fdb)
 {
@@ -603,6 +629,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 			if (unlikely(source != fdb->dst &&
 				     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
 				br_switchdev_fdb_notify(fdb, RTM_DELNEIGH);
+				br_fdb_learning_sync(br, fdb, RTM_DELNEIGH);
 				fdb->dst = source;
 				fdb_modified = true;
 				/* Take over HW learned entry */
@@ -799,6 +826,7 @@ static void fdb_notify(struct net_bridge *br,
 		goto errout;
 	}
 	rtnl_notify(skb, net, 0, RTNLGRP_NEIGH, NULL, GFP_ATOMIC);
+	br_fdb_learning_sync(br, fdb, type);
 	return;
 errout:
 	rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
-- 
2.25.1

Re: [Bridge] [PATCH net-next 2/2] net/bridge: Update uc addr on LEARNING_SYNC bp

[Nikolay Aleksandrov]

On 23/06/2021 16:34, Alexandra Winter wrote:
> Whenever a unicast fdb entry is added or deleted in the software
> bridge's fdb, synchronize it to the hardware fdb of a bridgeport
> device, if the bridgeport has the attribute LEARNING_SYNC and is not
> isolated from the target of the changed fdb entry.
> 
> To inform HW, that messages with a specific unicast target address
> should be sent to the software bridge via this bridgeport, simply
> register this address with the device.
> 
> Without this patch smart NICs attached to a bridgeport of a software
> bridge can already do their own learning on the messages that the
> SW bridge sends out via this port. And otherwise accept/flood all
> unknown target messages to the SW bridge (promiscuous port).
> This patch gives the attached HW the chance to update its fdb, even
> when it does not see the respective message, because it is forwarded
> to another piece of HW attached to another bridgeport. Or when the NIC
> is not capable of learning or flooding.
> 
> An alternative solution would be to subscribe to the
> SWITCHDEV_FDB_ADD/DEL_TO_DEVICE switchdev notifiers in the respective
> device drivers. But as there's no HW-specific part in this
> implementation, it was felt that this should rather be implemented in
> the common layer of the bridge code.
> 
> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
> ---
>  net/bridge/br_fdb.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 

Absolutely no way, I'm sorry but br_fdb_update() is called when learning on every packet,
walking the port list on every fdb port change is a disastrous overkill.
You already have specified the alternative yourself, please use the switchdev notifiers
that are there and already take up the necessary processing in the fast paths.

Nacked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 698b79747d32..2075b5da6db3 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -567,6 +567,32 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  	return ret;
>  }
>  
> +static void br_fdb_learning_sync(struct net_bridge *br,
> +				 const struct net_bridge_fdb_entry *fdb,
> +				 int type)
> +{
> +	struct net_bridge_port *p;
> +
> +	if (!fdb->dst)
> +		return;
> +	list_for_each_entry(p, &br->port_list, list) {

You can't just walk the port list without any synchronization.

> +		if ((p->flags & BR_LEARNING_SYNC) && p != fdb->dst &&
> +		    (!(p->flags & BR_ISOLATED) ||

These flags can change while running...

> +		     !(fdb->dst->flags & BR_ISOLATED))) {
> +			switch (type) {
> +			case RTM_DELNEIGH:
> +				dev_uc_del(p->dev, fdb->key.addr.addr);
> +				break;
> +			case RTM_NEWNEIGH:
> +				dev_uc_add(p->dev, fdb->key.addr.addr);
> +				break;

... and you can end up having programmed a lot of dynamic fdbs that will never get removed.

> +			default:
> +				break;
> +			}
> +		}
> +	}
> +}
> +
>  /* returns true if the fdb was modified */
>  static bool __fdb_mark_active(struct net_bridge_fdb_entry *fdb)
>  {
> @@ -603,6 +629,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>  			if (unlikely(source != fdb->dst &&
>  				     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
>  				br_switchdev_fdb_notify(fdb, RTM_DELNEIGH);
> +				br_fdb_learning_sync(br, fdb, RTM_DELNEIGH);
>  				fdb->dst = source;
>  				fdb_modified = true;
>  				/* Take over HW learned entry */
> @@ -799,6 +826,7 @@ static void fdb_notify(struct net_bridge *br,
>  		goto errout;
>  	}
>  	rtnl_notify(skb, net, 0, RTNLGRP_NEIGH, NULL, GFP_ATOMIC);
> +	br_fdb_learning_sync(br, fdb, type);
>  	return;
>  errout:
>  	rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
>