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

[Jeff Layton <jlayton@kernel.org>]

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>