netdev - Re: [RFC PATCH 00/12] Implement XDP bpf

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5967A71F.9010409@gmail.com>
Date:   Thu, 13 Jul 2017 10:00:15 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
CC:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        andy@...yhouse.net, daniel@...earbox.net, ast@...com,
        alexander.duyck@...il.com, bjorn.topel@...el.com,
        jakub.kicinski@...ronome.com, ecree@...arflare.com,
        sgoutham@...ium.com, Yuval.Mintz@...ium.com, saeedm@...lanox.com
Subject: Re: [RFC PATCH 00/12] Implement XDP bpf_redirect vairants

On 07/13/2017 09:16 AM, Jesper Dangaard Brouer wrote:
> On Thu, 13 Jul 2017 13:14:30 +0200
> Jesper Dangaard Brouer <brouer@...hat.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,
>