On 06/23/2019 04:17 AM, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <t...@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 fixes this by moving the map lookup into the bpf_redirect_map()
> helper, which makes it possible to return failure to the eBPF program. The
> lower bits of the flags argument is used as the return code, which means
> that existing users who pass a '0' flag argument will get XDP_ABORTED.
>
> With this, a BPF program can check the return code from the helper call and
> react 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 <t...@redhat.com>
Overall series looks good to me. Just very small things inline here & in the
other two patches:
[...]
> @@ -3750,9 +3742,16 @@ 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))
> + /* Lower bits of the flags are used as return code on lookup failure */
> + if (unlikely(flags > XDP_TX))
> return XDP_ABORTED;
>
> + ri->item = __xdp_map_lookup_elem(map, ifindex);
> + if (unlikely(!ri->item)) {
> + WRITE_ONCE(ri->map, NULL);
This WRITE_ONCE() is not needed. We never set it before at this point.
> + return flags;
> + }
> +
> ri->ifindex = ifindex;
> ri->flags = flags;
> WRITE_ONCE(ri->map, map);
>
