| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1cb01875-6044-540a-e1f8-0cafd9587ee8@iogearbox.net>
Date: Wed, 23 May 2018 11:34:22 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Jesper Dangaard Brouer <brouer@...hat.com>, netdev@...r.kernel.org,
Daniel Borkmann <borkmann@...earbox.net>,
Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Christoph Hellwig <hch@...radead.org>,
BjörnTöpel <bjorn.topel@...el.com>,
Magnus Karlsson <magnus.karlsson@...el.com>,
makita.toshiaki@....ntt.co.jp
Subject: Re: [bpf-next V4 PATCH 1/8] bpf: devmap introduce dev_map_enqueue
On 05/18/2018 03:34 PM, Jesper Dangaard Brouer wrote:
> Functionality is the same, but the ndo_xdp_xmit call is now
> simply invoked from inside the devmap.c code.
>
> V2: Fix compile issue reported by kbuild test robot <lkp@...el.com>
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
> include/linux/bpf.h | 14 +++++++++++---
> include/trace/events/xdp.h | 9 ++++++++-
> kernel/bpf/devmap.c | 37 +++++++++++++++++++++++++++++++------
> net/core/filter.c | 15 ++-------------
> 4 files changed, 52 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index ed0122b45b63..fc1459bdcafc 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -485,14 +485,15 @@ int bpf_check(struct bpf_prog **fp, union bpf_attr *attr);
> void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth);
>
> /* Map specifics */
> -struct net_device *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
> +struct xdp_buff;
> +struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
When you have some follow-up patches, would be great if you could clean this
up a bit. At least a newline after the struct declaration would make it a bit
more readable.
> void __dev_map_insert_ctx(struct bpf_map *map, u32 index);
> void __dev_map_flush(struct bpf_map *map);
> +int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp);
>
> struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key);
> void __cpu_map_insert_ctx(struct bpf_map *map, u32 index);
> void __cpu_map_flush(struct bpf_map *map);
> -struct xdp_buff;
> int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp,
> struct net_device *dev_rx);
>
> @@ -571,6 +572,14 @@ static inline void __dev_map_flush(struct bpf_map *map)
> {
> }
>
> +struct xdp_buff;
> +struct bpf_dtab_netdev;
> +static inline
In particular here as well.
> +int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp)
> +{
> + return 0;
> +}
> +
> static inline
> struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
> {
> @@ -585,7 +594,6 @@ static inline void __cpu_map_flush(struct bpf_map *map)
> {
> }
>
> -struct xdp_buff;
> static inline int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu,
> struct xdp_buff *xdp,
> struct net_device *dev_rx)
> diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
> index 8989a92c571a..96104610d40e 100644
> --- a/include/trace/events/xdp.h
> +++ b/include/trace/events/xdp.h
> @@ -138,11 +138,18 @@ DEFINE_EVENT_PRINT(xdp_redirect_template, xdp_redirect_map_err,
> __entry->map_id, __entry->map_index)
> );
>
> +#ifndef __DEVMAP_OBJ_TYPE
> +#define __DEVMAP_OBJ_TYPE
> +struct _bpf_dtab_netdev {
> + struct net_device *dev;
> +};
> +#endif /* __DEVMAP_OBJ_TYPE */
The __DEVMAP_OBJ_TYPE is not used anywhere, what's its purpose?
Also if you define struct _bpf_dtab_netdev this is rather fragile when mapping
to struct bpf_dtab_netdev. Best way of guarding this is to make a BUILD_BUG_ON()
where you compare both offsets in the struct and bail out compilation whenever
this changes.
> #define devmap_ifindex(fwd, map) \
> (!fwd ? 0 : \
> (!map ? 0 : \
> ((map->map_type == BPF_MAP_TYPE_DEVMAP) ? \
> - ((struct net_device *)fwd)->ifindex : 0)))
> + ((struct _bpf_dtab_netdev *)fwd)->dev->ifindex : 0)))
>
> #define _trace_xdp_redirect_map(dev, xdp, fwd, map, idx) \
> trace_xdp_redirect_map(dev, xdp, devmap_ifindex(fwd, map), \
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index 565f9ece9115..808808bf2bf2 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -48,18 +48,21 @@
> * calls will fail at this point.
> */
> #include <linux/bpf.h>
> +#include <net/xdp.h>
> #include <linux/filter.h>
>
> #define DEV_CREATE_FLAG_MASK \
> (BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
>
> +/* objects in the map */
This comment is unnecessary.
> struct bpf_dtab_netdev {
> - struct net_device *dev;
> + struct net_device *dev; /* must be first member, due to tracepoint */
> struct bpf_dtab *dtab;
> unsigned int bit;
> struct rcu_head rcu;
> };
>
> +/* bpf map container */
Ditto. Why add it? If it's unclear from the code, then it would probably be
better to clean up the code a bit to make it more obvious. Comments should
explain *why* we do certain things, not *what* the code is doing. Latter is
just a sign that the code itself should be improved potentially. :)
> struct bpf_dtab {
> struct bpf_map map;
> struct bpf_dtab_netdev **netdev_map;
> @@ -240,21 +243,43 @@ void __dev_map_flush(struct bpf_map *map)
> * update happens in parallel here a dev_put wont happen until after reading the
> * ifindex.
> */
> -struct net_device *__dev_map_lookup_elem(struct bpf_map *map, u32 key)
> +struct bpf_dtab_netdev *__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 *dev;
> + struct bpf_dtab_netdev *obj;
>
> if (key >= map->max_entries)
> return NULL;
>
> - dev = READ_ONCE(dtab->netdev_map[key]);
> - return dev ? dev->dev : NULL;
> + obj = READ_ONCE(dtab->netdev_map[key]);
> + return obj;
> +}
> +
> +int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp)
> +{
> + struct net_device *dev = dst->dev;
> + struct xdp_frame *xdpf;
> + int err;
> +
> + if (!dev->netdev_ops->ndo_xdp_xmit)
> + return -EOPNOTSUPP;
> +
> + xdpf = convert_to_xdp_frame(xdp);
> + if (unlikely(!xdpf))
> + return -EOVERFLOW;
> +
> + /* TODO: implement a bulking/enqueue step later */
> + err = dev->netdev_ops->ndo_xdp_xmit(dev, xdpf);
> + if (err)
> + return err;
> +
> + return 0;
The 'err' is just unnecessary, lets just do:
return dev->netdev_ops->ndo_xdp_xmit(dev, xdpf);
Later after the other patches this becomes:
return bq_enqueue(dst, xdpf, dev_rx);
> }
>
> static void *dev_map_lookup_elem(struct bpf_map *map, void *key)
> {
> - struct net_device *dev = __dev_map_lookup_elem(map, *(u32 *)key);
> + struct bpf_dtab_netdev *obj = __dev_map_lookup_elem(map, *(u32 *)key);
> + struct net_device *dev = dev = obj ? obj->dev : NULL;
>
> return dev ? &dev->ifindex : NULL;
> }
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 6d0d1560bd70..1447ec94ef74 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3061,20 +3061,9 @@ static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
>
> switch (map->map_type) {
> case BPF_MAP_TYPE_DEVMAP: {
> - struct net_device *dev = fwd;
> - struct xdp_frame *xdpf;
> + struct bpf_dtab_netdev *dst = fwd;
>
> - if (!dev->netdev_ops->ndo_xdp_xmit)
> - return -EOPNOTSUPP;
> -
> - xdpf = convert_to_xdp_frame(xdp);
> - if (unlikely(!xdpf))
> - return -EOVERFLOW;
> -
> - /* TODO: move to inside map code instead, for bulk support
> - * err = dev_map_enqueue(dev, xdp);
> - */
> - err = dev->netdev_ops->ndo_xdp_xmit(dev, xdpf);
> + err = dev_map_enqueue(dst, xdp);
> if (err)
> return err;
> __dev_map_insert_ctx(map, index);
>
Powered by blists - more mailing lists