Re: [GIT PULL] kdbus for 4.1-rc1

[Al Viro <viro@ZenIV.linux.org.uk>]

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.