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

["Jonathan Lemon" <jonathan.lemon@gmail.com>]

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