netdev - Re: [patch net-next v2 03/12] ipmr: Add FIB notification access functions

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]

Message-ID: <9360a6b5-c7e6-ec77-5efd-5dfc0a1a7848@cumulusnetworks.com>
Date:   Mon, 25 Sep 2017 12:59:07 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, yotamg@...lanox.com,
        idosch@...lanox.com, mlxsw@...lanox.com, andrew@...n.ch
Subject: Re: [patch net-next v2 03/12] ipmr: Add FIB notification access
 functions

On 25/09/17 12:47, Jiri Pirko wrote:
> Mon, Sep 25, 2017 at 11:40:16AM CEST, nikolay@...ulusnetworks.com wrote:
>> On 25/09/17 12:35, Nikolay Aleksandrov wrote:
>>> On 24/09/17 20:22, Jiri Pirko wrote:
>>>> From: Yotam Gigi <yotamg@...lanox.com>
>>>>
>>>> Make the ipmr module register as a FIB notifier. To do that, implement both
>>>> the ipmr_seq_read and ipmr_dump ops.
>>>>
>>>> The ipmr_seq_read op returns a sequence counter that is incremented on
>>>> every notification related operation done by the ipmr. To implement that,
>>>> add a sequence counter in the netns_ipv4 struct and increment it whenever a
>>>> new MFC route or VIF are added or deleted. The sequence operations are
>>>> protected by the RTNL lock.
>>>>
>>>> The ipmr_dump iterates the list of MFC routes and the list of VIF entries
>>>> and sends notifications about them. The entries dump is done under RCU
>>>> where the VIF dump uses the mrt_lock too, as the vif->dev field can change
>>>> under RCU.
>>>>
>>>> Signed-off-by: Yotam Gigi <yotamg@...lanox.com>
>>>> Reviewed-by: Ido Schimmel <idosch@...lanox.com>
>>>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>>>> ---
>>>> v1->v2:
>>>>  - Take the mrt_lock when dumping VIF entries.
>>>> ---
>>>>  include/linux/mroute.h   |  15 ++++++
>>>>  include/net/netns/ipv4.h |   3 ++
>>>>  net/ipv4/ipmr.c          | 137 ++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  3 files changed, 153 insertions(+), 2 deletions(-)
>>>>
>>>
>>> LGTM,
>>>
>>> Reviewed-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
>>>
>>>
>>
>> One note here if you're going to spin another version of the set, you can
>> consider renaming the call_* functions to either mroute_* or ipmr_* (e.g.
>> ipmr_call_...). I personally prefer the ipmr prefix.
> 
> The naming scheme in this patch is aligned with the rest of the code.

Definitely not aligned with the rest of the ipmr code because it does not
have such calls. Its notifications have a prefix which is not call_.

> Please see "call_netdevice_notifiers" for example.

Sure, I don't care that much which style you choose, that's why I wrote
_consider_, since this code is contained within ipmr and is not exported
anywhere.

> Please feel free to send a patch to chanche them all.
> 

Jumping the gun a little bit here. :-)