Re: Fanotify API - Tracking File Movement

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> So I've slept on it and agree that allowing FAN_RENAME on a file with the
> semantics Matthew wants is consistent with the current design and probably
> the only sensible meaning we can give to it. I also agree that updating
> permission checks for reporting parent dirs isn't that big of a headache
> and maintenance burden going further.
>
> I'm still somewhat concerned about how the propagation of two parent
> directories and then formatting into the event is going to work out (i.e.,
> how many special cases that's going to need) but I'm willing to have a look
> at the patch. Maybe it won't be as bad as I was afraid :).

Here is a patch. I don't think it is so bad.(?)
In fact, I think the code is made a bit more clear when we stop overloading
ITER_TYPE_INODE as the new_dir in FS_RENAME case.
But yes, it does add yet another special (moved non-dir vs. moved dir).

Sorry Matthew, I was looking at the code to give you pointers, but there were
so many subtle details (as Jan has expected) that I could only communicate
them with a patch.
I tested that this patch does not break anything, but did not implement the
UAPI changes, so the functionality that it adds is not tested - I leave that
to you.

>
> > > Ah, I really wanted to stay away from watching the super block for all
> > > FAN_RENAME events. I feel like userspace wearing the pain for such
> > > cases is suboptimal, as this is something that can effectively be done
> > > in-kernel.
>
> I agree that kernel can do this more efficiently than userspace but the
> question is how much in terms of code (and thus maintenance overhead) are
> we willing to spend for this IMO rather specialized feature. The code to
> build necessary information to pass with the event, dealing with all
> different types of watches and backends and then formatting it to the event
> for userspace is complex as hell. Whenever I have to do or review some
> non-trivial changes to it, my head hurts ;) And the amount of subtle
> cornercase bugs we were fixing in that code over the years is just a
> testimony of this. So that's why I'm reluctant to add even small
> complications to it for something I find relatively specialized use (think
> for how many userspace programs this feature is going to be useful, I don't
> think many).
>

My 0.02$ - while FAN_RENAME is a snowflake, this is not because
of our design, this is because rename(2) is a snowflake vfs operation.
The event information simply reflects the operation complexity and when
looking at non-Linux filesystem event APIs, the event information for rename
looks very similar to FAN_RENAME. In some cases (lustre IIRC) the protocol
was enhanced at some point exactly as we did with FAN_RENAME to
have all the info in one event vs. having to join two events.

Hopefully, the attached patch simplifies the specialized implementation
a little bit.

But... (there is always a but when it comes to UAPI),
When looking at my patch, one cannot help wondering -
what about FAN_CREATE/FAN_DELETE/FAN_MOVE?
If those can report child fid, why should they be treated differently
than FAN_RENAME w.r.t marking the child inode?

For example, when watching a non-dir for FAN_CREATE, it could
be VERY helpful to get the dirfid+name of where the inode was
hard linked. In fact, if an application is watching FAN_RENAME to track
the movement of a non-dir file and does not watch hardlink+unlink, then
the file could escape under the application's nose.

We should definitely not extend the UAPI to a non-dir file before we provide
an answer to this question. For that reason I also sent v2 of Fix patch to
deny setting all dirent events on non-dir with FAN_REPORT_TARGET_FID.

Thanks,
Amir.
From d25f3ce8da49ce1a3b0a0621f0bf7b1d6ba2dad6 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Sat, 7 May 2022 10:04:08 +0300
Subject: [PATCH] fsnotify: send FS_RENAME to groups watching the moved inode

Send FS_RENAME to groups watching the moved inode itself if the
moved inode is a non-dir to allow tracking the movement of a watched
inode.

Sending FS_RENAME to a moved watched directory would be confusing
and FAN_MOVE_SELF provided enough information to track the movements
of a watched directory.

At the moment, no backend allows watching FS_RENAME on a non-dir inode.
When backends (i.e. fanotify) will allow watching FS_RENAME on a non-dir
inode, old/new dir+name should be reported only if the group proves to
have read permission on old/new dir.

Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
 fs/notify/fanotify/fanotify.c    |  4 +-
 fs/notify/fsnotify.c             | 72 ++++++++++++++++++++------------
 include/linux/fsnotify.h         | 19 +++++----
 include/linux/fsnotify_backend.h |  6 ++-
 4 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 985e995d2a39..fc498fc090da 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -780,9 +780,9 @@ static struct fanotify_event *fanotify_alloc_event(
 			report_old = report_new =
 				match_mask & (1U << FSNOTIFY_ITER_TYPE_SB);
 			report_old |=
-				match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE);
+				match_mask & (1U << FSNOTIFY_ITER_TYPE_OLD_DIR);
 			report_new |=
