Message-id: <171819286884.14261.11045203598673536466@noble.neil.brown.name>
Date: Wed, 12 Jun 2024 21:47:48 +1000
From: "NeilBrown" <neilb@...e.de>
To: "Amir Goldstein" <amir73il@...il.com>
Cc: "Alexander Viro" <viro@...iv.linux.org.uk>,
 "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "James Clark" <james.clark@....com>, ltp@...ts.linux.it,
 linux-nfs@...r.kernel.org, "LKML" <linux-kernel@...r.kernel.org>,
 linux-fsdevel@...r.kernel.org, "Miklos Szeredi" <miklos@...redi.hu>
Subject: Re: [PATCH v2] VFS: generate FS_CREATE before FS_OPEN when
 ->atomic_open used.

On Wed, 12 Jun 2024, Amir Goldstein wrote:
> On Wed, Jun 12, 2024 at 10:10 AM NeilBrown <neilb@...e.de> wrote:
> >
> >
> > When a file is opened and created with open(..., O_CREAT) we get
> > both the CREATE and OPEN fsnotify events and would expect them in that
> > order.   For most filesystems we get them in that order because
> > open_last_lookups() calls fsnofify_create() and then do_open() (from
> > path_openat()) calls vfs_open()->do_dentry_open() which calls
> > fsnotify_open().
> >
> > However when ->atomic_open is used, the
> >    do_dentry_open() -> fsnotify_open()
> > call happens from finish_open() which is called from the ->atomic_open
> > handler in lookup_open() which is called *before* open_last_lookups()
> > calls fsnotify_create.  So we get the "open" notification before
> > "create" - which is backwards.  ltp testcase inotify02 tests this and
> > reports the inconsistency.
> >
> > This patch lifts the fsnotify_open() call out of do_dentry_open() and
> > places it higher up the call stack.  There are three callers of
> > do_dentry_open().
> >
> > For vfs_open() and kernel_file_open() the fsnotify_open() is placed
> > directly in that caller so there should be no behavioural change.
> >
> > For finish_open() there are two cases:
> >  - finish_open is used in ->atomic_open handlers.  For these we add a
> >    call to fsnotify_open() at the top of do_open() if FMODE_OPENED is
> >    set - which means do_dentry_open() has been called.
> >  - finish_open is used in ->tmpfile() handlers.  For these a similar
> >    call to fsnotify_open() is added to vfs_tmpfile()
> 
> Any handlers other than ovl_tmpfile()?

Local filesystems tend to call finish_open_simple() which is a trivial
wrapper around finish_open().
Every .tmpfile handler calls either finish_open() or finish_open_simple().

> This one is a very recent and pretty special case.
> Did open(O_TMPFILE) used to emit an OPEN event before that change?

I believe so, yes.

Thanks,
NeilBrown

> 
> >
> > With this patch NFSv3 is restored to its previous behaviour (before
> > ->atomic_open support was added) of generating CREATE notifications
> > before OPEN, and NFSv4 now has that same correct ordering that is has
> > not had before.  I haven't tested other filesystems.
> >
> > Fixes: 7c6c5249f061 ("NFS: add atomic_open for NFSv3 to handle O_TRUNC correctly.")
> > Reported-by: James Clark <james.clark@....com>
> > Closes: https://lore.kernel.org/all/01c3bf2e-eb1f-4b7f-a54f-d2a05dd3d8c8@arm.com
> > Signed-off-by: NeilBrown <neilb@...e.de>
> > ---
> >  fs/namei.c |  5 +++++
> >  fs/open.c  | 19 ++++++++++++-------
> >  2 files changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 37fb0a8aa09a..057afacc4b60 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3612,6 +3612,9 @@ static int do_open(struct nameidata *nd,
> >         int acc_mode;
> >         int error;
> >
> > +       if (file->f_mode & FMODE_OPENED)
> > +               fsnotify_open(file);
> > +
> >         if (!(file->f_mode & (FMODE_OPENED | FMODE_CREATED))) {
> >                 error = complete_walk(nd);
> >                 if (error)
> > @@ -3700,6 +3703,8 @@ int vfs_tmpfile(struct mnt_idmap *idmap,
> >         mode = vfs_prepare_mode(idmap, dir, mode, mode, mode);
> >         error = dir->i_op->tmpfile(idmap, dir, file, mode);
> >         dput(child);
> > +       if (file->f_mode & FMODE_OPENED)
> > +               fsnotify_open(file);
> >         if (error)
> >                 return error;
> >         /* Don't check for other permissions, the inode was just created */
> > diff --git a/fs/open.c b/fs/open.c
> > index 89cafb572061..970f299c0e77 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -1004,11 +1004,6 @@ static int do_dentry_open(struct file *f,
> >                 }
> >         }
> >
> > -       /*
> > -        * Once we return a file with FMODE_OPENED, __fput() will call
> > -        * fsnotify_close(), so we need fsnotify_open() here for symmetry.
> > -        */
> > -       fsnotify_open(f);
> >         return 0;
> >
> >  cleanup_all:
> > @@ -1085,8 +1080,17 @@ EXPORT_SYMBOL(file_path);
> >   */
> >  int vfs_open(const struct path *path, struct file *file)
> >  {
> > +       int ret;
> > +
> >         file->f_path = *path;
> > -       return do_dentry_open(file, NULL);
> > +       ret = do_dentry_open(file, NULL);
> > +       if (!ret)
> > +               /*
> > +                * Once we return a file with FMODE_OPENED, __fput() will call
> > +                * fsnotify_close(), so we need fsnotify_open() here for symmetry.
> > +                */
> > +               fsnotify_open(file);
> 
> I agree that this change preserves the logic, but (my own) comment
> above is inconsistent with the case of:
> 
>         if ((f->f_flags & O_DIRECT) && !(f->f_mode & FMODE_CAN_ODIRECT))
>                 return -EINVAL;
> 
> Which does set FMODE_OPENED, but does not emit an OPEN event.

If I understand correctly, that case doesn't emit an OPEN event before
my patch, but will result in a CLOSE event.
After my patch ... I think it still doesn't emit OPEN.

I wonder if, instead of adding the the fsnotify_open() in do_open(), we
should put it in the\
	if (file->f_mode & (FMODE_OPENED | FMODE_CREATED)) {
case of open_last_lookups().

Or maybe it really doesn't hurt to have a CLOSE event without and OPEN. 
OPEN without CLOSE would be problematic, but the other way around
shouldn't matter....  It feels untidy, but maybe we don't care.

Thanks,
NeilBrown


> 
> I have a feeling that the comment is correct about the CLOSE event in
> that case, but honestly, I don't think this corner case is that important,
> just maybe the comment needs to be slightly clarified?
> 
> Thanks,
> Amir.
> 
> > +       return ret;
> >  }
> >
> >  struct file *dentry_open(const struct path *path, int flags,
> > @@ -1178,7 +1182,8 @@ struct file *kernel_file_open(const struct path *path, int flags,
> >         if (error) {
> >                 fput(f);
> >                 f = ERR_PTR(error);
> > -       }
> > +       } else
> > +               fsnotify_open(f);
> >         return f;
> >  }
> >  EXPORT_SYMBOL_GPL(kernel_file_open);
> > --
> > 2.44.0
> >
> 

Powered by blists - more mailing lists