Re: [PATCH 5/7] fanotify: record new parent and name in MOVED_FROM event

[Jan Kara <jack@suse.cz>]

On Sat 13-11-21 11:40:39, Amir Goldstein wrote:
> On Fri, Nov 12, 2021 at 6:48 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 29-10-21 14:40:26, Amir Goldstein wrote:
> > > In the special case of MOVED_FROM event, if we are recording the child
> > > fid due to FAN_REPORT_TARGET_FID init flag, we also record the new
> > > parent and name.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ...
> > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > index 795bedcb6f9b..d1adcb3437c5 100644
> > > --- a/fs/notify/fanotify/fanotify.c
> > > +++ b/fs/notify/fanotify/fanotify.c
> > > @@ -592,21 +592,30 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
> > >                                                       __kernel_fsid_t *fsid,
> > >                                                       const struct qstr *name,
> > >                                                       struct inode *child,
> > > +                                                     struct dentry *moved,
> > >                                                       unsigned int *hash,
> > >                                                       gfp_t gfp)
> > >  {
> > >       struct fanotify_name_event *fne;
> > >       struct fanotify_info *info;
> > >       struct fanotify_fh *dfh, *ffh;
> > > +     struct inode *dir2 = moved ? d_inode(moved->d_parent) : NULL;
> >
> > I think we need to be more careful here (like dget_parent()?). Fsnotify is
> > called after everything is unlocked after rename so dentry can be changing
> > under us, cannot it? Also does that mean that we could actually get a wrong
> > parent here if the dentry is renamed immediately after we unlock things and
> > before we report fsnotify event?
> 
> fsnotify_move() is called inside lock_rename() (both old_dir and new_dir locks),
> which are held by the caller of vfs_rename(), and prevent d_parent/d_name
> changes to child dentries, so moved->d_parent is stable here.
> You are probably confusing with inode_unlock(target), which is the
> child inode lock.
> 
> d_parent/d_name are also stable for fsnotify_create/link/unlink/mkdir/rmdir
> per the vfs locking rules for those operations. In all those cases, the parent
> dir lock is held for vfs helper callers.
> This is why we can use dentry->d_name (and moved->d_name) in all those
> fsnotify hooks.

Bah, you're right. I got confused by the locking of source and target
inside vfs_rename() but those are not parent directories but rather "files"
being renamed from / two. Sorry for the noise.

								Honza

> 
> Thanks,
> Amir.
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR