Re: [RFC][PATCH] fanotify: allow setting FAN_CREATE in mount mark mask

[Amir Goldstein <amir73il@gmail.com>]

> > > My example probably would be something like:
> > >
> > > mount -t ext4 /dev/sdb /A
> > >
> > > 1. FAN_MARK_MOUNT(/A)
> > >
> > > mount --bind /A /B
> > >
> > > 2. FAN_MARK_MOUNT(/B)
> > >
> > > mount -t ecryptfs /B /C
> > >
> > > 3. FAN_MARK_MOUNT(/C)
> > >
> > > let's say I now do
> > >
> > > touch /C/bla
> > >
> > > I may be way off here but intuitively it seems both 1. and 2. should get
> > > a creation event but not 3., right?
> > >
> >
> > Why not 3?
> > You explicitly set a mark on /C requesting to be notified when
> > objects are created via /C.
>
> Sorry, that was a typo. I meant to write, both 2. and 3. should get a
> creation event but not 1.
>
> >
> > > But with your proposal would both 1. and 2. still get a creation event?
> > >
>
> Same obvious typo. The correct question would be: with your proposal do
> 2. and 3. both get an event?
>
> Because it feels like they both should since /C is mounted on top of /B
> and ecryptfs acts as a shim. Both FAN_MARK_MOUNT(/B) and
> FAN_MARK_MOUNT(/C) should get a creation event after all both will have
> mnt->mnt_fsnotify_marks set.
>

Right.

There are two ways to address this inconsistency:
1. Change internal callers of vfs_ helpers to use a private mount,
    as you yourself suggested for ecryptfs and cachefiles
2. Add fsnotify_path_ hooks at caller site - that would be the
    correct thing to do for nfsd IMO

> >
> > They would not get an event, because fsnotify() looks for CREATE event
> > subscribers on inode->i_fsnotify_marks and inode->i_sb_s_fsnotify_marks
> > and does not find any.
>
> Well yes, but my example has FAN_MARK_MOUNT(/B) set. So fanotify
> _should_ look at
>             (!mnt || !mnt->mnt_fsnotify_marks) &&
> and see that there are subscribers and should notify the subscribers in
> /B even if the file is created through /C.
>
> My point is with your solution this can't be handled and I want to make
> sure that this is ok. Because right now you'd not be notified about a
> new file having been created in /B even though mnt->mnt_fsnotify_marks
> is set and the creation went through /B via /C.
>

If you are referring to the ecryptfs use case specifically, then I think it is
ok. After all, whether ecryptfs uses a private mount clone or not is not
something the user can know.

> _Unless_ we switch to an argument like overlayfs and say "This is a
> private mount which is opaque and so we don't need to generate events.".
> Overlayfs handles this cleanly due to clone_private_mount() which will
> shed all mnt->mnt_fsnotify_marks and ecryptfs should too if that is the
> argument we follow, no?
>

There is simply no way that the user can infer from the documentation
of FAN_MARK_MOUNT that the event on /B is expected when /B is
underlying layer of ecryptfs or overlayfs.
It requires deep internal knowledge of the stacked fs implementation.
In best case, the user can infer that she MAY get an event on /B.
Some users MAY also expect to get an event on /A because they do not
understand the concept of bind mounts...
Clone a mount ns and you will get more lost users...

> >
> > The vfs_create() -> fsnotify_create() hook passes data_type inode to
> > fsnotify() so there is no fsnotify_data_path() to extract mnt event
> > subscribers from.
>
> Right, that was my point. You don't have the mnt context for the
> underlying fs at a time when e.g. call vfs_link() which ultimately calls
> fsnotify_create/link() which I'm saying might be a problem.
>

It's a problem. If it wasn't a problem I wouldn't need to work around it ;-)

It would be a problem if people think that the FAN_MOUNT_MARK
is a subtree mark, which it certainly is not. And I have no doubt that
as Jan said, people really do want a subtree mark.

My question to you with this RFC is: Does the ability to subscribe to
CREATE/DELETE/MOVE events on a mount help any of your use
cases? With or without the property that mount marks are allowed
inside userns for idmapped mounts.

Note that if we think the semantics of this are useful for container
managers, but too complex for most mortals, we may decide to
restrict the ability to subscribe to those events to idmapped mounts(?).

Thanks,
Amir.