On Mon, Mar 30, 2015 at 8:10 PM, Alexander Duyck
[off-list ref] wrote:

On 03/30/2015 05:12 PM, Cong Wang wrote:

quoted

On Mon, Mar 30, 2015 at 5:02 PM, Alexander Duyck
[off-list ref] wrote:

quoted

On 03/30/2015 04:47 PM, Cong Wang wrote:

quoted

As long as we agree rtnl lock should be taken, you already take my point
here ($subject says so).

Yes, I agree lock can be held.  For fib4 it was already holding the RTNL
lock when it made that call.  You can update the other users of
fib_rules_unregister so that they call it with the RTNL lock held as
well.

quoted

It is just API change to move rtnl_lock up to caller or whatever
appropriate.

Right, so like I said for fib4 this is resolved.  That just leaves ipmr,
ip6mr, fib6, and dn_rules that need to be updated so that they correctly
handle the RTNL locking in their exit/cleanup paths. Since you already
have
some related patches out for these I will let you take them otherwise I
might try to go through and clean them up next week.

Ok, then we are finally on the same page. We need two patches:

1) move unregister under rtnl lock (as what this patch intended to do)

Yes I think the only disagreement was on how to do it.  Your original patch
placed it in fib_rules_cleanup_ops, and the preference of myself and Thomas
was to hold the lock before you even call fib_rules_unregister.  So the IPv4
code is fine with the patch I submitted, it is the other callers of
fib_rules_unregister which must be updated.

I never say your patch is not fine (except on the ops->delete issue which is
not related here) to fix it, as I said your patch only fixes one of the problems
I saw, that is it.

quoted

2) remove the unnecessary rules_mod_lock

Thanks.

Please define "unnecessary" as we have had a bit of back and forth on how
our views can differ there.  As far as I know it still has to be held for
the fib_rule_ops list manipulation, specifically the call to list_del_rcu.
However, it doesn't need to be held when we call fib_rules_cleanup_ops.

Look at where rules_mod_lock are held: either when the net is initialized
or when unregistering, neither of them really needs this per netns lock:
new netns is not ready to expose;
concurrent unregistering is prevented by upper layer locking,
readers (lookup_rules_ops) hold RCU but we already should hold rtnl lock
(after patch of course).