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
- signature.asc [application/pgp-signature] 228 bytes