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

[Ira Weiny <ira.weiny@intel.com>]

On Thu, Nov 12, 2020 at 08:01:36PM -0400, Jason Gunthorpe wrote:
> 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

I agree.  IOW, if you are storing a pointer to something you should take a
reference to it.  Here you store 2 pointers but only take 1 reference and the
user has to 'know' the lifetime is the same...

> 
> > 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..

I'm not 100% on board that a helper call is an anti-pattern, it can help to ID
that users are 'reaching into' the notifier 'object'...  That said, in this
case I would not bother.  Also you defined other helper calls which reach into
the notifier...  so FWIW you were not consistent in this patch.  That flagged
my attention in the first place...   ;-)

Ira