From: John Fastabend <john.fastabend@gmail•com>
To: Jesper Dangaard Brouer <brouer@redhat•com>,
Daniel Borkmann <daniel@iogearbox•net>
Cc: netdev@vger•kernel.org, "David S. Miller" <davem@davemloft•net>,
Andy Gospodarek <andy@greyhouse•net>
Subject: Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP
Date: Wed, 06 Sep 2017 11:42:18 -0700 [thread overview]
Message-ID: <59B0418A.5090609@gmail.com> (raw)
In-Reply-To: <20170906201835.7bc26f4f@redhat.com>
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.
next prev parent reply other threads:[~2017-09-06 18:42 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-06 15:26 [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP Jesper Dangaard Brouer
2017-09-06 15:44 ` Andy Gospodarek
2017-09-06 16:01 ` Jesper Dangaard Brouer
2017-09-06 16:24 ` Daniel Borkmann
2017-09-06 17:02 ` Daniel Borkmann
2017-09-06 18:18 ` Jesper Dangaard Brouer
2017-09-06 18:42 ` John Fastabend [this message]
2017-09-06 18:51 ` Daniel Borkmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=59B0418A.5090609@gmail.com \
--to=john.fastabend@gmail$(echo .)com \
--cc=andy@greyhouse$(echo .)net \
--cc=brouer@redhat$(echo .)com \
--cc=daniel@iogearbox$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox