| 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
| ||
|
Message-ID: <fb08799d-f1ae-6ee7-c65f-f949e60bf07d@huawei.com>
Date: Mon, 25 Sep 2017 14:32:37 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Yotam Gigi <yotamg@...lanox.com>, Jiri Pirko <jiri@...nulli.us>,
<netdev@...r.kernel.org>
CC: <davem@...emloft.net>, <idosch@...lanox.com>, <mlxsw@...lanox.com>,
<nikolay@...ulusnetworks.com>, <andrew@...n.ch>
Subject: Re: [patch net-next v2 03/12] ipmr: Add FIB notification access
functions
Hi, Yotam
On 2017/9/25 13:38, Yotam Gigi wrote:
> On 09/25/2017 04:19 AM, Yunsheng Lin wrote:
>> Hi, Jiri
>>
>> On 2017/9/25 1: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(-)
>>>
>>> diff --git a/include/linux/mroute.h b/include/linux/mroute.h
>>> index 10028f2..54c5cb8 100644
>>> --- a/include/linux/mroute.h
>>> +++ b/include/linux/mroute.h
>>> @@ -5,6 +5,7 @@
>>> #include <linux/pim.h>
>>> #include <linux/rhashtable.h>
>>> #include <net/sock.h>
>>> +#include <net/fib_notifier.h>
>>> #include <uapi/linux/mroute.h>
>>>
>>> #ifdef CONFIG_IP_MROUTE
>>> @@ -58,6 +59,14 @@ struct vif_device {
>>> int link; /* Physical interface index */
>>> };
>>>
>>> +struct vif_entry_notifier_info {
>>> + struct fib_notifier_info info;
>>> + struct net_device *dev;
>>> + vifi_t vif_index;
>>> + unsigned short vif_flags;
>>> + u32 tb_id;
>>> +};
>>> +
>>> #define VIFF_STATIC 0x8000
>>>
>>> #define VIF_EXISTS(_mrt, _idx) ((_mrt)->vif_table[_idx].dev != NULL)
>>> @@ -146,6 +155,12 @@ struct mfc_cache {
>>> struct rcu_head rcu;
>>> };
>>>
>>> +struct mfc_entry_notifier_info {
>>> + struct fib_notifier_info info;
>>> + struct mfc_cache *mfc;
>>> + u32 tb_id;
>>> +};
>>> +
>>> struct rtmsg;
>>> int ipmr_get_route(struct net *net, struct sk_buff *skb,
>>> __be32 saddr, __be32 daddr,
>>> diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
>>> index 8387f09..abc84d9 100644
>>> --- a/include/net/netns/ipv4.h
>>> +++ b/include/net/netns/ipv4.h
>>> @@ -163,6 +163,9 @@ struct netns_ipv4 {
>>> struct fib_notifier_ops *notifier_ops;
>>> unsigned int fib_seq; /* protected by rtnl_mutex */
>>>
>>> + struct fib_notifier_ops *ipmr_notifier_ops;
>> Can we add a const here?
>
> It cannot be const as it get initialized it in ipmr_notifier_init.
>
>>
>>> + unsigned int ipmr_seq; /* protected by rtnl_mutex */
>>> +
>>> atomic_t rt_genid;
>>> };
>>> #endif
>>> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
>>> index 86dc5f9..49879c3 100644
>>> --- a/net/ipv4/ipmr.c
>>> +++ b/net/ipv4/ipmr.c
>>> @@ -264,6 +264,16 @@ static void __net_exit ipmr_rules_exit(struct net *net)
>>> fib_rules_unregister(net->ipv4.mr_rules_ops);
>>> rtnl_unlock();
>>> }
>>> +
>>> +static int ipmr_rules_dump(struct net *net, struct notifier_block *nb)
>>> +{
>>> + return fib_rules_dump(net, nb, RTNL_FAMILY_IPMR);
>>> +}
>>> +
>>> +static unsigned int ipmr_rules_seq_read(struct net *net)
>>> +{
>>> + return fib_rules_seq_read(net, RTNL_FAMILY_IPMR);
>>> +}
>>> #else
>>> #define ipmr_for_each_table(mrt, net) \
>>> for (mrt = net->ipv4.mrt; mrt; mrt = NULL)
>>> @@ -298,6 +308,16 @@ static void __net_exit ipmr_rules_exit(struct net *net)
>>> net->ipv4.mrt = NULL;
>>> rtnl_unlock();
>>> }
>>> +
>>> +static int ipmr_rules_dump(struct net *net, struct notifier_block *nb)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static unsigned int ipmr_rules_seq_read(struct net *net)
>>> +{
>>> + return 0;
>>> +}
>>> #endif
>>>
>>> static inline int ipmr_hash_cmp(struct rhashtable_compare_arg *arg,
>>> @@ -587,6 +607,43 @@ static struct net_device *ipmr_reg_vif(struct net *net, struct mr_table *mrt)
>>> }
>>> #endif
>>>
>>> +static int call_ipmr_vif_entry_notifier(struct notifier_block *nb,
>>> + struct net *net,
>>> + enum fib_event_type event_type,
>>> + struct vif_device *vif,
>>> + vifi_t vif_index, u32 tb_id)
>>> +{
>>> + struct vif_entry_notifier_info info = {
>>> + .info = {
>>> + .family = RTNL_FAMILY_IPMR,
>>> + .net = net,
>>> + },
>>> + .dev = vif->dev,
>>> + .vif_index = vif_index,
>>> + .vif_flags = vif->flags,
>>> + .tb_id = tb_id,
>>> + };
>> We only use info.info which is fib_notifier_info, the
>> vif_entry_notifier_info seems to be not needed, why not just
>> use fib_notifier_info?
>
> No, that's not true.
>
> The driver gets the notification with a pointer to a fib_notifier_info struct,
> and according to the type field uses container_of to get to the parent struct,
> which in this case is vif_entry_notifier_info. All the fields here are needed.
> You can see this code in patch 10.
>
> By the way, this function is completely symmetric to fib4 (which is in
> fib_trie.c +88) and fib6 (which is in ip6_fib +336) notify functions, who uses
> the exact same process.
Thanks for clarifying, I am not familar with ipmr, so only checking
coding style.
>
>>
>>> +
>>> + return call_fib_notifier(nb, net, event_type, &info.info);
>>> +}
>>> +
>>> +static int call_ipmr_mfc_entry_notifier(struct notifier_block *nb,
>>> + struct net *net,
>>> + enum fib_event_type event_type,
>>> + struct mfc_cache *mfc, u32 tb_id)
>>> +{
>>> + struct mfc_entry_notifier_info info = {
>>> + .info = {
>>> + .family = RTNL_FAMILY_IPMR,
>>> + .net = net,
>>> + },
>>> + .mfc = mfc,
>>> + .tb_id = tb_id
>>> + };
>>> +
>> As above.
>
>
> As above.
>
>
>>
>>> + return call_fib_notifier(nb, net, event_type, &info.info);
>>> +}
>>> +
>>> /**
>>> * vif_delete - Delete a VIF entry
>>> * @notify: Set to 1, if the caller is a notifier_call
>>> @@ -3050,14 +3107,87 @@ static const struct net_protocol pim_protocol = {
>>> };
>>> #endif
>>>
>>> +static unsigned int ipmr_seq_read(struct net *net)
>>> +{
>>> + ASSERT_RTNL();
>>> +
>>> + return net->ipv4.ipmr_seq + ipmr_rules_seq_read(net);
>>> +}
>>> +
>>> +static int ipmr_dump(struct net *net, struct notifier_block *nb)
>>> +{
>>> + struct mr_table *mrt;
>>> + int err;
>>> +
>>> + err = ipmr_rules_dump(net, nb);
>>> + if (err)
>>> + return err;
>>> +
>>> + ipmr_for_each_table(mrt, net) {
>>> + struct vif_device *v = &mrt->vif_table[0];
>>> + struct mfc_cache *mfc;
>>> + int vifi;
>>> +
>>> + /* Notifiy on table VIF entries */
>>> + read_lock(&mrt_lock);
>>> + for (vifi = 0; vifi < mrt->maxvif; vifi++, v++) {
>>> + if (!v->dev)
>>> + continue;
>>> +
>>> + call_ipmr_vif_entry_notifier(nb, net, FIB_EVENT_VIF_ADD,
>>> + v, vifi, mrt->id);
>>> + }
>>> + read_unlock(&mrt_lock);
>>> +
>>> + /* Notify on table MFC entries */
>>> + list_for_each_entry_rcu(mfc, &mrt->mfc_cache_list, list)
>>> + call_ipmr_mfc_entry_notifier(nb, net,
>>> + FIB_EVENT_ENTRY_ADD, mfc,
>>> + mrt->id);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct fib_notifier_ops ipmr_notifier_ops_template = {
>>> + .family = RTNL_FAMILY_IPMR,
>>> + .fib_seq_read = ipmr_seq_read,
>>> + .fib_dump = ipmr_dump,
>>> + .owner = THIS_MODULE,
>>> +};
>>> +
>>> +int __net_init ipmr_notifier_init(struct net *net)
>>> +{
>>> + struct fib_notifier_ops *ops;
>>> +
>>> + net->ipv4.ipmr_seq = 0;
>>> +
>>> + ops = fib_notifier_ops_register(&ipmr_notifier_ops_template, net);
>>> + if (IS_ERR(ops))
>>> + return PTR_ERR(ops);
>>> + net->ipv4.ipmr_notifier_ops = ops;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void __net_exit ipmr_notifier_exit(struct net *net)
>>> +{
>>> + fib_notifier_ops_unregister(net->ipv4.ipmr_notifier_ops);
>>> + net->ipv4.ipmr_notifier_ops = NULL;
>>> +}
>>> +
>>> /* Setup for IP multicast routing */
>>> static int __net_init ipmr_net_init(struct net *net)
>>> {
>>> int err;
>>>
>>> + err = ipmr_notifier_init(net);
>>> + if (err)
>>> + goto ipmr_notifier_fail;
>>> +
>>> err = ipmr_rules_init(net);
>>> if (err < 0)
>>> - goto fail;
>>> + goto ipmr_rules_fail;
>>>
>>> #ifdef CONFIG_PROC_FS
>>> err = -ENOMEM;
>>> @@ -3074,7 +3204,9 @@ static int __net_init ipmr_net_init(struct net *net)
>>> proc_vif_fail:
>>> ipmr_rules_exit(net);
>>> #endif
>>> -fail:
>>> +ipmr_rules_fail:
>>> + ipmr_notifier_exit(net);
>>> +ipmr_notifier_fail:
>>> return err;
>>> }
>>>
>>> @@ -3084,6 +3216,7 @@ static void __net_exit ipmr_net_exit(struct net *net)
>>> remove_proc_entry("ip_mr_cache", net->proc_net);
>>> remove_proc_entry("ip_mr_vif", net->proc_net);
>>> #endif
>>> + ipmr_notifier_exit(net);
>>> ipmr_rules_exit(net);
>>> }
>>>
>>>
>
>
> .
>
Powered by blists - more mailing lists