[PATCH v1 08/27] NFSD: Use xdrgen XDR functions for NFSv2 SETATTR procedure

[Chuck Lever <cel@kernel.org>]

Replace the NFSPROC_SETATTR entry in the nfsd_procedures2 array
with an entry that dispatches the xdrgen-generated
nfs_svc_decode_sattrargs and nfs_svc_encode_attrstat. Wrapper
structures bridge the generated xdrgen types and the legacy
svc_fh, iattr, and kstat representations the NFSD VFS layer still
uses. The result reuses the attrstat wrapper from the GETATTR
conversion; the new nfsd_sattr_to_iattr() and
nfsd_timeval_to_timespec64() helpers translate the decoded sattr
into the iattr that nfsd_setattr() consumes.

The hand-coded svcxdr_decode_sattr() rejects a time-useconds value
greater than one second before converting it, preventing the
microsecond-to-nanosecond multiplication from wrapping to a valid
but incorrect value on 32-bit platforms. nfsd_sattr_to_iattr()
preserves that range check. Because it runs after the generated
decoder rather than inside it, an out-of-range value fails the
procedure with nfserr_io rather than the RPC GARBAGE_ARGS the
hand-coded decoder returned.

The pc_argzero field is now set to zero for the NFSv2 SETATTR
procedure. The xdrgen decoders are trusted to initialize all
arguments in the argp->xdrgen field, making the early defensive
memset unnecessary. The remaining argp fields are cleared as
needed. The wrapper's iattr is one of them: nfsd_sattr_to_iattr()
zeroes the whole structure rather than only ia_valid, because the
nfsd_vfs_setattr tracepoint records ia_size, ia_uid, and ia_gid
whether or not their ATTR bits are set.

This refactor replaces the use of svcxdr_encode_fattr(), so the
reference to the file handle can be released directly by
nfsd_proc_setattr(). A separate ->pc_release callback is thus no
longer needed. This makes it straightforward to confirm that the
file handle is properly released during every error flow.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 fs/nfsd/nfsproc.c | 165 ++++++++++++++++++++++++++++++++++++----------
 fs/nfsd/nfsxdr.c  |   9 ---
 fs/nfsd/xdr.h     |   6 --
 3 files changed, 131 insertions(+), 49 deletions(-)

diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 0196a554de5e..7fb5895f24d5 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -40,6 +40,14 @@ struct attrstat_wrapper {
 
 static_assert(offsetof(struct attrstat_wrapper, xdrgen) == 0);
 
+struct sattrargs_wrapper {
+	struct sattrargs	xdrgen;
+	struct svc_fh		fh;
+	struct iattr		iattrs;
+};
+
+static_assert(offsetof(struct sattrargs_wrapper, xdrgen) == 0);
+
 static __be32 nfsd_map_status(__be32 status)
 {
 	switch (status) {
@@ -81,6 +89,13 @@ nfsd_timespec64_to_timeval(struct timeval *dst,
 	dst->useconds = src->tv_nsec / NSEC_PER_USEC;
 }
 
+static __always_inline void
+nfsd_timeval_to_timespec64(struct timespec64 *dst, const struct timeval *src)
+{
+	dst->tv_sec = src->seconds;
+	dst->tv_nsec = src->useconds * NSEC_PER_USEC;
+}
+
 static u32
 nfsd_mode_to_ftype(umode_t mode)
 {
@@ -162,6 +177,76 @@ static __be32 nfsd_map_io_status(__be32 status)
 	return nfsd_map_status(status);
 }
 
+/*
+ * Sun convention: a sattr time-useconds field of one full second (an
+ * otherwise out-of-range value) means "set this time to the current
+ * server time." It's needed to make permissions checks for the "touch"
+ * program across NFSv2 mounts work correctly. See description of
+ * sattr in section 6.1 of "NFS Illustrated" by Brent Callaghan,
+ * Addison-Wesley, ISBN 0-201-32750-5
+ */
+#define NFS2_SATTR_SET_TO_SERVER_TIME	(1000000)
+
+static bool
+nfsd_sattr_to_iattr(struct svc_rqst *rqstp, struct iattr *iap,
+		    const struct sattr *sattr)
+{
+	static const unsigned int dont_set_it = (unsigned int)-1;
+
+	/* Consumers read ia_size, ia_uid, and ia_gid unguarded by ia_valid */
+	memset(iap, 0, sizeof(*iap));
+
+	/*
+	 * Some Sun NFS clients put 0xffff in the mode field when they
+	 * mean 0xffffffff.
+	 */
+	if (sattr->mode != dont_set_it && sattr->mode != 0xffff) {
+		iap->ia_valid |= ATTR_MODE;
+		iap->ia_mode = sattr->mode;
+	}
+	if (sattr->uid != dont_set_it) {
+		iap->ia_uid = make_kuid(nfsd_user_namespace(rqstp), sattr->uid);
+		if (uid_valid(iap->ia_uid))
+			iap->ia_valid |= ATTR_UID;
+	}
+	if (sattr->gid != dont_set_it) {
+		iap->ia_gid = make_kgid(nfsd_user_namespace(rqstp), sattr->gid);
+		if (gid_valid(iap->ia_gid))
+			iap->ia_valid |= ATTR_GID;
+	}
+	if (sattr->size != dont_set_it) {
+		iap->ia_valid |= ATTR_SIZE;
+		iap->ia_size = sattr->size;
+	}
+	if (sattr->atime.seconds != dont_set_it &&
+	    sattr->atime.useconds != dont_set_it) {
+		/*
+		 * Reject out-of-range useconds so the conversion to
+		 * nanoseconds cannot wrap to a valid but incorrect
+		 * value on 32-bit platforms.
+		 */
+		if (sattr->atime.useconds > NFS2_SATTR_SET_TO_SERVER_TIME)
+			return false;
+		iap->ia_valid |= ATTR_ATIME | ATTR_ATIME_SET;
+		nfsd_timeval_to_timespec64(&iap->ia_atime, &sattr->atime);
+
+		if (sattr->atime.useconds == NFS2_SATTR_SET_TO_SERVER_TIME)
+			iap->ia_valid &= ~ATTR_ATIME_SET;
+	}
+	if (sattr->mtime.seconds != dont_set_it &&
+	    sattr->mtime.useconds != dont_set_it) {
+		if (sattr->mtime.useconds > NFS2_SATTR_SET_TO_SERVER_TIME)
+			return false;
+		iap->ia_valid |= ATTR_MTIME | ATTR_MTIME_SET;
+		nfsd_timeval_to_timespec64(&iap->ia_mtime, &sattr->mtime);
+
+		if (sattr->mtime.useconds == NFS2_SATTR_SET_TO_SERVER_TIME)
+			iap->ia_valid &= ~(ATTR_ATIME_SET | ATTR_MTIME_SET);
+	}
+
+	return true;
+}
+
 /*
  * A full specification of each of the following NFSv2 procedures is
  * available in RFC 1094 Section 2.2.
@@ -218,27 +303,32 @@ static __be32 nfsd_proc_getattr(struct svc_rqst *rqstp)
 	return rpc_success;
 }
 
-/*
- * Set a file's attributes
- * N.B. After this call resp->fh needs an fh_put
+/**
+ * nfsd_proc_setattr - SETATTR: Set file attributes
+ * @rqstp: RPC transaction context
+ *
+ * Return:
+ *   %rpc_success:		RPC executed successfully
+ *
+ * RPC synopsis:
+ *   attrstat NFSPROC_SETATTR(sattrargs) = 2;
  */
-static __be32
-nfsd_proc_setattr(struct svc_rqst *rqstp)
+static __be32 nfsd_proc_setattr(struct svc_rqst *rqstp)
 {
-	struct nfsd_sattrargs *argp = rqstp->rq_argp;
-	struct nfsd_attrstat *resp = rqstp->rq_resp;
-	struct iattr *iap = &argp->attrs;
-	struct nfsd_attrs attrs = {
+	struct sattrargs_wrapper *argp = rqstp->rq_argp;
+	struct attrstat_wrapper *resp = rqstp->rq_resp;
+	struct kstat *statp = &resp->stat;
+	struct iattr *iap = &argp->iattrs;
+	struct nfsd_attrs nattrs = {
 		.na_iattr	= iap,
 	};
-	struct svc_fh *fhp;
-	int hosterr;
+	struct svc_fh *fhp = &argp->fh;
 
-	dprintk("nfsd: SETATTR  %s, valid=%x, size=%ld\n",
-		SVCFH_fmt(&argp->fh),
-		argp->attrs.ia_valid, (long) argp->attrs.ia_size);
-
-	fhp = fh_copy(&resp->fh, &argp->fh);
+	nfsd_fhandle_to_svc_fh(fhp, &argp->xdrgen.file);
+	if (!nfsd_sattr_to_iattr(rqstp, iap, &argp->xdrgen.attributes)) {
+		resp->xdrgen.status = nfserr_io;
+		goto out;
+	}
 
 	/*
 	 * NFSv2 does not differentiate between "set-[ac]time-to-now"
@@ -255,6 +345,8 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
 #define	MAX_TOUCH_TIME_ERROR (30*60)
 	if ((iap->ia_valid & BOTH_TIME_SET) == BOTH_TIME_SET &&
 	    iap->ia_mtime.tv_sec == iap->ia_atime.tv_sec) {
+		int hosterr;
+
 		/*
 		 * Looks probable.
 		 *
@@ -264,13 +356,13 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
 		 */
 		time64_t delta = iap->ia_atime.tv_sec - ktime_get_real_seconds();
 
-		resp->status = fh_verify(rqstp, fhp, 0, NFSD_MAY_NOP);
-		if (resp->status != nfs_ok)
+		resp->xdrgen.status = fh_verify(rqstp, fhp, 0, NFSD_MAY_NOP);
+		if (resp->xdrgen.status != nfs_ok)
 			goto out;
 
 		hosterr = fh_want_write(fhp);
 		if (hosterr) {
-			resp->status = nfserrno(hosterr);
+			resp->xdrgen.status = nfserrno(hosterr);
 			goto out;
 		}
 
@@ -287,13 +379,19 @@ nfsd_proc_setattr(struct svc_rqst *rqstp)
 		}
 	}
 
-	resp->status = nfsd_setattr(rqstp, fhp, &attrs, NULL);
-	if (resp->status != nfs_ok)
+	resp->xdrgen.status = nfsd_setattr(rqstp, fhp, &nattrs, NULL);
+	if (resp->xdrgen.status != nfs_ok)
 		goto out;
 
-	resp->status = fh_getattr(&resp->fh, &resp->stat);
+	resp->xdrgen.status = fh_getattr(fhp, statp);
+
 out:
-	resp->status = nfsd_map_status(resp->status);
+	if (resp->xdrgen.status == nfs_ok)
+		nfsd_stat_to_fattr(rqstp, &resp->xdrgen.u.attributes, statp, fhp);
+	else
+		resp->xdrgen.status = nfsd_map_status(resp->xdrgen.status);
+
+	fh_put(fhp);
 	return rpc_success;
 }
 
@@ -827,16 +925,15 @@ static const struct svc_procedure nfsd_procedures2[18] = {
 		.pc_name	= "GETATTR",
 	},
 	[NFSPROC_SETATTR] = {
-		.pc_func = nfsd_proc_setattr,
-		.pc_decode = nfssvc_decode_sattrargs,
-		.pc_encode = nfssvc_encode_attrstatres,
-		.pc_release = nfssvc_release_attrstat,
-		.pc_argsize = sizeof(struct nfsd_sattrargs),
-		.pc_argzero = sizeof(struct nfsd_sattrargs),
-		.pc_ressize = sizeof(struct nfsd_attrstat),
-		.pc_cachetype = RC_REPLBUFF,
-		.pc_xdrressize = ST+AT,
-		.pc_name = "SETATTR",
+		.pc_func	= nfsd_proc_setattr,
+		.pc_decode	= nfs_svc_decode_sattrargs,
+		.pc_encode	= nfs_svc_encode_attrstat,
+		.pc_argsize	= sizeof(struct sattrargs_wrapper),
+		.pc_argzero	= 0,
+		.pc_ressize	= sizeof(struct attrstat_wrapper),
+		.pc_cachetype	= RC_REPLBUFF,
+		.pc_xdrressize	= NFS2_attrstat_sz,
+		.pc_name	= "SETATTR",
 	},
 	[NFSPROC_ROOT] = {
 		.pc_func = nfsd_proc_root,
@@ -1014,7 +1111,7 @@ static const struct svc_procedure nfsd_procedures2[18] = {
  */
 union nfsd_xdrstore {
 	struct fhandle_wrapper		fhandle;
-	struct nfsd_sattrargs	sattr;
+	struct sattrargs_wrapper	sattrargs;
 	struct nfsd_diropargs	dirop;
 	struct nfsd_readargs	read;
 	struct nfsd_writeargs	write;
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index d55e7d6414a0..174e6f5622f9 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -302,15 +302,6 @@ nfssvc_decode_fhandleargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 	return svcxdr_decode_fhandle(xdr, &args->fh);
 }
 
-bool
-nfssvc_decode_sattrargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
-{
-	struct nfsd_sattrargs *args = rqstp->rq_argp;
-
-	return svcxdr_decode_fhandle(xdr, &args->fh) &&
-		svcxdr_decode_sattr(rqstp, xdr, &args->attrs);
-}
-
 bool
 nfssvc_decode_diropargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 {
diff --git a/fs/nfsd/xdr.h b/fs/nfsd/xdr.h
index 05b3a18c879a..9983fe212f3a 100644
--- a/fs/nfsd/xdr.h
+++ b/fs/nfsd/xdr.h
@@ -8,11 +8,6 @@
 
 #include "vfs.h"
 
-struct nfsd_sattrargs {
-	struct svc_fh		fh;
-	struct iattr		attrs;
-};
-
 struct nfsd_diropargs {
 	struct svc_fh		fh;
 	char *			name;
@@ -120,7 +115,6 @@ struct nfsd_statfsres {
 };
 
 bool nfssvc_decode_fhandleargs(struct svc_rqst *rqstp, struct xdr_stream *xdr);
-bool nfssvc_decode_sattrargs(struct svc_rqst *rqstp, struct xdr_stream *xdr);
 bool nfssvc_decode_diropargs(struct svc_rqst *rqstp, struct xdr_stream *xdr);
 bool nfssvc_decode_readargs(struct svc_rqst *rqstp, struct xdr_stream *xdr);
 bool nfssvc_decode_writeargs(struct svc_rqst *rqstp, struct xdr_stream *xdr);
-- 
2.55.0