Hi Alexei, > On Nov 18, 2024, at 4:10 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: [...] >>> >>> 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. We still need fsnotify_group. It matches each fanotify file descriptor. Moving struct_ops hook inside send_to_group does save us an indirect call. But this also means we need to introduce the fastpath concept to both fsnotify and fanotify. I personally don't really like duplications like this (see the big BUILD_BUG_ON array in fanotify_handle_event). OTOH, maybe the benefit of one fewer indirect call justifies the extra complexity. Let me think more about it. > >> 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. OK. I am convinced on this one. I will adjust the code to remove 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, The biggest issue with inode_local_storage in i_security is not the extra indirection and thus the extra latency. The bigger problem is, once we make inode local storage available to tracing programs, we will not disable it at boot time. Therefore, the extra indirection through i_security doesn't give us any memory savings. Instead, it will cause latency and memory fragmentations. IOW, we are paying the cost for no benefits at all. > 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. I think the difference is one indirect call. But that may worth the work. I will think more about it. Also, I would really appreciate other folks' input. Thanks again for your review! Song
