[PATCH net-next] xdp: implement xdp_redirect_map for generic XDP

[PATCH net-next] xdp: implement xdp_redirect_map for generic XDP

[Jesper Dangaard Brouer]

Using bpf_redirect_map is allowed for generic XDP programs, but the
appropriate map lookup was never performed in xdp_do_generic_redirect().

Instead the map-index is directly used as the ifindex.  For the
xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
sending on ifindex 0 which isn't valid, resulting in getting SKB
packets dropped.  Thus, the reported performance numbers are wrong in
commit 24251c264798 ("samples/bpf: add option for native and skb mode
for redirect apps") for the 'xdp_redirect_map -S' case.

It might seem innocent this was lacking, but it can actually crash the
kernel.  The potential crash is caused by not consuming redirect_info->map.
The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map
pointer, which will survive even after unloading the xdp bpf_prog and
deallocating the devmap data-structure.  This leaves a dead map
pointer around.  The kernel will crash when loading the xdp_redirect
sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)
and returns XDP_REDIRECT, which will cause it to dereference the map
pointer.

Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 net/core/filter.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 5912c738a7b2..6a4745bf2c9f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);
 
+static int xdp_do_generic_redirect_map(struct net_device *dev,
+				       struct sk_buff *skb,
+				       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;
+
+	ri->ifindex = 0;
+	ri->map = NULL;
+
+	fwd = __dev_map_lookup_elem(map, index);
+	if (!fwd) {
+		err = -EINVAL;
+		goto err;
+	}
+	skb->dev = fwd;
+	_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
+	return 0;
+err:
+	_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
+	return err;
+}
+
 int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 			    struct bpf_prog *xdp_prog)
 {
@@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 	unsigned int len;
 	int err = 0;
 
+	if (ri->map)
+		return xdp_do_generic_redirect_map(dev, skb, xdp_prog);
+
 	fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
 	if (unlikely(!fwd)) {

Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP

[Andy Gospodarek]

On Wed, Sep 6, 2017 at 11:26 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> Using bpf_redirect_map is allowed for generic XDP programs, but the
> appropriate map lookup was never performed in xdp_do_generic_redirect().
>
> Instead the map-index is directly used as the ifindex.  For the
> xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
> sending on ifindex 0 which isn't valid, resulting in getting SKB
> packets dropped.  Thus, the reported performance numbers are wrong in
> commit 24251c264798 ("samples/bpf: add option for native and skb mode
> for redirect apps") for the 'xdp_redirect_map -S' case.
>
> It might seem innocent this was lacking, but it can actually crash the
> kernel.  The potential crash is caused by not consuming redirect_info->map.
> The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map
> pointer, which will survive even after unloading the xdp bpf_prog and
> deallocating the devmap data-structure.  This leaves a dead map
> pointer around.  The kernel will crash when loading the xdp_redirect
> sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)
> and returns XDP_REDIRECT, which will cause it to dereference the map
> pointer.

Nice catch!

Since 'net-next' is closed and this is a bugfix it seems like this is
a good candidate for 'net' right?

>
> Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
> Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Acked-by: Andy Gospodarek <andy@greyhouse.net>


> ---
>  net/core/filter.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 5912c738a7b2..6a4745bf2c9f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>  }
>  EXPORT_SYMBOL_GPL(xdp_do_redirect);
>
> +static int xdp_do_generic_redirect_map(struct net_device *dev,
> +                                      struct sk_buff *skb,
> +                                      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;
> +
> +       ri->ifindex = 0;
> +       ri->map = NULL;
> +
> +       fwd = __dev_map_lookup_elem(map, index);
> +       if (!fwd) {
> +               err = -EINVAL;
> +               goto err;
> +       }
> +       skb->dev = fwd;
> +       _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
> +       return 0;
> +err:
> +       _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
> +       return err;
> +}
> +
>  int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>                             struct bpf_prog *xdp_prog)
>  {
> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>         unsigned int len;
>         int err = 0;
>
> +       if (ri->map)
> +               return xdp_do_generic_redirect_map(dev, skb, xdp_prog);
> +
>         fwd = dev_get_by_index_rcu(dev_net(dev), index);
>         ri->ifindex = 0;
>         if (unlikely(!fwd)) {
>

Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP

[Jesper Dangaard Brouer]

On Wed, 6 Sep 2017 11:44:18 -0400
Andy Gospodarek <andy@greyhouse.net> wrote:

> On Wed, Sep 6, 2017 at 11:26 AM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > Using bpf_redirect_map is allowed for generic XDP programs, but the
> > appropriate map lookup was never performed in xdp_do_generic_redirect().
> >
> > Instead the map-index is directly used as the ifindex.  For the
> > xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
> > sending on ifindex 0 which isn't valid, resulting in getting SKB
> > packets dropped.  Thus, the reported performance numbers are wrong in
> > commit 24251c264798 ("samples/bpf: add option for native and skb mode
> > for redirect apps") for the 'xdp_redirect_map -S' case.
> >
> > It might seem innocent this was lacking, but it can actually crash the
> > kernel.  The potential crash is caused by not consuming redirect_info->map.
> > The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map
> > pointer, which will survive even after unloading the xdp bpf_prog and
> > deallocating the devmap data-structure.  This leaves a dead map
> > pointer around.  The kernel will crash when loading the xdp_redirect
> > sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)
> > and returns XDP_REDIRECT, which will cause it to dereference the map
> > pointer.  
> 
> Nice catch!
> 
> Since 'net-next' is closed and this is a bugfix it seems like this is
> a good candidate for 'net' right?

Yes, I know 'net-next' is closed, but 'net' doesn't contain the
XDP_REDIRECT code yet... thus I had to base it on net-next ;-)


> >
> > Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
> > Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>  
> 
> Acked-by: Andy Gospodarek <andy@greyhouse.net>

Thanks
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP

[Daniel Borkmann]

On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:
> Using bpf_redirect_map is allowed for generic XDP programs, but the
> appropriate map lookup was never performed in xdp_do_generic_redirect().
>
> Instead the map-index is directly used as the ifindex.  For the

Good point, but ...

[...]
>   net/core/filter.c |   29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 5912c738a7b2..6a4745bf2c9f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>   }
>   EXPORT_SYMBOL_GPL(xdp_do_redirect);
>
> +static int xdp_do_generic_redirect_map(struct net_device *dev,
> +				       struct sk_buff *skb,
> +				       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;
> +
> +	ri->ifindex = 0;
> +	ri->map = NULL;
> +
> +	fwd = __dev_map_lookup_elem(map, index);
> +	if (!fwd) {
> +		err = -EINVAL;
> +		goto err;
> +	}
> +	skb->dev = fwd;
> +	_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
> +	return 0;
> +err:
> +	_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
> +	return err;
> +}
> +
>   int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>   			    struct bpf_prog *xdp_prog)
>   {
> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>   	unsigned int len;
>   	int err = 0;
>
> +	if (ri->map)
> +		return xdp_do_generic_redirect_map(dev, skb, xdp_prog);

This is not quite correct. Really, the only thing you want
to do here is more or less ...

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);
	struct bpf_map *map = ri->map;
	u32 index = ri->ifindex;
	struct net_device *fwd;
	unsigned int len;
	int err = 0;

	ri->ifindex = 0;
	ri->map = NULL;

	if (map)
		fwd = __dev_map_lookup_elem(map, index);
	else
		fwd = dev_get_by_index_rcu(dev_net(dev), index);
	if (unlikely(!fwd)) {
		err = -EINVAL;
		goto err;
	}
