> 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.
Good point. And I guess the only pitfall is that someone could manage to read an execute only file that happens to be in a directory that is busy enough to cause an apparent cinfo change. Such an enterprising person could figure out other ways to read an execute only file (extracting the file data from a network trace, or a custom client).
My tests pass, so consider that an ack for your patch, thanks.
Frank
> > 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
