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

[Bruce Fields <bfields@fieldses.org>]

On Mon, Mar 01, 2021 at 05:44:02PM +0000, Chuck Lever wrote:
> > 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.
> 
> Perhaps a deeper audit is necessary.
> 
> A small code change to inject SVC_CLOSE returns at random would enable
> a more dynamic analysis.

That might be interesting.

I don't think tihs patch necessarily has to wait for that.

> > 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?
> 
> I would prefer that module reference counting work as expected. When it
> doesn't that tends to lead to people (say, me) hunting for bugs that
> might actually be serious.
> 
> 
> > Does anyone really *need* to unload modules?
> 
> Anyone who wants to replace the module with a newer build that fixes a
> bug. It avoids a full reboot, and for some that's important.

Fair enough.

--b.