| Message ID | 20240617162303.1596-2-jack@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fsnotify: Generate FS_CREATE event before FS_OPEN | expand |
On Mon, Jun 17, 2024 at 7:23 PM Jan Kara <jack@suse.cz> wrote: > > From: NeilBrown <neilb@suse.de> > > 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 open_last_lookups() 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() > > 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@arm.com> > Closes: https://lore.kernel.org/all/01c3bf2e-eb1f-4b7f-a54f-d2a05dd3d8c8@arm.com > Signed-off-by: NeilBrown <neilb@suse.de> > Link: https://lore.kernel.org/r/171817619547.14261.975798725161704336@noble.neil.brown.name > Fixes: 7b8c9d7bb457 ("fsnotify: move fsnotify_open() hook into do_dentry_open()") > Tested-by: James Clark <james.clark@arm.com> > Signed-off-by: Christian Brauner <brauner@kernel.org> > Signed-off-by: Jan Kara <jack@suse.cz> Looks good. Reviewed-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/namei.c | 10 ++++++++-- > fs/open.c | 22 +++++++++++++++------- > 2 files changed, 23 insertions(+), 9 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index 37fb0a8aa09a..1e05a0f3f04d 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -3572,8 +3572,12 @@ static const char *open_last_lookups(struct nameidata *nd, > else > inode_lock_shared(dir->d_inode); > dentry = lookup_open(nd, file, op, got_write); > - if (!IS_ERR(dentry) && (file->f_mode & FMODE_CREATED)) > - fsnotify_create(dir->d_inode, dentry); > + if (!IS_ERR(dentry)) { > + if (file->f_mode & FMODE_CREATED) > + fsnotify_create(dir->d_inode, dentry); > + if (file->f_mode & FMODE_OPENED) > + fsnotify_open(file); > + } > if (open_flag & O_CREAT) > inode_unlock(dir->d_inode); > else > @@ -3700,6 +3704,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..f1607729acb9 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,19 @@ 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); > + } > + return ret; > } > > struct file *dentry_open(const struct path *path, int flags, > @@ -1177,8 +1183,10 @@ struct file *kernel_file_open(const struct path *path, int flags, > error = do_dentry_open(f, NULL); > if (error) { > fput(f); > - f = ERR_PTR(error); > + return ERR_PTR(error); > } > + > + fsnotify_open(f); > return f; > } > EXPORT_SYMBOL_GPL(kernel_file_open); > -- > 2.35.3 >
diff --git a/fs/namei.c b/fs/namei.c index 37fb0a8aa09a..1e05a0f3f04d 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3572,8 +3572,12 @@ static const char *open_last_lookups(struct nameidata *nd, else inode_lock_shared(dir->d_inode); dentry = lookup_open(nd, file, op, got_write); - if (!IS_ERR(dentry) && (file->f_mode & FMODE_CREATED)) - fsnotify_create(dir->d_inode, dentry); + if (!IS_ERR(dentry)) { + if (file->f_mode & FMODE_CREATED) + fsnotify_create(dir->d_inode, dentry); + if (file->f_mode & FMODE_OPENED) + fsnotify_open(file); + } if (open_flag & O_CREAT) inode_unlock(dir->d_inode); else @@ -3700,6 +3704,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..f1607729acb9 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,19 @@ 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); + } + return ret; } struct file *dentry_open(const struct path *path, int flags, @@ -1177,8 +1183,10 @@ struct file *kernel_file_open(const struct path *path, int flags, error = do_dentry_open(f, NULL); if (error) { fput(f); - f = ERR_PTR(error); + return ERR_PTR(error); } + + fsnotify_open(f); return f; } EXPORT_SYMBOL_GPL(kernel_file_open);