On Mon, 2013-08-19 at 11:52 -0700, Hugh Dickins wrote:
> > > > We could use the semaphore instead, I believe, but I don't really
> > > > understand the mutex vs. semaphore well enough to be sure that's
> > > > correct.
> > I don't believe so, the semaphore and cb_mutex don't have a dependency
> > yet, afaict.
>
> The down_read(&cb_lock) patch you suggested gives the lockdep trace below.
Clearly I was wrong then, not sure what I missed in the code though.
>From the lockdep trace it looks like the dependency comes via the
genl_lock, I didn't consider that.
David, can you please just revert it for now and tag the revert for
stable as well, in case Greg already took it somewhere? I think that
some stable trees may need a different fix anyway since they don't have
parallel_ops.
We never saw a problem due to this, and though it's probably fairly easy
to trigger by hand-rolling the dump loop in userspace, one does need to
be able to rmmod to crash the kernel with it.
The only way to fix this that I see right now (that doesn't rewrite the
locking completely) would be to make genetlink use parallel_ops itself,
thereby removing the genl_lock() in genl_rcv_msg() and breaking all
those lock chains that lockdep reported. After that, it should be safe
to use genl_lock() inside all the operations. Something like the patch
below, perhaps? Completely untested so far.
johannes
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index f85f8a2..dea9586 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -665,6 +665,7 @@ static struct genl_family genl_ctrl = {
.version = 0x2,
.maxattr = CTRL_ATTR_MAX,
.netnsok = true,
+ .parallel_ops = true,
};
static int ctrl_fill_info(struct genl_family *family, u32 portid, u32 seq,
@@ -789,10 +790,8 @@ static int ctrl_dumpfamily(struct sk_buff *skb, struct netlink_callback *cb)
struct net *net = sock_net(skb->sk);
int chains_to_skip = cb->args[0];
int fams_to_skip = cb->args[1];
- bool need_locking = chains_to_skip || fams_to_skip;
- if (need_locking)
- genl_lock();
+ genl_lock();
for (i = chains_to_skip; i < GENL_FAM_TAB_SIZE; i++) {
n = 0;
@@ -814,8 +813,7 @@ errout:
cb->args[0] = i;
cb->args[1] = n;
- if (need_locking)
- genl_unlock();
+ genl_unlock();
return skb->len;
}
@@ -870,6 +868,8 @@ static int ctrl_getfamily(struct sk_buff *skb, struct genl_info *info)
struct genl_family *res = NULL;
int err = -EINVAL;
+ genl_lock();
+
if (info->attrs[CTRL_ATTR_FAMILY_ID]) {
u16 id = nla_get_u16(info->attrs[CTRL_ATTR_FAMILY_ID]);
res = genl_family_find_byid(id);
@@ -896,19 +896,25 @@ static int ctrl_getfamily(struct sk_buff *skb, struct genl_info *info)
}
if (res == NULL)
- return err;
+ goto out_unlock;
if (!res->netnsok && !net_eq(genl_info_net(info), &init_net)) {
/* family doesn't exist here */
- return -ENOENT;
+ err = -ENOENT;
+ goto out_unlock;
}
msg = ctrl_build_family_msg(res, info->snd_portid, info->snd_seq,
CTRL_CMD_NEWFAMILY);
- if (IS_ERR(msg))
- return PTR_ERR(msg);
+ if (IS_ERR(msg)) {
+ err = PTR_ERR(msg);
+ goto out_unlock;
+ }
- return genlmsg_reply(msg, info);
+ err = genlmsg_reply(msg, info);
+out_unlock:
+ genl_unlock();
+ return err;
}
static int genl_ctrl_event(int event, void *data)
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html