Re: fsnotify path hooks

[Christian Brauner <christian.brauner@ubuntu.com>]

On Tue, Apr 20, 2021 at 09:01:09AM +0300, Amir Goldstein wrote:
> > One thing, whatever you end up passing to vfs_create() please make sure
> > to retrieve mnt_userns once so permission checking and object creation
> > line-up:
> >
> > int vfs_create(struct vfsmount *mnt, struct inode *dir,
> >                struct dentry *dentry, umode_t mode, bool want_excl)
> > {
> >         struct user_namespace *mnt_userns;
> >
> >         mnt_userns = mnt_user_ns(mnt);
> >
> >         int error = may_create(mnt_userns, dir, dentry);
> >         if (error)
> >                 return error;
> >
> >         if (!dir->i_op->create)
> >                 return -EACCES; /* shouldn't it be ENOSYS? */
> >         mode &= S_IALLUGO;
> >         mode |= S_IFREG;
> >         error = security_inode_create(dir, dentry, mode);
> >         if (error)
> >                 return error;
> >         error = dir->i_op->create(mnt_userns, dir, dentry, mode, want_excl);
> >         if (!error)
> >                 fsnotify_create(mnt, dir, dentry);
> >         return error;
> > }
> >
> 
> Christian,
> 
> What is the concern here?
> Can mnt_user_ns() change under us?
> I am asking because Al doesn't like both mnt_userns AND path to
> be passed to do_tuncate() => notify_change()
> So I will need to retrieve mnt_userns again inside notify_change()
> after it had been used for security checks in do_open().
> Would that be acceptable to you?

The mnt_userns can't change once a mnt has been idmapped and it can
never change if the mount is visible in the filesystem already. The only
case we've been worried about and why we did it this way is when you
have a caller do fd = open_tree(OPEN_TREE_CLONE) and then share that
unattached fd with multiple processes
T1: mkdirat(fd, "dir1", 0755);
T2: mount_setattr(fd, "",); /* changes idmapping */
That case isn't a problem if the mnt_userns is only retrieved once for
permission checking and operating on the inode. I think with your
changes that still shouldn't be an issue though since the vfs_*()
helpers encompass the permission checking anyway and for notify_change,
we could simply add a mnt_userns field to struct iattr and pass it down.