[PATCH v3 fanotify 2/2] samples/fanotify: Add a sample fanotify fiter

[Alexei Starovoitov]

On Sat, Nov 23, 2024 at 9:07 PM Amir Goldstein <amir73il at gmail.com> wrote:
> > +++ b/samples/fanotify/filter-mod.c
> > @@ -0,0 +1,105 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> > +
> > +#include <linux/fsnotify.h>
> > +#include <linux/fanotify.h>
> > +#include <linux/module.h>
> > +#include <linux/path.h>
> > +#include <linux/file.h>
> > +#include "filter.h"
> > +
> > +struct fan_filter_sample_data {
> > +       struct path subtree_path;
> > +       enum fan_filter_sample_mode mode;
> > +};
> > +
> > +static int sample_filter(struct fsnotify_group *group,
> > +                        struct fanotify_filter_hook *filter_hook,
> > +                        struct fanotify_filter_event *filter_event)
> > +{
> > +       struct fan_filter_sample_data *data;
> > +       struct dentry *dentry;
> > +
> > +       dentry = fsnotify_data_dentry(filter_event->data, filter_event->data_type);
> > +       if (!dentry)
> > +               return FAN_FILTER_RET_SEND_TO_USERSPACE;
> > +
> > +       data = filter_hook->data;
> > +
> > +       if (is_subdir(dentry, data->subtree_path.dentry)) {
> > +               if (data->mode == FAN_FILTER_SAMPLE_MODE_BLOCK)
> > +                       return -EPERM;
> > +               return FAN_FILTER_RET_SEND_TO_USERSPACE;
> > +       }
> > +       return FAN_FILTER_RET_SKIP_EVENT;
> > +}
> > +
> > +static int sample_filter_init(struct fsnotify_group *group,
> > +                             struct fanotify_filter_hook *filter_hook,
> > +                             void *argp)
> > +{
> > +       struct fan_filter_sample_args *args;
> > +       struct fan_filter_sample_data *data;
> > +       struct file *file;
> > +       int fd;
> > +
> > +       args = (struct fan_filter_sample_args *)argp;
> > +       fd = args->subtree_fd;
> > +
> > +       file = fget(fd);
> > +       if (!file)
> > +               return -EBADF;
> > +       data = kzalloc(sizeof(struct fan_filter_sample_data), GFP_KERNEL);
> > +       if (!data) {
> > +               fput(file);
> > +               return -ENOMEM;
> > +       }
> > +       path_get(&file->f_path);
> > +       data->subtree_path = file->f_path;
> > +       fput(file);
> > +       data->mode = args->mode;
> > +       filter_hook->data = data;
> > +       return 0;
> > +}
> > +
> > +static void sample_filter_free(struct fanotify_filter_hook *filter_hook)
> > +{
> > +       struct fan_filter_sample_data *data = filter_hook->data;
> > +
> > +       path_put(&data->subtree_path);
> > +       kfree(data);
> > +}
> > +
>
> Hi Song,
>
> This example looks fine but it raises a question.
> This filter will keep the mount of subtree_path busy until the group is closed
> or the filter is detached.
> This is probably fine for many services that keep the mount busy anyway.
>
> But what if this wasn't the intention?
> What if an Anti-malware engine that watches all mounts wanted to use that
> for configuring some ignore/block subtree filters?
>
> One way would be to use a is_subtree() variant that looks for a
> subtree root inode
> number and then verifies it with a subtree root fid.
> A production subtree filter will need to use a variant of is_subtree()
> anyway that
> looks for a set of subtree root inodes, because doing a loop of is_subtree() for
> multiple paths is a no go.
>
> Don't need to change anything in the example, unless other people
> think that we do need to set a better example to begin with...

I think we have to treat this patch as a real filter and not as an example
to make sure that the whole approach is workable end to end.
The point about not holding path/dentry is very valid.
The algorithm needs to support that.
It may very well turn out that the logic of handling many filters
without a loop and not grabbing a path refcnt is too complex for bpf.
Then this subtree filtering would have to stay as a kernel module
or extra flag/feature for fanotify.