open() of device special files
Subsystems (2)
How is the client supposed to handle opens of device special files? On a 3.1-rc1-based client to a linux server over v4.0 I'm seeing it try an OPEN call and failing when it gets an INVAL return.
This looks like bogus client behavior (OPEN should fail on such files), unless the server has the error return wrong and the client's using an OPEN error to recover.
If I first stat the device and then open it then it works as expected (the client does an open of the local device).
I'm a bit annoyed at myself as I have a feeling we've discussed this before but I can't find a reference now.
--b.
Quoted 8 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
Sent: Monday, August 15, 2011 11:37 AM
To: Myklebust, Trond; [email protected]
Cc: [email protected]
Subject:open()of device special filesHow is the client supposed to handle opens of device special files?
On
Quoted 1 line
a 3.1-rc1-based client to a linux server over v4.0 I'm seeing it try
an
Quoted 3 lines
OPEN call and failing when it gets an INVAL return.
This looks like bogus client behavior (OPEN should fail on such
files),
Quoted 8 lines
unless the server has the error return wrong and the client's using an OPEN error to recover.
If I first stat the device and then open it then it works as expected (the client does an open of the local device).
I'm a bit annoyed at myself as I have a feeling we've discussed this before but I can't find a reference now.
Hmm... NFS4ERR_INVAL means 'invalid argument', which is not the case here; all the arguments that the client is passing to the server are valid, however the file cannot be opened because the pathname resolves on the server to a file of the wrong type.
I can't see any other error definition that is "obviously correct", but it looks to me as if NFS4ERR_SYMLINK might be the closest thing. One reason is the dot-x file defines NFS4ERR_SYMLINK as meaning "should be file/directory". The other reason is that NFS4ERR_SYMLINK should _always_ trigger the correct behaviour on a client: a fresh lookup of the component.
Cheers
Trond
Quoted 24 lines · Trond
Quoted 3 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
How is the client supposed to handle opens of device special files?On
Quoted 1 line
a 3.1-rc1-based client to a linux server over v4.0 I'm seeing it try
an
Quoted 3 lines
OPEN call and failing when it gets an INVAL return.
This looks like bogus client behavior (OPEN should fail on such
files),
Quoted 8 lines
unless the server has the error return wrong and the client's using an OPEN error to recover.
If I first stat the device and then open it then it works as expected (the client does an open of the local device).
I'm a bit annoyed at myself as I have a feeling we've discussed this before but I can't find a reference now.
Hmm...
NFS4ERR_INVALmeans 'invalid argument', which is not the case here; all the arguments that the client is passing to the server are valid, however the file cannot be opened because the pathname resolves on the server to a file of the wrong type.
Quoted 6 lines
I can't see any other error definition that is "obviously correct", but it looks to me as if
NFS4ERR_SYMLINKmight be the closest thing. One reason is the dot-x file definesNFS4ERR_SYMLINKas meaning "should be file/directory". The other reason is thatNFS4ERR_SYMLINKshould _always_ trigger the correct behaviour on a client: a fresh lookup of the component.
Confirmed the fix; I'll apply.
But that's tricky--we should be document it for other server implementers if it's the right thing to do (working group list cc'd).
--b.
commit 3d83c016da8652f30dcac372772b67d68f33bfbd
Author: J. Bruce Fields <[email protected]> Date: Mon Aug 15 16:55:02 2011 -0400
nfsd4: return nfserr_symlink on v4 OPEN of non-regular file
Without this, an attempt to open a device special file without first stat'ing it will fail.
fs/nfsd/nfsfh.c +11 -0
@@ -69,6 +69,17 @@ nfsd_mode_check(struct svc_rqst *rqstp, umode_t mode, int type)return nfserr_notdir; else if ((mode & S_IFMT) == S_IFDIR)return nfserr_isdir; +/* +* err_symlink is our catch-all error in the v4 case; this +* looks odd, but: +* - the comment next to ERR_SYMLINK in file is +* "should be file/directory" +* - we happen to know this will cause the linux v4 +* client to do the right thing on attempts to open +* something other than a regular file: +*/ + else if (rqstp->rq_vers == 4) + return nfserr_symlink; else return nfserr_inval; }
Quoted 17 lines · J. Bruce Fields
Quoted 16 lines · Trond
Quoted 3 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
How is the client supposed to handle opens of device special files?On
Quoted 1 line
a 3.1-rc1-based client to a linux server over v4.0 I'm seeing it try
an
Quoted 3 lines
OPEN call and failing when it gets an INVAL return.
This looks like bogus client behavior (OPEN should fail on such
files),
Quoted 6 lines
unless the server has the error return wrong and the client's using an OPEN error to recover.
If I first stat the device and then open it then it works as expected (the client does an open of the local device).
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
I'm a bit annoyed at myself as I have a feeling we've discussed this before but I can't find a reference now.
Hmm...
NFS4ERR_INVALmeans 'invalid argument', which is not the case here; all the arguments that the client is passing to the server are valid, however the file cannot be opened because the pathname resolves on the server to a file of the wrong type.
The long description of NFS4ERR_INVAL from rfc 3530:
"Invalid argument or unsupported argument for an operation. Two examples are attempting a READLINK on an object other than a symbolic link or specifying a value for an enum field that is not defined in the protocol (e.g., nfs_ftype4)."
So the text does recommend NFS4ERR_INVAL for the case where the operation doesn't make sense for the type of object given. (In the readlink example there's no pathname resolution, though).
EINVAL seems to be used in the kernel for some such cases as well (e.g. attempt to truncate a device special file).
Quoted 4 lines
Quoted 4 lines
I can't see any other error definition that is "obviously correct", but it looks to me as if
NFS4ERR_SYMLINKmight be the closest thing. One reason is the dot-x file definesNFS4ERR_SYMLINKas meaning "should be file/directory".
And this seems more a coincidence due to vagueness resulting from the need for a single-line comment; the longer description is:
"NFS4ERR_SYMLINK The current filehandle provided for a LOOKUP is
not a directory but a symbolic link. Also used if the final component of the OPEN path is a symbolic link."
Quoted 5 lines
Quoted 3 lines
The other reason is that
NFS4ERR_SYMLINKshould
_always_ trigger the correct behaviour on a client: a fresh lookup of the component.Confirmed the fix; I'll apply.
Applying anyway for compatibility with existing clients, but this all seems a little weird.
--b.
Quoted 53 lines · J. Bruce Fields
Quoted 17 lines · J. Bruce Fields
Quoted 16 lines · Trond
Quoted 3 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
How is the client supposed to handle opens of device special files?On
Quoted 1 line
a 3.1-rc1-based client to a linux server over v4.0 I'm seeing it try
an
Quoted 3 lines
OPEN call and failing when it gets an INVAL return.
This looks like bogus client behavior (OPEN should fail on such
files),
Quoted 6 lines
unless the server has the error return wrong and the client's using an OPEN error to recover.
If I first stat the device and then open it then it works as expected (the client does an open of the local device).
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
I'm a bit annoyed at myself as I have a feeling we've discussed this before but I can't find a reference now.
Hmm...
NFS4ERR_INVALmeans 'invalid argument', which is not the case here; all the arguments that the client is passing to the server are valid, however the file cannot be opened because the pathname resolves on the server to a file of the wrong type.The long description of
NFS4ERR_INVALfrom rfc 3530:"Invalid argument or unsupported argument for an operation. Two examples are attempting a READLINK on an object other than a symbolic link or specifying a value for an enum field that is not defined in the protocol (e.g.,
nfs_ftype4)."So the text does recommend
NFS4ERR_INVALfor the case where the operation doesn't make sense for the type of object given. (In the readlink example there's no pathname resolution, though).EINVAL seems to be used in the kernel for some such cases as well (e.g. attempt to truncate a device special file).
Quoted 4 lines
Quoted 4 lines
I can't see any other error definition that is "obviously correct", but it looks to me as if
NFS4ERR_SYMLINKmight be the closest thing. One reason is the dot-x file definesNFS4ERR_SYMLINKas meaning "should be file/directory".And this seems more a coincidence due to vagueness resulting from the need for a single-line comment; the longer description is:
"
NFS4ERR_SYMLINKThe current filehandle provided for a LOOKUP is
not a directory but a symbolic link. Also used if the final component of the OPEN path is a symbolic link."
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
The other reason is that
NFS4ERR_SYMLINKshould
_always_ trigger the correct behaviour on a client: a fresh lookup of the component.Confirmed the fix; I'll apply.
Applying anyway for compatibility with existing clients, but this all seems a little weird.
And the change also makes a bunch of pynfs tests fail, complaining that various operations against incompatible types should have returned INVAL and not SYMLINK.
Not that I'm convinced pynfs is correct--at the very least it should have accepted a range of errors for those tests, I think--but anyone else that ran pynfs against their server may have assumed it pynfs was correct in these cases....
--b.
Quoted 9 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
Sent: Monday, August 15, 2011 6:27 PM
To: Myklebust, Trond
Cc: [email protected]; [email protected]; [email protected]
Subject: Re:open()of device special filesQuoted 1 line · J. Bruce Fields
On Mon, Aug 15, 2011 at 05:25:39PM -0400, J. Bruce Fields wrote:
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines · Trond
Quoted 3 lines
Quoted 3 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]] How is the client supposed to handle opens of device specialfiles?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
On
Quoted 1 line
a 3.1-rc1-based client to a linux server over v4.0 I'm seeing
it try
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
an
Quoted 3 lines
OPEN call and failing when it gets an INVAL return.
This looks like bogus client behavior (OPEN should fail on
such
Quoted 10 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
files),
Quoted 1 line
unless the server has the error return wrong and the client's
using an
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
OPEN error to recover.
If I first stat the device and then open it then it works as
expected
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(the client does an open of the local device).
I'm a bit annoyed at myself as I have a feeling we've
discussed
Quoted 24 lines
this
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
before but I can't find a reference now.
Hmm...
NFS4ERR_INVALmeans 'invalid argument', which is not thecase
Quoted 1 line
Quoted 1 line
Quoted 1 line
here; all the arguments that the client is passing to the server
are
Quoted 1 line
Quoted 1 line
Quoted 1 line
valid, however the file cannot be opened because the pathname
resolves
Quoted 9 lines
Quoted 1 line
Quoted 1 line
on the server to a file of the wrong type.
The long description of
NFS4ERR_INVALfrom rfc 3530:"Invalid argument or unsupported argument for an operation. Two examples are attempting a READLINK on an object other than a symbolic link or specifying a value for an enum field that is not defined in the protocol (e.g.,
nfs_ftype4)."Quoted 5 lines
So the text does recommend
NFS4ERR_INVALfor the case where the operation doesn't make sense for the type of object given. (In the readlink example there's no pathname resolution, though).EINVAL seems to be used in the kernel for some such cases as well
(e.g.
Quoted 11 lines
Quoted 3 lines
attempt to truncate a device special file).
Quoted 1 line
Quoted 1 line
I can't see any other error definition that is "obviously
correct", but
Quoted 1 line
Quoted 1 line
Quoted 1 line
it looks to me as if
NFS4ERR_SYMLINKmight be the closest thing.One
Quoted 1 line
Quoted 1 line
Quoted 1 line
reason is the dot-x file defines
NFS4ERR_SYMLINKas meaning"should be
Quoted 3 lines
Quoted 1 line
Quoted 1 line
file/directory".
And this seems more a coincidence due to vagueness resulting from
the
Quoted 15 lines
Quoted 9 lines
need for a single-line comment; the longer description is:
"
NFS4ERR_SYMLINKThe current filehandle provided for a LOOKUP is
not a directory but a symbolic link. Also used if the final component of the OPEN path is a symbolic link."Quoted 2 lines
Quoted 2 lines
The other reason is that
NFS4ERR_SYMLINKshould _always_ trigger the correct behaviour on a client: a freshlookup of
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
the component.
Confirmed the fix; I'll apply.
Applying anyway for compatibility with existing clients, but this
all
Quoted 3 lines
Quoted 1 line
seems a little weird.
And the change also makes a bunch of pynfs tests fail, complaining
that
Quoted 8 lines
various operations against incompatible types should have returned INVAL
and not SYMLINK.Not that I'm convinced pynfs is correct--at the very least it should have accepted a range of errors for those tests, I think--but anyone else that ran pynfs against their server may have assumed it pynfs was correct in these cases....
Pynfs is not an authoritative source for anything.
Trond
Quoted 8 lines · Trond
Quoted 3 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
And the change also makes a bunch of pynfs tests fail, complainingthat
Quoted 4 lines
various operations against incompatible types should have returned INVAL
and not SYMLINK.
Quoted 6 lines
Quoted 4 lines
Not that I'm convinced pynfs is correct--at the very least it should have accepted a range of errors for those tests, I think--but anyone else that ran pynfs against their server may have assumed it pynfs was correct in these cases....
Pynfs is not an authoritative source for anything.
We should say that, for what it's worth. (Below.)
But it's just another reason why servers may have been written to return something other than err_symlink.... I have in the past had to tell developers of more than one server that fixing error returns to make pynfs happy may not be a good idea.
--b.
README +6 -0
@@ -12,3 +12,9 @@ in place. For more details about 4.0 and 4.1 testing, see nfs4.0/README and nfs4.1/README, respectively. For information about automatic code generation from an XDR file, see xdr/README. + +Note that test results should *not* be considered authoritative +statements about the protocol--if you find that a server fails a test, +you should consult the rfc's and think carefully before assuming that +the server is at fault. (However, we do appreciate patches if you +find a test that requires incorrect behavior.)
Quoted 43 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
Sent: Monday, August 15, 2011 6:24 PM
To: Myklebust, Trond
Cc: [email protected]; [email protected]; [email protected]
Subject: Re:open()of device special filesQuoted 4 lines · J. Bruce Fields
Quoted 3 lines · Trond
Quoted 3 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]] How is the client supposed to handle opens of device specialfiles?
Quoted 2 lines
Quoted 2 lines
On
Quoted 1 line
a 3.1-rc1-based client to a linux server over v4.0 I'm seeing it
try
Quoted 6 lines
Quoted 6 lines
an
Quoted 3 lines
OPEN call and failing when it gets an INVAL return.
This looks like bogus client behavior (OPEN should fail on such
files),
Quoted 1 line
unless the server has the error return wrong and the client's
using an
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
OPEN error to recover.
If I first stat the device and then open it then it works as
expected
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(the client does an open of the local device).
I'm a bit annoyed at myself as I have a feeling we've discussed
this
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
before but I can't find a reference now.
Hmm...
NFS4ERR_INVALmeans 'invalid argument', which is not thecase
Quoted 1 line
Quoted 1 line
here; all the arguments that the client is passing to the server
are
Quoted 1 line
Quoted 1 line
valid, however the file cannot be opened because the pathname
resolves
Quoted 1 line
Quoted 1 line
on the server to a file of the wrong type.
The long description of
NFS4ERR_INVALfrom rfc 3530:
Quoted 8 lines
"Invalid argument or unsupported argument for an operation. Two examples are attempting a READLINK on an object other than a symbolic link or specifying a value for an enum field that is not defined in the protocol (e.g.,
nfs_ftype4)."So the text does recommend
NFS4ERR_INVALfor the case where the operation doesn't make sense for the type of object given. (In the readlink example there's no pathname resolution, though).
Right. In the case of readlink, you pass a file handle, which points to an object for which you _can_ check the type before attempting the readlink operation.
OPEN (and LOOKUP, RENAME, REMOVE,...) takes a filename, which could point to anything. There is no way for the client to know a priori the type of the object being pointed to.
Quoted 1 line
EINVAL seems to be used in the kernel for some such cases as well
(e.g.
Quoted 1 line
attempt to truncate a device special file).
My reading of the manpages and the posix spec is that truncate() will return EINVAL if and only if the length argument is wrong. Ftruncate() will in addition return EINVAL if the file descriptor points to a non-regular file (the assumption presumably being that you should have tried to fstat() first).
IOW: The criterion that they apply is that "if the user should have known better" then EINVAL is correct. If there is no way for the user to be sure a priori, then it is not. So, for instance, rmdir() will not return EINVAL if you try to apply it to a file.
Quoted 1 line
Quoted 1 line
Quoted 1 line
I can't see any other error definition that is "obviously
correct",
Quoted 4 lines
but
Quoted 1 line
Quoted 1 line
it looks to me as if
NFS4ERR_SYMLINKmight be the closest thing.One
Quoted 1 line
Quoted 1 line
reason is the dot-x file defines
NFS4ERR_SYMLINKas meaning
"should
Quoted 13 lines
be
Quoted 1 line
Quoted 1 line
file/directory".
And this seems more a coincidence due to vagueness resulting from the need for a single-line comment; the longer description is:
"
NFS4ERR_SYMLINKThe current filehandle provided for a LOOKUP is
not a directory but a symbolic link. Also used if the final component of the OPEN path is a symbolic link."Quoted 2 lines
Quoted 2 lines
The other reason is that
NFS4ERR_SYMLINKshould _always_ trigger the correct behaviour on a client: a fresh lookup
Quoted 7 lines
of
Quoted 3 lines
Quoted 1 line
the component.
Confirmed the fix; I'll apply.
Applying anyway for compatibility with existing clients, but this all seems a little weird.
Agreed, but it appears to be a case that has been missed when we defined OPEN.
Trond
Quoted 31 lines · Trond
Quoted 4 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]] The long description ofNFS4ERR_INVALfrom rfc 3530:Quoted 8 lines
"Invalid argument or unsupported argument for an operation. Two examples are attempting a READLINK on an object other than a symbolic link or specifying a value for an enum field that is not defined in the protocol (e.g.,
nfs_ftype4)."So the text does recommend
NFS4ERR_INVALfor the case where the operation doesn't make sense for the type of object given. (In the readlink example there's no pathname resolution, though).Right. In the case of readlink, you pass a file handle, which points to an object for which you _can_ check the type before attempting the readlink operation.
OPEN (and LOOKUP, RENAME, REMOVE,...) takes a filename, which could point to anything. There is no way for the client to know a priori the type of the object being pointed to.
Quoted 1 line
EINVAL seems to be used in the kernel for some such cases as well
(e.g.
Quoted 1 line
attempt to truncate a device special file).
My reading of the manpages and the posix spec is that
truncate()will return EINVAL if and only if the length argument is wrong.Ftruncate()will in addition return EINVAL if the file descriptor points to a non-regular file (the assumption presumably being that you should have tried tofstat()first).
Quoted 4 lines
IOW: The criterion that they apply is that "if the user should have known better" then EINVAL is correct. If there is no way for the user to be sure a priori, then it is not. So, for instance,
rmdir()will not return EINVAL if you try to apply it to a file.
That's perfectly logical, but, taking the example of (f)truncate:
- truncate(2) as far as I can tell doesn't specify behavior for truncate in this case, only for ftruncate.
- http://pubs.opengroup.org/onlinepubs/009695399/ says behavior is undefined for ftruncate, and says nothing for truncate.
- linux actually returns EINVAL in both cases:
$ sudo mknod tmp c 1 3 -m 666
$ make trunc-vs-ftrunc
cc trunc-vs-ftrunc.c -o trunc-vs-ftrunc
$ ./trunc-vs-ftrunc tmp
trunc-vs-ftrunc: truncate: Invalid argument
trunc-vs-ftrunc: ftruncate: Invalid argument
But I don't really know why I'm arguing here--I don't disagree. You have a reasonable way to define EINVAL. This just appears to be easy to get wrong....
--b.
trunc-vs-ftrunc.c:
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <err.h>
int main(int argc, char *argv[])
{
int fd, ret;
if (argc != 2)
errx(1, "usage: %s path\n", argv[0]);
ret = truncate(argv[1], 0);
if (ret)
warn("truncate");
fd = open(argv[1], O_WRONLY);
if (fd < 0)
err(1, "open");
ret = ftruncate(fd, 0);
if (ret)
warn("ftruncate");
}Quoted 1 line · J. Bruce Fields
Confirmed the fix; I'll apply.
Also found some opportunity for cleanups while I was there, as follows.
--b.
PATCH 1/5 nfsd4: clean up S_IS -> NF4 file type mapping
A slightly unconventional approach to make the code more compact I could live with, but let's give the poor reader *some* chance.
--- fs/nfsd/nfs4xdr.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)
fs/nfsd/nfs4xdr.c +14 -7
@@ -1760,12 +1760,19 @@ static __be32 nfsd4_encode_fs_locations(struct svc_rqst *rqstp,return 0 ; } -static u32 nfs4_ftypes[16 ] = { -NF4BAD ,NF4FIFO ,NF4CHR ,NF4BAD , -NF4DIR ,NF4BAD ,NF4BLK ,NF4BAD , -NF4REG ,NF4BAD ,NF4LNK ,NF4BAD , -NF4SOCK ,NF4BAD ,NF4LNK ,NF4BAD , -}; +static u32 nfs4_file_type (umode_t mode) +{ +switch (mode &S_IFMT ) { +case S_IFIFO :return NF4FIFO ; +case S_IFCHR :return NF4CHR ; +case S_IFDIR :return NF4DIR ; +case S_IFBLK :return NF4BLK ; +case S_IFLNK :return NF4LNK ; +case S_IFREG :return NF4REG ; +case S_IFSOCK :return NF4SOCK ; +default :return NF4BAD ; + }; +} static __be32 nfsd4_encode_name(struct svc_rqst *rqstp, int whotype, uid_t id, int group,
@@ -1954,7 +1961,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,if (bmval0 &FATTR4_WORD0_TYPE ) {if ((buflen -=4 ) <0 ) goto out_resource; - dummy = nfs4_ftypes[(stat.mode &S_IFMT ) >>12 ]; + dummy =nfs4_file_type (stat.mode);if (dummy ==NF4BAD ) goto out_serverfault;WRITE32 (dummy);
Signature · 1 line
1.7.4.1
PATCH 2/5 nfsd4: return nfserr_symlink on v4 OPEN of non-regular file
In reply to J. Bruce FieldsWithout this, an attempt to open a device special file without first stat'ing it will fail.
--- fs/nfsd/nfsfh.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
fs/nfsd/nfsfh.c +11 -0
@@ -69,6 +69,17 @@ nfsd_mode_check(struct svc_rqst *rqstp, umode_t mode, int type)return nfserr_notdir; else if ((mode & S_IFMT) == S_IFDIR)return nfserr_isdir; +/* +* err_symlink is our catch-all error in the v4 case; this +* looks odd, but: +* - the comment next to ERR_SYMLINK in file is +* "should be file/directory" +* - we happen to know this will cause the linux v4 +* client to do the right thing on attempts to open +* something other than a regular file: +*/ + else if (rqstp->rq_vers == 4) + return nfserr_symlink; else return nfserr_inval; }
Signature · 1 line
1.7.4.1
PATCH 3/5 nfsd4: fix incorrect comment in nfsd4_set_nfs4_acl
In reply to J. Bruce FieldsZero means "I don't care what kind of file this is". And that's probably what we want--acls are also settable at least on directories, and if the filesystem doesn't want them on other objects, leave it to it to complain.
--- fs/nfsd/vfs.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
fs/nfsd/vfs.c +1 -1
@@ -502,7 +502,7 @@ nfsd4_set_nfs4_acl(struct svc_rqst *rqstp, struct svc_fh *fhp,unsigned int flags =0 ;/* Get inode */ - error =fh_verify (rqstp, fhp,0 /* S_IFREG */ ,NFSD_MAY_SATTR ); + error =fh_verify (rqstp, fhp,0 ,NFSD_MAY_SATTR );if (error)return error;
Signature · 1 line
1.7.4.1
PATCH 4/5 nfsd: open-code special directory-hardlink check
In reply to J. Bruce FieldsWe allow the fh_verify caller to specify that any object *except* those of a given type is allowed, by passing a negative type. But only one caller actually uses it. Open-code that check in the one caller.
--- fs/nfsd/nfsfh.c | 9 --------- fs/nfsd/vfs.c | 6 ++++-- 2 files changed, 4 insertions(+), 11 deletions(-)
fs/nfsd/nfsfh.c +0 -9
@@ -61,7 +61,6 @@ static int nfsd_acceptable(void *expv, struct dentry *dentry)static inline __be32 nfsd_mode_check (struct svc_rqst *rqstp,umode_t mode,int type) { -/* Type can be negative when creating hardlinks - not to a dir */ if (type >0 && (mode &S_IFMT ) != type) {if (rqstp->rq_vers ==4 && (mode &S_IFMT ) ==S_IFLNK )return nfserr_symlink;
@@ -83,14 +82,6 @@ nfsd_mode_check(struct svc_rqst *rqstp, umode_t mode, int type)else return nfserr_inval; } -if (type <0 && (mode &S_IFMT ) == -type) { -if (rqstp->rq_vers ==4 && (mode &S_IFMT ) ==S_IFLNK ) -return nfserr_symlink; -else if (type == -S_IFDIR ) -return nfserr_isdir; -else -return nfserr_notdir; - }return 0 ; }
fs/nfsd/vfs.c +4 -2
@@ -1632,10 +1632,12 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, err =fh_verify (rqstp, ffhp,S_IFDIR ,NFSD_MAY_CREATE );if (err) goto out; - err =fh_verify (rqstp, tfhp, -S_IFDIR ,NFSD_MAY_NOP ); + err =fh_verify (rqstp, tfhp,0 ,NFSD_MAY_NOP );if (err) goto out; - + err = nfserr_isdir; +if (S_ISDIR (tfhp->fh_dentry->d_inode->i_mode)) + goto out; err = nfserr_perm;if (!len) goto out;
Signature · 1 line
1.7.4.1
PATCH 5/5 nfsd: clean up nfsd_mode_check()
In reply to J. Bruce FieldsAdd some more comments, simplify logic, do & S_IFMT just once, name "type" more helpfully.
--- fs/nfsd/nfsfh.c | 52 +++++++++++++++++++++++++++++----------------------- 1 files changed, 29 insertions(+), 23 deletions(-)
fs/nfsd/nfsfh.c +29 -23
@@ -59,30 +59,36 @@ static int nfsd_acceptable(void *expv, struct dentry *dentry)* the write call). */ static inline __be32 -nfsd_mode_check (struct svc_rqst *rqstp,umode_t mode,int type) +nfsd_mode_check (struct svc_rqst *rqstp,umode_t mode,int requested) { -if (type >0 && (mode &S_IFMT ) != type) { -if (rqstp->rq_vers ==4 && (mode &S_IFMT ) ==S_IFLNK ) -return nfserr_symlink; -else if (type ==S_IFDIR ) -return nfserr_notdir; -else if ((mode &S_IFMT ) ==S_IFDIR ) -return nfserr_isdir; -/* -* err_symlink is our catch-all error in the v4 case; this -* looks odd, but: -* - the comment next to ERR_SYMLINK in file is -* "should be file/directory" -* - we happen to know this will cause the linux v4 -* client to do the right thing on attempts to open -* something other than a regular file: -*/ -else if (rqstp->rq_vers ==4 ) -return nfserr_symlink; -else -return nfserr_inval; - } -return 0 ; + mode &=S_IFMT ; + +if (requested ==0 )/* the caller doesn't care */ +return 0 ; +if (mode == requested) +return 0 ; +/* +* v4 has an error more specific than err_notdir which we should +* return in preference to err_notdir: +*/ +if (rqstp->rq_vers ==4 && mode ==S_IFLNK ) +return nfserr_symlink; +if (requested ==S_IFDIR ) +return nfserr_notdir; +if (mode ==S_IFDIR ) +return nfserr_isdir; +/* +* err_symlink is our catch-all error in the v4 case; this +* looks odd, but: +* - the comment next to ERR_SYMLINK in file is +* "should be file/directory" +* - we happen to know this will cause the linux v4 +* client to do the right thing on attempts to open +* something other than a regular file: +*/ +if (rqstp->rq_vers ==4 ) +return nfserr_symlink; +return nfserr_inval; } static __be32 nfsd_setuser_and_check_port(struct svc_rqst *rqstp,
Signature · 1 line
1.7.4.1
Re: [PATCH 5/5] nfsd: clean up nfsd_mode_check()
Also noticed while auditing these error returns. There's all kinds of weird crap in here.
--b.
commit fe0a6f4711dba408fb771a402876db33ffe219bf
Author: J. Bruce Fields <[email protected]> Date: Mon Aug 15 18:39:32 2011 -0400
nfsd4: it's OK to return nfserr_symlink
The nfsd4 code has a bunch of special exceptions for error returns which map nfserr_symlink to other errors.
In fact, the spec makes it clear that nfserr_symlink is to be preferred over less specific errors where possible.
The patch that introduced it back in 2.6.4 is "kNFSd: correct symlink related error returns.", which claims that these special exceptions are represent an NFSv4 break from v2/v3 tradition--when in fact the symlink error was introduced with v4.
I suspect what happened was pynfs tests were written that were overly faithful to the (known-incomplete) rfc3530 error return lists, and then code was fixed up mindlessly to make the tests pass.
Delete these unnecessary exceptions.
fs/nfsd/nfs4proc.c +1 -12
@@ -473,11 +473,8 @@ nfsd4_commit(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, *p++ = nfssvc_boot.tv_sec; *p++ = nfssvc_boot.tv_usec; - status =nfsd_commit (rqstp, &cstate->current_fh, commit->co_offset, +return nfsd_commit (rqstp, &cstate->current_fh, commit->co_offset, commit->co_count); -if (status == nfserr_symlink) - status = nfserr_inval; -return status; } static __be32
@@ -492,8 +489,6 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, status =fh_verify (rqstp, &cstate->current_fh,S_IFDIR ,NFSD_MAY_CREATE ); -if (status == nfserr_symlink) - status = nfserr_notdir;if (status)return status;
@@ -719,8 +714,6 @@ nfsd4_remove(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,return nfserr_grace; status =nfsd_unlink (rqstp, &cstate->current_fh,0 , remove->rm_name, remove->rm_namelen); -if (status == nfserr_symlink) -return nfserr_notdir;if (!status) {fh_unlock (&cstate->current_fh);set_change_info (&remove->rm_cinfo, &cstate->current_fh);
@@ -751,8 +744,6 @@ nfsd4_rename(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, (S_ISDIR(cstate->save_fh.fh_dentry->d_inode->i_mode) && S_ISDIR(cstate->current_fh.fh_dentry->d_inode->i_mode))) status = nfserr_exist; - else if (status == nfserr_symlink) - status = nfserr_notdir;if (!status) {set_change_info (&rename->rn_sinfo, &cstate->current_fh);
@@ -892,8 +883,6 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, write->wr_bytes_written = cnt; -if (status == nfserr_symlink) - status = nfserr_inval;return status; }
fs/nfsd/nfs4state.c +1 -5
@@ -4179,12 +4179,8 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,if (!nfsd4_has_session (cstate) &&STALE_CLIENTID (&lockt->lt_clientid)) goto out; -if ((status =fh_verify (rqstp, &cstate->current_fh,S_IFREG ,0 ))) { -dprintk ("NFSD: nfsd4_lockt: fh_verify() failed!\n" ); -if (status == nfserr_symlink) - status = nfserr_inval; +if ((status =fh_verify (rqstp, &cstate->current_fh,S_IFREG ,0 ))) goto out; - } inode = cstate->current_fh.fh_dentry->d_inode;locks_init_lock (&file_lock);
fs/nfsd/nfs4xdr.c +0 -4
@@ -2766,8 +2766,6 @@ nfsd4_encode_read(struct nfsd4_compoundres *resp, __be32 nfserr, read->rd_offset, resp->rqstp->rq_vec, read->rd_vlen, &maxcount); -if (nfserr == nfserr_symlink) - nfserr = nfserr_inval;if (nfserr)return nfserr; eof = (read->rd_offset + maxcount >=
@@ -2893,8 +2891,6 @@ nfsd4_encode_readdir(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4 readdir->common.err == nfserr_toosmall && readdir->buffer == page) nfserr = nfserr_toosmall; -if (nfserr == nfserr_symlink) - nfserr = nfserr_notdir;if (nfserr) goto err_no_verf;
Quoted 25 lines · J. Bruce Fields
Quoted 24 lines · Trond
Quoted 3 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
How is the client supposed to handle opens of device special files?On
Quoted 1 line
a 3.1-rc1-based client to a linux server over v4.0 I'm seeing it try
an
Quoted 3 lines
OPEN call and failing when it gets an INVAL return.
This looks like bogus client behavior (OPEN should fail on such
files),
Quoted 8 lines
unless the server has the error return wrong and the client's using an OPEN error to recover.
If I first stat the device and then open it then it works as expected (the client does an open of the local device).
I'm a bit annoyed at myself as I have a feeling we've discussed this before but I can't find a reference now.
Hmm...
NFS4ERR_INVALmeans 'invalid argument', which is not the case here; all the arguments that the client is passing to the server are valid, however the file cannot be opened because the pathname resolves on the server to a file of the wrong type.
Quoted 8 lines
Quoted 6 lines
I can't see any other error definition that is "obviously correct", but it looks to me as if
NFS4ERR_SYMLINKmight be the closest thing. One reason is the dot-x file definesNFS4ERR_SYMLINKas meaning "should be file/directory". The other reason is thatNFS4ERR_SYMLINKshould _always_ trigger the correct behaviour on a client: a fresh lookup of the component.Confirmed the fix; I'll apply.
Confirmed! The following open now works with all NFS versions:
int main (void)
{
int fd;
char buf[20];size_t nbytes;ssize_t bytes_read;
nbytes = sizeof(buf);
fd = open("/mnt/dev/urandom", O_RDONLY|O_NOCTTY|O_NONBLOCK);
if (fd < 0) {
perror("open");
exit(1);
}bytes_read = read(fd, buf, nbytes);
return 0;
}
Thanks!
steved.
Quoted 39 lines
But that's tricky--we should be document it for other server implementers if it's the right thing to do (working group list cc'd).
--b.
commit
3d83c016da8652f30dcac372772b67d68f33bfbdAuthor: J. Bruce Fields <[email protected]> Date: Mon Aug 15 16:55:02 2011 -0400nfsd4: return
nfserr_symlinkon v4 OPEN of non-regular fileWithout this, an attempt to open a device special file without first stat'ing it will fail.
Signed-off-by J. Bruce Fields <[email protected]>fs/nfsd/nfsfh.c +11
-0@@ -69,6 +69,17 @@ nfsd_mode_check(struct svc_rqst *rqstp, umode_t mode, int type)return nfserr_notdir; else if ((mode & S_IFMT) == S_IFDIR)return nfserr_isdir; +/* +* err_symlink is our catch-all error in the v4 case; this +* looks odd, but: +* - the comment next to ERR_SYMLINK in file is +* "should be file/directory" +* - we happen to know this will cause the linux v4 +* client to do the right thing on attempts to open +* something other than a regular file: +*/ + else if (rqstp->rq_vers == 4) + return nfserr_symlink; else return nfserr_inval; }
Quoted 1 line
Confirmed the fix; I'll apply.
Actually that changed NFS4ERR_INVAL's to NFS4ERR_SYMLINK's all over the place, which I'd rather not do; the following just affects the OPEN.
--b.
commit 618459ca1f7613f0dde4f09138e9acbe02690264
Author: J. Bruce Fields <[email protected]> Date: Mon Aug 15 16:55:02 2011 -0400
nfsd4: return nfserr_symlink on v4 OPEN of non-regular file
Without this, an attempt to open a device special file without first stat'ing it will fail.
fs/nfsd/nfs4proc.c +21 -0
@@ -168,6 +168,24 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsreturn status; } +static __be32 nfsd_check_obj_isreg (struct svc_fh *fh) +{ +umode_t mode = fh->fh_dentry->d_inode->i_mode; + +if (S_ISREG (mode)) +return nfs_ok; +if (S_ISDIR (mode)) +return nfserr_isdir; +/* +* Using err_symlink as our catch-all case may look odd; but +* there's no other obvious error for this case in 4.0, and we +* happen to know that it will cause the linux v4 client to do +* the right thing on attempts to open something other than a +* regular file. +*/ +return nfserr_symlink; +} +static __be32 do_open_lookup (struct svc_rqst *rqstp,struct svc_fh *current_fh,struct nfsd4_open *open) {
@@ -216,6 +234,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o status =nfsd_lookup (rqstp, current_fh, open->op_fname.data, open->op_fname.len, &resfh);fh_unlock (current_fh); +if (status) + goto out; + status =nfsd_check_obj_isreg (&resfh); }if (status) goto out;
Quoted 11 lines · Trond
Hmm...
NFS4ERR_INVALmeans 'invalid argument', which is not the case here; all the arguments that the client is passing to the server are valid, however the file cannot be opened because the pathname resolves on the server to a file of the wrong type.I can't see any other error definition that is "obviously correct", but it looks to me as if
NFS4ERR_SYMLINKmight be the closest thing. One reason is the dot-x file definesNFS4ERR_SYMLINKas meaning "should be file/directory". The other reason is thatNFS4ERR_SYMLINKshould _always_ trigger the correct behaviour on a client: a fresh lookup of the component.
By the way, note rfc 5661 is unambiguous: the server should return WRONG_TYPE. I haven't looked into fixing that yet on either client or server side yet.
--b.