On Thu, Jul 10, 2014 at 11:21 AM, Frank Filz <ffilzlnx@xxxxxxxxxxxxxx> wrote:
> 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...
Agreed, but it is a best effort. There is nothing other than the cinfo
available to the client to tell it if the file was just created or
not.
> 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@xxxxxxxxxxxxxx>
>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>> ---
>> 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
>
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
