| Message ID | 20241121112218.8249-4-jack@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fanotify: add pre-content hooks | expand |
On Thu, Nov 21, 2024 at 12:22 PM Jan Kara <jack@suse.cz> wrote: > > From: Amir Goldstein <amir73il@gmail.com> > > So far, we set FMODE_NONOTIFY_ flags at open time if we know that there > are no permission event watchers at all on the filesystem, but lack of > FMODE_NONOTIFY_ flags does not mean that the file is actually watched. > > For pre-content events, it is possible to optimize things so that we > don't bother trying to send pre-content events if file was not watched > (through sb, mnt, parent or inode itself) on open. Set FMODE_NONOTIFY_ > flags according to that. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > Signed-off-by: Jan Kara <jack@suse.cz> > Link: https://patch.msgid.link/2ddcc9f8d1fde48d085318a6b5a889289d8871d8.1731684329.git.josef@toxicpanda.com > --- > fs/notify/fsnotify.c | 27 +++++++++++++++++++++++++-- > include/linux/fsnotify_backend.h | 3 +++ > 2 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 569ec356e4ce..dd1dffd89fd6 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -193,7 +193,7 @@ static bool fsnotify_event_needs_parent(struct inode *inode, __u32 mnt_mask, > return mask & marks_mask; > } > > -/* Are there any inode/mount/sb objects that are interested in this event? */ > +/* Are there any inode/mount/sb objects that watch for these events? */ > static inline bool fsnotify_object_watched(struct inode *inode, __u32 mnt_mask, > __u32 mask) > { > @@ -632,7 +632,9 @@ EXPORT_SYMBOL_GPL(fsnotify); > */ > void file_set_fsnotify_mode(struct file *file) > { > - struct super_block *sb = file->f_path.dentry->d_sb; > + struct dentry *dentry = file->f_path.dentry, *parent; > + struct super_block *sb = dentry->d_sb; > + __u32 mnt_mask, p_mask; > > /* Is it a file opened by fanotify? */ > if (FMODE_FSNOTIFY_NONE(file->f_mode)) > @@ -658,6 +660,27 @@ void file_set_fsnotify_mode(struct file *file) > file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > return; > } This was lost in translation: @@ -672,8 +672,10 @@ void file_set_fsnotify_mode(struct file *file) /* * If there are permission event watchers but no pre-content event * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that. + * Pre-content events are only reported for regular files and dirs. */ - if (likely(!fsnotify_sb_has_priority_watchers(sb, + if ((!d_is_dir(dentry) && !d_is_reg(dentry)) || + likely(!fsnotify_sb_has_priority_watchers(sb, FSNOTIFY_PRIO_PRE_CONTENT))) { file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; return; > + > + /* > + * OK, there are some pre-content watchers. Check if anybody can be > + * watching for pre-content events on *this* file. > + */ > + mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask); > + if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) && > + !fsnotify_object_watched(d_inode(dentry), mnt_mask, > + FSNOTIFY_PRE_CONTENT_EVENTS))) { > + file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > + return; > + } > + > + /* Even parent is not watching for pre-content events on this file? */ > + parent = dget_parent(dentry); > + p_mask = fsnotify_inode_watches_children(d_inode(parent)); > + dput(parent); > + if (!(p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) { > + file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > + return; > + } inlining broke the logic and your branch fails the new PRE_ACCESS test cases of fanotify03 LTP test (now pushed to branch fan_hsm). Specifically in the test case that fails, parent is not watching and inode is watching pre-content and your code gets to the p_mask test and marks this file as no-pre-content watchers. This passes the test: @@ -684,17 +686,18 @@ void file_set_fsnotify_mode(struct file *file) * watching for pre-content events on *this* file. */ mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask); - if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) && - !fsnotify_object_watched(d_inode(dentry), mnt_mask, - FSNOTIFY_PRE_CONTENT_EVENTS))) { - file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; + if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask, + FSNOTIFY_PRE_CONTENT_EVENTS))) { return; } /* Even parent is not watching for pre-content events on this file? */ - parent = dget_parent(dentry); - p_mask = fsnotify_inode_watches_children(d_inode(parent)); - dput(parent); + p_mask = 0; + if (unlikely(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED)) { + parent = dget_parent(dentry); + p_mask = fsnotify_inode_watches_children(d_inode(parent)); + dput(parent); + } if (!(p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) { file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; return; Thanks, Amir.
On Thu 21-11-24 17:01:13, Amir Goldstein wrote: > On Thu, Nov 21, 2024 at 12:22 PM Jan Kara <jack@suse.cz> wrote: > > > > From: Amir Goldstein <amir73il@gmail.com> > > > > So far, we set FMODE_NONOTIFY_ flags at open time if we know that there > > are no permission event watchers at all on the filesystem, but lack of > > FMODE_NONOTIFY_ flags does not mean that the file is actually watched. > > > > For pre-content events, it is possible to optimize things so that we > > don't bother trying to send pre-content events if file was not watched > > (through sb, mnt, parent or inode itself) on open. Set FMODE_NONOTIFY_ > > flags according to that. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Signed-off-by: Jan Kara <jack@suse.cz> > > Link: https://patch.msgid.link/2ddcc9f8d1fde48d085318a6b5a889289d8871d8.1731684329.git.josef@toxicpanda.com > > --- > > fs/notify/fsnotify.c | 27 +++++++++++++++++++++++++-- > > include/linux/fsnotify_backend.h | 3 +++ > > 2 files changed, 28 insertions(+), 2 deletions(-) > > > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > > index 569ec356e4ce..dd1dffd89fd6 100644 > > --- a/fs/notify/fsnotify.c > > +++ b/fs/notify/fsnotify.c > > @@ -193,7 +193,7 @@ static bool fsnotify_event_needs_parent(struct inode *inode, __u32 mnt_mask, > > return mask & marks_mask; > > } > > > > -/* Are there any inode/mount/sb objects that are interested in this event? */ > > +/* Are there any inode/mount/sb objects that watch for these events? */ > > static inline bool fsnotify_object_watched(struct inode *inode, __u32 mnt_mask, > > __u32 mask) > > { > > @@ -632,7 +632,9 @@ EXPORT_SYMBOL_GPL(fsnotify); > > */ > > void file_set_fsnotify_mode(struct file *file) > > { > > - struct super_block *sb = file->f_path.dentry->d_sb; > > + struct dentry *dentry = file->f_path.dentry, *parent; > > + struct super_block *sb = dentry->d_sb; > > + __u32 mnt_mask, p_mask; > > > > /* Is it a file opened by fanotify? */ > > if (FMODE_FSNOTIFY_NONE(file->f_mode)) > > @@ -658,6 +660,27 @@ void file_set_fsnotify_mode(struct file *file) > > file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > > return; > > } > > This was lost in translation: > > @@ -672,8 +672,10 @@ void file_set_fsnotify_mode(struct file *file) > /* > * If there are permission event watchers but no pre-content event > * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that. > + * Pre-content events are only reported for regular files and dirs. > */ > - if (likely(!fsnotify_sb_has_priority_watchers(sb, > + if ((!d_is_dir(dentry) && !d_is_reg(dentry)) || > + likely(!fsnotify_sb_has_priority_watchers(sb, > FSNOTIFY_PRIO_PRE_CONTENT))) { > file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > return; Right. Specifically the (!d_is_dir(dentry) && !d_is_reg(dentry)) got lost, likely when I moved stuff between different commits and then was resolving patch conflicts. I'll add it back. Thanks for finding this! > > + > > + /* > > + * OK, there are some pre-content watchers. Check if anybody can be > > + * watching for pre-content events on *this* file. > > + */ > > + mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask); > > + if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) && > > + !fsnotify_object_watched(d_inode(dentry), mnt_mask, > > + FSNOTIFY_PRE_CONTENT_EVENTS))) { > > + file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > > + return; > > + } > > + > > + /* Even parent is not watching for pre-content events on this file? */ > > + parent = dget_parent(dentry); > > + p_mask = fsnotify_inode_watches_children(d_inode(parent)); > > + dput(parent); > > + if (!(p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) { > > + file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > > + return; > > + } > > inlining broke the logic and your branch fails the new PRE_ACCESS > test cases of fanotify03 LTP test (now pushed to branch fan_hsm). > > Specifically in the test case that fails, parent is not watching and > inode is watching pre-content and your code gets to the p_mask > test and marks this file as no-pre-content watchers. > > This passes the test: > > @@ -684,17 +686,18 @@ void file_set_fsnotify_mode(struct file *file) > * watching for pre-content events on *this* file. > */ > mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask); > - if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) && > - !fsnotify_object_watched(d_inode(dentry), mnt_mask, > - FSNOTIFY_PRE_CONTENT_EVENTS))) { > - file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > + if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask, > + FSNOTIFY_PRE_CONTENT_EVENTS))) { > return; > } > > /* Even parent is not watching for pre-content events on this file? */ > - parent = dget_parent(dentry); > - p_mask = fsnotify_inode_watches_children(d_inode(parent)); > - dput(parent); > + p_mask = 0; > + if (unlikely(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED)) { > + parent = dget_parent(dentry); > + p_mask = fsnotify_inode_watches_children(d_inode(parent)); > + dput(parent); > + } > if (!(p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) { > file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > return; Right. I've fixed that up. Thanks! LTP from your fan_hsm branch is now passing so I've re-pushed the current state to my fsnotify_hsm branch. Honza
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 569ec356e4ce..dd1dffd89fd6 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -193,7 +193,7 @@ static bool fsnotify_event_needs_parent(struct inode *inode, __u32 mnt_mask, return mask & marks_mask; } -/* Are there any inode/mount/sb objects that are interested in this event? */ +/* Are there any inode/mount/sb objects that watch for these events? */ static inline bool fsnotify_object_watched(struct inode *inode, __u32 mnt_mask, __u32 mask) { @@ -632,7 +632,9 @@ EXPORT_SYMBOL_GPL(fsnotify); */ void file_set_fsnotify_mode(struct file *file) { - struct super_block *sb = file->f_path.dentry->d_sb; + struct dentry *dentry = file->f_path.dentry, *parent; + struct super_block *sb = dentry->d_sb; + __u32 mnt_mask, p_mask; /* Is it a file opened by fanotify? */ if (FMODE_FSNOTIFY_NONE(file->f_mode)) @@ -658,6 +660,27 @@ void file_set_fsnotify_mode(struct file *file) file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; return; } + + /* + * OK, there are some pre-content watchers. Check if anybody can be + * watching for pre-content events on *this* file. + */ + mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask); + if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) && + !fsnotify_object_watched(d_inode(dentry), mnt_mask, + FSNOTIFY_PRE_CONTENT_EVENTS))) { + file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; + return; + } + + /* Even parent is not watching for pre-content events on this file? */ + parent = dget_parent(dentry); + p_mask = fsnotify_inode_watches_children(d_inode(parent)); + dput(parent); + if (!(p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) { + file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; + return; + } } #endif diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index 3ecf7768e577..9c105244815d 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -77,6 +77,9 @@ */ #define ALL_FSNOTIFY_DIRENT_EVENTS (FS_CREATE | FS_DELETE | FS_MOVE | FS_RENAME) +/* Pre-content events can be used to fill file content */ +#define FSNOTIFY_PRE_CONTENT_EVENTS 0 + #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \ FS_OPEN_EXEC_PERM)