On Fri, Sep 30, 2022 at 5:05 PM Daniel Rosenberg <drosen@xxxxxxxxxx> wrote:
>
> > - Please make the patches that can be applied to the bpf-next tree cleanly.
> > For example, in patch 3, where is 18e2ec5bf453 coming from? I cannot find it in
> > bpf-next and linux-next tree.
> > - Without applying it to an upstream tree cleanly, in a big set like this, I
> > have no idea when bpf_prog_run() is called in patch 24 because the diff context
> > is in fuse_bpf_cleanup and apparently it is not where the bpf prog is run.
> >
>
> Currently this is based off of
> bf682942cd26ce9cd5e87f73ae099b383041e782 in
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> I would have rebased on top of bpf-next, except that from my
> conversations at plumbers, I figured that the set up would need to
> change significantly, and that effort would be wasted. My goal
> including them here was to give more of a sense of what our needs are,
> and be a starting point for working out what we really ought to be
> using.
It was a good idea to send it early :)
>
> > Some high level comments on the set:
> > - Instead of adding bpf helpers, you should consider kfunc instead. You can take
> > a look at the recent HID patchset v10 or the recent nf conntrack bpf set.
> >
> > - Instead of expressing as packet data, using the recent dynptr is a better way
> > to go for handling a mem blob.
> >
>
> I'll look into those, I remember them coming up at LPC. My current use
> of packets/buffers does seem to abuse their intended meaning a bit.
This 'abuse' is sorta, kinda, ok-ish. We can accept that,
but once you convert to kfunc interface you might realize that
"packet" abstraction is not necessary here and there are
cleaner alternatives. Have you looked at dynptr ?
> > - iiuc, the idea is to allow bpf prog to optionally handle the 'struct
> > file_operations' without going back to the user daemon? Have you looked at
> > struct_ops which seems to be a better fit here? If the bpf prog does not know
> > how to handle an operation (or file?), it can call fuse_file_llseek (for
> > example) as a kfunc to handle the request.
> >
>
> I wasn't aware of struct_ops. It looks like that may work for us
> instead of making a new prog type. I'll definitely look into that.
> I'll likely sign up for the bpf office hours next week.
I have to second everything that Martin suggested.
To reiterate his points in different words:
. patch 26 with printk debug only gives very low
confidence that the presented api towards bpf programs
will be usable.
The patch series gotta have a production worthy bpf program
that actually does things you want it to do.
. please use kfunc mechanism similar to the way hid-bpf is doing.
If individual funcs are not enough and you need to attach
a set of bpf programs all at once then use struct_ops.
kfuncs are prefered if you don't need atomicity of a set of progs.
If it's fine to attach progs one at a time to different nop==empty
functions than just use a set of nop funcs and call back into
the kernel with kfuncs.
That would be easier to rip out when api turns out to
be insufficient or extensions are necessary.
> > - The test SEC("test_trace") seems mostly a synthetic test for checking
> > correctness. Does it have a test that shows a more real life use case? or I
> > have missed things in patch 26?
> >
>
> Patch 26 is pretty much all synthetic tests. A lot of them are just
> ensuring that we even call in to the bpf program, and some limited
> testing that changing the filters has the expected results. We
> mentioned a few more concrete usecases in the LPC talk. One of those
> is folder hiding. In Android we've had an issue with leaking the
> existence of some apps to other apps. We needed to hide certain
> directories from apps where they do have permissions to traverse the
> directory. Attempting to access a folder without permissions would
> result in EPERM, revealing the existence of that folder. Since the
> application doesn't have permission to create arbitrary folders at
> that level, we can hide it by using fuse-bpf to change the EPERM into
> an ENOENT, and then filter readdir to remove disallowed entries. You
> can see something like that in bpf_test_redact_readdir. We also have
> some file level redaction. If an app doesn't have location
> permissions, but does have file permissions, they could just read
> picture metadata to get location information. We could have bpf
> redirect reads that might contain location data to the daemon, while
> passing through other parts.
The described use cases sound useful.
Just present them as real bpf progs.
My main concern, though, is with patches 7-25.
I don't understand file systems, but it looks like the patches
bring features into fuse from other file systems.
In many ways it looks like overlayfs.
Maybe just add bpf hooks to overlayfs?
Why fuse at all?
It might look like a bunch of nop functions sprinkled around overlayfs
code.
If you need to talk to user space from bpf prog you'll have
ringbuf and user_ringbuf to stream data from bpf prog to user
and back.
