On Mon, Jun 17, 2024 at 4:07 PM Jan Kara <jack@xxxxxxx> wrote: > > On Mon 17-06-24 15:09:09, Amir Goldstein wrote: > > On Mon, Jun 17, 2024 at 12:37 PM Jan Kara <jack@xxxxxxx> wrote: > > > On Sat 15-06-24 07:35:42, Christian Brauner wrote: > > > > On Wed, 12 Jun 2024 17:09:55 +1000, NeilBrown 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(). > > > > > > > > > > [...] > > > > > > > > Applied to the vfs.fixes branch of the vfs/vfs.git tree. > > > > Patches in the vfs.fixes branch should appear in linux-next soon. > > > > > > > > Please report any outstanding bugs that were missed during review in a > > > > new review to the original patch series allowing us to drop it. > > > > > > > > It's encouraged to provide Acked-bys and Reviewed-bys even though the > > > > patch has now been applied. If possible patch trailers will be updated. > > > > > > > > Note that commit hashes shown below are subject to change due to rebase, > > > > trailer updates or similar. If in doubt, please check the listed branch. > > > > > > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git > > > > branch: vfs.fixes > > > > > > > > [1/1] VFS: generate FS_CREATE before FS_OPEN when ->atomic_open used. > > > > https://git.kernel.org/vfs/vfs/c/7536b2f06724 > > > > > > I have reviewed the patch you've committed since I wasn't quite sure which > > > changes you're going to apply after your discussion with Amir. And I have > > > two comments: > > > > > > @@ -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); > > > > Please add { } around multi line indented text. > > > > > + return ret; > > > } > > > > > > AFAICT this will have a side-effect that now fsnotify_open() will be > > > generated even for O_PATH open. It is true that fsnotify_close() is getting > > > generated for them already and we should strive for symmetry. Conceptually > > > it doesn't make sense to me to generate fsnotify events for O_PATH > > > opens/closes but maybe I miss something. Amir, any opinion here? > > > > Good catch! > > > > I agree that we do not need OPEN nor CLOSE events for O_PATH. > > I suggest to solve it with: > > > > @@ -915,7 +929,7 @@ 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; > > + f->f_mode = FMODE_PATH | FMODE_OPENED | __FMODE_NONOTIFY; > > f->f_op = &empty_fops; > > return 0; > > } > > First I was somewhat nervous about this as it results in returning O_PATH > fd with __FMODE_NONOTIFY to userspace and I was afraid it may influence > generation of events *somewhere*. It influences my POC code of future lookup permission event: https://github.com/amir73il/linux/commits/fan_lookup_perm/ which is supposed to generate events on lookup with O_PATH fd. > But checking a bit, we use 'file' for > generating only open, access, modify, and close events so yes, this should > be safe. Alternatively we could add explicit checks for !O_PATH when > generating open / close events. > So yeh, this would be better: --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -112,7 +112,7 @@ static inline int fsnotify_file(struct file *file, __u32 mask) { const struct path *path; - if (file->f_mode & FMODE_NONOTIFY) + if (file->f_mode & (FMODE_NONOTIFY | FMODE_PATH)) return 0; path = &file->f_path; -- It is a dilemma, if this patch should be separate. On the one hand it fixes unbalanced CLOSE events on O_PATH fd, so it is an independent fix. OTOH, it is a requirement for moving fsnotify_open() out of do_dentry_open(), so not so healthy to separate them, when it is less clear that they need to be backported together. > > > @@ -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) > > > > > > Frankly, this works but looks as an odd place to put this notification to. > > > Why not just placing it just next to where fsnotify_create() is generated > > > in open_last_lookups()? Like: > > > > > > if (open_flag & O_CREAT) > > > inode_lock(dir->d_inode); > > > 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 > > > inode_unlock_shared(dir->d_inode); > > > > > > That looks like a place where it is much more obvious this is for > > > atomic_open() handling? Now I admit I'm not really closely familiar with > > > the atomic_open() paths so maybe I miss something and do_open() is better. > > > > It looks nice, but I think it is missing the fast lookup case without O_CREAT > > (i.e. goto finish_lookup). > > I don't think so. AFAICT that case will generate the event in vfs_open() > anyway and not in open_last_lookups() / do_open(). Am I missing something? No. I am. This code is hard to follow. I am fine with your variant, but maybe after so many in-tree changes including the extra change of O_PATH, perhaps it would be better to move this patch to your fsnotify tree? Thanks, Amir.
