Dynptrs and Strings

Dynptrs and Strings

[Daniel Rosenberg]

For Fuse-BPF, I need to access strings and blocks of data in a bpf
context. In the initial patch set for Fuse-BPF [1] I was using
PTR_TO_PACKETs as a stand in for buffers of unknown size. That used
start and end pointers to give the verifier the ability to statically
check bounds. I'm currently attempting to switch this over to using
dynptrs for buffers of variable length, but have run into some issues.

So far as I can tell, while a dynptr can have a variable size, any
time you interact with a dynptr, you need to already know how large it
is. For instance, bpf_dynptr_read reads the dynptr into a local
buffer. However, that buffer may not be larger than the dynptr you're
reading. That seems pretty counter intuitive to me.
bpf_dynptr_check_off_len ensures that the length passed in is not
longer than the dynptr. This means I can't, for example, have a buffer
that could support NAME_MAX characters, and read a dynptr into it. I
assume this is to ensure that the entirety of the buffer is
initialized. If that's the case, I could create a variant that zeroes
out the remaining buffer area.

One workaround I've considered is attempting to read to the minimum
length of string I'm comparing against, treating read failures as a
nonmatching string. Then I could read any additional space for larger
comparisons afterwards. This would mean one call to dynptr_read for
every string length I'm checking against.

The bpf_dynptr_slice(_rdrw) functions looks nearly like what I want,
but require a buffer that will be unused for local dynptrs.
bpf_dynptr_data rejects readonly dynptrs, so is not a suitable
replacement. It seems like I could really use either a
bpf_dynptr_data_rdonly helper, or a similar kfunc, though I suspect
the kfunc will require some additions to the special_kfunc_set.

One alternative I'm looking into is providing kfuncs that perform the
requested operations. That allows checks to happen at runtime.
However, I'm having some difficulties working with strings as kfunc
arguments. There is an existing helper function bpf_strncmp, which
uses one constant argument which ends up interpreted as a
PTR_TO_MAP_VALUE. To make a kfunc, I assume I'd need to add another
special kfunc and then adjust the expected types.

Any of these solutions that use fixed sizes ignore cases where I may
need to compare two strings, neither of which is a constant string
known at compile time.
How should I go about using possibly read-only dynptrs whose lengths
are only known at runtime?

[1] https://lore.kernel.org/bpf/20220926231822.994383-1-drosen@google.com/


-Daniel

Re: Dynptrs and Strings

[Andrii Nakryiko]

On Mon, Apr 3, 2023 at 6:28 PM Daniel Rosenberg <drosen@google.com> wrote:
>
> For Fuse-BPF, I need to access strings and blocks of data in a bpf
> context. In the initial patch set for Fuse-BPF [1] I was using
> PTR_TO_PACKETs as a stand in for buffers of unknown size. That used
> start and end pointers to give the verifier the ability to statically
> check bounds. I'm currently attempting to switch this over to using
> dynptrs for buffers of variable length, but have run into some issues.
>
> So far as I can tell, while a dynptr can have a variable size, any
> time you interact with a dynptr, you need to already know how large it
> is. For instance, bpf_dynptr_read reads the dynptr into a local
> buffer. However, that buffer may not be larger than the dynptr you're
> reading. That seems pretty counter intuitive to me.
> bpf_dynptr_check_off_len ensures that the length passed in is not
> longer than the dynptr. This means I can't, for example, have a buffer
> that could support NAME_MAX characters, and read a dynptr into it. I
> assume this is to ensure that the entirety of the buffer is
> initialized. If that's the case, I could create a variant that zeroes
> out the remaining buffer area.

you are right, we should probably fix this logic to allow to fill up
min(<dynptr size>, <fixed buf size>) bytes of provided fixed-sized
buffers. Zero-filling at the end is an option, but I'm not sure we
have to do it (e.g., bpf_probe_read_str() variants don't zero fill, I
think).

