On Sat, Apr 16, 2022 at 1:19 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Sat, Apr 16, 2022 at 01:43:41PM IST, Kumar Kartikeya Dwivedi wrote:
> > On Sat, Apr 16, 2022 at 12:04:22PM IST, Joanne Koong wrote:
> > > This patchset implements the basics of dynamic pointers in bpf.
> > >
> > > A dynamic pointer (struct bpf_dynptr) is a pointer that stores extra metadata
> > > alongside the address it points to. This abstraction is useful in bpf, given
> > > that every memory access in a bpf program must be safe. The verifier and bpf
> > > helper functions can use the metadata to enforce safety guarantees for things
> > > such as dynamically sized strings and kernel heap allocations.
> > >
> > > From the program side, the bpf_dynptr is an opaque struct and the verifier
> > > will enforce that its contents are never written to by the program.
> > > It can only be written to through specific bpf helper functions.
> > >
> > > There are several uses cases for dynamic pointers in bpf programs. A list of
> > > some are: dynamically sized ringbuf reservations without any extra memcpys,
> > > dynamic string parsing and memory comparisons, dynamic memory allocations that
> > > can be persisted in a map, and dynamic parsing of sk_buff and xdp_md packet
> > > data.
> > >
> > > At a high-level, the patches are as follows:
> > > 1/7 - Adds MEM_UNINIT as a bpf_type_flag
> > > 2/7 - Adds MEM_RELEASE as a bpf_type_flag
> > > 3/7 - Adds bpf_dynptr_from_mem, bpf_dynptr_alloc, and bpf_dynptr_put
> > > 4/7 - Adds bpf_dynptr_read and bpf_dynptr_write
> > > 5/7 - Adds dynptr data slices (ptr to underlying dynptr memory)
> > > 6/7 - Adds dynptr support for ring buffers
> > > 7/7 - Tests to check that verifier rejects certain fail cases and passes
> > > certain success cases
> > >
> > > This is the first dynptr patchset in a larger series. The next series of
> > > patches will add persisting dynamic memory allocations in maps, parsing packet
> > > data through dynptrs, dynptrs to referenced objects, convenience helpers for
> > > using dynptrs as iterators, and more helper functions for interacting with
> > > strings and memory dynamically.
> > >
> >
> > test_verifier has 5 failed tests, the following diff fixes them (three for
> > changed verifier error string, and two because we missed to do offset checks for
> > ARG_PTR_TO_ALLOC_MEM in check_func_arg_reg_off). Since this is all, I guess you
> > can wait for the review to complete for this version before respinning.
> >
>
> Ugh, hit send too early.
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index bf64946ced84..24e5d494d991 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5681,7 +5681,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> /* Some of the argument types nevertheless require a
> * zero register offset.
> */
> - if (arg_type != ARG_PTR_TO_ALLOC_MEM)
> + if (base_type(arg_type) != ARG_PTR_TO_ALLOC_MEM)
> return 0;
> break;
> /* All the rest must be rejected, except PTR_TO_BTF_ID which allows
> diff --git a/tools/testing/selftests/bpf/verifier/ref_tracking.c b/tools/testing/selftests/bpf/verifier/ref_tracking.c
> index fbd682520e47..f1ad3b3cc145 100644
> --- a/tools/testing/selftests/bpf/verifier/ref_tracking.c
> +++ b/tools/testing/selftests/bpf/verifier/ref_tracking.c
> @@ -796,7 +796,7 @@
> },
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> .result = REJECT,
> - .errstr = "reference has not been acquired before",
> + .errstr = "arg 1 is an unacquired reference",
> },
> {
> /* !bpf_sk_fullsock(sk) is checked but !bpf_tcp_sock(sk) is not checked */
> diff --git a/tools/testing/selftests/bpf/verifier/sock.c b/tools/testing/selftests/bpf/verifier/sock.c
> index 86b24cad27a7..055a61205906 100644
> --- a/tools/testing/selftests/bpf/verifier/sock.c
> +++ b/tools/testing/selftests/bpf/verifier/sock.c
> @@ -417,7 +417,7 @@
> },
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> .result = REJECT,
> - .errstr = "reference has not been acquired before",
> + .errstr = "arg 1 is an unacquired reference",
> },
> {
> "bpf_sk_release(bpf_sk_fullsock(skb->sk))",
> @@ -436,7 +436,7 @@
> },
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> .result = REJECT,
> - .errstr = "reference has not been acquired before",
> + .errstr = "arg 1 is an unacquired reference",
> },
> {
> "bpf_sk_release(bpf_tcp_sock(skb->sk))",
> @@ -455,7 +455,7 @@
> },
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> .result = REJECT,
> - .errstr = "reference has not been acquired before",
> + .errstr = "arg 1 is an unacquired reference",
> },
> {
> "sk_storage_get(map, skb->sk, NULL, 0): value == NULL",
>
> > [...]
Awesome, thanks for noting this Kumar! I'll make sure to locally run
the verifier tests before I submit the next iteration of it upstream
>
> --
> Kartikeya
