[PATCH net-next 00/12] net: bridge: vlan: optimize standard fdb fwding path

[PATCH net-next 00/12] net: bridge: vlan: optimize standard fdb fwding path

[Nikolay Aleksandrov]

Hi,
This patch-set is a follow-up after the bridge flood fwding path
optimizations and applies the same idea to the standard fdb fwding path.
We are able to remove 2 vlan hash lookups in the standard fdb fwding
path by caching the port-VLAN pointer in the fdb, to do that we switch the
fdb dst to an opaque type that can be either a port pointer or vlan pointer
differentiated by a bit. The field is a struct and also has a __private tag
so we can catch direct users, it should be used only via the helpers.
Sharing a field makes it easier to pass it around and also allows us to
save struct space for the fast-path. Interesting case is when an fdb is
promoted from a raw port to port-VLAN on the same port, it needs to be
handled carefully so we use cmpxchg to make sure we don't generate
deletion/replace notifications because it doesn't change the port.
I tested VLAN deletion after these changes (we now wait a grace period for
every delete) and the hit was ~20% reduction in deleted VLANs / sec
deleting 4k VLANs took 14ms more and on my VM the VLANs deleted / sec went
from 63k to 52k / sec. The complexity to batch them is not worth it.

Note again internal sashiko finds some pre-existing issues - I will take
care of those separately as usual.

Overall the improvement is +10% Mpps and -10% cycles spent in fdb fwding:
 64 byte vlan packets, 4k randomized fdb hits with 4k fdbs, 5 million
 packets passed 5 times for every case
     VIDs  Ports       Mpps                 Gbps             CPU1 cycles/input
                  before   after     gain    before  after   before  after  reduction
       64    8/2  2.375486 2.635811  11.0%   1.216   1.350   1724.6  1553.7    9.9%
       64    8/4  2.376731 2.636459  10.9%   1.217   1.350   1714.8  1554.7    9.3%
       64    8/8  2.376621 2.635783  10.9%   1.217   1.350   1722.6  1553.2    9.8%
       64   32/2  2.375876 2.637897  11.0%   1.216   1.351   1726.7  1552.3   10.1%
       64  32/16  2.376442 2.636426  10.9%   1.217   1.350   1721.3  1554.2    9.7%
       64  32/32  2.376763 2.636268  10.9%   1.217   1.350   1726.8  1553.5   10.0%
       64   64/2  2.375678 2.638707  11.1%   1.216   1.351   1725.0  1552.1   10.0%
       64  64/32  2.376681 2.637359  11.0%   1.217   1.350   1725.1  1555.9    9.8%
       64  64/64  2.376787 2.638396  11.0%   1.217   1.351   1722.8  1550.9   10.0%
     1024    8/2  2.263768 2.498881  10.4%   1.159   1.279   1808.5  1637.1    9.5%
     1024    8/4  2.264519 2.499075  10.4%   1.159   1.280   1807.6  1638.8    9.3%
     1024    8/8  2.263551 2.499231  10.4%   1.159   1.280   1812.0  1640.2    9.5%
     1024   32/2  2.263841 2.499120  10.4%   1.159   1.280   1806.8  1639.6    9.3%
     1024  32/16  2.263990 2.499562  10.4%   1.159   1.280   1806.1  1639.2    9.2%
     1024  32/32  2.263719 2.498905  10.4%   1.159   1.279   1807.7  1639.3    9.3%
     1024   64/2  2.263635 2.501889  10.5%   1.159   1.281   1809.4  1635.6    9.6%
     1024  64/32  2.264467 2.500359  10.4%   1.159   1.280   1806.5  1633.0    9.6%
     1024  64/64  2.240494 2.470246  10.3%   1.147   1.265   1810.0  1637.1    9.6%

This information has been also included in the commit that removes the
lookups.

Quick patch overview:
Patch 01 - adds the new opaque destination type with its basic helpers
Patch 02 - uses the new dsts for standard port fdb destinations
Patch 03 - adds port-VLAN pointer support to the destination type
Patch 04 - changes ingress helpers so we can get rid of the vid argument
           and pass a vlan pointer around
Patch 05 - passes VLAN pointers instead of vid to br_fdb_update
Patch 06 - consolidates vlan fdb cleanups, that will allow us to clean
           entries in one pass later on
Patch 07 - splits vlan unpublishing from hash and flood array from deletion
           that will be needed to optimize bulk deletes and flushes
Patch 08 - makes sure that readers cannot publish the vlan dst before
           cleaning up all fdbs that use it
Patch 09 - factors out the fdb update path to prepare it for port-VLAN dsts
Patch 10 - the first port-VLAN publishing, fdbs can now have port-VLAN dsts
Patch 11 - port-VLAN dst publishing for configured entries
Patch 12 - remove 2 vlan hash lookups in fdb fwding fast-path

For sashiko:
 [Severity: High]
 Could nbp_vlan_delete() in net/bridge/br_vlan.c avoid waiting for a network
 RCU grace period once per VLAN while RTNL is held?

 Nik: Yes, it could but the complexity this brings is not worth it.
      See above.

 [Severity: High]
 Could this synchronize_net() in
 net/bridge/br_vlan.c:nbp_vlan_delete() be batched by its callers?

 Nik: Yes, it could but again same thing - it is not worth the complexity
      and deleting VLANs is still fast enough.

Thanks,
 Nik


Nikolay Aleksandrov (12):
  net: bridge: introduce a bridge destination type
  net: bridge: use net_bridge_dst for fdb destinations
  net: bridge: add VLAN support to bridge destinations
  net: bridge: vlan: return VLAN entries from ingress helpers
  net: bridge: fdb: pass VLAN entries to learning updates
  net: bridge: fdb: consolidate port-VLAN cleanup
  net: bridge: vlan: split unpublishing from deletion
  net: bridge: vlan: quiesce readers before freeing port VLANs
  net: bridge: fdb: factor out existing entry updates
  net: bridge: fdb: cache port VLANs in learned entries
  net: bridge: fdb: cache VLAN destinations in configured entries
  net: bridge: fdb: avoid VLAN lookups in unicast forwarding

 include/trace/events/bridge.h            |   7 +-
 net/bridge/br.c                          |   2 +-
 net/bridge/br_arp_nd_proxy.c             |   4 +-
 net/bridge/br_device.c                   |   9 +-
 net/bridge/br_fdb.c                      | 270 +++++++++++++++--------
 net/bridge/br_forward.c                  |  24 +-
 net/bridge/br_if.c                       |  13 +-
 net/bridge/br_input.c                    |  26 ++-
 net/bridge/br_mrp.c                      |   6 +-
 net/bridge/br_netlink.c                  |   2 +-
 net/bridge/br_private.h                  | 157 ++++++++++++-
 net/bridge/br_stp_if.c                   |   2 +-
 net/bridge/br_switchdev.c                |   2 +-
 net/bridge/br_sysfs_if.c                 |   3 +-
 net/bridge/br_vlan.c                     | 107 +++++----
 net/bridge/netfilter/nft_reject_bridge.c |   8 +-
 16 files changed, 448 insertions(+), 194 deletions(-)

-- 
2.47.3

[PATCH net-next 01/12] net: bridge: introduce a bridge destination type

[Nikolay Aleksandrov]

Add an opaque destination type and helpers for representing bridge port
destinations. Using a separate structure makes raw pointer assignments and
comparisons fail at build time while marking its value member __private
makes sparse warn about accesses that bypass the helpers.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_private.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 67117fb3dc88..1146187aa2ba 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -306,6 +306,10 @@ struct net_bridge_fdb_key {
 	u16 vlan_id;
 };
 
