Re: [PATCH v2] VFS: generate FS_CREATE before FS_OPEN when ->atomic_open used.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.





[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux