Re: [PATCH for-rc v2] IB/hfi1: Move cached value of mm into handler

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Nov 12, 2020 at 05:06:30PM -0500, Dennis Dalessandro wrote:
> On 11/12/2020 12:14 PM, Ira Weiny wrote:
> > On Wed, Nov 11, 2020 at 09:58:37PM -0500, Dennis Dalessandro wrote:
> > > Two earlier bug fixes have created a security problem in the hfi1
> > > driver. One fix aimed to solve an issue where current->mm was not valid
> > > when closing the hfi1 cdev. It attempted to do this by saving a cached
> > > value of the current->mm pointer at file open time. This is a problem if
> > > another process with access to the FD calls in via write() or ioctl() to
> > > pin pages via the hfi driver. The other fix tried to solve a use after
> > > free by taking a reference on the mm. This was just wrong because its
> > > possible for a race condition between one process with an mm that opened
> > > the cdev if it was accessing via an IOCTL, and another process
> > > attempting to close the cdev with a different current->mm.
> > 
> > Again I'm still not seeing the race here.  It is entirely possible that the fix
> > I was trying to do way back was mistaken too...  ;-)  I would just delete the
> > last 2 sentences...  and/or reference the commit of those fixes and help
> > explain this more.
> 
> I was attempting to refer to [1], the email that started all of this.
> 
> > > 
> > > To fix this correctly we move the cached value of the mm into the mmu
> > > handler struct for the driver.
> > 
> > Looking at this closer I don't think you need the mm member of mmu_rb_handler
> > any longer.  See below.
> 
> We went back and forth on this as well. We thought it better to rely on our
> own pointer vs looking into the notifier to get the mm. Same reasoning for
> doing our own referecne counting. Question is what is the preferred way
> here. Functionally it makes no difference and I'm fine going either way.

Use the mm pointer in the notifier if you have a notifier registered,
it is clearer as to the lifetime and matches what other places do

> That's the question. It does make sense to do that if we are sticking iwth
> the notifier's reference vs our own explicit one. I'm not 100% sold that we
> should not be doing the ref counting and keeping our own pointer. To me we
> shoudln't be looking inside the notifer struct and instead honestly there
> should probably be an API/helper call to get the mm from it. I'm open to
> either approach.

The notifier is there to support users of the notifier, and nearly all
notifier users require the mm at various points.

Adding get accessors is a bit of a kernel anti-pattern, this isn't Java..

Jason