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

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

> On Mar 1, 2021, at 1:15 PM, Bruce Fields <bfields@fieldses.org> wrote:
> 
> 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 this patch necessarily has to wait for that.

OK. It's in for-rc now, and sounds like that doesn't need to change.

Poking around a little, I see a try_module_get() and module_put() done
for every RPC. Considering that both have a preempt_disable/enable pair,
that seems a little expensive for the value it provides. One might like
to see the module reference count handled a little less frequently, but
I don't see an obvious way to address 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.

--
Chuck Lever