Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932080AbbDRLoH (ORCPT ); Sat, 18 Apr 2015 07:44:07 -0400 Received: from mail-qk0-f171.google.com ([209.85.220.171]:34168 "EHLO mail-qk0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752291AbbDRLoD (ORCPT ); Sat, 18 Apr 2015 07:44:03 -0400 MIME-Version: 1.0 In-Reply-To: <20150416205557.GX889@ZenIV.linux.org.uk> References: <20150414175019.GA2874@kroah.com> <20150414192357.GA6107@kroah.com> <20150414193533.GF889@ZenIV.linux.org.uk> <20150414194348.GA7540@kroah.com> <552EA700.7000200@gmail.com> <20150415232218.7df214ba@lxorguk.ukuu.org.uk> <20150416205557.GX889@ZenIV.linux.org.uk> Date: Sat, 18 Apr 2015 13:44:02 +0200 Message-ID: Subject: Re: [GIT PULL] kdbus for 4.1-rc1 From: David Herrmann To: Al Viro Cc: Greg Kroah-Hartman , Jiri Kosina , Borislav Petkov , Andy Lutomirski , Linus Torvalds , Andrew Morton , Arnd Bergmann , "Eric W. Biederman" , One Thousand Gnomes , Tom Gundersen , "linux-kernel@vger.kernel.org" , Daniel Mack , Djalal Harouni Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5642 Lines: 138 Hi On Thu, Apr 16, 2015 at 10:55 PM, Al Viro 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/