Messages in this thread | | | Date | Tue, 4 Nov 2014 10:11:09 +0100 | Subject | Re: How Not To Use kref (was Re: kdbus: add code for buses, domains and endpoints) | From | David Herrmann <> |
| |
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
| |