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

Jiri Slaby <jirislaby@xxxxxxxxxx> · Tue, 2 May 2023 13:01:51 +0200

On 08. 01. 23, 17:31, Chuck Lever wrote:
From: Chuck Lever <chuck.lever@xxxxxxxxxx>

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@xxxxxxxxxx>
---
  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







