[PATCH v1 00/27] Server-side RPC reply header parsing overhaul

[PATCH v1 00/27] Server-side RPC reply header parsing overhaul

[Chuck Lever]

The purpose of this series is to replace the svc_put* macros in the
Linux kernel server's RPC reply header construction code with
xdr_stream helpers. I've measured no change in CPU utilization after
the overhaul.

Memory safety: Buffer bounds checking after encoding each XDR item
is more memory-safe than the current mechanism. Subsequent memory
safety improvements to the common xdr_stream helpers will benefit
all who use them.

Audit friendliness: The new code has additional comments and other
clean-up to help align it with the relevant RPC protocol
specifications. The use of common helpers also makes the encoders
easier to audit and maintain.

I've split the full series in half to make it easier to review. The
patches posted here are the second half, handling RPC reply header
encoding.

Note that another benefit of this work is that we are taking one or
two more strides closer to greater commonality between the client
and server implementations of RPCSEC GSS.

---

Chuck Lever (27):
      SUNRPC: Clean up svcauth_gss_release()
      SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()
      SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()
      SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()
      SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()
      SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()
      SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()
      SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()
      SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()
      SUNRPC: Check rq_auth_stat when preparing to wrap a response
      SUNRPC: Remove the rpc_stat variable in svc_process_common()
      SUNRPC: Add XDR encoding helper for opaque_auth
      SUNRPC: Push svcxdr_init_encode() into svc_process_common()
      SUNRPC: Move svcxdr_init_encode() into ->accept methods
      SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()
      SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()
      SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()
      SUNRPC: Convert unwrap data paths to use xdr_stream for replies
      SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers
      SUNRPC: Use xdr_stream for encoding GSS reply verifiers
      SUNRPC: Hoist init_encode out of svc_authenticate()
      SUNRPC: Convert RPC Reply header encoding to use xdr_stream
      SUNRPC: Final clean-up of svc_process_common()
      SUNRPC: Remove no-longer-used helper functions
      SUNRPC: Refactor RPC server dispatch method
      SUNRPC: Set rq_accept_statp inside ->accept methods
      SUNRPC: Go back to using gsd->body_start


 fs/lockd/svc.c                    |   5 +-
 fs/nfs/callback_xdr.c             |   6 +-
 fs/nfsd/nfscache.c                |   4 +-
 fs/nfsd/nfsd.h                    |   2 +-
 fs/nfsd/nfssvc.c                  |  10 +-
 include/linux/sunrpc/svc.h        | 116 +++----
 include/linux/sunrpc/xdr.h        |  23 ++
 include/trace/events/rpcgss.h     |  22 ++
 net/sunrpc/auth_gss/svcauth_gss.c | 505 +++++++++++++++---------------
 net/sunrpc/svc.c                  |  91 +++---
 net/sunrpc/svcauth_unix.c         |  40 ++-
 net/sunrpc/xdr.c                  |  29 ++
 12 files changed, 451 insertions(+), 402 deletions(-)

--
Chuck Lever

[PATCH v1 01/27] SUNRPC: Clean up svcauth_gss_release()

[Chuck Lever]

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

Now that upper layers use an xdr_stream to track the construction
of each RPC Reply message, resbuf->len is kept up-to-date
automatically. There's no need to recompute it in svc_gss_release().

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 2e603358fae1..4a576ed7aa32 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -969,12 +969,6 @@ svcauth_gss_unwrap_integ(struct svc_rqst *rqstp, u32 seq, struct gss_ctx *ctx)
 	return -EINVAL;
 }
 