>
> One workaround I've considered is attempting to read to the minimum
> length of string I'm comparing against, treating read failures as a
> nonmatching string. Then I could read any additional space for larger
> comparisons afterwards. This would mean one call to dynptr_read for
> every string length I'm checking against.
>
> The bpf_dynptr_slice(_rdrw) functions looks nearly like what I want,
> but require a buffer that will be unused for local dynptrs.
> bpf_dynptr_data rejects readonly dynptrs, so is not a suitable
> replacement. It seems like I could really use either a
> bpf_dynptr_data_rdonly helper, or a similar kfunc, though I suspect
> the kfunc will require some additions to the special_kfunc_set.

bpf_dynptr_slice(_rdwr) is basically the same as bpf_dynptr_data() and
bpf_dynptr_read() as far as fixed length of read/accessible memory
goes, so I don't think it gives you anything new, tbh.

>
> One alternative I'm looking into is providing kfuncs that perform the
> requested operations. That allows checks to happen at runtime.
> However, I'm having some difficulties working with strings as kfunc
> arguments. There is an existing helper function bpf_strncmp, which
> uses one constant argument which ends up interpreted as a
> PTR_TO_MAP_VALUE. To make a kfunc, I assume I'd need to add another
> special kfunc and then adjust the expected types.

I think we need something like bpf_dynptr_strncmp(), which would take
two dynptrs, one for each string you'd like to compare.

But in general, when you say "I need to access strings and blocks of
data " above, what exact operations do you need to do on them? strncmp
is one of them, anything else?

Note, Joanne is working on another dynptr-related patch set that adds
bpf_dynptr_clone()+bpf_dynptr_trim()+bpf_dynptr_advance(), which
allows to create temporary smaller views into other dynptrs, which
will make dynptr a universal "memory view" mechanism. So designing
APIs with assumption that dynptr is describing a smaller portion of a
bigger variable-sized memory is on the table with that.

>
> Any of these solutions that use fixed sizes ignore cases where I may
> need to compare two strings, neither of which is a constant string
> known at compile time.
> How should I go about using possibly read-only dynptrs whose lengths
> are only known at runtime?
>
> [1] https://lore.kernel.org/bpf/20220926231822.994383-1-drosen@google.com/
>
>
> -Daniel

Re: Dynptrs and Strings

[Daniel Rosenberg]

On Mon, Apr 3, 2023 at 9:57 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> bpf_dynptr_slice(_rdwr) is basically the same as bpf_dynptr_data() and
> bpf_dynptr_read() as far as fixed length of read/accessible memory
> goes, so I don't think it gives you anything new, tbh.
>

It does in the case of a readonly dynptr. bpf_dynptr_read requires a
buffer. bpf_dynptr_data rejects readonly dynptrs, and doesn't really
provide a way for the verifier to know the resulting buffer is
readonly. bpf_dynptr_slice handles read-only dynptrs, but requires a
buffer which will be unused for local dynptrs. Verification fails if I
try to pass it a null pointer and a nonzero buffer length. That means
to gain read only access to a read only dynptr of size 255, I'd need
to either copy it with dynptr read, or declare an initialized buffer
of size 255 which will be unused in order to gain direct access to the
dynptr data.

>
> I think we need something like bpf_dynptr_strncmp(), which would take
> two dynptrs, one for each string you'd like to compare.
>
> But in general, when you say "I need to access strings and blocks of
> data " above, what exact operations do you need to do on them? strncmp
> is one of them, anything else?
>

For blocks of data, probably memcmp. The upcoming dynptr apis should
make editing specific parts easier. Might want a memmove style command
at some point?
For context, fuse-bpf allows intercepting FUSE_READ and editing the
data before it's passed along. Or at least it will ;) The upcoming
dynptr APIS you mentioned should handle most  things there.

Re: Dynptrs and Strings

[Alexei Starovoitov]

