[REGRESSION] NFSv4: open(O_TRUNC) hangs

[REGRESSION] NFSv4: open(O_TRUNC) hangs

[Miklos Szeredi]

Now this test program hangs on latest git.

Thanks,
Miklos
---

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>

int main(int argc, char *argv[])
{
	int res;
	char *name = argv[1];
	int fd;

	unlink(name);
	fd = creat(name, 0644);
	write(fd, "123", 3);
	close(fd);
	close(open(name, O_RDONLY | O_TRUNC));

	return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [REGRESSION] NFSv4: open(O_TRUNC) hangs

[Myklebust, Trond]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2115 bytes --]

On Tue, 2012-04-03 at 18:26 +0200, Miklos Szeredi wrote:
> Now this test program hangs on latest git.
> 
> Thanks,
> Miklos
> ---
> 
> #include <stdio.h>
> #include <unistd.h>
> #include <fcntl.h>
> 
> int main(int argc, char *argv[])
> {
> 	int res;
> 	char *name = argv[1];
> 	int fd;
> 
> 	unlink(name);
> 	fd = creat(name, 0644);
> 	write(fd, "123", 3);
> 	close(fd);
> 	close(open(name, O_RDONLY | O_TRUNC));
> 
> 	return 0;
> }

Hi Miklos,

The following client patch fixes the regression for me.

Cheers
  Trond
8<----------------------------------------------------
From 5ec0ed6cd88c65464092533787706d9ecfb85320 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Wed, 4 Apr 2012 10:51:33 -0700
Subject: [PATCH 1/2] NFSv4: Don't use open stateids that have the wrong open
 mode

If the client is doing an operation that needs a particular open
mode, then nfs4_select_rw_stateid() should only copy the
open stateid if the latter has the correct open mode.

Otherwise we should just use the zero stateid.

Reported-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/nfs4state.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 0f43414..71dcfd4 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -932,6 +932,13 @@ void nfs4_select_rw_stateid(nfs4_stateid *dst, struct nfs4_state *state,
 {
 	if (nfs4_copy_delegation_stateid(dst, state->inode, fmode))
 		return;
+
+	fmode &= FMODE_READ|FMODE_WRITE;
+	if ((state->state & fmode) != fmode) {
+		nfs4_stateid_copy(dst, &zero_stateid);
+		return;
+	}
+
 	if (nfs4_copy_lock_stateid(dst, state, fl_owner, fl_pid))
 		return;
 	nfs4_copy_open_stateid(dst, state);
-- 
1.7.7.6


-- 
Trond Myklebust
Linux NFS client maintainer

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

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±û"žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: [REGRESSION] NFSv4: open(O_TRUNC) hangs

[Miklos Szeredi]

"Myklebust, Trond" <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> 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,
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");

	return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [REGRESSION] NFSv4: open(O_TRUNC) hangs

[Myklebust, Trond]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4278 bytes --]

Hi Miklos

On Fri, 2012-04-06 at 11:43 +0200, Miklos Szeredi wrote:
> "Myklebust, Trond" <Trond.Myklebust@netapp.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@netapp.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@szeredi.hu>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: stable@vger.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@netapp.com
www.netapp.com

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±û"žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: [REGRESSION] NFSv4: open(O_TRUNC) hangs

[Miklos Szeredi]

"Myklebust, Trond" <Trond.Myklebust@netapp.com> writes:

> Hi Miklos
>
> On Fri, 2012-04-06 at 11:43 +0200, Miklos Szeredi wrote:
>> "Myklebust, Trond" <Trond.Myklebust@netapp.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().

Yes it should be EACCESS.  That test is broken, it should be:

	if (res != -1 || errno != EACCES)

The one in the fuse tree is correct though...

Thanks,
Miklos