Re: [PATCH v3 00/10] Sort out fsnotify_nameremove() mess

[Amir Goldstein <amir73il@gmail.com>]

On Mon, May 27, 2019 at 11:25 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, May 26, 2019 at 05:34:01PM +0300, Amir Goldstein wrote:
> > Jan,
> >
> > For v3 I went with a straight forward approach.
> > Filesystems that have fsnotify_{create,mkdir} hooks also get
> > explicit fsnotify_{unlink,rmdir} hooks.
> >
> > Hopefully, this approach is orthogonal to whatever changes Al is
> > planning for recursive tree remove code, because in many of the
> > cases, the hooks are added at the entry point for the recursive
> > tree remove.
> >
> > After looking closer at all the filesystems that were converted to
> > simple_remove in v2, I decided to exempt another 3 filesystems from
> > the fsnotify delete hooks: hypfs,qibfs and aafs.
> > hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user
> > configuration change, but they do not generate create events, so it
> > is less likely that users depend on the delete events.
> >
> > That leaves configfs the only filesystem that gets the new delete hooks
> > even though it does not have create hooks.
>
> why doesn't configfs have create hooks? That's what userspace does in
> configfs, shouldn't it be notified about it?  Keeping it "unequal" seems
> odd to me.
>

So it's not exactly that configfs has no create hooks at all.
For "normal" filesystems mkdir (for example) is only possible
by mkdir(2) syscall and there is create hook in vfs_mkdir().

The configfs (as well as debugfs/tracefs/etc), there are other code paths
that create directories, namely: configfs_register_grup/subsystem().
Those code paths have explicit fsnotify_mkdir() hook in debugfs/tracefs/etc,
but not in configfs. Why? because nobody put the hooks and no user
complained.

Should we add fsnotify_mkdir() hooks in configfs - probably yes.
I can do it as followup, but this is not the purpose of this patch set.
The purpose of this patch set (achieved in the last patch) is to simplify
the implementation of the fsnotify delete hook.
Today it is overly complicated by the fact that the hooks was placed
in d_delete() and d_delete() is called from some code paths that have
no business with fsnotify notifications at all.

Once this patch set is done sprinkling fsnotify_rmdir/unlink() hooks
in "proper" places, it removes the current fsnotify hook from d_delete().
d_delete() is called from configfs_unregister_group/subsystem(), so if
we do not add fsnotify delete hooks to configfs we will regress the existing
"unequal" behavior.
Maybe there are no users depending on fsnotify delete notifications from
configfs - I do not know. If we believe that is the case, we can drop the
configfs patch.
Maybe there *are* user depending on fsnotify delete notifications from aafs
(apparmorfs), which I decided to exclude from v3 patch set.
If we believe that is the case, or if we find out later that in case -
no problem
it is simple to add the missing fsnotify hooks in apparmorfs.

Thought? About inclusion of configfs? About exclusion of apparmorfs?
Again this is only exclusion/inclusion of hooks from code path that does
NOT come from vfs syscalls (e.g. aa_remove_profiles()).

Thanks,
Amir.