> On Mar 1, 2021, at 1:15 PM, Bruce Fields <bfields@xxxxxxxxxxxx> 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
