[PATCH net-next v2 0/2] xdp: Allow lookup into devmaps before redirect

[PATCH net-next v2 0/2] xdp: Allow lookup into devmaps before redirect

[Toke Høiland-Jørgensen]

When using the bpf_redirect_map() helper to redirect packets from XDP, the eBPF
program cannot currently know whether the redirect will succeed, which makes it
impossible to gracefully handle errors. To properly fix this will probably
require deeper changes to the way TX resources are allocated, but one thing that
is fairly straight forward to fix is to allow lookups into devmaps, so programs
can at least know when a redirect is *guaranteed* to fail because there is no
entry in the map. Currently, programs work around this by keeping a shadow map
of another type which indicates whether a map index is valid.

This series contains two changes that are complementary ways to fix this issue.
The first patch adds a flag to make the bpf_redirect_map() helper itself do a
lookup in the map and return XDP_PASS if the map index is unset, while the
second patch allows regular lookups into devmaps from eBPF programs.

The performance impact of both patches is negligible, in the sense that I cannot
measure it because the variance between test runs is higher than the difference
pre/post patch.

Changelog:

v2:
  - For patch 1, make it clear that the change works for any map type.
  - For patch 2, just use the new BPF_F_RDONLY_PROG flag to make the return
    value read-only.

---

Toke Høiland-Jørgensen (2):
      bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure
      devmap: Allow map lookups from eBPF


 include/uapi/linux/bpf.h |    8 ++++++++
 kernel/bpf/devmap.c      |    5 +++++
 kernel/bpf/verifier.c    |    7 ++-----
 net/core/filter.c        |   10 +++++++++-
 4 files changed, 24 insertions(+), 6 deletions(-)

[PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

The bpf_redirect_map() helper used by XDP programs doesn't return any
indication of whether it can successfully redirect to the map index it was
given. Instead, BPF programs have to track this themselves, leading to
programs using duplicate maps to track which entries are populated in the
devmap.

This patch adds a flag to the XDP version of the bpf_redirect_map() helper,
which makes the helper do a lookup in the map when called, and return
XDP_PASS if there is no value at the provided index.

With this, a BPF program can check the return code from the helper call and
react if it is XDP_PASS (by, for instance, substituting a different
redirect). This works for any type of map used for redirect.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/uapi/linux/bpf.h |    8 ++++++++
 net/core/filter.c        |   10 +++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 7c6aef253173..d57df4f0b837 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3098,6 +3098,14 @@ enum xdp_action {
 	XDP_REDIRECT,
 };
 
+/* Flags for bpf_xdp_redirect_map helper */
+
+/* If set, the help will check if the entry exists in the map and return
+ * XDP_PASS if it doesn't.
+ */
+#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
+#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
+
 /* user accessible metadata for XDP packet hook
  * new fields must be added to the end of this structure
  */
diff --git a/net/core/filter.c b/net/core/filter.c
index 55bfc941d17a..2e532a9b2605 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
 {
 	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
 
-	if (unlikely(flags))
+	if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
 		return XDP_ABORTED;
 
+	if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
+		void *val;
+
+		val = __xdp_map_lookup_elem(map, ifindex);
+		if (unlikely(!val))
+			return XDP_PASS;
+	}
+
 	ri->ifindex = ifindex;
 	ri->flags = flags;
 	WRITE_ONCE(ri->map, map);

Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Daniel Borkmann]

On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> The bpf_redirect_map() helper used by XDP programs doesn't return any
> indication of whether it can successfully redirect to the map index it was
> given. Instead, BPF programs have to track this themselves, leading to
> programs using duplicate maps to track which entries are populated in the
> devmap.
> 
> This patch adds a flag to the XDP version of the bpf_redirect_map() helper,
> which makes the helper do a lookup in the map when called, and return
> XDP_PASS if there is no value at the provided index.
> 
> With this, a BPF program can check the return code from the helper call and
> react if it is XDP_PASS (by, for instance, substituting a different
> redirect). This works for any type of map used for redirect.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  include/uapi/linux/bpf.h |    8 ++++++++
>  net/core/filter.c        |   10 +++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 7c6aef253173..d57df4f0b837 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3098,6 +3098,14 @@ enum xdp_action {
>  	XDP_REDIRECT,
>  };
>  
> +/* Flags for bpf_xdp_redirect_map helper */
> +
> +/* If set, the help will check if the entry exists in the map and return
> + * XDP_PASS if it doesn't.
> + */
> +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
> +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
> +
>  /* user accessible metadata for XDP packet hook
>   * new fields must be added to the end of this structure
>   */
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 55bfc941d17a..2e532a9b2605 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
>  {
>  	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>  
> -	if (unlikely(flags))
> +	if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
>  		return XDP_ABORTED;
>  
> +	if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
> +		void *val;
> +
> +		val = __xdp_map_lookup_elem(map, ifindex);
> +		if (unlikely(!val))
> +			return XDP_PASS;

Generally looks good to me, also the second part with the flag. Given we store into
the per-CPU scratch space and function like xdp_do_redirect() pick this up again, we
could even propagate val onwards and save a second lookup on the /same/ element (which
also avoids a race if the val was dropped from the map in the meantime). Given this
should all still be within RCU it should work. Perhaps it even makes sense to do the
lookup unconditionally inside bpf_xdp_redirect_map() helper iff we manage to do it
only once anyway?

> +	}
> +
>  	ri->ifindex = ifindex;
>  	ri->flags = flags;
>  	WRITE_ONCE(ri->map, map);
> 

Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Alexei Starovoitov]

On Thu, Jun 6, 2019 at 8:51 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
> > From: Toke Høiland-Jørgensen <toke@redhat.com>
> >
> > The bpf_redirect_map() helper used by XDP programs doesn't return any
> > indication of whether it can successfully redirect to the map index it was
> > given. Instead, BPF programs have to track this themselves, leading to
> > programs using duplicate maps to track which entries are populated in the
> > devmap.
> >
> > This patch adds a flag to the XDP version of the bpf_redirect_map() helper,
> > which makes the helper do a lookup in the map when called, and return
> > XDP_PASS if there is no value at the provided index.
> >
> > With this, a BPF program can check the return code from the helper call and
> > react if it is XDP_PASS (by, for instance, substituting a different
> > redirect). This works for any type of map used for redirect.
> >
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > ---
> >  include/uapi/linux/bpf.h |    8 ++++++++
> >  net/core/filter.c        |   10 +++++++++-
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 7c6aef253173..d57df4f0b837 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3098,6 +3098,14 @@ enum xdp_action {
> >       XDP_REDIRECT,
> >  };
> >
> > +/* Flags for bpf_xdp_redirect_map helper */
> > +
> > +/* If set, the help will check if the entry exists in the map and return
> > + * XDP_PASS if it doesn't.
> > + */
> > +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
> > +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
> > +
> >  /* user accessible metadata for XDP packet hook
> >   * new fields must be added to the end of this structure
> >   */
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 55bfc941d17a..2e532a9b2605 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
> >  {
> >       struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> >
> > -     if (unlikely(flags))
> > +     if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
> >               return XDP_ABORTED;
> >
> > +     if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
> > +             void *val;
> > +
> > +             val = __xdp_map_lookup_elem(map, ifindex);
> > +             if (unlikely(!val))
> > +                     return XDP_PASS;
>
> Generally looks good to me, also the second part with the flag. Given we store into
> the per-CPU scratch space and function like xdp_do_redirect() pick this up again, we
> could even propagate val onwards and save a second lookup on the /same/ element (which
> also avoids a race if the val was dropped from the map in the meantime). Given this
> should all still be within RCU it should work. Perhaps it even makes sense to do the
> lookup unconditionally inside bpf_xdp_redirect_map() helper iff we manage to do it
> only once anyway?

+1

also I don't think we really need a new flag here.
Yes, it could be considered an uapi change, but it
looks more like bugfix in uapi to me.
Since original behavior was so clunky to use.

Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Toke Høiland-Jørgensen]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Thu, Jun 6, 2019 at 8:51 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
>> > From: Toke Høiland-Jørgensen <toke@redhat.com>
>> >
>> > The bpf_redirect_map() helper used by XDP programs doesn't return any
>> > indication of whether it can successfully redirect to the map index it was
>> > given. Instead, BPF programs have to track this themselves, leading to
>> > programs using duplicate maps to track which entries are populated in the
>> > devmap.
>> >
>> > This patch adds a flag to the XDP version of the bpf_redirect_map() helper,
>> > which makes the helper do a lookup in the map when called, and return
>> > XDP_PASS if there is no value at the provided index.
>> >
>> > With this, a BPF program can check the return code from the helper call and
>> > react if it is XDP_PASS (by, for instance, substituting a different
>> > redirect). This works for any type of map used for redirect.
>> >
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> > ---
>> >  include/uapi/linux/bpf.h |    8 ++++++++
>> >  net/core/filter.c        |   10 +++++++++-
>> >  2 files changed, 17 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> > index 7c6aef253173..d57df4f0b837 100644
>> > --- a/include/uapi/linux/bpf.h
>> > +++ b/include/uapi/linux/bpf.h
>> > @@ -3098,6 +3098,14 @@ enum xdp_action {
>> >       XDP_REDIRECT,
>> >  };
>> >
>> > +/* Flags for bpf_xdp_redirect_map helper */
>> > +
>> > +/* If set, the help will check if the entry exists in the map and return
>> > + * XDP_PASS if it doesn't.
>> > + */
>> > +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
>> > +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
>> > +
>> >  /* user accessible metadata for XDP packet hook
>> >   * new fields must be added to the end of this structure
>> >   */
>> > diff --git a/net/core/filter.c b/net/core/filter.c
>> > index 55bfc941d17a..2e532a9b2605 100644
>> > --- a/net/core/filter.c
>> > +++ b/net/core/filter.c
>> > @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
>> >  {
>> >       struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>> >
>> > -     if (unlikely(flags))
>> > +     if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
>> >               return XDP_ABORTED;
>> >
>> > +     if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
>> > +             void *val;
>> > +
>> > +             val = __xdp_map_lookup_elem(map, ifindex);
>> > +             if (unlikely(!val))
>> > +                     return XDP_PASS;
>>
>> Generally looks good to me, also the second part with the flag. Given we store into
>> the per-CPU scratch space and function like xdp_do_redirect() pick this up again, we
>> could even propagate val onwards and save a second lookup on the /same/ element (which
>> also avoids a race if the val was dropped from the map in the meantime). Given this
>> should all still be within RCU it should work. Perhaps it even makes sense to do the
>> lookup unconditionally inside bpf_xdp_redirect_map() helper iff we manage to do it
>> only once anyway?
>
> +1
>
> also I don't think we really need a new flag here.
> Yes, it could be considered an uapi change, but it
> looks more like bugfix in uapi to me.
> Since original behavior was so clunky to use.

Hmm, the problem with this is that eBPF programs generally do something
like:

return bpf_redirect_map(map, idx, 0);

after having already modified the packet headers. This will get them a
return code of XDP_REDIRECT, and the lookup will then subsequently fail,
which returns in XDP_ABORTED in the driver, which you can catch with
tracing.

However, if we just change it to XDP_PASS, the packet will go up the
stack, but because it has already been modified the stack will drop it,
more or less invisibly.

So the question becomes, is that behaviour change really OK?

-Toke

Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Jonathan Lemon]

On 6 Jun 2019, at 9:15, Toke Høiland-Jørgensen wrote:

> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>
>> On Thu, Jun 6, 2019 at 8:51 AM Daniel Borkmann <daniel@iogearbox.net> 
>> wrote:
>>>
>>> On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
>>>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>
>>>> The bpf_redirect_map() helper used by XDP programs doesn't return 
>>>> any
>>>> indication of whether it can successfully redirect to the map index 
>>>> it was
>>>> given. Instead, BPF programs have to track this themselves, leading 
>>>> to
>>>> programs using duplicate maps to track which entries are populated 
>>>> in the
>>>> devmap.
>>>>
>>>> This patch adds a flag to the XDP version of the bpf_redirect_map() 
>>>> helper,
>>>> which makes the helper do a lookup in the map when called, and 
>>>> return
>>>> XDP_PASS if there is no value at the provided index.
>>>>
>>>> With this, a BPF program can check the return code from the helper 
>>>> call and
>>>> react if it is XDP_PASS (by, for instance, substituting a different
>>>> redirect). This works for any type of map used for redirect.
>>>>
>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>> ---
>>>>  include/uapi/linux/bpf.h |    8 ++++++++
>>>>  net/core/filter.c        |   10 +++++++++-
>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>> index 7c6aef253173..d57df4f0b837 100644
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -3098,6 +3098,14 @@ enum xdp_action {
>>>>       XDP_REDIRECT,
>>>>  };
>>>>
>>>> +/* Flags for bpf_xdp_redirect_map helper */
>>>> +
>>>> +/* If set, the help will check if the entry exists in the map and 
>>>> return
>>>> + * XDP_PASS if it doesn't.
>>>> + */
>>>> +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
>>>> +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
>>>> +
>>>>  /* user accessible metadata for XDP packet hook
>>>>   * new fields must be added to the end of this structure
>>>>   */
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index 55bfc941d17a..2e532a9b2605 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>> @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct 
>>>> bpf_map *, map, u32, ifindex,
>>>>  {
>>>>       struct bpf_redirect_info *ri = 
>>>> this_cpu_ptr(&bpf_redirect_info);
>>>>
>>>> -     if (unlikely(flags))
>>>> +     if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
>>>>               return XDP_ABORTED;
>>>>
>>>> +     if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
>>>> +             void *val;
>>>> +
>>>> +             val = __xdp_map_lookup_elem(map, ifindex);
>>>> +             if (unlikely(!val))
>>>> +                     return XDP_PASS;
>>>
>>> Generally looks good to me, also the second part with the flag. 
>>> Given we store into
>>> the per-CPU scratch space and function like xdp_do_redirect() pick 
>>> this up again, we
>>> could even propagate val onwards and save a second lookup on the 
>>> /same/ element (which
>>> also avoids a race if the val was dropped from the map in the 
>>> meantime). Given this
>>> should all still be within RCU it should work. Perhaps it even makes 
>>> sense to do the
>>> lookup unconditionally inside bpf_xdp_redirect_map() helper iff we 
>>> manage to do it
>>> only once anyway?
>>
>> +1
>>
>> also I don't think we really need a new flag here.
>> Yes, it could be considered an uapi change, but it
>> looks more like bugfix in uapi to me.
>> Since original behavior was so clunky to use.
>
> Hmm, the problem with this is that eBPF programs generally do 
> something
> like:
>
> return bpf_redirect_map(map, idx, 0);
>
> after having already modified the packet headers. This will get them a
> return code of XDP_REDIRECT, and the lookup will then subsequently 
> fail,
> which returns in XDP_ABORTED in the driver, which you can catch with
> tracing.
>
> However, if we just change it to XDP_PASS, the packet will go up the
> stack, but because it has already been modified the stack will drop 
> it,
> more or less invisibly.
>
> So the question becomes, is that behaviour change really OK?

Another option would be treating the flags (or the lower bits of flags)
as the default xdp action taken if the lookup fails.  0 just happens to
map to XDP_ABORTED, which gives the initial behavior.  Then the new 
behavior
would be:

     return bpf_redirect_map(map, index, XDP_PASS);

-- 
Jonathan


> -Toke

Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Daniel Borkmann]

On 06/06/2019 08:15 PM, Jonathan Lemon wrote:
> On 6 Jun 2019, at 9:15, Toke Høiland-Jørgensen wrote:
>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>> On Thu, Jun 6, 2019 at 8:51 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>> On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
>>>>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>
>>>>> The bpf_redirect_map() helper used by XDP programs doesn't return any
>>>>> indication of whether it can successfully redirect to the map index it was
>>>>> given. Instead, BPF programs have to track this themselves, leading to
>>>>> programs using duplicate maps to track which entries are populated in the
>>>>> devmap.
>>>>>
>>>>> This patch adds a flag to the XDP version of the bpf_redirect_map() helper,
>>>>> which makes the helper do a lookup in the map when called, and return
>>>>> XDP_PASS if there is no value at the provided index.
>>>>>
>>>>> With this, a BPF program can check the return code from the helper call and
>>>>> react if it is XDP_PASS (by, for instance, substituting a different
>>>>> redirect). This works for any type of map used for redirect.
>>>>>
>>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>> ---
>>>>>  include/uapi/linux/bpf.h |    8 ++++++++
>>>>>  net/core/filter.c        |   10 +++++++++-
>>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>>> index 7c6aef253173..d57df4f0b837 100644
>>>>> --- a/include/uapi/linux/bpf.h
>>>>> +++ b/include/uapi/linux/bpf.h
>>>>> @@ -3098,6 +3098,14 @@ enum xdp_action {
>>>>>       XDP_REDIRECT,
>>>>>  };
>>>>>
>>>>> +/* Flags for bpf_xdp_redirect_map helper */
>>>>> +
>>>>> +/* If set, the help will check if the entry exists in the map and return
>>>>> + * XDP_PASS if it doesn't.
>>>>> + */
>>>>> +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
>>>>> +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
>>>>> +
>>>>>  /* user accessible metadata for XDP packet hook
>>>>>   * new fields must be added to the end of this structure
>>>>>   */
>>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>>> index 55bfc941d17a..2e532a9b2605 100644
>>>>> --- a/net/core/filter.c
>>>>> +++ b/net/core/filter.c
>>>>> @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
>>>>>  {
>>>>>       struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>>>>>
>>>>> -     if (unlikely(flags))
>>>>> +     if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
>>>>>               return XDP_ABORTED;
>>>>>
>>>>> +     if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
>>>>> +             void *val;
>>>>> +
>>>>> +             val = __xdp_map_lookup_elem(map, ifindex);
>>>>> +             if (unlikely(!val))
>>>>> +                     return XDP_PASS;
>>>>
>>>> Generally looks good to me, also the second part with the flag. Given we store into
>>>> the per-CPU scratch space and function like xdp_do_redirect() pick this up again, we
>>>> could even propagate val onwards and save a second lookup on the /same/ element (which
>>>> also avoids a race if the val was dropped from the map in the meantime). Given this
>>>> should all still be within RCU it should work. Perhaps it even makes sense to do the
>>>> lookup unconditionally inside bpf_xdp_redirect_map() helper iff we manage to do it
>>>> only once anyway?
>>>
>>> +1
>>>
>>> also I don't think we really need a new flag here.
>>> Yes, it could be considered an uapi change, but it
>>> looks more like bugfix in uapi to me.
>>> Since original behavior was so clunky to use.
>>
>> Hmm, the problem with this is that eBPF programs generally do something
>> like:
>>
>> return bpf_redirect_map(map, idx, 0);
>>
>> after having already modified the packet headers. This will get them a
>> return code of XDP_REDIRECT, and the lookup will then subsequently fail,
>> which returns in XDP_ABORTED in the driver, which you can catch with
>> tracing.
>>
>> However, if we just change it to XDP_PASS, the packet will go up the
>> stack, but because it has already been modified the stack will drop it,
>> more or less invisibly.
>>
>> So the question becomes, is that behaviour change really OK?
> 
> Another option would be treating the flags (or the lower bits of flags)
> as the default xdp action taken if the lookup fails.  0 just happens to
> map to XDP_ABORTED, which gives the initial behavior.  Then the new behavior
> would be:
> 
>     return bpf_redirect_map(map, index, XDP_PASS);

Makes sense, that should work, but as default (flags == 0), you'd have
to return XDP_REDIRECT to stay consistent with existing behavior.

Thanks,
Daniel

Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Jonathan Lemon]

On 6 Jun 2019, at 12:24, Daniel Borkmann wrote:

