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

[Amir Goldstein <amir73il@gmail.com>]

> > As long as "exp_export: export of idmapped mounts not yet supported.\n"
> > I don't think it matters much.
> > It feels like adding idmapped mounts to nfsd is on your roadmap.
> > When you get to that we can discuss adding fsnotify path hooks to nfsd
> > if Jan agrees to the fsnotify path hooks concept.
>
> I was looking at the patch and thinking about it for a few days already. I
> think that generating fsnotify event later (higher up the stack where we
> have mount information) is fine and a neat idea. I just dislike the hackery
> with dentry flags.

Me as well. I used this hack for fast POC.

If we stick with the dual hooks approach, we will have to either pass a new
argument to vfs helpers or use another trick:

Convert all the many calls sites that were converted by Christian to:
   vfs_XXX(&init_user_ns, ...
because they do not have mount context, to:
   vfs_XXX(NULL, ...

Inside the vfs helpers, use init_user_ns when mnt_userns is NULL,
but pass the original mnt_userns argument to fsnotify_ns_XXX hooks.
A non-NULL mnt_userns arg means "path_notify" context.
I have already POC code for passing mnt_userns to fsnotify hooks [1].

I did not check if this assumption always works, but there seems to
be a large overlap between idmapped aware callers and use cases
that will require sending events to a mount mark.

> Also I'm somewhat uneasy that it is random (from
> userspace POV) when path event is generated and when not (at least that's
> my impression from the patch - maybe I'm wrong). How difficult would it be
> to get rid of it? I mean what if we just moved say fsnotify_create() call
> wholly up the stack? It would mean more explicit calls to fsnotify_create()
> from filesystems - as far as I'm looking nfsd, overlayfs, cachefiles,
> ecryptfs. But that would seem to be manageable.  Also, to maintain sanity,

1. I don't think we can do that for all the fsnotify_create() hooks, such as
    debugfs for example
2. It is useless to pass the mount from overlayfs to fsnotify, its a private
    mount that users cannot set a mark on anyway and Christian has
    promised to propose the same change for cachefiles and ecryptfs,
    so I think it's not worth the churn in those call sites
3. I am uneasy with removing the fsnotify hooks from vfs helpers and
    trusting that new callers of vfs_create() will remember to add the high
    level hooks, so I prefer the existing behavior remains for such callers

> we would probably have to lift generation of all directory events like
> that. That would be already notable churn but maybe doable... I know you've
> been looking at similar things in the past so if you are aware why this
> won't fly, please tell me.

I agree with that and since I posted this RFC patch, I have already added
support for FAN_DELETE and FAN_MOVE_SELF [2].
This was easy - not much churn at all.

FAN_MOVED_FROM I dropped because of the old name snapshot.
FAN_MOVED_TO I dropped because it needs the cookie to be in sync with
that of the FAN_MOVED_FROM event.

Besides, this event pair is "inotify legacy" as far as I am concerned.
FAN_MOVE_SELF can provide most of the needed functionality.
The rest of the functionality should be provided by a new event pair
IMO, FAN_LINK/FAN_UNLINK, as described in this proposal [3].

Which leaves us with two events: FAN_DELETE_SELF and FAN_ATTRIB.
FAN_DELETE_SELF is not appropriate for a mount mark IMO.
FAN_ATTRIB would be useful on mount mark IMO.
It would incur a bit more churn to add it, but I think it's certainly doable.

Just need to decide if we stay with the "dual hooks" approach and if so
on the technique to pass the "notify_path" state into vfs helpers and
existing fsnotify hooks.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fanotify_in_userns
[2] https://github.com/amir73il/linux/commits/fsnotify_path_hooks
[3] https://lore.kernel.org/linux-fsdevel/CAOQ4uxhEsbfA5+sW4XPnUKgCkXtwoDA-BR3iRO34Nx5c4y7Nug@mail.gmail.com/