> On Nov 23, 2024, at 10:25 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Sat, Nov 23, 2024 at 12:00 AM Song Liu <song@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
