Re: [PATCH 4/5] cifs: take reference to inode for oplock breaks

On Fri, 21 Aug 2009 10:18:16 -0500
Steve French <[email protected]> wrote:

> On Fri, Aug 21, 2009 at 5:36 AM, Jeff Layton <[email protected]> wrote:
> 
> > On Thu, 20 Aug 2009 19:42:42 -0500
> > Steve French <[email protected]> wrote:
> >
> > > Why don't we have a reference to this inode already?   We can't have an
> > > oplock break unless the file is open, if the file is open then we have a
> > > reference to the inode ...
> > >
> >
> > Well, you have a reference when the entry goes onto the list. There's
> > no guarantee that you'll still have one when cifs_oplock_thread goes to
> > take it off the list however. The file could have been closed by then
> >
> 
> An oplock response is handle based so we need to make sure the fid
> is valid (if not, throw away the oplock response, except for the case
> of batch oplock which we don't use yet).  Seems odd to lock the
> inode when we really need the fid (file struct)
> 

True. Though that's sort of a design issue with the oplock queue
itself. The entry tracks an inode and not a filp.

The goal with this set is to fix the use-after-free problems without
changing the underlying design too much. I'm more inclined to leave
this as-is until the whole approach can be redesigned.

> We could mark the file struct (similar to the write pending flag) and
> delete such an entry off the oplockq in close (if we are closing
> a file with an oplock pending)
> 

As long as you can do so in a non-racy way, then I'm not opposed to
that long-term. The problem though is that I don't have a lot of
confidence in the open file tracking code. It's extremely hard to
follow and definitely has races. I don't think it's really possible to
do what you suggest safely until the open file tracking code has been
fixed.

For now, I'm pretty sure this set should fix the problems that users
are hitting in the oplock codepath today. I'd like to fix that first
before we embark on a redesign of it.

-- 
Jeff Layton <[email protected]>