From d08adb82fda523cb51a767e2833c5c5bb0906162 Mon Sep 17 00:00:00 2001 From: Jesper Dangaard Brouer Date: Thu, 24 Aug 2017 12:33:03 +0200 Subject: xdp: remove bpf_warn_invalid_xdp_redirect Given there is a tracepoint that can track the error code of xdp_do_redirect calls, the WARN_ONCE in bpf_warn_invalid_xdp_redirect doesn't seem relevant any longer. Simply remove the function. Signed-off-by: Jesper Dangaard Brouer Acked-by: Daniel Borkmann Signed-off-by: David S. Miller --- net/core/filter.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/net/core/filter.c b/net/core/filter.c index 2a0d762a20d8..a5e5f31b41b9 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -2476,7 +2476,6 @@ static int __bpf_tx_xdp(struct net_device *dev, int err; if (!dev->netdev_ops->ndo_xdp_xmit) { - bpf_warn_invalid_xdp_redirect(dev->ifindex); return -EOPNOTSUPP; } @@ -2543,7 +2542,6 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, fwd = dev_get_by_index_rcu(dev_net(dev), index); ri->ifindex = 0; if (unlikely(!fwd)) { - bpf_warn_invalid_xdp_redirect(index); err = -EINVAL; goto out; } @@ -2564,7 +2562,6 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb) dev = dev_get_by_index_rcu(dev_net(dev), index); ri->ifindex = 0; if (unlikely(!dev)) { - bpf_warn_invalid_xdp_redirect(index); return -EINVAL; } @@ -3565,11 +3562,6 @@ void bpf_warn_invalid_xdp_action(u32 act) } EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action); -void bpf_warn_invalid_xdp_redirect(u32 ifindex) -{ - WARN_ONCE(1, "Illegal XDP redirect to unsupported device ifindex(%i)\n", ifindex); -} - static bool __is_valid_sock_ops_access(int off, int size) { if (off < 0 || off >= sizeof(struct bpf_sock_ops)) -- cgit v1.2.3 From 2facaad6000f2322eb40ca379aced31c957f0a41 Mon Sep 17 00:00:00 2001 From: Jesper Dangaard Brouer Date: Thu, 24 Aug 2017 12:33:08 +0200 Subject: xdp: make generic xdp redirect use tracepoint trace_xdp_redirect If the xdp_do_generic_redirect() call fails, it trigger the trace_xdp_exception tracepoint. It seems better to use the same tracepoint trace_xdp_redirect, as the native xdp_do_redirect{,_map} does. Signed-off-by: Jesper Dangaard Brouer Acked-by: Daniel Borkmann Signed-off-by: David S. Miller --- include/linux/filter.h | 3 ++- net/core/dev.c | 4 ++-- net/core/filter.c | 35 +++++++++++++++++++++++------------ 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/include/linux/filter.h b/include/linux/filter.h index 7015116331af..d29e58fde364 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -718,7 +718,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off, * because we only track one map and force a flush when the map changes. * This does not appear to be a real limitation for existing software. */ -int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb); +int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb, + struct bpf_prog *prog); int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, struct bpf_prog *prog); diff --git a/net/core/dev.c b/net/core/dev.c index 40b28e417072..270b54754821 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3953,7 +3953,8 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb) if (act != XDP_PASS) { switch (act) { case XDP_REDIRECT: - err = xdp_do_generic_redirect(skb->dev, skb); + err = xdp_do_generic_redirect(skb->dev, skb, + xdp_prog); if (err) goto out_redir; /* fallthru to submit skb */ @@ -3966,7 +3967,6 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb) } return XDP_PASS; out_redir: - trace_xdp_exception(skb->dev, xdp_prog, XDP_REDIRECT); kfree_skb(skb); return XDP_DROP; } diff --git a/net/core/filter.c b/net/core/filter.c index a5e5f31b41b9..a04680331033 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -2553,26 +2553,37 @@ out: } EXPORT_SYMBOL_GPL(xdp_do_redirect); -int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb) +int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb, + struct bpf_prog *xdp_prog) { struct redirect_info *ri = this_cpu_ptr(&redirect_info); - unsigned int len; u32 index = ri->ifindex; + struct net_device *fwd; + unsigned int len; + int err = 0; - dev = dev_get_by_index_rcu(dev_net(dev), index); + fwd = dev_get_by_index_rcu(dev_net(dev), index); ri->ifindex = 0; - if (unlikely(!dev)) { - return -EINVAL; + if (unlikely(!fwd)) { + err = -EINVAL; + goto out; } - if (unlikely(!(dev->flags & IFF_UP))) - return -ENETDOWN; - len = dev->mtu + dev->hard_header_len + VLAN_HLEN; - if (skb->len > len) - return -E2BIG; + if (unlikely(!(fwd->flags & IFF_UP))) { + err = -ENETDOWN; + goto out; + } - skb->dev = dev; - return 0; + len = fwd->mtu + fwd->hard_header_len + VLAN_HLEN; + if (skb->len > len) { + err = -EMSGSIZE; + goto out; + } + + skb->dev = fwd; +out: + trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err); + return err; } EXPORT_SYMBOL_GPL(xdp_do_generic_redirect); -- cgit v1.2.3 From 2886447dc5add28226451c163cd9ac1d1b3bf59a Mon Sep 17 00:00:00 2001 From: Jesper Dangaard Brouer Date: Thu, 24 Aug 2017 12:33:13 +0200 Subject: ixgbe: use return codes from ndo_xdp_xmit that are distinguishable For XDP_REDIRECT the use of return code -EINVAL is confusing, as it is used in three different cases. (1) When the index or ifindex lookup fails, and in the ixgbe driver (2) when link is down and (3) when XDP have not been enabled. The return code can be picked up by the tracepoint xdp:xdp_redirect for diagnosing why XDP_REDIRECT isn't working. Thus, there is a need different return codes to tell the issues apart. I'm considering using a specific err-code scheme for XDP_REDIRECT instead of using these errno codes. Signed-off-by: Jesper Dangaard Brouer Acked-by: Daniel Borkmann Signed-off-by: David S. Miller --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 8d3224ad6434..d962368d08d0 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -9849,14 +9849,14 @@ static int ixgbe_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp) int err; if (unlikely(test_bit(__IXGBE_DOWN, &adapter->state))) - return -EINVAL; + return -ENETDOWN; /* During program transitions its possible adapter->xdp_prog is assigned * but ring has not been configured yet. In this case simply abort xmit. */ ring = adapter->xdp_prog ? adapter->xdp_ring[smp_processor_id()] : NULL; if (unlikely(!ring)) - return -EINVAL; + return -ENXIO; err = ixgbe_xmit_xdp_ring(adapter, xdp); if (err != IXGBE_XDP_TX) -- cgit v1.2.3 From a873585587205750e7accfb2c93c29239ffa6e09 Mon Sep 17 00:00:00 2001 From: Jesper Dangaard Brouer Date: Thu, 24 Aug 2017 12:33:18 +0200 Subject: xdp: remove net_device names from xdp_redirect tracepoint There is too much overhead in the current trace_xdp_redirect tracepoint as it does strcpy and strlen on the net_device names. Besides, exposing the ifindex/index is actually the information that is needed in the tracepoint to diagnose issues. When a lookup fails (either ifindex or devmap index) then there is a need for saying which to_index that have issues. V2: Adjust args to be aligned with trace_xdp_exception. Signed-off-by: Jesper Dangaard Brouer Acked-by: Daniel Borkmann Signed-off-by: David S. Miller --- include/trace/events/xdp.h | 24 ++++++++++++------------ net/core/filter.c | 6 +++--- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h index 0e42e69f773b..cd37706c6f55 100644 --- a/include/trace/events/xdp.h +++ b/include/trace/events/xdp.h @@ -51,33 +51,33 @@ TRACE_EVENT(xdp_exception, TRACE_EVENT(xdp_redirect, - TP_PROTO(const struct net_device *from, - const struct net_device *to, - const struct bpf_prog *xdp, u32 act, int err), + TP_PROTO(const struct net_device *dev, + const struct bpf_prog *xdp, u32 act, + int to_index, int err), - TP_ARGS(from, to, xdp, act, err), + TP_ARGS(dev, xdp, act, to_index, err), TP_STRUCT__entry( - __string(name_from, from->name) - __string(name_to, to->name) __array(u8, prog_tag, 8) __field(u32, act) + __field(int, ifindex) + __field(int, to_index) __field(int, err) ), TP_fast_assign( BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag)); memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag)); - __assign_str(name_from, from->name); - __assign_str(name_to, to->name); - __entry->act = act; - __entry->err = err; + __entry->act = act; + __entry->ifindex = dev->ifindex; + __entry->to_index = to_index; + __entry->err = err; ), - TP_printk("prog=%s from=%s to=%s action=%s err=%d", + TP_printk("prog=%s action=%s ifindex=%d to_index=%d err=%d", __print_hex_str(__entry->prog_tag, 8), - __get_str(name_from), __get_str(name_to), __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB), + __entry->ifindex, __entry->to_index, __entry->err) ); #endif /* _TRACE_XDP_H */ diff --git a/net/core/filter.c b/net/core/filter.c index a04680331033..4bcd6baa80c9 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -2524,7 +2524,7 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp, if (likely(!err)) ri->map_to_flush = map; out: - trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err); + trace_xdp_redirect(dev, xdp_prog, XDP_REDIRECT, index, err); return err; } @@ -2548,7 +2548,7 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, err = __bpf_tx_xdp(fwd, NULL, xdp, 0); out: - trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err); + trace_xdp_redirect(dev, xdp_prog, XDP_REDIRECT, index, err); return err; } EXPORT_SYMBOL_GPL(xdp_do_redirect); @@ -2582,7 +2582,7 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb, skb->dev = fwd; out: - trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err); + trace_xdp_redirect(dev, xdp_prog, XDP_REDIRECT, index, err); return err; } EXPORT_SYMBOL_GPL(xdp_do_generic_redirect); -- cgit v1.2.3 From 315ec3990efd71f87e556cf7827a1ac2d565d5e8 Mon Sep 17 00:00:00 2001 From: Jesper Dangaard Brouer Date: Thu, 24 Aug 2017 12:33:23 +0200 Subject: xdp: get tracepoints xdp_exception and xdp_redirect in sync Remove the net_device string name from the xdp_exception tracepoint, like the xdp_redirect tracepoint. Align the TP_STRUCT to have common entries between these two tracepoint. Signed-off-by: Jesper Dangaard Brouer Acked-by: Daniel Borkmann Signed-off-by: David S. Miller --- include/trace/events/xdp.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h index cd37706c6f55..27cf2ef35f5f 100644 --- a/include/trace/events/xdp.h +++ b/include/trace/events/xdp.h @@ -31,22 +31,22 @@ TRACE_EVENT(xdp_exception, TP_ARGS(dev, xdp, act), TP_STRUCT__entry( - __string(name, dev->name) __array(u8, prog_tag, 8) __field(u32, act) + __field(int, ifindex) ), TP_fast_assign( BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag)); memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag)); - __assign_str(name, dev->name); - __entry->act = act; + __entry->act = act; + __entry->ifindex = dev->ifindex; ), - TP_printk("prog=%s device=%s action=%s", + TP_printk("prog=%s action=%s ifindex=%d", __print_hex_str(__entry->prog_tag, 8), - __get_str(name), - __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB)) + __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB), + __entry->ifindex) ); TRACE_EVENT(xdp_redirect, -- cgit v1.2.3