[...]

... such that you have a common path to also do the IFF_UP
and MTU checks that are done here, but otherwise omitted in
your patch.

Otherwise it looks good, but note that it also doesn't really
resolve the issue you mention wrt stale map pointers by the
way. This would need a different way to clear out the pointers
from redirect_info, I'm thinking when we have devmap dismantle
time after RCU grace period we should check whether there are
still stale pointers from this map around and clear them under
disabled preemption, but need to brainstorm a bit more on that
first.

>   	fwd = dev_get_by_index_rcu(dev_net(dev), index);
>   	ri->ifindex = 0;
>   	if (unlikely(!fwd)) {
>

Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP

[Daniel Borkmann]

On 09/06/2017 06:24 PM, Daniel Borkmann wrote:
[...]
> Otherwise it looks good, but note that it also doesn't really
> resolve the issue you mention wrt stale map pointers by the
> way. This would need a different way to clear out the pointers
> from redirect_info, I'm thinking when we have devmap dismantle
> time after RCU grace period we should check whether there are
> still stale pointers from this map around and clear them under
> disabled preemption, but need to brainstorm a bit more on that
> first.

Scratch that approach, doesn't work. So thinking bit more on
this, what we could do here is the following: verifier knows
we called bpf_xdp_redirect_map() helper, so it could do a small
insn rewrite in the sense that it fills R4 with a pointer to
the bpf_prog. We have that at verification time anyway and R4
is allowed to be populated since we scratch it per convention.
Then, the helper would store the prog pointer in struct redirect_info.
Later in xdp_do_*_redirect() we check whether the redirect_info's
prog pointer is the same as passed xdp_prog pointer, and if
that's the case then all good, since the prog holds a ref on
the map anyway, if they are not equal in the unlikely case, it
means stale pointer, so we bail out right there. That would
work imo, will see to code it up and check it out.

Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP

[Jesper Dangaard Brouer]

On Wed, 06 Sep 2017 18:24:07 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:
> > Using bpf_redirect_map is allowed for generic XDP programs, but the
> > appropriate map lookup was never performed in xdp_do_generic_redirect().
> >
> > Instead the map-index is directly used as the ifindex.  For the  
> 
> Good point, but ...
> 
> [...]
> >   net/core/filter.c |   29 +++++++++++++++++++++++++++++
> >   1 file changed, 29 insertions(+)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 5912c738a7b2..6a4745bf2c9f 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> >   }
> >   EXPORT_SYMBOL_GPL(xdp_do_redirect);
> >
> > +static int xdp_do_generic_redirect_map(struct net_device *dev,
> > +				       struct sk_buff *skb,
> > +				       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;
> > +
> > +	ri->ifindex = 0;
> > +	ri->map = NULL;
> > +
> > +	fwd = __dev_map_lookup_elem(map, index);
> > +	if (!fwd) {
> > +		err = -EINVAL;
> > +		goto err;
> > +	}
> > +	skb->dev = fwd;
> > +	_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
> > +	return 0;
> > +err:
> > +	_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
> > +	return err;
> > +}
> > +
> >   int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> >   			    struct bpf_prog *xdp_prog)
> >   {
> > @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> >   	unsigned int len;
> >   	int err = 0;
> >
> > +	if (ri->map)
> > +		return xdp_do_generic_redirect_map(dev, skb, xdp_prog);  
> 
> This is not quite correct. Really, the only thing you want
> to do here is more or less ...
> 
> 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);
> 	struct bpf_map *map = ri->map;
> 	u32 index = ri->ifindex;
> 	struct net_device *fwd;
> 	unsigned int len;
> 	int err = 0;
> 
> 	ri->ifindex = 0;
> 	ri->map = NULL;
> 
> 	if (map)
> 		fwd = __dev_map_lookup_elem(map, index);
> 	else
> 		fwd = dev_get_by_index_rcu(dev_net(dev), index);
> 	if (unlikely(!fwd)) {
> 		err = -EINVAL;
> 		goto err;
> 	}
> [...]
> 
> ... such that you have a common path to also do the IFF_UP
> and MTU checks that are done here, but otherwise omitted in
> your patch.

