lkml.org 
[lkml]   [2014]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectHow Not To Use kref (was Re: kdbus: add code for buses, domains and endpoints)
    On Wed, Oct 29, 2014 at 03:00:52PM -0700, Greg Kroah-Hartman wrote:

    > +static void __kdbus_domain_user_free(struct kref *kref)
    > +{
    > + struct kdbus_domain_user *user =
    > + container_of(kref, struct kdbus_domain_user, kref);
    > +
    > + BUG_ON(atomic_read(&user->buses) > 0);
    > + BUG_ON(atomic_read(&user->connections) > 0);
    > +
    > + mutex_lock(&user->domain->lock);
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    > + idr_remove(&user->domain->user_idr, user->idr);
    > + hash_del(&user->hentry);
    ^^^^^^^^^^^^^^^^^^^^^^^^
    > + mutex_unlock(&user->domain->lock);
    > +
    > + kdbus_domain_unref(user->domain);
    > + kfree(user);
    > +}

    > +struct kdbus_domain_user *kdbus_domain_user_unref(struct kdbus_domain_user *u)
    > +{
    > + if (u)
    > + kref_put(&u->kref, __kdbus_domain_user_free);
    > + return NULL;
    > +}

    If you remove an object from some search structures, taking the lock in
    destructor is Too Fucking Late(tm). Somebody might have already found
    that puppy and decided to pick it (all under that lock) just as we'd
    got to that point in destructor and blocked there. Oops...

    Normally I'd say "just use kref_put_mutex()", but this case is even worse.
    Look:

    refcount is 1
    A: kref_put_mutex()
    see that it's potential 1->0 crossing, need to take mutex
    mutex_lock()
    B: kref_get()
    refcount is 2
    A: got the sodding mutex
    atomic_dec_and_test
    refcount is 1 now
    OK, it's not 1->0, after all, just drop the mutex and bugger off
    B: kref_put_mutex()
    see that it's potential 1->0 crossing, need to take mutex
    mutex_lock() blocks
    A: mutex_unlock() lets B go
    B: ... got it
    atomic_dec_and_test
    refcount is 0
    call the destructor now, which ends with
    kdbus_domain_unref(user->domain);
    ... which just happens to be the last reference to ->domain
    ... and frees it, along with ->domain->mutex

    But what's to guarantee that A will be past the last point where mutex_unlock()
    is looking at the mutex? Sure, it's hard to hit, but AFAICS it's not
    impossible, especially if the following happens (assuming mutex-dec.h-style
    mutices):

    B: mutex_lock()
    atomic_dec_return -> -1
    __mutex_lock_slowpath()
    A: mutex_unlock()
    atomic_inc_return -> 0
    get preempted
    B: note that A has already incremented it to 0 and bugger off - we'd got it

    and there we go, with A getting the timeslice back and deciding to call
    __mutex_unlock_slowpath() when B has already freed the damn thing.

    Basically, kref_put_mutex() is only usable when destructor callback cannot end
    up freeing the mutex.

    kref_get_unless_zero() might be a usable approach, but IMO the whole thing is
    simply outside of kref applicability. Using it for something that needs to
    deal with removal from search structures from the destructor callback is
    already stretching the things; this one is far worse. kref isn't a universal
    tool for expressing lifetime cycles. It works for really simple cases and
    might eliminate some amount of boilerplate code. It's been greatly oversold
    and overused, though...


    \
     
     \ /
      Last update: 2014-10-31 01:01    [W:4.036 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site