| Message ID | 20250203223205.861346-2-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix for huge faults regression | expand |
On Mon, Feb 03, 2025 at 11:32:03PM +0100, Amir Goldstein wrote: > The FMODE_NONOTIFY_* bits are a 2-bits mode. Open coding manipulation > of those bits is risky. Use an accessor file_set_fsnotify_mode() to > set the mode. > > Rename file_set_fsnotify_mode() => file_set_fsnotify_mode_from_watchers() > to make way for the simple accessor name. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > drivers/tty/pty.c | 2 +- > fs/notify/fsnotify.c | 18 ++++++++++++------ > fs/open.c | 7 ++++--- > include/linux/fs.h | 9 ++++++++- > include/linux/fsnotify.h | 4 ++-- > 5 files changed, 27 insertions(+), 13 deletions(-) > > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > index df08f13052ff4..8bb1a01fef2a1 100644 > --- a/drivers/tty/pty.c > +++ b/drivers/tty/pty.c > @@ -798,7 +798,7 @@ static int ptmx_open(struct inode *inode, struct file *filp) > nonseekable_open(inode, filp); > > /* We refuse fsnotify events on ptmx, since it's a shared resource */ > - filp->f_mode |= FMODE_NONOTIFY; > + file_set_fsnotify_mode(filp, FMODE_NONOTIFY); > > retval = tty_alloc_file(filp); > if (retval) > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 8ee495a58d0ad..77a1521335a10 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -648,7 +648,7 @@ EXPORT_SYMBOL_GPL(fsnotify); > * Later, fsnotify permission hooks do not check if there are permission event > * watches, but that there were permission event watches at open time. > */ > -void file_set_fsnotify_mode(struct file *file) > +void file_set_fsnotify_mode_from_watchers(struct file *file) > { > struct dentry *dentry = file->f_path.dentry, *parent; > struct super_block *sb = dentry->d_sb; > @@ -665,7 +665,7 @@ void file_set_fsnotify_mode(struct file *file) > */ > if (likely(!fsnotify_sb_has_priority_watchers(sb, > FSNOTIFY_PRIO_CONTENT))) { > - file->f_mode |= FMODE_NONOTIFY_PERM; > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM); > return; > } > > @@ -676,7 +676,7 @@ void file_set_fsnotify_mode(struct file *file) > 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; > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_HSM); > return; > } > > @@ -686,19 +686,25 @@ void file_set_fsnotify_mode(struct file *file) > */ > mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask); > if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask, > - FSNOTIFY_PRE_CONTENT_EVENTS))) > + FSNOTIFY_PRE_CONTENT_EVENTS))) { > + /* Enable pre-content events */ > + file_set_fsnotify_mode(file, 0); > return; > + } > > /* Is parent watching for pre-content events on this file? */ > if (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) > + if (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS) { > + /* Enable pre-content events */ > + file_set_fsnotify_mode(file, 0); > return; > + } > } > /* Nobody watching for pre-content events from this file */ > - file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_HSM); > } > #endif > > diff --git a/fs/open.c b/fs/open.c > index 932e5a6de63bb..3fcbfff8aede8 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -905,7 +905,8 @@ static int do_dentry_open(struct file *f, > f->f_sb_err = file_sample_sb_err(f); > > if (unlikely(f->f_flags & O_PATH)) { > - f->f_mode = FMODE_PATH | FMODE_OPENED | FMODE_NONOTIFY; > + f->f_mode = FMODE_PATH | FMODE_OPENED; > + file_set_fsnotify_mode(f, FMODE_NONOTIFY); > f->f_op = &empty_fops; > return 0; > } > @@ -938,7 +939,7 @@ static int do_dentry_open(struct file *f, > * If FMODE_NONOTIFY was already set for an fanotify fd, this doesn't > * change anything. > */ > - file_set_fsnotify_mode(f); > + file_set_fsnotify_mode_from_watchers(f); > error = fsnotify_open_perm(f); > if (error) > goto cleanup_all; > @@ -1122,7 +1123,7 @@ struct file *dentry_open_nonotify(const struct path *path, int flags, > if (!IS_ERR(f)) { > int error; > > - f->f_mode |= FMODE_NONOTIFY; > + file_set_fsnotify_mode(f, FMODE_NONOTIFY); > error = vfs_open(path, f); > if (error) { > fput(f); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index be3ad155ec9f7..e73d9b998780d 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -206,6 +206,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, > * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events. > * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - suppress only pre-content events. > */ > +#define FMODE_NONOTIFY_HSM \ > + (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM) After this patch series this define is used exactly twice and it's currently identical to FMODE_FSNOTIFY_HSM. I suggest to remove it and simply pass FMODE_NONOTIFY | FMODE_NONOTIFY_PERM in the two places it's used. I can do this myself though so if Jan doesn't have other comments don't bother resending. > #define FMODE_FSNOTIFY_MASK \ > (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM) > > @@ -222,7 +224,6 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, > #define FMODE_FSNOTIFY_HSM(mode) 0 > #endif > > - > /* > * Attribute flags. These should be or-ed together to figure out what > * has been changed! > @@ -3140,6 +3141,12 @@ static inline void exe_file_allow_write_access(struct file *exe_file) > allow_write_access(exe_file); > } > > +static inline void file_set_fsnotify_mode(struct file *file, fmode_t mode) > +{ > + file->f_mode &= ~FMODE_FSNOTIFY_MASK; > + file->f_mode |= mode; > +} > + > static inline bool inode_is_open_for_write(const struct inode *inode) > { > return atomic_read(&inode->i_writecount) > 0; > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > index 1a9ef8f6784dd..6a33288bd6a1f 100644 > --- a/include/linux/fsnotify.h > +++ b/include/linux/fsnotify.h > @@ -129,7 +129,7 @@ static inline int fsnotify_file(struct file *file, __u32 mask) > > #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > > -void file_set_fsnotify_mode(struct file *file); > +void file_set_fsnotify_mode_from_watchers(struct file *file); > > /* > * fsnotify_file_area_perm - permission hook before access to file range > @@ -213,7 +213,7 @@ static inline int fsnotify_open_perm(struct file *file) > } > > #else > -static inline void file_set_fsnotify_mode(struct file *file) > +static inline void file_set_fsnotify_mode_from_watchers(struct file *file) > { > } > > -- > 2.34.1 >
On Tue, Feb 4, 2025 at 11:43 AM Christian Brauner <brauner@kernel.org> wrote: > > On Mon, Feb 03, 2025 at 11:32:03PM +0100, Amir Goldstein wrote: > > The FMODE_NONOTIFY_* bits are a 2-bits mode. Open coding manipulation > > of those bits is risky. Use an accessor file_set_fsnotify_mode() to > > set the mode. > > > > Rename file_set_fsnotify_mode() => file_set_fsnotify_mode_from_watchers() > > to make way for the simple accessor name. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > drivers/tty/pty.c | 2 +- > > fs/notify/fsnotify.c | 18 ++++++++++++------ > > fs/open.c | 7 ++++--- > > include/linux/fs.h | 9 ++++++++- > > include/linux/fsnotify.h | 4 ++-- > > 5 files changed, 27 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > > index df08f13052ff4..8bb1a01fef2a1 100644 > > --- a/drivers/tty/pty.c > > +++ b/drivers/tty/pty.c > > @@ -798,7 +798,7 @@ static int ptmx_open(struct inode *inode, struct file *filp) > > nonseekable_open(inode, filp); > > > > /* We refuse fsnotify events on ptmx, since it's a shared resource */ > > - filp->f_mode |= FMODE_NONOTIFY; > > + file_set_fsnotify_mode(filp, FMODE_NONOTIFY); > > > > retval = tty_alloc_file(filp); > > if (retval) > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > > index 8ee495a58d0ad..77a1521335a10 100644 > > --- a/fs/notify/fsnotify.c > > +++ b/fs/notify/fsnotify.c > > @@ -648,7 +648,7 @@ EXPORT_SYMBOL_GPL(fsnotify); > > * Later, fsnotify permission hooks do not check if there are permission event > > * watches, but that there were permission event watches at open time. > > */ > > -void file_set_fsnotify_mode(struct file *file) > > +void file_set_fsnotify_mode_from_watchers(struct file *file) > > { > > struct dentry *dentry = file->f_path.dentry, *parent; > > struct super_block *sb = dentry->d_sb; > > @@ -665,7 +665,7 @@ void file_set_fsnotify_mode(struct file *file) > > */ > > if (likely(!fsnotify_sb_has_priority_watchers(sb, > > FSNOTIFY_PRIO_CONTENT))) { > > - file->f_mode |= FMODE_NONOTIFY_PERM; > > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM); > > return; > > } > > > > @@ -676,7 +676,7 @@ void file_set_fsnotify_mode(struct file *file) > > 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; > > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_HSM); > > return; > > } > > > > @@ -686,19 +686,25 @@ void file_set_fsnotify_mode(struct file *file) > > */ > > mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask); > > if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask, > > - FSNOTIFY_PRE_CONTENT_EVENTS))) > > + FSNOTIFY_PRE_CONTENT_EVENTS))) { > > + /* Enable pre-content events */ > > + file_set_fsnotify_mode(file, 0); > > return; > > + } > > > > /* Is parent watching for pre-content events on this file? */ > > if (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) > > + if (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS) { > > + /* Enable pre-content events */ > > + file_set_fsnotify_mode(file, 0); > > return; > > + } > > } > > /* Nobody watching for pre-content events from this file */ > > - file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_HSM); > > } > > #endif > > > > diff --git a/fs/open.c b/fs/open.c > > index 932e5a6de63bb..3fcbfff8aede8 100644 > > --- a/fs/open.c > > +++ b/fs/open.c > > @@ -905,7 +905,8 @@ static int do_dentry_open(struct file *f, > > f->f_sb_err = file_sample_sb_err(f); > > > > if (unlikely(f->f_flags & O_PATH)) { > > - f->f_mode = FMODE_PATH | FMODE_OPENED | FMODE_NONOTIFY; > > + f->f_mode = FMODE_PATH | FMODE_OPENED; > > + file_set_fsnotify_mode(f, FMODE_NONOTIFY); > > f->f_op = &empty_fops; > > return 0; > > } > > @@ -938,7 +939,7 @@ static int do_dentry_open(struct file *f, > > * If FMODE_NONOTIFY was already set for an fanotify fd, this doesn't > > * change anything. > > */ > > - file_set_fsnotify_mode(f); > > + file_set_fsnotify_mode_from_watchers(f); > > error = fsnotify_open_perm(f); > > if (error) > > goto cleanup_all; > > @@ -1122,7 +1123,7 @@ struct file *dentry_open_nonotify(const struct path *path, int flags, > > if (!IS_ERR(f)) { > > int error; > > > > - f->f_mode |= FMODE_NONOTIFY; > > + file_set_fsnotify_mode(f, FMODE_NONOTIFY); > > error = vfs_open(path, f); > > if (error) { > > fput(f); > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index be3ad155ec9f7..e73d9b998780d 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -206,6 +206,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, > > * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events. > > * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - suppress only pre-content events. > > */ > > +#define FMODE_NONOTIFY_HSM \ > > + (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM) > > After this patch series this define is used exactly twice and it's > currently identical to FMODE_FSNOTIFY_HSM. I suggest to remove it and > simply pass FMODE_NONOTIFY | FMODE_NONOTIFY_PERM in the two places it's > used. I can do this myself though so if Jan doesn't have other comments > don't bother resending. > I thought that file_set_fsnotify_mode(file, FMODE_NONOTIFY_HSM); is more readable then file_set_fsnotify_mode(FMODE_NONOTIFY | FMODE_NONOTIFY_PERM); and makes it clearer that the individual bits should not be messed with but since Jan was one who pushed for open coding f->f_mode = FMODE_PATH | FMODE_OPENED | FMODE_NONOTIFY; I think he will agree with you, so I humbly accept your suggestion :) Thanks, Amir.
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index df08f13052ff4..8bb1a01fef2a1 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -798,7 +798,7 @@ static int ptmx_open(struct inode *inode, struct file *filp) nonseekable_open(inode, filp); /* We refuse fsnotify events on ptmx, since it's a shared resource */ - filp->f_mode |= FMODE_NONOTIFY; + file_set_fsnotify_mode(filp, FMODE_NONOTIFY); retval = tty_alloc_file(filp); if (retval) diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 8ee495a58d0ad..77a1521335a10 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -648,7 +648,7 @@ EXPORT_SYMBOL_GPL(fsnotify); * Later, fsnotify permission hooks do not check if there are permission event * watches, but that there were permission event watches at open time. */ -void file_set_fsnotify_mode(struct file *file) +void file_set_fsnotify_mode_from_watchers(struct file *file) { struct dentry *dentry = file->f_path.dentry, *parent; struct super_block *sb = dentry->d_sb; @@ -665,7 +665,7 @@ void file_set_fsnotify_mode(struct file *file) */ if (likely(!fsnotify_sb_has_priority_watchers(sb, FSNOTIFY_PRIO_CONTENT))) { - file->f_mode |= FMODE_NONOTIFY_PERM; + file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM); return; } @@ -676,7 +676,7 @@ void file_set_fsnotify_mode(struct file *file) 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; + file_set_fsnotify_mode(file, FMODE_NONOTIFY_HSM); return; } @@ -686,19 +686,25 @@ void file_set_fsnotify_mode(struct file *file) */ mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask); if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask, - FSNOTIFY_PRE_CONTENT_EVENTS))) + FSNOTIFY_PRE_CONTENT_EVENTS))) { + /* Enable pre-content events */ + file_set_fsnotify_mode(file, 0); return; + } /* Is parent watching for pre-content events on this file? */ if (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) + if (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS) { + /* Enable pre-content events */ + file_set_fsnotify_mode(file, 0); return; + } } /* Nobody watching for pre-content events from this file */ - file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; + file_set_fsnotify_mode(file, FMODE_NONOTIFY_HSM); } #endif diff --git a/fs/open.c b/fs/open.c index 932e5a6de63bb..3fcbfff8aede8 100644 --- a/fs/open.c +++ b/fs/open.c @@ -905,7 +905,8 @@ static int do_dentry_open(struct file *f, f->f_sb_err = file_sample_sb_err(f); if (unlikely(f->f_flags & O_PATH)) { - f->f_mode = FMODE_PATH | FMODE_OPENED | FMODE_NONOTIFY; + f->f_mode = FMODE_PATH | FMODE_OPENED; + file_set_fsnotify_mode(f, FMODE_NONOTIFY); f->f_op = &empty_fops; return 0; } @@ -938,7 +939,7 @@ static int do_dentry_open(struct file *f, * If FMODE_NONOTIFY was already set for an fanotify fd, this doesn't * change anything. */ - file_set_fsnotify_mode(f); + file_set_fsnotify_mode_from_watchers(f); error = fsnotify_open_perm(f); if (error) goto cleanup_all; @@ -1122,7 +1123,7 @@ struct file *dentry_open_nonotify(const struct path *path, int flags, if (!IS_ERR(f)) { int error; - f->f_mode |= FMODE_NONOTIFY; + file_set_fsnotify_mode(f, FMODE_NONOTIFY); error = vfs_open(path, f); if (error) { fput(f); diff --git a/include/linux/fs.h b/include/linux/fs.h index be3ad155ec9f7..e73d9b998780d 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -206,6 +206,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events. * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - suppress only pre-content events. */ +#define FMODE_NONOTIFY_HSM \ + (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM) #define FMODE_FSNOTIFY_MASK \ (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM) @@ -222,7 +224,6 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, #define FMODE_FSNOTIFY_HSM(mode) 0 #endif - /* * Attribute flags. These should be or-ed together to figure out what * has been changed! @@ -3140,6 +3141,12 @@ static inline void exe_file_allow_write_access(struct file *exe_file) allow_write_access(exe_file); } +static inline void file_set_fsnotify_mode(struct file *file, fmode_t mode) +{ + file->f_mode &= ~FMODE_FSNOTIFY_MASK; + file->f_mode |= mode; +} + static inline bool inode_is_open_for_write(const struct inode *inode) { return atomic_read(&inode->i_writecount) > 0; diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index 1a9ef8f6784dd..6a33288bd6a1f 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -129,7 +129,7 @@ static inline int fsnotify_file(struct file *file, __u32 mask) #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS -void file_set_fsnotify_mode(struct file *file); +void file_set_fsnotify_mode_from_watchers(struct file *file); /* * fsnotify_file_area_perm - permission hook before access to file range @@ -213,7 +213,7 @@ static inline int fsnotify_open_perm(struct file *file) } #else -static inline void file_set_fsnotify_mode(struct file *file) +static inline void file_set_fsnotify_mode_from_watchers(struct file *file) { }
The FMODE_NONOTIFY_* bits are a 2-bits mode. Open coding manipulation of those bits is risky. Use an accessor file_set_fsnotify_mode() to set the mode. Rename file_set_fsnotify_mode() => file_set_fsnotify_mode_from_watchers() to make way for the simple accessor name. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- drivers/tty/pty.c | 2 +- fs/notify/fsnotify.c | 18 ++++++++++++------ fs/open.c | 7 ++++--- include/linux/fs.h | 9 ++++++++- include/linux/fsnotify.h | 4 ++-- 5 files changed, 27 insertions(+), 13 deletions(-)