Ah, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by
xdp_do_redirect_map).


> Otherwise it looks good, but note that it also doesn't really
> resolve the issue you mention wrt stale map pointers by the
> way.  [...]

I actually discovered more cases where we can crash the kernel :-(

E.g. driver not supporting XDP_REDIRECT, are still allowed to load an
XDP bpf_prog that call bpf_redirect_map() and set the ->map pointer,
but it will never call xdp_do_redirect() (which is responsible for
clearing/consuming ->map pointer).

Another case: You can also call bpf_redirect_map() and then NOT return
XDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP

[John Fastabend]

On 09/06/2017 11:18 AM, Jesper Dangaard Brouer wrote:
> On Wed, 06 Sep 2017 18:24:07 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
>> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:
>>> Using bpf_redirect_map is allowed for generic XDP programs, but the
>>> appropriate map lookup was never performed in xdp_do_generic_redirect().
>>>
>>> Instead the map-index is directly used as the ifindex.  For the  
>>
>> Good point, but ...
>>
>> [...]
>>>   net/core/filter.c |   29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 5912c738a7b2..6a4745bf2c9f 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>>>   }
>>>   EXPORT_SYMBOL_GPL(xdp_do_redirect);
>>>
>>> +static int xdp_do_generic_redirect_map(struct net_device *dev,
>>> +				       struct sk_buff *skb,
>>> +				       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;
>>> +
>>> +	ri->ifindex = 0;
>>> +	ri->map = NULL;
>>> +
>>> +	fwd = __dev_map_lookup_elem(map, index);
>>> +	if (!fwd) {
>>> +		err = -EINVAL;
>>> +		goto err;
>>> +	}
>>> +	skb->dev = fwd;
>>> +	_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
>>> +	return 0;
>>> +err:
>>> +	_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
>>> +	return err;
>>> +}
>>> +
>>>   int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>>   			    struct bpf_prog *xdp_prog)
>>>   {
>>> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>>   	unsigned int len;
>>>   	int err = 0;
>>>
>>> +	if (ri->map)
>>> +		return xdp_do_generic_redirect_map(dev, skb, xdp_prog);  
>>
>> This is not quite correct. Really, the only thing you want
>> to do here is more or less ...
>>
>> 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);
>> 	struct bpf_map *map = ri->map;
>> 	u32 index = ri->ifindex;
>> 	struct net_device *fwd;
>> 	unsigned int len;
>> 	int err = 0;
>>
>> 	ri->ifindex = 0;
>> 	ri->map = NULL;
>>
>> 	if (map)
>> 		fwd = __dev_map_lookup_elem(map, index);
>> 	else
>> 		fwd = dev_get_by_index_rcu(dev_net(dev), index);
>> 	if (unlikely(!fwd)) {
>> 		err = -EINVAL;
>> 		goto err;
>> 	}
>> [...]
>>
>> ... such that you have a common path to also do the IFF_UP
>> and MTU checks that are done here, but otherwise omitted in
>> your patch.
> 
> Ah, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by
> xdp_do_redirect_map).
> 
> 
>> Otherwise it looks good, but note that it also doesn't really
>> resolve the issue you mention wrt stale map pointers by the
>> way.  [...]
> 
> I actually discovered more cases where we can crash the kernel :-(
> 
> E.g. driver not supporting XDP_REDIRECT, are still allowed to load an
> XDP bpf_prog that call bpf_redirect_map() and set the ->map pointer,
> but it will never call xdp_do_redirect() (which is responsible for
> clearing/consuming ->map pointer).
> 
> Another case: You can also call bpf_redirect_map() and then NOT return
> XDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it).
> 

