[PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[Trond Myklebust]

The NFSv4 and NFSv4.1 specs are both clear that the server should only check
stateid open mode if a SETATTR specifies the size attribute. If the
open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.

In the case where the SETATTR is not changing the size, the client will
still pass it the delegation stateid to ensure that the server does not
recall that delegation. In that case, the server should _ignore_ the
delegation open mode, and simply apply standard permission checks.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/nfs4proc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 3bc847c..982b452 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2142,20 +2142,25 @@ static int _nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 		.rpc_cred	= cred,
         };
 	unsigned long timestamp = jiffies;
+	fmode_t fmode;
+	bool truncate;
 	int status;
 
 	nfs_fattr_init(fattr);
 
-	if (state != NULL && nfs4_valid_open_stateid(state)) {
+	/* Servers should only apply open mode checks for file size changes */
+	truncate = (sattr->ia_valid & ATTR_SIZE) ? true : false;
+	fmode = truncate ? FMODE_WRITE : FMODE_READ;
+
+	if (nfs4_copy_delegation_stateid(&arg.stateid, inode, fmode)) {
+		/* Use that stateid */
+	} else if (truncate && state != NULL && nfs4_valid_open_stateid(state)) {
 		struct nfs_lockowner lockowner = {
 			.l_owner = current->files,
 			.l_pid = current->tgid,
 		};
 		nfs4_select_rw_stateid(&arg.stateid, state, FMODE_WRITE,
 				&lockowner);
-	} else if (nfs4_copy_delegation_stateid(&arg.stateid, inode,
-				FMODE_WRITE)) {
-		/* Use that stateid */
 	} else
 		nfs4_stateid_copy(&arg.stateid, &zero_stateid);
 
-- 
1.8.1.4

[PATCH 2/2] NFSv4: Warn once about servers that incorrectly apply open mode to setattr

[Trond Myklebust]

Debugging aid to help identify servers that incorrectly apply open mode
checks to setattr requests that are not changing the file size.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/nfs4proc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 982b452..9da4bd5 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2184,6 +2184,13 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 		err = _nfs4_do_setattr(inode, cred, fattr, sattr, state);
 		switch (err) {
 		case -NFS4ERR_OPENMODE:
+			if (!(sattr->ia_valid & ATTR_SIZE)) {
+				pr_warn_once("NFSv4: server %s is incorrectly "
+						"applying open mode checks to "
+						"a SETATTR that is not "
+						"changing file size.\n",
+						server->nfs_client->cl_hostname);
+			}
 			if (state && !(state->state & FMODE_WRITE)) {
 				err = -EBADF;
 				if (sattr->ia_valid & ATTR_OPEN)
-- 
1.8.1.4

Re: [PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[Myklebust, Trond]

On Mon, 2013-04-29 at 11:15 -0400, Trond Myklebust wrote:
> The NFSv4 and NFSv4.1 specs are both clear that the server should only check
> stateid open mode if a SETATTR specifies the size attribute. If the
> open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.
> 
> In the case where the SETATTR is not changing the size, the client will
> still pass it the delegation stateid to ensure that the server does not
> recall that delegation. In that case, the server should _ignore_ the
> delegation open mode, and simply apply standard permission checks.

Bruce, what does the Linux server do when we send it a delegation
stateid as part of a SETATTR that just changes the mode or acl (no size
change)? Will it recall the delegation? How about the delegations held
by other clients?

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[J. Bruce Fields]

On Mon, Apr 29, 2013 at 06:15:52PM +0000, Myklebust, Trond wrote:
> On Mon, 2013-04-29 at 11:15 -0400, Trond Myklebust wrote:
> > The NFSv4 and NFSv4.1 specs are both clear that the server should only check
> > stateid open mode if a SETATTR specifies the size attribute. If the
> > open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.
> > 
> > In the case where the SETATTR is not changing the size, the client will
> > still pass it the delegation stateid to ensure that the server does not
> > recall that delegation. In that case, the server should _ignore_ the
> > delegation open mode, and simply apply standard permission checks.
> 
> Bruce, what does the Linux server do when we send it a delegation
> stateid as part of a SETATTR that just changes the mode or acl (no size
> change)? Will it recall the delegation? How about the delegations held
> by other clients?

Yes, it breaks all delegations on any setattr.

--b.

Re: [PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[Myklebust, Trond]

On Mon, 2013-04-29 at 14:21 -0400, J. Bruce Fields wrote:
> On Mon, Apr 29, 2013 at 06:15:52PM +0000, Myklebust, Trond wrote:
> > On Mon, 2013-04-29 at 11:15 -0400, Trond Myklebust wrote:
> > > The NFSv4 and NFSv4.1 specs are both clear that the server should only check
> > > stateid open mode if a SETATTR specifies the size attribute. If the
> > > open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.
> > > 
> > > In the case where the SETATTR is not changing the size, the client will
> > > still pass it the delegation stateid to ensure that the server does not
> > > recall that delegation. In that case, the server should _ignore_ the
> > > delegation open mode, and simply apply standard permission checks.
> > 
> > Bruce, what does the Linux server do when we send it a delegation
> > stateid as part of a SETATTR that just changes the mode or acl (no size
> > change)? Will it recall the delegation? How about the delegations held
> > by other clients?
> 
> Yes, it breaks all delegations on any setattr.

Do you plan to change that?

The main problem with the current scheme is that you can't hand out a
delegation as part of an exclusive create: the client would have to
return it immediately so that it can do the setattr to initialise the
file attributes.


-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[J. Bruce Fields]

On Mon, Apr 29, 2013 at 06:27:07PM +0000, Myklebust, Trond wrote:
> On Mon, 2013-04-29 at 14:21 -0400, J. Bruce Fields wrote:
> > On Mon, Apr 29, 2013 at 06:15:52PM +0000, Myklebust, Trond wrote:
> > > On Mon, 2013-04-29 at 11:15 -0400, Trond Myklebust wrote:
> > > > The NFSv4 and NFSv4.1 specs are both clear that the server should only check
> > > > stateid open mode if a SETATTR specifies the size attribute. If the
> > > > open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.
> > > > 
> > > > In the case where the SETATTR is not changing the size, the client will
> > > > still pass it the delegation stateid to ensure that the server does not
> > > > recall that delegation. In that case, the server should _ignore_ the
> > > > delegation open mode, and simply apply standard permission checks.
> > > 
> > > Bruce, what does the Linux server do when we send it a delegation
> > > stateid as part of a SETATTR that just changes the mode or acl (no size
> > > change)? Will it recall the delegation? How about the delegations held
> > > by other clients?
> > 
> > Yes, it breaks all delegations on any setattr.
> 
> Do you plan to change that?

I'd take patches.

--b.

> The main problem with the current scheme is that you can't hand out a
> delegation as part of an exclusive create: the client would have to
> return it immediately so that it can do the setattr to initialise the
> file attributes.

Re: [PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[Myklebust, Trond]

On Mon, 2013-04-29 at 14:29 -0400, J. Bruce Fields wrote:
> On Mon, Apr 29, 2013 at 06:27:07PM +0000, Myklebust, Trond wrote:
> > On Mon, 2013-04-29 at 14:21 -0400, J. Bruce Fields wrote:
> > > On Mon, Apr 29, 2013 at 06:15:52PM +0000, Myklebust, Trond wrote:
> > > > On Mon, 2013-04-29 at 11:15 -0400, Trond Myklebust wrote:
> > > > > The NFSv4 and NFSv4.1 specs are both clear that the server should only check
> > > > > stateid open mode if a SETATTR specifies the size attribute. If the
> > > > > open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.
> > > > > 
> > > > > In the case where the SETATTR is not changing the size, the client will
> > > > > still pass it the delegation stateid to ensure that the server does not
> > > > > recall that delegation. In that case, the server should _ignore_ the
> > > > > delegation open mode, and simply apply standard permission checks.
> > > > 
> > > > Bruce, what does the Linux server do when we send it a delegation
> > > > stateid as part of a SETATTR that just changes the mode or acl (no size
> > > > change)? Will it recall the delegation? How about the delegations held
> > > > by other clients?
> > > 
> > > Yes, it breaks all delegations on any setattr.
> > 
> > Do you plan to change that?
> 
> I'd take patches.

Have the NFSv4 delegation patches been merged yet, and do they hand out
delegations on file creation?

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[J. Bruce Fields]

On Mon, Apr 29, 2013 at 06:33:02PM +0000, Myklebust, Trond wrote:
> On Mon, 2013-04-29 at 14:29 -0400, J. Bruce Fields wrote:
> > On Mon, Apr 29, 2013 at 06:27:07PM +0000, Myklebust, Trond wrote:
> > > On Mon, 2013-04-29 at 14:21 -0400, J. Bruce Fields wrote:
> > > > On Mon, Apr 29, 2013 at 06:15:52PM +0000, Myklebust, Trond wrote:
> > > > > On Mon, 2013-04-29 at 11:15 -0400, Trond Myklebust wrote:
> > > > > > The NFSv4 and NFSv4.1 specs are both clear that the server should only check
> > > > > > stateid open mode if a SETATTR specifies the size attribute. If the
> > > > > > open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.
> > > > > > 
> > > > > > In the case where the SETATTR is not changing the size, the client will
> > > > > > still pass it the delegation stateid to ensure that the server does not
> > > > > > recall that delegation. In that case, the server should _ignore_ the
> > > > > > delegation open mode, and simply apply standard permission checks.
> > > > > 
> > > > > Bruce, what does the Linux server do when we send it a delegation
> > > > > stateid as part of a SETATTR that just changes the mode or acl (no size
> > > > > change)? Will it recall the delegation? How about the delegations held
> > > > > by other clients?
> > > > 
> > > > Yes, it breaks all delegations on any setattr.
> > > 
> > > Do you plan to change that?
> > 
> > I'd take patches.
> 
> Have the NFSv4 delegation patches been merged yet, and do they hand out
> delegations on file creation?

The vfs support patches haven't been merged, but nfsd still continues to
give out read delegations on any confirmed read-open of a file that
hasn't had a recent delegation conflict.  (They're just not consistent
w.r.t local metadata operations....)

--b.

Re: [PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[Myklebust, Trond]

On Mon, 2013-04-29 at 14:41 -0400, J. Bruce Fields wrote:
> On Mon, Apr 29, 2013 at 06:33:02PM +0000, Myklebust, Trond wrote:
> > On Mon, 2013-04-29 at 14:29 -0400, J. Bruce Fields wrote:
> > > On Mon, Apr 29, 2013 at 06:27:07PM +0000, Myklebust, Trond wrote:
> > > > On Mon, 2013-04-29 at 14:21 -0400, J. Bruce Fields wrote:
> > > > > On Mon, Apr 29, 2013 at 06:15:52PM +0000, Myklebust, Trond wrote:
> > > > > > On Mon, 2013-04-29 at 11:15 -0400, Trond Myklebust wrote:
> > > > > > > The NFSv4 and NFSv4.1 specs are both clear that the server should only check
> > > > > > > stateid open mode if a SETATTR specifies the size attribute. If the
> > > > > > > open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.
> > > > > > > 
> > > > > > > In the case where the SETATTR is not changing the size, the client will
> > > > > > > still pass it the delegation stateid to ensure that the server does not
> > > > > > > recall that delegation. In that case, the server should _ignore_ the
> > > > > > > delegation open mode, and simply apply standard permission checks.
> > > > > > 
> > > > > > Bruce, what does the Linux server do when we send it a delegation
> > > > > > stateid as part of a SETATTR that just changes the mode or acl (no size
> > > > > > change)? Will it recall the delegation? How about the delegations held
> > > > > > by other clients?
> > > > > 
> > > > > Yes, it breaks all delegations on any setattr.
> > > > 
> > > > Do you plan to change that?
> > > 
> > > I'd take patches.
> > 
> > Have the NFSv4 delegation patches been merged yet, and do they hand out
> > delegations on file creation?
> 
> The vfs support patches haven't been merged, but nfsd still continues to
> give out read delegations on any confirmed read-open of a file that
> hasn't had a recent delegation conflict.  (They're just not consistent
> w.r.t local metadata operations....)

The problem is that without those there seems to be no VFS level
functionality that allows you to say "break the leases for all processes
except this one".

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[J. Bruce Fields]

On Mon, Apr 29, 2013 at 07:09:41PM +0000, Myklebust, Trond wrote:
> On Mon, 2013-04-29 at 14:41 -0400, J. Bruce Fields wrote:
> > On Mon, Apr 29, 2013 at 06:33:02PM +0000, Myklebust, Trond wrote:
> > > On Mon, 2013-04-29 at 14:29 -0400, J. Bruce Fields wrote:
> > > > On Mon, Apr 29, 2013 at 06:27:07PM +0000, Myklebust, Trond wrote:
> > > > > On Mon, 2013-04-29 at 14:21 -0400, J. Bruce Fields wrote:
> > > > > > On Mon, Apr 29, 2013 at 06:15:52PM +0000, Myklebust, Trond wrote:
> > > > > > > On Mon, 2013-04-29 at 11:15 -0400, Trond Myklebust wrote:
> > > > > > > > The NFSv4 and NFSv4.1 specs are both clear that the server should only check
> > > > > > > > stateid open mode if a SETATTR specifies the size attribute. If the
> > > > > > > > open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.
> > > > > > > > 
> > > > > > > > In the case where the SETATTR is not changing the size, the client will
> > > > > > > > still pass it the delegation stateid to ensure that the server does not
> > > > > > > > recall that delegation. In that case, the server should _ignore_ the
> > > > > > > > delegation open mode, and simply apply standard permission checks.
> > > > > > > 
> > > > > > > Bruce, what does the Linux server do when we send it a delegation
> > > > > > > stateid as part of a SETATTR that just changes the mode or acl (no size
> > > > > > > change)? Will it recall the delegation? How about the delegations held
> > > > > > > by other clients?
> > > > > > 
> > > > > > Yes, it breaks all delegations on any setattr.
> > > > > 
> > > > > Do you plan to change that?
> > > > 
> > > > I'd take patches.
> > > 
> > > Have the NFSv4 delegation patches been merged yet, and do they hand out
> > > delegations on file creation?
> > 
> > The vfs support patches haven't been merged, but nfsd still continues to
> > give out read delegations on any confirmed read-open of a file that
> > hasn't had a recent delegation conflict.  (They're just not consistent
> > w.r.t local metadata operations....)
> 
> The problem is that without those there seems to be no VFS level
> functionality that allows you to say "break the leases for all processes
> except this one".

There isn't any *with* those patches, either.  I agree that it would be
nice to allow a client issuing a write or setattr to continue to hold
its read delegation, if it's the only holder of a delegation.  But my
understanding is that a server isn't required to do that.  So I'm taking
this one step at a time and ignoring that problem for now.

--b.

Re: [PATCH 1/2] NFSv4: Servers should only check SETATTR stateid open mode on size change

[J. Bruce Fields]

On Mon, Apr 29, 2013 at 02:21:45PM -0400, J. Bruce Fields wrote:
> On Mon, Apr 29, 2013 at 06:15:52PM +0000, Myklebust, Trond wrote:
> > On Mon, 2013-04-29 at 11:15 -0400, Trond Myklebust wrote:
> > > The NFSv4 and NFSv4.1 specs are both clear that the server should only check
> > > stateid open mode if a SETATTR specifies the size attribute. If the
> > > open mode is not one that allows writing, then it returns NFS4ERR_OPENMODE.
> > > 
> > > In the case where the SETATTR is not changing the size, the client will
> > > still pass it the delegation stateid to ensure that the server does not
> > > recall that delegation. In that case, the server should _ignore_ the
> > > delegation open mode, and simply apply standard permission checks.
> > 
> > Bruce, what does the Linux server do when we send it a delegation
> > stateid as part of a SETATTR that just changes the mode or acl (no size
> > change)? Will it recall the delegation? How about the delegations held
> > by other clients?
> 
> Yes, it breaks all delegations on any setattr.

(Well, any setattr that doesn't set size--if it sets size, you'll get
OPENMODE (since we don't support write delegations) before we get around
to breaking any delegations.

In fact, it skips all the stateid processing entirely in the case size
isn't set--so you could send garbage in the stateid field and it'd
work.)

--b.