Message ID | 20170713181644.2d421e4d@redhat.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On 07/13/2017 09:16 AM, Jesper Dangaard Brouer wrote: > On Thu, 13 Jul 2017 13:14:30 +0200 > Jesper Dangaard Brouer <brouer@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, >
From: John Fastabend <john.fastabend@gmail.com> Date: Thu, 13 Jul 2017 10:00:15 -0700 > On 07/13/2017 09:16 AM, Jesper Dangaard Brouer wrote: >> On Thu, 13 Jul 2017 13:14:30 +0200 >> Jesper Dangaard Brouer <brouer@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. I am so looking forward to merging this, great work everyone.
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,