| Message ID | 1528877178-2521-1-git-send-email-makita.toshiaki@lab.ntt.co.jp |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] xdp: Fix handling of devmap in generic XDP | expand |
Hi Toshiaki,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf/master]
url: https://github.com/0day-ci/linux/commits/Toshiaki-Makita/xdp-Fix-handling-of-devmap-in-generic-XDP/20180613-161204
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
config: parisc-c3000_defconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=parisc
All warnings (new ones prefixed by >>):
In file included from net/bpf/test_run.c:7:0:
>> include/linux/bpf.h:593:16: warning: 'struct sk_buff' declared inside parameter list will not be visible outside of this definition or declaration
struct sk_buff *skb,
^~~~~~~
vim +593 include/linux/bpf.h
591
592 static inline int dev_map_generic_redirect(struct bpf_dtab_netdev *dst,
> 593 struct sk_buff *skb,
594 struct bpf_prog *xdp_prog)
595 {
596 return 0;
597 }
598
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Toshiaki,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf/master]
url: https://github.com/0day-ci/linux/commits/Toshiaki-Makita/xdp-Fix-handling-of-devmap-in-generic-XDP/20180613-161204
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
config: i386-randconfig-a1-201823 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from net//bpf/test_run.c:7:0:
>> include/linux/bpf.h:594:16: warning: 'struct sk_buff' declared inside parameter list
struct bpf_prog *xdp_prog)
^
>> include/linux/bpf.h:594:16: warning: its scope is only this definition or declaration, which is probably not what you want
vim +594 include/linux/bpf.h
591
592 static inline int dev_map_generic_redirect(struct bpf_dtab_netdev *dst,
593 struct sk_buff *skb,
> 594 struct bpf_prog *xdp_prog)
595 {
596 return 0;
597 }
598
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 2018/06/13 18:27, kbuild test robot wrote: > Hi Toshiaki, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on bpf/master] > > url: https://github.com/0day-ci/linux/commits/Toshiaki-Makita/xdp-Fix-handling-of-devmap-in-generic-XDP/20180613-161204 > base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master > config: i386-randconfig-a1-201823 (attached as .config) > compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > All warnings (new ones prefixed by >>): > > In file included from net//bpf/test_run.c:7:0: >>> include/linux/bpf.h:594:16: warning: 'struct sk_buff' declared inside parameter list > struct bpf_prog *xdp_prog) > ^ >>> include/linux/bpf.h:594:16: warning: its scope is only this definition or declaration, which is probably not what you want > > vim +594 include/linux/bpf.h > > 591 > 592 static inline int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, > 593 struct sk_buff *skb, > > 594 struct bpf_prog *xdp_prog) > 595 { > 596 return 0; > 597 } > 598 Ugh I did build test for the entire tree with CONFIG_BPF_SYSCALL, and net/core and kernel/bpf without CONFIG_BPF_SYSCALL but not net/bpf. I'll make sure to test the entire tree next. will send v2.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 995c3b1..2fe3aa1 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -487,6 +487,7 @@ static inline void bpf_long_memcpy(void *dst, const void *src, u32 size) void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth); /* Map specifics */ +struct sk_buff; struct xdp_buff; struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key); @@ -494,6 +495,8 @@ static inline void bpf_long_memcpy(void *dst, const void *src, u32 size) void __dev_map_flush(struct bpf_map *map); int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp, struct net_device *dev_rx); +int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb, + struct bpf_prog *xdp_prog); 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); @@ -586,6 +589,13 @@ int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp, return 0; } +static inline int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, + struct sk_buff *skb, + struct bpf_prog *xdp_prog) +{ + return 0; +} + static inline struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key) { diff --git a/include/linux/filter.h b/include/linux/filter.h index 45fc0f5..8ddff1f 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -19,6 +19,7 @@ #include <linux/cryptohash.h> #include <linux/set_memory.h> #include <linux/kallsyms.h> +#include <linux/if_vlan.h> #include <net/sch_generic.h> @@ -786,6 +787,21 @@ static inline bool bpf_dump_raw_ok(void) struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off, const struct bpf_insn *patch, u32 len); +static inline int __xdp_generic_ok_fwd_dev(struct sk_buff *skb, + struct net_device *fwd) +{ + unsigned int len; + + if (unlikely(!(fwd->flags & IFF_UP))) + return -ENETDOWN; + + len = fwd->mtu + fwd->hard_header_len + VLAN_HLEN; + if (skb->len > len) + return -EMSGSIZE; + + return 0; +} + /* The pair of xdp_do_redirect and xdp_do_flush_map MUST be called in the * same cpu context. Further for best results no more than a single map * for the do_redirect/do_flush pair should be used. This limitation is diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index a7cc7b3..642c97f 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -345,6 +345,20 @@ int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp, return bq_enqueue(dst, xdpf, dev_rx); } +int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb, + struct bpf_prog *xdp_prog) +{ + int err; + + err = __xdp_generic_ok_fwd_dev(skb, dst->dev); + if (unlikely(err)) + return err; + skb->dev = dst->dev; + generic_xdp_tx(skb, xdp_prog); + + return 0; +} + static void *dev_map_lookup_elem(struct bpf_map *map, void *key) { struct bpf_dtab_netdev *obj = __dev_map_lookup_elem(map, *(u32 *)key); diff --git a/net/core/filter.c b/net/core/filter.c index 3d9ba7e..e7f12e9 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3214,20 +3214,6 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, } EXPORT_SYMBOL_GPL(xdp_do_redirect); -static int __xdp_generic_ok_fwd_dev(struct sk_buff *skb, struct net_device *fwd) -{ - unsigned int len; - - if (unlikely(!(fwd->flags & IFF_UP))) - return -ENETDOWN; - - len = fwd->mtu + fwd->hard_header_len + VLAN_HLEN; - if (skb->len > len) - return -EMSGSIZE; - - return 0; -} - static int xdp_do_generic_redirect_map(struct net_device *dev, struct sk_buff *skb, struct xdp_buff *xdp, @@ -3256,10 +3242,11 @@ static int xdp_do_generic_redirect_map(struct net_device *dev, } if (map->map_type == BPF_MAP_TYPE_DEVMAP) { - if (unlikely((err = __xdp_generic_ok_fwd_dev(skb, fwd)))) + struct bpf_dtab_netdev *dst = fwd; + + err = dev_map_generic_redirect(dst, skb, xdp_prog); + if (unlikely(err)) goto err; - skb->dev = fwd; - generic_xdp_tx(skb, xdp_prog); } else if (map->map_type == BPF_MAP_TYPE_XSKMAP) { struct xdp_sock *xs = fwd;
Commit 67f29e07e131 ("bpf: devmap introduce dev_map_enqueue") changed the return value type of __devmap_lookup_elem() from struct net_device * to struct bpf_dtab_netdev * but forgot to modify generic XDP code accordingly. Thus generic XDP incorrectly used struct bpf_dtab_netdev where struct net_device is expected, then skb->dev was set to invalid value. Fixes: 67f29e07e131 ("bpf: devmap introduce dev_map_enqueue") Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> --- include/linux/bpf.h | 10 ++++++++++ include/linux/filter.h | 16 ++++++++++++++++ kernel/bpf/devmap.c | 14 ++++++++++++++ net/core/filter.c | 21 ++++----------------- 4 files changed, 44 insertions(+), 17 deletions(-)