-static inline int
-total_buf_len(struct xdr_buf *buf)
-{
-	return buf->head[0].iov_len + buf->page_len + buf->tail[0].iov_len;
-}
-
 /*
  * RFC 2203, Section 5.3.2.3
  *
@@ -1882,14 +1876,25 @@ svcauth_gss_wrap_resp_priv(struct svc_rqst *rqstp)
 	return 0;
 }
 
+/**
+ * svcauth_gss_release - Wrap payload and release resources
+ * @rqstp: RPC transaction context
+ *
+ * Return values:
+ *    %0: the Reply is ready to be sent
+ *    %-ENOMEM: failed to allocate memory
+ *    %-EINVAL: encoding error
+ *
+ * XXX: These return values do not match the return values documented
+ *      for the auth_ops ->release method in linux/sunrpc/svcauth.h.
+ */
 static int
 svcauth_gss_release(struct svc_rqst *rqstp)
 {
-	struct gss_svc_data *gsd = (struct gss_svc_data *)rqstp->rq_auth_data;
-	struct rpc_gss_wire_cred *gc;
-	struct xdr_buf *resbuf = &rqstp->rq_res;
-	int stat = -EINVAL;
 	struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
+	struct gss_svc_data *gsd = rqstp->rq_auth_data;
+	struct rpc_gss_wire_cred *gc;
+	int stat;
 
 	if (!gsd)
 		goto out;
@@ -1899,10 +1904,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 	/* Release can be called twice, but we only wrap once. */
 	if (gsd->verf_start == NULL)
 		goto out;
-	/* normally not set till svc_send, but we need it here: */
-	/* XXX: what for?  Do we mess it up the moment we call svc_putu32
-	 * or whatever? */
-	resbuf->len = total_buf_len(resbuf);
+
 	switch (gc->gc_svc) {
 	case RPC_GSS_SVC_NONE:
 		break;

[PATCH v1 02/27] SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()

[Chuck Lever]

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

Clean up: To help orient readers, name the stack variables to match
the XDR field names.

Additionally, the explicit type cast on @gsd is unnecessary; and
@resbuf is renamed to match the variable naming in the unwrap
functions.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   70 +++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 4a576ed7aa32..fe0bd0ad8ace 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1758,49 +1758,65 @@ svcauth_gss_prepare_to_wrap(struct xdr_buf *resbuf, struct gss_svc_data *gsd)
 	return p;
 }
 
-static inline int
-svcauth_gss_wrap_resp_integ(struct svc_rqst *rqstp)
+/*
+ * RFC 2203, Section 5.3.2.2
+ *
+ *	struct rpc_gss_integ_data {
+ *		opaque databody_integ<>;
+ *		opaque checksum<>;
+ *	};
+ *
+ *	struct rpc_gss_data_t {
+ *		unsigned int seq_num;
+ *		proc_req_arg_t arg;
+ *	};
+ *
+ * The RPC Reply message has already been XDR-encoded. rq_res_stream
+ * is now positioned so that the checksum can be written just past
+ * the RPC Reply message.
+ */
+static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 {
-	struct gss_svc_data *gsd = (struct gss_svc_data *)rqstp->rq_auth_data;
+	struct gss_svc_data *gsd = rqstp->rq_auth_data;
 	struct rpc_gss_wire_cred *gc = &gsd->clcred;
-	struct xdr_buf *resbuf = &rqstp->rq_res;
-	struct xdr_buf integ_buf;
-	struct xdr_netobj mic;
+	struct xdr_buf *buf = &rqstp->rq_res;
+	struct xdr_buf databody_integ;
+	struct xdr_netobj checksum;
 	struct kvec *resv;
+	u32 offset, len;
 	__be32 *p;
-	int integ_offset, integ_len;
 	int stat = -EINVAL;
 
-	p = svcauth_gss_prepare_to_wrap(resbuf, gsd);
+	p = svcauth_gss_prepare_to_wrap(buf, gsd);
 	if (p == NULL)
 		goto out;
-	integ_offset = (u8 *)(p + 1) - (u8 *)resbuf->head[0].iov_base;
-	integ_len = resbuf->len - integ_offset;
-	if (integ_len & 3)
+	offset = (u8 *)(p + 1) - (u8 *)buf->head[0].iov_base;
+	len = buf->len - offset;
+	if (len & 3)
 		goto out;
-	*p++ = htonl(integ_len);
+	*p++ = htonl(len);
 	*p++ = htonl(gc->gc_seq);
-	if (xdr_buf_subsegment(resbuf, &integ_buf, integ_offset, integ_len)) {
+	if (xdr_buf_subsegment(buf, &databody_integ, offset, len)) {
 		WARN_ON_ONCE(1);
 		goto out_err;
 	}
-	if (resbuf->tail[0].iov_base == NULL) {
-		if (resbuf->head[0].iov_len + RPC_MAX_AUTH_SIZE > PAGE_SIZE)
+	if (!buf->tail[0].iov_base) {
+		if (buf->head[0].iov_len + RPC_MAX_AUTH_SIZE > PAGE_SIZE)
 			goto out_err;
-		resbuf->tail[0].iov_base = resbuf->head[0].iov_base
-						+ resbuf->head[0].iov_len;
-		resbuf->tail[0].iov_len = 0;
+		buf->tail[0].iov_base = buf->head[0].iov_base
+						+ buf->head[0].iov_len;
+		buf->tail[0].iov_len = 0;
 	}
-	resv = &resbuf->tail[0];
-	mic.data = (u8 *)resv->iov_base + resv->iov_len + 4;
-	if (gss_get_mic(gsd->rsci->mechctx, &integ_buf, &mic))
+	resv = &buf->tail[0];
+	checksum.data = (u8 *)resv->iov_base + resv->iov_len + 4;
+	if (gss_get_mic(gsd->rsci->mechctx, &databody_integ, &checksum))
 		goto out_err;
-	svc_putnl(resv, mic.len);
-	memset(mic.data + mic.len, 0,
-			round_up_to_quad(mic.len) - mic.len);
-	resv->iov_len += XDR_QUADLEN(mic.len) << 2;
+	svc_putnl(resv, checksum.len);
+	memset(checksum.data + checksum.len, 0,
+	       round_up_to_quad(checksum.len) - checksum.len);
+	resv->iov_len += XDR_QUADLEN(checksum.len) << 2;
 	/* not strictly required: */
-	resbuf->len += XDR_QUADLEN(mic.len) << 2;
+	buf->len += XDR_QUADLEN(checksum.len) << 2;
 	if (resv->iov_len > PAGE_SIZE)
 		goto out_err;
 out:
@@ -1909,7 +1925,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 	case RPC_GSS_SVC_NONE:
 		break;
 	case RPC_GSS_SVC_INTEGRITY:
-		stat = svcauth_gss_wrap_resp_integ(rqstp);
+		stat = svcauth_gss_wrap_integ(rqstp);
 		if (stat)
 			goto out_err;
 		break;

[PATCH v1 03/27] SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()

[Chuck Lever]

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

An error computing the checksum here is an exceptional event.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/trace/events/rpcgss.h     |    1 +
 net/sunrpc/auth_gss/svcauth_gss.c |   26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/include/trace/events/rpcgss.h b/include/trace/events/rpcgss.h
index 3f121eed369e..261751ac241c 100644
--- a/include/trace/events/rpcgss.h
+++ b/include/trace/events/rpcgss.h
@@ -208,6 +208,7 @@ DECLARE_EVENT_CLASS(rpcgss_svc_gssapi_class,
 
 DEFINE_SVC_GSSAPI_EVENT(unwrap);
 DEFINE_SVC_GSSAPI_EVENT(mic);
+DEFINE_SVC_GSSAPI_EVENT(get_mic);
 
 TRACE_EVENT(rpcgss_svc_unwrap_failed,
 	TP_PROTO(
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index fe0bd0ad8ace..2d1e8431e903 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1782,10 +1782,9 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 	struct xdr_buf *buf = &rqstp->rq_res;
 	struct xdr_buf databody_integ;
 	struct xdr_netobj checksum;
+	u32 offset, len, maj_stat;
 	struct kvec *resv;
-	u32 offset, len;
 	__be32 *p;
-	int stat = -EINVAL;
 
 	p = svcauth_gss_prepare_to_wrap(buf, gsd);
 	if (p == NULL)
@@ -1796,21 +1795,20 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 		goto out;
 	*p++ = htonl(len);
 	*p++ = htonl(gc->gc_seq);
-	if (xdr_buf_subsegment(buf, &databody_integ, offset, len)) {
-		WARN_ON_ONCE(1);
-		goto out_err;
-	}
+	if (xdr_buf_subsegment(buf, &databody_integ, offset, len))
+		goto wrap_failed;
 	if (!buf->tail[0].iov_base) {
 		if (buf->head[0].iov_len + RPC_MAX_AUTH_SIZE > PAGE_SIZE)
-			goto out_err;
+			goto wrap_failed;
 		buf->tail[0].iov_base = buf->head[0].iov_base
 						+ buf->head[0].iov_len;
 		buf->tail[0].iov_len = 0;
 	}
 	resv = &buf->tail[0];
 	checksum.data = (u8 *)resv->iov_base + resv->iov_len + 4;
-	if (gss_get_mic(gsd->rsci->mechctx, &databody_integ, &checksum))
-		goto out_err;
+	maj_stat = gss_get_mic(gsd->rsci->mechctx, &databody_integ, &checksum);
+	if (maj_stat != GSS_S_COMPLETE)
+		goto bad_mic;
 	svc_putnl(resv, checksum.len);
 	memset(checksum.data + checksum.len, 0,
 	       round_up_to_quad(checksum.len) - checksum.len);
@@ -1818,11 +1816,13 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 	/* not strictly required: */
 	buf->len += XDR_QUADLEN(checksum.len) << 2;
 	if (resv->iov_len > PAGE_SIZE)
-		goto out_err;
+		goto wrap_failed;
 out:
-	stat = 0;
-out_err:
-	return stat;
+	return 0;
+bad_mic:
+	trace_rpcgss_svc_get_mic(rqstp, maj_stat);
+wrap_failed:
+	return -EINVAL;
 }
 
 static inline int

[PATCH v1 04/27] SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()

[Chuck Lever]

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

Replace finicky logic: Instead of trying to find scratch space in
the response buffer, use the scratch buffer from struct
gss_svc_data.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 2d1e8431e903..6aefe24953fa 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1797,15 +1797,8 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 	*p++ = htonl(gc->gc_seq);
 	if (xdr_buf_subsegment(buf, &databody_integ, offset, len))
 		goto wrap_failed;
-	if (!buf->tail[0].iov_base) {
-		if (buf->head[0].iov_len + RPC_MAX_AUTH_SIZE > PAGE_SIZE)
-			goto wrap_failed;
-		buf->tail[0].iov_base = buf->head[0].iov_base
-						+ buf->head[0].iov_len;
-		buf->tail[0].iov_len = 0;
-	}
-	resv = &buf->tail[0];
-	checksum.data = (u8 *)resv->iov_base + resv->iov_len + 4;
+
+	checksum.data = gsd->gsd_scratch;
 	maj_stat = gss_get_mic(gsd->rsci->mechctx, &databody_integ, &checksum);
 	if (maj_stat != GSS_S_COMPLETE)
 		goto bad_mic;

[PATCH v1 05/27] SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()

[Chuck Lever]

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

Done as part of hardening the server-side RPC header decoding path.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 6aefe24953fa..3715ff842ca1 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1778,40 +1778,39 @@ svcauth_gss_prepare_to_wrap(struct xdr_buf *resbuf, struct gss_svc_data *gsd)
 static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 {
 	struct gss_svc_data *gsd = rqstp->rq_auth_data;
+	struct xdr_stream *xdr = &rqstp->rq_res_stream;
 	struct rpc_gss_wire_cred *gc = &gsd->clcred;
-	struct xdr_buf *buf = &rqstp->rq_res;
+	struct xdr_buf *buf = xdr->buf;
 	struct xdr_buf databody_integ;
 	struct xdr_netobj checksum;
 	u32 offset, len, maj_stat;
-	struct kvec *resv;
 	__be32 *p;
 
 	p = svcauth_gss_prepare_to_wrap(buf, gsd);
 	if (p == NULL)
 		goto out;
+
 	offset = (u8 *)(p + 1) - (u8 *)buf->head[0].iov_base;
 	len = buf->len - offset;
-	if (len & 3)
-		goto out;
-	*p++ = htonl(len);
-	*p++ = htonl(gc->gc_seq);
 	if (xdr_buf_subsegment(buf, &databody_integ, offset, len))
 		goto wrap_failed;
+	/* Buffer space for these has already been reserved in
+	 * svcauth_gss_accept(). */
+	*p++ = cpu_to_be32(len);
+	*p = cpu_to_be32(gc->gc_seq);
 
 	checksum.data = gsd->gsd_scratch;
 	maj_stat = gss_get_mic(gsd->rsci->mechctx, &databody_integ, &checksum);
 	if (maj_stat != GSS_S_COMPLETE)
 		goto bad_mic;
-	svc_putnl(resv, checksum.len);
-	memset(checksum.data + checksum.len, 0,
-	       round_up_to_quad(checksum.len) - checksum.len);
-	resv->iov_len += XDR_QUADLEN(checksum.len) << 2;
-	/* not strictly required: */
-	buf->len += XDR_QUADLEN(checksum.len) << 2;
-	if (resv->iov_len > PAGE_SIZE)
+
+	if (xdr_stream_encode_opaque(xdr, checksum.data, checksum.len) < 0)
 		goto wrap_failed;
+	xdr_commit_encode(xdr);
+
 out:
 	return 0;
+
 bad_mic:
 	trace_rpcgss_svc_get_mic(rqstp, maj_stat);
 wrap_failed:

[PATCH v1 06/27] SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()

[Chuck Lever]

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

Clean up variable names to match the other unwrap and wrap
functions.

Additionally, the explicit type cast on @gsd in unnecessary; and
@resbuf is renamed to match the variable naming in the unwrap
functions.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   72 ++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 3715ff842ca1..f0cd89f201bc 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1817,24 +1817,35 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 	return -EINVAL;
 }
 
-static inline int
-svcauth_gss_wrap_resp_priv(struct svc_rqst *rqstp)
+/*
+ * RFC 2203, Section 5.3.2.3
+ *
+ *	struct rpc_gss_priv_data {
+ *		opaque databody_priv<>
+ *	};
+ *
+ *	struct rpc_gss_data_t {
+ *		unsigned int seq_num;
+ *		proc_req_arg_t arg;
+ *	};
+ */
+static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 {
-	struct gss_svc_data *gsd = (struct gss_svc_data *)rqstp->rq_auth_data;
+	struct gss_svc_data *gsd = rqstp->rq_auth_data;
 	struct rpc_gss_wire_cred *gc = &gsd->clcred;
-	struct xdr_buf *resbuf = &rqstp->rq_res;
+	struct xdr_buf *buf = &rqstp->rq_res;
 	struct page **inpages = NULL;
-	__be32 *p, *len;
+	__be32 *p, *lenp;
 	int offset;
 	int pad;
 
-	p = svcauth_gss_prepare_to_wrap(resbuf, gsd);
+	p = svcauth_gss_prepare_to_wrap(buf, gsd);
 	if (p == NULL)
 		return 0;
-	len = p++;
-	offset = (u8 *)p - (u8 *)resbuf->head[0].iov_base;
+	lenp = p++;
+	offset = (u8 *)p - (u8 *)buf->head[0].iov_base;
 	*p++ = htonl(gc->gc_seq);
-	inpages = resbuf->pages;
+	inpages = buf->pages;
 	/* XXX: Would be better to write some xdr helper functions for
 	 * nfs{2,3,4}xdr.c that place the data right, instead of copying: */
 
@@ -1845,19 +1856,19 @@ svcauth_gss_wrap_resp_priv(struct svc_rqst *rqstp)
 	 * there is RPC_MAX_AUTH_SIZE slack space available in
 	 * both the head and tail.
 	 */
-	if (resbuf->tail[0].iov_base) {
-		if (resbuf->tail[0].iov_base >=
-			resbuf->head[0].iov_base + PAGE_SIZE)
+	if (buf->tail[0].iov_base) {
+		if (buf->tail[0].iov_base >=
+			buf->head[0].iov_base + PAGE_SIZE)
 			return -EINVAL;
-		if (resbuf->tail[0].iov_base < resbuf->head[0].iov_base)
+		if (buf->tail[0].iov_base < buf->head[0].iov_base)
 			return -EINVAL;
-		if (resbuf->tail[0].iov_len + resbuf->head[0].iov_len
+		if (buf->tail[0].iov_len + buf->head[0].iov_len
 				+ 2 * RPC_MAX_AUTH_SIZE > PAGE_SIZE)
 			return -ENOMEM;
-		memmove(resbuf->tail[0].iov_base + RPC_MAX_AUTH_SIZE,
-			resbuf->tail[0].iov_base,
-			resbuf->tail[0].iov_len);
-		resbuf->tail[0].iov_base += RPC_MAX_AUTH_SIZE;
+		memmove(buf->tail[0].iov_base + RPC_MAX_AUTH_SIZE,
+			buf->tail[0].iov_base,
+			buf->tail[0].iov_len);
+		buf->tail[0].iov_base += RPC_MAX_AUTH_SIZE;
 	}
 	/*
 	 * If there is no current tail data, make sure there is
@@ -1866,21 +1877,22 @@ svcauth_gss_wrap_resp_priv(struct svc_rqst *rqstp)
 	 * is RPC_MAX_AUTH_SIZE slack space available in both the
 	 * head and tail.
 	 */
-	if (resbuf->tail[0].iov_base == NULL) {
-		if (resbuf->head[0].iov_len + 2*RPC_MAX_AUTH_SIZE > PAGE_SIZE)
+	if (!buf->tail[0].iov_base) {
+		if (buf->head[0].iov_len + 2 * RPC_MAX_AUTH_SIZE > PAGE_SIZE)
 			return -ENOMEM;
-		resbuf->tail[0].iov_base = resbuf->head[0].iov_base
-			+ resbuf->head[0].iov_len + RPC_MAX_AUTH_SIZE;
-		resbuf->tail[0].iov_len = 0;
+		buf->tail[0].iov_base = buf->head[0].iov_base
+			+ buf->head[0].iov_len + RPC_MAX_AUTH_SIZE;
+		buf->tail[0].iov_len = 0;
 	}
-	if (gss_wrap(gsd->rsci->mechctx, offset, resbuf, inpages))
+	if (gss_wrap(gsd->rsci->mechctx, offset, buf, inpages))
 		return -ENOMEM;
-	*len = htonl(resbuf->len - offset);
-	pad = 3 - ((resbuf->len - offset - 1)&3);
-	p = (__be32 *)(resbuf->tail[0].iov_base + resbuf->tail[0].iov_len);
+	*lenp = htonl(buf->len - offset);
+	pad = 3 - ((buf->len - offset - 1) & 3);
+	p = (__be32 *)(buf->tail[0].iov_base + buf->tail[0].iov_len);
 	memset(p, 0, pad);
-	resbuf->tail[0].iov_len += pad;
-	resbuf->len += pad;
+	buf->tail[0].iov_len += pad;
+	buf->len += pad;
+
 	return 0;
 }
 
@@ -1922,7 +1934,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 			goto out_err;
 		break;
 	case RPC_GSS_SVC_PRIVACY:
-		stat = svcauth_gss_wrap_resp_priv(rqstp);
+		stat = svcauth_gss_wrap_priv(rqstp);
 		if (stat)
 			goto out_err;
 		break;

[PATCH v1 07/27] SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()

[Chuck Lever]

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

Match the error reporting in the other unwrap and wrap functions.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/trace/events/rpcgss.h     |   21 +++++++++++++++++++++
 net/sunrpc/auth_gss/svcauth_gss.c |   29 +++++++++++++++++++----------
 2 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/include/trace/events/rpcgss.h b/include/trace/events/rpcgss.h
index 261751ac241c..ba2d96a1bc2f 100644
--- a/include/trace/events/rpcgss.h
+++ b/include/trace/events/rpcgss.h
@@ -206,10 +206,31 @@ DECLARE_EVENT_CLASS(rpcgss_svc_gssapi_class,
 			),						\
 			TP_ARGS(rqstp, maj_stat))
 
+DEFINE_SVC_GSSAPI_EVENT(wrap);
 DEFINE_SVC_GSSAPI_EVENT(unwrap);
 DEFINE_SVC_GSSAPI_EVENT(mic);
 DEFINE_SVC_GSSAPI_EVENT(get_mic);
 
+TRACE_EVENT(rpcgss_svc_wrap_failed,
+	TP_PROTO(
+		const struct svc_rqst *rqstp
+	),
+
+	TP_ARGS(rqstp),
+
+	TP_STRUCT__entry(
+		__field(u32, xid)
+		__string(addr, rqstp->rq_xprt->xpt_remotebuf)
+	),
+
+	TP_fast_assign(
+		__entry->xid = be32_to_cpu(rqstp->rq_xid);
+		__assign_str(addr, rqstp->rq_xprt->xpt_remotebuf);
+	),
+
+	TP_printk("addr=%s xid=0x%08x", __get_str(addr), __entry->xid)
+);
+
 TRACE_EVENT(rpcgss_svc_unwrap_failed,
 	TP_PROTO(
 		const struct svc_rqst *rqstp
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index f0cd89f201bc..573b9d029709 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1813,7 +1813,9 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 
 bad_mic:
 	trace_rpcgss_svc_get_mic(rqstp, maj_stat);
+	return -EINVAL;
 wrap_failed:
+	trace_rpcgss_svc_wrap_failed(rqstp);
 	return -EINVAL;
 }
 
@@ -1834,18 +1836,16 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	struct gss_svc_data *gsd = rqstp->rq_auth_data;
 	struct rpc_gss_wire_cred *gc = &gsd->clcred;
 	struct xdr_buf *buf = &rqstp->rq_res;
-	struct page **inpages = NULL;
+	u32 offset, pad, maj_stat;
 	__be32 *p, *lenp;
-	int offset;
-	int pad;
 
 	p = svcauth_gss_prepare_to_wrap(buf, gsd);
 	if (p == NULL)
 		return 0;
+
 	lenp = p++;
 	offset = (u8 *)p - (u8 *)buf->head[0].iov_base;
 	*p++ = htonl(gc->gc_seq);
-	inpages = buf->pages;
 	/* XXX: Would be better to write some xdr helper functions for
 	 * nfs{2,3,4}xdr.c that place the data right, instead of copying: */
 
@@ -1859,12 +1859,12 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	if (buf->tail[0].iov_base) {
 		if (buf->tail[0].iov_base >=
 			buf->head[0].iov_base + PAGE_SIZE)
-			return -EINVAL;
+			goto wrap_failed;
 		if (buf->tail[0].iov_base < buf->head[0].iov_base)
-			return -EINVAL;
+			goto wrap_failed;
 		if (buf->tail[0].iov_len + buf->head[0].iov_len
 				+ 2 * RPC_MAX_AUTH_SIZE > PAGE_SIZE)
-			return -ENOMEM;
+			goto wrap_failed;
 		memmove(buf->tail[0].iov_base + RPC_MAX_AUTH_SIZE,
 			buf->tail[0].iov_base,
 			buf->tail[0].iov_len);
@@ -1879,13 +1879,16 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	 */
 	if (!buf->tail[0].iov_base) {
 		if (buf->head[0].iov_len + 2 * RPC_MAX_AUTH_SIZE > PAGE_SIZE)
-			return -ENOMEM;
+			goto wrap_failed;
 		buf->tail[0].iov_base = buf->head[0].iov_base
 			+ buf->head[0].iov_len + RPC_MAX_AUTH_SIZE;
 		buf->tail[0].iov_len = 0;
 	}
-	if (gss_wrap(gsd->rsci->mechctx, offset, buf, inpages))
-		return -ENOMEM;
+
+	maj_stat = gss_wrap(gsd->rsci->mechctx, offset, buf, buf->pages);
+	if (maj_stat != GSS_S_COMPLETE)
+		goto bad_wrap;
+
 	*lenp = htonl(buf->len - offset);
 	pad = 3 - ((buf->len - offset - 1) & 3);
 	p = (__be32 *)(buf->tail[0].iov_base + buf->tail[0].iov_len);
@@ -1894,6 +1897,12 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	buf->len += pad;
 
 	return 0;
+wrap_failed:
+	trace_rpcgss_svc_wrap_failed(rqstp);
+	return -EINVAL;
+bad_wrap:
+	trace_rpcgss_svc_wrap(rqstp, maj_stat);
+	return -ENOMEM;
 }
 
 /**

[PATCH v1 08/27] SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()

[Chuck Lever]

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

Simplify the references to the head and tail iovecs for readability.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 573b9d029709..cfcd74e6369d 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1836,6 +1836,8 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	struct gss_svc_data *gsd = rqstp->rq_auth_data;
 	struct rpc_gss_wire_cred *gc = &gsd->clcred;
 	struct xdr_buf *buf = &rqstp->rq_res;
+	struct kvec *head = buf->head;
+	struct kvec *tail = buf->tail;
 	u32 offset, pad, maj_stat;
 	__be32 *p, *lenp;
 
@@ -1844,7 +1846,7 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 		return 0;
 
 	lenp = p++;
-	offset = (u8 *)p - (u8 *)buf->head[0].iov_base;
+	offset = (u8 *)p - (u8 *)head->iov_base;
 	*p++ = htonl(gc->gc_seq);
 	/* XXX: Would be better to write some xdr helper functions for
 	 * nfs{2,3,4}xdr.c that place the data right, instead of copying: */
@@ -1856,19 +1858,17 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	 * there is RPC_MAX_AUTH_SIZE slack space available in
 	 * both the head and tail.
 	 */
-	if (buf->tail[0].iov_base) {
-		if (buf->tail[0].iov_base >=
-			buf->head[0].iov_base + PAGE_SIZE)
+	if (tail->iov_base) {
+		if (tail->iov_base >= head->iov_base + PAGE_SIZE)
 			goto wrap_failed;
-		if (buf->tail[0].iov_base < buf->head[0].iov_base)
+		if (tail->iov_base < head->iov_base)
 			goto wrap_failed;
-		if (buf->tail[0].iov_len + buf->head[0].iov_len
+		if (tail->iov_len + head->iov_len
 				+ 2 * RPC_MAX_AUTH_SIZE > PAGE_SIZE)
 			goto wrap_failed;
-		memmove(buf->tail[0].iov_base + RPC_MAX_AUTH_SIZE,
-			buf->tail[0].iov_base,
-			buf->tail[0].iov_len);
-		buf->tail[0].iov_base += RPC_MAX_AUTH_SIZE;
+		memmove(tail->iov_base + RPC_MAX_AUTH_SIZE, tail->iov_base,
+			tail->iov_len);
+		tail->iov_base += RPC_MAX_AUTH_SIZE;
 	}
 	/*
 	 * If there is no current tail data, make sure there is
@@ -1877,12 +1877,12 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	 * is RPC_MAX_AUTH_SIZE slack space available in both the
 	 * head and tail.
 	 */
-	if (!buf->tail[0].iov_base) {
-		if (buf->head[0].iov_len + 2 * RPC_MAX_AUTH_SIZE > PAGE_SIZE)
+	if (!tail->iov_base) {
+		if (head->iov_len + 2 * RPC_MAX_AUTH_SIZE > PAGE_SIZE)
 			goto wrap_failed;
-		buf->tail[0].iov_base = buf->head[0].iov_base
-			+ buf->head[0].iov_len + RPC_MAX_AUTH_SIZE;
-		buf->tail[0].iov_len = 0;
+		tail->iov_base = head->iov_base
+			+ head->iov_len + RPC_MAX_AUTH_SIZE;
+		tail->iov_len = 0;
 	}
 
 	maj_stat = gss_wrap(gsd->rsci->mechctx, offset, buf, buf->pages);
@@ -1891,9 +1891,9 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 
 	*lenp = htonl(buf->len - offset);
 	pad = 3 - ((buf->len - offset - 1) & 3);
-	p = (__be32 *)(buf->tail[0].iov_base + buf->tail[0].iov_len);
+	p = (__be32 *)(tail->iov_base + tail->iov_len);
 	memset(p, 0, pad);
-	buf->tail[0].iov_len += pad;
+	tail->iov_len += pad;
 	buf->len += pad;
 
 	return 0;

[PATCH v1 09/27] SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()

[Chuck Lever]

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

Actually xdr_stream does not add value here because of how
gss_wrap() works. This is just a clean-up patch.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index cfcd74e6369d..6c49750c0f7a 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1830,6 +1830,11 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
  *		unsigned int seq_num;
  *		proc_req_arg_t arg;
  *	};
+ *
+ * gss_wrap() expands the size of the RPC message payload in the
+ * response buffer. The main purpose of svcauth_gss_wrap_priv()
+ * is to ensure there is adequate space in the response buffer to
+ * avoid overflow during the wrap.
  */
 static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 {
@@ -1847,9 +1852,9 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 
 	lenp = p++;
 	offset = (u8 *)p - (u8 *)head->iov_base;
-	*p++ = htonl(gc->gc_seq);
-	/* XXX: Would be better to write some xdr helper functions for
-	 * nfs{2,3,4}xdr.c that place the data right, instead of copying: */
+	/* Buffer space for this field has already been reserved
+	 * in svcauth_gss_accept(). */
+	*p = cpu_to_be32(gc->gc_seq);
 
 	/*
 	 * If there is currently tail data, make sure there is
@@ -1889,8 +1894,8 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	if (maj_stat != GSS_S_COMPLETE)
 		goto bad_wrap;
 
-	*lenp = htonl(buf->len - offset);
-	pad = 3 - ((buf->len - offset - 1) & 3);
+	*lenp = cpu_to_be32(buf->len - offset);
+	pad = xdr_pad_size(buf->len - offset);
 	p = (__be32 *)(tail->iov_base + tail->iov_len);
 	memset(p, 0, pad);
 	tail->iov_len += pad;

[PATCH v1 10/27] SUNRPC: Check rq_auth_stat when preparing to wrap a response

[Chuck Lever]

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

Commit 5b304bc5bfcc ("[PATCH] knfsd: svcrpc: gss: fix failure on
SVC_DENIED in integrity case") added a check to prevent wrapping an
RPC response if reply_stat == MSG_DENIED, assuming that the only way
to get to svcauth_gss_release() with that reply_stat value was if
the reject_stat was AUTH_ERROR (reject_stat == MISMATCH is handled
earlier in svc_process_common()).

The code there is somewhat confusing. For one thing, rpc_success is
an accept_stat value, not a reply_stat value. The correct reply_stat
value to look for is RPC_MSG_DENIED. It happens to be the same value
as rpc_success, so it all works out, but it's not terribly readable.

Since commit 438623a06bac ("SUNRPC: Add svc_rqst::rq_auth_stat"),
the actual auth_stat value is stored in the svc_rqst, so that value
is now available to svcauth_gss_prepare_to_wrap() to make its
decision to wrap, based on direct information about the
authentication status of the RPC caller.

No behavior change is intended, this simply replaces some old code
with something that should be more self-documenting.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 6c49750c0f7a..71a147b0f90b 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1732,17 +1732,19 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 }
 
 static __be32 *
-svcauth_gss_prepare_to_wrap(struct xdr_buf *resbuf, struct gss_svc_data *gsd)
+svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)
 {
+	struct xdr_buf *resbuf = &rqstp->rq_res;
 	__be32 *p;
 	u32 verf_len;
 
 	p = gsd->verf_start;
 	gsd->verf_start = NULL;
 
-	/* If the reply stat is nonzero, don't wrap: */
-	if (*(p-1) != rpc_success)
+	/* AUTH_ERROR replies are not wrapped. */
+	if (rqstp->rq_auth_stat != rpc_auth_ok)
 		return NULL;
+
 	/* Skip the verifier: */
 	p += 1;
 	verf_len = ntohl(*p++);
@@ -1786,7 +1788,7 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 	u32 offset, len, maj_stat;
 	__be32 *p;
 
-	p = svcauth_gss_prepare_to_wrap(buf, gsd);
+	p = svcauth_gss_prepare_to_wrap(rqstp, gsd);
 	if (p == NULL)
 		goto out;
 
@@ -1846,7 +1848,7 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	u32 offset, pad, maj_stat;
 	__be32 *p, *lenp;
 
-	p = svcauth_gss_prepare_to_wrap(buf, gsd);
+	p = svcauth_gss_prepare_to_wrap(rqstp, gsd);
 	if (p == NULL)
 		return 0;
 

[PATCH v1 11/27] SUNRPC: Remove the rpc_stat variable in svc_process_common()

[Chuck Lever]

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

There's no RPC header field called rpc_stat; more precisely, the
variable appears to be recording an accept_stat value. But it looks
like we don't need to preserve this value at all, actually, so
simply remove the variable.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/svc.c |   24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 94f7efca60fc..489c5d1b67f9 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1232,12 +1232,9 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	struct svc_serv		*serv = rqstp->rq_server;
 	struct svc_process_info process;
 	__be32			*p, *statp;
-	__be32			rpc_stat;
 	int			auth_res, rc;
 	__be32			*reply_statp;
 
-	rpc_stat = rpc_success;
-
 	/* Will be turned off by GSS integrity and privacy services */
 	__set_bit(RQ_SPLICE_OK, &rqstp->rq_flags);
 	/* Will be turned off only when NFSv4 Sessions are used */
@@ -1279,10 +1276,9 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	case SVC_OK:
 		break;
 	case SVC_GARBAGE:
-		goto err_garbage;
+		goto err_garbage_args;
 	case SVC_SYSERR:
-		rpc_stat = rpc_system_err;
-		goto err_bad;
+		goto err_system_err;
 	case SVC_DENIED:
 		goto err_bad_auth;
 	case SVC_CLOSE:
@@ -1296,8 +1292,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	if (progp == NULL)
 		goto err_bad_prog;
 
-	rpc_stat = progp->pg_init_request(rqstp, progp, &process);
-	switch (rpc_stat) {
+	switch (progp->pg_init_request(rqstp, progp, &process)) {
 	case rpc_success:
 		break;
 	case rpc_prog_unavail:
@@ -1408,13 +1403,16 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	svc_putnl(resv, RPC_PROC_UNAVAIL);
 	goto sendit;
 
-err_garbage:
-	svc_printk(rqstp, "failed to decode args\n");
+err_garbage_args:
+	svc_printk(rqstp, "failed to decode RPC header\n");
+
+	serv->sv_stats->rpcbadfmt++;
+	svc_putnl(resv, RPC_GARBAGE_ARGS);
+	goto sendit;
 
-	rpc_stat = rpc_garbage_args;
-err_bad:
+err_system_err:
 	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, ntohl(rpc_stat));
+	svc_putnl(resv, RPC_SYSTEM_ERR);
 	goto sendit;
 }
 

[PATCH v1 12/27] SUNRPC: Add XDR encoding helper for opaque_auth

[Chuck Lever]

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

RFC 5531 defines an MSG_ACCEPTED Reply message like this:

	struct accepted_reply {
		opaque_auth verf;
		union switch (accept_stat stat) {
		case SUCCESS:
		   ...

In the current server code, struct opaque_auth encoding is open-
coded. Introduce a helper that encodes an opaque_auth data item
within the context of a xdr_stream.

Done as part of hardening the server-side RPC header decoding and
encoding paths.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/xdr.h |    2 ++
 net/sunrpc/xdr.c           |   29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index 884df67009f4..f3b6eb9accd7 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -348,6 +348,8 @@ ssize_t xdr_stream_decode_string_dup(struct xdr_stream *xdr, char **str,
 		size_t maxlen, gfp_t gfp_flags);
 ssize_t xdr_stream_decode_opaque_auth(struct xdr_stream *xdr, u32 *flavor,
 		void **body, unsigned int *body_len);
+ssize_t xdr_stream_encode_opaque_auth(struct xdr_stream *xdr, u32 flavor,
+		void *body, unsigned int body_len);
 
 /**
  * xdr_align_size - Calculate padded size of an object
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 56d87c784c9e..6b2ec24ec62d 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -2310,3 +2310,32 @@ ssize_t xdr_stream_decode_opaque_auth(struct xdr_stream *xdr, u32 *flavor,
 	return len + ret;
 }
 EXPORT_SYMBOL_GPL(xdr_stream_decode_opaque_auth);
+
+/**
+ * xdr_stream_encode_opaque_auth - Encode struct opaque_auth (RFC5531 S8.2)
+ * @xdr: pointer to xdr_stream
+ * @flavor: verifier flavor to encode
+ * @body: content of body to encode
+ * @body_len: length of body to encode
+ *
+ * Return values:
+ *   On success, returns length in bytes of XDR buffer consumed
+ *   %-EBADMSG on XDR buffer overflow
+ *   %-EMSGSIZE if the size of @body exceeds 400 octets
+ */
+ssize_t xdr_stream_encode_opaque_auth(struct xdr_stream *xdr, u32 flavor,
+				      void *body, unsigned int body_len)
+{
+	ssize_t ret, len;
+
+	if (unlikely(body_len > RPC_MAX_AUTH_SIZE))
+		return -EMSGSIZE;
+	len = xdr_stream_encode_u32(xdr, flavor);
+	if (unlikely(len < 0))
+		return len;
+	ret = xdr_stream_encode_opaque(xdr, body, body_len);
+	if (unlikely(ret < 0))
+		return ret;
+	return len + ret;
+}
+EXPORT_SYMBOL_GPL(xdr_stream_encode_opaque_auth);

[PATCH v1 13/27] SUNRPC: Push svcxdr_init_encode() into svc_process_common()

[Chuck Lever]

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

Now that all vs_dispatch functions invoke svcxdr_init_encode(), it
is common code and can be pushed down into the generic RPC server.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/lockd/svc.c             |    1 -
 fs/nfs/callback_xdr.c      |    2 --
 fs/nfsd/nfscache.c         |    2 +-
 fs/nfsd/nfssvc.c           |    6 ------
 include/linux/sunrpc/xdr.h |   21 +++++++++++++++++++++
 net/sunrpc/svc.c           |    1 +
 6 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index e56d85335599..642e394e7a2d 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -704,7 +704,6 @@ static int nlmsvc_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 	if (*statp != rpc_success)
 		return 1;
 
-	svcxdr_init_encode(rqstp);
 	if (!procp->pc_encode(rqstp, &rqstp->rq_res_stream))
 		goto out_encode_err;
 
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index 46d3f5986b4e..b4c3b7182198 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -984,8 +984,6 @@ nfs_callback_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 {
 	const struct svc_procedure *procp = rqstp->rq_procinfo;
 
-	svcxdr_init_encode(rqstp);
-
 	*statp = procp->pc_func(rqstp);
 	return 1;
 }
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index 3e64a3d50a1c..ef5ee548053b 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -488,7 +488,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp)
 	case RC_NOCACHE:
 		break;
 	case RC_REPLSTAT:
-		svc_putu32(&rqstp->rq_res.head[0], rp->c_replstat);
+		xdr_stream_encode_be32(&rqstp->rq_res_stream, rp->c_replstat);
 		rtn = RC_REPLY;
 		break;
 	case RC_REPLBUFF:
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 1ed29eac80ed..dfa8ee6c04d5 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -1052,12 +1052,6 @@ int nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 		goto out_dropit;
 	}
 
-	/*
-	 * Need to grab the location to store the status, as
-	 * NFSv4 does some encoding while processing
-	 */
-	svcxdr_init_encode(rqstp);
-
 	*statp = proc->pc_func(rqstp);
 	if (test_bit(RQ_DROPME, &rqstp->rq_flags))
 		goto out_update_drop;
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index f3b6eb9accd7..72014c9216fc 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -474,6 +474,27 @@ xdr_stream_encode_u32(struct xdr_stream *xdr, __u32 n)
 	return len;
 }
 
+/**
+ * xdr_stream_encode_be32 - Encode a big-endian 32-bit integer
+ * @xdr: pointer to xdr_stream
+ * @n: integer to encode
+ *
+ * Return values:
+ *   On success, returns length in bytes of XDR buffer consumed
+ *   %-EMSGSIZE on XDR buffer overflow
+ */
+static inline ssize_t
+xdr_stream_encode_be32(struct xdr_stream *xdr, __be32 n)
+{
+	const size_t len = sizeof(n);
+	__be32 *p = xdr_reserve_space(xdr, len);
+
+	if (unlikely(!p))
+		return -EMSGSIZE;
+	*p = n;
+	return len;
+}
+
 /**
  * xdr_stream_encode_u64 - Encode a 64-bit integer
  * @xdr: pointer to xdr_stream
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 489c5d1b67f9..1cdf68fda3f8 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1321,6 +1321,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	 */
 	if (procp->pc_xdrressize)
 		svc_reserve_auth(rqstp, procp->pc_xdrressize<<2);
+	svcxdr_init_encode(rqstp);
 
 	/* Call the function that processes the request. */
 	rc = process.dispatch(rqstp, statp);

[PATCH v1 14/27] SUNRPC: Move svcxdr_init_encode() into ->accept methods

[Chuck Lever]

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

Refactor: So that the overhaul of each ->accept method can be done
in separate smaller patches, temporarily move the
svcxdr_init_encode() call into those methods.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |    5 +++++
 net/sunrpc/svc.c                  |    6 ++----
 net/sunrpc/svcauth_unix.c         |    3 +++
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 71a147b0f90b..759169baa52f 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1250,6 +1250,7 @@ svcauth_gss_legacy_init(struct svc_rqst *rqstp,
 		goto out;
 
 	ret = SVC_COMPLETE;
+	svcxdr_init_encode(rqstp);
 out:
 	cache_put(&rsip->h, sn->rsi_cache);
 	return ret;
@@ -1378,6 +1379,7 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,
 		goto out;
 
 	ret = SVC_COMPLETE;
+	svcxdr_init_encode(rqstp);
 out:
 	gss_free_in_token_pages(&ud.in_token);
 	gssp_free_upcall_data(&ud);
@@ -1680,6 +1682,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 		rqstp->rq_auth_stat = rpc_autherr_badcred;
 		switch (gc->gc_svc) {
 		case RPC_GSS_SVC_NONE:
+			svcxdr_init_encode(rqstp);
 			break;
 		case RPC_GSS_SVC_INTEGRITY:
 			/* placeholders for length and seq. number: */
@@ -1689,6 +1692,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 						     rsci->mechctx))
 				goto garbage_args;
 			rqstp->rq_auth_slack = RPC_MAX_AUTH_SIZE;
+			svcxdr_init_encode(rqstp);
 			break;
 		case RPC_GSS_SVC_PRIVACY:
 			/* placeholders for length and seq. number: */
@@ -1698,6 +1702,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 						    rsci->mechctx))
 				goto garbage_args;
 			rqstp->rq_auth_slack = RPC_MAX_AUTH_SIZE * 2;
+			svcxdr_init_encode(rqstp);
 			break;
 		default:
 			goto auth_err;
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 1cdf68fda3f8..9951311790bf 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1312,16 +1312,14 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	serv->sv_stats->rpccnt++;
 	trace_svc_process(rqstp, progp->pg_name);
 
-	/* Build the reply header. */
-	statp = resv->iov_base +resv->iov_len;
-	svc_putnl(resv, RPC_SUCCESS);
+	statp = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT);
+	*statp = rpc_success;
 
 	/* un-reserve some of the out-queue now that we have a
 	 * better idea of reply size
 	 */
 	if (procp->pc_xdrressize)
 		svc_reserve_auth(rqstp, procp->pc_xdrressize<<2);
-	svcxdr_init_encode(rqstp);
 
 	/* Call the function that processes the request. */
 	rc = process.dispatch(rqstp, statp);
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index f09a148aa0c1..6281d23f98bf 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -778,6 +778,7 @@ svcauth_null_accept(struct svc_rqst *rqstp)
 	svc_putnl(resv, 0);
 
 	rqstp->rq_cred.cr_flavor = RPC_AUTH_NULL;
+	svcxdr_init_encode(rqstp);
 	return SVC_OK;
 }
 
@@ -865,6 +866,7 @@ svcauth_tls_accept(struct svc_rqst *rqstp)
 		svc_putnl(resv, 0);
 
 	rqstp->rq_cred.cr_flavor = RPC_AUTH_TLS;
+	svcxdr_init_encode(rqstp);
 	return SVC_OK;
 }
 
@@ -960,6 +962,7 @@ svcauth_unix_accept(struct svc_rqst *rqstp)
 	svc_putnl(resv, 0);
 
 	rqstp->rq_cred.cr_flavor = RPC_AUTH_UNIX;
+	svcxdr_init_encode(rqstp);
 	return SVC_OK;
 
 badcred:

[PATCH v1 15/27] SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()

[Chuck Lever]

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

Done as part of hardening the server-side RPC header encoding path.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/svcauth_unix.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 6281d23f98bf..b24d6c75588f 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -744,7 +744,6 @@ EXPORT_SYMBOL_GPL(svcauth_unix_set_client);
 static int
 svcauth_null_accept(struct svc_rqst *rqstp)
 {
-	struct kvec	*resv = &rqstp->rq_res.head[0];
 	struct xdr_stream *xdr = &rqstp->rq_arg_stream;
 	struct svc_cred	*cred = &rqstp->rq_cred;
 	u32 flavor, len;
@@ -773,12 +772,12 @@ svcauth_null_accept(struct svc_rqst *rqstp)
 	if (cred->cr_group_info == NULL)
 		return SVC_CLOSE; /* kmalloc failure - client must retry */
 
-	/* Put NULL verifier */
-	svc_putnl(resv, RPC_AUTH_NULL);
-	svc_putnl(resv, 0);
+	svcxdr_init_encode(rqstp);
+	if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
+					  RPC_AUTH_NULL, NULL, 0) < 0)
+		return SVC_CLOSE;
 
 	rqstp->rq_cred.cr_flavor = RPC_AUTH_NULL;
-	svcxdr_init_encode(rqstp);
 	return SVC_OK;
 }
 

[PATCH v1 16/27] SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()

[Chuck Lever]

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

Done as part of hardening the server-side RPC header encoding path.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/svcauth_unix.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index b24d6c75588f..e3981e124042 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -894,7 +894,6 @@ struct auth_ops svcauth_tls = {
 static int
 svcauth_unix_accept(struct svc_rqst *rqstp)
 {
-	struct kvec	*resv = &rqstp->rq_res.head[0];
 	struct xdr_stream *xdr = &rqstp->rq_arg_stream;
 	struct svc_cred	*cred = &rqstp->rq_cred;
 	struct user_namespace *userns;
@@ -956,12 +955,12 @@ svcauth_unix_accept(struct svc_rqst *rqstp)
 		return SVC_DENIED;
 	}
 
-	/* Put NULL verifier */
-	svc_putnl(resv, RPC_AUTH_NULL);
-	svc_putnl(resv, 0);
+	svcxdr_init_encode(rqstp);
+	if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
+					  RPC_AUTH_NULL, NULL, 0) < 0)
+		return SVC_CLOSE;
 
 	rqstp->rq_cred.cr_flavor = RPC_AUTH_UNIX;
-	svcxdr_init_encode(rqstp);
 	return SVC_OK;
 
 badcred:

[PATCH v1 17/27] SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()

[Chuck Lever]

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

Done as part of hardening the server-side RPC header encoding path.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/svcauth_unix.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index e3981e124042..632150a6b947 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -822,9 +822,9 @@ svcauth_tls_accept(struct svc_rqst *rqstp)
 {
 	struct xdr_stream *xdr = &rqstp->rq_arg_stream;
 	struct svc_cred	*cred = &rqstp->rq_cred;
-	struct kvec *resv = rqstp->rq_res.head;
 	u32 flavor, len;
 	void *body;
+	__be32 *p;
 
 	/* Length of Call's credential body field: */
 	if (xdr_stream_decode_u32(xdr, &len) < 0)
@@ -855,17 +855,21 @@ svcauth_tls_accept(struct svc_rqst *rqstp)
 	if (cred->cr_group_info == NULL)
 		return SVC_CLOSE;
 
-	/* Reply's verifier */
-	svc_putnl(resv, RPC_AUTH_NULL);
+	svcxdr_init_encode(rqstp);
 	if (rqstp->rq_xprt->xpt_ops->xpo_start_tls) {
-		svc_putnl(resv, 8);
-		memcpy(resv->iov_base + resv->iov_len, "STARTTLS", 8);
-		resv->iov_len += 8;
-	} else
-		svc_putnl(resv, 0);
+		p = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT * 2 + 8);
+		if (!p)
+			return SVC_CLOSE;
+		*p++ = rpc_auth_null;
+		*p++ = cpu_to_be32(8);
+		memcpy(p, "STARTTLS", 8);
+	} else {
+		if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
+						  RPC_AUTH_NULL, NULL, 0) < 0)
+			return SVC_CLOSE;
+	}
 
 	rqstp->rq_cred.cr_flavor = RPC_AUTH_TLS;
