On 07/13/2017 09:16 AM, Jesper Dangaard Brouer wrote:
> On Thu, 13 Jul 2017 13:14:30 +0200
> Jesper Dangaard Brouer <bro...@redhat.com> wrote:
>
>> I'm still getting crashes (but much harder to provoke), but I figured
>> out why. We sort of missed one case, where map_to_flush gets set, when
>> the ndo_xdp_xmit() call starts to fail, and the ixgbe driver then
>> forgets to call xdp_do_flush_map, if all packets in that NAPI cycle
>> failed. We could blame the driver, but yhe clean solution is making
>> sure, that we don't set map_to_flush when the __bpf_tx_xdp() call
>> fails. It should also handle the other case I fixed .... I'll cleanup
>> my PoC-fix patch, test it and provide it here.
>
> I changed flow in the function to be:
Great, I'll merge this, the other couple fixes, and the bitops optimization and
hopefully then we are set. I'll post a v2 and we can do some final checks.
Thanks!
John
>
> int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
> struct bpf_prog *xdp_prog)
> {
> struct redirect_info *ri = this_cpu_ptr(&redirect_info);
> struct bpf_map *map = ri->map;
> u32 index = ri->ifindex;
> struct net_device *fwd;
> int err = -EINVAL;
>
> ri->ifindex = 0;
> ri->map = NULL;
>
> fwd = __dev_map_lookup_elem(map, index);
> if (!fwd)
> goto out;
>
> if (ri->map_to_flush && (ri->map_to_flush != map))
> xdp_do_flush_map();
>
> err = __bpf_tx_xdp(fwd, map, xdp, index);
> if (likely(!err))
> ri->map_to_flush = map;
>
> out:
> trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT);
> return err;
> }
>
>
> The diff is:
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 4ca895d6ed51..c50a7ec2cdab 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2483,26 +2483,25 @@ int xdp_do_redirect_map(struct net_device *dev,
> struct xdp_buff *xdp,
> struct bpf_map *map = ri->map;
> u32 index = ri->ifindex;
> struct net_device *fwd;
> + int err = -EINVAL;
> +
> + ri->ifindex = 0;
> + ri->map = NULL;
>
> fwd = __dev_map_lookup_elem(map, index);
> if (!fwd)
> goto out;
>
> - ri->ifindex = 0;
> -
> if (ri->map_to_flush && (ri->map_to_flush != map))
> xdp_do_flush_map();
>
> - ri->map_to_flush = map;
> - ri->map = NULL;
> + err = __bpf_tx_xdp(fwd, map, xdp, index);
> + if (likely(!err))
> + ri->map_to_flush = map;
>
> - trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT);
> -
> - return __bpf_tx_xdp(fwd, map, xdp, index);
> out:
> - ri->ifindex = 0;
> - ri->map = NULL;
> - return -EINVAL;
> + trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT);
> + return err;
> }
>
> int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>