On Tue, Apr 4, 2023 at 2:06 PM Daniel Rosenberg <drosen@google.com> wrote:
>
> On Mon, Apr 3, 2023 at 9:57 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > bpf_dynptr_slice(_rdwr) is basically the same as bpf_dynptr_data() and
> > bpf_dynptr_read() as far as fixed length of read/accessible memory
> > goes, so I don't think it gives you anything new, tbh.
> >
>
> It does in the case of a readonly dynptr. bpf_dynptr_read requires a
> buffer. bpf_dynptr_data rejects readonly dynptrs, and doesn't really
> provide a way for the verifier to know the resulting buffer is
> readonly. bpf_dynptr_slice handles read-only dynptrs, but requires a
> buffer which will be unused for local dynptrs. Verification fails if I
> try to pass it a null pointer and a nonzero buffer length. That means
> to gain read only access to a read only dynptr of size 255, I'd need
> to either copy it with dynptr read, or declare an initialized buffer
> of size 255 which will be unused in order to gain direct access to the
> dynptr data.

I'm pretty sure we can make bpf_dynptr_data() support readonly dynptrs.
Should be easy to add in the verifier.
But could you pseudo code what you're trying to do first?

Do you expect bpf prog to see both ro and rw dynptrs on input?
And you want bpf prog to use bpf_dynptr_data() to access these buffers
wrapped as dynptr-s?
The string manipulation questions muddy the picture here.
If bpf progs deals with file system block data why strings?
Is that a separate set of bpf prog hooks that receive strings on
input wrapped as dynptrs?
What are those strings? file path?
We need more info to help you design the interface, so it's easy to
use from bpf prog pov.

Re: Dynptrs and Strings

[Daniel Rosenberg]

On Tue, Apr 4, 2023 at 3:58 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> I'm pretty sure we can make bpf_dynptr_data() support readonly dynptrs.
> Should be easy to add in the verifier.
> But could you pseudo code what you're trying to do first?
>

I'm trying to do something like this:

bpf_fuse_get_ro_dynptr(name, &name_ptr);
name_buf = bpf_dynptr_data(&name_ptr, 4);
if (!bpf_strncmp(name_buf, 4, "test"))
   return 42;
return 0;

Really I just want to work with the data in the dynptrs in as
convenient a way as possible.
I'd like to avoid copying the data if it isn't necessary.

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.

I guess for dynptr_data to support R/O, the dynptr in question would
just need to be tagged as read-only or read/write by the verifier
previously, and then it could just pass along that tag to the mem it
returns.

>
> Do you expect bpf prog to see both ro and rw dynptrs on input?
> And you want bpf prog to use bpf_dynptr_data() to access these buffers
> wrapped as dynptr-s?
> The string manipulation questions muddy the picture here.
> If bpf progs deals with file system block data why strings?
> Is that a separate set of bpf prog hooks that receive strings on
> input wrapped as dynptrs?
> What are those strings? file path?
> We need more info to help you design the interface, so it's easy to
> use from bpf prog pov.

I have a few usecases for them. I've restructured fuse-bpf to use
struct ops. Each fuse operation has an associated prefilter and
postfilter op.
At the moment I'm using dynptrs for any variable length block of data
that these ops need. For many operations, this includes the file name.
In some, a path. Other times, it's file data, or xattr names/data.
They can all have different sizes, and may be backed by data that may
not be changed, like the dentry name field. I have a pair of kfuncs
for requesting a dynptr from an opaque storage type, so you can avoid
having to make unnecessary copies if you're not planning on making
modifications. The r/w version of the kfunc will allocate new space
and copy data as needed. They're a direct analog of the helper
functions from the initial patch. The opaque structure includes
information about the current and max size of the argument, whether
the current backing is read-only, and other flags for cleanup if the
helper allocates memory.

The Fuse bpf programs may alter those fields, and then returns whether
it was able to handle the request, or if the request must go to
userspace.

