On Sat, Nov 23, 2024 at 9:07 PM Amir Goldstein <amir73il@xxxxxxxxx> 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.
