linux-kernel - Re: [GIT PULL] kdbus for 4.1-rc1

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]

Message-ID: <20150416205557.GX889@ZenIV.linux.org.uk>
Date:	Thu, 16 Apr 2015 21:55:57 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	David Herrmann <dh.herrmann@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Kosina <jkosina@...e.cz>, Borislav Petkov <bp@...en8.de>,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Arnd Bergmann <arnd@...db.de>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Tom Gundersen <teg@...m.no>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Daniel Mack <daniel@...que.org>,
	Djalal Harouni <tixxdz@...ndz.org>
Subject: Re: [GIT PULL] kdbus for 4.1-rc1

On Thu, Apr 16, 2015 at 07:31:22PM +0200, David Herrmann wrote:

> I'm working on patches to add more comments similar to how we did in
> node.c. For now, please see my explanations below:
> 
> node->lock is the _innermost_ lock.
> node->active implements revoke
> support for nodes. It follows what kernfs->active does and isn't a
> lock in particular. We kinda treat it as rwsem, where down_write() is
> the outer-most lock in kdbus and _only_ called without any other lock
> held (kdbus_node_deactivate()). Read-side, we never ever block on the
> "lock", but only use try-lock. If it fails, the node is dead/revoked.
> Therefore, the read-side of 'active' nests almost arbitrarily. We hold
> 'active'-references almost everywhere, to make sure a node is not
> destroyed while we use it. However, we never sleep for an indefinite
> time while holding it.

Umm...  Theoretically, but ->mmap_sem being under it means that it might
involve something like an NFS server timing out, so the latency might
suck very badly.

> Given that the write-side is the outer-most lock in kdbus, it doesn't
> dead-lock against the try-lock readers.

Huh?  I see at least this call chain:
kdbus_handle_ioctl_control()
	kdbus_node_acquire()
	kdbus_cmd_bus_make()
		kdbus_node_deactivate()
Granted, it won't be the _same_ node (otherwise you'd deadlock solid
right there and then), but it means that your locking order is sensitive
to something about nodes; it's not entirely determined by the lock type.

> Locking order (outer-most to inner-most):
>  1) domain->lock
>  2) names->rwlock
>  3) endpoint->lock
>  4) bus->conn_rwlock
>  5) policy->entries_rwlock
>  6) connection->lock
>  7) metadata->lock
> 
> mmap_sem nests below metadata->lock. With the rcu-protected exe_file
> patches by Davidlohr Bueso, we can even drop that dependency. They
> have kinda stalled, though.
> 
> Then we have a bunch of data structure protection, which can be called
> from any context:
>  * bus->notify_lock
>  * pool->lock
>  * match->mdb_rwlock
>  * node->lock
> 
> Lastly, there're 2 locks which nest around everything and must not be
> taken with any lock held:
>  * handle->rwlock (taken in ioctl-entry)

as well as in ->poll(), for completeness sake.  The latter, BTW, isn't
nice - kdbus is far from being the only thing that does it, but having
->poll() block can be somewhat surprising...

>  * bus->notify_flush_lock (taken in work-queue)

Hmm...  That needs some care - it means that it nests inside anything held
by callers of cancel_delayed_work_sync() on the corresponding work.  AFAICS,
there's at least one call chain leading to that from kdbus_node_deactivate()
(via ->release_cb == kdbus_ep_release -> kdbus_conn_disconnect ->
cancel_delayed_work_sync(&conn->work)) wait for kdbus_reply_list_scan_work
-> kdbus_notify_flush grabs ->notify_flush_lock).  Tracking back further is
harder - not all call sites of kdbus_node_deactivate() can lead to that...

BTW, it's not only done in wq callbacks - there's a direct chain from
kdbus_conn_disconnect() as well (both through kdbus_name_release_all ->
kdbus_notify_flush and directly through kdbus_notify_flush()).  And from
ioctl(), by many paths, while we are at it, but that only means that it
nests inside handle->rwlock, and _that_ is really the outermost.

What nests inside that one?  It definitely a part of hierarchy - it can't
be excluded from deadlock analysis as effectively outermost.  As for the
stuff under it...  registry->rwlock is obvious, what else?

> General object stacking is:
> domain -> bus -> endpoint -> policy -> connection -> {metadata,pool,match,node}
> The conn_rwlock protection of the conn-list locks on kdbus_bus is the
> only lock that doesn't follow this ordering.
--
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/