Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Miklos Szeredi <miklos@szeredi.hu>]

On Wed, 21 Sept 2022 at 10:54, Christian Brauner <brauner@kernel.org> wrote:
>
> On Wed, Sep 21, 2022 at 05:06:57AM +0200, Miklos Szeredi wrote:
> > On Tue, 20 Sept 2022 at 22:57, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > On Tue, Sep 20, 2022 at 09:36:30PM +0200, Miklos Szeredi wrote:
> > >
> > > >       inode = child->d_inode;
> > >
> > > Better
> > >         inode = file_inode(file);
> > >
> > > so that child would be completely ignored after dput().
> > >
> > > > +     error = vfs_tmpfile(mnt_userns, &path, file, op->mode);
> > > > +     if (error)
> > > >               goto out2;
> > > > -     dput(path.dentry);
> > > > -     path.dentry = child;
> > > > -     audit_inode(nd->name, child, 0);
> > > > +     audit_inode(nd->name, file->f_path.dentry, 0);
> > > >       /* Don't check for other permissions, the inode was just created */
> > > > -     error = may_open(mnt_userns, &path, 0, op->open_flag);
> > >
> > > Umm...  I'm not sure that losing it is the right thing - it might
> > > be argued that ->permission(..., MAY_OPEN) is to be ignored for
> > > tmpfile (and the only thing checking for MAY_OPEN is nfs, which is
> > > *not* going to grow tmpfile any time soon - certainly not with these
> > > calling conventions), but you are also dropping the call of
> > > security_inode_permission(inode, MAY_OPEN) and that's a change
> > > compared to what LSM crowd used to get...
> >
> > Not losing it, just moving it into vfs_tmpfile().
>
> Afaict, we haven't called may_open() for tmpfile creation in either
> cachefiles or overlayfs before. So from that perspective I wonder if
> there's a good reason for us to do it now.

For overlayfs we did check MAY_WRITE | MAY_OPEN through
ovl_path_open().  Just checking MAY_OPEN relaxes this, but it's in
line with the overlay model of checking the same permissions as if the
operation was invoked directly.

For cachefiles no permission was checked before this patch, so in
theory it could change behavior.  Moving the permission check back out
to callers would fix this, but I'm not entirely sure that that is the
best way forward.

David, what is the model for cachefiles?  Is this okay to check for
permissions on underlying ops, or that must be avoided?

Thanks,
Miklos