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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 4 Nov 2014 10:11:09 +0100
From:	David Herrmann <dh.herrmann@...il.com>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linux API <linux-api@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	John Stultz <john.stultz@...aro.org>,
	Arnd Bergmann <arnd@...db.de>, Tejun Heo <tj@...nel.org>,
	Marcel Holtmann <marcel@...tmann.org>,
	Ryan Lortie <desrt@...rt.ca>,
	Bastien Nocera <hadess@...ess.net>,
	Djalal Harouni <tixxdz@...ndz.org>,
	Simon McVittie <simon.mcvittie@...labora.co.uk>,
	Daniel Mack <daniel@...que.org>,
	Alban Crequy <alban.crequy@...labora.co.uk>,
	javier.martinez@...labora.co.uk, Tom Gundersen <teg@...m.no>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: 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 <viro@...iv.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/