Re: [patch 1/4] kref: warn kref_put() with unreferenced kref

[Greg KH <greg@kroah.com>]

On Tue, Apr 25, 2006 at 12:19:15AM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Mon, 24 Apr 2006 20:51:28 PDT, Greg KH said:
> > On Mon, Apr 24, 2006 at 04:33:34PM +0800, Akinobu Mita wrote:
> 
> > > --- 2.6-git.orig/lib/kref.c
> > > +++ 2.6-git/lib/kref.c
> > > @@ -49,6 +49,7 @@ void kref_get(struct kref *kref)
> > >   */
> > >  int kref_put(struct kref *kref, void (*release)(struct kref *kref))
> > >  {
> > > +	WARN_ON(atomic_read(&kref->refcount) < 1);
> > 
> > How can this ever be true?  If the refcount _ever_ goes below 1, the
> > object is freed.
> 
> Maybe it should BUG_ON instead in that case. ;)
> 
> And strictly speaking, as long as the kref.c stuff is the only stuff to
> play with ->refcount, that *should* be true.  On the other hand, if somebody
> has a bad pointer that just did a fandango on core, it would be a nice thing
> to know that.  Looking at the *next* few lines:
> 
>         if ((atomic_read(&kref->refcount) == 1) ||
>             (atomic_dec_and_test(&kref->refcount))) {
>                 release(kref); 
>                 return 1; 
>         }    
>         return 0;
> 
> If we managed to get a -1 smashed in there, this won't actually trigger
> for another 2**32-2 or so kref_put calls - the first test is for "exactly 1",
> and the dec_and_test is for "exactly zero"...

Those two lines were recently added to make this test faster.  See the
archives for details.  If you are really worried about some memory
getting clobbered in there, we should worry about this for the entire
kernel :)

thanks,

greg k-h