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

quoted

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/ (local)