-	svcxdr_init_encode(rqstp);
 	return SVC_OK;
 }
 

[PATCH v1 18/27] SUNRPC: Convert unwrap data paths to use xdr_stream for replies

[Chuck Lever]

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

We're now moving svcxdr_init_encode() to /before/ the flavor's
->accept method has set rq_auth_slack. Add a helper that can
set rq_auth_slack /after/ svcxdr_init_encode() has been called.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/svc.h        |   26 ++++++++++++++++++++++++--
 net/sunrpc/auth_gss/svcauth_gss.c |   18 ++++++++----------
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index ed722dd33b46..dd9f68acd9f1 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -577,12 +577,34 @@ static inline void svcxdr_init_encode(struct svc_rqst *rqstp)
 	xdr->buf = buf;
 	xdr->iov = resv;
 	xdr->p   = resv->iov_base + resv->iov_len;
-	xdr->end = resv->iov_base + PAGE_SIZE - rqstp->rq_auth_slack;
+	xdr->end = resv->iov_base + PAGE_SIZE;
 	buf->len = resv->iov_len;
 	xdr->page_ptr = buf->pages - 1;
 	buf->buflen = PAGE_SIZE * (rqstp->rq_page_end - buf->pages);
-	buf->buflen -= rqstp->rq_auth_slack;
 	xdr->rqst = NULL;
 }
 
+/**
+ * svcxdr_set_auth_slack -
+ * @rqstp: RPC transaction
+ * @slack: buffer space to reserve for the transaction's security flavor
+ *
+ * Set the request's slack space requirement, and set aside that much
+ * space in the rqstp's rq_res.head for use when the auth wraps the Reply.
+ */
+static inline void svcxdr_set_auth_slack(struct svc_rqst *rqstp, int slack)
+{
+	struct xdr_stream *xdr = &rqstp->rq_res_stream;
+	struct xdr_buf *buf = &rqstp->rq_res;
+	struct kvec *resv = buf->head;
+
+	rqstp->rq_auth_slack = slack;
+
+	xdr->end -= XDR_QUADLEN(slack);
+	buf->buflen -= rqstp->rq_auth_slack;
+
+	WARN_ON(xdr->iov != resv);
+	WARN_ON(xdr->p > xdr->end);
+}
+
 #endif /* SUNRPC_SVC_H */
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 759169baa52f..fd1fd4143a8e 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1685,24 +1685,22 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 			svcxdr_init_encode(rqstp);
 			break;
 		case RPC_GSS_SVC_INTEGRITY:
-			/* placeholders for length and seq. number: */
-			svc_putnl(resv, 0);
-			svc_putnl(resv, 0);
+			svcxdr_init_encode(rqstp);
+			/* placeholders for body length and seq. number: */
+			xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT * 2);
 			if (svcauth_gss_unwrap_integ(rqstp, gc->gc_seq,
 						     rsci->mechctx))
 				goto garbage_args;
-			rqstp->rq_auth_slack = RPC_MAX_AUTH_SIZE;
-			svcxdr_init_encode(rqstp);
+			svcxdr_set_auth_slack(rqstp, RPC_MAX_AUTH_SIZE);
 			break;
 		case RPC_GSS_SVC_PRIVACY:
