Re: [GIT PULL] kdbus for 4.1-rc1

David Herrmann
2015-04-18T11:44:07Z

Hi

On Thu, Apr 16, 2015 at 10:55 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 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.

Fixed! [1]
Linus just pulled akpm#3, which includes the rcu-protection for
exe-file. No more direct mmap_sem access in kdbus, anymore.

>> 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.

Indeed. We do allow pinning parent objects when deactivating its
children. I updated my doc-drafts accordingly.

>> 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...

I have a patch to fix this [2]. But it's more complex than the rwsem,
and requires some more review. However, it reduces the handle-locking
to a minimum, such that we only lock it during setup and can reduce it
to a mutex.

>> * 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.

Sorry, this was a mistake on my side. We do call kdbus_notify_flush()
directly quite often. And it nests underneath the handle, correct. I
noted this down.

I did have patches to actually move the kdbus_notify_flush() call to
the end of kdbus_handle_ioctl() and friends. Such so we flush all
collected notifications on return to user-space, which would make the
locking more obvious. However, it didn't make it much simpler, imo, so
it was never applied.

> 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?

(Updated) Data-structure locks:
* bus->notify_lock
* pool->lock
* match->mdb_rwlock
* node->lock

Updated locking order:
1) handle->rwlock
2) bus->notify_flush_lock
3) domain->lock
4) names->rwlock
5) endpoint->lock
6) bus->conn_rwlock
7) policy->entries_rwlock
8) connection->lock
9) metadata->lock

* node->active read-side locks arbitrarily underneath handle->rwlock.

* node->active write-side nests underneath handle->rwlock, and
underneath read-side of any parent-node->active.

Thanks! Much appreciated!
David

[1] http://cgit.freedesktop.org/~dvdhrm/linux/commit/?h=kdbus&id=f396c12ecfda1717e5f76d6b4ab11e4db232e60d
[2] http://cgit.freedesktop.org/~dvdhrm/linux/commit/?h=kdbus&id=61875e1abd38a965c9f7dfca28068dd0a871961c