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

Commit Message

Chuck Lever Jan. 8, 2023, 4:31 p.m. UTC

From: Chuck Lever <[e-mail]>

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 <[e-mail]>
---
 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(-)

Comments

Jiri Slaby May 2, 2023, 11:01 a.m. UTC | #1

On 08. 01. 23, 17:31, Chuck Lever wrote:
> From: Chuck Lever <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>   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;
> 
> 
>

Chuck Lever III May 2, 2023, 2:14 p.m. UTC | #2

> On May 2, 2023, at 7:01 AM, Jiri Slaby <[e-mail]> wrote:
> 
> On 08. 01. 23, 17:31, Chuck Lever wrote:
>> From: Chuck Lever <[e-mail]>
>> 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 <[e-mail]>
>> ---
>>  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

NeilBrown May 2, 2023, 9:29 p.m. UTC | #3

On Wed, 03 May 2023, Chuck Lever III wrote:
> 
> > On May 2, 2023, at 7:01 AM, Jiri Slaby <[e-mail]> 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

Jeff Layton May 16, 2023, 7:23 p.m. UTC | #4

On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
> 
> > On May 2, 2023, at 7:01 AM, Jiri Slaby <[e-mail]> wrote:
> > 
> > On 08. 01. 23, 17:31, Chuck Lever wrote:
> > > From: Chuck Lever <[e-mail]>
> > > 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?

Chuck Lever III May 16, 2023, 7:25 p.m. UTC | #5

> On May 16, 2023, at 3:23 PM, Jeff Layton <[e-mail]> wrote:
> 
> On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
>> 
>>> On May 2, 2023, at 7:01 AM, Jiri Slaby <[e-mail]> wrote:
>>> 
>>> On 08. 01. 23, 17:31, Chuck Lever wrote:
>>>> From: Chuck Lever <[e-mail]>
>>>> 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

Jeff Layton May 16, 2023, 9:25 p.m. UTC | #6

On Tue, 2023-05-16 at 19:25 +0000, Chuck Lever III wrote:
> 
> > On May 16, 2023, at 3:23 PM, Jeff Layton <[e-mail]> wrote:
> > 
> > On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
> > > 
> > > > On May 2, 2023, at 7:01 AM, Jiri Slaby <[e-mail]> wrote:
> > > > 
> > > > On 08. 01. 23, 17:31, Chuck Lever wrote:
> > > > > From: Chuck Lever <[e-mail]>
> > > > > 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?

Chuck Lever III May 16, 2023, 9:27 p.m. UTC | #7

> On May 16, 2023, at 5:25 PM, Jeff Layton <[e-mail]> wrote:
> 
> On Tue, 2023-05-16 at 19:25 +0000, Chuck Lever III wrote:
>> 
>>> On May 16, 2023, at 3:23 PM, Jeff Layton <[e-mail]> wrote:
>>> 
>>> On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
>>>> 
>>>>> On May 2, 2023, at 7:01 AM, Jiri Slaby <[e-mail]> wrote:
>>>>> 
>>>>> On 08. 01. 23, 17:31, Chuck Lever wrote:
>>>>>> From: Chuck Lever <[e-mail]>
>>>>>> 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/[e-mail]/T/#t


--
Chuck Lever

Jeff Layton May 16, 2023, 10:28 p.m. UTC | #8

On Tue, 2023-05-16 at 21:27 +0000, Chuck Lever III wrote:
> 
> > On May 16, 2023, at 5:25 PM, Jeff Layton <[e-mail]> wrote:
> > 
> > On Tue, 2023-05-16 at 19:25 +0000, Chuck Lever III wrote:
> > > 
> > > > On May 16, 2023, at 3:23 PM, Jeff Layton <[e-mail]> wrote:
> > > > 
> > > > On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
> > > > > 
> > > > > > On May 2, 2023, at 7:01 AM, Jiri Slaby <[e-mail]> wrote:
> > > > > > 
> > > > > > On 08. 01. 23, 17:31, Chuck Lever wrote:
> > > > > > > From: Chuck Lever <[e-mail]>
> > > > > > > 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/[e-mail]/T/#t
> 
> 

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

Reviewed-by: Jeff Layton <[e-mail]>

Patch

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;