+struct net_bridge_dst {
+	unsigned long __private value;
+};
+
 struct net_bridge_fdb_entry {
 	struct rhash_head		rhnode;
 	struct net_bridge_port		*dst;
@@ -670,6 +674,40 @@ struct br_input_skb_cb {
 #define br_debug(br, format, args...)			\
 	pr_debug("%s: " format,  (br)->dev->name, ##args)
 
+static inline struct net_bridge_dst
+br_dst_read(const struct net_bridge_dst *src)
+{
+	struct net_bridge_dst dst;
+
+	ACCESS_PRIVATE(&dst, value) =
+		READ_ONCE(ACCESS_PRIVATE(src, value));
+
+	return dst;
+}
+
+static inline void br_dst_write(struct net_bridge_dst *dst,
+				struct net_bridge_dst src)
+{
+	WRITE_ONCE(ACCESS_PRIVATE(dst, value),
+		   ACCESS_PRIVATE(&src, value));
+}
+
+static inline struct net_bridge_dst
+br_port_to_dst(const struct net_bridge_port *p)
+{
+	struct net_bridge_dst dst;
+
+	ACCESS_PRIVATE(&dst, value) = (unsigned long)p;
+
+	return dst;
+}
+
+static inline struct net_bridge_port *
+br_dst_port(struct net_bridge_dst dst)
+{
+	return (struct net_bridge_port *)ACCESS_PRIVATE(&dst, value);
+}
+
 /* called under bridge lock */
 static inline int br_is_root_bridge(const struct net_bridge *br)
 {
-- 
2.47.3

[PATCH net-next 02/12] net: bridge: use net_bridge_dst for fdb destinations

[Nikolay Aleksandrov]

Store fdb destinations in struct net_bridge_dst and access them through
the destination helpers. This doesn't change their representation or
behavior, fdb destinations still contain only bridge port pointers.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Masami Hiramatsu <mhiramat@kernel.org>
CC: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

 include/trace/events/bridge.h |  7 +++--
 net/bridge/br_arp_nd_proxy.c  |  4 +--
 net/bridge/br_device.c        |  4 +--
 net/bridge/br_fdb.c           | 53 ++++++++++++++++++-----------------
 net/bridge/br_input.c         |  4 +--
 net/bridge/br_private.h       | 20 ++++++++++++-
 net/bridge/br_switchdev.c     |  2 +-
 7 files changed, 58 insertions(+), 36 deletions(-)

diff --git a/include/trace/events/bridge.h b/include/trace/events/bridge.h
index 3fe4725c83ff..f676516b33fa 100644
--- a/include/trace/events/bridge.h
+++ b/include/trace/events/bridge.h
@@ -68,13 +68,14 @@ TRACE_EVENT(br_fdb_external_learn_add,
 
 TRACE_EVENT(fdb_delete,
 
-	TP_PROTO(struct net_bridge *br, struct net_bridge_fdb_entry *f),
+	TP_PROTO(struct net_bridge *br, struct net_bridge_fdb_entry *f,
+		 const struct net_bridge_port *dst),
 
-	TP_ARGS(br, f),
+	TP_ARGS(br, f, dst),
 
 	TP_STRUCT__entry(
 		__string(br_dev, br->dev->name)
-		__string(dev, f->dst ? f->dst->dev->name : "null")
+		__string(dev, dst ? dst->dev->name : "null")
 		__array(unsigned char, addr, ETH_ALEN)
 		__field(u16, vid)
 	),
diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index da15f4d7c1ae..ba4a63840b21 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -223,7 +223,7 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 		neigh_ha_snapshot(ha, n, n->dev);
 		f = br_fdb_find_rcu(br, ha, vid);
 		if (f) {
-			const struct net_bridge_port *dst = READ_ONCE(f->dst);
+			const struct net_bridge_port *dst = br_fdb_dst_port(f);
 			bool replied = false;
 
 			if ((p && test_bit(BR_PROXYARP_BIT, &p->flags)) ||
@@ -500,7 +500,7 @@ void br_do_suppress_nd(struct sk_buff *skb, struct net_bridge *br,
 		neigh_ha_snapshot(ha, n, n->dev);
 		f = br_fdb_find_rcu(br, ha, vid);
 		if (f) {
-			const struct net_bridge_port *dst = READ_ONCE(f->dst);
+			const struct net_bridge_port *dst = br_fdb_dst_port(f);
 			bool replied = false;
 
 			if (br_is_neigh_suppress_enabled_vid(dst, vid)) {
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index ce9ea9ac3d0a..c6804409b60b 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -107,7 +107,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		else
 			br_flood(br, vlan, skb, BR_PKT_MULTICAST, false, true);
 	} else if ((dst = br_fdb_find_rcu(br, dest, vid)) != NULL) {
-		br_forward(READ_ONCE(dst->dst), skb, false, true);
+		br_forward(br_fdb_dst_port(dst), skb, false, true);
 	} else {
 		br_flood(br, vlan, skb, BR_PKT_UNICAST, false, true);
 	}
@@ -400,7 +400,7 @@ static int br_fill_forward_path(struct net_device_path_ctx *ctx,
 	if (!f)
 		return -1;
 
-	dst = READ_ONCE(f->dst);
+	dst = br_fdb_dst_port(f);
 	if (!dst)
 		return -1;
 
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e4570bbed854..0f5cf5615b56 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -90,7 +90,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
 			 const struct net_bridge_fdb_entry *fdb,
 			 u32 portid, u32 seq, int type, unsigned int flags)
 {
-	const struct net_bridge_port *dst = READ_ONCE(fdb->dst);
+	const struct net_bridge_port *dst = br_fdb_dst_port(fdb);
 	unsigned long now = jiffies;
 	struct nda_cacheinfo ci;
 	struct nlmsghdr *nlh;
@@ -250,7 +250,7 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev,
 	rcu_read_lock();
 	f = br_fdb_find_rcu(br, addr, vid);
 	if (f) {
-		dst = READ_ONCE(f->dst);
+		dst = br_fdb_dst_port(f);
 		if (dst)
 			dev = dst->dev;
 	}
@@ -315,7 +315,7 @@ static void fdb_del_hw_addr(struct net_bridge *br, const unsigned char *addr)
 static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
 		       bool swdev_notify)
 {
-	trace_fdb_delete(br, f);
+	trace_fdb_delete(br, f, br_fdb_dst_port(f));
 
 	if (test_bit(BR_FDB_STATIC, &f->flags))
 		fdb_del_hw_addr(br, f->key.addr.addr);
@@ -350,7 +350,7 @@ static void fdb_delete_local(struct net_bridge *br,
 		vg = nbp_vlan_group(op);
 		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
 		    (!vid || br_vlan_find(vg, vid))) {
-			WRITE_ONCE(f->dst, op);
+			br_fdb_dst_write(f, br_port_to_dst(op));
 			clear_bit(BR_FDB_ADDED_BY_USER, &f->flags);
 			return;
 		}
@@ -361,7 +361,7 @@ static void fdb_delete_local(struct net_bridge *br,
 	/* Maybe bridge device has same hw addr? */
 	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
 	    (!vid || (v && br_vlan_should_use(v)))) {
-		WRITE_ONCE(f->dst, NULL);
+		br_fdb_dst_write(f, br_port_to_dst(NULL));
 		clear_bit(BR_FDB_ADDED_BY_USER, &f->flags);
 		return;
 	}
@@ -378,7 +378,8 @@ void br_fdb_find_delete_local(struct net_bridge *br,
 	spin_lock_bh(&br->hash_lock);
 	f = br_fdb_find(br, addr, vid);
 	if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
-	    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags) && f->dst == p)
+	    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags) &&
+	    br_fdb_dst_port(f) == p)
 		fdb_delete_local(br, p, f);
 	spin_unlock_bh(&br->hash_lock);
 }
@@ -408,7 +409,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
 		return NULL;
 
 	memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
-	WRITE_ONCE(fdb->dst, source);
+	br_fdb_dst_write(fdb, br_port_to_dst(source));
 	fdb->key.vlan_id = vid;
 	fdb->flags = flags;
 	fdb->updated = fdb->used = jiffies;
@@ -470,7 +471,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 	spin_lock_bh(&br->hash_lock);
 	vg = nbp_vlan_group(p);
 	hlist_for_each_entry(f, &br->fdb_list, fdb_node) {
-		if (READ_ONCE(f->dst) == p &&
+		if (br_fdb_dst_port(f) == p &&
 		    test_bit(BR_FDB_LOCAL, &f->flags) &&
 		    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags)) {
 			/* delete old one */
@@ -517,7 +518,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 	/* If old entry was unassociated with any port, then delete it. */
 	f = br_fdb_find(br, br->dev->dev_addr, 0);
 	if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
-	    !f->dst && !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
+	    !br_fdb_dst_port(f) &&
+	    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
 		fdb_delete_local(br, NULL, f);
 
 	fdb_add_local(br, NULL, newaddr, 0);
@@ -533,7 +535,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 			continue;
 		f = br_fdb_find(br, br->dev->dev_addr, v->vid);
 		if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
-		    !f->dst && !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
+		    !br_fdb_dst_port(f) &&
+		    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
 			fdb_delete_local(br, NULL, f);
 		fdb_add_local(br, NULL, newaddr, v->vid);
 	}
@@ -693,7 +696,7 @@ static bool __fdb_flush_matches(const struct net_bridge *br,
 				const struct net_bridge_fdb_entry *f,
 				const struct net_bridge_fdb_flush_desc *desc)
 {
-	const struct net_bridge_port *dst = READ_ONCE(f->dst);
+	const struct net_bridge_port *dst = br_fdb_dst_port(f);
 	int port_ifidx = dst ? dst->dev->ifindex : br->dev->ifindex;
 
 	if (desc->vlan_id && desc->vlan_id != f->key.vlan_id)
@@ -879,7 +882,7 @@ void br_fdb_delete_by_port(struct net_bridge *br,
 
 	spin_lock_bh(&br->hash_lock);
 	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
-		if (READ_ONCE(f->dst) != p)
+		if (br_fdb_dst_port(f) != p)
 			continue;
 
 		if (!do_all)
@@ -921,7 +924,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
 			continue;
 
 		/* ignore pseudo entry for local MAC address */
-		dst = READ_ONCE(f->dst);
+		dst = br_fdb_dst_port(f);
 		if (!dst)
 			continue;
 
@@ -996,10 +999,10 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 			}
 
 			/* fastpath: update of existing entry */
-			if (unlikely(source != READ_ONCE(fdb->dst) &&
+			if (unlikely(source != br_fdb_dst_port(fdb) &&
 				     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
 				br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
-				WRITE_ONCE(fdb->dst, source);
+				br_fdb_dst_write(fdb, br_port_to_dst(source));
 				fdb_modified = true;
 				/* Take over HW learned entry */
 				if (unlikely(test_bit(BR_FDB_ADDED_BY_EXT_LEARN,
@@ -1061,17 +1064,17 @@ int br_fdb_dump(struct sk_buff *skb,
 
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
-		const struct net_bridge_port *dst = READ_ONCE(f->dst);
+		const struct net_bridge_port *dst = br_fdb_dst_port(f);
 
 		if (*idx < ctx->fdb_idx)
 			goto skip;
 		if (filter_dev && (!dst || dst->dev != filter_dev)) {
 			if (filter_dev != dev)
 				goto skip;
-			/* !f->dst is a special case for bridge
+			/* A NULL destination is a special case for bridge
 			 * It means the MAC belongs to the bridge
 			 * Therefore need a little more filtering
-			 * we only want to dump the !f->dst case
+			 * we only want to dump the NULL destination case
 			 */
 			if (dst)
 				goto skip;
@@ -1193,8 +1196,8 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 		if (flags & NLM_F_EXCL)
 			return -EEXIST;
 
-		if (READ_ONCE(fdb->dst) != source) {
-			WRITE_ONCE(fdb->dst, source);
+		if (br_fdb_dst_port(fdb) != source) {
+			br_fdb_dst_write(fdb, br_port_to_dst(source));
 			modified = true;
 		}
 
@@ -1385,7 +1388,7 @@ static int fdb_delete_by_addr_and_port(struct net_bridge *br,
 	struct net_bridge_fdb_entry *fdb;
 
 	fdb = br_fdb_find(br, addr, vlan);
-	if (!fdb || READ_ONCE(fdb->dst) != p)
+	if (!fdb || br_fdb_dst_port(fdb) != p)
 		return -ENOENT;
 
 	fdb_delete(br, fdb, true);
@@ -1541,15 +1544,15 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 	} else {
 		if (locked &&
 		    (!test_bit(BR_FDB_LOCKED, &fdb->flags) ||
-		     READ_ONCE(fdb->dst) != p)) {
+		     br_fdb_dst_port(fdb) != p)) {
 			err = -EINVAL;
 			goto err_unlock;
 		}
 
 		WRITE_ONCE(fdb->updated, jiffies);
 
-		if (READ_ONCE(fdb->dst) != p) {
-			WRITE_ONCE(fdb->dst, p);
+		if (br_fdb_dst_port(fdb) != p) {
+			br_fdb_dst_write(fdb, br_port_to_dst(p));
 			modified = true;
 		}
 
@@ -1632,7 +1635,7 @@ void br_fdb_clear_offload(const struct net_device *dev, u16 vid)
 
 	spin_lock_bh(&p->br->hash_lock);
 	hlist_for_each_entry(f, &p->br->fdb_list, fdb_node) {
-		if (READ_ONCE(f->dst) == p && f->key.vlan_id == vid)
+		if (br_fdb_dst_port(f) == p && f->key.vlan_id == vid)
 			clear_bit(BR_FDB_OFFLOADED, &f->flags);
 	}
 	spin_unlock_bh(&p->br->hash_lock);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 4357d78524a6..68aa8fa52eba 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -123,7 +123,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 				br_fdb_update(br, p, eth_hdr(skb)->h_source,
 					      vid, BIT(BR_FDB_LOCKED));
 			goto drop;
-		} else if (READ_ONCE(fdb_src->dst) != p ||
+		} else if (br_fdb_dst_port(fdb_src) != p ||
 			   test_bit(BR_FDB_LOCAL, &fdb_src->flags)) {
 			/* FDB mismatch. Drop the packet without roaming. */
 			goto drop;
@@ -223,7 +223,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 
 		if (now != READ_ONCE(dst->used))
 			WRITE_ONCE(dst->used, now);
-		br_forward(READ_ONCE(dst->dst), skb, local_rcv, false);
+		br_forward(br_fdb_dst_port(dst), skb, local_rcv, false);
 	} else {
 		if (!mcast_hit)
 			br_flood(br, vlan, skb, pkt_type, local_rcv, false);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1146187aa2ba..bbb59b53e745 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -312,7 +312,7 @@ struct net_bridge_dst {
 
 struct net_bridge_fdb_entry {
 	struct rhash_head		rhnode;
-	struct net_bridge_port		*dst;
+	struct net_bridge_dst		dst;
 
 	struct net_bridge_fdb_key	key;
 	struct hlist_node		fdb_node;
@@ -708,6 +708,24 @@ br_dst_port(struct net_bridge_dst dst)
 	return (struct net_bridge_port *)ACCESS_PRIVATE(&dst, value);
 }
 
+static inline struct net_bridge_dst
+br_fdb_dst_read(const struct net_bridge_fdb_entry *fdb)
+{
+	return br_dst_read(&fdb->dst);
+}
+
+static inline void br_fdb_dst_write(struct net_bridge_fdb_entry *fdb,
+				    struct net_bridge_dst dst)
+{
+	br_dst_write(&fdb->dst, dst);
+}
+
+static inline struct net_bridge_port *
+br_fdb_dst_port(const struct net_bridge_fdb_entry *fdb)
+{
+	return br_dst_port(br_fdb_dst_read(fdb));
+}
+
 /* called under bridge lock */
 static inline int br_is_root_bridge(const struct net_bridge *br)
 {
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 990c6b38fd39..10f3aea6a1f8 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -129,7 +129,7 @@ static void br_switchdev_fdb_populate(struct net_bridge *br,
 				      const struct net_bridge_fdb_entry *fdb,
 				      const void *ctx)
 {
-	const struct net_bridge_port *p = READ_ONCE(fdb->dst);
+	const struct net_bridge_port *p = br_fdb_dst_port(fdb);
 
 	item->addr = fdb->key.addr.addr;
 	item->vid = fdb->key.vlan_id;
-- 
2.47.3

[PATCH net-next 03/12] net: bridge: add VLAN support to bridge destinations

[Nikolay Aleksandrov]

Use bit 0 to distinguish port-VLAN pointers from bridge port pointers in
struct net_bridge_dst. Add helpers for constructing and decoding VLAN
destinations. No caller creates one yet, so behavior remains unchanged.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_private.h | 50 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index bbb59b53e745..2bf7f4429a3b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -306,6 +306,8 @@ struct net_bridge_fdb_key {
 	u16 vlan_id;
 };
 
+#define BR_DST_VLAN_TAG	BIT(0)
+
 struct net_bridge_dst {
 	unsigned long __private value;
 };
@@ -702,10 +704,56 @@ br_port_to_dst(const struct net_bridge_port *p)
 	return dst;
 }
 
+static inline struct net_bridge_dst
+br_vlan_to_dst(const struct net_bridge_vlan *v)
+{
+	struct net_bridge_dst dst;
+
+	ACCESS_PRIVATE(&dst, value) = (unsigned long)v | BR_DST_VLAN_TAG;
+
+	return dst;
+}
+
+static inline void br_dst_decode(struct net_bridge_dst dst,
+				 struct net_bridge_port **port,
+				 struct net_bridge_vlan **vlan)
+{
+	struct net_bridge_vlan *v;
+	unsigned long value;
+
+	value = ACCESS_PRIVATE(&dst, value);
+	if (!(value & BR_DST_VLAN_TAG)) {
+		*port = (struct net_bridge_port *)value;
+		*vlan = NULL;
+		return;
+	}
+
+	v = (struct net_bridge_vlan *)(value & ~BR_DST_VLAN_TAG);
+	*port = v->port;
+	*vlan = v;
+}
+
 static inline struct net_bridge_port *
 br_dst_port(struct net_bridge_dst dst)
 {
-	return (struct net_bridge_port *)ACCESS_PRIVATE(&dst, value);
+	struct net_bridge_port *p;
+	struct net_bridge_vlan *v;
+
+	br_dst_decode(dst, &p, &v);
+
+	return p;
+}
+
+static inline struct net_bridge_vlan *
+br_dst_vlan(struct net_bridge_dst dst)
+{
+	unsigned long value;
+
+	value = ACCESS_PRIVATE(&dst, value);
+	if (!(value & BR_DST_VLAN_TAG))
+		return NULL;
+
+	return (struct net_bridge_vlan *)(value & ~BR_DST_VLAN_TAG);
 }
 
 static inline struct net_bridge_dst
-- 
2.47.3

[PATCH net-next 04/12] net: bridge: vlan: return VLAN entries from ingress helpers

[Nikolay Aleksandrov]

Have the ingress helpers return the resolved vlan entry directly and let
callers derive the vid from it. This keeps the vlan available for later fdb
operations without carrying parallel vlan pointer and vid.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_device.c  |  5 +++--
 net/bridge/br_input.c   | 14 ++++++++------
 net/bridge/br_private.h | 12 +++++++-----
 net/bridge/br_vlan.c    | 38 +++++++++++++++++++++-----------------
 4 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index c6804409b60b..2423d69f2498 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -65,9 +65,10 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 	skb_reset_mac_header(skb);
 	skb_pull(skb, ETH_HLEN);
 
-	if (!br_allowed_ingress(br, br_vlan_group_rcu(br), skb, &vid,
-				&state, &vlan))
+	if (!br_allowed_ingress(br, br_vlan_group_rcu(br), skb, &state,
+				&vlan))
 		goto out;
+	vid = vlan ? vlan->vid : 0;
 
 	if (IS_ENABLED(CONFIG_INET) &&
 	    (eth_hdr(skb)->h_proto == htons(ETH_P_ARP) ||
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 68aa8fa52eba..d9a0e1f65ec9 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -86,8 +86,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	struct net_bridge_vlan *vlan;
 	struct net_bridge *br;
 	bool promisc;
-	u16 vid = 0;
 	u8 state;
+	u16 vid;
 
 	if (!p)
 		goto drop;
@@ -107,9 +107,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 
 	brmctx = &p->br->multicast_ctx;
 	pmctx = &p->multicast_ctx;
-	if (!br_allowed_ingress(p->br, nbp_vlan_group_rcu(p), skb, &vid,
-				&state, &vlan))
+	if (!br_allowed_ingress(p->br, nbp_vlan_group_rcu(p), skb, &state,
+				&vlan))
 		goto out;
+	vid = vlan ? vlan->vid : 0;
 
 	if (test_bit(BR_PORT_LOCKED_BIT, &p->flags)) {
 		struct net_bridge_fdb_entry *fdb_src =
@@ -245,14 +246,15 @@ EXPORT_SYMBOL_GPL(br_handle_frame_finish);
 static void __br_handle_local_finish(struct sk_buff *skb)
 {
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
-	u16 vid = 0;
+	struct net_bridge_vlan *vlan;
 
 	/* check if vlan is allowed, to avoid spoofing */
 	if (test_bit(BR_LEARNING_BIT, &p->flags) &&
 	    nbp_state_should_learn(p) &&
 	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
-	    br_should_learn(p, skb, &vid))
-		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0);
+	    br_should_learn(p, skb, &vlan))
+		br_fdb_update(p->br, p, eth_hdr(skb)->h_source,
+			      vlan ? vlan->vid : 0, 0);
 }
 
 /* note: already called with rcu_read_lock */
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 2bf7f4429a3b..34e95741b940 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1702,11 +1702,11 @@ br_multicast_ctx_options_equal(const struct net_bridge_mcast *brmctx1,
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
 bool br_allowed_ingress(const struct net_bridge *br,
 			struct net_bridge_vlan_group *vg, struct sk_buff *skb,
-			u16 *vid, u8 *state,
-			struct net_bridge_vlan **vlan);
+			u8 *state, struct net_bridge_vlan **vlan);
 bool br_allowed_egress(struct net_bridge_vlan_group *vg,
 		       const struct sk_buff *skb);
-bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid);
+bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb,
+		     struct net_bridge_vlan **vlan);
 struct sk_buff *br_handle_vlan(struct net_bridge *br,
 			       const struct net_bridge_port *port,
 			       struct net_bridge_vlan_group *vg,
@@ -1825,7 +1825,7 @@ static inline u16 br_vlan_flags(const struct net_bridge_vlan *v, u16 pvid)
 static inline bool br_allowed_ingress(const struct net_bridge *br,
 				      struct net_bridge_vlan_group *vg,
 				      struct sk_buff *skb,
-				      u16 *vid, u8 *state,
+				      u8 *state,
 				      struct net_bridge_vlan **vlan)
 
 {
@@ -1840,8 +1840,10 @@ static inline bool br_allowed_egress(struct net_bridge_vlan_group *vg,
 }
 
 static inline bool br_should_learn(struct net_bridge_port *p,
-				   struct sk_buff *skb, u16 *vid)
+				   struct sk_buff *skb,
+				   struct net_bridge_vlan **vlan)
 {
+	*vlan = NULL;
 	return true;
 }
 
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index ce5aa15c4540..471b4e7f0571 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -583,13 +583,13 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
 /* Called under RCU */
 static bool __allowed_ingress(const struct net_bridge *br,
 			      struct net_bridge_vlan_group *vg,
-			      struct sk_buff *skb, u16 *vid,
-			      u8 *state,
+			      struct sk_buff *skb, u8 *state,
 			      struct net_bridge_vlan **vlan)
 {
 	struct pcpu_sw_netstats *stats;
 	struct net_bridge_vlan *v;
 	bool tagged;
+	u16 vid;
 
 	BR_INPUT_SKB_CB(skb)->vlan_filtered = true;
 	/* If vlan tx offload is disabled on bridge device and frame was
@@ -603,7 +603,7 @@ static bool __allowed_ingress(const struct net_bridge *br,
 			return false;
 	}
 
-	if (!br_vlan_get_tag(skb, vid)) {
+	if (!br_vlan_get_tag(skb, &vid)) {
 		/* Tagged frame */
 		if (skb->vlan_proto != br->vlan_proto) {
 			/* Protocol-mismatch, empty out vlan_tci for new tag */
@@ -615,7 +615,7 @@ static bool __allowed_ingress(const struct net_bridge *br,
 
 			skb_pull(skb, ETH_HLEN);
 			skb_reset_mac_len(skb);
-			*vid = 0;
+			vid = 0;
 			tagged = false;
 		} else {
 			tagged = true;
@@ -625,7 +625,7 @@ static bool __allowed_ingress(const struct net_bridge *br,
 		tagged = false;
 	}
 
-	if (!*vid) {
+	if (!vid) {
 		v = vg ? rcu_dereference(vg->pvid) : NULL;
 		/* Frame had a tag with VID 0 or did not have a tag.
 		 * See if pvid is set on this port.  That tells us which
@@ -637,7 +637,6 @@ static bool __allowed_ingress(const struct net_bridge *br,
 		/* PVID is set on this port.  Any untagged or priority-tagged
 		 * ingress frame is considered to belong to this vlan.
 		 */
-		*vid = v->vid;
 		if (likely(!tagged))
 			/* Untagged Frame. */
 			__vlan_hwaccel_put_tag(skb, br->vlan_proto, v->vid);
@@ -649,7 +648,7 @@ static bool __allowed_ingress(const struct net_bridge *br,
 			 */
 			skb->vlan_tci |= v->vid;
 	} else {
-		v = br_vlan_find(vg, *vid);
+		v = br_vlan_find(vg, vid);
 	}
 
 	if (!v || !br_vlan_should_use(v))
@@ -680,8 +679,7 @@ static bool __allowed_ingress(const struct net_bridge *br,
 
 bool br_allowed_ingress(const struct net_bridge *br,
 			struct net_bridge_vlan_group *vg, struct sk_buff *skb,
-			u16 *vid, u8 *state,
-			struct net_bridge_vlan **vlan)
+			u8 *state, struct net_bridge_vlan **vlan)
 {
 	/* If VLAN filtering is disabled on the bridge, all packets are
 	 * permitted.
@@ -692,7 +690,7 @@ bool br_allowed_ingress(const struct net_bridge *br,
 		return true;
 	}
 
-	return __allowed_ingress(br, vg, skb, vid, state, vlan);
+	return __allowed_ingress(br, vg, skb, state, vlan);
 }
 
 /* Called under RCU. */
@@ -716,11 +714,15 @@ bool br_allowed_egress(struct net_bridge_vlan_group *vg,
 }
 
 /* Called under RCU */
-bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid)
+bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb,
+		     struct net_bridge_vlan **vlan)
 {
 	struct net_bridge_vlan_group *vg;
 	struct net_bridge *br = p->br;
 	struct net_bridge_vlan *v;
+	u16 vid;
+
+	*vlan = NULL;
 
 	/* If filtering was disabled at input, let it pass. */
 	if (!br_opt_get(br, BROPT_VLAN_ENABLED))
@@ -730,20 +732,22 @@ bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid)
 	if (!vg || !READ_ONCE(vg->num_vlans))
 		return false;
 
-	if (!br_vlan_get_tag(skb, vid) && skb->vlan_proto != br->vlan_proto)
-		*vid = 0;
+	if (!br_vlan_get_tag(skb, &vid) && skb->vlan_proto != br->vlan_proto)
+		vid = 0;
 
-	if (!*vid) {
+	if (!vid) {
 		v = rcu_dereference(vg->pvid);
 		if (!v || !br_vlan_state_allowed(br_vlan_get_state(v), true))
 			return false;
-		*vid = v->vid;
+		*vlan = v;
 		return true;
 	}
 
-	v = br_vlan_find(vg, *vid);
-	if (v && br_vlan_state_allowed(br_vlan_get_state(v), true))
+	v = br_vlan_find(vg, vid);
+	if (v && br_vlan_state_allowed(br_vlan_get_state(v), true)) {
+		*vlan = v;
 		return true;
+	}
 
 	return false;
 }
-- 
2.47.3

[PATCH net-next 05/12] net: bridge: fdb: pass VLAN entries to learning updates

[Nikolay Aleksandrov]

Pass the resolved VLAN entry through fdb learning paths and derive the vid
inside br_fdb_update(). The fdb destination remains a port for now, this is
a preparation for caching the VLAN.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_fdb.c     | 16 ++++++++++------
 net/bridge/br_input.c   | 12 ++++++------
 net/bridge/br_private.h |  3 ++-
 3 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 0f5cf5615b56..9cd6deae8635 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -974,9 +974,11 @@ static bool __fdb_mark_active(struct net_bridge_fdb_entry *fdb)
 }
 
 void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
-		   const unsigned char *addr, u16 vid, unsigned long flags)
+		   struct net_bridge_vlan *vlan, const unsigned char *addr,
+		   unsigned long flags)
 {
 	struct net_bridge_fdb_entry *fdb;
+	u16 vid = vlan ? vlan->vid : 0;
 
 	/* some users want to always flood. */
 	if (hold_time(br) == 0)
@@ -1247,9 +1249,11 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 
 static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
 			struct net_bridge_port *p, const unsigned char *addr,
-			u16 nlh_flags, u16 vid, struct nlattr *nfea_tb[],
+			u16 nlh_flags, struct net_bridge_vlan *vlan,
+			struct nlattr *nfea_tb[],
 			bool *notified, struct netlink_ext_ack *extack)
 {
+	u16 vid = vlan ? vlan->vid : 0;
 	int err = 0;
 
 	if (ndm->ndm_flags & NTF_USE) {
@@ -1263,7 +1267,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
 
 		local_bh_disable();
 		rcu_read_lock();
-		br_fdb_update(br, p, addr, vid, BIT(BR_FDB_ADDED_BY_USER));
+		br_fdb_update(br, p, vlan, addr, BIT(BR_FDB_ADDED_BY_USER));
 		rcu_read_unlock();
 		local_bh_enable();
 	} else if (ndm->ndm_flags & NTF_EXT_LEARNED) {
@@ -1355,10 +1359,10 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 		}
 
 		/* VID was specified, so use it. */
-		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, vid, nfea_tb,
+		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v, nfea_tb,
 				   notified, extack);
 	} else {
-		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, 0, nfea_tb,
+		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, NULL, nfea_tb,
 				   notified, extack);
 		if (err || !vg || !vg->num_vlans)
 			goto out;
@@ -1370,7 +1374,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			if (!br_vlan_should_use(v))
 				continue;
-			err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v->vid,
+			err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v,
 					   nfea_tb, notified, extack);
 			if (err)
 				goto out;
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index d9a0e1f65ec9..44a217d65e7f 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -121,8 +121,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 			 * and drop the packet.
 			 */
 			if (test_bit(BR_PORT_MAB_BIT, &p->flags))
-				br_fdb_update(br, p, eth_hdr(skb)->h_source,
-					      vid, BIT(BR_FDB_LOCKED));
+				br_fdb_update(br, p, vlan,
+					      eth_hdr(skb)->h_source,
+					      BIT(BR_FDB_LOCKED));
 			goto drop;
 		} else if (br_fdb_dst_port(fdb_src) != p ||
 			   test_bit(BR_FDB_LOCAL, &fdb_src->flags)) {
@@ -132,7 +133,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 			/* FDB match, but entry is locked. Refresh it and drop
 			 * the packet.
 			 */
-			br_fdb_update(br, p, eth_hdr(skb)->h_source, vid,
+			br_fdb_update(br, p, vlan, eth_hdr(skb)->h_source,
 				      BIT(BR_FDB_LOCKED));
 			goto drop;
 		}
@@ -142,7 +143,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 
 	/* insert into forwarding database after filtering to avoid spoofing */
 	if (test_bit(BR_LEARNING_BIT, &p->flags))
-		br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0);
+		br_fdb_update(br, p, vlan, eth_hdr(skb)->h_source, 0);
 
 	promisc = !!(br->dev->flags & IFF_PROMISC);
 	local_rcv = promisc;
@@ -253,8 +254,7 @@ static void __br_handle_local_finish(struct sk_buff *skb)
 	    nbp_state_should_learn(p) &&
 	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
 	    br_should_learn(p, skb, &vlan))
-		br_fdb_update(p->br, p, eth_hdr(skb)->h_source,
-			      vlan ? vlan->vid : 0, 0);
+		br_fdb_update(p->br, p, vlan, eth_hdr(skb)->h_source, 0);
 }
 
 /* note: already called with rcu_read_lock */
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 34e95741b940..058cc082311a 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -993,7 +993,8 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf, unsigned long count,
 int br_fdb_add_local(struct net_bridge *br, struct net_bridge_port *source,
 		     const unsigned char *addr, u16 vid);
 void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
-		   const unsigned char *addr, u16 vid, unsigned long flags);
+		   struct net_bridge_vlan *vlan, const unsigned char *addr,
+		   unsigned long flags);
 
 int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 		  struct net_device *dev, const unsigned char *addr, u16 vid,
-- 
2.47.3

[PATCH net-next 06/12] net: bridge: fdb: consolidate port-VLAN cleanup

[Nikolay Aleksandrov]

Deleting a port-VLAN currently takes the hash_lock twice and performs two
fdb ops. The first removes its automatically generated local entry and the
second deletes dynamic entries associated with the port and vid. Rename
br_fdb_delete_by_port() to br_fdb_cleanup_by_dst() and pass a bridge dst so
it can distinguish a port-VLAN from a raw port. This allows both operations
to be done during the same locked fdb walk.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br.c          |  2 +-
 net/bridge/br_fdb.c      | 17 +++++++++++++----
 net/bridge/br_if.c       |  6 +++---
 net/bridge/br_netlink.c  |  2 +-
 net/bridge/br_private.h  |  4 ++--
 net/bridge/br_stp_if.c   |  2 +-
 net/bridge/br_sysfs_if.c |  3 ++-
 net/bridge/br_vlan.c     |  5 ++---
 8 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/net/bridge/br.c b/net/bridge/br.c
index 09a120b3b76c..b1abe1c5bbdd 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -202,7 +202,7 @@ static int br_switchdev_event(struct notifier_block *unused,
 	case SWITCHDEV_FDB_FLUSH_TO_BRIDGE:
 		fdb_info = ptr;
 		/* Don't delete static entries */
-		br_fdb_delete_by_port(br, p, fdb_info->vid, 0);
+		br_fdb_cleanup_by_dst(br, br_port_to_dst(p), fdb_info->vid, 0);
 		break;
 	}
 
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 9cd6deae8635..7c68b540b358 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -868,15 +868,16 @@ int br_fdb_delete_bulk(struct nlmsghdr *nlh, struct net_device *dev,
 	return 0;
 }
 
-/* Flush all entries referring to a specific port.
+/* Clean up all entries referring to a specific destination.
  * if do_all is set also flush static entries
  * if vid is set delete all entries that match the vlan_id
  */
-void br_fdb_delete_by_port(struct net_bridge *br,
-			   const struct net_bridge_port *p,
-			   u16 vid,
+void br_fdb_cleanup_by_dst(struct net_bridge *br,
+			   struct net_bridge_dst cleanup_dst, u16 vid,
 			   int do_all)
 {
+	const struct net_bridge_vlan *vlan = br_dst_vlan(cleanup_dst);
+	const struct net_bridge_port *p = br_dst_port(cleanup_dst);
 	struct net_bridge_fdb_entry *f;
 	struct hlist_node *tmp;
 
@@ -885,6 +886,14 @@ void br_fdb_delete_by_port(struct net_bridge *br,
 		if (br_fdb_dst_port(f) != p)
 			continue;
 
+		if (vlan && f->key.vlan_id == vlan->vid &&
+		    test_bit(BR_FDB_LOCAL, &f->flags) &&
+		    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags) &&
+		    ether_addr_equal(f->key.addr.addr, p->dev->dev_addr)) {
+			fdb_delete_local(br, p, f);
+			continue;
+		}
+
 		if (!do_all)
 			if (test_bit(BR_FDB_STATIC, &f->flags) ||
 			    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c52613431f88..d94558a5e3e9 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -355,7 +355,7 @@ static void del_nbp(struct net_bridge_port *p)
 	netdev_reset_rx_headroom(dev);
 
 	nbp_vlan_flush(p);
-	br_fdb_delete_by_port(br, p, 0, 1);
+	br_fdb_cleanup_by_dst(br, br_port_to_dst(p), 0, 1);
 	switchdev_deferred_process();
 	nbp_backup_clear(p);
 
@@ -390,7 +390,7 @@ void br_dev_delete(struct net_device *dev, struct list_head *head)
 	br_mst_uninit(br);
 	br_recalculate_neigh_suppress_enabled(br);
 
-	br_fdb_delete_by_port(br, NULL, 0, 1);
+	br_fdb_cleanup_by_dst(br, br_port_to_dst(NULL), 0, 1);
 
 	timer_shutdown_sync(&br->hello_timer);
 	timer_shutdown_sync(&br->topology_change_timer);
@@ -696,7 +696,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
 	if (fdb_synced)
 		br_fdb_unsync_static(br, p);
 	list_del_rcu(&p->list);
-	br_fdb_delete_by_port(br, p, 0, 1);
+	br_fdb_cleanup_by_dst(br, br_port_to_dst(p), 0, 1);
 	nbp_update_port_count(br);
 	netdev_upper_dev_unlink(dev, br->dev);
 err5:
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index ae76df0de05a..f65b8b6ca97c 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1044,7 +1044,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
 	}
 
 	if (tb[IFLA_BRPORT_FLUSH])
-		br_fdb_delete_by_port(p->br, p, 0, 0);
+		br_fdb_cleanup_by_dst(p->br, br_port_to_dst(p), 0, 0);
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	if (tb[IFLA_BRPORT_MULTICAST_ROUTER]) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 058cc082311a..a790368b69e9 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -983,8 +983,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr);
 void br_fdb_cleanup(struct work_struct *work);
 int br_fdb_toggle_local_vlan_0(struct net_bridge *br, bool on,
 			       struct netlink_ext_ack *extack);
-void br_fdb_delete_by_port(struct net_bridge *br,
-			   const struct net_bridge_port *p, u16 vid, int do_all);
+void br_fdb_cleanup_by_dst(struct net_bridge *br,
+			   struct net_bridge_dst dst, u16 vid, int do_all);
 struct net_bridge_fdb_entry *br_fdb_find_rcu(struct net_bridge *br,
 					     const unsigned char *addr,
 					     __u16 vid);
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index a7e5422eb5d1..2e5190278c90 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -114,7 +114,7 @@ void br_stp_disable_port(struct net_bridge_port *p)
 	timer_delete(&p->hold_timer);
 
 	if (!rcu_access_pointer(p->backup_port))
-		br_fdb_delete_by_port(br, p, 0, 0);
+		br_fdb_cleanup_by_dst(br, br_port_to_dst(p), 0, 0);
 	br_multicast_disable_port(p);
 
 	br_configuration_update(br);
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index b9bcafa7f034..621d62a063d9 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -191,7 +191,8 @@ static BRPORT_ATTR(hold_timer, 0444, show_hold_timer, NULL);
 
 static int store_flush(struct net_bridge_port *p, unsigned long v)
 {
-	br_fdb_delete_by_port(p->br, p, 0, 0); // Don't delete local entry
+	/* Don't delete local entry */
+	br_fdb_cleanup_by_dst(p->br, br_port_to_dst(p), 0, 0);
 	return 0;
 }
 static BRPORT_ATTR(flush, 0200, NULL, store_flush);
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 471b4e7f0571..7ba6e57b18fa 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -870,7 +870,7 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
 		return -ENOENT;
 
 	br_fdb_find_delete_local(br, NULL, br->dev->dev_addr, vid);
-	br_fdb_delete_by_port(br, NULL, vid, 0);
+	br_fdb_cleanup_by_dst(br, br_port_to_dst(NULL), vid, 0);
 
 	vlan_tunnel_info_del(vg, v);
 	__vlan_del(v);
@@ -1396,8 +1396,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	v = br_vlan_find(nbp_vlan_group(port), vid);
 	if (!v)
 		return -ENOENT;
-	br_fdb_find_delete_local(port->br, port, port->dev->dev_addr, vid);
-	br_fdb_delete_by_port(port->br, port, vid, 0);
+	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
 	__vlan_del(v);
 
 	return 0;
-- 
2.47.3

[PATCH net-next 07/12] net: bridge: vlan: split unpublishing from deletion

[Nikolay Aleksandrov]

Split removal of a VLAN from its lookup and flood structures from object
destruction. Pass the VLAN group explicitly to __vlan_del() and call both
operations consecutively for now. This prepares deletion paths to quiesce
readers after making an entry unreachable and before freeing it.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_vlan.c | 45 +++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 7ba6e57b18fa..8a914e249767 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -429,22 +429,34 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
 	goto out;
 }
 
-static void __vlan_del(struct net_bridge_vlan *v)
+static void __vlan_unpublish(struct net_bridge_vlan_group *vg,
+			     struct net_bridge_vlan *v)
+{
+	__vlan_delete_pvid(vg, v);
+	if (!br_vlan_is_master(v)) {
+		struct net_bridge_vlan *masterv = v->brvlan;
+
+		rhashtable_remove_fast(&vg->vlan_hash, &v->vnode,
+				       br_vlan_rht_params);
+		__vlan_del_list(v);
+		/* -1 because br_vlan_put_master() is called later */
+		br_vlan_rebuild_port_array(masterv,
+					   br_vlan_num_ports(masterv) - 1);
+	}
+}
+
+static void __vlan_del(struct net_bridge_vlan_group *vg,
+		       struct net_bridge_vlan *v)
 {
 	struct net_bridge_vlan *masterv = v;
-	struct net_bridge_vlan_group *vg;
 	struct net_bridge_port *p = NULL;
 	int err;
 
-	if (br_vlan_is_master(v)) {
-		vg = br_vlan_group(v->br);
-	} else {
+	if (!br_vlan_is_master(v)) {
 		p = v->port;
-		vg = nbp_vlan_group(v->port);
 		masterv = v->brvlan;
 	}
 
-	__vlan_delete_pvid(vg, v);
 	if (p) {
 		err = __vlan_vid_del(p->dev, p->br, v);
 		if (err)
@@ -467,12 +479,6 @@ static void __vlan_del(struct net_bridge_vlan *v)
 
 	if (masterv != v) {
 		vlan_tunnel_info_del(vg, v);
-		rhashtable_remove_fast(&vg->vlan_hash, &v->vnode,
-				       br_vlan_rht_params);
-		__vlan_del_list(v);
-		/* -1 because br_vlan_put_master() is called later */
-		br_vlan_rebuild_port_array(masterv,
-					   br_vlan_num_ports(masterv) - 1);
 		nbp_vlan_set_vlan_dev_state(p, v->vid);
 		br_multicast_toggle_one_vlan(v, false);
 		br_multicast_port_ctx_deinit(&v->port_mcast_ctx);
@@ -509,7 +515,8 @@ static void __vlan_flush(const struct net_bridge *br,
 		}
 		v_end = vlan->vid;
 
-		__vlan_del(vlan);
+		__vlan_unpublish(vg, vlan);
+		__vlan_del(vg, vlan);
 	}
 
 	/* notify about the last/whole vlan range */
@@ -873,7 +880,8 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
 	br_fdb_cleanup_by_dst(br, br_port_to_dst(NULL), vid, 0);
 
 	vlan_tunnel_info_del(vg, v);
-	__vlan_del(v);
+	__vlan_unpublish(vg, v);
+	__vlan_del(vg, v);
 
 	return 0;
 }
@@ -1389,15 +1397,18 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
  */
 int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 {
+	struct net_bridge_vlan_group *vg;
 	struct net_bridge_vlan *v;
 
 	ASSERT_RTNL();
 
-	v = br_vlan_find(nbp_vlan_group(port), vid);
+	vg = nbp_vlan_group(port);
+	v = br_vlan_find(vg, vid);
 	if (!v)
 		return -ENOENT;
 	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
-	__vlan_del(v);
+	__vlan_unpublish(vg, v);
+	__vlan_del(vg, v);
 
 	return 0;
 }
-- 
2.47.3

[PATCH net-next 08/12] net: bridge: vlan: quiesce readers before freeing port VLANs

[Nikolay Aleksandrov]

Later fdb entries will cache port-VLAN pointers so unpublish a VLAN, wait
for a grace period (existing readers) and then purge or rewrite fdb
references before releasing it. Cached destinations can continue forwarding
until they are cleaned, that is acceptable so add a comment to document it.
During port teardown unpublish the complete VLAN group first, clean the
port fdbs and then release the VLANs. This lets all VLANs share one grace
period.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_fdb.c     | 20 ++++++++++++++++----
 net/bridge/br_if.c      |  7 +++++--
 net/bridge/br_private.h | 12 ++++++++++--
 net/bridge/br_vlan.c    | 21 +++++++++++++++------
 4 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 7c68b540b358..307f9c12914e 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -883,7 +883,9 @@ void br_fdb_cleanup_by_dst(struct net_bridge *br,
 
 	spin_lock_bh(&br->hash_lock);
 	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
-		if (br_fdb_dst_port(f) != p)
+		struct net_bridge_dst dst = br_fdb_dst_read(f);
+
+		if (br_dst_port(dst) != p)
 			continue;
 
 		if (vlan && f->key.vlan_id == vlan->vid &&
@@ -894,12 +896,22 @@ void br_fdb_cleanup_by_dst(struct net_bridge *br,
 			continue;
 		}
 
-		if (!do_all)
+		if (!do_all) {
+			if (vid && f->key.vlan_id != vid)
+				continue;
+
 			if (test_bit(BR_FDB_STATIC, &f->flags) ||
 			    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
-			     !test_bit(BR_FDB_OFFLOADED, &f->flags)) ||
-			    (vid && f->key.vlan_id != vid))
+			     !test_bit(BR_FDB_OFFLOADED, &f->flags))) {
+				/* The entry outlives the VLAN, so it must fall
+				 * back to the raw port destination
+				 */
+				if (vlan && br_dst_vlan(dst) == vlan)
+					br_fdb_dst_write(f,
+							 br_port_to_dst(p));
 				continue;
+			}
+		}
 
 		if (test_bit(BR_FDB_LOCAL, &f->flags))
 			fdb_delete_local(br, p, f);
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d94558a5e3e9..2c05ebc1299d 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -333,8 +333,9 @@ static void update_headroom(struct net_bridge *br, int new_hr)
  */
 static void del_nbp(struct net_bridge_port *p)
 {
-	struct net_bridge *br = p->br;
+	struct net_bridge_vlan_group *vg;
 	struct net_device *dev = p->dev;
+	struct net_bridge *br = p->br;
 
 	sysfs_remove_link(br->ifobj, p->dev->name);
 
@@ -354,8 +355,10 @@ static void del_nbp(struct net_bridge_port *p)
 		update_headroom(br, get_max_headroom(br));
 	netdev_reset_rx_headroom(dev);
 
-	nbp_vlan_flush(p);
+	vg = nbp_vlan_group(p);
+	nbp_vlan_group_unpublish(p);
 	br_fdb_cleanup_by_dst(br, br_port_to_dst(p), 0, 1);
+	nbp_vlan_flush(p, vg);
 	switchdev_deferred_process();
 	nbp_backup_clear(p);
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index a790368b69e9..951b6ac5f484 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1735,7 +1735,9 @@ int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid,
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 		 bool *changed, struct netlink_ext_ack *extack);
 int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
-void nbp_vlan_flush(struct net_bridge_port *port);
+void nbp_vlan_group_unpublish(struct net_bridge_port *port);
+void nbp_vlan_flush(struct net_bridge_port *port,
+		    struct net_bridge_vlan_group *vg);
 int nbp_vlan_init(struct net_bridge_port *port, struct netlink_ext_ack *extack);
 int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
 void br_vlan_get_stats(const struct net_bridge_vlan *v,
@@ -1894,7 +1896,13 @@ static inline int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	return -EOPNOTSUPP;
 }
 
-static inline void nbp_vlan_flush(struct net_bridge_port *port)
+static inline void nbp_vlan_group_unpublish(struct net_bridge_port *port)
+{
+}
+
+static inline void
+nbp_vlan_flush(struct net_bridge_port *port,
+	       struct net_bridge_vlan_group *vg)
 {
 }
 
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 8a914e249767..0675f74cf1b9 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -1406,23 +1406,32 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	v = br_vlan_find(vg, vid);
 	if (!v)
 		return -ENOENT;
-	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
 	__vlan_unpublish(vg, v);
+	synchronize_net();
+	/* Traffic may still use v through cached fdb dsts until they are
+	 * cleaned below. This is acceptable during vlan deletion. Above we
+	 * drain the readers that could republish the dst before cleaning it
+	 */
+	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
 	__vlan_del(vg, v);
 
 	return 0;
 }
 
-void nbp_vlan_flush(struct net_bridge_port *port)
+void nbp_vlan_group_unpublish(struct net_bridge_port *port)
 {
-	struct net_bridge_vlan_group *vg;
-
 	ASSERT_RTNL();
 
-	vg = nbp_vlan_group(port);
-	__vlan_flush(port->br, port, vg);
 	RCU_INIT_POINTER(port->vlgrp, NULL);
 	synchronize_net();
+}
+
+void nbp_vlan_flush(struct net_bridge_port *port,
+		    struct net_bridge_vlan_group *vg)
+{
+	ASSERT_RTNL();
+
+	__vlan_flush(port->br, port, vg);
 	__vlan_group_free(vg);
 }
 
-- 
2.47.3

[PATCH net-next 09/12] net: bridge: fdb: factor out existing entry updates

[Nikolay Aleksandrov]

Move the existing fdb entry update path out of br_fdb_update() into
__fdb_update(). This reduces nesting in br_fdb_update() and keeps the
lookup and creation flow separate from updates to an entry that was found.
No functional changes intended.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_fdb.c | 96 +++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 46 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 307f9c12914e..b87c6f8875da 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -994,6 +994,55 @@ static bool __fdb_mark_active(struct net_bridge_fdb_entry *fdb)
 		  test_and_clear_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags));
 }
 
+static void __fdb_update(struct net_bridge *br,
+			 struct net_bridge_fdb_entry *fdb,
+			 struct net_bridge_port *source,
+			 const unsigned char *addr, u16 vid,
+			 unsigned long flags)
+{
+	bool fdb_modified = false;
+	unsigned long now;
+
+	/* attempt to update an entry for a local interface */
+	if (unlikely(test_bit(BR_FDB_LOCAL, &fdb->flags))) {
+		if (net_ratelimit())
+			br_warn(br, "received packet on %s with own address as source address (addr:%pM, vlan:%u)\n",
+				source->dev->name, addr, vid);
+		return;
+	}
+
+	now = jiffies;
+	if (now != READ_ONCE(fdb->updated)) {
+		WRITE_ONCE(fdb->updated, now);
+		fdb_modified = __fdb_mark_active(fdb);
+	}
+
+	/* fastpath: update of existing entry */
+	if (unlikely(source != br_fdb_dst_port(fdb) &&
+		     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
+		br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
+		br_fdb_dst_write(fdb, br_port_to_dst(source));
+		fdb_modified = true;
+		/* Take over HW learned entry */
+		if (unlikely(test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)))
+			clear_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
+		/* Clear locked flag when roaming to an unlocked port */
+		if (unlikely(test_bit(BR_FDB_LOCKED, &fdb->flags)))
+			clear_bit(BR_FDB_LOCKED, &fdb->flags);
+	}
+
+	if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags))) {
+		set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
+		if (test_and_clear_bit(BR_FDB_DYNAMIC_LEARNED, &fdb->flags))
+			atomic_dec(&br->fdb_n_learned);
+	}
+
+	if (unlikely(fdb_modified)) {
+		trace_br_fdb_update(br, source, addr, vid, flags);
+		fdb_notify(br, fdb, RTM_NEWNEIGH, true);
+	}
+}
+
 void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 		   struct net_bridge_vlan *vlan, const unsigned char *addr,
 		   unsigned long flags)
@@ -1007,49 +1056,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 
 	fdb = fdb_find_rcu(&br->fdb_hash_tbl, addr, vid);
 	if (likely(fdb)) {
-		/* attempt to update an entry for a local interface */
-		if (unlikely(test_bit(BR_FDB_LOCAL, &fdb->flags))) {
-			if (net_ratelimit())
-				br_warn(br, "received packet on %s with own address as source address (addr:%pM, vlan:%u)\n",
-					source->dev->name, addr, vid);
-		} else {
-			unsigned long now = jiffies;
-			bool fdb_modified = false;
-
-			if (now != READ_ONCE(fdb->updated)) {
-				WRITE_ONCE(fdb->updated, now);
-				fdb_modified = __fdb_mark_active(fdb);
-			}
-
-			/* fastpath: update of existing entry */
-			if (unlikely(source != br_fdb_dst_port(fdb) &&
-				     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
-				br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
-				br_fdb_dst_write(fdb, br_port_to_dst(source));
-				fdb_modified = true;
-				/* Take over HW learned entry */
-				if (unlikely(test_bit(BR_FDB_ADDED_BY_EXT_LEARN,
-						      &fdb->flags)))
-					clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
-						  &fdb->flags);
-				/* Clear locked flag when roaming to an
-				 * unlocked port.
-				 */
-				if (unlikely(test_bit(BR_FDB_LOCKED, &fdb->flags)))
-					clear_bit(BR_FDB_LOCKED, &fdb->flags);
-			}
-
-			if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags))) {
-				set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
-				if (test_and_clear_bit(BR_FDB_DYNAMIC_LEARNED,
-						       &fdb->flags))
-					atomic_dec(&br->fdb_n_learned);
-			}
-			if (unlikely(fdb_modified)) {
-				trace_br_fdb_update(br, source, addr, vid, flags);
-				fdb_notify(br, fdb, RTM_NEWNEIGH, true);
-			}
-		}
+		__fdb_update(br, fdb, source, addr, vid, flags);
 	} else {
 		spin_lock(&br->hash_lock);
 		fdb = fdb_create(br, source, addr, vid, flags);
@@ -1057,9 +1064,6 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 			trace_br_fdb_update(br, source, addr, vid, flags);
 			fdb_notify(br, fdb, RTM_NEWNEIGH, true);
 		}
-		/* else  we lose race and someone else inserts
-		 * it first, don't bother updating
-		 */
 		spin_unlock(&br->hash_lock);
 	}
 }
-- 
2.47.3

[PATCH net-next 10/12] net: bridge: fdb: cache port VLANs in learned entries

[Nikolay Aleksandrov]

Store the resolved port-VLAN as the destination of dynamically learned FDB
entries. Roaming updates the destination directly, while an existing raw
port destination is conditionally upgraded without generating a spurious
notification. Conditional replacements prevent both this upgrade and a VLAN
deletion fallback from overwriting a concurrent roam.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_fdb.c     | 57 ++++++++++++++++++++++++++---------------
 net/bridge/br_private.h | 18 +++++++++++++
 2 files changed, 55 insertions(+), 20 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index b87c6f8875da..5664f3d649fa 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -385,7 +385,7 @@ void br_fdb_find_delete_local(struct net_bridge *br,
 }
 
 static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
-					       struct net_bridge_port *source,
+					       struct net_bridge_dst dst,
 					       const unsigned char *addr,
 					       __u16 vid,
 					       unsigned long flags)
@@ -409,7 +409,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
 		return NULL;
 
 	memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
-	br_fdb_dst_write(fdb, br_port_to_dst(source));
+	br_fdb_dst_write(fdb, dst);
 	fdb->key.vlan_id = vid;
 	fdb->flags = flags;
 	fdb->updated = fdb->used = jiffies;
@@ -448,7 +448,7 @@ static int fdb_add_local(struct net_bridge *br, struct net_bridge_port *source,
 		fdb_delete(br, fdb, true);
 	}
 
-	fdb = fdb_create(br, source, addr, vid,
+	fdb = fdb_create(br, br_port_to_dst(source), addr, vid,
 			 BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
 	if (!fdb)
 		return -ENOMEM;
@@ -907,8 +907,8 @@ void br_fdb_cleanup_by_dst(struct net_bridge *br,
 				 * back to the raw port destination
 				 */
 				if (vlan && br_dst_vlan(dst) == vlan)
-					br_fdb_dst_write(f,
-							 br_port_to_dst(p));
+					br_fdb_dst_replace(f, dst,
+							   br_port_to_dst(p));
 				continue;
 			}
 		}
@@ -997,9 +997,12 @@ static bool __fdb_mark_active(struct net_bridge_fdb_entry *fdb)
 static void __fdb_update(struct net_bridge *br,
 			 struct net_bridge_fdb_entry *fdb,
 			 struct net_bridge_port *source,
+			 struct net_bridge_dst dst,
 			 const unsigned char *addr, u16 vid,
 			 unsigned long flags)
 {
+	struct net_bridge_port *old_port;
+	struct net_bridge_dst old_dst;
 	bool fdb_modified = false;
 	unsigned long now;
 
@@ -1017,18 +1020,30 @@ static void __fdb_update(struct net_bridge *br,
 		fdb_modified = __fdb_mark_active(fdb);
 	}
 
+	old_dst = br_fdb_dst_read(fdb);
+	old_port = br_dst_port(old_dst);
 	/* fastpath: update of existing entry */
-	if (unlikely(source != br_fdb_dst_port(fdb) &&
-		     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
-		br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
-		br_fdb_dst_write(fdb, br_port_to_dst(source));
-		fdb_modified = true;
-		/* Take over HW learned entry */
-		if (unlikely(test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)))
-			clear_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
-		/* Clear locked flag when roaming to an unlocked port */
-		if (unlikely(test_bit(BR_FDB_LOCKED, &fdb->flags)))
-			clear_bit(BR_FDB_LOCKED, &fdb->flags);
+	if (unlikely(!br_dst_equal(dst, old_dst) &&
+		     (source == old_port ||
+		      !test_bit(BR_FDB_STICKY, &fdb->flags)))) {
+		if (source != old_port) {
+			br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
+			br_fdb_dst_write(fdb, dst);
+			fdb_modified = true;
+			/* take over HW learned entry */
+			if (unlikely(test_bit(BR_FDB_ADDED_BY_EXT_LEARN,
+					      &fdb->flags)))
+				clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
+					  &fdb->flags);
+			/* clear locked flag when roaming to an unlocked port */
+			if (unlikely(test_bit(BR_FDB_LOCKED, &fdb->flags)))
+				clear_bit(BR_FDB_LOCKED, &fdb->flags);
+		} else {
+			/* raw port-to-VLAN promotion shouldn't overwrite a
+			 * concurrent roam to another port
+			 */
+			br_fdb_dst_replace(fdb, old_dst, dst);
+		}
 	}
 
 	if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags))) {
@@ -1047,6 +1062,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 		   struct net_bridge_vlan *vlan, const unsigned char *addr,
 		   unsigned long flags)
 {
+	struct net_bridge_dst dst = vlan ? br_vlan_to_dst(vlan) :
+					   br_port_to_dst(source);
 	struct net_bridge_fdb_entry *fdb;
 	u16 vid = vlan ? vlan->vid : 0;
 
@@ -1056,10 +1073,10 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 
 	fdb = fdb_find_rcu(&br->fdb_hash_tbl, addr, vid);
 	if (likely(fdb)) {
-		__fdb_update(br, fdb, source, addr, vid, flags);
+		__fdb_update(br, fdb, source, dst, addr, vid, flags);
 	} else {
 		spin_lock(&br->hash_lock);
-		fdb = fdb_create(br, source, addr, vid, flags);
+		fdb = fdb_create(br, dst, addr, vid, flags);
 		if (fdb) {
 			trace_br_fdb_update(br, source, addr, vid, flags);
 			fdb_notify(br, fdb, RTM_NEWNEIGH, true);
@@ -1213,7 +1230,7 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 		if (!(flags & NLM_F_CREATE))
 			return -ENOENT;
 
-		fdb = fdb_create(br, source, addr, vid,
+		fdb = fdb_create(br, br_port_to_dst(source), addr, vid,
 				 BIT(BR_FDB_ADDED_BY_USER));
 		if (!fdb)
 			return -ENOMEM;
@@ -1564,7 +1581,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 		if (locked)
 			flags |= BIT(BR_FDB_LOCKED);
 
-		fdb = fdb_create(br, p, addr, vid, flags);
+		fdb = fdb_create(br, br_port_to_dst(p), addr, vid, flags);
 		if (!fdb) {
 			err = -ENOMEM;
 			goto err_unlock;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 951b6ac5f484..a1c5b1abc1ef 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -694,6 +694,13 @@ static inline void br_dst_write(struct net_bridge_dst *dst,
 		   ACCESS_PRIVATE(&src, value));
 }
 
+static inline bool br_dst_equal(struct net_bridge_dst dst1,
+				struct net_bridge_dst dst2)
+{
+	return ACCESS_PRIVATE(&dst1, value) ==
+	       ACCESS_PRIVATE(&dst2, value);
+}
+
 static inline struct net_bridge_dst
 br_port_to_dst(const struct net_bridge_port *p)
 {
@@ -768,6 +775,17 @@ static inline void br_fdb_dst_write(struct net_bridge_fdb_entry *fdb,
 	br_dst_write(&fdb->dst, dst);
 }
 
+static inline bool
+br_fdb_dst_replace(struct net_bridge_fdb_entry *fdb,
+		   struct net_bridge_dst old,
+		   struct net_bridge_dst new)
+{
+	unsigned long old_value = ACCESS_PRIVATE(&old, value);
+
+	return cmpxchg(&ACCESS_PRIVATE(&fdb->dst, value), old_value,
+		       ACCESS_PRIVATE(&new, value)) == old_value;
+}
+
 static inline struct net_bridge_port *
 br_fdb_dst_port(const struct net_bridge_fdb_entry *fdb)
 {
-- 
2.47.3

[PATCH net-next 11/12] net: bridge: fdb: cache VLAN destinations in configured entries

[Nikolay Aleksandrov]

Use the known port-VLAN destination for user-configured fdb entries and
resolve switchdev vids with an fdb helper. VLAN 0, bridge entries and
unconfigured switchdev vids retain raw port destinations. Keep the vid
alongside the destination because it remains part of the fdb key.
Use conditional replacement for same port destination upgrades so they
don't overwrite a concurrent packet-learned roam. Port changing updates
remain direct and report a forwarding change.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_fdb.c | 61 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 11 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 5664f3d649fa..730b178cf6c4 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -1192,10 +1192,11 @@ static bool fdb_handle_notify(struct net_bridge_fdb_entry *fdb, u8 notify)
 }
 
 /* Update (create or replace) forwarding database entry */
-static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
+static int fdb_add_entry(struct net_bridge *br, struct net_bridge_dst dst,
 			 const u8 *addr, struct ndmsg *ndm, u16 flags, u16 vid,
 			 struct nlattr *nfea_tb[])
 {
+	struct net_bridge_port *source = br_dst_port(dst);
 	bool is_sticky = !!(ndm->ndm_flags & NTF_STICKY);
 	bool refresh = !nfea_tb[NFEA_DONT_REFRESH];
 	struct net_bridge_fdb_entry *fdb;
@@ -1230,19 +1231,26 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 		if (!(flags & NLM_F_CREATE))
 			return -ENOENT;
 
-		fdb = fdb_create(br, br_port_to_dst(source), addr, vid,
+		fdb = fdb_create(br, dst, addr, vid,
 				 BIT(BR_FDB_ADDED_BY_USER));
 		if (!fdb)
 			return -ENOMEM;
 
 		modified = true;
 	} else {
+		struct net_bridge_dst old_dst;
+
 		if (flags & NLM_F_EXCL)
 			return -EEXIST;
 
-		if (br_fdb_dst_port(fdb) != source) {
-			br_fdb_dst_write(fdb, br_port_to_dst(source));
-			modified = true;
+		old_dst = br_fdb_dst_read(fdb);
+		if (!br_dst_equal(old_dst, dst)) {
+			if (br_dst_port(old_dst) != source) {
+				modified = true;
+				br_fdb_dst_write(fdb, dst);
+			} else {
+				br_fdb_dst_replace(fdb, old_dst, dst);
+			}
 		}
 
 		set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
@@ -1296,6 +1304,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
 			bool *notified, struct netlink_ext_ack *extack)
 {
 	u16 vid = vlan ? vlan->vid : 0;
+	struct net_bridge_dst dst;
 	int err = 0;
 
 	if (ndm->ndm_flags & NTF_USE) {
@@ -1320,8 +1329,10 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
 		}
 		err = br_fdb_external_learn_add(br, p, addr, vid, false, true);
 	} else {
+		dst = p && vlan ? br_vlan_to_dst(vlan) : br_port_to_dst(p);
 		spin_lock_bh(&br->hash_lock);
-		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
+		err = fdb_add_entry(br, dst, addr, ndm, nlh_flags, vid,
+				    nfea_tb);
 		spin_unlock_bh(&br->hash_lock);
 	}
 
@@ -1553,10 +1564,31 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
 	rcu_read_unlock();
 }
 
+static struct net_bridge_dst br_fdb_resolve_dst(const struct net_bridge_port *p,
+						u16 vid)
+{
+	struct net_bridge_dst dst = br_port_to_dst(p);
+	struct net_bridge_vlan *v;
+
+	lockdep_assert(rcu_read_lock_held() || lockdep_rtnl_is_held());
+
+	if (!p || !vid)
+		return dst;
+
+	rcu_read_lock();
+	v = br_vlan_find(nbp_vlan_group_rcu(p), vid);
+	if (v)
+		dst = br_vlan_to_dst(v);
+	rcu_read_unlock();
+
+	return dst;
+}
+
 int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 			      const unsigned char *addr, u16 vid, bool locked,
 			      bool swdev_notify)
 {
+	struct net_bridge_dst dst = br_fdb_resolve_dst(p, vid);
 	struct net_bridge_fdb_entry *fdb;
 	bool modified = false;
 	int err = 0;
@@ -1581,25 +1613,32 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 		if (locked)
 			flags |= BIT(BR_FDB_LOCKED);
 
-		fdb = fdb_create(br, br_port_to_dst(p), addr, vid, flags);
+		fdb = fdb_create(br, dst, addr, vid, flags);
 		if (!fdb) {
 			err = -ENOMEM;
 			goto err_unlock;
 		}
 		fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
 	} else {
+		struct net_bridge_dst old_dst;
+
+		old_dst = br_fdb_dst_read(fdb);
 		if (locked &&
 		    (!test_bit(BR_FDB_LOCKED, &fdb->flags) ||
-		     br_fdb_dst_port(fdb) != p)) {
+		     br_dst_port(old_dst) != p)) {
 			err = -EINVAL;
 			goto err_unlock;
 		}
 
 		WRITE_ONCE(fdb->updated, jiffies);
 
-		if (br_fdb_dst_port(fdb) != p) {
-			br_fdb_dst_write(fdb, br_port_to_dst(p));
-			modified = true;
+		if (!br_dst_equal(old_dst, dst)) {
+			if (br_dst_port(old_dst) != p) {
+				modified = true;
+				br_fdb_dst_write(fdb, dst);
+			} else {
+				br_fdb_dst_replace(fdb, old_dst, dst);
+			}
 		}
 
 		if (test_and_set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
-- 
2.47.3

[PATCH net-next 12/12] net: bridge: fdb: avoid VLAN lookups in unicast forwarding

[Nikolay Aleksandrov]

Pass the complete fdb destination to br_forward(). The tagged value is
decoded once into br_fwd_dst on entry so all subsequent forwarding checks
use the decoded port and VLAN. A cached port-VLAN is then used to avoid two
VLAN hash lookups in the standard unicast forwarding path. A backup-port
redirect also uses a raw destination because the cached VLAN belongs to the
original port.

[1] 64 byte vlan packets, 4k randomized fdb hits with 4k fdbs, 5 million
    packets passed 5 times for every case
 VIDs  Ports       Mpps                 Gbps             CPU1 cycles/input
                before    after   gain  before   after   before   after  reduction
   64    8/2  2.375486 2.635811  11.0%   1.216   1.350   1724.6  1553.7    9.9%
   64    8/4  2.376731 2.636459  10.9%   1.217   1.350   1714.8  1554.7    9.3%
   64    8/8  2.376621 2.635783  10.9%   1.217   1.350   1722.6  1553.2    9.8%
   64   32/2  2.375876 2.637897  11.0%   1.216   1.351   1726.7  1552.3   10.1%
   64  32/16  2.376442 2.636426  10.9%   1.217   1.350   1721.3  1554.2    9.7%
   64  32/32  2.376763 2.636268  10.9%   1.217   1.350   1726.8  1553.5   10.0%
   64   64/2  2.375678 2.638707  11.1%   1.216   1.351   1725.0  1552.1   10.0%
   64  64/32  2.376681 2.637359  11.0%   1.217   1.350   1725.1  1555.9    9.8%
   64  64/64  2.376787 2.638396  11.0%   1.217   1.351   1722.8  1550.9   10.0%
 1024    8/2  2.263768 2.498881  10.4%   1.159   1.279   1808.5  1637.1    9.5%
 1024    8/4  2.264519 2.499075  10.4%   1.159   1.280   1807.6  1638.8    9.3%
 1024    8/8  2.263551 2.499231  10.4%   1.159   1.280   1812.0  1640.2    9.5%
 1024   32/2  2.263841 2.499120  10.4%   1.159   1.280   1806.8  1639.6    9.3%
 1024  32/16  2.263990 2.499562  10.4%   1.159   1.280   1806.1  1639.2    9.2%
 1024  32/32  2.263719 2.498905  10.4%   1.159   1.279   1807.7  1639.3    9.3%
 1024   64/2  2.263635 2.501889  10.5%   1.159   1.281   1809.4  1635.6    9.6%
 1024  64/32  2.264467 2.500359  10.4%   1.159   1.280   1806.5  1633.0    9.6%
 1024  64/64  2.240494 2.470246  10.3%   1.147   1.265   1810.0  1637.1    9.6%

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
CC: Pablo Neira Ayuso <pablo@netfilter.org>
CC: Florian Westphal <fw@strlen.de>
CC: Phil Sutter <phil@nwl.cc>
CC: netfilter-devel@vger.kernel.org
CC: coreteam@netfilter.org

 net/bridge/br_device.c                   |  2 +-
 net/bridge/br_forward.c                  | 24 +++++++++++++-----------
 net/bridge/br_input.c                    |  2 +-
 net/bridge/br_mrp.c                      |  6 +++---
 net/bridge/br_private.h                  |  2 +-
 net/bridge/netfilter/nft_reject_bridge.c |  8 ++++----
 6 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 2423d69f2498..0a1ce9e59a4d 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -108,7 +108,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		else
 			br_flood(br, vlan, skb, BR_PKT_MULTICAST, false, true);
 	} else if ((dst = br_fdb_find_rcu(br, dest, vid)) != NULL) {
-		br_forward(br_fdb_dst_port(dst), skb, false, true);
+		br_forward(br_fdb_dst_read(dst), skb, false, true);
 	} else {
 		br_flood(br, vlan, skb, BR_PKT_UNICAST, false, true);
 	}
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index b5eece1ff9e8..82645db32efb 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -17,7 +17,7 @@
 #include "br_private.h"
 
 struct br_fwd_dst {
-	const struct net_bridge_port *port;
+	struct net_bridge_port *port;
 	struct net_bridge_vlan *vlan;
 };
 
@@ -149,35 +149,37 @@ static int deliver_clone(const struct br_fwd_dst *fwd,
 
 /**
  * br_forward - forward a packet to a specific port
- * @to: destination port
+ * @dst: bridge destination
  * @skb: packet being forwarded
  * @local_rcv: packet will be received locally after forwarding
  * @local_orig: packet is locally originated
  *
  * Should be called with rcu_read_lock.
  */
-void br_forward(const struct net_bridge_port *to,
+void br_forward(struct net_bridge_dst dst,
 		struct sk_buff *skb, bool local_rcv, bool local_orig)
 {
 	struct br_fwd_dst fwd;
 
-	if (unlikely(!to))
+	br_dst_decode(dst, &fwd.port, &fwd.vlan);
+
+	if (unlikely(!fwd.port))
 		goto out;
 
 	/* redirect to backup link if the destination port is down */
-	if (rcu_access_pointer(to->backup_port) &&
-	    (!netif_carrier_ok(to->dev) || !netif_running(to->dev))) {
+	if (rcu_access_pointer(fwd.port->backup_port) &&
+	    (!netif_carrier_ok(fwd.port->dev) ||
+	     !netif_running(fwd.port->dev))) {
 		struct net_bridge_port *backup_port;
 
-		backup_port = rcu_dereference(to->backup_port);
+		backup_port = rcu_dereference(fwd.port->backup_port);
 		if (unlikely(!backup_port))
 			goto out;
-		BR_INPUT_SKB_CB(skb)->backup_nhid = READ_ONCE(to->backup_nhid);
-		to = backup_port;
+		BR_INPUT_SKB_CB(skb)->backup_nhid = READ_ONCE(fwd.port->backup_nhid);
+		fwd.port = backup_port;
+		fwd.vlan = NULL;
 	}
 
-	fwd.port = to;
-	fwd.vlan = NULL;
 	if (should_deliver(&fwd, skb)) {
 		if (local_rcv)
 			deliver_clone(&fwd, skb, local_orig);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 44a217d65e7f..fc2e381c4fe0 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -225,7 +225,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 
 		if (now != READ_ONCE(dst->used))
 			WRITE_ONCE(dst->used, now);
-		br_forward(br_fdb_dst_port(dst), skb, local_rcv, false);
+		br_forward(br_fdb_dst_read(dst), skb, local_rcv, false);
 	} else {
 		if (!mcast_hit)
 			br_flood(br, vlan, skb, pkt_type, local_rcv, false);
diff --git a/net/bridge/br_mrp.c b/net/bridge/br_mrp.c
index dce6efa96c4c..c8cd5b4f3af8 100644
--- a/net/bridge/br_mrp.c
+++ b/net/bridge/br_mrp.c
@@ -1219,11 +1219,11 @@ static int br_mrp_rcv(struct net_bridge_port *p,
 
 forward:
 	if (p_dst)
-		br_forward(p_dst, skb, true, false);
+		br_forward(br_port_to_dst(p_dst), skb, true, false);
 	if (s_dst)
-		br_forward(s_dst, skb, true, false);
+		br_forward(br_port_to_dst(s_dst), skb, true, false);
 	if (i_dst)
-		br_forward(i_dst, skb, true, false);
+		br_forward(br_port_to_dst(i_dst), skb, true, false);
 
 no_forward:
 	return 1;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index a1c5b1abc1ef..05921192ebda 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1045,7 +1045,7 @@ enum br_pkt_type {
 	BR_PKT_BROADCAST
 };
 int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb);
-void br_forward(const struct net_bridge_port *to, struct sk_buff *skb,
+void br_forward(struct net_bridge_dst dst, struct sk_buff *skb,
 		bool local_rcv, bool local_orig);
 int br_forward_finish(struct net *net, struct sock *sk, struct sk_buff *skb);
 void br_flood(struct net_bridge *br, struct net_bridge_vlan *v,
diff --git a/net/bridge/netfilter/nft_reject_bridge.c b/net/bridge/netfilter/nft_reject_bridge.c
index cd2b04236a99..92b488793914 100644
--- a/net/bridge/netfilter/nft_reject_bridge.c
+++ b/net/bridge/netfilter/nft_reject_bridge.c
@@ -55,7 +55,7 @@ static void nft_reject_br_send_v4_tcp_reset(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_forward(br_port_get_rcu(dev), nskb, false, true);
+	br_forward(br_port_to_dst(br_port_get_rcu(dev)), nskb, false, true);
 }
 
 static void nft_reject_br_send_v4_unreach(struct net *net,
@@ -71,7 +71,7 @@ static void nft_reject_br_send_v4_unreach(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_forward(br_port_get_rcu(dev), nskb, false, true);
+	br_forward(br_port_to_dst(br_port_get_rcu(dev)), nskb, false, true);
 }
 
 static void nft_reject_br_send_v6_tcp_reset(struct net *net,
@@ -87,7 +87,7 @@ static void nft_reject_br_send_v6_tcp_reset(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_forward(br_port_get_rcu(dev), nskb, false, true);
+	br_forward(br_port_to_dst(br_port_get_rcu(dev)), nskb, false, true);
 }
 
 
@@ -104,7 +104,7 @@ static void nft_reject_br_send_v6_unreach(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_forward(br_port_get_rcu(dev), nskb, false, true);
+	br_forward(br_port_to_dst(br_port_get_rcu(dev)), nskb, false, true);
 }
 
 static void nft_reject_bridge_eval(const struct nft_expr *expr,
-- 
2.47.3

Re: [PATCH net-next 01/12] net: bridge: introduce a bridge destination type

[Nikolay Aleksandrov]

On 30/09/2026 10:14, Nikolay Aleksandrov wrote:
> Add an opaque destination type and helpers for representing bridge port
> destinations. Using a separate structure makes raw pointer assignments and
> comparisons fail at build time while marking its value member __private
> makes sparse warn about accesses that bypass the helpers.
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>   net/bridge/br_private.h | 38 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
> 
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 67117fb3dc88..1146187aa2ba 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -306,6 +306,10 @@ struct net_bridge_fdb_key {
>   	u16 vlan_id;
>   };
>   
> +struct net_bridge_dst {
> +	unsigned long __private value;
> +};
> +
>   struct net_bridge_fdb_entry {
>   	struct rhash_head		rhnode;
>   	struct net_bridge_port		*dst;
> @@ -670,6 +674,40 @@ struct br_input_skb_cb {
>   #define br_debug(br, format, args...)			\
>   	pr_debug("%s: " format,  (br)->dev->name, ##args)
>   
> +static inline struct net_bridge_dst
> +br_dst_read(const struct net_bridge_dst *src)
> +{
> +	struct net_bridge_dst dst;
> +
> +	ACCESS_PRIVATE(&dst, value) =
> +		READ_ONCE(ACCESS_PRIVATE(src, value));
> +
> +	return dst;
> +}
> +
> +static inline void br_dst_write(struct net_bridge_dst *dst,
> +				struct net_bridge_dst src)
> +{
> +	WRITE_ONCE(ACCESS_PRIVATE(dst, value),
> +		   ACCESS_PRIVATE(&src, value));
> +}
> +
> +static inline struct net_bridge_dst
> +br_port_to_dst(const struct net_bridge_port *p)
> +{
> +	struct net_bridge_dst dst;
> +
> +	ACCESS_PRIVATE(&dst, value) = (unsigned long)p;
> +


Sashiko says:
  Does casting this const-qualified struct net_bridge_port pointer to an
  unsigned long drop the const restriction?

  - Yes, it does but that is expected.

> +	return dst;
> +}
> +
> +static inline struct net_bridge_port *
> +br_dst_port(struct net_bridge_dst dst)
> +{
> +	return (struct net_bridge_port *)ACCESS_PRIVATE(&dst, value);
> +}

Sashiko says:
  When the opaque token from br_port_to_dst() is resolved here in
  br_dst_port(), is it unconditionally cast back to a mutable
  struct net_bridge_port pointer?
  Could this provide a silent mechanism to cast away const, bypassing C type
  safety constraints in the API design?

  - That is again expected and wanted behaviour, we're extracting the dst.


> +
>   /* called under bridge lock */
>   static inline int br_is_root_bridge(const struct net_bridge *br)
>   {

Re: [PATCH net-next 08/12] net: bridge: vlan: quiesce readers before freeing port VLANs

[Nikolay Aleksandrov]

On 30/09/2026 10:14, Nikolay Aleksandrov wrote:
> Later fdb entries will cache port-VLAN pointers so unpublish a VLAN, wait
> for a grace period (existing readers) and then purge or rewrite fdb
> references before releasing it. Cached destinations can continue forwarding
> until they are cleaned, that is acceptable so add a comment to document it.
> During port teardown unpublish the complete VLAN group first, clean the
> port fdbs and then release the VLANs. This lets all VLANs share one grace
> period.
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>   net/bridge/br_fdb.c     | 20 ++++++++++++++++----
>   net/bridge/br_if.c      |  7 +++++--
>   net/bridge/br_private.h | 12 ++++++++++--
>   net/bridge/br_vlan.c    | 21 +++++++++++++++------
>   4 files changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 7c68b540b358..307f9c12914e 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -883,7 +883,9 @@ void br_fdb_cleanup_by_dst(struct net_bridge *br,
>   
>   	spin_lock_bh(&br->hash_lock);
>   	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
> -		if (br_fdb_dst_port(f) != p)
> +		struct net_bridge_dst dst = br_fdb_dst_read(f);
> +
> +		if (br_dst_port(dst) != p)
>   			continue;
>   
>   		if (vlan && f->key.vlan_id == vlan->vid &&
> @@ -894,12 +896,22 @@ void br_fdb_cleanup_by_dst(struct net_bridge *br,
>   			continue;
>   		}
>   
> -		if (!do_all)
> +		if (!do_all) {
> +			if (vid && f->key.vlan_id != vid)
> +				continue;
> +
>   			if (test_bit(BR_FDB_STATIC, &f->flags) ||
>   			    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
> -			     !test_bit(BR_FDB_OFFLOADED, &f->flags)) ||
> -			    (vid && f->key.vlan_id != vid))
> +			     !test_bit(BR_FDB_OFFLOADED, &f->flags))) {
> +				/* The entry outlives the VLAN, so it must fall
> +				 * back to the raw port destination
> +				 */
> +				if (vlan && br_dst_vlan(dst) == vlan)
> +					br_fdb_dst_write(f,
> +							 br_port_to_dst(p));
>   				continue;
> +			}
> +		}
>   
>   		if (test_bit(BR_FDB_LOCAL, &f->flags))
>   			fdb_delete_local(br, p, f);
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index d94558a5e3e9..2c05ebc1299d 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -333,8 +333,9 @@ static void update_headroom(struct net_bridge *br, int new_hr)
>    */
>   static void del_nbp(struct net_bridge_port *p)
>   {
> -	struct net_bridge *br = p->br;
> +	struct net_bridge_vlan_group *vg;
>   	struct net_device *dev = p->dev;
> +	struct net_bridge *br = p->br;
>   
>   	sysfs_remove_link(br->ifobj, p->dev->name);
>   
> @@ -354,8 +355,10 @@ static void del_nbp(struct net_bridge_port *p)
>   		update_headroom(br, get_max_headroom(br));
>   	netdev_reset_rx_headroom(dev);
>   
> -	nbp_vlan_flush(p);
> +	vg = nbp_vlan_group(p);
> +	nbp_vlan_group_unpublish(p);
>   	br_fdb_cleanup_by_dst(br, br_port_to_dst(p), 0, 1);
> +	nbp_vlan_flush(p, vg);
>   	switchdev_deferred_process();
>   	nbp_backup_clear(p);
>   
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index a790368b69e9..951b6ac5f484 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -1735,7 +1735,9 @@ int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid,
>   int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
>   		 bool *changed, struct netlink_ext_ack *extack);
>   int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
> -void nbp_vlan_flush(struct net_bridge_port *port);
> +void nbp_vlan_group_unpublish(struct net_bridge_port *port);
> +void nbp_vlan_flush(struct net_bridge_port *port,
> +		    struct net_bridge_vlan_group *vg);
>   int nbp_vlan_init(struct net_bridge_port *port, struct netlink_ext_ack *extack);
>   int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
>   void br_vlan_get_stats(const struct net_bridge_vlan *v,
> @@ -1894,7 +1896,13 @@ static inline int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
>   	return -EOPNOTSUPP;
>   }
>   
> -static inline void nbp_vlan_flush(struct net_bridge_port *port)
> +static inline void nbp_vlan_group_unpublish(struct net_bridge_port *port)
> +{
> +}
> +
> +static inline void
> +nbp_vlan_flush(struct net_bridge_port *port,
> +	       struct net_bridge_vlan_group *vg)
>   {
>   }
>   
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 8a914e249767..0675f74cf1b9 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -1406,23 +1406,32 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
>   	v = br_vlan_find(vg, vid);
>   	if (!v)
>   		return -ENOENT;
> -	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
>   	__vlan_unpublish(vg, v);
> +	synchronize_net();

Sashiko says:
  Does this synchronize_net() call introduce an RTNL stall and IPI storm when
  processing a large VLAN range?
  When a user issues an RTM_DELLINK command specifying a large range of VLANs
  (e.g., vid 1-4094), br_process_vlan_info() iterates over the range sequentially:
  net/bridge/br_netlink.c:br_process_vlan_info() {
      ...
  		for (v = (*vinfo_last)->vid; v <= vinfo_curr->vid; v++) {
  			bool curr_change = false;
  			tmp_vinfo.vid = v;
  			err = br_vlan_info(br, p, cmd, &tmp_vinfo, &curr_change,
  					   extack);
      ...
  }
  This loop calls into br_vlan_info(), which then calls nbp_vlan_delete() for
  each VLAN. Because the global RTNL lock is held during this process,
  synchronize_net() translates to synchronize_rcu_expedited().
  Calling this iteratively up to 4094 times blocks the RTNL lock for several
  seconds and spams all CPUs with IPIs, causing a severe system-wide network
  configuration Denial of Service. This is reachable by any user with
  CAP_NET_ADMIN in the network namespace, which can often be obtained by
  unprivileged local users via user namespaces.
  Could these grace periods be batched or deferred when deleting multiple VLANs?

  - Yes, they can be batched and I had them batched initially but when I ran my tests I didn't
    notice any problems deleting a full VLAN range. It took 79ms on my host to delete 4k VLANs.
    As I've stated in my cover letter - the complexity is not worth it, so I dropped that patch.

> +	/* Traffic may still use v through cached fdb dsts until they are
> +	 * cleaned below. This is acceptable during vlan deletion. Above we
> +	 * drain the readers that could republish the dst before cleaning it
> +	 */
> +	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
>   	__vlan_del(vg, v);
>   
>   	return 0;
>   }
>   
> -void nbp_vlan_flush(struct net_bridge_port *port)
> +void nbp_vlan_group_unpublish(struct net_bridge_port *port)
>   {
> -	struct net_bridge_vlan_group *vg;
> -
>   	ASSERT_RTNL();
>   
> -	vg = nbp_vlan_group(port);
> -	__vlan_flush(port->br, port, vg);
>   	RCU_INIT_POINTER(port->vlgrp, NULL);
>   	synchronize_net();
> +}
> +
> +void nbp_vlan_flush(struct net_bridge_port *port,
> +		    struct net_bridge_vlan_group *vg)
> +{
> +	ASSERT_RTNL();
> +
> +	__vlan_flush(port->br, port, vg);
>   	__vlan_group_free(vg);
>   }
>   

Re: [PATCH net-next 11/12] net: bridge: fdb: cache VLAN destinations in configured entries

[Nikolay Aleksandrov]

On 30/09/2026 10:14, Nikolay Aleksandrov wrote:
> Use the known port-VLAN destination for user-configured fdb entries and
> resolve switchdev vids with an fdb helper. VLAN 0, bridge entries and
> unconfigured switchdev vids retain raw port destinations. Keep the vid
> alongside the destination because it remains part of the fdb key.
> Use conditional replacement for same port destination upgrades so they
> don't overwrite a concurrent packet-learned roam. Port changing updates
> remain direct and report a forwarding change.
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>   net/bridge/br_fdb.c | 61 +++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 50 insertions(+), 11 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 5664f3d649fa..730b178cf6c4 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -1192,10 +1192,11 @@ static bool fdb_handle_notify(struct net_bridge_fdb_entry *fdb, u8 notify)
>   }
>   
>   /* Update (create or replace) forwarding database entry */
> -static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
> +static int fdb_add_entry(struct net_bridge *br, struct net_bridge_dst dst,
>   			 const u8 *addr, struct ndmsg *ndm, u16 flags, u16 vid,
>   			 struct nlattr *nfea_tb[])
>   {
> +	struct net_bridge_port *source = br_dst_port(dst);
>   	bool is_sticky = !!(ndm->ndm_flags & NTF_STICKY);
>   	bool refresh = !nfea_tb[NFEA_DONT_REFRESH];
>   	struct net_bridge_fdb_entry *fdb;
> @@ -1230,19 +1231,26 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
>   		if (!(flags & NLM_F_CREATE))
>   			return -ENOENT;
>   
> -		fdb = fdb_create(br, br_port_to_dst(source), addr, vid,
> +		fdb = fdb_create(br, dst, addr, vid,
>   				 BIT(BR_FDB_ADDED_BY_USER));
>   		if (!fdb)
>   			return -ENOMEM;
>   
>   		modified = true;
>   	} else {
> +		struct net_bridge_dst old_dst;
> +
>   		if (flags & NLM_F_EXCL)
>   			return -EEXIST;
>   
> -		if (br_fdb_dst_port(fdb) != source) {
> -			br_fdb_dst_write(fdb, br_port_to_dst(source));
> -			modified = true;
> +		old_dst = br_fdb_dst_read(fdb);
> +		if (!br_dst_equal(old_dst, dst)) {
> +			if (br_dst_port(old_dst) != source) {
> +				modified = true;
> +				br_fdb_dst_write(fdb, dst);
> +			} else {
> +				br_fdb_dst_replace(fdb, old_dst, dst);
> +			}
>   		}
>   
>   		set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);

Sashiko says:
  If a concurrent packet learning roam occurs via __fdb_update() updating the
  destination locklessly, could br_fdb_dst_replace() fail here?
  If it fails, is it safe to ignore the failure and proceed to set
  BR_FDB_ADDED_BY_USER and potentially other authoritative flags like
  BR_FDB_STATIC below? It appears this might apply the flags to the concurrently
  roamed destination rather than the requested one, potentially corrupting
  hardware offload state and locking traffic to the wrong port.

  - Yes, it is safe and intentional behaviour, it must fail if a roam to a different port
    happened. The flags races are well-known and pre-existing behaviour.

> @@ -1296,6 +1304,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
>   			bool *notified, struct netlink_ext_ack *extack)
>   {
>   	u16 vid = vlan ? vlan->vid : 0;
> +	struct net_bridge_dst dst;
>   	int err = 0;
>   
>   	if (ndm->ndm_flags & NTF_USE) {
> @@ -1320,8 +1329,10 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
>   		}
>   		err = br_fdb_external_learn_add(br, p, addr, vid, false, true);
>   	} else {
> +		dst = p && vlan ? br_vlan_to_dst(vlan) : br_port_to_dst(p);
>   		spin_lock_bh(&br->hash_lock);
> -		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
> +		err = fdb_add_entry(br, dst, addr, ndm, nlh_flags, vid,
> +				    nfea_tb);
>   		spin_unlock_bh(&br->hash_lock);
>   	}
>   
> @@ -1553,10 +1564,31 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
>   	rcu_read_unlock();
>   }
>   
> +static struct net_bridge_dst br_fdb_resolve_dst(const struct net_bridge_port *p,
> +						u16 vid)
> +{
> +	struct net_bridge_dst dst = br_port_to_dst(p);
> +	struct net_bridge_vlan *v;
> +
> +	lockdep_assert(rcu_read_lock_held() || lockdep_rtnl_is_held());
> +
> +	if (!p || !vid)
> +		return dst;
> +
> +	rcu_read_lock();
> +	v = br_vlan_find(nbp_vlan_group_rcu(p), vid);
> +	if (v)
> +		dst = br_vlan_to_dst(v);
> +	rcu_read_unlock();
> +
> +	return dst;
> +}
> +
>   int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>   			      const unsigned char *addr, u16 vid, bool locked,
>   			      bool swdev_notify)
>   {
> +	struct net_bridge_dst dst = br_fdb_resolve_dst(p, vid);
>   	struct net_bridge_fdb_entry *fdb;
>   	bool modified = false;
>   	int err = 0;
> @@ -1581,25 +1613,32 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>   		if (locked)
>   			flags |= BIT(BR_FDB_LOCKED);
>   
> -		fdb = fdb_create(br, br_port_to_dst(p), addr, vid, flags);
> +		fdb = fdb_create(br, dst, addr, vid, flags);
>   		if (!fdb) {
>   			err = -ENOMEM;
>   			goto err_unlock;
>   		}
>   		fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
>   	} else {
> +		struct net_bridge_dst old_dst;
> +
> +		old_dst = br_fdb_dst_read(fdb);
>   		if (locked &&
>   		    (!test_bit(BR_FDB_LOCKED, &fdb->flags) ||
> -		     br_fdb_dst_port(fdb) != p)) {
> +		     br_dst_port(old_dst) != p)) {
>   			err = -EINVAL;
>   			goto err_unlock;
>   		}
>   
>   		WRITE_ONCE(fdb->updated, jiffies);
>   
> -		if (br_fdb_dst_port(fdb) != p) {
> -			br_fdb_dst_write(fdb, br_port_to_dst(p));
> -			modified = true;
> +		if (!br_dst_equal(old_dst, dst)) {
> +			if (br_dst_port(old_dst) != p) {
> +				modified = true;
> +				br_fdb_dst_write(fdb, dst);
> +			} else {
> +				br_fdb_dst_replace(fdb, old_dst, dst);
> +			}
>   		}
>   
>   		if (test_and_set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {

Sashiko says:
  Similarly, if br_fdb_dst_replace() fails here due to a concurrent destination
  update in __fdb_update(), could hardware learning flags like
  BR_FDB_ADDED_BY_EXT_LEARN be misapplied to the new roamed port?

  - Yes, again pre-existing and well-known behaviour. See my reply above.

Re: [PATCH net-next 03/12] net: bridge: add VLAN support to bridge destinations

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Baseline decision: false positive.

--

net: bridge: add VLAN support to bridge destinations

This commit sets bit 0 of struct net_bridge_dst to mark a port-VLAN
pointer, so it can be told apart from a bridge port pointer. It adds
helpers that build VLAN destinations and decode them back into a port and
a VLAN. Nothing creates a VLAN destination yet.

> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index bbb59b53e7452..2bf7f4429a3b2 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h

[ ... ]

> @@ -702,10 +704,56 @@ br_port_to_dst(const struct net_bridge_port *p)
>  	return dst;
>  }
>  
> +static inline struct net_bridge_dst
> +br_vlan_to_dst(const struct net_bridge_vlan *v)
> +{
> +	struct net_bridge_dst dst;
> +
> +	ACCESS_PRIVATE(&dst, value) = (unsigned long)v | BR_DST_VLAN_TAG;
> +
> +	return dst;
> +}
> +
> +static inline void br_dst_decode(struct net_bridge_dst dst,
> +				 struct net_bridge_port **port,
> +				 struct net_bridge_vlan **vlan)
> +{
> +	struct net_bridge_vlan *v;
> +	unsigned long value;
> +
> +	value = ACCESS_PRIVATE(&dst, value);
> +	if (!(value & BR_DST_VLAN_TAG)) {
> +		*port = (struct net_bridge_port *)value;
> +		*vlan = NULL;
> +		return;
> +	}
> +
> +	v = (struct net_bridge_vlan *)(value & ~BR_DST_VLAN_TAG);
> +	*port = v->port;

[Severity: Low]
This isn't a bug, but br_vlan_to_dst() takes any struct net_bridge_vlan
pointer, and br_dst_decode() reads v->port without checking which kind of
VLAN entry it has.

In struct net_bridge_vlan, port is in a union with br:

	union {
		struct net_bridge	*br;
		struct net_bridge_port	*port;
	};

For a bridge master VLAN, br is the active member. Decoding one would hand
back a struct net_bridge pointer typed as a struct net_bridge_port.
Passing NULL would encode to 0x1, and decoding that would dereference a
near-NULL address.

As far as I can tell, no caller can hit this. Later in the series, all of
br_fdb_update(), __br_fdb_add(), br_fdb_resolve_dst() and
nbp_vlan_delete() pass a non-NULL VLAN taken from a port VLAN group.
__br_fdb_add() also excludes the bridge master case on purpose:

	dst = p && vlan ? br_vlan_to_dst(vlan) : br_port_to_dst(p);

Would a comment on br_vlan_to_dst() be worth adding, stating that it only
takes non-NULL port VLANs? Something like
WARN_ON_ONCE(br_vlan_is_master(v)) would also enforce that.

> +	*vlan = v;
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930071411.2786201-1-razor%40blackwall.org

Re: [PATCH net-next 06/12] net: bridge: fdb: consolidate port-VLAN cleanup

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch rewrote the header comment of br_fdb_cleanup_by_dst()
  in net/bridge/br_fdb.c, but the comment does not describe what the…

--

net: bridge: fdb: consolidate port-VLAN cleanup

This renames br_fdb_delete_by_port() to br_fdb_cleanup_by_dst() and passes
a bridge dst, so the function can tell a port-VLAN from a raw port. For a
port-VLAN dst, the port's automatically generated local entry for that VLAN
is now removed in the same locked fdb walk that flushes dynamic entries.
As a result, nbp_vlan_delete() no longer calls br_fdb_find_delete_local()
separately.

> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 9cd6deae86359..7c68b540b3580 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -868,15 +868,16 @@ int br_fdb_delete_bulk(struct nlmsghdr *nlh, struct net_device *dev,
>  	return 0;
>  }
>  
> -/* Flush all entries referring to a specific port.
> +/* Clean up all entries referring to a specific destination.
>   * if do_all is set also flush static entries
>   * if vid is set delete all entries that match the vlan_id
>   */
> -void br_fdb_delete_by_port(struct net_bridge *br,
> -			   const struct net_bridge_port *p,
> -			   u16 vid,
> +void br_fdb_cleanup_by_dst(struct net_bridge *br,
> +			   struct net_bridge_dst cleanup_dst, u16 vid,
>  			   int do_all)
>  {
> +	const struct net_bridge_vlan *vlan = br_dst_vlan(cleanup_dst);
> +	const struct net_bridge_port *p = br_dst_port(cleanup_dst);
>  	struct net_bridge_fdb_entry *f;
>  	struct hlist_node *tmp;
>  
> @@ -885,6 +886,14 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>  		if (br_fdb_dst_port(f) != p)
>  			continue;
>  
> +		if (vlan && f->key.vlan_id == vlan->vid &&
> +		    test_bit(BR_FDB_LOCAL, &f->flags) &&
> +		    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags) &&
> +		    ether_addr_equal(f->key.addr.addr, p->dev->dev_addr)) {
> +			fdb_delete_local(br, p, f);
> +			continue;
> +		}
> +
>  		if (!do_all)
>  			if (test_bit(BR_FDB_STATIC, &f->flags) ||
>  			    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&

[Severity: Low]
This isn't a bug, but does the rewritten header comment above
br_fdb_cleanup_by_dst() still describe what the function does?

The comment says static entries are only flushed when do_all is set. With a
port-VLAN dst, the new branch runs before the do_all check. It removes the
port's auto-generated local entry even when do_all == 0. fdb_add_local()
creates that entry as static:

net/bridge/br_fdb.c:fdb_add_local() {
    ...
	fdb = fdb_create(br, source, addr, vid,
			 BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
    ...
}

The comment also says the function cleans up "all entries referring to a
specific destination". The walk only compares the port part of the dst:

	if (br_fdb_dst_port(f) != p)
		continue;

The VLAN part is used only for the local-entry case.

The local-entry match uses vlan->vid, but the dynamic-entry filter uses the
separate vid argument, and nothing ties the two together. nbp_vlan_delete()
is fine today because it gets v from br_vlan_find(vg, vid).

A later caller might pass br_vlan_to_dst(v) with vid == 0 or with a
different vid. That call would remove v's local entry. It would then flush
dynamic entries across all of the port's VLANs, or in a different VLAN.

Could the comment say that vid is expected to match the VLAN's vid when a
VLAN dst is passed?

The line "if vid is set delete all entries that match the vlan_id" was
already inaccurate before this patch, because vid is ignored under do_all
and static or ext-learned entries are kept. That part is pre-existing, but
the patch kept the line when it rewrote the comment.

Later in the series, in "net: bridge: fdb: avoid VLAN lookups in unicast
forwarding", the comment is still the same. By then the function does more
VLAN-specific work, including falling back from the VLAN dst to the port
dst.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930071411.2786201-1-razor%40blackwall.org

Re: [PATCH net-next 08/12] net: bridge: vlan: quiesce readers before freeing port VLANs

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] nbp_vlan_delete() in net/bridge/br_vlan.c now calls
  synchronize_net() for every port-VLAN it deletes.
- [Low] br_fdb_cleanup_by_dst() in net/bridge/br_fdb.c reads f->dst once
  into `dst`.

--

net: bridge: vlan: quiesce readers before freeing port VLANs

This patch prepares for fdb entries caching port-VLAN pointers. A port
VLAN is now unpublished, a grace period passes, and only then are the fdb
references purged or rewritten and the VLAN freed. During port teardown
the whole VLAN group is unpublished first, so all VLANs share one grace
period.

> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 7c68b540b3580..307f9c12914ef 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -883,7 +883,9 @@ void br_fdb_cleanup_by_dst(struct net_bridge *br,
>
>  	spin_lock_bh(&br->hash_lock);
>  	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
> -		if (br_fdb_dst_port(f) != p)
> +		struct net_bridge_dst dst = br_fdb_dst_read(f);
> +
> +		if (br_dst_port(dst) != p)
>  			continue;
>

[ ... ]

> +			     !test_bit(BR_FDB_OFFLOADED, &f->flags))) {
> +				/* The entry outlives the VLAN, so it must fall
> +				 * back to the raw port destination
> +				 */
> +				if (vlan && br_dst_vlan(dst) == vlan)
> +					br_fdb_dst_write(f,
> +							 br_port_to_dst(p));

[Severity: Low]
Can this unconditional write lose a concurrent roam?

dst is read once at the top of the loop. br_fdb_update() roams entries by
writing fdb->dst under RCU only, without br->hash_lock:

br_fdb_update()
    br_fdb_dst_write(fdb, br_port_to_dst(source));

If a roam lands between br_fdb_dst_read() and this br_fdb_dst_write(), the
new port would be overwritten with p.

This branch can't be reached at this commit. Every br_fdb_dst_write()
caller stores br_port_to_dst(), so br_dst_vlan(dst) is always NULL here.

Later in the series, "net: bridge: fdb: cache port VLANs in learned
entries" replaces this with br_fdb_dst_replace(f, dst, br_port_to_dst(p)).
That is a cmpxchg against the snapshot, so a concurrent roam is kept.

Would it make sense to use the conditional replace here from the start?

>  				continue;
> +			}
> +		}

[ ... ]

> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 8a914e249767a..0675f74cf1b99 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -1406,23 +1406,32 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
>  	v = br_vlan_find(vg, vid);
>  	if (!v)
>  		return -ENOENT;
> -	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
>  	__vlan_unpublish(vg, v);
> +	synchronize_net();

[Severity: Medium]
Does this add one expedited grace period for each port VLAN deleted, all
while RTNL is held?

Every caller of nbp_vlan_delete() holds RTNL, so synchronize_net() always
goes through synchronize_rcu_expedited(). Range requests are processed one
VID at a time under a single RTNL hold:

br_process_vlan_info()
    for (v = (*vinfo_last)->vid; v <= vinfo_curr->vid; v++) {
        br_vlan_info()
            nbp_vlan_delete(p, vinfo->vid)
                synchronize_net()

So a request like "bridge vlan del dev swp1 vid 1-4094" could run up to
4094 expedited grace periods back to back with RTNL held. The
cond_resched() in that loop doesn't drop RTNL.

The per-port loops in br_vlan_disable_default_pvid() and
__br_vlan_set_default_pvid() also now take one grace period per bridge
port. That includes the error unwind loop in __br_vlan_set_default_pvid().

Before this patch, nbp_vlan_delete() waited for no grace period at all,
because __vlan_del() only queues call_rcu().

The commit message only describes batching for port teardown:

  "This lets all VLANs share one grace period."

Could range deletion be batched the same way? For example, unpublish the
whole range, wait once, then clean the fdbs and free the VLANs.

At this commit, no fdb writer stores a VLAN dst yet. That starts with "net:
bridge: fdb: cache port VLANs in learned entries". The per-VLAN
synchronize_net() is still unchanged at the end of the series, in "net:
bridge: fdb: avoid VLAN lookups in unicast forwarding".

> +	/* Traffic may still use v through cached fdb dsts until they are
> +	 * cleaned below. This is acceptable during vlan deletion. Above we
> +	 * drain the readers that could republish the dst before cleaning it
> +	 */
> +	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
>  	__vlan_del(vg, v);
>
>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930071411.2786201-1-razor%40blackwall.org

Re: [PATCH net-next 03/12] net: bridge: add VLAN support to bridge destinations

[Nikolay Aleksandrov]

On 01/10/2026 14:59, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] Baseline decision: false positive.
> 
> --
> 
> net: bridge: add VLAN support to bridge destinations
> 
> This commit sets bit 0 of struct net_bridge_dst to mark a port-VLAN
> pointer, so it can be told apart from a bridge port pointer. It adds
> helpers that build VLAN destinations and decode them back into a port and
> a VLAN. Nothing creates a VLAN destination yet.
> 
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index bbb59b53e7452..2bf7f4429a3b2 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
> 
> [ ... ]
> 
>> @@ -702,10 +704,56 @@ br_port_to_dst(const struct net_bridge_port *p)
>>   	return dst;
>>   }
>>   
>> +static inline struct net_bridge_dst
>> +br_vlan_to_dst(const struct net_bridge_vlan *v)
>> +{
>> +	struct net_bridge_dst dst;
>> +
>> +	ACCESS_PRIVATE(&dst, value) = (unsigned long)v | BR_DST_VLAN_TAG;
>> +
>> +	return dst;
>> +}
>> +
>> +static inline void br_dst_decode(struct net_bridge_dst dst,
>> +				 struct net_bridge_port **port,
>> +				 struct net_bridge_vlan **vlan)
>> +{
>> +	struct net_bridge_vlan *v;
>> +	unsigned long value;
>> +
>> +	value = ACCESS_PRIVATE(&dst, value);
>> +	if (!(value & BR_DST_VLAN_TAG)) {
>> +		*port = (struct net_bridge_port *)value;
>> +		*vlan = NULL;
>> +		return;
>> +	}
>> +
>> +	v = (struct net_bridge_vlan *)(value & ~BR_DST_VLAN_TAG);
>> +	*port = v->port;
> 
> [Severity: Low]
> This isn't a bug, but br_vlan_to_dst() takes any struct net_bridge_vlan
> pointer, and br_dst_decode() reads v->port without checking which kind of
> VLAN entry it has.
> 
> In struct net_bridge_vlan, port is in a union with br:
> 
> 	union {
> 		struct net_bridge	*br;
> 		struct net_bridge_port	*port;
> 	};
> 
> For a bridge master VLAN, br is the active member. Decoding one would hand
> back a struct net_bridge pointer typed as a struct net_bridge_port.
> Passing NULL would encode to 0x1, and decoding that would dereference a
> near-NULL address.
> 
> As far as I can tell, no caller can hit this. Later in the series, all of
> br_fdb_update(), __br_fdb_add(), br_fdb_resolve_dst() and
> nbp_vlan_delete() pass a non-NULL VLAN taken from a port VLAN group.
> __br_fdb_add() also excludes the bridge master case on purpose:
> 
> 	dst = p && vlan ? br_vlan_to_dst(vlan) : br_port_to_dst(p);
> 
> Would a comment on br_vlan_to_dst() be worth adding, stating that it only
> takes non-NULL port VLANs? Something like
> WARN_ON_ONCE(br_vlan_is_master(v)) would also enforce that.
> 

- No, that is clear from the code and its usage. It would be redundant.

>> +	*vlan = v;
>> +}
>> +
> 
> [ ... ]
> 

Re: [PATCH net-next 06/12] net: bridge: fdb: consolidate port-VLAN cleanup

[Nikolay Aleksandrov]

On 01/10/2026 14:59, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The patch rewrote the header comment of br_fdb_cleanup_by_dst()
>    in net/bridge/br_fdb.c, but the comment does not describe what the…
> 
> --
> 
> net: bridge: fdb: consolidate port-VLAN cleanup
> 
> This renames br_fdb_delete_by_port() to br_fdb_cleanup_by_dst() and passes
> a bridge dst, so the function can tell a port-VLAN from a raw port. For a
> port-VLAN dst, the port's automatically generated local entry for that VLAN
> is now removed in the same locked fdb walk that flushes dynamic entries.
> As a result, nbp_vlan_delete() no longer calls br_fdb_find_delete_local()
> separately.
> 
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index 9cd6deae86359..7c68b540b3580 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -868,15 +868,16 @@ int br_fdb_delete_bulk(struct nlmsghdr *nlh, struct net_device *dev,
>>   	return 0;
>>   }
>>   
>> -/* Flush all entries referring to a specific port.
>> +/* Clean up all entries referring to a specific destination.
>>    * if do_all is set also flush static entries
>>    * if vid is set delete all entries that match the vlan_id
>>    */
>> -void br_fdb_delete_by_port(struct net_bridge *br,
>> -			   const struct net_bridge_port *p,
>> -			   u16 vid,
>> +void br_fdb_cleanup_by_dst(struct net_bridge *br,
>> +			   struct net_bridge_dst cleanup_dst, u16 vid,
>>   			   int do_all)
>>   {
>> +	const struct net_bridge_vlan *vlan = br_dst_vlan(cleanup_dst);
>> +	const struct net_bridge_port *p = br_dst_port(cleanup_dst);
>>   	struct net_bridge_fdb_entry *f;
>>   	struct hlist_node *tmp;
>>   
>> @@ -885,6 +886,14 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>>   		if (br_fdb_dst_port(f) != p)
>>   			continue;
>>   
>> +		if (vlan && f->key.vlan_id == vlan->vid &&
>> +		    test_bit(BR_FDB_LOCAL, &f->flags) &&
>> +		    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags) &&
>> +		    ether_addr_equal(f->key.addr.addr, p->dev->dev_addr)) {
>> +			fdb_delete_local(br, p, f);
>> +			continue;
>> +		}
>> +
>>   		if (!do_all)
>>   			if (test_bit(BR_FDB_STATIC, &f->flags) ||
>>   			    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
> 
> [Severity: Low]
> This isn't a bug, but does the rewritten header comment above
> br_fdb_cleanup_by_dst() still describe what the function does?
> 
> The comment says static entries are only flushed when do_all is set. With a
> port-VLAN dst, the new branch runs before the do_all check. It removes the
> port's auto-generated local entry even when do_all == 0. fdb_add_local()
> creates that entry as static:
> 
> net/bridge/br_fdb.c:fdb_add_local() {
>      ...
> 	fdb = fdb_create(br, source, addr, vid,
> 			 BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
>      ...
> }
> 
> The comment also says the function cleans up "all entries referring to a
> specific destination". The walk only compares the port part of the dst:
> 
> 	if (br_fdb_dst_port(f) != p)
> 		continue;
> 
> The VLAN part is used only for the local-entry case.
> 
> The local-entry match uses vlan->vid, but the dynamic-entry filter uses the
> separate vid argument, and nothing ties the two together. nbp_vlan_delete()
> is fine today because it gets v from br_vlan_find(vg, vid).
> 
> A later caller might pass br_vlan_to_dst(v) with vid == 0 or with a
> different vid. That call would remove v's local entry. It would then flush
> dynamic entries across all of the port's VLANs, or in a different VLAN.
> 
> Could the comment say that vid is expected to match the VLAN's vid when a
> VLAN dst is passed?
> 
> The line "if vid is set delete all entries that match the vlan_id" was
> already inaccurate before this patch, because vid is ignored under do_all
> and static or ext-learned entries are kept. That part is pre-existing, but
> the patch kept the line when it rewrote the comment.
> 
> Later in the series, in "net: bridge: fdb: avoid VLAN lookups in unicast
> forwarding", the comment is still the same. By then the function does more
> VLAN-specific work, including falling back from the VLAN dst to the port
> dst.
> 

Hmm ok, I'd prefer to take care of the comment in a follow up as I have a few
minor cleanups gathered from the previous set review and can add this on top.
I'll take care of the pre-existing inaccuracy as well.

Re: [PATCH net-next 08/12] net: bridge: vlan: quiesce readers before freeing port VLANs

[Nikolay Aleksandrov]

On 01/10/2026 14:59, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] nbp_vlan_delete() in net/bridge/br_vlan.c now calls
>    synchronize_net() for every port-VLAN it deletes.
> - [Low] br_fdb_cleanup_by_dst() in net/bridge/br_fdb.c reads f->dst once
>    into `dst`.
> 
> --
> 

I already answered, but here it is again:

> net: bridge: vlan: quiesce readers before freeing port VLANs
> 
> This patch prepares for fdb entries caching port-VLAN pointers. A port
> VLAN is now unpublished, a grace period passes, and only then are the fdb
> references purged or rewritten and the VLAN freed. During port teardown
> the whole VLAN group is unpublished first, so all VLANs share one grace
> period.
> 
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index 7c68b540b3580..307f9c12914ef 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -883,7 +883,9 @@ void br_fdb_cleanup_by_dst(struct net_bridge *br,
>>
>>   	spin_lock_bh(&br->hash_lock);
>>   	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
>> -		if (br_fdb_dst_port(f) != p)
>> +		struct net_bridge_dst dst = br_fdb_dst_read(f);
>> +
>> +		if (br_dst_port(dst) != p)
>>   			continue;
>>
> 
> [ ... ]
> 
>> +			     !test_bit(BR_FDB_OFFLOADED, &f->flags))) {
>> +				/* The entry outlives the VLAN, so it must fall
>> +				 * back to the raw port destination
>> +				 */
>> +				if (vlan && br_dst_vlan(dst) == vlan)
>> +					br_fdb_dst_write(f,
>> +							 br_port_to_dst(p));
> 
> [Severity: Low]
> Can this unconditional write lose a concurrent roam?
> 
> dst is read once at the top of the loop. br_fdb_update() roams entries by
> writing fdb->dst under RCU only, without br->hash_lock:
> 
> br_fdb_update()
>      br_fdb_dst_write(fdb, br_port_to_dst(source));
> 
> If a roam lands between br_fdb_dst_read() and this br_fdb_dst_write(), the
> new port would be overwritten with p.
> 
> This branch can't be reached at this commit. Every br_fdb_dst_write()
> caller stores br_port_to_dst(), so br_dst_vlan(dst) is always NULL here.
> 
> Later in the series, "net: bridge: fdb: cache port VLANs in learned
> entries" replaces this with br_fdb_dst_replace(f, dst, br_port_to_dst(p)).
> That is a cmpxchg against the snapshot, so a concurrent roam is kept.
> 
> Would it make sense to use the conditional replace here from the start?
> 

- No, we're fine with the roam race, it is normal. Trying to change these races
   requires much more work and isn't worth it at the moment.

>>   				continue;
>> +			}
>> +		}
> 
> [ ... ]
> 
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 8a914e249767a..0675f74cf1b99 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -1406,23 +1406,32 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
>>   	v = br_vlan_find(vg, vid);
>>   	if (!v)
>>   		return -ENOENT;
>> -	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
>>   	__vlan_unpublish(vg, v);
>> +	synchronize_net();
> 
> [Severity: Medium]
> Does this add one expedited grace period for each port VLAN deleted, all
> while RTNL is held?
> 
> Every caller of nbp_vlan_delete() holds RTNL, so synchronize_net() always
> goes through synchronize_rcu_expedited(). Range requests are processed one
> VID at a time under a single RTNL hold:
> 
> br_process_vlan_info()
>      for (v = (*vinfo_last)->vid; v <= vinfo_curr->vid; v++) {
>          br_vlan_info()
>              nbp_vlan_delete(p, vinfo->vid)
>                  synchronize_net()
> 
> So a request like "bridge vlan del dev swp1 vid 1-4094" could run up to
> 4094 expedited grace periods back to back with RTNL held. The
> cond_resched() in that loop doesn't drop RTNL.
> 
> The per-port loops in br_vlan_disable_default_pvid() and
> __br_vlan_set_default_pvid() also now take one grace period per bridge
> port. That includes the error unwind loop in __br_vlan_set_default_pvid().
> 
> Before this patch, nbp_vlan_delete() waited for no grace period at all,
> because __vlan_del() only queues call_rcu().
> 
> The commit message only describes batching for port teardown:
> 
>    "This lets all VLANs share one grace period."
> 
> Could range deletion be batched the same way? For example, unpublish the
> whole range, wait once, then clean the fdbs and free the VLANs.
> 
> At this commit, no fdb writer stores a VLAN dst yet. That starts with "net:
> bridge: fdb: cache port VLANs in learned entries". The per-VLAN
> synchronize_net() is still unchanged at the end of the series, in "net:
> bridge: fdb: avoid VLAN lookups in unicast forwarding".
> 

- Yes, they can be batched and I had them batched initially but when I ran my tests I didn't
   notice any problems deleting a full VLAN range. It took 79ms on my host to delete 4k VLANs.
   As I've stated in my cover letter - the complexity is not worth it, so I dropped that patch.

   (from my previous reply)

>> +	/* Traffic may still use v through cached fdb dsts until they are
>> +	 * cleaned below. This is acceptable during vlan deletion. Above we
>> +	 * drain the readers that could republish the dst before cleaning it
>> +	 */
>> +	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
>>   	__vlan_del(vg, v);
>>
>>   	return 0;
>>   }
> 
> [ ... ]
> 

Re: [PATCH net-next 01/12] net: bridge: introduce a bridge destination type

[Jakub Kicinski]

On Wed, 30 Sep 2026 10:14:00 +0300 Nikolay Aleksandrov wrote:
> Add an opaque destination type and helpers for representing bridge port
> destinations. Using a separate structure makes raw pointer assignments and
> comparisons fail at build time while marking its value member __private
> makes sparse warn about accesses that bypass the helpers.

First time I'm seeing the __private thing. FTR feels quite foreign to C.
__bitwise works very nicely for the same thing 🤷️

Re: [PATCH net-next 08/12] net: bridge: vlan: quiesce readers before freeing port VLANs

[Jakub Kicinski]

On Thu, 1 Oct 2026 15:07:21 +0300 Nikolay Aleksandrov wrote:
> - Yes, they can be batched and I had them batched initially but when
>   I ran my tests I didn't notice any problems deleting a full VLAN
>   range. It took 79ms on my host to delete 4k VLANs. As I've stated in
>   my cover letter - the complexity is not worth it, so I dropped that
>   patch.

For the 79ms number - how big was the system and was it busy?

Re: [PATCH net-next 08/12] net: bridge: vlan: quiesce readers before freeing port VLANs

[Nikolay Aleksandrov]

On 02/10/2026 22:50, Jakub Kicinski wrote:
> On Thu, 1 Oct 2026 15:07:21 +0300 Nikolay Aleksandrov wrote:
>> - Yes, they can be batched and I had them batched initially but when
>>    I ran my tests I didn't notice any problems deleting a full VLAN
>>    range. It took 79ms on my host to delete 4k VLANs. As I've stated in
>>    my cover letter - the complexity is not worth it, so I dropped that
>>    patch.
> 
> For the 79ms number - how big was the system and was it busy?

It was a VM with 3 vCPUs, not load. The baseline (without patches) was ~25% faster.
This is still a slow path and deleting 4k VLANs is not a common thing. :)

Note that flushing all VLANs usually happens when a port is being deleted and
that is still fast as before (it doesn't wait a gp for every VLAN).

Re: [PATCH net-next 00/12] net: bridge: vlan: optimize standard fdb fwding path

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 30 Sep 2026 10:13:59 +0300 you wrote:
> Hi,
> This patch-set is a follow-up after the bridge flood fwding path
> optimizations and applies the same idea to the standard fdb fwding path.
> We are able to remove 2 vlan hash lookups in the standard fdb fwding
> path by caching the port-VLAN pointer in the fdb, to do that we switch the
> fdb dst to an opaque type that can be either a port pointer or vlan pointer
> differentiated by a bit. The field is a struct and also has a __private tag
> so we can catch direct users, it should be used only via the helpers.
> Sharing a field makes it easier to pass it around and also allows us to
> save struct space for the fast-path. Interesting case is when an fdb is
> promoted from a raw port to port-VLAN on the same port, it needs to be
> handled carefully so we use cmpxchg to make sure we don't generate
> deletion/replace notifications because it doesn't change the port.
> I tested VLAN deletion after these changes (we now wait a grace period for
> every delete) and the hit was ~20% reduction in deleted VLANs / sec
> deleting 4k VLANs took 14ms more and on my VM the VLANs deleted / sec went
> from 63k to 52k / sec. The complexity to batch them is not worth it.
> 
> [...]

Here is the summary with links:
  - [net-next,01/12] net: bridge: introduce a bridge destination type
    https://git.kernel.org/netdev/net-next/c/13bb47965e02
  - [net-next,02/12] net: bridge: use net_bridge_dst for fdb destinations
    https://git.kernel.org/netdev/net-next/c/fd7a8eb03c32
  - [net-next,03/12] net: bridge: add VLAN support to bridge destinations
    https://git.kernel.org/netdev/net-next/c/a39c339fd6c5
  - [net-next,04/12] net: bridge: vlan: return VLAN entries from ingress helpers
    https://git.kernel.org/netdev/net-next/c/cebd6130e6c2
  - [net-next,05/12] net: bridge: fdb: pass VLAN entries to learning updates
    https://git.kernel.org/netdev/net-next/c/8247ef76032b
  - [net-next,06/12] net: bridge: fdb: consolidate port-VLAN cleanup
    https://git.kernel.org/netdev/net-next/c/f0a349b92120
  - [net-next,07/12] net: bridge: vlan: split unpublishing from deletion
    https://git.kernel.org/netdev/net-next/c/e5c71bf91447
  - [net-next,08/12] net: bridge: vlan: quiesce readers before freeing port VLANs
    https://git.kernel.org/netdev/net-next/c/83edc87a9172
  - [net-next,09/12] net: bridge: fdb: factor out existing entry updates
    https://git.kernel.org/netdev/net-next/c/941056f91907
  - [net-next,10/12] net: bridge: fdb: cache port VLANs in learned entries
    https://git.kernel.org/netdev/net-next/c/8009eb405fc6
  - [net-next,11/12] net: bridge: fdb: cache VLAN destinations in configured entries
    https://git.kernel.org/netdev/net-next/c/8533e9b85bd2
  - [net-next,12/12] net: bridge: fdb: avoid VLAN lookups in unicast forwarding
    https://git.kernel.org/netdev/net-next/c/f1829618e0ad

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 01/12] net: bridge: introduce a bridge destination type

[Nikolay Aleksandrov]

On 02/10/2026 22:45, Jakub Kicinski wrote:
> On Wed, 30 Sep 2026 10:14:00 +0300 Nikolay Aleksandrov wrote:
>> Add an opaque destination type and helpers for representing bridge port
>> destinations. Using a separate structure makes raw pointer assignments and
>> comparisons fail at build time while marking its value member __private
>> makes sparse warn about accesses that bypass the helpers.
> 
> First time I'm seeing the __private thing. FTR feels quite foreign to C.
> __bitwise works very nicely for the same thing 🤷️

I don't mind switching to __bitwise, no preference here. I saw __private initially
in the console code. It seems to be more restrictive (__bitwise allows direct member
copies and comparisons, __private warns about those), but if __bitwise is preferred
I can send a follow-up.

Cheers,
  Nik

Re: [PATCH net-next 01/12] net: bridge: introduce a bridge destination type

[Jakub Kicinski]

On Fri, 2 Oct 2026 23:09:56 +0300 Nikolay Aleksandrov wrote:
> On 02/10/2026 22:45, Jakub Kicinski wrote:
> > On Wed, 30 Sep 2026 10:14:00 +0300 Nikolay Aleksandrov wrote:  
> >> Add an opaque destination type and helpers for representing bridge port
> >> destinations. Using a separate structure makes raw pointer assignments and
> >> comparisons fail at build time while marking its value member __private
> >> makes sparse warn about accesses that bypass the helpers.  
> > 
> > First time I'm seeing the __private thing. FTR feels quite foreign to C.
> > __bitwise works very nicely for the same thing 🤷️  
> 
> I don't mind switching to __bitwise, no preference here. I saw __private initially
> in the console code. It seems to be more restrictive (__bitwise allows direct member
> copies and comparisons, __private warns about those), but if __bitwise is preferred
> I can send a follow-up.

It's alright, mostly flagging for future reference.
And I was curious how you chose it.