| 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: <54bef87c-8b7a-2512-3763-bac3469db421@mellanox.com>
Date: Mon, 25 Sep 2017 08:38:28 +0300
From: Yotam Gigi <yotamg@...lanox.com>
To: Yunsheng Lin <linyunsheng@...wei.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
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.
>
>> +
>> + 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