On Mon, Nov 18, 2024 at 12:51 PM Song Liu <songliubraving@xxxxxxxx> wrote: > > > > > On Nov 15, 2024, at 1:05 PM, Song Liu <songliubraving@xxxxxxxx> wrote: > > [...] > > > >> > >> fsnotify_open_perm->fsnotify->send_to_group->fanotify_handle_event. > >> > >> is a pretty long path to call bpf prog and > >> preparing a giant 'struct fanotify_fastpath_event' > >> is not going to fast either. > >> > >> If we want to accelerate that with bpf it needs to be done > >> sooner with negligible overhead. > > > > Agreed. This is actually something I have been thinking > > since the beginning of this work: Shall it be fanotify-bpf > > or fsnotify-bpf. Given we have more materials, this is a > > good time to have broader discussions on this. > > > > @all, please chime in whether we should redo this as > > fsnotify-bpf. AFAICT: > > > > Pros of fanotify-bpf: > > - There is existing user space that we can leverage/reuse. > > > > Pros of fsnotify-bpf: > > - Faster fast path. > > > > Another major pros/cons did I miss? > > Adding more thoughts on this: I think it makes more sense to > go with fanotify-bpf. This is because one of the benefits of > fsnotify/fanotify over LSM solutions is the built-in event > filtering of events. While this call chain is a bit long: > > fsnotify_open_perm->fsnotify->send_to_group->fanotify_handle_event. > > There are built-in filtering in fsnotify() and > send_to_group(), so logics in the call chain are useful. fsnotify_marks based filtering happens in fsnotify. No need to do more indirect calls to get to fanotify. I would add the bpf struct_ops hook right before send_to_group or inside of it. Not sure whether fsnotify_group concept should be reused or avoided. Per inode mark/mask filter should stay. > struct fanotify_fastpath_event is indeed big. But I think > we need to pass these information to the fastpath handler > either way. Disagree. That was the old way of hooking bpf bits in. uapi/bpf.h is full of such "context" structs. xpd_md, bpf_tcp_sock, etc. They pack fields into one struct only because old style bpf has one input argument: ctx. struct_ops doesn't have this limitation. Pass things like path/dentry/inode/whatever pointers directly. No need to pack into fanotify_fastpath_event. > Overall, I think current fastpath design makes sense, > though there are things we need to fix (as Amir and Alexei > pointed out). Please let me know comments and suggestions > on this. On one side you're arguing that extra indirection for inode local storage due to inode->i_secruity is needed for performance, but on the other side you're not worried about the deep call stack of fsnotify->fanotify and argument packing which add way more overhead than i_security hop.
