linux-kernel - Re: [REGRESSION] NFSv4: open(O

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1334786606.12880.14.camel@lade.trondhjem.org>
Date:	Wed, 18 Apr 2012 22:03:26 +0000
From:	"Myklebust, Trond" <Trond.Myklebust@...app.com>
To:	Miklos Szeredi <miklos@...redi.hu>
CC:	"bfields@...ldses.org" <bfields@...ldses.org>,
	"linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [REGRESSION] NFSv4: open(O_TRUNC) hangs

Hi Miklos

On Fri, 2012-04-06 at 11:43 +0200, Miklos Szeredi wrote:
> "Myklebust, Trond" <Trond.Myklebust@...app.com> writes:
> 
> > The following client patch fixes the regression for me.
> 
> It fixes the hang, but it still doesn't work 100% correctly.  Try the
> following test program.
> 
> BTW, do you run any fs test suits?  All these were caught with one I use
> to quick test fuse (it's in the fuse git tree(*) under the "test"
> directory).  But I guess others like LTP would catch these as well.

Thanks! I'll look into that. Bryan has been helping me to set up a test
rig for the NFS client, but for now we don't have much coverage of basic
POSIX tests.

> Thanks,
> Miklos
> 
> (*)  git://fuse.git.sourceforge.net/gitroot/fuse/fuse
> ---
> 
> #include <stdio.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <errno.h>
> 
> int main(int argc, char *argv[])
> {
> 	int res;
> 	char *name = argv[1];
> 
> 	unlink(name);
> 	close(creat(name, 0400));
> 	res = open(name, O_RDONLY | O_TRUNC);
> 	if (res != -1 && errno != EPERM)
> 		fprintf(stderr, "should have failed!\n");

Shouldn't that be EACCES? As far as I know, POSIX doesn't list EPERM as
an allowed return value for open().

> 
> 	return 0;
> }

I'm trying to find out what the correct behaviour should be. Clearly we
should not be hanging, and so we definitely do need a fix. However
according to POSIX, the behaviour of open(O_RDONLY|O_TRUNC) is
undefined. I'm therefore thinking that returning EACCES should be
acceptable in the case where the server returns NFS4ERR_OPENMODE to our
open stateid (which is the case for the Linux server).

Something like the following will 'break' your previous test, in that it
returns EACCES to the open(O_RDONLY|O_TRUNC) instead of truncating the
file. However as I said, that appears to be POSIX-compliant.

Cheers
  Trond
8<-----------------------------------------------------------------------
From f3efde63559c33a906dc325b192ecdb6c590c0f6 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@...app.com>
Date: Wed, 18 Apr 2012 16:29:11 -0400
Subject: [PATCH] NFSv4: Fix open(O_TRUNC) and ftruncate() error handling

If the file wasn't opened for writing, then truncate and ftruncate
need to report the appropriate errors.

Reported-by: Miklos Szeredi <miklos@...redi.hu>
Signed-off-by: Trond Myklebust <Trond.Myklebust@...app.com>
Cc: stable@...r.kernel.org
---
 fs/nfs/dir.c      |    4 ++--
 fs/nfs/nfs4proc.c |   15 ++++++++++++---
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 4aaf031..8789210 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1429,7 +1429,7 @@ static struct dentry *nfs_atomic_lookup(struct inode *dir, struct dentry *dentry
 	}
 
 	open_flags = nd->intent.open.flags;
-	attr.ia_valid = 0;
+	attr.ia_valid = ATTR_OPEN;
 
 	ctx = create_nfs_open_context(dentry, open_flags);
 	res = ERR_CAST(ctx);
@@ -1536,7 +1536,7 @@ static int nfs_open_revalidate(struct dentry *dentry, struct nameidata *nd)
 	if (IS_ERR(ctx))
 		goto out;
 
-	attr.ia_valid = 0;
+	attr.ia_valid = ATTR_OPEN;
 	if (openflags & O_TRUNC) {
 		attr.ia_valid |= ATTR_SIZE;
 		attr.ia_size = 0;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ba837d9..f875cf3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1954,10 +1954,19 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 	};
 	int err;
 	do {
-		err = nfs4_handle_exception(server,
-				_nfs4_do_setattr(inode, cred, fattr, sattr, state),
-				&exception);
+		err = _nfs4_do_setattr(inode, cred, fattr, sattr, state);
+		switch (err) {
+		case -NFS4ERR_OPENMODE:
+			if (state && !(state->state & FMODE_WRITE)) {
+				err = -EBADF;
+				if (sattr->ia_valid & ATTR_OPEN)
+					err = -EACCES;
+				goto out;
+			}
+		}
+		err = nfs4_handle_exception(server, err, &exception);
 	} while (exception.retry);
+out:
 	return err;
 }
 
-- 
1.7.7.6


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@...app.com
www.netapp.com