While the fields have a max size, their actual size isn't available
until runtime. That makes the current set of dynptr functions largely
unusable, since I believe they all require a known constant size to
access, and I don't think the helper to return the length of a dynptr
counts for that as far as the verifier is concerned. Worst case, I
suppose more of those interactions could happen behind kfuncs, where
they can perform runtime checks. That may end up being overly
restrictive though.

Re: Dynptrs and Strings

[Alexei Starovoitov]

On Tue, Apr 04, 2023 at 06:50:08PM -0700, Daniel Rosenberg wrote:
> On Tue, Apr 4, 2023 at 3:58 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > I'm pretty sure we can make bpf_dynptr_data() support readonly dynptrs.
> > Should be easy to add in the verifier.
> > But could you pseudo code what you're trying to do first?
> >
> 
> I'm trying to do something like this:
> 
> bpf_fuse_get_ro_dynptr(name, &name_ptr);

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?

> name_buf = bpf_dynptr_data(&name_ptr, 4);
> if (!bpf_strncmp(name_buf, 4, "test"))
>    return 42;
> return 0;
> 
> Really I just want to work with the data in the dynptrs in as
> convenient a way as possible.
> I'd like to avoid copying the data if it isn't necessary.

of course.

> 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.
> 
> I guess for dynptr_data to support R/O, the dynptr in question would
> just need to be tagged as read-only or read/write by the verifier
> previously, and then it could just pass along that tag to the mem it
> returns.

yep. Don't be shy from improving the verifier to your needs.

> >
> > Do you expect bpf prog to see both ro and rw dynptrs on input?
> > And you want bpf prog to use bpf_dynptr_data() to access these buffers
> > wrapped as dynptr-s?
> > The string manipulation questions muddy the picture here.
> > If bpf progs deals with file system block data why strings?
> > Is that a separate set of bpf prog hooks that receive strings on
> > input wrapped as dynptrs?
> > What are those strings? file path?
> > We need more info to help you design the interface, so it's easy to
> > use from bpf prog pov.
> 
> I have a few usecases for them. I've restructured fuse-bpf to use
> struct ops. Each fuse operation has an associated prefilter and
> postfilter op.
> At the moment I'm using dynptrs for any variable length block of data
> that these ops need. For many operations, this includes the file name.
> In some, a path. Other times, it's file data, or xattr names/data.
> They can all have different sizes, and may be backed by data that may
> not be changed, like the dentry name field. I have a pair of kfuncs
> for requesting a dynptr from an opaque storage type, so you can avoid
> having to make unnecessary copies if you're not planning on making
> modifications. The r/w version of the kfunc will allocate new space
> and copy data as needed. They're a direct analog of the helper
> functions from the initial patch. The opaque structure includes
> information about the current and max size of the argument, whether
> the current backing is read-only, and other flags for cleanup if the
> helper allocates memory.
> 
> The Fuse bpf programs may alter those fields, and then returns whether
> it was able to handle the request, or if the request must go to
> userspace.
> 
> While the fields have a max size, their actual size isn't available
> until runtime. That makes the current set of dynptr functions largely
> unusable, since I believe they all require a known constant size to
> access, and I don't think the helper to return the length of a dynptr
> counts for that as far as the verifier is concerned. Worst case, I
> suppose more of those interactions could happen behind kfuncs, where
> they can perform runtime checks. That may end up being overly
> restrictive though.

Overall makes sense, but a bit too abstract for concrete suggestions.
I think it would be best if you just send your rough patches to the list
with comments where things still need work and we will go from there.

Re: Dynptrs and Strings

[Andrii Nakryiko]

