Re: [RFC/PATCH v2 bpf-next fanotify 1/7] fanotify: Introduce fanotify fastpath handler

[Song Liu <songliubraving@meta.com>]

Hi Amir,

> On Nov 15, 2024, at 12:51 AM, Amir Goldstein <amir73il@gmail.com> wrote:

[...]

>> 
>> +#ifdef CONFIG_FANOTIFY_FASTPATH
>> +       fp_hook = srcu_dereference(group->fanotify_data.fp_hook, &fsnotify_mark_srcu);
>> +       if (fp_hook) {
>> +               struct fanotify_fastpath_event fp_event = {
>> +                       .mask = mask,
>> +                       .data = data,
>> +                       .data_type = data_type,
>> +                       .dir = dir,
>> +                       .file_name = file_name,
>> +                       .fsid = &fsid,
>> +                       .match_mask = match_mask,
>> +               };
>> +
>> +               ret = fp_hook->ops->fp_handler(group, fp_hook, &fp_event);
>> +               if (ret == FAN_FP_RET_SKIP_EVENT) {
>> +                       ret = 0;
>> +                       goto finish;
>> +               }
>> +       }
>> +#endif
>> +
> 
> To me it makes sense that the fastpath module could also return a negative
> (deny) result for permission events.

Yes, this should just work. And I actually plan to use it. 

> Is there a specific reason that you did not handle this or just didn't think
> of this option?

But I haven't tested permission events yet. At first glance, maybe we just
need to change the above code a bit, as:


>> f (ret == FAN_FP_RET_SKIP_EVENT) {
>> +                       ret = 0;
>> +                       goto finish;
>> +               }

if (ret != FAN_FP_RET_SEND_TO_USERSPACE) { 
	if (ret == FAN_FP_RET_SKIP_EVENT)
		ret = 0;
	goto finish;
}

Well, I guess we should change the value of FAN_FP_RET_SEND_TO_USERSPACE,
so that this condition will look better. 

We may also consider reorder the code so that we do not call
fsnotify_prepare_user_wait() when the fastpath handles the event. 

Does this look reasonable?

Thanks,
Song