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

Hi Al

On Fri, Oct 31, 2014 at 12:38 AM, Al Viro [off-list ref] wrote:

On Wed, Oct 29, 2014 at 03:00:52PM -0700, Greg Kroah-Hartman wrote:

quoted

+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);

        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

quoted

+     idr_remove(&user->domain->user_idr, user->idr);
+     hash_del(&user->hentry);

        ^^^^^^^^^^^^^^^^^^^^^^^^

quoted

+     mutex_unlock(&user->domain->lock);
+
+     kdbus_domain_unref(user->domain);
+     kfree(user);
+}

quoted

+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