netdev - Re: [PATCH bpf-next v1 0/5] Introduce bpf_packet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Mar 2022 21:48:52 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, Lorenz Bauer <linux@....io>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v1 0/5] Introduce bpf_packet_pointer helper

On Sun, Mar 6, 2022 at 3:43 PM Kumar Kartikeya Dwivedi <memxor@...il.com> wrote:
>
> Expose existing 'bpf_xdp_pointer' as a BPF helper named 'bpf_packet_pointer'
> returning a packet pointer with a fixed immutable range. This can be useful to
> enable DPA without having to use memcpy (currently the case in
> bpf_xdp_load_bytes and bpf_xdp_store_bytes).
>
> The intended usage to read and write data for multi-buff XDP is:
>
>         int err = 0;
>         char buf[N];
>
>         off &= 0xffff;
>         ptr = bpf_packet_pointer(ctx, off, sizeof(buf), &err);
>         if (unlikely(!ptr)) {
>                 if (err < 0)
>                         return XDP_ABORTED;
>                 err = bpf_xdp_load_bytes(ctx, off, buf, sizeof(buf));
>                 if (err < 0)
>                         return XDP_ABORTED;
>                 ptr = buf;
>         }
>         ...
>         // Do some stores and loads in [ptr, ptr + N) region
>         ...
>         if (unlikely(ptr == buf)) {
>                 err = bpf_xdp_store_bytes(ctx, off, buf, sizeof(buf));
>                 if (err < 0)
>                         return XDP_ABORTED;
>         }
>
> Note that bpf_packet_pointer returns a PTR_TO_PACKET, not PTR_TO_MEM, because
> these pointers need to be invalidated on clear_all_pkt_pointers invocation, and
> it is also more meaningful to the user to see return value as R0=pkt.
>
> This series is meant to collect feedback on the approach, next version can
> include a bpf_skb_pointer and exposing it as bpf_packet_pointer helper for TC
> hooks, and explore not resetting range to zero on r0 += rX, instead check access
> like check_mem_region_access (var_off + off < range), since there would be no
> data_end to compare against and obtain a new range.
>
> The common name and func_id is supposed to allow writing generic code using
> bpf_packet_pointer that works for both XDP and TC programs.
>
> Please see the individual patches for implementation details.
>

Joanne is working on a "bpf_dynptr" framework that will support
exactly this feature, in addition to working with dynamically
allocated memory, working with memory of statically unknown size (but
safe and checked at runtime), etc. And all that within a generic
common feature implemented uniformly within the verifier. E.g., it
won't need any of the custom bits of logic added in patch #2 and #3.
So I'm thinking that instead of custom-implementing a partial case of
bpf_dynptr just for skb and xdp packets, let's maybe wait for dynptr
and do it only once there?

See also my ARG_CONSTANT comment. It seems like a pretty common thing
where input constant is used to characterize some pointer returned
from the helper (e.g., bpf_ringbuf_reserve() case), and we'll need
that for bpf_dynptr for exactly this "give me direct access of N
bytes, if possible" case. So improving/generalizing it now before
dynptr lands makes a lot of sense, outside of bpf_packet_pointer()
feature itself.

> Kumar Kartikeya Dwivedi (5):
>   bpf: Add ARG_SCALAR and ARG_CONSTANT
>   bpf: Introduce pkt_uid concept for PTR_TO_PACKET
>   bpf: Introduce bpf_packet_pointer helper to do DPA
>   selftests/bpf: Add verifier tests for pkt pointer with pkt_uid
>   selftests/bpf: Update xdp_adjust_frags to use bpf_packet_pointer
>
>  include/linux/bpf.h                           |   4 +
>  include/linux/bpf_verifier.h                  |   9 +-
>  include/uapi/linux/bpf.h                      |  12 ++
>  kernel/bpf/verifier.c                         |  97 ++++++++++--
>  net/core/filter.c                             |  48 +++---
>  tools/include/uapi/linux/bpf.h                |  12 ++
>  .../bpf/prog_tests/xdp_adjust_frags.c         |  46 ++++--
>  .../bpf/progs/test_xdp_update_frags.c         |  46 ++++--
>  tools/testing/selftests/bpf/verifier/xdp.c    | 146 ++++++++++++++++++
>  9 files changed, 358 insertions(+), 62 deletions(-)
>
> --
> 2.35.1
>