[RFC/PATCH v2 bpf-next fanotify 7/7] selftests/bpf: Add test for BPF based fanotify fastpath handler

[Song Liu]

Hi Alexei,

> On Nov 18, 2024, at 4:10 PM, Alexei Starovoitov <alexei.starovoitov at gmail.com> 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