Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 1 Mar 2021 13:15:01 -0500
From:   Bruce Fields <bfields@fieldses.org>
To:     Chuck Lever <chuck.lever@oracle.com>
Cc:     Daniel Kobras <kobras@puzzle-itc.de>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] sunrpc: fix refcount leak for rpc auth modules
Message-ID: <20210301181501.GC11772@fieldses.org>
References: <20210226230437.jfgagcq5magzlrtv@tuedko18.puzzle-itc.de>
 <C2704113-2581-4B58-806B-BB65148AC14B@oracle.com>
 <20210301162820.GB11772@fieldses.org>
 <F2DE890D-C2D7-476A-AF6F-949B105728E9@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <F2DE890D-C2D7-476A-AF6F-949B105728E9@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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.