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. 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@xxxxxxxxxx/ > > 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.