-			/* placeholders for length and seq. number: */
-			svc_putnl(resv, 0);
-			svc_putnl(resv, 0);
+			svcxdr_init_encode(rqstp);
+			/* placeholders for body length and seq. number: */
+			xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT * 2);
 			if (svcauth_gss_unwrap_priv(rqstp, gc->gc_seq,
 						    rsci->mechctx))
 				goto garbage_args;
-			rqstp->rq_auth_slack = RPC_MAX_AUTH_SIZE * 2;
-			svcxdr_init_encode(rqstp);
+			svcxdr_set_auth_slack(rqstp, RPC_MAX_AUTH_SIZE * 2);
 			break;
 		default:
 			goto auth_err;

[PATCH v1 19/27] SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers

[Chuck Lever]

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

This code constructs replies to the decorated NULL procedure calls
that establish GSS contexts. Convert this code path to use struct
xdr_stream to encode such responses.

Done as part of hardening the server-side RPC header encoding path.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |  144 +++++++++++++++++++++++--------------
 1 file changed, 90 insertions(+), 54 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index fd1fd4143a8e..9f3633c42ebd 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -77,6 +77,7 @@ struct gss_svc_data {
 	struct rsc			*rsci;
 
 	/* for temporary results */
+	__be32				gsd_seq_num;
 	u8				gsd_scratch[GSS_SCRATCH_SIZE];
 };
 
@@ -771,20 +772,6 @@ svcauth_gss_verify_header(struct svc_rqst *rqstp, struct rsc *rsci,
 	return SVC_OK;
 }
 
-static int
-gss_write_null_verf(struct svc_rqst *rqstp)
-{
-	__be32     *p;
-
-	svc_putnl(rqstp->rq_res.head, RPC_AUTH_NULL);
-	p = rqstp->rq_res.head->iov_base + rqstp->rq_res.head->iov_len;
-	/* don't really need to check if head->iov_len > PAGE_SIZE ... */
-	*p++ = 0;
-	if (!xdr_ressize_check(rqstp, p))
-		return -1;
-	return 0;
-}
-
 static int
 gss_write_verf(struct svc_rqst *rqstp, struct gss_ctx *ctx_id, u32 seq)
 {
@@ -821,6 +808,38 @@ gss_write_verf(struct svc_rqst *rqstp, struct gss_ctx *ctx_id, u32 seq)
 	return err;
 }
 
+/*
+ * Construct and encode a Reply's verifier field. The verifier's body
+ * field contains a variable-length checksum of the GSS sequence
+ * number.
+ */
+static bool
+svcauth_gss_encode_verf(struct svc_rqst *rqstp, struct gss_ctx *ctx_id, u32 seq)
+{
+	struct gss_svc_data	*gsd = rqstp->rq_auth_data;
+	u32			maj_stat;
+	struct xdr_buf		verf_data;
+	struct xdr_netobj	checksum;
+	struct kvec		iov;
+
+	gsd->gsd_seq_num = cpu_to_be32(seq);
+	iov.iov_base = &gsd->gsd_seq_num;
+	iov.iov_len = XDR_UNIT;
+	xdr_buf_from_iov(&iov, &verf_data);
+
+	checksum.data = gsd->gsd_scratch;
+	maj_stat = gss_get_mic(ctx_id, &verf_data, &checksum);
+	if (maj_stat != GSS_S_COMPLETE)
+		goto bad_mic;
+
+	return xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream, RPC_AUTH_GSS,
+					     checksum.data, checksum.len) > 0;
+
+bad_mic:
+	trace_rpcgss_svc_get_mic(rqstp, maj_stat);
+	return false;
+}
+
 struct gss_domain {
 	struct auth_domain	h;
 	u32			pseudoflavor;
@@ -1057,23 +1076,29 @@ svcauth_gss_set_client(struct svc_rqst *rqstp)
 	return SVC_OK;
 }
 
-static inline int
-gss_write_init_verf(struct cache_detail *cd, struct svc_rqst *rqstp,
-		struct xdr_netobj *out_handle, int *major_status)
+static bool
+svcauth_gss_proc_init_verf(struct cache_detail *cd, struct svc_rqst *rqstp,
+			   struct xdr_netobj *out_handle, int *major_status,
+			   u32 seq_num)
 {
+	struct xdr_stream *xdr = &rqstp->rq_res_stream;
 	struct rsc *rsci;
-	int        rc;
+	bool rc;
 
 	if (*major_status != GSS_S_COMPLETE)
-		return gss_write_null_verf(rqstp);
+		goto null_verifier;
 	rsci = gss_svc_searchbyctx(cd, out_handle);
 	if (rsci == NULL) {
 		*major_status = GSS_S_NO_CONTEXT;
-		return gss_write_null_verf(rqstp);
+		goto null_verifier;
 	}
-	rc = gss_write_verf(rqstp, rsci->mechctx, GSS_SEQ_WIN);
+
+	rc = svcauth_gss_encode_verf(rqstp, rsci->mechctx, seq_num);
 	cache_put(&rsci->h, cd);
 	return rc;
+
+null_verifier:
+	return xdr_stream_encode_opaque_auth(xdr, RPC_AUTH_NULL, NULL, 0) > 0;
 }
 
 static void gss_free_in_token_pages(struct gssp_in_token *in_token)
@@ -1163,24 +1188,35 @@ static int gss_read_proxy_verf(struct svc_rqst *rqstp,
 	return SVC_DENIED;
 }
 
-static inline int
-gss_write_resv(struct kvec *resv, size_t size_limit,
-	       struct xdr_netobj *out_handle, struct xdr_netobj *out_token,
-	       int major_status, int minor_status)
+/*
+ * RFC 2203, Section 5.2.3.1.
+ *
+ *	struct rpc_gss_init_res {
+ *		opaque handle<>;
+ *		unsigned int gss_major;
+ *		unsigned int gss_minor;
+ *		unsigned int seq_window;
+ *		opaque gss_token<>;
+ *	};
+ */
+static bool
+svcxdr_encode_gss_init_res(struct xdr_stream *xdr,
+			   struct xdr_netobj *handle,
+			   struct xdr_netobj *gss_token,
+			   unsigned int major_status,
+			   unsigned int minor_status, u32 seq_num)
 {
-	if (resv->iov_len + 4 > size_limit)
-		return -1;
-	svc_putnl(resv, RPC_SUCCESS);
-	if (svc_safe_putnetobj(resv, out_handle))
-		return -1;
-	if (resv->iov_len + 3 * 4 > size_limit)
-		return -1;
-	svc_putnl(resv, major_status);
-	svc_putnl(resv, minor_status);
-	svc_putnl(resv, GSS_SEQ_WIN);
-	if (svc_safe_putnetobj(resv, out_token))
-		return -1;
-	return 0;
+	if (xdr_stream_encode_opaque(xdr, handle->data, handle->len) < 0)
+		return false;
+	if (xdr_stream_encode_u32(xdr, major_status) < 0)
+		return false;
+	if (xdr_stream_encode_u32(xdr, minor_status) < 0)
+		return false;
+	if (xdr_stream_encode_u32(xdr, seq_num) < 0)
+		return false;
+	if (xdr_stream_encode_opaque(xdr, gss_token->data, gss_token->len) < 0)
+		return false;
+	return true;
 }
 
 /*
@@ -1195,7 +1231,6 @@ svcauth_gss_legacy_init(struct svc_rqst *rqstp,
 			struct rpc_gss_wire_cred *gc)
 {
 	struct xdr_stream *xdr = &rqstp->rq_arg_stream;
-	struct kvec *resv = &rqstp->rq_res.head[0];
 	struct rsi *rsip, rsikey;
 	__be32 *p;
 	u32 len;
@@ -1240,17 +1275,17 @@ svcauth_gss_legacy_init(struct svc_rqst *rqstp,
 		return SVC_CLOSE;
 
 	ret = SVC_CLOSE;
-	/* Got an answer to the upcall; use it: */
-	if (gss_write_init_verf(sn->rsc_cache, rqstp,
-				&rsip->out_handle, &rsip->major_status))
+	if (!svcauth_gss_proc_init_verf(sn->rsc_cache, rqstp, &rsip->out_handle,
+					&rsip->major_status, GSS_SEQ_WIN))
+		goto out;
+	if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
 		goto out;
-	if (gss_write_resv(resv, PAGE_SIZE,
-			   &rsip->out_handle, &rsip->out_token,
-			   rsip->major_status, rsip->minor_status))
+	if (!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream, &rsip->out_handle,
+					&rsip->out_token, rsip->major_status,
+					rsip->minor_status, GSS_SEQ_WIN))
 		goto out;
 
 	ret = SVC_COMPLETE;
-	svcxdr_init_encode(rqstp);
 out:
 	cache_put(&rsip->h, sn->rsi_cache);
 	return ret;
@@ -1331,7 +1366,6 @@ static int gss_proxy_save_rsc(struct cache_detail *cd,
 static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,
 				  struct rpc_gss_wire_cred *gc)
 {
-	struct kvec *resv = &rqstp->rq_res.head[0];
 	struct xdr_netobj cli_handle;
 	struct gssp_upcall_data ud;
 	uint64_t handle;
@@ -1369,17 +1403,17 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,
 		goto out;
 	}
 
-	/* Got an answer to the upcall; use it: */
-	if (gss_write_init_verf(sn->rsc_cache, rqstp,
-				&cli_handle, &ud.major_status))
+	if (!svcauth_gss_proc_init_verf(sn->rsc_cache, rqstp, &cli_handle,
+					&ud.major_status, GSS_SEQ_WIN))
+		goto out;
+	if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
 		goto out;
-	if (gss_write_resv(resv, PAGE_SIZE,
-			   &cli_handle, &ud.out_token,
-			   ud.major_status, ud.minor_status))
+	if (!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream, &cli_handle,
+					&ud.out_token, ud.major_status,
+					ud.minor_status, GSS_SEQ_WIN))
 		goto out;
 
 	ret = SVC_COMPLETE;
-	svcxdr_init_encode(rqstp);
 out:
 	gss_free_in_token_pages(&ud.in_token);
 	gssp_free_upcall_data(&ud);
@@ -1420,6 +1454,8 @@ svcauth_gss_proc_init(struct svc_rqst *rqstp, struct rpc_gss_wire_cred *gc)
 	u32 flavor, len;
 	void *body;
 
+	svcxdr_init_encode(rqstp);
+
 	/* Call's verf field: */
 	if (xdr_stream_decode_opaque_auth(xdr, &flavor, &body, &len) < 0)
 		return SVC_GARBAGE;

[PATCH v1 20/27] SUNRPC: Use xdr_stream for encoding GSS reply verifiers

[Chuck Lever]

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

Done as part of hardening the server-side RPC header encoding path.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   81 ++++---------------------------------
 1 file changed, 8 insertions(+), 73 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 9f3633c42ebd..89333669af26 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -693,28 +693,6 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
 	goto out;
 }
 
-static inline u32 round_up_to_quad(u32 i)
-{
-	return (i + 3 ) & ~3;
-}
-
-static inline int
-svc_safe_putnetobj(struct kvec *resv, struct xdr_netobj *o)
-{
-	u8 *p;
-
-	if (resv->iov_len + 4 > PAGE_SIZE)
-		return -1;
-	svc_putnl(resv, o->len);
-	p = resv->iov_base + resv->iov_len;
-	resv->iov_len += round_up_to_quad(o->len);
-	if (resv->iov_len > PAGE_SIZE)
-		return -1;
-	memcpy(p, o->data, o->len);
-	memset(p + o->len, 0, round_up_to_quad(o->len) - o->len);
-	return 0;
-}
-
 /*
  * Decode and verify a Call's verifier field. For RPC_AUTH_GSS Calls,
  * the body of this field contains a variable length checksum.
@@ -772,42 +750,6 @@ svcauth_gss_verify_header(struct svc_rqst *rqstp, struct rsc *rsci,
 	return SVC_OK;
 }
 
-static int
-gss_write_verf(struct svc_rqst *rqstp, struct gss_ctx *ctx_id, u32 seq)
-{
-	__be32			*xdr_seq;
-	u32			maj_stat;
-	struct xdr_buf		verf_data;
-	struct xdr_netobj	mic;
-	__be32			*p;
-	struct kvec		iov;
-	int err = -1;
-
-	svc_putnl(rqstp->rq_res.head, RPC_AUTH_GSS);
-	xdr_seq = kmalloc(4, GFP_KERNEL);
-	if (!xdr_seq)
-		return -ENOMEM;
-	*xdr_seq = htonl(seq);
-
-	iov.iov_base = xdr_seq;
-	iov.iov_len = 4;
-	xdr_buf_from_iov(&iov, &verf_data);
-	p = rqstp->rq_res.head->iov_base + rqstp->rq_res.head->iov_len;
-	mic.data = (u8 *)(p + 1);
-	maj_stat = gss_get_mic(ctx_id, &verf_data, &mic);
-	if (maj_stat != GSS_S_COMPLETE)
-		goto out;
-	*p++ = htonl(mic.len);
-	memset((u8 *)p + mic.len, 0, round_up_to_quad(mic.len) - mic.len);
-	p += XDR_QUADLEN(mic.len);
-	if (!xdr_ressize_check(rqstp, p))
-		goto out;
-	err = 0;
-out:
-	kfree(xdr_seq);
-	return err;
-}
-
 /*
  * Construct and encode a Reply's verifier field. The verifier's body
  * field contains a variable-length checksum of the GSS sequence
@@ -1454,8 +1396,6 @@ svcauth_gss_proc_init(struct svc_rqst *rqstp, struct rpc_gss_wire_cred *gc)
 	u32 flavor, len;
 	void *body;
 
-	svcxdr_init_encode(rqstp);
-
 	/* Call's verf field: */
 	if (xdr_stream_decode_opaque_auth(xdr, &flavor, &body, &len) < 0)
 		return SVC_GARBAGE;
