On Mon, Mar 01, 2021 at 03:20:24PM +0000, Chuck Lever wrote:
>
> > On Feb 26, 2021, at 6:04 PM, Daniel Kobras <kobras@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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.
So, the effect of this is to call svc_authorise more often. I think
that's always safe, because svc_authorise is a no-op unless rq_authops
is set, it clears rq_authops itself, and rq_authops being set is a
guarantee that ->accept() already ran.
It's harder to know if this solves the problem, as I see a lot of other
mentions of THIS_MODULE in svcauth_gss.c.
Possibly orthogonal to this problem, but: svcauth_gss_release
unconditionally dereferences rqstp->rq_auth_data. Isn't that a NULL
dereference if the kmalloc at the start of svcauth_gss_accept() fails?
Finally, should we care about module reference leaks? Does anyone
really *need* to unload modules? And will bad stuff happen when the
count overflows, or does the module code fail safely somehow in the
overflow case? I know, bugs are bugs, I should care about fixing all of
them, shame on me....
--b.
>
>
> > 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
>
>
>
