Re: [PATCH] sunrpc: fix refcount leak for rpc auth modules

[Chuck Lever <chuck.lever@oracle.com>]

> On Feb 26, 2021, at 6:04 PM, Daniel Kobras <kobras@puzzle-itc.de> wrote:
> 
> If an auth module's accept op returns SVC_CLOSE, svc_process_common()
> enters a call path that does not call svc_authorise() before leaving the
> function, and thus leaks a reference on the auth module's refcount. Hence,
> make sure calls to svc_authenticate() and svc_authorise() are paired for
> all call paths, to make sure rpc auth modules can be unloaded.
> 
> Fixes: 4d712ef1db05 ("svcauth_gss: Close connection when dropping an incoming message")
> Signed-off-by: Daniel Kobras <kobras@puzzle-itc.de>
> ---
> Hi!
> 
> While debugging NFS on a system with misconfigured krb5 settings, we noticed
> a suspiciously high refcount on the auth_rpcgss module, despite all of its
> consumers already unloaded. I wasn't able to analyze any further on the live
> system, but had a look at the code afterwards, and found a path that seems
> to leak references if the mechanism's accept() op shuts down a connection
> early. Although I couldn't verify, this seem to be a plausible fix.
> 
> Kind regards,
> 
> Daniel

Hi Daniel-

I've provisionally included your patch in my NFSD for-rc topic branch
here:

git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git

Your bug report seems plausible, but I need to take a closer look at that
code and your proposed change. Would very much like to hear from others,
too.


> net/sunrpc/svc.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 61fb8a18552c..d76dc9d95d16 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1413,7 +1413,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
> 
>  sendit:
> 	if (svc_authorise(rqstp))
> -		goto close;
> +		goto close_xprt;
> 	return 1;		/* Caller can now send it */
> 
> release_dropit:
> @@ -1425,6 +1425,8 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
> 	return 0;
> 
>  close:
> +	svc_authorise(rqstp);
> +close_xprt:
> 	if (rqstp->rq_xprt && test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags))
> 		svc_close_xprt(rqstp->rq_xprt);
> 	dprintk("svc: svc_process close\n");
> @@ -1433,7 +1435,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
> err_short_len:
> 	svc_printk(rqstp, "short len %zd, dropping request\n",
> 			argv->iov_len);
> -	goto close;
> +	goto close_xprt;
> 
> err_bad_rpc:
> 	serv->sv_stats->rpcbadfmt++;
> -- 
> 2.25.1
> 
> 
> -- 
> Puzzle ITC Deutschland GmbH
> Sitz der Gesellschaft: Eisenbahnstraße 1, 72072 
> Tübingen
> 
> Eingetragen am Amtsgericht Stuttgart HRB 765802
> Geschäftsführer: 
> Lukas Kallies, Daniel Kobras, Mark Pröhl
> 

--
Chuck Lever