Re: How Not To Use kref (was Re: kdbus: add code for buses, domains and endpoints)

[David Herrmann]

Hi Al

On Fri, Oct 31, 2014 at 12:38 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> 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...

Nice catch! I fixed it up via kref_get_unless_zero(). This has the
side-effect that there might be multiple domain_user objects for the
same user, but all but one will have ref==0. They don't carry and
valuable data in those cases, so we're fine. We will just end up using
the next one, or creating a new one.

Thanks a lot!
David
--
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/