netdev - Re: [Patch net-next] fib: move fib_rules_cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 31 Mar 2015 09:47:09 -0700
From:	Cong Wang <cwang@...pensource.com>
To:	Alexander Duyck <alexander.h.duyck@...hat.com>
Cc:	Thomas Graf <tgraf@...g.ch>, Cong Wang <xiyou.wangcong@...il.com>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [Patch net-next] fib: move fib_rules_cleanup_ops() under rtnl lock

On Mon, Mar 30, 2015 at 8:10 PM, Alexander Duyck
<alexander.h.duyck@...hat.com> wrote:
>
> On 03/30/2015 05:12 PM, Cong Wang wrote:
>>
>> On Mon, Mar 30, 2015 at 5:02 PM, Alexander Duyck
>> <alexander.h.duyck@...hat.com> wrote:
>>>
>>> On 03/30/2015 04:47 PM, Cong Wang wrote:
>>>>
>>>> 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.
>>>
>>>> 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.

>
>> 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).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html