Re: [PATCH 2/2] genetlink: convert family dump code to use RCU

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2013-08-21 at 21:32 -0700, Pravin Shelar wrote:

> I send out a fix which fixes genl locking for dump operation and it
> might fix this issue, Can you try this?

I don't think that fix is correct. I don't have a copy, so let me reply
here.

> http://marc.info/?l=linux-netdev&m=137714389705485&w=2

> In case of genl-family with parallel ops off, dumpif() callback
> is expected to run under genl_lock, 
> But commit def3117493eafd9df
> (genl: Allow concurrent genl callbacks.) changed this behaviour
> where only first dumpit() op was called under genl-lock.

I don't think either of those statements are true - dump() has alway
taken a shortcut in netlink_recvmsg() and doesn't go into genl_rcv(), so
how could your patch have changed locking? Therefore, it can't have been
expected either.

> For subsequent dump, only nlk->cb_lock was taken.
> Following patch fixes it by defining locked dumpit() and done()
> callback which takes care of genl-locking.

This might help for generic netlink itself, but I'm not convinced that
it's really useful for other families. I've fixed bugs like in commit
3a5a423bb958ad22eeccca66c533e85bf69ba10e, which actually made it in
after your commit, but per above I don't think it was actually a problem
introduced by your commit.

johannes