open() of device special files

open() of device special files

[J. Bruce Fields]

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.

RE: open() of device special files

[Myklebust, Trond]

> -----Original Message-----
> From: J. Bruce Fields [mailto:bfields@fieldses.org]
> Sent: Monday, August 15, 2011 11:37 AM
> To: Myklebust, Trond; linux-nfs@vger.kernel.org
> Cc: steved@redhat.com
> Subject: open() of device special files
> 
> 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.

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

Re: open() of device special files

[J. Bruce Fields]

On Mon, Aug 15, 2011 at 09:03:30AM -0700, Myklebust, Trond wrote:
> > -----Original Message-----
> > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > 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.
> 
> 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.

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 <bfields@redhat.com>
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.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 90c6aa6..cc8eb8d 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -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;
 	}

Re: open() of device special files

[J. Bruce Fields]

On Mon, Aug 15, 2011 at 05:25:39PM -0400, J. Bruce Fields wrote:
> On Mon, Aug 15, 2011 at 09:03:30AM -0700, Myklebust, Trond wrote:
> > > -----Original Message-----
> > > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > > 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.
> > 
> > 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.

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).

> > 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".

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."

> > The other reason is that NFS4ERR_SYMLINK should
> > _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.

Re: open() of device special files

[J. Bruce Fields]

On Mon, Aug 15, 2011 at 06:23:36PM -0400, J. Bruce Fields wrote:
> On Mon, Aug 15, 2011 at 05:25:39PM -0400, J. Bruce Fields wrote:
> > On Mon, Aug 15, 2011 at 09:03:30AM -0700, Myklebust, Trond wrote:
> > > > -----Original Message-----
> > > > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > > > 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.
> > > 
> > > 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.
> 
> 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).
> 
> > > 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".
> 
> 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."
> 
> > > The other reason is that NFS4ERR_SYMLINK should
> > > _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.

RE: open() of device special files

[Myklebust, Trond]

> -----Original Message-----
> From: J. Bruce Fields [mailto:bfields@fieldses.org]
> Sent: Monday, August 15, 2011 6:27 PM
> To: Myklebust, Trond
> Cc: linux-nfs@vger.kernel.org; steved@redhat.com; nfsv4@ietf.org
> Subject: Re: open() of device special files
> 
> On Mon, Aug 15, 2011 at 06:23:36PM -0400, J. Bruce Fields wrote:
> > On Mon, Aug 15, 2011 at 05:25:39PM -0400, J. Bruce Fields wrote:
> > > On Mon, Aug 15, 2011 at 09:03:30AM -0700, Myklebust, Trond wrote:
> > > > > -----Original Message-----
> > > > > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > > > > 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.
> > > >
> > > > 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.
> >
> > 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).
> >
> > > > 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".
> >
> > 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."
> >
> > > > The other reason is that NFS4ERR_SYMLINK should
> > > > _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....

Pynfs is not an authoritative source for anything.

Trond

Re: open() of device special files

[J. Bruce Fields]

On Mon, Aug 15, 2011 at 10:04:29PM -0700, Myklebust, Trond wrote:
> > -----Original Message-----
> > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > 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....
> 
> 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.

diff --git a/README b/README
index 0c34b2a..ecb2aa6 100644
--- a/README
+++ b/README
@@ -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.)

RE: open() of device special files

[Myklebust, Trond]

> -----Original Message-----
> From: J. Bruce Fields [mailto:bfields@fieldses.org]
> Sent: Monday, August 15, 2011 6:24 PM
> To: Myklebust, Trond
> Cc: linux-nfs@vger.kernel.org; steved@redhat.com; nfsv4@ietf.org
> Subject: Re: open() of device special files
> 
> On Mon, Aug 15, 2011 at 05:25:39PM -0400, J. Bruce Fields wrote:
> > On Mon, Aug 15, 2011 at 09:03:30AM -0700, Myklebust, Trond wrote:
> > > > -----Original Message-----
> > > > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > > > 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.
> > >
> > > 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.
> 
> 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).

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.

> EINVAL seems to be used in the kernel for some such cases as well
(e.g.
> 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.

> > > 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".
> 
> 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."
> 
> > > The other reason is that NFS4ERR_SYMLINK should
> > > _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.

Agreed, but it appears to be a case that has been missed when we defined
OPEN.

Trond

Re: open() of device special files

[J. Bruce Fields]

On Mon, Aug 15, 2011 at 10:03:49PM -0700, Myklebust, Trond wrote:
> > -----Original Message-----
> > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > 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).
> 
> 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.
> 
> > EINVAL seems to be used in the kernel for some such cases as well
> (e.g.
> > 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.

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");
}

Re: open() of device special files

[J. Bruce Fields]

On Mon, Aug 15, 2011 at 05:25:39PM -0400, J. Bruce Fields wrote:
> 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

[J. Bruce Fields]

A slightly unconventional approach to make the code more compact I could
live with, but let's give the poor reader *some* chance.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4xdr.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index c8bf405..42ef8ff 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -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);
-- 
1.7.4.1

[PATCH 2/5] nfsd4: return nfserr_symlink on v4 OPEN of non-regular file

[J. Bruce Fields]

Without this, an attempt to open a device special file without first
stat'ing it will fail.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfsfh.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 90c6aa6..cc8eb8d 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -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;
 	}
-- 
1.7.4.1

[PATCH 3/5] nfsd4: fix incorrect comment in nfsd4_set_nfs4_acl

[J. Bruce Fields]

Zero 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.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/vfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index fd0acca..556ca2b 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -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;
 
-- 
1.7.4.1

[PATCH 4/5] nfsd: open-code special directory-hardlink check

[J. Bruce Fields]

We 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.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfsfh.c |    9 ---------
 fs/nfsd/vfs.c   |    6 ++++--
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index cc8eb8d..dc0f9ff 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -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;
 }
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 556ca2b..4c22870 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -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;
-- 
1.7.4.1

[PATCH 5/5] nfsd: clean up nfsd_mode_check()

[J. Bruce Fields]

Add some more comments, simplify logic, do & S_IFMT just once, name
"type" more helpfully.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfsfh.c |   52 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index dc0f9ff..b4fd50e 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -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,
-- 
1.7.4.1

Re: [PATCH 5/5] nfsd: clean up nfsd_mode_check()

[J. Bruce Fields]

Also noticed while auditing these error returns.  There's all kinds of
weird crap in here.

--b.

commit fe0a6f4711dba408fb771a402876db33ffe219bf
Author: J. Bruce Fields <bfields@redhat.com>
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.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 9bf0a66..3b159ab 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -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;
 }
 
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3787ec1..aa0a36e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -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);
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 42ef8ff..2834ee2 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -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;
 

Re: [nfsv4] open() of device special files

[Steve Dickson]

On 08/15/2011 05:25 PM, J. Bruce Fields wrote:
> On Mon, Aug 15, 2011 at 09:03:30AM -0700, Myklebust, Trond wrote:
>>> -----Original Message-----
>>> From: J. Bruce Fields [mailto:bfields@fieldses.org]
>>> 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.
>>
>> 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.
> 
> 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.

> 
> 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 <bfields@redhat.com>
> 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.
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index 90c6aa6..cc8eb8d 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -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;
>  	}

Re: open() of device special files

[J. Bruce Fields]

> 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 <bfields@redhat.com>
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.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 9bf0a66..d784ceb 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -168,6 +168,24 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
 	return 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;

Re: open() of device special files

[J. Bruce Fields]

On Mon, Aug 15, 2011 at 09:03:30AM -0700, Myklebust, Trond wrote:
> 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.

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.