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

[Song Liu <songliubraving@meta.com>]

> On Nov 26, 2024, at 4:50 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 

[...]

>>> +
>>> +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.

Hmm.. I am not sure whether we cannot hold a refcount. If that is a 
requirement, the algorithm will be more complex. 

IIUC, fsnotify_mark on a inode does not hold a refcount to inode. 
And when the inode is evicted, the mark is freed. I guess this 
requires the user space, the AntiVirus scanner for example, to 
hold a reference to the inode? If this is the case, I think it 
is OK for the filter, either bpf or kernel module, to hold a 
reference to the subtree root. 

> 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.

Handling multiple subtrees is indeed an issue. Since we rely on 
the mark in the SB, multiple subtrees under the same SB will share
that mark. Unless we use some cache, accessing a file will 
trigger multiple is_subdir() calls. 

One possible solution is that have a new helper that checks
is_subdir() for a list of parent subtrees with a single series
of dentry walk. IOW, something like:

bool is_subdir_of_any(struct dentry *new_dentry, 
                      struct list_head *list_of_dentry).

For BPF, one possible solution is to walk the dentry tree 
up to the root, under bpf_rcu_read_lock(). 

Does this sound reasonable?

Thanks,
Song