Re: [PATCH v4 0/3] FUSE: Implement atomic lookup + open/create

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, May 11, 2022 at 01:21:13PM -0400, Vivek Goyal wrote:
> On Wed, May 11, 2022 at 11:40:59AM +0200, Miklos Szeredi wrote:
> > On Thu, 5 May 2022 at 21:59, Vivek Goyal <vgoyal@redhat.com> wrote:
> > 
> > > Oh, I have no issues with the intent. I will like to see cut in network
> > > traffic too (if we can do this without introducing problems). My primary
> > > interest is that this kind of change should benefit virtiofs as well.
> > 
> > One issue with that appears to be checking permissions.   AFAIU this
> > patchset only enables the optimization if default_permissions is
> > turned off (i.e. all permission checking is done by the server).  But
> > virtiofs uses the default_permissions model.
> 
> IIUC, only 3rd patch mentions that default_permission should be turned
> off. IOW, first patch where lookup + create + open is a single operation
> and second patch which does "lookup + open" in a single operation does
> not seem to require that default_permissions are not in effect.
> 
> So if first two patches work fine, I think virtiofs should benefit too.
> (IMHO, 3rd patch is too hacky anyway)
> 
> W.r.t permission checks, looks like may_open() will finally be called
> after ->atomic_open(). So even if we open the file, we should still be
> able to check whether we have permissions to open the file or not
> after the fact.
> 
> fs/namei.c
> 
> path_openat()
> {
> 	open_last_lookups()  <--- This calls ->atomic_open()
> 	do_open()  <--- This calls may_open()
> }

Actually I am not sure about it. I was playing with 

open(foo.txt, O_CREAT | O_RDWR, O_IRUSR)

This succeeds if file is newly created but if file already existed, this
fails with -EACCESS

So man 2 open says following. Thanks to Andy Price for pointing me to it.

    Note that mode applies only to future accesses of the newly cre‐
    ated  file;  the  open()  call that creates a read-only file may
    well return a read/write file descriptor.


Now I am wondering how will it look like with first patch. Assume file
already exists on the server (But there is no negative dentry present)
and I do following. And assume file only has read permission for user
and I am trying to open it read-write.

open(foo.txt, O_CREAT | O_RDWR, O_IRUSR)

In normal circumstances, user will expect -EACCESS as file is read-only
and user is trying to open it read-write.

I am wondering how will it look like with this first patch.

Current fuse ->atomic_open() looks up the dentry and does not open
the file if dentry is positive.

New implementation will skip lookup and open the file anyway and
set file->f_mode |= FMODE_CREATED; (First patch in series)

So first of all this seems wrong. I thought FMODE_CREATED should be
set only if file was newly created. Is that a correct understanding.

And I am looking at do_open() code. It does bunch of things based
on FMODE_CREATED flag. One of the things it does is reset acc_mode =0

        if (file->f_mode & FMODE_CREATED) {
                /* Don't check for write permission, don't truncate */
                open_flag &= ~O_TRUNC;
                acc_mode = 0;
	}
	error = may_open(mnt_userns, &nd->path, acc_mode, open_flag);

I suspect this is the code which allows opening a newly created read-only
file as O_RDWR. (Though I am not 100% sure).

I suspect with first patch this will be broken. We will set FMODE_CREATED
even if file already existed and VFS will assume a new file has been
created and do bunch of things which is wrong.

So looks like fuse ->atomic_open() should set FMODE_CREATED only if
it really created the file.

Thanks
Vivek