On Tue, Apr 4, 2023 at 7:57 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Apr 04, 2023 at 06:50:08PM -0700, Daniel Rosenberg wrote:
> > On Tue, Apr 4, 2023 at 3:58 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > I'm pretty sure we can make bpf_dynptr_data() support readonly dynptrs.
> > > Should be easy to add in the verifier.
> > > But could you pseudo code what you're trying to do first?
> > >
> >
> > I'm trying to do something like this:
> >
> > bpf_fuse_get_ro_dynptr(name, &name_ptr);
>
> 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?
>
> > name_buf = bpf_dynptr_data(&name_ptr, 4);
> > if (!bpf_strncmp(name_buf, 4, "test"))
> >    return 42;
> > return 0;
> >
> > Really I just want to work with the data in the dynptrs in as
> > convenient a way as possible.
> > I'd like to avoid copying the data if it isn't necessary.
>
> of course.

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.

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.

>
> > 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 :) ).

> >
> > I guess for dynptr_data to support R/O, the dynptr in question would
> > just need to be tagged as read-only or read/write by the verifier
> > previously, and then it could just pass along that tag to the mem it
> > returns.
>
> yep. Don't be shy from improving the verifier to your needs.

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.

>
> > >
> > > Do you expect bpf prog to see both ro and rw dynptrs on input?
> > > And you want bpf prog to use bpf_dynptr_data() to access these buffers
> > > wrapped as dynptr-s?
> > > The string manipulation questions muddy the picture here.
> > > If bpf progs deals with file system block data why strings?
> > > Is that a separate set of bpf prog hooks that receive strings on
> > > input wrapped as dynptrs?
> > > What are those strings? file path?
> > > We need more info to help you design the interface, so it's easy to
> > > use from bpf prog pov.
> >
> > I have a few usecases for them. I've restructured fuse-bpf to use
> > struct ops. Each fuse operation has an associated prefilter and
> > postfilter op.
> > At the moment I'm using dynptrs for any variable length block of data
> > that these ops need. For many operations, this includes the file name.
> > In some, a path. Other times, it's file data, or xattr names/data.
> > They can all have different sizes, and may be backed by data that may
> > not be changed, like the dentry name field. I have a pair of kfuncs
> > for requesting a dynptr from an opaque storage type, so you can avoid
> > having to make unnecessary copies if you're not planning on making
> > modifications. The r/w version of the kfunc will allocate new space
> > and copy data as needed. They're a direct analog of the helper
> > functions from the initial patch. The opaque structure includes
> > information about the current and max size of the argument, whether
> > the current backing is read-only, and other flags for cleanup if the
> > helper allocates memory.
> >
> > The Fuse bpf programs may alter those fields, and then returns whether
> > it was able to handle the request, or if the request must go to
> > userspace.
> >
> > While the fields have a max size, their actual size isn't available
> > until runtime. That makes the current set of dynptr functions largely
> > unusable, since I believe they all require a known constant size to
> > access, and I don't think the helper to return the length of a dynptr
> > counts for that as far as the verifier is concerned. Worst case, I

correct, because operations like bpf_dynptr_data() and
bpf_dynptr_slice() are asking verifier to access any of N requested
bytes directly. So the verifier has to make sure that all N bytes (if
returned as non-NULL pointer) are valid and accessible.

> > suppose more of those interactions could happen behind kfuncs, where
> > they can perform runtime checks. That may end up being overly
> > restrictive though.
>
> Overall makes sense, but a bit too abstract for concrete suggestions.
> I think it would be best if you just send your rough patches to the list
> with comments where things still need work and we will go from there.

+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.

Re: Dynptrs and Strings

[Daniel Rosenberg]

On Tue, Apr 4, 2023 at 7:57 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> 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@gmail.com> 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.
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.

> >
> > > 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@google.com/

>
> 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.

>
> +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.

Re: Dynptrs and Strings

[Andrii Nakryiko]

On Thu, Apr 6, 2023 at 2:05 PM Daniel Rosenberg <drosen@google.com> wrote:
>
> On Tue, Apr 4, 2023 at 7:57 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> 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@gmail.com> 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@google.com/

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.