-				match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE2);
+				match_mask & (1U << FSNOTIFY_ITER_TYPE_NEW_DIR);
 
 			if (!report_old) {
 				/* Do not report old parent+name */
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 6eee19d15e8c..ce1ae725ec8c 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -277,19 +277,19 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
 	    WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
 		return 0;
 
-	/*
-	 * For FS_RENAME, 'dir' is old dir and 'data' is new dentry.
-	 * The only ->handle_inode_event() backend that supports FS_RENAME is
-	 * dnotify, where it means file was renamed within same parent.
-	 */
 	if (mask & FS_RENAME) {
-		struct dentry *moved = fsnotify_data_dentry(data, data_type);
+		inode_mark = fsnotify_iter_old_dir_mark(iter_info);
+		parent_mark = fsnotify_iter_new_dir_mark(iter_info);
 
-		if (dir != moved->d_parent->d_inode)
+		/*
+		 * The only ->handle_inode_event() backend that supports
+		 * FS_RENAME is dnotify, where DN_RENAME means that file
+		 * was renamed within the same parent.
+		 */
+		if (WARN_ON_ONCE(!inode_mark) ||
+		    inode_mark != parent_mark)
 			return 0;
-	}
-
-	if (parent_mark) {
+	} else if (parent_mark) {
 		/*
 		 * parent_mark indicates that the parent inode is watching
 		 * children and interested in this event, which is an event
@@ -479,9 +479,9 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	struct super_block *sb = fsnotify_data_sb(data, data_type);
 	struct fsnotify_iter_info iter_info = {};
 	struct mount *mnt = NULL;
-	struct inode *inode2 = NULL;
+	struct inode *dir1, *dir2;
 	struct dentry *moved;
-	int inode2_type;
+	int dir1_type = 0;
 	int ret = 0;
 	__u32 test_mask, marks_mask;
 
@@ -491,19 +491,31 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	if (!inode) {
 		/* Dirent event - report on TYPE_INODE to dir */
 		inode = dir;
-		/* For FS_RENAME, inode is old_dir and inode2 is new_dir */
-		if (mask & FS_RENAME) {
-			moved = fsnotify_data_dentry(data, data_type);
-			inode2 = moved->d_parent->d_inode;
-			inode2_type = FSNOTIFY_ITER_TYPE_INODE2;
-		}
+	} else if (mask & FS_RENAME) {
+		/* For FS_RENAME, dir1 is old_dir and dir2 is new_dir */
+		moved = fsnotify_data_dentry(data, data_type);
+		dir1 = moved->d_parent->d_inode;
+		dir2 = dir;
+		if (dir1->i_fsnotify_marks || dir2->i_fsnotify_marks)
+			dir1_type = FSNOTIFY_ITER_TYPE_OLD_DIR;
+		/*
+		 * Send FS_RENAME to groups watching the moved inode itself
+		 * only if the moved inode is a non-dir.
+		 * Sending FS_RENAME to a moved watched directory would be
+		 * confusing and FS_MOVE_SELF provided enough information to
+		 * track the movements of a watched directory.
+		 */
+		if (mask & FS_ISDIR)
+			inode = NULL;
 	} else if (mask & FS_EVENT_ON_CHILD) {
 		/*
 		 * Event on child - report on TYPE_PARENT to dir if it is
 		 * watching children and on TYPE_INODE to child.
 		 */
-		inode2 = dir;
-		inode2_type = FSNOTIFY_ITER_TYPE_PARENT;
+		dir1 = dir;
+		dir2 = NULL;
+		if (dir1->i_fsnotify_marks)
+			dir1_type = FSNOTIFY_ITER_TYPE_PARENT;
 	}
 
 	/*
@@ -516,7 +528,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	if (!sb->s_fsnotify_marks &&
 	    (!mnt || !mnt->mnt_fsnotify_marks) &&
 	    (!inode || !inode->i_fsnotify_marks) &&
-	    (!inode2 || !inode2->i_fsnotify_marks))
+	    !dir1_type)
 		return 0;
 
 	marks_mask = sb->s_fsnotify_mask;
@@ -524,8 +536,12 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 		marks_mask |= mnt->mnt_fsnotify_mask;
 	if (inode)
 		marks_mask |= inode->i_fsnotify_mask;
-	if (inode2)
-		marks_mask |= inode2->i_fsnotify_mask;
+	if (dir1_type) {
+		if (dir1)
+			marks_mask |= dir1->i_fsnotify_mask;
+		if (dir2)
+			marks_mask |= dir2->i_fsnotify_mask;
+	}
 
 
 	/*
@@ -550,9 +566,13 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 		iter_info.marks[FSNOTIFY_ITER_TYPE_INODE] =
 			fsnotify_first_mark(&inode->i_fsnotify_marks);
 	}
-	if (inode2) {
-		iter_info.marks[inode2_type] =
-			fsnotify_first_mark(&inode2->i_fsnotify_marks);
+	if (dir1_type) {
+		if (dir1)
+			iter_info.marks[dir1_type] =
+				fsnotify_first_mark(&dir1->i_fsnotify_marks);
+		if (dir2)
+			iter_info.marks[FSNOTIFY_ITER_TYPE_NEW_DIR] =
+				fsnotify_first_mark(&dir2->i_fsnotify_marks);
 	}
 
 	/*
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index bb8467cd11ae..75f1048443a5 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -28,18 +28,19 @@
  */
 static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
 				struct inode *dir, const struct qstr *name,
-				u32 cookie)
+				struct inode *inode, u32 cookie)
 {
 	if (atomic_long_read(&dir->i_sb->s_fsnotify_connectors) == 0)
 		return 0;
 
-	return fsnotify(mask, data, data_type, dir, name, NULL, cookie);
+	return fsnotify(mask, data, data_type, dir, name, inode, cookie);
 }
 
 static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
 				   __u32 mask)
 {
-	fsnotify_name(mask, dentry, FSNOTIFY_EVENT_DENTRY, dir, &dentry->d_name, 0);
+	fsnotify_name(mask, dentry, FSNOTIFY_EVENT_DENTRY, dir, &dentry->d_name,
+		      NULL, 0);
 }
 
 static inline void fsnotify_inode(struct inode *inode, __u32 mask)
