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