Thread (19 messages) 19 messages, 6 authors, 2022-01-26

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: 2022-01-26 11:42:21
Also in: bpf

[ snip to focus on the API ]
quoted
+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;
Why is the BUILD_BUG_ON needed? Or why is the NF_BPF_FDB_OPTS_SZ
constant even needed?
I added it to be symmetric with respect to ct counterpart
quoted
+	rcu_read_lock();
This is not needed when the function is only being called from XDP...
don't we need it since we do not hold the rtnl here?
quoted
+
+	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;
+
+	port = br_port_get_check_rcu(dev);
+	if (unlikely(!port || !port->br))
+		goto out;
+
+	dev = __br_fdb_find_port(port->br->dev, opt->addr, opt->vid, true);
+	if (dev)
+		ret = dev->ifindex;
+out:
+	rcu_read_unlock();
+
+	return ret;
+}
+
 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_private.h b/net/bridge/br_private.h
index 2661dda1a92b..64d4f1727da2 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -18,6 +18,7 @@
 #include <linux/if_vlan.h>
 #include <linux/rhashtable.h>
 #include <linux/refcount.h>
+#include <linux/bpf.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)
@@ -2094,4 +2095,15 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 void br_do_suppress_nd(struct sk_buff *skb, struct net_bridge *br,
 		       u16 vid, struct net_bridge_port *p, struct nd_msg *msg);
 struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb, struct nd_msg *m);
+
+#define NF_BPF_FDB_OPTS_SZ	12
+struct bpf_fdb_lookup {
+	u8	addr[ETH_ALEN]; /* ETH_ALEN */
+	u16	vid;
+	u32	ifindex;
+};
It seems like addr and ifindex should always be required, right? So why
not make them regular function args? That way the ptr to eth addr could
be a ptr directly to the packet header (saving a memcpy), and the common
case(?) could just pass a NULL opts struct?
ack, right. I agree.
quoted
+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz);
It should probably be documented that the return value is an ifindex as
well; I guess one of the drawbacks of kfunc's relative to regular
helpers is that there is no convention for how to document their usage -
maybe we should fix that before we get too many of them? :)
kfunc is probably too new :)

Regards,
Lorenzo
-Toke

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help