Re: [GIT PULL] kdbus for 4.1-rc1

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

On Wed, Apr 15, 2015 at 11:09:48AM +0200, Greg Kroah-Hartman wrote:

> I've asked for it, but finding people to review code is hard, as you
> know.  It's only 13k lines long, smaller than a serial port driver (my
> unit of code review), so it's not all that big.
> 
> It's smaller than the USB3 host controller driver as well, and very few
> people ever reviewed that beast :)
> 
> > For something that's potentially such a core mechanism as a completely 
> > new, massively-adopted IPC, this does send a warning singal.
> 
> If you know of a way to force others to review code, please let me know.

Have it in a less nasty state, perhaps?  Random question:

al@duke:~/linux/trees/vfs$ git grep -n -w kdbus_node_idr_lock
ipc/kdbus/node.c:237:static DECLARE_RWSEM(kdbus_node_idr_lock);
ipc/kdbus/node.c:340:   down_write(&kdbus_node_idr_lock);
ipc/kdbus/node.c:344:   up_write(&kdbus_node_idr_lock);
ipc/kdbus/node.c:444:           down_write(&kdbus_node_idr_lock);
ipc/kdbus/node.c:452:           up_write(&kdbus_node_idr_lock);

Do you see anything wrong with that?  Or with things like that:
                mutex_lock(&pos->lock);
                v_pre = atomic_read(&pos->active);
                if (v_pre >= 0)
                        atomic_add_return(KDBUS_NODE_BIAS, &pos->active);
                else if (v_pre == KDBUS_NODE_NEW)
                        atomic_set(&pos->active, KDBUS_NODE_RELEASE_DIRECT);
                mutex_unlock(&pos->lock);
What are the locking rules for ->active/->waitq/->lock?  Are those the
outermost thing in the hierarchy?  Or is that dependent on the node location?
It sure as hell is outside of (at least) ->mmap_sem (by way of
kdbus_conn_connect() establishing that ->active/->waitq is outside of
->conn_rwlock, which due to kdbus_bus_broadcast() nests outside of anything
taken by kdbus_meta_proc_collect(), which includes ->mmap_sem) and that alone
brings in a lot...

Document your goddamn locking, would you?  It *IS* new code, and you, as you
say, had very few people working on it, so you don't have the excuses for
the mess existing in older parts of the tree.

Locking complexity in there is easily as bad as that of VFS sans the RCU fun;
sure, I can spend a week and (hopefully) document it for you, but I would
really prefer if you guys had done that.  And I *do* appreciate the comments
in node.c, but they are nowhere near enough.

Tracking the call chains in there and trying to derive the locking ordering
from those is quite a bit of work; _verifying_ that it matches the claimed
one would be expected from reviewers, but as it is you are asking to spend
a lot of efforts to close the gaps in your documentation.  Sheesh...