> On 06/06/2019 08:15 PM, Jonathan Lemon wrote:
>> On 6 Jun 2019, at 9:15, Toke Høiland-Jørgensen wrote:
>>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>>> On Thu, Jun 6, 2019 at 8:51 AM Daniel Borkmann 
>>>> <daniel@iogearbox.net> wrote:
>>>>> On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
>>>>>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>>
>>>>>> The bpf_redirect_map() helper used by XDP programs doesn't return 
>>>>>> any
>>>>>> indication of whether it can successfully redirect to the map 
>>>>>> index it was
>>>>>> given. Instead, BPF programs have to track this themselves, 
>>>>>> leading to
>>>>>> programs using duplicate maps to track which entries are 
>>>>>> populated in the
>>>>>> devmap.
>>>>>>
>>>>>> This patch adds a flag to the XDP version of the 
>>>>>> bpf_redirect_map() helper,
>>>>>> which makes the helper do a lookup in the map when called, and 
>>>>>> return
>>>>>> XDP_PASS if there is no value at the provided index.
>>>>>>
>>>>>> With this, a BPF program can check the return code from the 
>>>>>> helper call and
>>>>>> react if it is XDP_PASS (by, for instance, substituting a 
>>>>>> different
>>>>>> redirect). This works for any type of map used for redirect.
>>>>>>
>>>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>> ---
>>>>>>  include/uapi/linux/bpf.h |    8 ++++++++
>>>>>>  net/core/filter.c        |   10 +++++++++-
>>>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>>>> index 7c6aef253173..d57df4f0b837 100644
>>>>>> --- a/include/uapi/linux/bpf.h
>>>>>> +++ b/include/uapi/linux/bpf.h
>>>>>> @@ -3098,6 +3098,14 @@ enum xdp_action {
>>>>>>       XDP_REDIRECT,
>>>>>>  };
>>>>>>
>>>>>> +/* Flags for bpf_xdp_redirect_map helper */
>>>>>> +
>>>>>> +/* If set, the help will check if the entry exists in the map 
>>>>>> and return
>>>>>> + * XDP_PASS if it doesn't.
>>>>>> + */
>>>>>> +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
>>>>>> +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
>>>>>> +
>>>>>>  /* user accessible metadata for XDP packet hook
>>>>>>   * new fields must be added to the end of this structure
>>>>>>   */
>>>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>>>> index 55bfc941d17a..2e532a9b2605 100644
>>>>>> --- a/net/core/filter.c
>>>>>> +++ b/net/core/filter.c
>>>>>> @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct 
>>>>>> bpf_map *, map, u32, ifindex,
>>>>>>  {
>>>>>>       struct bpf_redirect_info *ri = 
>>>>>> this_cpu_ptr(&bpf_redirect_info);
>>>>>>
>>>>>> -     if (unlikely(flags))
>>>>>> +     if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
>>>>>>               return XDP_ABORTED;
>>>>>>
>>>>>> +     if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
>>>>>> +             void *val;
>>>>>> +
>>>>>> +             val = __xdp_map_lookup_elem(map, 
>>>>>> ifindex);
>>>>>> +             if (unlikely(!val))
>>>>>> +                     return XDP_PASS;
>>>>>
>>>>> Generally looks good to me, also the second part with the flag. 
>>>>> Given we store into
>>>>> the per-CPU scratch space and function like xdp_do_redirect() pick 
>>>>> this up again, we
>>>>> could even propagate val onwards and save a second lookup on the 
>>>>> /same/ element (which
>>>>> also avoids a race if the val was dropped from the map in the 
>>>>> meantime). Given this
>>>>> should all still be within RCU it should work. Perhaps it even 
>>>>> makes sense to do the
>>>>> lookup unconditionally inside bpf_xdp_redirect_map() helper iff we 
>>>>> manage to do it
>>>>> only once anyway?
>>>>
>>>> +1
>>>>
>>>> also I don't think we really need a new flag here.
>>>> Yes, it could be considered an uapi change, but it
>>>> looks more like bugfix in uapi to me.
>>>> Since original behavior was so clunky to use.
>>>
>>> Hmm, the problem with this is that eBPF programs generally do 
>>> something
>>> like:
>>>
>>> return bpf_redirect_map(map, idx, 0);
>>>
>>> after having already modified the packet headers. This will get them 
>>> a
>>> return code of XDP_REDIRECT, and the lookup will then subsequently 
>>> fail,
>>> which returns in XDP_ABORTED in the driver, which you can catch with
>>> tracing.
>>>
>>> However, if we just change it to XDP_PASS, the packet will go up the
>>> stack, but because it has already been modified the stack will drop 
>>> it,
>>> more or less invisibly.
>>>
>>> So the question becomes, is that behaviour change really OK?
>>
>> Another option would be treating the flags (or the lower bits of 
>> flags)
>> as the default xdp action taken if the lookup fails.  0 just happens 
>> to
>> map to XDP_ABORTED, which gives the initial behavior.  Then the new 
>> behavior
>> would be:
>>
>>     return bpf_redirect_map(map, index, XDP_PASS);
>
> Makes sense, that should work, but as default (flags == 0), you'd have
> to return XDP_REDIRECT to stay consistent with existing behavior.

Right - I was thinking something along the lines of:

    val = __xdp_map_lookup_elem(map, ifindex);
    if (unlikely(!val))
        return (flags & 3);
    ...
    return XDP_REDIRECT;


Stated another way, if the map lookup succeeds, return REDIRECT, 
otherwise
return one (ABORT, DROP, PASS, TX).
-- 
Jonathan

	
>
> Thanks,
> Daniel

Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Toke Høiland-Jørgensen]

Jonathan Lemon <jonathan.lemon@gmail.com> writes:

