Re: [patch] prevent slab corruption by fixing race codition in cifs

On Wed, 26 Aug 2009 06:59:17 -0400
Jeff Layton <[email protected]> wrote:

> On Tue, 25 Aug 2009 16:23:35 -0500
> Dave Kleikamp <[email protected]> wrote:
> 
> > On Tue, 2009-08-18 at 12:43 -0400, Jeff Layton wrote:
> > > On Tue, 18 Aug 2009 10:23:09 -0500
> > > Shirish Pargaonkar <[email protected]> wrote:
> > > 
> > > > On Tue, Aug 18, 2009 at 6:15 AM, Jeff Layton<[email protected]> wrote:
> > 
> > > > > I'm less than thrilled with this patch. This looks like like it's
> > > > > layering more complexity onto a codepath that is already far too
> > > > > complex for what it does.
> > > > >
> > > > > Is it not possible to just use regular old refcounting for the open
> > > > > filehandles? i.e. have each user of the filehandle take a reference,
> > > > > and the last one to put it does the actual close. That seems like a
> > > > > much better approach to me than all of this crazy flag business.
> > > > >
> > > > 
> > > > I think this needs, some re-designing the code right?
> > > > 
> > > 
> > > My vote would be "yes". Redesign and simplify the code rather than
> > > adding in new hacks to work around the flaws in the existing design.
> > > 
> > > Redesigning it with actual refcounting (and not this half-assed
> > > wrtPending stuff) seems like a much better approach.
> > 
> > How's this?  Untested so far:
> > 
> > cifs: Replace wrtPending with a real reference count
> > 
> > Currently, cifs_close() tries to wait until all I/O is complete and then
> > frees the file private data.  If I/O does not completely in a reasonable
> > amount of time it frees the structure anyway, leaving a potential use-
> > after-free situation.
> > 
> > This patch changes the wrtPending counter to a complete reference count and
> > lets the last user free the structure.
> > 
> > WARNING: compile tested only at this time
> > 
> > Signed-off-by: Dave Kleikamp <[email protected]>
> > 
> 
> Follow up to yesterday's email.
> 
> As I said yesterday, this is a step in the right direction, but there's
> still room for improvement in this area:
> 
> 1) rather than having cifs_close wait for all of the users of the
> filehandle to finish, should we just have the last user issue the close
> on the wire? cifsFileInfo is really just a representation of an open
> filehandle on the server. CIFS should probably just create them and
> attach VFS filehandles to them on an as-needed basis.
> 
> 2) find_readable_file and find_writable_file could be consolidated.
> Rather than finding and matching filehandles based on the f_flags in
> the filp, it seems like it would be preferable to track the actual
> flags that were sent to the server and have an interface that searches
> for open filehandles based on those. That might also allow us to reduce
> the number of open filehandles in use. When a VFS open call comes in,
> we could search for an existing filehandle that has the right flags and
> just bump the refcount instead of doing a new open if one is found.
> 
> Some other comments below...
> 

Let me be clear here...

This code is still an improvement and will likely fix the
use-after-free that Shirish identified. Assuming that it tests out OK,
I'm inclined to take this patch in the near term and then plan an
overhaul of the open filehandle code in the future.

Sound reasonable?
-- 
Jeff Layton <[email protected]>