On 24/01/2022 19:20, Lorenzo Bianconi wrote:

Similar to bpf_xdp_ct_lookup routine, introduce
br_fdb_find_port_from_ifindex unstable helper in order to accelerate
linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a
lookup in the associated bridge fdb table and it will return the
output ifindex if the destination address is associated to a bridge
port or -ENODEV for BOM traffic or if lookup fails.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

Hi Lorenzo,
Please CC bridge maintainers for bridge-related patches, I've added Roopa and the
bridge mailing list as well. Aside from that, the change is certainly interesting, I've been
thinking about a similar helper for some time now, few comments below.

Have you thought about the egress path and if by the current bridge state the packet would
be allowed to egress through the found port from the lookup? I'd guess you have to keep updating
the active ports list based on netlink events, but there's a lot of egress bridge logic that
either have to be duplicated or somehow synced. Check should_deliver() (br_forward.c) and later
egress stages, but I see how this is a good first step and perhaps we can build upon it.
There are a few possible solutions, but I haven't tried anything yet, most obvious being
yet another helper. :)

Add err_out7 and handle it there please. Let's keep it consistent.
Also I cannot find register_btf_kfunc_id_set() in net-next or Linus' master, but
should it be paired with an unregister on unload (br_deinit) ?

This is wrong, f->updated should be set only if anything changed for the fdb.
Also you can optimize f->used a little bit if you check if jiffies != current value
before setting, you can have millions of packets per sec dirtying that cache line.

Aside from the above, it will change expected behaviour for br_fdb_find_port users
(mlxsw, added Ido to CC as well) because it will mark the fdb as active and refresh it
which should be done only for the ebpf helper, or might be exported through another helper
so ebpf users can decide if they want it updated. There are 2 different use cases and it is
not ok for both as we'll start refreshing fdbs that have been inactive for a while
and would've expired otherwise.

+		return f->dst->dev;

This is wrong as well, f->dst can become NULL (fdb switched to point to the bridge itself).
You should make sure to read f->dst only once and work with the result. I know it's
been like that, but it was ok when accessed with rtnl held.

+	}
+	return NULL;
+}
+
+struct net_device *br_fdb_find_port(const struct net_device *br_dev,
+				    const unsigned char *addr,
+				    __u16 vid)
+{
+	struct net_device *dev;
+
+	ASSERT_RTNL();
+
+	rcu_read_lock();
+	dev = __br_fdb_find_port(br_dev, addr, vid, false);
 	rcu_read_unlock();
 
 	return dev;
 }
 EXPORT_SYMBOL_GPL(br_fdb_find_port);
 
+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz)
+{
+	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+	struct net_bridge_port *port;
+	struct net_device *dev;
+	int ret = -ENODEV;
+
+	BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);
+	if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))
+		return -ENODEV;
+
+	rcu_read_lock();
+
+	dev = dev_get_by_index_rcu(dev_net(ctx->rxq->dev), opt->ifindex);
+	if (!dev)
+		goto out;
+
+	if (unlikely(!netif_is_bridge_port(dev)))
+		goto out;

This check shouldn't be needed if the port checks below succeed.

Thanks,
 Nik