> On 6 Jun 2019, at 12:24, Daniel Borkmann wrote:
>
>> On 06/06/2019 08:15 PM, Jonathan Lemon wrote:
>>> On 6 Jun 2019, at 9:15, Toke Høiland-Jørgensen wrote:
>>>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>>>> On Thu, Jun 6, 2019 at 8:51 AM Daniel Borkmann 
>>>>> <daniel@iogearbox.net> wrote:
>>>>>> On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
>>>>>>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>>>
>>>>>>> The bpf_redirect_map() helper used by XDP programs doesn't return 
>>>>>>> any
>>>>>>> indication of whether it can successfully redirect to the map 
>>>>>>> index it was
>>>>>>> given. Instead, BPF programs have to track this themselves, 
>>>>>>> leading to
>>>>>>> programs using duplicate maps to track which entries are 
>>>>>>> populated in the
>>>>>>> devmap.
>>>>>>>
>>>>>>> This patch adds a flag to the XDP version of the 
>>>>>>> bpf_redirect_map() helper,
>>>>>>> which makes the helper do a lookup in the map when called, and 
>>>>>>> return
>>>>>>> XDP_PASS if there is no value at the provided index.
>>>>>>>
>>>>>>> With this, a BPF program can check the return code from the 
>>>>>>> helper call and
>>>>>>> react if it is XDP_PASS (by, for instance, substituting a 
>>>>>>> different
>>>>>>> redirect). This works for any type of map used for redirect.
>>>>>>>
>>>>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>>> ---
>>>>>>>  include/uapi/linux/bpf.h |    8 ++++++++
>>>>>>>  net/core/filter.c        |   10 +++++++++-
>>>>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>>>>> index 7c6aef253173..d57df4f0b837 100644
>>>>>>> --- a/include/uapi/linux/bpf.h
>>>>>>> +++ b/include/uapi/linux/bpf.h
>>>>>>> @@ -3098,6 +3098,14 @@ enum xdp_action {
>>>>>>>       XDP_REDIRECT,
>>>>>>>  };
>>>>>>>
>>>>>>> +/* Flags for bpf_xdp_redirect_map helper */
>>>>>>> +
>>>>>>> +/* If set, the help will check if the entry exists in the map 
>>>>>>> and return
>>>>>>> + * XDP_PASS if it doesn't.
>>>>>>> + */
>>>>>>> +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
>>>>>>> +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
>>>>>>> +
>>>>>>>  /* user accessible metadata for XDP packet hook
>>>>>>>   * new fields must be added to the end of this structure
>>>>>>>   */
>>>>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>>>>> index 55bfc941d17a..2e532a9b2605 100644
>>>>>>> --- a/net/core/filter.c
>>>>>>> +++ b/net/core/filter.c
>>>>>>> @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct 
>>>>>>> bpf_map *, map, u32, ifindex,
>>>>>>>  {
>>>>>>>       struct bpf_redirect_info *ri = 
>>>>>>> this_cpu_ptr(&bpf_redirect_info);
>>>>>>>
>>>>>>> -     if (unlikely(flags))
>>>>>>> +     if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
>>>>>>>               return XDP_ABORTED;
>>>>>>>
>>>>>>> +     if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
>>>>>>> +             void *val;
>>>>>>> +
>>>>>>> +             val = __xdp_map_lookup_elem(map, 
>>>>>>> ifindex);
>>>>>>> +             if (unlikely(!val))
>>>>>>> +                     return XDP_PASS;
>>>>>>
>>>>>> Generally looks good to me, also the second part with the flag. 
>>>>>> Given we store into
>>>>>> the per-CPU scratch space and function like xdp_do_redirect() pick 
>>>>>> this up again, we
>>>>>> could even propagate val onwards and save a second lookup on the 
>>>>>> /same/ element (which
>>>>>> also avoids a race if the val was dropped from the map in the 
>>>>>> meantime). Given this
>>>>>> should all still be within RCU it should work. Perhaps it even 
>>>>>> makes sense to do the
>>>>>> lookup unconditionally inside bpf_xdp_redirect_map() helper iff we 
>>>>>> manage to do it
>>>>>> only once anyway?
>>>>>
>>>>> +1
>>>>>
>>>>> also I don't think we really need a new flag here.
>>>>> Yes, it could be considered an uapi change, but it
>>>>> looks more like bugfix in uapi to me.
>>>>> Since original behavior was so clunky to use.
>>>>
>>>> Hmm, the problem with this is that eBPF programs generally do 
>>>> something
>>>> like:
>>>>
>>>> return bpf_redirect_map(map, idx, 0);
>>>>
>>>> after having already modified the packet headers. This will get them 
>>>> a
>>>> return code of XDP_REDIRECT, and the lookup will then subsequently 
>>>> fail,
>>>> which returns in XDP_ABORTED in the driver, which you can catch with
>>>> tracing.
>>>>
>>>> However, if we just change it to XDP_PASS, the packet will go up the
>>>> stack, but because it has already been modified the stack will drop 
>>>> it,
>>>> more or less invisibly.
>>>>
>>>> So the question becomes, is that behaviour change really OK?
>>>
>>> Another option would be treating the flags (or the lower bits of 
>>> flags)
>>> as the default xdp action taken if the lookup fails.  0 just happens 
>>> to
>>> map to XDP_ABORTED, which gives the initial behavior.  Then the new 
>>> behavior
>>> would be:
>>>
>>>     return bpf_redirect_map(map, index, XDP_PASS);
>>
>> Makes sense, that should work, but as default (flags == 0), you'd have
>> to return XDP_REDIRECT to stay consistent with existing behavior.
>
> Right - I was thinking something along the lines of:
>
>     val = __xdp_map_lookup_elem(map, ifindex);
>     if (unlikely(!val))
>         return (flags & 3);
>     ...
>     return XDP_REDIRECT;
>
>
> Stated another way, if the map lookup succeeds, return REDIRECT, 
> otherwise
> return one (ABORT, DROP, PASS, TX).

But then we're still changing UAPI on flags==0? Also, what would be the
use case for this, wouldn't the program have to react explicitly in any
case (to, e.g., not modify the packet if it decides to XDP_PASS)?

-Toke

Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Jonathan Lemon]

On 6 Jun 2019, at 14:14, Toke Høiland-Jørgensen wrote:

