RE: [PATCH] NFSv4: Fix OPEN w/create access mode checking

["Frank Filz" <ffilzlnx@mindspring.com>]

Ok, that should work, though what about the case where cinfo is not atomic?
Even if the server is able to do an atomic open/create, the cinfo might
still not be atomic, thus might give a false indication of file create (it
doesn't look like the client checks for cinfo.atomic.

The result would be that in a busy directory, you COULD call
open("some-execute-only-file", O_CREAT | O_RDONLY, 0) to get read access to
an execute only file...

Ganesha for one can't guarantee atomicity (we can't create a file AND get an
updated cinfo from the kernel in a single operation...). So if a check for
cinfo.atomic is put in place, the exception would be skipped. (It looks like
knfsd may also always set cinfo.atomic to false). Currently, it looks like
the only use of file_created is to set FILE_CREATED, and it looks like the
only real use of that is to call fsnotify_create, which if that happens to
be called for a file that actually already existed, would not be horrendous.

To cut to the chase, this patch should make all of my tests pass (which my
patch in truth did not do), and also for the cases you suggested is correct,
suggests this patch is more correct than mine, but might still not be 100%
correct...

Frank

> POSIX states that open("foo", O_CREAT|O_RDONLY, 000) should succeed if
> the file "foo" does not already exist. With the current NFS client, it
will fail
> with an EACCES error because of the permissions checks in
> nfs4_opendata_access().
> 
> Fix is to turn that test off if the server says that we created the file.
> 
> Reported-by: "Frank S. Filz" <ffilzlnx@mindspring.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfs/nfs4proc.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index
> d0e0e54fb2b9..70e53a2ac75e 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1954,6 +1954,14 @@ static int _nfs4_recover_proc_open(struct
> nfs4_opendata *data)
>  	return status;
>  }
> 
> +/*
> + * Additional permission checks in order to distinguish between an
> + * open for read, and an open for execute. This works around the
> + * fact that NFSv4 OPEN treats read and execute permissions as being
> + * the same.
> + * Note that in the non-execute case, we want to turn off permission
> + * checking if we just created a new file (POSIX open() semantics).
> + */
>  static int nfs4_opendata_access(struct rpc_cred *cred,
>  				struct nfs4_opendata *opendata,
>  				struct nfs4_state *state, fmode_t fmode,
> @@ -1968,14 +1976,14 @@ static int nfs4_opendata_access(struct rpc_cred
> *cred,
>  		return 0;
> 
>  	mask = 0;
> -	/* don't check MAY_WRITE - a newly created file may not have
> -	 * write mode bits, but POSIX allows the creating process to write.
> -	 * use openflags to check for exec, because fmode won't
> -	 * always have FMODE_EXEC set when file open for exec. */
> +	/*
> +	 * Use openflags to check for exec, because fmode won't
> +	 * always have FMODE_EXEC set when file open for exec.
> +	 */
>  	if (openflags & __FMODE_EXEC) {
>  		/* ONLY check for exec rights */
>  		mask = MAY_EXEC;
> -	} else if (fmode & FMODE_READ)
> +	} else if ((fmode & FMODE_READ) && !opendata->file_created)
>  		mask = MAY_READ;
> 
>  	cache.cred = cred;
> --
> 1.9.3