Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync() in exit/teardown

[Julia Lawall]

On Mon, 24 Mar 2014, Thomas Gleixner wrote:

> On Sun, 23 Mar 2014, Julia Lawall wrote:
> > As far as I could tell, (part of) the issue is that any kind of exit or 
> > close function should use del_timer_sync, because they could be called 
> > from a different CPU than the one that started up the timer?
> > 
> > Here is a semantic patch that takes care of the case of simple module_exit 
> > functions:
> > 
> > @r@
> > declarer name module_exit;
> > identifier ex;
> > @@
> > 
> > module_exit(ex);
> > 
> > @@
> > identifier r.ex;
> > @@
> > 
> > ex(...) {
> >   <...
> > - del_timer
> > + del_timer_sync
> >     (...)
> >   ...>
> > }
> > 
> > The transformations it makes are below.  I haven't had a chance to check 
> > which results overlap with what Thomas has already sent, but I could look 
> 
> Minimal overlap, but as I said I did just a few conversions.
> 
> > into it if this is the right idea.  I guess other kinds of close/exit 
> > functions would have to be identified manually, to make more rules.
> 
> If you look through the examples I sent, you'll find the close()
> callbacks of various devices. So everything which is a function
> pointer to a ops->close(), ops->remove(), ops_>teardown() is dangerous
> if using del_timer(). There are a few exceptions, but....
> 
> Another thing I saw is 
> 
> 	del_timer(&bla->timer);
> 	....
> 	kfree(&bla);
> 
> That's also a potential source of trouble. You can't tell without
> analyzing the code, whether it's a problem or not. But making the
> responsible people to look at it is definitely a good thing.
>  
> > In what circumstance can one be sure that two instructions are executed on 
> > the same CPU?
> 
> If interrupts or preemption are disabled. But that's not the issue at
> hand.
> 
> The del_timer vs. del_timer_sync problem is:
> 
> CPU0	      	  		 CPU1
> 
> add_timer(&bla->timer);
> 
> 				 close(bla)
> timer expires		   	   del_timer(&bla->timer);
>   callback is invoked
> 				   kfree(bla);
>     derefernces bla
> 
> I'll look at your findings on Tuesday, but feel free to send them to
> the relevant maintainers for review.

Thanks for all of the suggestions!

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/