On Tue, Feb 4, 2025 at 11:43 AM Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
