Björn Töpel <bjorn.to...@gmail.com> writes:
> From: Björn Töpel <bjorn.to...@intel.com>
>
> Currently the bpf_redirect_map() implementation dispatches to the
> correct map-lookup function via a switch-statement. To avoid the
> dispatching, this change adds one bpf_redirect_map() implementation per
> map. Correct function is automatically selected by the BPF verifier.
>
> rfc->v1: Get rid of the macro and use __always_inline. (Jesper)
>
> Signed-off-by: Björn Töpel <bjorn.to...@intel.com>
Nice! Way better with the __always_inline. One small nit below, but
otherwise:
Acked-by: Toke Høiland-Jørgensen <t...@redhat.com>
> ---
> include/linux/bpf.h | 20 +++++++------
> include/linux/filter.h | 2 ++
> include/net/xdp_sock.h | 6 ++--
> kernel/bpf/cpumap.c | 2 +-
> kernel/bpf/devmap.c | 4 +--
> kernel/bpf/verifier.c | 28 +++++++++++-------
> net/core/filter.c | 67 ++++++++++++++++++++++++++----------------
> 7 files changed, 76 insertions(+), 53 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cccaef1088ea..3dd186eeaf98 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -314,12 +314,14 @@ enum bpf_return_type {
> RET_PTR_TO_BTF_ID, /* returns a pointer to a btf_id */
> };
>
> +typedef u64 (*bpf_func_proto_func)(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
> +
> /* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF
> programs
> * to in-kernel helper functions and for adjusting imm32 field in BPF_CALL
> * instructions after verifying
> */
> struct bpf_func_proto {
> - u64 (*func)(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
> + bpf_func_proto_func func;
> bool gpl_only;
> bool pkt_access;
> enum bpf_return_type ret_type;
> @@ -1429,9 +1431,11 @@ struct btf *bpf_get_btf_vmlinux(void);
> /* Map specifics */
> struct xdp_buff;
> struct sk_buff;
> +struct bpf_dtab_netdev;
> +struct bpf_cpu_map_entry;
>
> -struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
> -struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32
> key);
> +void *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
> +void *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key);
> void __dev_flush(void);
> int dev_xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp,
> struct net_device *dev_rx);
> @@ -1441,7 +1445,7 @@ int dev_map_generic_redirect(struct bpf_dtab_netdev
> *dst, struct sk_buff *skb,
> struct bpf_prog *xdp_prog);
> bool dev_map_can_have_prog(struct bpf_map *map);
>
> -struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32
> key);
> +void *__cpu_map_lookup_elem(struct bpf_map *map, u32 key);
> void __cpu_map_flush(void);
> int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp,
> struct net_device *dev_rx);
> @@ -1568,14 +1572,12 @@ static inline int bpf_obj_get_user(const char __user
> *pathname, int flags)
> return -EOPNOTSUPP;
> }
>
> -static inline struct net_device *__dev_map_lookup_elem(struct bpf_map *map,
> - u32 key)
> +static inline void *__dev_map_lookup_elem(struct bpf_map *map, u32 key)
> {
> return NULL;
> }
>
> -static inline struct net_device *__dev_map_hash_lookup_elem(struct bpf_map
> *map,
> - u32 key)
> +static inline void *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key)
> {
> return NULL;
> }
> @@ -1615,7 +1617,7 @@ static inline int dev_map_generic_redirect(struct
> bpf_dtab_netdev *dst,
> }
>
> static inline
> -struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
> +void *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
> {
> return NULL;
> }
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 3b00fc906ccd..1dedcf66b694 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1472,4 +1472,6 @@ static inline bool bpf_sk_lookup_run_v6(struct net
> *net, int protocol,
> }
> #endif /* IS_ENABLED(CONFIG_IPV6) */
>
> +bpf_func_proto_func get_xdp_redirect_func(enum bpf_map_type map_type);
> +
> #endif /* __LINUX_FILTER_H__ */
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index cc17bc957548..da4139a58630 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -80,8 +80,7 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff
> *xdp);
> int __xsk_map_redirect(struct xdp_sock *xs, struct xdp_buff *xdp);
> void __xsk_map_flush(void);
>
> -static inline struct xdp_sock *__xsk_map_lookup_elem(struct bpf_map *map,
> - u32 key)
> +static inline void *__xsk_map_lookup_elem(struct bpf_map *map, u32 key)
> {
> struct xsk_map *m = container_of(map, struct xsk_map, map);
> struct xdp_sock *xs;
> @@ -109,8 +108,7 @@ static inline void __xsk_map_flush(void)
> {
> }
>
> -static inline struct xdp_sock *__xsk_map_lookup_elem(struct bpf_map *map,
> - u32 key)
> +static inline void *__xsk_map_lookup_elem(struct bpf_map *map, u32 key)
> {
> return NULL;
> }
> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> index 5d1469de6921..a4d2cb93cd69 100644
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c
> @@ -563,7 +563,7 @@ static void cpu_map_free(struct bpf_map *map)
> kfree(cmap);
> }
>
> -struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
> +void *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
> {
> struct bpf_cpu_map *cmap = container_of(map, struct bpf_cpu_map, map);
> struct bpf_cpu_map_entry *rcpu;
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index 85d9d1b72a33..37ac4cde9713 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -258,7 +258,7 @@ static int dev_map_get_next_key(struct bpf_map *map, void
> *key, void *next_key)
> return 0;
> }
>
> -struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32
> key)
> +void *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key)
> {
> struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
> struct hlist_head *head = dev_map_index_hash(dtab, key);
> @@ -392,7 +392,7 @@ void __dev_flush(void)
> * update happens in parallel here a dev_put wont happen until after reading
> the
> * ifindex.
> */
> -struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key)
> +void *__dev_map_lookup_elem(struct bpf_map *map, u32 key)
> {
> struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
> struct bpf_dtab_netdev *obj;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3d34ba492d46..b5fb0c4e911a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5409,7 +5409,8 @@ record_func_map(struct bpf_verifier_env *env, struct
> bpf_call_arg_meta *meta,
> func_id != BPF_FUNC_map_delete_elem &&
> func_id != BPF_FUNC_map_push_elem &&
> func_id != BPF_FUNC_map_pop_elem &&
> - func_id != BPF_FUNC_map_peek_elem)
> + func_id != BPF_FUNC_map_peek_elem &&
> + func_id != BPF_FUNC_redirect_map)
> return 0;
>
> if (map == NULL) {
> @@ -11860,17 +11861,22 @@ static int fixup_bpf_calls(struct bpf_verifier_env
> *env)
> }
>
> patch_call_imm:
> - fn = env->ops->get_func_proto(insn->imm, env->prog);
> - /* all functions that have prototype and verifier allowed
> - * programs to call them, must be real in-kernel functions
> - */
> - if (!fn->func) {
> - verbose(env,
> - "kernel subsystem misconfigured func %s#%d\n",
> - func_id_name(insn->imm), insn->imm);
> - return -EFAULT;
> + if (insn->imm == BPF_FUNC_redirect_map) {
> + aux = &env->insn_aux_data[i];
> + map_ptr = BPF_MAP_PTR(aux->map_ptr_state);
> + insn->imm = get_xdp_redirect_func(map_ptr->map_type) -
> __bpf_call_base;
> + } else {
> + fn = env->ops->get_func_proto(insn->imm, env->prog);
> + /* all functions that have prototype and verifier
> allowed
> + * programs to call them, must be real in-kernel
> functions
> + */
> + if (!fn->func) {
> + verbose(env, "kernel subsystem misconfigured
> func %s#%d\n",
> + func_id_name(insn->imm), insn->imm);
> + return -EFAULT;
> + }
> + insn->imm = fn->func - __bpf_call_base;
> }
> - insn->imm = fn->func - __bpf_call_base;
> }
>
> /* Since poke tab is now finalized, publish aux to tracker. */
> diff --git a/net/core/filter.c b/net/core/filter.c
> index adfdad234674..fd64d768e16a 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3944,22 +3944,6 @@ void xdp_do_flush(void)
> }
> EXPORT_SYMBOL_GPL(xdp_do_flush);
>
> -static inline void *__xdp_map_lookup_elem(struct bpf_map *map, u32 index)
> -{
> - switch (map->map_type) {
> - case BPF_MAP_TYPE_DEVMAP:
> - return __dev_map_lookup_elem(map, index);
> - case BPF_MAP_TYPE_DEVMAP_HASH:
> - return __dev_map_hash_lookup_elem(map, index);
> - case BPF_MAP_TYPE_CPUMAP:
> - return __cpu_map_lookup_elem(map, index);
> - case BPF_MAP_TYPE_XSKMAP:
> - return __xsk_map_lookup_elem(map, index);
> - default:
> - return NULL;
> - }
> -}
> -
> void bpf_clear_redirect_map(struct bpf_map *map)
> {
> struct bpf_redirect_info *ri;
> @@ -4110,22 +4094,17 @@ static const struct bpf_func_proto
> bpf_xdp_redirect_proto = {
> .arg2_type = ARG_ANYTHING,
> };
>
> -BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
> - u64, flags)
> +static __always_inline s64 __bpf_xdp_redirect_map(struct bpf_map *map, u32
> ifindex, u64 flags,
> + void *lookup_elem(struct
> bpf_map *map,
> + u32 key))
> {
> struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>
> - /* Lower bits of the flags are used as return code on lookup failure */
> if (unlikely(flags > XDP_TX))
> return XDP_ABORTED;
>
> - ri->tgt_value = __xdp_map_lookup_elem(map, ifindex);
> + ri->tgt_value = lookup_elem(map, ifindex);
> if (unlikely(!ri->tgt_value)) {
> - /* If the lookup fails we want to clear out the state in the
> - * redirect_info struct completely, so that if an eBPF program
> - * performs multiple lookups, the last one always takes
> - * precedence.
> - */
Why remove the comments?
> WRITE_ONCE(ri->map, NULL);
> return flags;
> }
> @@ -4137,8 +4116,44 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *,
> map, u32, ifindex,
> return XDP_REDIRECT;
> }
>
> +BPF_CALL_3(bpf_xdp_redirect_devmap, struct bpf_map *, map, u32, ifindex,
> u64, flags)
> +{
> + return __bpf_xdp_redirect_map(map, ifindex, flags,
> __dev_map_lookup_elem);
> +}
> +
> +BPF_CALL_3(bpf_xdp_redirect_devmap_hash, struct bpf_map *, map, u32,
> ifindex, u64, flags)
> +{
> + return __bpf_xdp_redirect_map(map, ifindex, flags,
> __dev_map_hash_lookup_elem);
> +}
> +
> +BPF_CALL_3(bpf_xdp_redirect_cpumap, struct bpf_map *, map, u32, ifindex,
> u64, flags)
> +{
> + return __bpf_xdp_redirect_map(map, ifindex, flags,
> __cpu_map_lookup_elem);
> +}
> +
> +BPF_CALL_3(bpf_xdp_redirect_xskmap, struct bpf_map *, map, u32, ifindex,
> u64, flags)
> +{
> + return __bpf_xdp_redirect_map(map, ifindex, flags,
> __xsk_map_lookup_elem);
> +}
> +
> +bpf_func_proto_func get_xdp_redirect_func(enum bpf_map_type map_type)
> +{
> + switch (map_type) {
> + case BPF_MAP_TYPE_DEVMAP:
> + return bpf_xdp_redirect_devmap;
> + case BPF_MAP_TYPE_DEVMAP_HASH:
> + return bpf_xdp_redirect_devmap_hash;
> + case BPF_MAP_TYPE_CPUMAP:
> + return bpf_xdp_redirect_cpumap;
> + case BPF_MAP_TYPE_XSKMAP:
> + return bpf_xdp_redirect_xskmap;
> + default:
> + return NULL;
> + }
> +}
> +
> +/* NB! .func is NULL! get_xdp_redirect_func() is used instead! */
> static const struct bpf_func_proto bpf_xdp_redirect_map_proto = {
> - .func = bpf_xdp_redirect_map,
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> .arg1_type = ARG_CONST_MAP_PTR,
> --
> 2.27.0
