> 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
