Received-SPF: pass (google.com: domain of linux-nfs+bounces-3632-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: cel@kernel.org
To: <linux-nfs@vger.kernel.org>
Cc: Jeff Layton <jlayton@kernel.org>,
	Neil Brown <neilb@suse.de>,
	Dai Ngo <dai.ngo@oracle.com>,
	Olga Kornievskaia <kolga@netapp.com>,
	Tom Talpey <tom@talpey.com>,
	Chuck Lever <chuck.lever@oracle.com>,
	Christoph Hellwig <hch@lst.de>
Subject: [RFC PATCH] NFSD: Support write delegations for pNFS LAYOUT operations
Date: Mon, 10 Jun 2024 11:04:49 -0400
Message-ID: <20240610150448.2377-2-cel@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

From: Chuck Lever <chuck.lever@oracle.com>

I noticed LAYOUTGET(LAYOUTIOMODE4_RW) returning NFS4ERR_ACCESS
unexpectedly. The NFS client had created a file with mode 0444, and
the server had returned a write delegation on the OPEN(CREATE). The
client was requesting a RW layout using the write delegation stateid
so that it could flush file modifications.

This client behavior was permitted for NFSv4.1 without pNFS, so I
began looking at NFSD's implementation of LAYOUTGET.

The failure was because fh_verify() was doing a permission check as
part of verifying the FH. It uses the loga_iomode value to specify
the @accmode argument. fh_verify(MAY_WRITE) on a file whose mode is
0444 fails with -EACCES.

RFC 8881 Section 18.43.3 states:
> The use of the loga_iomode field depends upon the layout type, but
> should reflect the client's data access intent.

Further discussion of iomode values focuses on how the server is
permitted to change returned the iomode when coalescing layouts.
It says nothing about mandating the denial of LAYOUTGET requests
due to file permission settings.

Appropriate permission checking is done when the client acquires the
stateid used in the LAYOUTGET operation, so remove the permission
check from LAYOUTGET and LAYOUTCOMMIT, and rely on layout stateid
checking instead.

Cc: Christoph Hellwig <hch@lst.de>
X-Cc: stable@vger.kernel.org # v6.6+
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4proc.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 46bd20fe5c0f..c24f45870b28 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2269,23 +2269,17 @@ nfsd4_layoutget(struct svc_rqst *rqstp,
 	const struct nfsd4_layout_ops *ops;
 	struct nfs4_layout_stateid *ls;
 	__be32 nfserr;
-	int accmode = NFSD_MAY_READ_IF_EXEC;
 
+	nfserr = nfserr_badiomode;
 	switch (lgp->lg_seg.iomode) {
 	case IOMODE_READ:
-		accmode |= NFSD_MAY_READ;
-		break;
 	case IOMODE_RW:
-		accmode |= NFSD_MAY_READ | NFSD_MAY_WRITE;
 		break;
 	default:
-		dprintk("%s: invalid iomode %d\n",
-			__func__, lgp->lg_seg.iomode);
-		nfserr = nfserr_badiomode;
 		goto out;
 	}
 
-	nfserr = fh_verify(rqstp, current_fh, 0, accmode);
+	nfserr = fh_verify(rqstp, current_fh, 0, NFSD_MAY_NOP);
 	if (nfserr)
 		goto out;
 
@@ -2359,7 +2353,7 @@ nfsd4_layoutcommit(struct svc_rqst *rqstp,
 	struct nfs4_layout_stateid *ls;
 	__be32 nfserr;
 
-	nfserr = fh_verify(rqstp, current_fh, 0, NFSD_MAY_WRITE);
+	nfserr = fh_verify(rqstp, current_fh, 0, NFSD_MAY_NOP);
 	if (nfserr)
 		goto out;
 
-- 
2.45.1