Re: [GIT PULL] kdbus for 4.1-rc1

[David Herrmann]

Hi

On Thu, Apr 16, 2015 at 10:55 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/