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

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

Much better. Looks like a step in the right direction.

-- Jeff