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

From: "J. Bruce Fields" <[email-masked]>
Date: Mon, 15 Aug 2011 17:25:39 -0400

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

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

---

From: "J. Bruce Fields" <[email-masked]>
Date: Mon, 15 Aug 2011 18:23:36 -0400

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

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

Applying anyway for compatibility with existing clients, but this all
seems a little weird.

--b.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Mon, 15 Aug 2011 18:27:21 -0400

> Applying anyway for compatibility with existing clients, but this all

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.

---

From: "Myklebust, Trond" <[email-masked]>
Date: Mon, 15 Aug 2011 22:04:29 -0700

> Sent: Monday, August 15, 2011 6:27 PM
> To: Myklebust, Trond
> Cc: [email-masked]; [email-masked]; [email-masked]
> Subject: Re: open() of device special files
> 
> > > > > I'm a bit annoyed at myself as I have a feeling we've
discussed
> this

Pynfs is not an authoritative source for anything.

Trond

---

From: "Myklebust, Trond" <[email-masked]>
Date: Mon, 15 Aug 2011 22:03:49 -0700

> Sent: Monday, August 15, 2011 6:24 PM
> To: Myklebust, Trond

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.

> 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
> > > reason is the dot-x file defines NFS4ERR_SYMLINK as meaning
"should
> be

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

Trond

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 16 Aug 2011 06:49:32 -0400

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

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 16 Aug 2011 07:03:22 -0400

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

---

From: Steve Dickson <[email-masked]>
Date: Tue, 16 Aug 2011 07:32:22 -0400

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

---

From: "J. Bruce Fields" <[email-masked]>
Date: Tue, 16 Aug 2011 20:52:20 -0400

> 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-masked]>
Date:   Mon Aug 15 16:55:02 2011 -0400

    

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;