@@ -1642,15 +1582,15 @@ svcauth_gss_decode_credbody(struct xdr_stream *xdr,
 static int
 svcauth_gss_accept(struct svc_rqst *rqstp)
 {
-	struct kvec	*resv = &rqstp->rq_res.head[0];
 	struct gss_svc_data *svcdata = rqstp->rq_auth_data;
 	__be32		*rpcstart;
 	struct rpc_gss_wire_cred *gc;
 	struct rsc	*rsci = NULL;
-	__be32		*reject_stat = resv->iov_base + resv->iov_len;
 	int		ret;
 	struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
 
+	svcxdr_init_encode(rqstp);
+
 	rqstp->rq_auth_stat = rpc_autherr_badcred;
 	if (!svcdata)
 		svcdata = kmalloc(sizeof(*svcdata), GFP_KERNEL);
@@ -1700,28 +1640,25 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 	/* now act upon the command: */
 	switch (gc->gc_proc) {
 	case RPC_GSS_PROC_DESTROY:
-		if (gss_write_verf(rqstp, rsci->mechctx, gc->gc_seq))
+		if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
 			goto auth_err;
 		/* Delete the entry from the cache_list and call cache_put */
 		sunrpc_cache_unhash(sn->rsc_cache, &rsci->h);
-		if (resv->iov_len + 4 > PAGE_SIZE)
-			goto drop;
-		svc_putnl(resv, RPC_SUCCESS);
+		if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
+			goto auth_err;
 		goto complete;
 	case RPC_GSS_PROC_DATA:
 		rqstp->rq_auth_stat = rpcsec_gsserr_ctxproblem;
-		svcdata->verf_start = resv->iov_base + resv->iov_len;
-		if (gss_write_verf(rqstp, rsci->mechctx, gc->gc_seq))
+		svcdata->verf_start = xdr_reserve_space(&rqstp->rq_res_stream, 0);
+		if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
 			goto auth_err;
 		rqstp->rq_cred = rsci->cred;
 		get_group_info(rsci->cred.cr_group_info);
 		rqstp->rq_auth_stat = rpc_autherr_badcred;
 		switch (gc->gc_svc) {
 		case RPC_GSS_SVC_NONE:
-			svcxdr_init_encode(rqstp);
 			break;
 		case RPC_GSS_SVC_INTEGRITY:
-			svcxdr_init_encode(rqstp);
 			/* placeholders for body length and seq. number: */
 			xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT * 2);
 			if (svcauth_gss_unwrap_integ(rqstp, gc->gc_seq,
@@ -1730,7 +1667,6 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 			svcxdr_set_auth_slack(rqstp, RPC_MAX_AUTH_SIZE);
 			break;
 		case RPC_GSS_SVC_PRIVACY:
-			svcxdr_init_encode(rqstp);
 			/* placeholders for body length and seq. number: */
 			xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT * 2);
 			if (svcauth_gss_unwrap_priv(rqstp, gc->gc_seq,
@@ -1755,8 +1691,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 	ret = SVC_GARBAGE;
 	goto out;
 auth_err:
-	/* Restore write pointer to its original value: */
-	xdr_ressize_check(rqstp, reject_stat);
+	xdr_truncate_encode(&rqstp->rq_res_stream, XDR_UNIT * 2);
 	ret = SVC_DENIED;
 	goto out;
 complete:

[PATCH v1 21/27] SUNRPC: Hoist init_encode out of svc_authenticate()

[Chuck Lever]

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

Now that each ->accept method has been converted, the
svcxdr_init_encode() calls can be hoisted back up into the generic
RPC server code.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |    2 --
 net/sunrpc/svc.c                  |    2 ++
 net/sunrpc/svcauth_unix.c         |    3 ---
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 89333669af26..560fb8a2803d 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1589,8 +1589,6 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 	int		ret;
 	struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
 
-	svcxdr_init_encode(rqstp);
-
 	rqstp->rq_auth_stat = rpc_autherr_badcred;
 	if (!svcdata)
 		svcdata = kmalloc(sizeof(*svcdata), GFP_KERNEL);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 9951311790bf..393eebd1f6fe 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1262,6 +1262,8 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 		if (rqstp->rq_prog == progp->pg_prog)
 			break;
 
+	svcxdr_init_encode(rqstp);
+
 	/*
 	 * Decode auth data, and add verifier to reply buffer.
 	 * We do this before anything else in order to get a decent
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 632150a6b947..b101700d155c 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -772,7 +772,6 @@ svcauth_null_accept(struct svc_rqst *rqstp)
 	if (cred->cr_group_info == NULL)
 		return SVC_CLOSE; /* kmalloc failure - client must retry */
 
-	svcxdr_init_encode(rqstp);
 	if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
 					  RPC_AUTH_NULL, NULL, 0) < 0)
 		return SVC_CLOSE;
@@ -855,7 +854,6 @@ svcauth_tls_accept(struct svc_rqst *rqstp)
 	if (cred->cr_group_info == NULL)
 		return SVC_CLOSE;
 
-	svcxdr_init_encode(rqstp);
 	if (rqstp->rq_xprt->xpt_ops->xpo_start_tls) {
 		p = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT * 2 + 8);
 		if (!p)
@@ -959,7 +957,6 @@ svcauth_unix_accept(struct svc_rqst *rqstp)
 		return SVC_DENIED;
 	}
 
-	svcxdr_init_encode(rqstp);
 	if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
 					  RPC_AUTH_NULL, NULL, 0) < 0)
 		return SVC_CLOSE;

[PATCH v1 22/27] SUNRPC: Convert RPC Reply header encoding to use xdr_stream

[Chuck Lever]

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

The main part of RPC header encoding and the formation of error
responses are now done using the xdr_stream helpers. Bounds checking
before each XDR data item is encoded makes the server's encoding
path safer against accidental buffer overflows.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/svc.c |   50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 393eebd1f6fe..bcca1553c75a 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1227,13 +1227,14 @@ EXPORT_SYMBOL_GPL(svc_generic_init_request);
 static int
 svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 {
+	struct xdr_stream	*xdr = &rqstp->rq_res_stream;
 	struct svc_program	*progp;
 	const struct svc_procedure *procp = NULL;
 	struct svc_serv		*serv = rqstp->rq_server;
 	struct svc_process_info process;
 	__be32			*p, *statp;
 	int			auth_res, rc;
-	__be32			*reply_statp;
+	unsigned int		aoffset;
 
 	/* Will be turned off by GSS integrity and privacy services */
 	__set_bit(RQ_SPLICE_OK, &rqstp->rq_flags);
@@ -1242,9 +1243,9 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	__clear_bit(RQ_DROPME, &rqstp->rq_flags);
 
 	/* Construct the first words of the reply: */
-	svc_putu32(resv, rqstp->rq_xid);
-	svc_putnl(resv, RPC_REPLY);
-	reply_statp = resv->iov_base + resv->iov_len;
+	svcxdr_init_encode(rqstp);
+	xdr_stream_encode_be32(xdr, rqstp->rq_xid);
+	xdr_stream_encode_be32(xdr, rpc_reply);
 
 	p = xdr_inline_decode(&rqstp->rq_arg_stream, XDR_UNIT * 4);
 	if (unlikely(!p))
@@ -1252,7 +1253,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	if (*p++ != cpu_to_be32(RPC_VERSION))
 		goto err_bad_rpc;
 
-	svc_putnl(resv, 0);		/* ACCEPT */
+	xdr_stream_encode_be32(xdr, rpc_msg_accepted);
 
 	rqstp->rq_prog = be32_to_cpup(p++);
 	rqstp->rq_vers = be32_to_cpup(p++);
@@ -1262,8 +1263,6 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 		if (rqstp->rq_prog == progp->pg_prog)
 			break;
 
-	svcxdr_init_encode(rqstp);
-
 	/*
 	 * Decode auth data, and add verifier to reply buffer.
 	 * We do this before anything else in order to get a decent
@@ -1314,6 +1313,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	serv->sv_stats->rpccnt++;
 	trace_svc_process(rqstp, progp->pg_name);
 
+	aoffset = xdr_stream_pos(xdr);
 	statp = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT);
 	*statp = rpc_success;
 
@@ -1332,9 +1332,8 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 	if (rqstp->rq_auth_stat != rpc_auth_ok)
 		goto err_bad_auth;
 
-	/* Check RPC status result */
 	if (*statp != rpc_success)
-		resv->iov_len = ((void*)statp)  - resv->iov_base + 4;
+		xdr_truncate_encode(xdr, aoffset);
 
 	if (procp->pc_encode == NULL)
 		goto dropit;
@@ -1364,27 +1363,28 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 
 err_bad_rpc:
 	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, 1);	/* REJECT */
-	svc_putnl(resv, 0);	/* RPC_MISMATCH */
-	svc_putnl(resv, 2);	/* Only RPCv2 supported */
-	svc_putnl(resv, 2);
+	xdr_stream_encode_u32(xdr, RPC_MSG_DENIED);
+	xdr_stream_encode_u32(xdr, RPC_MISMATCH);
+	/* Only RPCv2 supported */
+	xdr_stream_encode_u32(xdr, RPC_VERSION);
+	xdr_stream_encode_u32(xdr, RPC_VERSION);
 	goto sendit;
 
 err_bad_auth:
 	dprintk("svc: authentication failed (%d)\n",
 		be32_to_cpu(rqstp->rq_auth_stat));
 	serv->sv_stats->rpcbadauth++;
-	/* Restore write pointer to location of accept status: */
-	xdr_ressize_check(rqstp, reply_statp);
-	svc_putnl(resv, 1);	/* REJECT */
-	svc_putnl(resv, 1);	/* AUTH_ERROR */
-	svc_putu32(resv, rqstp->rq_auth_stat);	/* status */
+	/* Restore write pointer to location of reply status: */
+	xdr_truncate_encode(xdr, XDR_UNIT * 2);
+	xdr_stream_encode_u32(xdr, RPC_MSG_DENIED);
+	xdr_stream_encode_u32(xdr, RPC_AUTH_ERROR);
+	xdr_stream_encode_be32(xdr, rqstp->rq_auth_stat);
 	goto sendit;
 
 err_bad_prog:
 	dprintk("svc: unknown program %d\n", rqstp->rq_prog);
 	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, RPC_PROG_UNAVAIL);
+	xdr_stream_encode_u32(xdr, RPC_PROG_UNAVAIL);
 	goto sendit;
 
 err_bad_vers:
@@ -1392,28 +1392,28 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
 		       rqstp->rq_vers, rqstp->rq_prog, progp->pg_name);
 
 	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, RPC_PROG_MISMATCH);
-	svc_putnl(resv, process.mismatch.lovers);
-	svc_putnl(resv, process.mismatch.hivers);
+	xdr_stream_encode_u32(xdr, RPC_PROG_MISMATCH);
+	xdr_stream_encode_u32(xdr, process.mismatch.lovers);
+	xdr_stream_encode_u32(xdr, process.mismatch.hivers);
 	goto sendit;
 
 err_bad_proc:
 	svc_printk(rqstp, "unknown procedure (%d)\n", rqstp->rq_proc);
 
 	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, RPC_PROC_UNAVAIL);
+	xdr_stream_encode_u32(xdr, RPC_PROC_UNAVAIL);
 	goto sendit;
 
 err_garbage_args:
 	svc_printk(rqstp, "failed to decode RPC header\n");
 
 	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, RPC_GARBAGE_ARGS);
+	xdr_stream_encode_u32(xdr, RPC_GARBAGE_ARGS);
 	goto sendit;
 
 err_system_err:
 	serv->sv_stats->rpcbadfmt++;
-	svc_putnl(resv, RPC_SYSTEM_ERR);
+	xdr_stream_encode_u32(xdr, RPC_SYSTEM_ERR);
 	goto sendit;
 }
 

[PATCH v1 23/27] SUNRPC: Final clean-up of svc_process_common()

[Chuck Lever]

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

The @resv parameter is no longer used.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/svc.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index bcca1553c75a..bb58915622ca 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1225,7 +1225,7 @@ EXPORT_SYMBOL_GPL(svc_generic_init_request);
  * Common routine for processing the RPC request.
  */
 static int
-svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
+svc_process_common(struct svc_rqst *rqstp)
 {
 	struct xdr_stream	*xdr = &rqstp->rq_res_stream;
 	struct svc_program	*progp;
@@ -1455,7 +1455,7 @@ svc_process(struct svc_rqst *rqstp)
 	if (unlikely(*p != rpc_call))
 		goto out_baddir;
 
-	if (!svc_process_common(rqstp, resv))
+	if (!svc_process_common(rqstp))
 		goto out_drop;
 	return svc_send(rqstp);
 
@@ -1478,7 +1478,6 @@ int
 bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
 	       struct svc_rqst *rqstp)
 {
-	struct kvec	*resv = &rqstp->rq_res.head[0];
 	struct rpc_task *task;
 	int proc_error;
 	int error;
@@ -1509,22 +1508,21 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
 		rqstp->rq_arg.len = rqstp->rq_arg.head[0].iov_len +
 			rqstp->rq_arg.page_len;
 
-	/* reset result send buffer "put" position */
-	resv->iov_len = 0;
-
-	svcxdr_init_decode(rqstp);
+	/* Reset the response buffer */
+	rqstp->rq_res.head[0].iov_len = 0;
 
 	/*
 	 * Skip the XID and calldir fields because they've already
 	 * been processed by the caller.
 	 */
+	svcxdr_init_decode(rqstp);
 	if (!xdr_inline_decode(&rqstp->rq_arg_stream, XDR_UNIT * 2)) {
 		error = -EINVAL;
 		goto out;
 	}
 
 	/* Parse and execute the bc call */
-	proc_error = svc_process_common(rqstp, resv);
+	proc_error = svc_process_common(rqstp);
 
 	atomic_dec(&req->rq_xprt->bc_slot_count);
 	if (!proc_error) {

[PATCH v1 24/27] SUNRPC: Remove no-longer-used helper functions

[Chuck Lever]

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

The svc_get/put helpers are no longer used.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/svc.h |   66 +-------------------------------------------
 1 file changed, 1 insertion(+), 65 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index dd9f68acd9f1..32eb98e621c3 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -193,40 +193,6 @@ extern u32 svc_max_payload(const struct svc_rqst *rqstp);
 #define RPCSVC_MAXPAGES		((RPCSVC_MAXPAYLOAD+PAGE_SIZE-1)/PAGE_SIZE \
 				+ 2 + 1)
 
-static inline u32 svc_getnl(struct kvec *iov)
-{
-	__be32 val, *vp;
-	vp = iov->iov_base;
-	val = *vp++;
-	iov->iov_base = (void*)vp;
-	iov->iov_len -= sizeof(__be32);
-	return ntohl(val);
-}
-
-static inline void svc_putnl(struct kvec *iov, u32 val)
-{
-	__be32 *vp = iov->iov_base + iov->iov_len;
-	*vp = htonl(val);
-	iov->iov_len += sizeof(__be32);
-}
-
-static inline __be32 svc_getu32(struct kvec *iov)
-{
-	__be32 val, *vp;
-	vp = iov->iov_base;
-	val = *vp++;
-	iov->iov_base = (void*)vp;
-	iov->iov_len -= sizeof(__be32);
-	return val;
-}
-
-static inline void svc_putu32(struct kvec *iov, __be32 val)
-{
-	__be32 *vp = iov->iov_base + iov->iov_len;
-	*vp = val;
-	iov->iov_len += sizeof(__be32);
-}
-
 /*
  * The context of a single thread, including the request currently being
  * processed.
@@ -345,29 +311,6 @@ static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
 	return (struct sockaddr *) &rqst->rq_daddr;
 }
 
-/*
- * Check buffer bounds after decoding arguments
- */
-static inline int
-xdr_argsize_check(struct svc_rqst *rqstp, __be32 *p)
-{
-	char *cp = (char *)p;
-	struct kvec *vec = &rqstp->rq_arg.head[0];
-	return cp >= (char*)vec->iov_base
-		&& cp <= (char*)vec->iov_base + vec->iov_len;
-}
-
-static inline int
-xdr_ressize_check(struct svc_rqst *rqstp, __be32 *p)
-{
-	struct kvec *vec = &rqstp->rq_res.head[0];
-	char *cp = (char*)p;
-
-	vec->iov_len = cp - (char*)vec->iov_base;
-
-	return vec->iov_len <= PAGE_SIZE;
-}
-
 static inline void svc_free_res_pages(struct svc_rqst *rqstp)
 {
 	while (rqstp->rq_next_page != rqstp->rq_respages) {
@@ -540,9 +483,6 @@ static inline void svc_reserve_auth(struct svc_rqst *rqstp, int space)
  * svcxdr_init_decode - Prepare an xdr_stream for Call decoding
  * @rqstp: controlling server RPC transaction context
  *
- * This function currently assumes the RPC header in rq_arg has
- * already been decoded. Upon return, xdr->p points to the
- * location of the upper layer header.
  */
 static inline void svcxdr_init_decode(struct svc_rqst *rqstp)
 {
@@ -550,11 +490,7 @@ static inline void svcxdr_init_decode(struct svc_rqst *rqstp)
 	struct xdr_buf *buf = &rqstp->rq_arg;
 	struct kvec *argv = buf->head;
 
-	/*
-	 * svc_getnl() and friends do not keep the xdr_buf's ::len
-	 * field up to date. Refresh that field before initializing
-	 * the argument decoding stream.
-	 */
+	WARN_ON(buf->len != buf->head->iov_len + buf->page_len + buf->tail->iov_len);
 	buf->len = buf->head->iov_len + buf->page_len + buf->tail->iov_len;
 
 	xdr_init_decode(xdr, buf, argv->iov_base, NULL);

[PATCH v1 25/27] SUNRPC: Refactor RPC server dispatch method

[Chuck Lever]

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

Currently, svcauth_gss_accept() pre-reserves response buffer space
for the RPC payload length and GSS sequence number before returning
to the dispatcher, which then adds the header's accept_stat field.

The problem is the accept_stat field is supposed to go before the
length and seq_num fields. So svcauth_gss_release() has to relocate
the accept_stat value (see svcauth_gss_prepare_to_wrap()).

To enable these fields to be added to the response buffer in the
correct (final) order, the pointer to the accept_stat has to be made
available to svcauth_gss_accept() so that it can set it before
reserving space for the length and seq_num fields.

As a first step, move the pointer to the location of the accept_stat
field into struct svc_rqst.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/lockd/svc.c             |    4 ++--
 fs/nfs/callback_xdr.c      |    4 ++--
 fs/nfsd/nfscache.c         |    2 +-
 fs/nfsd/nfsd.h             |    2 +-
 fs/nfsd/nfssvc.c           |    4 ++--
 include/linux/sunrpc/svc.h |    5 +++--
 net/sunrpc/svc.c           |   10 +++++-----
 7 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 642e394e7a2d..0b28a6cf9303 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -685,15 +685,15 @@ module_exit(exit_nlm);
 /**
  * nlmsvc_dispatch - Process an NLM Request
  * @rqstp: incoming request
- * @statp: pointer to location of accept_stat field in RPC Reply buffer
  *
  * Return values:
  *  %0: Processing complete; do not send a Reply
  *  %1: Processing complete; send Reply in rqstp->rq_res
  */
-static int nlmsvc_dispatch(struct svc_rqst *rqstp, __be32 *statp)
+static int nlmsvc_dispatch(struct svc_rqst *rqstp)
 {
 	const struct svc_procedure *procp = rqstp->rq_procinfo;
+	__be32 *statp = rqstp->rq_accept_statp;
 
 	if (!procp->pc_decode(rqstp, &rqstp->rq_arg_stream))
 		goto out_decode_err;
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index b4c3b7182198..13b2af55497d 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -980,11 +980,11 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp)
 }
 
 static int
-nfs_callback_dispatch(struct svc_rqst *rqstp, __be32 *statp)
+nfs_callback_dispatch(struct svc_rqst *rqstp)
 {
 	const struct svc_procedure *procp = rqstp->rq_procinfo;
 
-	*statp = procp->pc_func(rqstp);
+	*rqstp->rq_accept_statp = procp->pc_func(rqstp);
 	return 1;
 }
 
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index ef5ee548053b..041faa13b852 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -509,7 +509,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp)
  * nfsd_cache_update - Update an entry in the duplicate reply cache.
  * @rqstp: svc_rqst with a finished Reply
  * @cachetype: which cache to update
- * @statp: Reply's status code
+ * @statp: pointer to Reply's NFS status code, or NULL
  *
  * This is called from nfsd_dispatch when the procedure has been
  * executed and the complete reply is in rqstp->rq_res.
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 93b42ef9ed91..a7de2ffe943b 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -86,7 +86,7 @@ bool		nfssvc_encode_voidres(struct svc_rqst *rqstp,
  * Function prototypes.
  */
 int		nfsd_svc(int nrservs, struct net *net, const struct cred *cred);
-int		nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp);
+int		nfsd_dispatch(struct svc_rqst *rqstp);
 
 int		nfsd_nrthreads(struct net *);
 int		nfsd_nrpools(struct net *);
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index dfa8ee6c04d5..ff10c46b62d3 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -1022,7 +1022,6 @@ nfsd(void *vrqstp)
 /**
  * nfsd_dispatch - Process an NFS or NFSACL Request
  * @rqstp: incoming request
- * @statp: pointer to location of accept_stat field in RPC Reply buffer
  *
  * This RPC dispatcher integrates the NFS server's duplicate reply cache.
  *
@@ -1030,9 +1029,10 @@ nfsd(void *vrqstp)
  *  %0: Processing complete; do not send a Reply
  *  %1: Processing complete; send Reply in rqstp->rq_res
  */
-int nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
+int nfsd_dispatch(struct svc_rqst *rqstp)
 {
 	const struct svc_procedure *proc = rqstp->rq_procinfo;
+	__be32 *statp = rqstp->rq_accept_statp;
 
 	/*
 	 * Give the xdr decoder a chance to change this if it wants
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 32eb98e621c3..f40a90ca5de6 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -251,6 +251,7 @@ struct svc_rqst {
 
 	void *			rq_argp;	/* decoded arguments */
 	void *			rq_resp;	/* xdr'd results */
+	__be32			*rq_accept_statp;
 	void *			rq_auth_data;	/* flavor-specific data */
 	__be32			rq_auth_stat;	/* authentication status */
 	int			rq_auth_slack;	/* extra space xdr code
@@ -337,7 +338,7 @@ struct svc_deferred_req {
 
 struct svc_process_info {
 	union {
-		int  (*dispatch)(struct svc_rqst *, __be32 *);
+		int  (*dispatch)(struct svc_rqst *rqstp);
 		struct {
 			unsigned int lovers;
 			unsigned int hivers;
@@ -389,7 +390,7 @@ struct svc_version {
 	bool			vs_need_cong_ctrl;
 
 	/* Dispatch function */
-	int			(*vs_dispatch)(struct svc_rqst *, __be32 *);
+	int			(*vs_dispatch)(struct svc_rqst *rqstp);
 };
 
 /*
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index bb58915622ca..3c194e6f8f5e 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1232,9 +1232,9 @@ svc_process_common(struct svc_rqst *rqstp)
 	const struct svc_procedure *procp = NULL;
 	struct svc_serv		*serv = rqstp->rq_server;
 	struct svc_process_info process;
-	__be32			*p, *statp;
 	int			auth_res, rc;
 	unsigned int		aoffset;
+	__be32			*p;
 
 	/* Will be turned off by GSS integrity and privacy services */
 	__set_bit(RQ_SPLICE_OK, &rqstp->rq_flags);
@@ -1314,8 +1314,8 @@ svc_process_common(struct svc_rqst *rqstp)
 	trace_svc_process(rqstp, progp->pg_name);
 
 	aoffset = xdr_stream_pos(xdr);
-	statp = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT);
-	*statp = rpc_success;
+	rqstp->rq_accept_statp = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT);
+	*rqstp->rq_accept_statp = rpc_success;
 
 	/* un-reserve some of the out-queue now that we have a
 	 * better idea of reply size
@@ -1324,7 +1324,7 @@ svc_process_common(struct svc_rqst *rqstp)
 		svc_reserve_auth(rqstp, procp->pc_xdrressize<<2);
 
 	/* Call the function that processes the request. */
