Re: [PATCH] ovl: add RWF_NONITIFY flag to skip wrong duplicate fanotify event

[Amir Goldstein <amir73il@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5102 bytes --]

On Tue, Apr 23, 2019 at 6:05 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Apr 23, 2019 at 5:16 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, Apr 23, 2019 at 3:53 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Apr 23, 2019 at 4:41 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Tue, Apr 23, 2019 at 2:44 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > On Tue, Apr 23, 2019 at 2:40 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > > >
> > > > > > On Tue, Apr 23, 2019 at 1:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > > >
> > > > > > > On Tue, Apr 23, 2019 at 9:51 AM Murphy Zhou <jencce.kernel@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Overlays ovl_iter_write calls vfs_iter_write to write on real file,
> > > > > > > > in which calls fsnotify_modify on this change, however vfs_write also
> > > > > > > > calls fsnotify_modify after ovl_iter_write. The first notification
> > > > > > > > sent by vfs_iter_write grabs marks from upper inode and overlay mnt,
> > > > > > > > because of its fake path. The second one sent by vfs_write grabs marks
> > > > > > > > from ovl inode and ovl mnt.
> > > > > > > >
> > > > > > > > LTP fanotify06 add modify mark for mnt point, then add ignore modify
> > > > > > > > mask on testfile, then truncate and write the file. Because the ignore
> > > > > > > > mask is marked on ovl inode, not the upper inode, the first event is not
> > > > > > > > masked like the second one. So we get a modification event even with a
> > > > > > > > mask on the file.
> > > > > > >
> > > > > > > Care to extend fanotify06 in a similar manner to the way readahead02
> > > > > > > was extended to test overlay test case regardless of the base LTP filesystem?
> > > > > > >
> > > > > > > >
> > > > > > > > Proposing fixing this by add a new RWF flag to skip fsnotify on this IO.
> > > > > > > > vfs_iter_write used by ovl can use this flag to skip one duplicate event.
> > > > > > > >
> > > > > > >
> > > > > > > This fix is wrong for several reasons:
> > > > > > > - It exports RWF_NONOTIFY to uapi
> > > > > > > - It will cause no events at all when overlay writes to file even when user
> > > > > > > requested events on upper inode
> > > > > > >
> > > > > > > Please try attached patch.
> > > > > >
> > > > > > Would be nice, but until mmap stops using realfile this isn't a good solution.
> > > > > >
> > > > >
> > > > > Sigh! I figured there was a catch...
> > > > > Will it be ok if fake path used a cloned private mount of overlay mount?
> > > >
> > > > So the reason we have the fake path is e.g. /proc/$$/maps.  That can
> > > > only work with the original mount, AFAICS.
> > > >
> > > > We could have one realfile for regular I/O and a separate one for mmap
> > > > but that would increase complexity as well as resource use, so I'm not
> > > > quite sure if that's the right solution.
> > > >
> > >
> > > I see. Well what we could do is set a file flag for fake path, so at least
> > > fsnotify will be able to ignore f_path->mnt when calculating ignore mask.
> >
> > FMODE_NONOTIFY_MNT: like FMODE_NONOTIFY, but only affect mount marks?
> >
>
> On second thought I don't think there a need for an explicit
> FMODE_NONOTIFY_MNT/FMODE_FAKE_PATH flag because code
> can just check for file_inode(f) != d_inode(f->f_path->dentry).
> The question is what do do about this.
> I think situation is actually worse than reported (CC Jan).
>
> This problem existed even before stacked f_op, but is now manifesting
> differently because of stacked f_op.
>
> When user sets a mark on real inode or real parent dir inode, the
> user events that user gets when operations are performed via overlayfs file
> are with event->fd created by dentry_open(&event->path, ...
> So even when ignore mount marks are not involved, user will get the wrong
> path (overlayfs path) and referenced by event->fd while the mark was set
> on the real inode.
>
> The good new is that I believe fanotify marks are most commonly set on
> mount marks in the wild (?), so the events via overlayfs file on real inode
> won't be triggered in this case anyway and that's fine.
> FAN_MARK_FILESYSTEM may have opened up the possibility of exposing
> this bug to new users.
>
> In fsnotify_path(), when inode mismatches path->dentry->d_inode
> calling fsnotify_parent() is wrong (we are looking for parent watch of
> inode) and passing FSNOTIFY_EVENT_PATH info is wrong, because
> fsnotify() will filter by wrong mount and wrong path will we reported to
> user via event->fd.
>
> We could still report the event with FSNOTIFY_EVENT_INODE info,
> so at least inotify/audit watch on inode itself will work, but parent
> watches will not work as well as legacy fanotify inode watches.
> New FAN_REPORT_FID fanotify watches should work just fine.
>
> Any other ideas how to untangle?
>

How about this patch?

If prevents duplicate events due to "fake" path and addresses
the reported issue with ignore mask, but it leaves the "wrong"
paths reported with event->fd as is.

Murpphy, can you test it?

Thanks,
Amir.

[-- Attachment #2: 0001-fsnotify-do-not-generate-duplicate-fsnotify-events-f.patch.txt --]
[-- Type: text/plain, Size: 3044 bytes --]

From 2cc6fe16130e482176de105d6f906cf0047e2db8 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 23 Apr 2019 18:24:12 +0300
Subject: [PATCH] fsnotify: do not generate duplicate fsnotify events for
 "fake" path

Overlayfs "fake" path is used for stacked file operations on
underlying files.  Operations on files with "fake" path must not
generate events on mount marks and on parent watches, because
those events have already been generated at overlayfs layer.

The reported event->fd for inode/sb marks will have the wrong path
(overlayfs path), but we have no choice but to report them anyway.

Link: https://lore.kernel.org/linux-fsdevel/20190423065024.12695-1-jencce.kernel@gmail.com/
Reported-by: Murphy Zhou <jencce.kernel@gmail.com>
Fixes: d1d04ef8572b ("ovl: stack file ops")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c     | 18 ++++++++++++++++--
 include/linux/fsnotify.h | 14 +++++++++++---
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index df06f3da166c..ee5e4ea7e5ca 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -335,9 +335,23 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	__u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS);
 
 	if (data_is == FSNOTIFY_EVENT_PATH) {
-		mnt = real_mount(((const struct path *)data)->mnt);
-		mnt_or_sb_mask |= mnt->mnt_fsnotify_mask;
+		const struct path *path = (const struct path *)data;
+
+		/*
+		 * Overlayfs "fake" path used for stacked file operations on
+		 * underlying files must not generate events on mount marks,
+		 * because those events have already been generated at
+		 * overlayfs layer.  The reported event->fd for inode/sb marks
+		 * will have the wrong path (overlayfs path), but we have no
+		 * choice but to report them anyway.
+		 */
+		if (likely((mask & FS_EVENT_ON_CHILD) ||
+		    to_tell == d_inode(path->dentry))) {
+			mnt = real_mount(((const struct path *)data)->mnt);
+			mnt_or_sb_mask |= mnt->mnt_fsnotify_mask;
+		}
 	}
+
 	/* An event "on child" is not intended for a mount/sb mark */
 	if (mask & FS_EVENT_ON_CHILD)
 		mnt_or_sb_mask = 0;
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 09587e2860b5..2a197799c06d 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -47,10 +47,18 @@ static inline int fsnotify_parent(const struct path *path,
 static inline int fsnotify_path(struct inode *inode, const struct path *path,
 				__u32 mask)
 {
-	int ret = fsnotify_parent(path, NULL, mask);
+	/*
+	 * Overlayfs "fake" path used for stacked file operations on underlying
+	 * files must not generate events on parent, because those events have
+	 * already been generated at overlayfs layer.
+	 */
+	if (likely(inode == d_inode(path->dentry))) {
+		int ret = fsnotify_parent(path, NULL, mask);
+
+		if (ret)
+			return ret;
+	}
 
-	if (ret)
-		return ret;
 	return fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
 }
 
-- 
2.17.1