On Thu, Apr 6, 2023 at 2:05 PM Daniel Rosenberg <drosen@xxxxxxxxxx> wrote:
>
> On Tue, Apr 4, 2023 at 7:57 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > so the idea that bpf prog will see opaque ctx == name and a set
> > of kfuncs will extract different things from ctx into local dynptrs ?
> >
> > Have you considered passing dynptr-s directly into bpf progs
> > as arguments of struct_ops callbacks?
> > That would be faster or slower performance wise?
> >
>
> The kfunc records data that fuse then uses to clean up afterwards.
> That opaque struct seemed like the best place for it to live.
> Alternatively, I could provide dynpointers up front, but those are
> read only or read/write up front based on which operation they're
> dealing with, and may or may not be promotable to read/write, which
> involves creating a new dynpointer anyways. If I took that approach,
> I'd likely present a read-only dynpointer, and wrap it in a larger
> struct that contains the needed meta information. I'd definitely need
> to ensure that only fuse-bpf can call those kfuncs in that case.
>
> >
> > yep. Don't be shy from improving the verifier to your needs.
> >
>
> Uploaded a couple patches yesterday :)
> A patch to remove the buffer requirement for the slice functions, and
> another to accept dynpointer tagged mem as mem in helper functions.
>
>
> On Wed, Apr 5, 2023 at 11:49 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > note that even if bpf_dynptr_slice() accepts a buffer, it won't ever
> > touch it for LOCAL dynptrs, as the data is already linear in memory.
> > This buffer is filled out only for skb/xdp **and** only if requested
> > memory range can't be accessed as sequential memory. So you won't be
> > copying data.
> >
>
> I added an '__opt' tag annotation for this, so I can avoid needing to
> supply the buffer in those cases.
>
> > For bpf_dynptr_read(), yep, it will copy data. Regardless if you are
> > going to use it or not, we should relax the condition that final
> > buffer should be smaller or equal to dynptr actual size, it should be
> > bigger and we should just write to first N bytes of it.
> >
>
> Should dynptr_read return the length read? Otherwise you need to get
> the dynpointer length and adjust for all the offsets to figure out how
> much was probably read. But returning the length read would break
> existing programs.
You are right, that's a pretty big change in semantics. I take it
back, let's keep bpf_dynptr_read() as is. As it is currently, it
matches bpf_probe_read_{kernel,user} behavior and return semantics, so
it's good to keep all that consistent.
But I think that's ok, because your use case is solvable once we land
Joanne's bpf_dynptr_get_size() helper from her latest dynptr patch
set. Just to test, here's what I tried, and it works, so I think it
will also for for you with bpf_dynptr_get_size() to get a minimum of
your fixed buffer size and actual dynptr content size:
$ git diff
diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c
b/tools/testing/selftests/bpf/progs/dynptr_success.c
index b2fa6c47ecc0..4e0172156cc9 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -37,7 +37,7 @@ int test_read_write(void *ctx)
char write_data[64] = "hello there, world!!";
char read_data[64] = {};
struct bpf_dynptr ptr;
- int i;
+ int n, i;
if (bpf_get_current_pid_tgid() >> 32 != pid)
return 0;
@@ -46,9 +46,15 @@ int test_read_write(void *ctx)
/* Write data into the dynptr */
err = bpf_dynptr_write(&ptr, 0, write_data, sizeof(write_data), 0);
+ if (err)
+ goto cleanup;
+
+ n = bpf_get_prandom_u32();
+ if (n >= sizeof(read_data))
+ n = sizeof(read_data);
/* Read the data that was written into the dynptr */
- err = err ?: bpf_dynptr_read(read_data, sizeof(read_data), &ptr, 0, 0);
+ err = bpf_dynptr_read(read_data, n, &ptr, 0, 0);
/* Ensure the data we read matches the data we wrote */
for (i = 0; i < sizeof(read_data); i++) {
@@ -58,6 +64,7 @@ int test_read_write(void *ctx)
}
}
+cleanup:
bpf_ringbuf_discard_dynptr(&ptr, 0);
return 0;
}
> I also noticed that the various dynpointer offset/length checks don't
> seem to account for both the dynpointer offset and the read/write etc
> offset. All of the bounds checking there could use another pass.
If there are bugs, then yeah, let's definitely fix all of them. I
think offset problems are not noticeable right now, because we haven't
yet added the bpf_dynptr_advance() API, so currently offset is always
zero. So nothing should be broken right now, but please send fixes if
you've spotted issues.
>
> > >
> > > > At the moment I'm using bpf_dynptr_slice and declaring an empty and
> > > > unused buffer. I'm then hitting an issue with bpf_strncmp not
> > > > expecting mem that's tagged as being associated with a dynptr. I'm
> > > > currently working around that by adjusting check_reg_type to be less
> > > > picky, stripping away DYNPTR_TYPE_LOCAL if we're looking at an
> > > > ARG_PTR_TO_MEM. I suspect that would also be the case for other dynptr
> > > > types.
> >
> > So this seems unintended (or there is some unintentional misuse of
> > memory vs dynptr itself), we might be missing something in how we
> > handle arguments right now. It would be nice if you can send a patch
> > with a small selftest demonstrating this (and maybe a fix :) ).
> >
>
> Done :) Though not sure if the selftests I added are sufficient.
> https://lore.kernel.org/bpf/20230406004018.1439952-1-drosen@xxxxxxxxxx/
Yep, thanks! Already reviewed, let's iterate on respective patch set.
>
> >
> > We had previous discussions about whether to treat read-only as a
> > runtime-only or statically known attribute. There were pros and cons,
> > I don't remember what we ended up deciding. We do some custom handling
> > for some SKB programs, but it would be good to handle this
> > universally, yep.
> >
>
> I think it's currently not tracked, although the read only tag was
> being used for the dynptr itself. Almost tricked me there. In that
> case it'd probably be easier to add dynptr_data_ro than to add that
> information everywhere.
>
Not sure how hard it is to add static tracking of r/o vs r/w, but a
separate helper for read-only data probably makes sense anyways.
> >
> > +1. I feel like we are just missing a few helpers to help extract
> > and/or compare variable-sized strings (like bpf_dynptr_strncmp
> > mentioned earlier) for all this to work well. But a concrete use case
> > would allow us to design a coherent set of APIs to help with this.
>
> Should be able to get that patch set together soon. I'm reworking the
> test code with the verifier changes I mentioned above, and then will
> be doing a round of cleanup to make it a bit more possible to see
> what's actually in use now.
Nice.