> Jonathan Lemon <jonathan.lemon@gmail.com> writes:
>
>> On 6 Jun 2019, at 12:24, Daniel Borkmann wrote:
>>
>>> On 06/06/2019 08:15 PM, Jonathan Lemon wrote:
>>>> On 6 Jun 2019, at 9:15, Toke Høiland-Jørgensen wrote:
>>>>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>>>>> On Thu, Jun 6, 2019 at 8:51 AM Daniel Borkmann
>>>>>> <daniel@iogearbox.net> wrote:
>>>>>>> On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
>>>>>>>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>>>>
>>>>>>>> The bpf_redirect_map() helper used by XDP programs doesn't return
>>>>>>>> any
>>>>>>>> indication of whether it can successfully redirect to the map
>>>>>>>> index it was
>>>>>>>> given. Instead, BPF programs have to track this themselves,
>>>>>>>> leading to
>>>>>>>> programs using duplicate maps to track which entries are
>>>>>>>> populated in the
>>>>>>>> devmap.
>>>>>>>>
>>>>>>>> This patch adds a flag to the XDP version of the
>>>>>>>> bpf_redirect_map() helper,
>>>>>>>> which makes the helper do a lookup in the map when called, and
>>>>>>>> return
>>>>>>>> XDP_PASS if there is no value at the provided index.
>>>>>>>>
>>>>>>>> With this, a BPF program can check the return code from the
>>>>>>>> helper call and
>>>>>>>> react if it is XDP_PASS (by, for instance, substituting a
>>>>>>>> different
>>>>>>>> redirect). This works for any type of map used for redirect.
>>>>>>>>
>>>>>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>>>> ---
>>>>>>>>  include/uapi/linux/bpf.h |    8 ++++++++
>>>>>>>>  net/core/filter.c        |   10 +++++++++-
>>>>>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>>>>>> index 7c6aef253173..d57df4f0b837 100644
>>>>>>>> --- a/include/uapi/linux/bpf.h
>>>>>>>> +++ b/include/uapi/linux/bpf.h
>>>>>>>> @@ -3098,6 +3098,14 @@ enum xdp_action {
>>>>>>>>       XDP_REDIRECT,
>>>>>>>>  };
>>>>>>>>
>>>>>>>> +/* Flags for bpf_xdp_redirect_map helper */
>>>>>>>> +
>>>>>>>> +/* If set, the help will check if the entry exists in the map
>>>>>>>> and return
>>>>>>>> + * XDP_PASS if it doesn't.
>>>>>>>> + */
>>>>>>>> +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
>>>>>>>> +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
>>>>>>>> +
>>>>>>>>  /* user accessible metadata for XDP packet hook
>>>>>>>>   * new fields must be added to the end of this structure
>>>>>>>>   */
>>>>>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>>>>>> index 55bfc941d17a..2e532a9b2605 100644
>>>>>>>> --- a/net/core/filter.c
>>>>>>>> +++ b/net/core/filter.c
>>>>>>>> @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct
>>>>>>>> bpf_map *, map, u32, ifindex,
>>>>>>>>  {
>>>>>>>>       struct bpf_redirect_info *ri =
>>>>>>>> this_cpu_ptr(&bpf_redirect_info);
>>>>>>>>
>>>>>>>> -     if (unlikely(flags))
>>>>>>>> +     if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
>>>>>>>>               return XDP_ABORTED;
>>>>>>>>
>>>>>>>> +     if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
>>>>>>>> +             void *val;
>>>>>>>> +
>>>>>>>> +             val = __xdp_map_lookup_elem(map,
>>>>>>>> ifindex);
>>>>>>>> +             if (unlikely(!val))
>>>>>>>> +                     return XDP_PASS;
>>>>>>>
>>>>>>> Generally looks good to me, also the second part with the flag.
>>>>>>> Given we store into
>>>>>>> the per-CPU scratch space and function like xdp_do_redirect() pick
>>>>>>> this up again, we
>>>>>>> could even propagate val onwards and save a second lookup on the
>>>>>>> /same/ element (which
>>>>>>> also avoids a race if the val was dropped from the map in the
>>>>>>> meantime). Given this
>>>>>>> should all still be within RCU it should work. Perhaps it even
>>>>>>> makes sense to do the
>>>>>>> lookup unconditionally inside bpf_xdp_redirect_map() helper iff we
>>>>>>> manage to do it
>>>>>>> only once anyway?
>>>>>>
>>>>>> +1
>>>>>>
>>>>>> also I don't think we really need a new flag here.
>>>>>> Yes, it could be considered an uapi change, but it
>>>>>> looks more like bugfix in uapi to me.
>>>>>> Since original behavior was so clunky to use.
>>>>>
>>>>> Hmm, the problem with this is that eBPF programs generally do
>>>>> something
>>>>> like:
>>>>>
>>>>> return bpf_redirect_map(map, idx, 0);
>>>>>
>>>>> after having already modified the packet headers. This will get them
>>>>> a
>>>>> return code of XDP_REDIRECT, and the lookup will then subsequently
>>>>> fail,
>>>>> which returns in XDP_ABORTED in the driver, which you can catch with
>>>>> tracing.
>>>>>
>>>>> However, if we just change it to XDP_PASS, the packet will go up the
>>>>> stack, but because it has already been modified the stack will drop
>>>>> it,
>>>>> more or less invisibly.
>>>>>
>>>>> So the question becomes, is that behaviour change really OK?
>>>>
>>>> Another option would be treating the flags (or the lower bits of
>>>> flags)
>>>> as the default xdp action taken if the lookup fails.  0 just happens
>>>> to
>>>> map to XDP_ABORTED, which gives the initial behavior.  Then the new
>>>> behavior
>>>> would be:
>>>>
>>>>     return bpf_redirect_map(map, index, XDP_PASS);
>>>
>>> Makes sense, that should work, but as default (flags == 0), you'd have
>>> to return XDP_REDIRECT to stay consistent with existing behavior.
>>
>> Right - I was thinking something along the lines of:
>>
>>     val = __xdp_map_lookup_elem(map, ifindex);
>>     if (unlikely(!val))
>>         return (flags & 3);
>>     ...
>>     return XDP_REDIRECT;
>>
>>
>> Stated another way, if the map lookup succeeds, return REDIRECT,
>> otherwise
>> return one (ABORT, DROP, PASS, TX).
>
> But then we're still changing UAPI on flags==0?

I believe your point (and Daniel's) is that for flags==0, it should always
return REDIRECT, which is the current behavior? I'm not seeing why it
matters.

Returning REDIRECT indicates something was stored in redirect_info, and
xdp_do_redirect() is called.  This will fail the lookup (which was just done)
and return -EINVAL.  Callers treat this as XDP_DROP.

On the other hand, returning XDP_ABORTED bypasses the xdp_do_redirect() call
and all callsites treat this as DROP.  The main difference seems to be the
tracing call - whether _trace_xdp_redirect_map_err or trace_xdp_exception gets
called.

Is this really an UAPI breakage?


> Also, what would be the
> use case for this, wouldn't the program have to react explicitly in any
> case (to, e.g., not modify the packet if it decides to XDP_PASS)?

How is that any different from using XDP_REDIRECT_F_PASS_ON_INVALID?
-- 
Jonathan


>
> -Toke

Re: [PATCH net-next v2 1/2] bpf_xdp_redirect_map: Add flag to return XDP_PASS on map lookup failure

[Toke Høiland-Jørgensen]

Jonathan Lemon <jonathan.lemon@gmail.com> writes:

> On 6 Jun 2019, at 14:14, Toke Høiland-Jørgensen wrote:
>
>> Jonathan Lemon <jonathan.lemon@gmail.com> writes:
>>
>>> On 6 Jun 2019, at 12:24, Daniel Borkmann wrote:
>>>
>>>> On 06/06/2019 08:15 PM, Jonathan Lemon wrote:
>>>>> On 6 Jun 2019, at 9:15, Toke Høiland-Jørgensen wrote:
>>>>>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>>>>>> On Thu, Jun 6, 2019 at 8:51 AM Daniel Borkmann
>>>>>>> <daniel@iogearbox.net> wrote:
>>>>>>>> On 06/06/2019 03:24 PM, Toke Høiland-Jørgensen wrote:
>>>>>>>>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>>>>>
>>>>>>>>> The bpf_redirect_map() helper used by XDP programs doesn't return
>>>>>>>>> any
>>>>>>>>> indication of whether it can successfully redirect to the map
>>>>>>>>> index it was
>>>>>>>>> given. Instead, BPF programs have to track this themselves,
>>>>>>>>> leading to
>>>>>>>>> programs using duplicate maps to track which entries are
>>>>>>>>> populated in the
>>>>>>>>> devmap.
>>>>>>>>>
>>>>>>>>> This patch adds a flag to the XDP version of the
>>>>>>>>> bpf_redirect_map() helper,
>>>>>>>>> which makes the helper do a lookup in the map when called, and
>>>>>>>>> return
>>>>>>>>> XDP_PASS if there is no value at the provided index.
>>>>>>>>>
>>>>>>>>> With this, a BPF program can check the return code from the
>>>>>>>>> helper call and
>>>>>>>>> react if it is XDP_PASS (by, for instance, substituting a
>>>>>>>>> different
>>>>>>>>> redirect). This works for any type of map used for redirect.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>>>>> ---
>>>>>>>>>  include/uapi/linux/bpf.h |    8 ++++++++
>>>>>>>>>  net/core/filter.c        |   10 +++++++++-
>>>>>>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>>>>>>> index 7c6aef253173..d57df4f0b837 100644
>>>>>>>>> --- a/include/uapi/linux/bpf.h
>>>>>>>>> +++ b/include/uapi/linux/bpf.h
>>>>>>>>> @@ -3098,6 +3098,14 @@ enum xdp_action {
>>>>>>>>>       XDP_REDIRECT,
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>> +/* Flags for bpf_xdp_redirect_map helper */
>>>>>>>>> +
>>>>>>>>> +/* If set, the help will check if the entry exists in the map
>>>>>>>>> and return
>>>>>>>>> + * XDP_PASS if it doesn't.
>>>>>>>>> + */
>>>>>>>>> +#define XDP_REDIRECT_F_PASS_ON_INVALID BIT(0)
>>>>>>>>> +#define XDP_REDIRECT_ALL_FLAGS XDP_REDIRECT_F_PASS_ON_INVALID
>>>>>>>>> +
>>>>>>>>>  /* user accessible metadata for XDP packet hook
>>>>>>>>>   * new fields must be added to the end of this structure
>>>>>>>>>   */
>>>>>>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>>>>>>> index 55bfc941d17a..2e532a9b2605 100644
>>>>>>>>> --- a/net/core/filter.c
>>>>>>>>> +++ b/net/core/filter.c
>>>>>>>>> @@ -3755,9 +3755,17 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct
>>>>>>>>> bpf_map *, map, u32, ifindex,
>>>>>>>>>  {
>>>>>>>>>       struct bpf_redirect_info *ri =
>>>>>>>>> this_cpu_ptr(&bpf_redirect_info);
>>>>>>>>>
>>>>>>>>> -     if (unlikely(flags))
>>>>>>>>> +     if (unlikely(flags & ~XDP_REDIRECT_ALL_FLAGS))
>>>>>>>>>               return XDP_ABORTED;
>>>>>>>>>
>>>>>>>>> +     if (flags & XDP_REDIRECT_F_PASS_ON_INVALID) {
>>>>>>>>> +             void *val;
>>>>>>>>> +
>>>>>>>>> +             val = __xdp_map_lookup_elem(map,
>>>>>>>>> ifindex);
>>>>>>>>> +             if (unlikely(!val))
>>>>>>>>> +                     return XDP_PASS;
>>>>>>>>
>>>>>>>> Generally looks good to me, also the second part with the flag.
>>>>>>>> Given we store into
>>>>>>>> the per-CPU scratch space and function like xdp_do_redirect() pick
>>>>>>>> this up again, we
>>>>>>>> could even propagate val onwards and save a second lookup on the
>>>>>>>> /same/ element (which
>>>>>>>> also avoids a race if the val was dropped from the map in the
>>>>>>>> meantime). Given this
>>>>>>>> should all still be within RCU it should work. Perhaps it even
>>>>>>>> makes sense to do the
>>>>>>>> lookup unconditionally inside bpf_xdp_redirect_map() helper iff we
>>>>>>>> manage to do it
>>>>>>>> only once anyway?
>>>>>>>
>>>>>>> +1
>>>>>>>
>>>>>>> also I don't think we really need a new flag here.
>>>>>>> Yes, it could be considered an uapi change, but it
>>>>>>> looks more like bugfix in uapi to me.
>>>>>>> Since original behavior was so clunky to use.
>>>>>>
>>>>>> Hmm, the problem with this is that eBPF programs generally do
>>>>>> something
>>>>>> like:
>>>>>>
>>>>>> return bpf_redirect_map(map, idx, 0);
>>>>>>
>>>>>> after having already modified the packet headers. This will get them
>>>>>> a
>>>>>> return code of XDP_REDIRECT, and the lookup will then subsequently
>>>>>> fail,
>>>>>> which returns in XDP_ABORTED in the driver, which you can catch with
>>>>>> tracing.
>>>>>>
>>>>>> However, if we just change it to XDP_PASS, the packet will go up the
>>>>>> stack, but because it has already been modified the stack will drop
>>>>>> it,
>>>>>> more or less invisibly.
>>>>>>
>>>>>> So the question becomes, is that behaviour change really OK?
>>>>>
>>>>> Another option would be treating the flags (or the lower bits of
>>>>> flags)
>>>>> as the default xdp action taken if the lookup fails.  0 just happens
>>>>> to
>>>>> map to XDP_ABORTED, which gives the initial behavior.  Then the new
>>>>> behavior
>>>>> would be:
>>>>>
>>>>>     return bpf_redirect_map(map, index, XDP_PASS);
>>>>
>>>> Makes sense, that should work, but as default (flags == 0), you'd have
>>>> to return XDP_REDIRECT to stay consistent with existing behavior.
>>>
>>> Right - I was thinking something along the lines of:
>>>
>>>     val = __xdp_map_lookup_elem(map, ifindex);
>>>     if (unlikely(!val))
>>>         return (flags & 3);
>>>     ...
>>>     return XDP_REDIRECT;
>>>
>>>
>>> Stated another way, if the map lookup succeeds, return REDIRECT,
>>> otherwise
>>> return one (ABORT, DROP, PASS, TX).
>>
>> But then we're still changing UAPI on flags==0?
>
> I believe your point (and Daniel's) is that for flags==0, it should always
> return REDIRECT, which is the current behavior? I'm not seeing why it
> matters.
>
> Returning REDIRECT indicates something was stored in redirect_info, and
> xdp_do_redirect() is called.  This will fail the lookup (which was just done)
> and return -EINVAL.  Callers treat this as XDP_DROP.
>
> On the other hand, returning XDP_ABORTED bypasses the xdp_do_redirect() call
> and all callsites treat this as DROP.  The main difference seems to be the
> tracing call - whether _trace_xdp_redirect_map_err or trace_xdp_exception gets
> called.
>
> Is this really an UAPI breakage?

Well, that's what I'm trying to figure out :)

It will mean that the xdp_redirect_map_err() tracepoint is no longer
triggered, and anyone who counts the number of different return codes
seen by the program (as we do in the XDP tutorial, for instance[0]) is
going to see different values all of a sudden.

So it kinda feels dodgy to change it, I'd say? As in, I'm not vehemently
opposed, just trying to be extra cautious?

>> Also, what would be the use case for this, wouldn't the program have
>> to react explicitly in any case (to, e.g., not modify the packet if
>> it decides to XDP_PASS)?
>
> How is that any different from using XDP_REDIRECT_F_PASS_ON_INVALID?

My point is that it's not: If you have to check the return value anyway,
we're not really gaining everything from making it possible to select
what that return value is?

-Toke

[0] https://github.com/xdp-project/xdp-tutorial/blob/master/packet01-parsing/xdp_prog_kern.c#L94

[PATCH net-next v2 2/2] devmap: Allow map lookups from eBPF

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

We don't currently allow lookups into a devmap from eBPF, because the map
lookup returns a pointer directly to the dev->ifindex, which shouldn't be
modifiable from eBPF.

However, being able to do lookups in devmaps is useful to know (e.g.)
whether forwarding to a specific interface is enabled. Currently, programs
work around this by keeping a shadow map of another type which indicates
whether a map index is valid.

Since we now have a flag to make maps read-only from the eBPF side, we can
simply lift the lookup restriction if we make sure this flag is always set.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 kernel/bpf/devmap.c   |    5 +++++
 kernel/bpf/verifier.c |    7 ++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 5ae7cce5ef16..0e6875a462ef 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -99,6 +99,11 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
 	    attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
 		return ERR_PTR(-EINVAL);
 
+	/* Lookup returns a pointer straight to dev->ifindex, so make sure the
+	 * verifier prevents writes from the BPF side
+	 */
+	attr->map_flags |= BPF_F_RDONLY_PROG;
+
 	dtab = kzalloc(sizeof(*dtab), GFP_USER);
 	if (!dtab)
 		return ERR_PTR(-ENOMEM);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5c2cb5bd84ce..7128a9821481 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2893,12 +2893,9 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
 		if (func_id != BPF_FUNC_get_local_storage)
 			goto error;
 		break;
-	/* devmap returns a pointer to a live net_device ifindex that we cannot
-	 * allow to be modified from bpf side. So do not allow lookup elements
-	 * for now.
-	 */
 	case BPF_MAP_TYPE_DEVMAP:
-		if (func_id != BPF_FUNC_redirect_map)
+		if (func_id != BPF_FUNC_redirect_map &&
+		    func_id != BPF_FUNC_map_lookup_elem)
 			goto error;
 		break;
 	/* Restrict bpf side of cpumap and xskmap, open when use-cases

Re: [PATCH net-next v2 2/2] devmap: Allow map lookups from eBPF

[Jesper Dangaard Brouer]

On Thu, 06 Jun 2019 15:24:14 +0200
Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> We don't currently allow lookups into a devmap from eBPF, because the map
> lookup returns a pointer directly to the dev->ifindex, which shouldn't be
> modifiable from eBPF.
> 
> However, being able to do lookups in devmaps is useful to know (e.g.)
> whether forwarding to a specific interface is enabled. Currently, programs
> work around this by keeping a shadow map of another type which indicates
> whether a map index is valid.
> 
> Since we now have a flag to make maps read-only from the eBPF side, we can
> simply lift the lookup restriction if we make sure this flag is always set.

Nice, I didn't know this was possible.  I like it! :-)

> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  kernel/bpf/devmap.c   |    5 +++++
>  kernel/bpf/verifier.c |    7 ++-----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index 5ae7cce5ef16..0e6875a462ef 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -99,6 +99,11 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
>  	    attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
>  		return ERR_PTR(-EINVAL);
>  
> +	/* Lookup returns a pointer straight to dev->ifindex, so make sure the
> +	 * verifier prevents writes from the BPF side
> +	 */
> +	attr->map_flags |= BPF_F_RDONLY_PROG;
> +
>  	dtab = kzalloc(sizeof(*dtab), GFP_USER);
>  	if (!dtab)
>  		return ERR_PTR(-ENOMEM);
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5c2cb5bd84ce..7128a9821481 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2893,12 +2893,9 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
>  		if (func_id != BPF_FUNC_get_local_storage)
>  			goto error;
>  		break;
> -	/* devmap returns a pointer to a live net_device ifindex that we cannot
> -	 * allow to be modified from bpf side. So do not allow lookup elements
> -	 * for now.
> -	 */
>  	case BPF_MAP_TYPE_DEVMAP:
> -		if (func_id != BPF_FUNC_redirect_map)
> +		if (func_id != BPF_FUNC_redirect_map &&
> +		    func_id != BPF_FUNC_map_lookup_elem)
>  			goto error;
>  		break;
>  	/* Restrict bpf side of cpumap and xskmap, open when use-cases
> 



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

Re: [PATCH net-next v2 2/2] devmap: Allow map lookups from eBPF

[Toke Høiland-Jørgensen]

Jesper Dangaard Brouer <brouer@redhat.com> writes:

> On Thu, 06 Jun 2019 15:24:14 +0200
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> We don't currently allow lookups into a devmap from eBPF, because the map
>> lookup returns a pointer directly to the dev->ifindex, which shouldn't be
>> modifiable from eBPF.
>> 
>> However, being able to do lookups in devmaps is useful to know (e.g.)
>> whether forwarding to a specific interface is enabled. Currently, programs
>> work around this by keeping a shadow map of another type which indicates
>> whether a map index is valid.
>> 
>> Since we now have a flag to make maps read-only from the eBPF side, we can
>> simply lift the lookup restriction if we make sure this flag is always set.
>
> Nice, I didn't know this was possible.  I like it! :-)

Me neither; discovered it while looking through the verifier code to
figure out what would be needed to get the verifier to enforce read-only
semantics. Not much, as it turned out :)

The functionality was introduced in:
591fe9888d78 ("bpf: add program side {rd, wr}only support for maps") by
Daniel from April 9th.

-Toke

Re: [PATCH net-next v2 0/2] xdp: Allow lookup into devmaps before redirect

[David Miller]

Please target XDP/eBPF changes to the 'bpf' and 'bpf-next' tree rather
than 'net' and 'net-next'.

Thank you.