> [ snip to focus on the API ]
>
> > +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
>
> > + 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?
>
> > +
> > + 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.
>
> > +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
>
Attachment:
signature.asc
Description: PGP signature
