lkml.org 
[lkml]   [2014]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: How Not To Use kref (was Re: kdbus: add code for buses, domains and endpoints)
From
Hi Al

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


\
 
 \ /
  Last update: 2014-11-04 10:21    [W:0.111 / U:1.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site