-	rc = process.dispatch(rqstp, statp);
+	rc = process.dispatch(rqstp);
 	if (procp->pc_release)
 		procp->pc_release(rqstp);
 	if (!rc)
@@ -1332,7 +1332,7 @@ svc_process_common(struct svc_rqst *rqstp)
 	if (rqstp->rq_auth_stat != rpc_auth_ok)
 		goto err_bad_auth;
 
-	if (*statp != rpc_success)
+	if (*rqstp->rq_accept_statp != rpc_success)
 		xdr_truncate_encode(xdr, aoffset);
 
 	if (procp->pc_encode == NULL)

[PATCH v1 26/27] SUNRPC: Set rq_accept_statp inside ->accept methods

[Chuck Lever]

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

To navigate around the space that svcauth_gss_accept() reserves
for the RPC payload body length and sequence number fields,
svcauth_gss_release() does a little dance with the reply's
accept_stat, moving the accept_stat value in the response buffer
down by two words.

Instead, let's have the ->accept() methods each set the proper
final location of the accept_stat to avoid having to move
things.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/svc.h        |   19 +++++++++++++++++++
 net/sunrpc/auth_gss/svcauth_gss.c |   21 ++++++++++-----------
 net/sunrpc/svc.c                  |    2 --
 net/sunrpc/svcauth_unix.c         |    6 ++++++
 4 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index f40a90ca5de6..392d2d2620fa 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -544,4 +544,23 @@ static inline void svcxdr_set_auth_slack(struct svc_rqst *rqstp, int slack)
 	WARN_ON(xdr->p > xdr->end);
 }
 
+/**
+ * svcxdr_set_accept_stat - Reserve space for the accept_stat field
+ * @rqstp: RPC transaction context
+ *
+ * Return values:
+ *   %true: Success
+ *   %false: No response buffer space was available
+ */
+static inline bool svcxdr_set_accept_stat(struct svc_rqst *rqstp)
+{
+	struct xdr_stream *xdr = &rqstp->rq_res_stream;
+
+	rqstp->rq_accept_statp = xdr_reserve_space(xdr, XDR_UNIT);
+	if (unlikely(!rqstp->rq_accept_statp))
+		return false;
+	*rqstp->rq_accept_statp = rpc_success;
+	return true;
+}
+
 #endif /* SUNRPC_SVC_H */
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 560fb8a2803d..333873abb7d9 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1220,7 +1220,7 @@ svcauth_gss_legacy_init(struct svc_rqst *rqstp,
 	if (!svcauth_gss_proc_init_verf(sn->rsc_cache, rqstp, &rsip->out_handle,
 					&rsip->major_status, GSS_SEQ_WIN))
 		goto out;
-	if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
+	if (!svcxdr_set_accept_stat(rqstp))
 		goto out;
 	if (!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream, &rsip->out_handle,
 					&rsip->out_token, rsip->major_status,
@@ -1348,7 +1348,7 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,
 	if (!svcauth_gss_proc_init_verf(sn->rsc_cache, rqstp, &cli_handle,
 					&ud.major_status, GSS_SEQ_WIN))
 		goto out;
-	if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
+	if (!svcxdr_set_accept_stat(rqstp))
 		goto out;
 	if (!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream, &cli_handle,
 					&ud.out_token, ud.major_status,
@@ -1640,16 +1640,18 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 	case RPC_GSS_PROC_DESTROY:
 		if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
 			goto auth_err;
+		if (!svcxdr_set_accept_stat(rqstp))
+			goto auth_err;
 		/* Delete the entry from the cache_list and call cache_put */
 		sunrpc_cache_unhash(sn->rsc_cache, &rsci->h);
-		if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
-			goto auth_err;
 		goto complete;
 	case RPC_GSS_PROC_DATA:
 		rqstp->rq_auth_stat = rpcsec_gsserr_ctxproblem;
 		svcdata->verf_start = xdr_reserve_space(&rqstp->rq_res_stream, 0);
 		if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
 			goto auth_err;
+		if (!svcxdr_set_accept_stat(rqstp))
+			goto auth_err;
 		rqstp->rq_cred = rsci->cred;
 		get_group_info(rsci->cred.cr_group_info);
 		rqstp->rq_auth_stat = rpc_autherr_badcred;
@@ -1706,7 +1708,6 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 static __be32 *
 svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)
 {
-	struct xdr_buf *resbuf = &rqstp->rq_res;
 	__be32 *p;
 	u32 verf_len;
 
@@ -1721,13 +1722,11 @@ svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)
 	p += 1;
 	verf_len = ntohl(*p++);
 	p += XDR_QUADLEN(verf_len);
-	/* move accept_stat to right place: */
-	memcpy(p, p + 2, 4);
-	/* Also don't wrap if the accept stat is nonzero: */
-	if (*p != rpc_success) {
-		resbuf->head[0].iov_len -= 2 * 4;
+
+	/* Also don't wrap if the accept_stat is nonzero: */
+	if (*rqstp->rq_accept_statp != rpc_success)
 		return NULL;
-	}
+
 	p++;
 	return p;
 }
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 3c194e6f8f5e..c2ed8b06fadb 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1314,8 +1314,6 @@ svc_process_common(struct svc_rqst *rqstp)
 	trace_svc_process(rqstp, progp->pg_name);
 
 	aoffset = xdr_stream_pos(xdr);
-	rqstp->rq_accept_statp = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT);
-	*rqstp->rq_accept_statp = rpc_success;
 
 	/* un-reserve some of the out-queue now that we have a
 	 * better idea of reply size
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index b101700d155c..62dfc8cdf8c5 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -775,6 +775,8 @@ svcauth_null_accept(struct svc_rqst *rqstp)
 	if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
 					  RPC_AUTH_NULL, NULL, 0) < 0)
 		return SVC_CLOSE;
+	if (!svcxdr_set_accept_stat(rqstp))
+		return SVC_CLOSE;
 
 	rqstp->rq_cred.cr_flavor = RPC_AUTH_NULL;
 	return SVC_OK;
@@ -866,6 +868,8 @@ svcauth_tls_accept(struct svc_rqst *rqstp)
 						  RPC_AUTH_NULL, NULL, 0) < 0)
 			return SVC_CLOSE;
 	}
+	if (!svcxdr_set_accept_stat(rqstp))
+		return SVC_CLOSE;
 
 	rqstp->rq_cred.cr_flavor = RPC_AUTH_TLS;
 	return SVC_OK;
@@ -960,6 +964,8 @@ svcauth_unix_accept(struct svc_rqst *rqstp)
 	if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
 					  RPC_AUTH_NULL, NULL, 0) < 0)
 		return SVC_CLOSE;
+	if (!svcxdr_set_accept_stat(rqstp))
+		return SVC_CLOSE;
 
 	rqstp->rq_cred.cr_flavor = RPC_AUTH_UNIX;
 	return SVC_OK;

[PATCH v1 27/27] SUNRPC: Go back to using gsd->body_start

[Chuck Lever]

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

Now that svcauth_gss_prepare_to_wrap() no longer computes the
location of RPC header fields in the response buffer,
svcauth_gss_accept() can save the location of the databody
rather than the location of the verifier.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth_gss/svcauth_gss.c |   78 +++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 42 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 333873abb7d9..419d5ad6311c 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -71,9 +71,7 @@
 struct gss_svc_data {
 	/* decoded gss client cred: */
 	struct rpc_gss_wire_cred	clcred;
-	/* save a pointer to the beginning of the encoded verifier,
-	 * for use in encryption/checksumming in svcauth_gss_release: */
-	__be32				*verf_start;
+	u32				gsd_databody_offset;
 	struct rsc			*rsci;
 
 	/* for temporary results */
@@ -1595,7 +1593,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 	if (!svcdata)
 		goto auth_err;
 	rqstp->rq_auth_data = svcdata;
-	svcdata->verf_start = NULL;
+	svcdata->gsd_databody_offset = 0;
 	svcdata->rsci = NULL;
 	gc = &svcdata->clcred;
 
@@ -1647,11 +1645,11 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 		goto complete;
 	case RPC_GSS_PROC_DATA:
 		rqstp->rq_auth_stat = rpcsec_gsserr_ctxproblem;
-		svcdata->verf_start = xdr_reserve_space(&rqstp->rq_res_stream, 0);
 		if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
 			goto auth_err;
 		if (!svcxdr_set_accept_stat(rqstp))
 			goto auth_err;
+		svcdata->gsd_databody_offset = xdr_stream_pos(&rqstp->rq_res_stream);
 		rqstp->rq_cred = rsci->cred;
 		get_group_info(rsci->cred.cr_group_info);
 		rqstp->rq_auth_stat = rpc_autherr_badcred;
@@ -1705,30 +1703,24 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
 	return ret;
 }
 
-static __be32 *
+static u32
 svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)
 {
-	__be32 *p;
-	u32 verf_len;
+	u32 offset;
 
-	p = gsd->verf_start;
-	gsd->verf_start = NULL;
+	/* Release can be called twice, but we only wrap once. */
+	offset = gsd->gsd_databody_offset;
+	gsd->gsd_databody_offset = 0;
 
 	/* AUTH_ERROR replies are not wrapped. */
 	if (rqstp->rq_auth_stat != rpc_auth_ok)
-		return NULL;
-
-	/* Skip the verifier: */
-	p += 1;
-	verf_len = ntohl(*p++);
-	p += XDR_QUADLEN(verf_len);
+		return 0;
 
 	/* Also don't wrap if the accept_stat is nonzero: */
 	if (*rqstp->rq_accept_statp != rpc_success)
-		return NULL;
+		return 0;
 
-	p++;
-	return p;
+	return offset;
 }
 
 /*
@@ -1756,21 +1748,21 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
 	struct xdr_buf *buf = xdr->buf;
 	struct xdr_buf databody_integ;
 	struct xdr_netobj checksum;
-	u32 offset, len, maj_stat;
-	__be32 *p;
+	u32 offset, maj_stat;
 
-	p = svcauth_gss_prepare_to_wrap(rqstp, gsd);
-	if (p == NULL)
+	offset = svcauth_gss_prepare_to_wrap(rqstp, gsd);
+	if (!offset)
 		goto out;
 
-	offset = (u8 *)(p + 1) - (u8 *)buf->head[0].iov_base;
-	len = buf->len - offset;
-	if (xdr_buf_subsegment(buf, &databody_integ, offset, len))
+	if (xdr_buf_subsegment(buf, &databody_integ, offset + XDR_UNIT,
+			       buf->len - offset - XDR_UNIT))
 		goto wrap_failed;
 	/* Buffer space for these has already been reserved in
 	 * svcauth_gss_accept(). */
-	*p++ = cpu_to_be32(len);
-	*p = cpu_to_be32(gc->gc_seq);
+	if (xdr_encode_word(buf, offset, databody_integ.len))
+		goto wrap_failed;
+	if (xdr_encode_word(buf, offset + XDR_UNIT, gc->gc_seq))
+		goto wrap_failed;
 
 	checksum.data = gsd->gsd_scratch;
 	maj_stat = gss_get_mic(gsd->rsci->mechctx, &databody_integ, &checksum);
@@ -1817,17 +1809,19 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 	struct kvec *head = buf->head;
 	struct kvec *tail = buf->tail;
 	u32 offset, pad, maj_stat;
-	__be32 *p, *lenp;
+	__be32 *p;
 
-	p = svcauth_gss_prepare_to_wrap(rqstp, gsd);
-	if (p == NULL)
+	offset = svcauth_gss_prepare_to_wrap(rqstp, gsd);
+	if (!offset)
 		return 0;
 
-	lenp = p++;
-	offset = (u8 *)p - (u8 *)head->iov_base;
-	/* Buffer space for this field has already been reserved
-	 * in svcauth_gss_accept(). */
-	*p = cpu_to_be32(gc->gc_seq);
+	/*
+	 * Buffer space for this field has already been reserved
+	 * in svcauth_gss_accept(). Note that the GSS sequence
+	 * number is encrypted along with the RPC reply payload.
+	 */
+	if (xdr_encode_word(buf, offset + XDR_UNIT, gc->gc_seq))
+		goto wrap_failed;
 
 	/*
 	 * If there is currently tail data, make sure there is
@@ -1863,12 +1857,15 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
 		tail->iov_len = 0;
 	}
 
-	maj_stat = gss_wrap(gsd->rsci->mechctx, offset, buf, buf->pages);
+	maj_stat = gss_wrap(gsd->rsci->mechctx, offset + XDR_UNIT, buf,
+			    buf->pages);
 	if (maj_stat != GSS_S_COMPLETE)
 		goto bad_wrap;
 
-	*lenp = cpu_to_be32(buf->len - offset);
-	pad = xdr_pad_size(buf->len - offset);
+	/* Wrapping can change the size of databody_priv. */
+	if (xdr_encode_word(buf, offset, buf->len - offset - XDR_UNIT))
+		goto wrap_failed;
+	pad = xdr_pad_size(buf->len - offset - XDR_UNIT);
 	p = (__be32 *)(tail->iov_base + tail->iov_len);
 	memset(p, 0, pad);
 	tail->iov_len += pad;
@@ -1908,9 +1905,6 @@ svcauth_gss_release(struct svc_rqst *rqstp)
 	gc = &gsd->clcred;
 	if (gc->gc_proc != RPC_GSS_PROC_DATA)
 		goto out;
-	/* Release can be called twice, but we only wrap once. */
-	if (gsd->verf_start == NULL)
-		goto out;
 
 	switch (gc->gc_svc) {
 	case RPC_GSS_SVC_NONE:

Re: [PATCH v1 01/27] SUNRPC: Clean up svcauth_gss_release()

[Jeff Layton]

On Sun, 2023-01-08 at 11:28 -0500, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Now that upper layers use an xdr_stream to track the construction
> of each RPC Reply message, resbuf->len is kept up-to-date
> automatically. There's no need to recompute it in svc_gss_release().
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/auth_gss/svcauth_gss.c |   30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 2e603358fae1..4a576ed7aa32 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -969,12 +969,6 @@ svcauth_gss_unwrap_integ(struct svc_rqst *rqstp, u32 seq, struct gss_ctx *ctx)
>  	return -EINVAL;
>  }
>  
> -static inline int
> -total_buf_len(struct xdr_buf *buf)
> -{
> -	return buf->head[0].iov_len + buf->page_len + buf->tail[0].iov_len;
> -}
> -
>  /*
>   * RFC 2203, Section 5.3.2.3
>   *
> @@ -1882,14 +1876,25 @@ svcauth_gss_wrap_resp_priv(struct svc_rqst *rqstp)
>  	return 0;
>  }
>  
> +/**
> + * svcauth_gss_release - Wrap payload and release resources
> + * @rqstp: RPC transaction context
> + *
> + * Return values:
> + *    %0: the Reply is ready to be sent
> + *    %-ENOMEM: failed to allocate memory
> + *    %-EINVAL: encoding error
> + *
> + * XXX: These return values do not match the return values documented
> + *      for the auth_ops ->release method in linux/sunrpc/svcauth.h.

As an aside...

It looks like the only place ->release is called is in svc_authorise,
and its callers either ignore the return, or just test whether it
succeeded (returned 0). If it fails then it looks like the xprt is
closed.

The actual return code doesn't matter at all. We could make ->release a
bool return to make that clear.

That's not directly related to this set though.


>  static int
>  svcauth_gss_release(struct svc_rqst *rqstp)
>  {
> -	struct gss_svc_data *gsd = (struct gss_svc_data *)rqstp->rq_auth_data;
> -	struct rpc_gss_wire_cred *gc;
> -	struct xdr_buf *resbuf = &rqstp->rq_res;
> -	int stat = -EINVAL;
>  	struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
> +	struct gss_svc_data *gsd = rqstp->rq_auth_data;
> +	struct rpc_gss_wire_cred *gc;
> +	int stat;
>  
>  	if (!gsd)
>  		goto out;
> @@ -1899,10 +1904,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
>  	/* Release can be called twice, but we only wrap once. */
>  	if (gsd->verf_start == NULL)
>  		goto out;
> -	/* normally not set till svc_send, but we need it here: */
> -	/* XXX: what for?  Do we mess it up the moment we call svc_putu32
> -	 * or whatever? */
> -	resbuf->len = total_buf_len(resbuf);
> +
>  	switch (gc->gc_svc) {
>  	case RPC_GSS_SVC_NONE:
>  		break;
> 
> 

The patch itself looks fine.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1 00/27] Server-side RPC reply header parsing overhaul

[Jeff Layton]