@@ -154,13 +155,13 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 	}
 
 	/* Event with information about both old and new parent+name */
-	fsnotify_name(rename_mask, moved, FSNOTIFY_EVENT_DENTRY,
-		      old_dir, old_name, 0);
+	fsnotify(rename_mask, moved, FSNOTIFY_EVENT_DENTRY,
+		 old_dir, old_name, source, 0);
 
 	fsnotify_name(old_dir_mask, source, FSNOTIFY_EVENT_INODE,
-		      old_dir, old_name, fs_cookie);
+		      old_dir, old_name, NULL, fs_cookie);
 	fsnotify_name(new_dir_mask, source, FSNOTIFY_EVENT_INODE,
-		      new_dir, new_name, fs_cookie);
+		      new_dir, new_name, NULL, fs_cookie);
 
 	if (target)
 		fsnotify_link_count(target);
@@ -221,7 +222,7 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode,
 	audit_inode_child(dir, new_dentry, AUDIT_TYPE_CHILD_CREATE);
 
 	fsnotify_name(FS_CREATE, inode, FSNOTIFY_EVENT_INODE,
-		      dir, &new_dentry->d_name, 0);
+		      dir, &new_dentry->d_name, NULL, 0);
 }
 
 /*
@@ -241,7 +242,7 @@ static inline void fsnotify_delete(struct inode *dir, struct inode *inode,
 		mask |= FS_ISDIR;
 
 	fsnotify_name(mask, inode, FSNOTIFY_EVENT_INODE, dir, &dentry->d_name,
-		      0);
+		      NULL, 0);
 }
 
 /**
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9a1a9e78f69f..15c47e8b4ae3 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -374,11 +374,13 @@ static inline struct fs_error_report *fsnotify_data_error_report(
  * For example, both parent and child are watching an object of type inode.
  */
 enum fsnotify_iter_type {
+	FSNOTIFY_ITER_TYPE_NONE = 0,
 	FSNOTIFY_ITER_TYPE_INODE,
 	FSNOTIFY_ITER_TYPE_VFSMOUNT,
 	FSNOTIFY_ITER_TYPE_SB,
 	FSNOTIFY_ITER_TYPE_PARENT,
-	FSNOTIFY_ITER_TYPE_INODE2,
+	FSNOTIFY_ITER_TYPE_OLD_DIR,
+	FSNOTIFY_ITER_TYPE_NEW_DIR,
 	FSNOTIFY_ITER_TYPE_COUNT
 };
 
@@ -433,6 +435,8 @@ static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \
 
 FSNOTIFY_ITER_FUNCS(inode, INODE)
 FSNOTIFY_ITER_FUNCS(parent, PARENT)
+FSNOTIFY_ITER_FUNCS(old_dir, OLD_DIR)
+FSNOTIFY_ITER_FUNCS(new_dir, NEW_DIR)
 FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
 FSNOTIFY_ITER_FUNCS(sb, SB)
 
-- 
2.25.1


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux