Re: [PATCH v3 fanotify 1/2] fanotify: Introduce fanotify filter

[Song Liu <songliubraving@meta.com>]

> On Nov 23, 2024, at 10:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> 
> On Sat, Nov 23, 2024 at 12:00 AM Song Liu <song@kernel.org> wrote:

[...]

>> 
>> To make fanotify filters more flexible, a filter can take arguments at
>> attach time.
>> 
>> sysfs entry /sys/kernel/fanotify_filter is added to help users know
>> which fanotify filters are available. At the moment, these files are
>> added for each filter: flags, desc, and init_args.
> 
> It's a shame that we have fanotify knobs at /proc/sys/fs/fanotify/ and
> in sysfs, but understand we don't want to make more use of proc for this.
> 
> Still I would add the filter files under a new /sys/fs/fanotify/ dir and not
> directly under /sys/kernel/

I don't have a strong preference either way. We can create it under
/sys/fs/fanotify if that makes more sense. 

> 
>> 
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>> fs/notify/fanotify/Kconfig           |  13 ++
>> fs/notify/fanotify/Makefile          |   1 +
>> fs/notify/fanotify/fanotify.c        |  44 +++-
>> fs/notify/fanotify/fanotify_filter.c | 289 +++++++++++++++++++++++++++
>> fs/notify/fanotify/fanotify_user.c   |   7 +
>> include/linux/fanotify.h             | 128 ++++++++++++
>> include/linux/fsnotify_backend.h     |   6 +-
>> include/uapi/linux/fanotify.h        |  36 ++++
>> 8 files changed, 520 insertions(+), 4 deletions(-)
>> create mode 100644 fs/notify/fanotify/fanotify_filter.c
[...]
>> 
>>        BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS);
>>        BUILD_BUG_ON(FAN_MODIFY != FS_MODIFY);
>> @@ -921,6 +924,39 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
>>        pr_debug("%s: group=%p mask=%x report_mask=%x\n", __func__,
>>                 group, mask, match_mask);
>> 
>> +       if (FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS))
>> +               fsid = fanotify_get_fsid(iter_info);
>> +
>> +#ifdef CONFIG_FANOTIFY_FILTER
>> +       filter_hook = srcu_dereference(group->fanotify_data.filter_hook, &fsnotify_mark_srcu);
> 
> Do we actually need the sleeping rcu protection for calling the hook?
> Can regular rcu read side be nested inside srcu read side?

I was thinking the filter function can still sleep, for example, to 
read an xattr. 

> 
> Jan,
> 
> I don't remember why srcu is needed since we are not holding it
> when waiting for userspace anymore?
> 
>> +       if (filter_hook) {
>> +               struct fanotify_filter_event filter_event = {
>> +                       .mask = mask,
>> +                       .data = data,
>> +                       .data_type = data_type,
>> +                       .dir = dir,
>> +                       .file_name = file_name,
>> +                       .fsid = &fsid,
>> +                       .match_mask = match_mask,
>> +               };

[...]

>> +
>> +       spin_lock(&filter_list_lock);
>> +       filter_ops = fanotify_filter_find(args.name);
>> +       if (!filter_ops || !try_module_get(filter_ops->owner)) {
>> +               spin_unlock(&filter_list_lock);
>> +               ret = -ENOENT;
>> +               goto err_free_hook;
>> +       }
>> +       spin_unlock(&filter_list_lock);
>> +
>> +       if (!capable(CAP_SYS_ADMIN) && (filter_ops->flags & FAN_FILTER_F_SYS_ADMIN_ONLY)) {
> 
> 1. feels better to opt-in for UNPRIV (and maybe later on) rather than
> make it the default.

Sure. 

> 2. need to check that filter_ops->flags has only "known" flags

Agreed. 

> 3. probably need to add filter_ops->version check in case we want to
> change the ABI

I think we can let the author of the filter handle versioning 
inside filter_init function. Many users may not need any logic
for compatibility. 

> 
>> +               ret = -EPERM;
>> +               goto err_module_put;
>> +       }
>> +

[...]

>> +
>> +/*
>> + * fanotify_filter_del - Delete a filter from fsnotify_group.
>> + */
>> +void fanotify_filter_del(struct fsnotify_group *group)
>> +{
>> +       struct fanotify_filter_hook *filter_hook;
>> +
>> +       fsnotify_group_lock(group);
>> +       filter_hook = group->fanotify_data.filter_hook;
>> +       if (!filter_hook)
>> +               goto out;
>> +
>> +       rcu_assign_pointer(group->fanotify_data.filter_hook, NULL);
>> +       fanotify_filter_hook_free(filter_hook);
> 
> The read side is protected with srcu and there is no srcu/rcu delay of freeing.
> You will either need something along the lines of
> fsnotify_connector_destroy_workfn() with synchronize_srcu()

Yeah, we do need a synchronize_srcu() here. I will fix this in 
the next version. 

> or use regular rcu delay and read side (assuming that it can be nested inside
> srcu read side?).

Thanks for the review!

Song