On Sun, 2023-01-08 at 11:28 -0500, Chuck Lever wrote:
> The purpose of this series is to replace the svc_put* macros in the
> Linux kernel server's RPC reply header construction code with
> xdr_stream helpers. I've measured no change in CPU utilization after
> the overhaul.
> 
> Memory safety: Buffer bounds checking after encoding each XDR item
> is more memory-safe than the current mechanism. Subsequent memory
> safety improvements to the common xdr_stream helpers will benefit
> all who use them.
> 
> Audit friendliness: The new code has additional comments and other
> clean-up to help align it with the relevant RPC protocol
> specifications. The use of common helpers also makes the encoders
> easier to audit and maintain.
> 
> I've split the full series in half to make it easier to review. The
> patches posted here are the second half, handling RPC reply header
> encoding.
> 
> Note that another benefit of this work is that we are taking one or
> two more strides closer to greater commonality between the client
> and server implementations of RPCSEC GSS.
> 
> ---
> 
> Chuck Lever (27):
>       SUNRPC: Clean up svcauth_gss_release()
>       SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()
>       SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()
>       SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()
>       SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()
>       SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()
>       SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()
>       SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()
>       SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()
>       SUNRPC: Check rq_auth_stat when preparing to wrap a response
>       SUNRPC: Remove the rpc_stat variable in svc_process_common()
>       SUNRPC: Add XDR encoding helper for opaque_auth
>       SUNRPC: Push svcxdr_init_encode() into svc_process_common()
>       SUNRPC: Move svcxdr_init_encode() into ->accept methods
>       SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()
>       SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()
>       SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()
>       SUNRPC: Convert unwrap data paths to use xdr_stream for replies
>       SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers
>       SUNRPC: Use xdr_stream for encoding GSS reply verifiers
>       SUNRPC: Hoist init_encode out of svc_authenticate()
>       SUNRPC: Convert RPC Reply header encoding to use xdr_stream
>       SUNRPC: Final clean-up of svc_process_common()
>       SUNRPC: Remove no-longer-used helper functions
>       SUNRPC: Refactor RPC server dispatch method
>       SUNRPC: Set rq_accept_statp inside ->accept methods
>       SUNRPC: Go back to using gsd->body_start
> 
> 
>  fs/lockd/svc.c                    |   5 +-
>  fs/nfs/callback_xdr.c             |   6 +-
>  fs/nfsd/nfscache.c                |   4 +-
>  fs/nfsd/nfsd.h                    |   2 +-
>  fs/nfsd/nfssvc.c                  |  10 +-
>  include/linux/sunrpc/svc.h        | 116 +++----
>  include/linux/sunrpc/xdr.h        |  23 ++
>  include/trace/events/rpcgss.h     |  22 ++
>  net/sunrpc/auth_gss/svcauth_gss.c | 505 +++++++++++++++---------------
>  net/sunrpc/svc.c                  |  91 +++---
>  net/sunrpc/svcauth_unix.c         |  40 ++-
>  net/sunrpc/xdr.c                  |  29 ++
>  12 files changed, 451 insertions(+), 402 deletions(-)
> 
> --
> Chuck Lever
> 

I went through the whole set and this all looks like good stuff to me.
The result is a lot more readable, and there is a lot less manual
fiddling with buffer lengths and such.

Do you have a public branch with the current state of this set?

You can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1 00/27] Server-side RPC reply header parsing overhaul

[Chuck Lever III]

> On Jan 10, 2023, at 9:53 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Sun, 2023-01-08 at 11:28 -0500, Chuck Lever wrote:
>> The purpose of this series is to replace the svc_put* macros in the
>> Linux kernel server's RPC reply header construction code with
>> xdr_stream helpers. I've measured no change in CPU utilization after
>> the overhaul.
>> 
>> Memory safety: Buffer bounds checking after encoding each XDR item
>> is more memory-safe than the current mechanism. Subsequent memory
>> safety improvements to the common xdr_stream helpers will benefit
>> all who use them.
>> 
>> Audit friendliness: The new code has additional comments and other
>> clean-up to help align it with the relevant RPC protocol
>> specifications. The use of common helpers also makes the encoders
>> easier to audit and maintain.
>> 
>> I've split the full series in half to make it easier to review. The
>> patches posted here are the second half, handling RPC reply header
>> encoding.
>> 
>> Note that another benefit of this work is that we are taking one or
>> two more strides closer to greater commonality between the client
>> and server implementations of RPCSEC GSS.
>> 
>> ---
>> 
>> Chuck Lever (27):
>>      SUNRPC: Clean up svcauth_gss_release()
>>      SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()
>>      SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()
>>      SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()
>>      SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()
>>      SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()
>>      SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()
>>      SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()
>>      SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()
>>      SUNRPC: Check rq_auth_stat when preparing to wrap a response
>>      SUNRPC: Remove the rpc_stat variable in svc_process_common()
>>      SUNRPC: Add XDR encoding helper for opaque_auth
>>      SUNRPC: Push svcxdr_init_encode() into svc_process_common()
>>      SUNRPC: Move svcxdr_init_encode() into ->accept methods
>>      SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()
>>      SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()
>>      SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()
>>      SUNRPC: Convert unwrap data paths to use xdr_stream for replies
>>      SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers
>>      SUNRPC: Use xdr_stream for encoding GSS reply verifiers
>>      SUNRPC: Hoist init_encode out of svc_authenticate()
>>      SUNRPC: Convert RPC Reply header encoding to use xdr_stream
>>      SUNRPC: Final clean-up of svc_process_common()
>>      SUNRPC: Remove no-longer-used helper functions
>>      SUNRPC: Refactor RPC server dispatch method
>>      SUNRPC: Set rq_accept_statp inside ->accept methods
>>      SUNRPC: Go back to using gsd->body_start
>> 
>> 
>> fs/lockd/svc.c                    |   5 +-
>> fs/nfs/callback_xdr.c             |   6 +-
>> fs/nfsd/nfscache.c                |   4 +-
>> fs/nfsd/nfsd.h                    |   2 +-
>> fs/nfsd/nfssvc.c                  |  10 +-
>> include/linux/sunrpc/svc.h        | 116 +++----
>> include/linux/sunrpc/xdr.h        |  23 ++
>> include/trace/events/rpcgss.h     |  22 ++
>> net/sunrpc/auth_gss/svcauth_gss.c | 505 +++++++++++++++---------------
>> net/sunrpc/svc.c                  |  91 +++---
>> net/sunrpc/svcauth_unix.c         |  40 ++-
>> net/sunrpc/xdr.c                  |  29 ++
>> 12 files changed, 451 insertions(+), 402 deletions(-)
>> 
>> --
>> Chuck Lever
>> 
> 
> I went through the whole set and this all looks like good stuff to me.
> The result is a lot more readable, and there is a lot less manual
> fiddling with buffer lengths and such.
> 
> Do you have a public branch with the current state of this set?

These are in the topic-rpcsec-gss-krb5-enhancements branch in
my repo at kernel.org, although I'm about to push them to nfsd's
for-next.


> You can add:
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

I very much appreciate your time!

--
Chuck Lever

Re: [PATCH v1 26/27] SUNRPC: Set rq_accept_statp inside ->accept methods

[Jiri Slaby]

On 08. 01. 23, 17:31, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> To navigate around the space that svcauth_gss_accept() reserves
> for the RPC payload body length and sequence number fields,
> svcauth_gss_release() does a little dance with the reply's
> accept_stat, moving the accept_stat value in the response buffer
> down by two words.
> 
> Instead, let's have the ->accept() methods each set the proper
> final location of the accept_stat to avoid having to move
> things.

Hi,

I bisected to this (4bcf0343e8) as it breaks nfs3-only servers in 6.3. 
I.e. /etc/nfs.conf containing:
[nfsd]
  vers4=no

The client sees:
   mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, 
"vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
   write(2, "mount.nfs: mount system call fai"..., 45
   mount.nfs: mount system call failed for /mnt

And the kernel says:
   nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO

I reported in downstream as:
https://bugzilla.suse.com/show_bug.cgi?id=1210995

It cannot be reverted cleanly on the top of 6.3.

Any ideas?

Thanks.

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>   include/linux/sunrpc/svc.h        |   19 +++++++++++++++++++
>   net/sunrpc/auth_gss/svcauth_gss.c |   21 ++++++++++-----------
>   net/sunrpc/svc.c                  |    2 --
>   net/sunrpc/svcauth_unix.c         |    6 ++++++
>   4 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index f40a90ca5de6..392d2d2620fa 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -544,4 +544,23 @@ static inline void svcxdr_set_auth_slack(struct svc_rqst *rqstp, int slack)
>   	WARN_ON(xdr->p > xdr->end);
>   }
>   
> +/**
> + * svcxdr_set_accept_stat - Reserve space for the accept_stat field
> + * @rqstp: RPC transaction context
> + *
> + * Return values:
> + *   %true: Success
> + *   %false: No response buffer space was available
> + */
> +static inline bool svcxdr_set_accept_stat(struct svc_rqst *rqstp)
> +{
> +	struct xdr_stream *xdr = &rqstp->rq_res_stream;
> +
> +	rqstp->rq_accept_statp = xdr_reserve_space(xdr, XDR_UNIT);
> +	if (unlikely(!rqstp->rq_accept_statp))
> +		return false;
> +	*rqstp->rq_accept_statp = rpc_success;
> +	return true;
> +}
> +
>   #endif /* SUNRPC_SVC_H */
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 560fb8a2803d..333873abb7d9 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -1220,7 +1220,7 @@ svcauth_gss_legacy_init(struct svc_rqst *rqstp,
>   	if (!svcauth_gss_proc_init_verf(sn->rsc_cache, rqstp, &rsip->out_handle,
>   					&rsip->major_status, GSS_SEQ_WIN))
>   		goto out;
> -	if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
> +	if (!svcxdr_set_accept_stat(rqstp))
>   		goto out;
>   	if (!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream, &rsip->out_handle,
>   					&rsip->out_token, rsip->major_status,
> @@ -1348,7 +1348,7 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,
>   	if (!svcauth_gss_proc_init_verf(sn->rsc_cache, rqstp, &cli_handle,
>   					&ud.major_status, GSS_SEQ_WIN))
>   		goto out;
> -	if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
> +	if (!svcxdr_set_accept_stat(rqstp))
>   		goto out;
>   	if (!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream, &cli_handle,
>   					&ud.out_token, ud.major_status,
> @@ -1640,16 +1640,18 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
>   	case RPC_GSS_PROC_DESTROY:
>   		if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
>   			goto auth_err;
> +		if (!svcxdr_set_accept_stat(rqstp))
> +			goto auth_err;
>   		/* Delete the entry from the cache_list and call cache_put */
>   		sunrpc_cache_unhash(sn->rsc_cache, &rsci->h);
> -		if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
> -			goto auth_err;
>   		goto complete;
>   	case RPC_GSS_PROC_DATA:
>   		rqstp->rq_auth_stat = rpcsec_gsserr_ctxproblem;
>   		svcdata->verf_start = xdr_reserve_space(&rqstp->rq_res_stream, 0);
>   		if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
>   			goto auth_err;
> +		if (!svcxdr_set_accept_stat(rqstp))
> +			goto auth_err;
>   		rqstp->rq_cred = rsci->cred;
>   		get_group_info(rsci->cred.cr_group_info);
>   		rqstp->rq_auth_stat = rpc_autherr_badcred;
> @@ -1706,7 +1708,6 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
>   static __be32 *
>   svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)
>   {
> -	struct xdr_buf *resbuf = &rqstp->rq_res;
>   	__be32 *p;
>   	u32 verf_len;
>   
> @@ -1721,13 +1722,11 @@ svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)
>   	p += 1;
>   	verf_len = ntohl(*p++);
>   	p += XDR_QUADLEN(verf_len);
> -	/* move accept_stat to right place: */
> -	memcpy(p, p + 2, 4);
> -	/* Also don't wrap if the accept stat is nonzero: */
> -	if (*p != rpc_success) {
> -		resbuf->head[0].iov_len -= 2 * 4;
> +
> +	/* Also don't wrap if the accept_stat is nonzero: */
> +	if (*rqstp->rq_accept_statp != rpc_success)
>   		return NULL;
> -	}
> +
>   	p++;
>   	return p;
>   }
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 3c194e6f8f5e..c2ed8b06fadb 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1314,8 +1314,6 @@ svc_process_common(struct svc_rqst *rqstp)
>   	trace_svc_process(rqstp, progp->pg_name);
>   
>   	aoffset = xdr_stream_pos(xdr);
> -	rqstp->rq_accept_statp = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT);
> -	*rqstp->rq_accept_statp = rpc_success;
>   
>   	/* un-reserve some of the out-queue now that we have a
>   	 * better idea of reply size
> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index b101700d155c..62dfc8cdf8c5 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -775,6 +775,8 @@ svcauth_null_accept(struct svc_rqst *rqstp)
>   	if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
>   					  RPC_AUTH_NULL, NULL, 0) < 0)
>   		return SVC_CLOSE;
> +	if (!svcxdr_set_accept_stat(rqstp))
> +		return SVC_CLOSE;
>   
>   	rqstp->rq_cred.cr_flavor = RPC_AUTH_NULL;
>   	return SVC_OK;
> @@ -866,6 +868,8 @@ svcauth_tls_accept(struct svc_rqst *rqstp)
>   						  RPC_AUTH_NULL, NULL, 0) < 0)
>   			return SVC_CLOSE;
>   	}
> +	if (!svcxdr_set_accept_stat(rqstp))
> +		return SVC_CLOSE;
>   
>   	rqstp->rq_cred.cr_flavor = RPC_AUTH_TLS;
>   	return SVC_OK;
> @@ -960,6 +964,8 @@ svcauth_unix_accept(struct svc_rqst *rqstp)
>   	if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
>   					  RPC_AUTH_NULL, NULL, 0) < 0)
>   		return SVC_CLOSE;
> +	if (!svcxdr_set_accept_stat(rqstp))
> +		return SVC_CLOSE;
>   
>   	rqstp->rq_cred.cr_flavor = RPC_AUTH_UNIX;
>   	return SVC_OK;
> 
> 
> 

-- 
js
suse labs

Re: [PATCH v1 26/27] SUNRPC: Set rq_accept_statp inside ->accept methods

[Chuck Lever III]

> On May 2, 2023, at 7:01 AM, Jiri Slaby <jirislaby@kernel.org> wrote:
> 
> On 08. 01. 23, 17:31, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>> To navigate around the space that svcauth_gss_accept() reserves
>> for the RPC payload body length and sequence number fields,
>> svcauth_gss_release() does a little dance with the reply's
>> accept_stat, moving the accept_stat value in the response buffer
>> down by two words.
>> Instead, let's have the ->accept() methods each set the proper
>> final location of the accept_stat to avoid having to move
>> things.
> 
> Hi,
> 
> I bisected to this (4bcf0343e8)

Assuming you did the bisect on the NFS server's kernel?


> as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
> [nfsd]
> vers4=no

Note: Changing the settings in /etc/nfs.conf had no effect
on my server, so I effected the change by stopping the
server and poking values into /proc/fs/nfsd/versions by
hand.

Steve?


> The client sees:
>  mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
>  write(2, "mount.nfs: mount system call fai"..., 45
>  mount.nfs: mount system call failed for /mnt
> 
> And the kernel says:
>  nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
> 
> I reported in downstream as:
> https://bugzilla.suse.com/show_bug.cgi?id=1210995
> 
> It cannot be reverted cleanly on the top of 6.3.
> 
> Any ideas?

I can reproduce a similar problem. Network capture shows
that the server is responding with NFS4ERR_NOENT to the
EXCHANGE_ID operation, and the client kernel log says:

>  nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO

That's not the failure mode I expected given the commit
you bisected to, so it might not be the same problem you've
hit. I'll troubleshoot this and send a fix for testing.


> Thanks.
> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  include/linux/sunrpc/svc.h        |   19 +++++++++++++++++++
>>  net/sunrpc/auth_gss/svcauth_gss.c |   21 ++++++++++-----------
>>  net/sunrpc/svc.c                  |    2 --
>>  net/sunrpc/svcauth_unix.c         |    6 ++++++
>>  4 files changed, 35 insertions(+), 13 deletions(-)
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index f40a90ca5de6..392d2d2620fa 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -544,4 +544,23 @@ static inline void svcxdr_set_auth_slack(struct svc_rqst *rqstp, int slack)
>>   WARN_ON(xdr->p > xdr->end);
>>  }
>>  +/**
>> + * svcxdr_set_accept_stat - Reserve space for the accept_stat field
>> + * @rqstp: RPC transaction context
>> + *
>> + * Return values:
>> + *   %true: Success
>> + *   %false: No response buffer space was available
>> + */
>> +static inline bool svcxdr_set_accept_stat(struct svc_rqst *rqstp)
>> +{
>> + struct xdr_stream *xdr = &rqstp->rq_res_stream;
>> +
>> + rqstp->rq_accept_statp = xdr_reserve_space(xdr, XDR_UNIT);
>> + if (unlikely(!rqstp->rq_accept_statp))
>> + return false;
>> + *rqstp->rq_accept_statp = rpc_success;
>> + return true;
>> +}
>> +
>>  #endif /* SUNRPC_SVC_H */
>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
>> index 560fb8a2803d..333873abb7d9 100644
>> --- a/net/sunrpc/auth_gss/svcauth_gss.c
>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>> @@ -1220,7 +1220,7 @@ svcauth_gss_legacy_init(struct svc_rqst *rqstp,
>>   if (!svcauth_gss_proc_init_verf(sn->rsc_cache, rqstp, &rsip->out_handle,
>>   &rsip->major_status, GSS_SEQ_WIN))
>>   goto out;
>> - if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
>> + if (!svcxdr_set_accept_stat(rqstp))
>>   goto out;
>>   if (!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream, &rsip->out_handle,
>>   &rsip->out_token, rsip->major_status,
>> @@ -1348,7 +1348,7 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,
>>   if (!svcauth_gss_proc_init_verf(sn->rsc_cache, rqstp, &cli_handle,
>>   &ud.major_status, GSS_SEQ_WIN))
>>   goto out;
>> - if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
>> + if (!svcxdr_set_accept_stat(rqstp))
>>   goto out;
>>   if (!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream, &cli_handle,
>>   &ud.out_token, ud.major_status,
>> @@ -1640,16 +1640,18 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
>>   case RPC_GSS_PROC_DESTROY:
>>   if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
>>   goto auth_err;
>> + if (!svcxdr_set_accept_stat(rqstp))
>> + goto auth_err;
>>   /* Delete the entry from the cache_list and call cache_put */
>>   sunrpc_cache_unhash(sn->rsc_cache, &rsci->h);
>> - if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
>> - goto auth_err;
>>   goto complete;
>>   case RPC_GSS_PROC_DATA:
>>   rqstp->rq_auth_stat = rpcsec_gsserr_ctxproblem;
>>   svcdata->verf_start = xdr_reserve_space(&rqstp->rq_res_stream, 0);
>>   if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
>>   goto auth_err;
>> + if (!svcxdr_set_accept_stat(rqstp))
>> + goto auth_err;
>>   rqstp->rq_cred = rsci->cred;
>>   get_group_info(rsci->cred.cr_group_info);
>>   rqstp->rq_auth_stat = rpc_autherr_badcred;
>> @@ -1706,7 +1708,6 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
>>  static __be32 *
>>  svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)
>>  {
>> - struct xdr_buf *resbuf = &rqstp->rq_res;
>>   __be32 *p;
>>   u32 verf_len;
>>  @@ -1721,13 +1722,11 @@ svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)
>>   p += 1;
>>   verf_len = ntohl(*p++);
>>   p += XDR_QUADLEN(verf_len);
>> - /* move accept_stat to right place: */
>> - memcpy(p, p + 2, 4);
>> - /* Also don't wrap if the accept stat is nonzero: */
>> - if (*p != rpc_success) {
>> - resbuf->head[0].iov_len -= 2 * 4;
>> +
>> + /* Also don't wrap if the accept_stat is nonzero: */
>> + if (*rqstp->rq_accept_statp != rpc_success)
>>   return NULL;
>> - }
>> +
>>   p++;
>>   return p;
>>  }
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index 3c194e6f8f5e..c2ed8b06fadb 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -1314,8 +1314,6 @@ svc_process_common(struct svc_rqst *rqstp)
>>   trace_svc_process(rqstp, progp->pg_name);
>>     aoffset = xdr_stream_pos(xdr);
>> - rqstp->rq_accept_statp = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT);
>> - *rqstp->rq_accept_statp = rpc_success;
>>     /* un-reserve some of the out-queue now that we have a
>>    * better idea of reply size
>> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
>> index b101700d155c..62dfc8cdf8c5 100644
>> --- a/net/sunrpc/svcauth_unix.c
>> +++ b/net/sunrpc/svcauth_unix.c
>> @@ -775,6 +775,8 @@ svcauth_null_accept(struct svc_rqst *rqstp)
>>   if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
>>     RPC_AUTH_NULL, NULL, 0) < 0)
>>   return SVC_CLOSE;
>> + if (!svcxdr_set_accept_stat(rqstp))
>> + return SVC_CLOSE;
>>     rqstp->rq_cred.cr_flavor = RPC_AUTH_NULL;
>>   return SVC_OK;
>> @@ -866,6 +868,8 @@ svcauth_tls_accept(struct svc_rqst *rqstp)
>>     RPC_AUTH_NULL, NULL, 0) < 0)
>>   return SVC_CLOSE;
>>   }
>> + if (!svcxdr_set_accept_stat(rqstp))
>> + return SVC_CLOSE;
>>     rqstp->rq_cred.cr_flavor = RPC_AUTH_TLS;
>>   return SVC_OK;
>> @@ -960,6 +964,8 @@ svcauth_unix_accept(struct svc_rqst *rqstp)
>>   if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
>>     RPC_AUTH_NULL, NULL, 0) < 0)
>>   return SVC_CLOSE;
>> + if (!svcxdr_set_accept_stat(rqstp))
>> + return SVC_CLOSE;
>>     rqstp->rq_cred.cr_flavor = RPC_AUTH_UNIX;
>>   return SVC_OK;
> 
> -- 
> js
> suse labs


--
Chuck Lever

Re: [PATCH v1 26/27] SUNRPC: Set rq_accept_statp inside ->accept methods

[NeilBrown]

On Wed, 03 May 2023, Chuck Lever III wrote:
> 
> > On May 2, 2023, at 7:01 AM, Jiri Slaby <jirislaby@kernel.org> wrote:
> 
> > as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
> > [nfsd]
> > vers4=no
> 
> Note: Changing the settings in /etc/nfs.conf had no effect
> on my server, so I effected the change by stopping the
> server and poking values into /proc/fs/nfsd/versions by
> hand.
> 
> Steve?

Fixed in nfs-utils-2-3-4-rc1~7
  Commit: d68be5d6ae50 ("nfs.conf: fail to disable major NFS version 4 using "vers4=n" in /etc/nfs.conf")

NeilBrown

Re: [PATCH v1 26/27] SUNRPC: Set rq_accept_statp inside ->accept methods

[Jeff Layton]

[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]

On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
> 
> > On May 2, 2023, at 7:01 AM, Jiri Slaby <jirislaby@kernel.org> wrote:
> > 
> > On 08. 01. 23, 17:31, Chuck Lever wrote:
> > > From: Chuck Lever <chuck.lever@oracle.com>
> > > To navigate around the space that svcauth_gss_accept() reserves
> > > for the RPC payload body length and sequence number fields,
> > > svcauth_gss_release() does a little dance with the reply's
> > > accept_stat, moving the accept_stat value in the response buffer
> > > down by two words.
> > > Instead, let's have the ->accept() methods each set the proper
> > > final location of the accept_stat to avoid having to move
> > > things.
> > 
> > Hi,
> > 
> > I bisected to this (4bcf0343e8)
> 
> Assuming you did the bisect on the NFS server's kernel?
> 
> 
> > as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
> > [nfsd]
> > vers4=no
> 
> Note: Changing the settings in /etc/nfs.conf had no effect
> on my server, so I effected the change by stopping the
> server and poking values into /proc/fs/nfsd/versions by
> hand.
> 
> Steve?
> 
> 
> > The client sees:
> >  mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
> >  write(2, "mount.nfs: mount system call fai"..., 45
> >  mount.nfs: mount system call failed for /mnt
> > 
> > And the kernel says:
> >  nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
> > 
> > I reported in downstream as:
> > https://bugzilla.suse.com/show_bug.cgi?id=1210995
> > 
> > It cannot be reverted cleanly on the top of 6.3.
> > 
> > Any ideas?
> 
> I can reproduce a similar problem. Network capture shows
> that the server is responding with NFS4ERR_NOENT to the
> EXCHANGE_ID operation, and the client kernel log says:
> 
> >  nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
> 
> That's not the failure mode I expected given the commit
> you bisected to, so it might not be the same problem you've
> hit. I'll troubleshoot this and send a fix for testing.
> 

Alex hit this problem in testing too, and I took a quick look.

In the attached capture, the client should have gotten back a
RPC_PROG_MISMATCH error, but the server has recorded an extra successful
accept state before encoding the RPC_PROG_MISMATCH error, leading to a
malformed reply.

I think that the problem is that encoding the accept status too early
means that we can't properly handle failures from the pg_init_request
call.

Chuck, any thoughts on how you'd like to handle this?
-- 
Jeff Layton <jlayton@kernel.org>

[-- Attachment #2: bad-fallback.pcapng.gz --]
[-- Type: application/x-pcapng, Size: 2000 bytes --]

Re: [PATCH v1 26/27] SUNRPC: Set rq_accept_statp inside ->accept methods

[Chuck Lever III]

> On May 16, 2023, at 3:23 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
>> 
>>> On May 2, 2023, at 7:01 AM, Jiri Slaby <jirislaby@kernel.org> wrote:
>>> 
>>> On 08. 01. 23, 17:31, Chuck Lever wrote:
>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>> To navigate around the space that svcauth_gss_accept() reserves
>>>> for the RPC payload body length and sequence number fields,
>>>> svcauth_gss_release() does a little dance with the reply's
>>>> accept_stat, moving the accept_stat value in the response buffer
>>>> down by two words.
>>>> Instead, let's have the ->accept() methods each set the proper
>>>> final location of the accept_stat to avoid having to move
>>>> things.
>>> 
>>> Hi,
>>> 
>>> I bisected to this (4bcf0343e8)
>> 
>> Assuming you did the bisect on the NFS server's kernel?
>> 
>> 
>>> as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
>>> [nfsd]
>>> vers4=no
>> 
>> Note: Changing the settings in /etc/nfs.conf had no effect
>> on my server, so I effected the change by stopping the
>> server and poking values into /proc/fs/nfsd/versions by
>> hand.
>> 
>> Steve?
>> 
>> 
>>> The client sees:
>>> mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
>>> write(2, "mount.nfs: mount system call fai"..., 45
>>> mount.nfs: mount system call failed for /mnt
>>> 
>>> And the kernel says:
>>> nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
>>> 
>>> I reported in downstream as:
>>> https://bugzilla.suse.com/show_bug.cgi?id=1210995
>>> 
>>> It cannot be reverted cleanly on the top of 6.3.
>>> 
>>> Any ideas?
>> 
>> I can reproduce a similar problem. Network capture shows
>> that the server is responding with NFS4ERR_NOENT to the
>> EXCHANGE_ID operation, and the client kernel log says:
>> 
>>> nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
>> 
>> That's not the failure mode I expected given the commit
>> you bisected to, so it might not be the same problem you've
>> hit. I'll troubleshoot this and send a fix for testing.
>> 
> 
> Alex hit this problem in testing too, and I took a quick look.
> 
> In the attached capture, the client should have gotten back a
> RPC_PROG_MISMATCH error, but the server has recorded an extra successful
> accept state before encoding the RPC_PROG_MISMATCH error, leading to a
> malformed reply.
> 
> I think that the problem is that encoding the accept status too early
> means that we can't properly handle failures from the pg_init_request
> call.
> 
> Chuck, any thoughts on how you'd like to handle this?

With this:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-fixes&id=29cd2927fb914cc53b5ba4f67d2b74695c994ba4

I plan to send the fix to Linus tomorrow.


--
Chuck Lever

Re: [PATCH v1 26/27] SUNRPC: Set rq_accept_statp inside ->accept methods

[Jeff Layton]

On Tue, 2023-05-16 at 19:25 +0000, Chuck Lever III wrote:
> 
> > On May 16, 2023, at 3:23 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
> > > 
> > > > On May 2, 2023, at 7:01 AM, Jiri Slaby <jirislaby@kernel.org> wrote:
> > > > 
> > > > On 08. 01. 23, 17:31, Chuck Lever wrote:
> > > > > From: Chuck Lever <chuck.lever@oracle.com>
> > > > > To navigate around the space that svcauth_gss_accept() reserves
> > > > > for the RPC payload body length and sequence number fields,
> > > > > svcauth_gss_release() does a little dance with the reply's
> > > > > accept_stat, moving the accept_stat value in the response buffer
> > > > > down by two words.
> > > > > Instead, let's have the ->accept() methods each set the proper
> > > > > final location of the accept_stat to avoid having to move
> > > > > things.
> > > > 
> > > > Hi,
> > > > 
> > > > I bisected to this (4bcf0343e8)
> > > 
> > > Assuming you did the bisect on the NFS server's kernel?
> > > 
> > > 
> > > > as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
> > > > [nfsd]
> > > > vers4=no
> > > 
> > > Note: Changing the settings in /etc/nfs.conf had no effect
> > > on my server, so I effected the change by stopping the
> > > server and poking values into /proc/fs/nfsd/versions by
> > > hand.
> > > 
> > > Steve?
> > > 
> > > 
> > > > The client sees:
> > > > mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
> > > > write(2, "mount.nfs: mount system call fai"..., 45
> > > > mount.nfs: mount system call failed for /mnt
> > > > 
> > > > And the kernel says:
> > > > nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
> > > > 
> > > > I reported in downstream as:
> > > > https://bugzilla.suse.com/show_bug.cgi?id=1210995
> > > > 
> > > > It cannot be reverted cleanly on the top of 6.3.
> > > > 
> > > > Any ideas?
> > > 
> > > I can reproduce a similar problem. Network capture shows
> > > that the server is responding with NFS4ERR_NOENT to the
> > > EXCHANGE_ID operation, and the client kernel log says:
> > > 
> > > > nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
> > > 
> > > That's not the failure mode I expected given the commit
> > > you bisected to, so it might not be the same problem you've
> > > hit. I'll troubleshoot this and send a fix for testing.
> > > 
> > 
> > Alex hit this problem in testing too, and I took a quick look.
> > 
> > In the attached capture, the client should have gotten back a
> > RPC_PROG_MISMATCH error, but the server has recorded an extra successful
> > accept state before encoding the RPC_PROG_MISMATCH error, leading to a
> > malformed reply.
> > 
> > I think that the problem is that encoding the accept status too early
> > means that we can't properly handle failures from the pg_init_request
> > call.
> > 
> > Chuck, any thoughts on how you'd like to handle this?
> 
> With this:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-fixes&id=29cd2927fb914cc53b5ba4f67d2b74695c994ba4
> 
> I plan to send the fix to Linus tomorrow.
> 
> 

Oh! I hadn't seen that cross the list. Did I miss it?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1 26/27] SUNRPC: Set rq_accept_statp inside ->accept methods

[Chuck Lever III]

> On May 16, 2023, at 5:25 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Tue, 2023-05-16 at 19:25 +0000, Chuck Lever III wrote:
>> 
>>> On May 16, 2023, at 3:23 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
>>>> 
>>>>> On May 2, 2023, at 7:01 AM, Jiri Slaby <jirislaby@kernel.org> wrote:
>>>>> 
>>>>> On 08. 01. 23, 17:31, Chuck Lever wrote:
>>>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>>> To navigate around the space that svcauth_gss_accept() reserves
>>>>>> for the RPC payload body length and sequence number fields,
>>>>>> svcauth_gss_release() does a little dance with the reply's
>>>>>> accept_stat, moving the accept_stat value in the response buffer
>>>>>> down by two words.
>>>>>> Instead, let's have the ->accept() methods each set the proper
>>>>>> final location of the accept_stat to avoid having to move
>>>>>> things.
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> I bisected to this (4bcf0343e8)
>>>> 
>>>> Assuming you did the bisect on the NFS server's kernel?
>>>> 
>>>> 
>>>>> as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
>>>>> [nfsd]
>>>>> vers4=no
>>>> 
>>>> Note: Changing the settings in /etc/nfs.conf had no effect
>>>> on my server, so I effected the change by stopping the
>>>> server and poking values into /proc/fs/nfsd/versions by
>>>> hand.
>>>> 
>>>> Steve?
>>>> 
>>>> 
>>>>> The client sees:
>>>>> mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
>>>>> write(2, "mount.nfs: mount system call fai"..., 45
>>>>> mount.nfs: mount system call failed for /mnt
>>>>> 
>>>>> And the kernel says:
>>>>> nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
>>>>> 
>>>>> I reported in downstream as:
>>>>> https://bugzilla.suse.com/show_bug.cgi?id=1210995
>>>>> 
>>>>> It cannot be reverted cleanly on the top of 6.3.
>>>>> 
>>>>> Any ideas?
>>>> 
>>>> I can reproduce a similar problem. Network capture shows
>>>> that the server is responding with NFS4ERR_NOENT to the
>>>> EXCHANGE_ID operation, and the client kernel log says:
>>>> 
>>>>> nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
>>>> 
>>>> That's not the failure mode I expected given the commit
>>>> you bisected to, so it might not be the same problem you've
>>>> hit. I'll troubleshoot this and send a fix for testing.
>>>> 
>>> 
>>> Alex hit this problem in testing too, and I took a quick look.
>>> 
>>> In the attached capture, the client should have gotten back a
>>> RPC_PROG_MISMATCH error, but the server has recorded an extra successful
>>> accept state before encoding the RPC_PROG_MISMATCH error, leading to a
>>> malformed reply.
>>> 
>>> I think that the problem is that encoding the accept status too early
>>> means that we can't properly handle failures from the pg_init_request
>>> call.
>>> 
>>> Chuck, any thoughts on how you'd like to handle this?
>> 
>> With this:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-fixes&id=29cd2927fb914cc53b5ba4f67d2b74695c994ba4
>> 
>> I plan to send the fix to Linus tomorrow.
>> 
>> 
> 
> Oh! I hadn't seen that cross the list. Did I miss it?

https://lore.kernel.org/linux-nfs/8cd5d041-77c3-51dd-a960-7fd8ce1d1271@kernel.org/T/#t


--
Chuck Lever

Re: [PATCH v1 26/27] SUNRPC: Set rq_accept_statp inside ->accept methods

[Jeff Layton]

On Tue, 2023-05-16 at 21:27 +0000, Chuck Lever III wrote:
> 
> > On May 16, 2023, at 5:25 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Tue, 2023-05-16 at 19:25 +0000, Chuck Lever III wrote:
> > > 
> > > > On May 16, 2023, at 3:23 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > > > 
> > > > On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
> > > > > 
> > > > > > On May 2, 2023, at 7:01 AM, Jiri Slaby <jirislaby@kernel.org> wrote:
> > > > > > 
> > > > > > On 08. 01. 23, 17:31, Chuck Lever wrote:
> > > > > > > From: Chuck Lever <chuck.lever@oracle.com>
> > > > > > > To navigate around the space that svcauth_gss_accept() reserves
> > > > > > > for the RPC payload body length and sequence number fields,
> > > > > > > svcauth_gss_release() does a little dance with the reply's
> > > > > > > accept_stat, moving the accept_stat value in the response buffer
> > > > > > > down by two words.
> > > > > > > Instead, let's have the ->accept() methods each set the proper
> > > > > > > final location of the accept_stat to avoid having to move
> > > > > > > things.
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > I bisected to this (4bcf0343e8)
> > > > > 
> > > > > Assuming you did the bisect on the NFS server's kernel?
> > > > > 
> > > > > 
> > > > > > as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
> > > > > > [nfsd]
> > > > > > vers4=no
> > > > > 
> > > > > Note: Changing the settings in /etc/nfs.conf had no effect
> > > > > on my server, so I effected the change by stopping the
> > > > > server and poking values into /proc/fs/nfsd/versions by
> > > > > hand.
> > > > > 
> > > > > Steve?
> > > > > 
> > > > > 
> > > > > > The client sees:
> > > > > > mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
> > > > > > write(2, "mount.nfs: mount system call fai"..., 45
> > > > > > mount.nfs: mount system call failed for /mnt
> > > > > > 
> > > > > > And the kernel says:
> > > > > > nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
> > > > > > 
> > > > > > I reported in downstream as:
> > > > > > https://bugzilla.suse.com/show_bug.cgi?id=1210995
> > > > > > 
> > > > > > It cannot be reverted cleanly on the top of 6.3.
> > > > > > 
> > > > > > Any ideas?
> > > > > 
> > > > > I can reproduce a similar problem. Network capture shows
> > > > > that the server is responding with NFS4ERR_NOENT to the
> > > > > EXCHANGE_ID operation, and the client kernel log says:
> > > > > 
> > > > > > nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
> > > > > 
> > > > > That's not the failure mode I expected given the commit
> > > > > you bisected to, so it might not be the same problem you've
> > > > > hit. I'll troubleshoot this and send a fix for testing.
> > > > > 
> > > > 
> > > > Alex hit this problem in testing too, and I took a quick look.
> > > > 
> > > > In the attached capture, the client should have gotten back a
> > > > RPC_PROG_MISMATCH error, but the server has recorded an extra successful
> > > > accept state before encoding the RPC_PROG_MISMATCH error, leading to a
> > > > malformed reply.
> > > > 
> > > > I think that the problem is that encoding the accept status too early
> > > > means that we can't properly handle failures from the pg_init_request
> > > > call.
> > > > 
> > > > Chuck, any thoughts on how you'd like to handle this?
> > > 
> > > With this:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-fixes&id=29cd2927fb914cc53b5ba4f67d2b74695c994ba4
> > > 
> > > I plan to send the fix to Linus tomorrow.
> > > 
> > > 
> > 
> > Oh! I hadn't seen that cross the list. Did I miss it?
> 
> https://lore.kernel.org/linux-nfs/8cd5d041-77c3-51dd-a960-7fd8ce1d1271@kernel.org/T/#t
> 
> 

Ahh ok, it wasn't under its own title. LGTM. You can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>