I think we can cover both these cases with previous suggestion to check
prog pointers. Working up a patch now.

Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP

[Daniel Borkmann]

On 09/06/2017 08:42 PM, John Fastabend wrote:
> On 09/06/2017 11:18 AM, Jesper Dangaard Brouer wrote:
>> On Wed, 06 Sep 2017 18:24:07 +0200
>> Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:
>>>> Using bpf_redirect_map is allowed for generic XDP programs, but the
>>>> appropriate map lookup was never performed in xdp_do_generic_redirect().
>>>>
>>>> Instead the map-index is directly used as the ifindex.  For the
>>>
>>> Good point, but ...
>>>
>>> [...]
>>>>    net/core/filter.c |   29 +++++++++++++++++++++++++++++
>>>>    1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index 5912c738a7b2..6a4745bf2c9f 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(xdp_do_redirect);
>>>>
>>>> +static int xdp_do_generic_redirect_map(struct net_device *dev,
>>>> +				       struct sk_buff *skb,
>>>> +				       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;
>>>> +
>>>> +	ri->ifindex = 0;
>>>> +	ri->map = NULL;
>>>> +
>>>> +	fwd = __dev_map_lookup_elem(map, index);
>>>> +	if (!fwd) {
>>>> +		err = -EINVAL;
>>>> +		goto err;
>>>> +	}
>>>> +	skb->dev = fwd;
>>>> +	_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
>>>> +	return 0;
>>>> +err:
>>>> +	_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
>>>> +	return err;
>>>> +}
>>>> +
>>>>    int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>>>    			    struct bpf_prog *xdp_prog)
>>>>    {
>>>> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>>>    	unsigned int len;
>>>>    	int err = 0;
>>>>
>>>> +	if (ri->map)
>>>> +		return xdp_do_generic_redirect_map(dev, skb, xdp_prog);
>>>
>>> This is not quite correct. Really, the only thing you want
>>> to do here is more or less ...
>>>
>>> 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);
>>> 	struct bpf_map *map = ri->map;
>>> 	u32 index = ri->ifindex;
>>> 	struct net_device *fwd;
>>> 	unsigned int len;
>>> 	int err = 0;
>>>
>>> 	ri->ifindex = 0;
>>> 	ri->map = NULL;
>>>
>>> 	if (map)
>>> 		fwd = __dev_map_lookup_elem(map, index);
>>> 	else
>>> 		fwd = dev_get_by_index_rcu(dev_net(dev), index);
>>> 	if (unlikely(!fwd)) {
>>> 		err = -EINVAL;
>>> 		goto err;
>>> 	}
>>> [...]
>>>
>>> ... such that you have a common path to also do the IFF_UP
>>> and MTU checks that are done here, but otherwise omitted in
>>> your patch.
>>
>> Ah, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by
>> xdp_do_redirect_map).
>>
>>> Otherwise it looks good, but note that it also doesn't really
>>> resolve the issue you mention wrt stale map pointers by the
>>> way.  [...]
>>
>> I actually discovered more cases where we can crash the kernel :-(
>>
>> E.g. driver not supporting XDP_REDIRECT, are still allowed to load an
>> XDP bpf_prog that call bpf_redirect_map() and set the ->map pointer,
>> but it will never call xdp_do_redirect() (which is responsible for
>> clearing/consuming ->map pointer).
>>
>> Another case: You can also call bpf_redirect_map() and then NOT return
>> XDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it).
>
> I think we can cover both these cases with previous suggestion to check
> prog pointers. Working up a patch now.

Yep, they would both be covered.