| 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: <8312fdd3-9a69-f9e3-1ab0-ea95df577cf9@nvidia.com>
Date: Fri, 25 Feb 2022 10:00:00 -0800
From: Roopa Prabhu <roopa@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
stephen@...workplumber.org, nikolay@...ulusnetworks.com,
idosch@...dia.com, dsahern@...il.com, bpoirier@...dia.com
Subject: Re: [PATCH net-next v2 09/12] vxlan: vni filtering support on collect
metadata device
On 2/23/22 20:25, Jakub Kicinski wrote:
> On Tue, 22 Feb 2022 02:52:27 +0000 Roopa Prabhu wrote:
>> +static int vxlan_vni_add(struct vxlan_dev *vxlan,
>> + struct vxlan_vni_group *vg,
>> + u32 vni, union vxlan_addr *group,
>> + struct netlink_ext_ack *extack)
>> +{
>> + struct vxlan_vni_node *vninode;
>> + __be32 v = cpu_to_be32(vni);
>> + bool changed = false;
>> + int err = 0;
>> +
>> + if (vxlan_vnifilter_lookup(vxlan, v))
>> + return vxlan_vni_update(vxlan, vg, v, group, &changed, extack);
>> +
>> + err = vxlan_vni_in_use(vxlan->net, vxlan, &vxlan->cfg, v);
>> + if (err) {
>> + NL_SET_ERR_MSG(extack, "VNI in use");
>> + return err;
>> + }
>> +
>> + vninode = vxlan_vni_alloc(vxlan, v);
>> + if (!vninode)
>> + return -ENOMEM;
>> +
>> + err = rhashtable_lookup_insert_fast(&vg->vni_hash,
>> + &vninode->vnode,
>> + vxlan_vni_rht_params);
>> + if (err)
> leak ?
ouch, will fix.
>
>> + return err;
>> +
>> + __vxlan_vni_add_list(vg, vninode);
>> +
>> + if (vxlan->dev->flags & IFF_UP)
>> + vxlan_vs_add_del_vninode(vxlan, vninode, false);
>> +
>> + err = vxlan_vni_update_group(vxlan, vninode, group, true, &changed,
>> + extack);
>> +
>> + if (changed)
>> + vxlan_vnifilter_notify(vxlan, vninode, RTM_NEWTUNNEL);
>> +
>> + return err;
>> +}
>> +
>> +static void vxlan_vni_node_rcu_free(struct rcu_head *rcu)
>> +{
>> + struct vxlan_vni_node *v;
>> +
>> + v = container_of(rcu, struct vxlan_vni_node, rcu);
>> + kfree(v);
>> +}
> kfree_rcu()?
you are right it can move it to kfree_rcu in this patch. But, later in
the stats patch, this also frees up stats.
so, wont be able to switch it to kfree_rcu. but will look again.
>
>> +static int vxlan_vni_del(struct vxlan_dev *vxlan,
>> + struct vxlan_vni_group *vg,
>> + u32 vni, struct netlink_ext_ack *extack)
>> +{
>> + struct vxlan_vni_node *vninode;
>> + __be32 v = cpu_to_be32(vni);
>> + int err = 0;
>> +
>> + vg = rtnl_dereference(vxlan->vnigrp);
>> +
>> + vninode = rhashtable_lookup_fast(&vg->vni_hash, &v,
>> + vxlan_vni_rht_params);
>> + if (!vninode) {
>> + err = -ENOENT;
>> + goto out;
>> + }
>> +
>> + vxlan_vni_delete_group(vxlan, vninode);
>> +
>> + err = rhashtable_remove_fast(&vg->vni_hash,
>> + &vninode->vnode,
>> + vxlan_vni_rht_params);
>> + if (err)
>> + goto out;
>> +
>> + __vxlan_vni_del_list(vg, vninode);
>> +
>> + vxlan_vnifilter_notify(vxlan, vninode, RTM_DELTUNNEL);
>> +
>> + if (vxlan->dev->flags & IFF_UP)
>> + vxlan_vs_add_del_vninode(vxlan, vninode, true);
>> +
>> + call_rcu(&vninode->rcu, vxlan_vni_node_rcu_free);
>> +
>> + return 0;
>> +out:
>> + return err;
>> +}
>> +
>> +static int vxlan_vni_add_del(struct vxlan_dev *vxlan, __u32 start_vni,
>> + __u32 end_vni, union vxlan_addr *group,
>> + int cmd, struct netlink_ext_ack *extack)
>> +{
>> + struct vxlan_vni_group *vg;
>> + int v, err = 0;
>> +
>> + vg = rtnl_dereference(vxlan->vnigrp);
>> +
>> + for (v = start_vni; v <= end_vni; v++) {
>> + switch (cmd) {
>> + case RTM_NEWTUNNEL:
>> + err = vxlan_vni_add(vxlan, vg, v, group, extack);
>> + break;
>> + case RTM_DELTUNNEL:
>> + err = vxlan_vni_del(vxlan, vg, v, extack);
>> + break;
>> + default:
>> + err = -EOPNOTSUPP;
>> + break;
>> + }
>> + if (err)
>> + goto out;
>> + }
>> +
>> + return 0;
>> +out:
>> + return err;
>> +}
>> +
>> +static int vxlan_process_vni_filter(struct vxlan_dev *vxlan,
>> + struct nlattr *nlvnifilter,
>> + int cmd, struct netlink_ext_ack *extack)
>> +{
>> + struct nlattr *vattrs[VXLAN_VNIFILTER_ENTRY_MAX + 1];
>> + u32 vni_start = 0, vni_end = 0;
>> + union vxlan_addr group;
>> + int err = 0;
> unnecessary init
>
>> + err = nla_parse_nested(vattrs,
>> + VXLAN_VNIFILTER_ENTRY_MAX,
>> + nlvnifilter, vni_filter_entry_policy,
>> + extack);
>> + if (err)
>> + return err;
>> +
>> + if (vattrs[VXLAN_VNIFILTER_ENTRY_START]) {
>> + vni_start = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_START]);
>> + vni_end = vni_start;
>> + }
>> +
>> + if (vattrs[VXLAN_VNIFILTER_ENTRY_END])
>> + vni_end = nla_get_u32(vattrs[VXLAN_VNIFILTER_ENTRY_END]);
>> +
>> + if (!vni_start && !vni_end) {
>> + NL_SET_ERR_MSG_ATTR(extack, nlvnifilter,
>> + "vni start nor end found in vni entry");
>> + return -EINVAL;
>> + }
>> +
>> + if (vattrs[VXLAN_VNIFILTER_ENTRY_GROUP]) {
>> + group.sin.sin_addr.s_addr =
>> + nla_get_in_addr(vattrs[VXLAN_VNIFILTER_ENTRY_GROUP]);
>> + group.sa.sa_family = AF_INET;
>> + } else if (vattrs[VXLAN_VNIFILTER_ENTRY_GROUP6]) {
>> + group.sin6.sin6_addr =
>> + nla_get_in6_addr(vattrs[VXLAN_VNIFILTER_ENTRY_GROUP6]);
>> + group.sa.sa_family = AF_INET6;
>> + } else {
>> + memset(&group, 0, sizeof(group));
>> + }
>> +
>> + err = vxlan_vni_add_del(vxlan, vni_start, vni_end, &group, cmd,
>> + extack);
>> + if (err)
>> + return err;
>> +
>> + return 0;
>> +}
>> +
>> +void vxlan_vnigroup_uninit(struct vxlan_dev *vxlan)
>> +{
>> + struct vxlan_vni_node *v, *tmp;
>> + struct vxlan_vni_group *vg;
>> +
>> + vg = rtnl_dereference(vxlan->vnigrp);
>> + list_for_each_entry_safe(v, tmp, &vg->vni_list, vlist) {
>> + rhashtable_remove_fast(&vg->vni_hash, &v->vnode,
>> + vxlan_vni_rht_params);
>> + hlist_del_init_rcu(&v->hlist4.hlist);
>> +#if IS_ENABLED(CONFIG_IPV6)
>> + hlist_del_init_rcu(&v->hlist6.hlist);
>> +#endif
> no need to generate the notifications here?
great catch, will fix.
>
>> + __vxlan_vni_del_list(vg, v);
>> + call_rcu(&v->rcu, vxlan_vni_node_rcu_free);
>> + }
>> + rhashtable_destroy(&vg->vni_hash);
>> + kfree(vg);
>> +}
>> +
>> +int vxlan_vnigroup_init(struct vxlan_dev *vxlan)
>> +{
>> + struct vxlan_vni_group *vg;
>> + int ret = -ENOMEM;
>> +
>> + vg = kzalloc(sizeof(*vg), GFP_KERNEL);
>> + if (!vg)
>> + goto out;
> return -ENOMEM;
> the jumping dance is really not worth it here
agreed :)
>> + ret = rhashtable_init(&vg->vni_hash, &vxlan_vni_rht_params);
>> + if (ret)
>> + goto err_rhtbl;
>> + INIT_LIST_HEAD(&vg->vni_list);
>> + rcu_assign_pointer(vxlan->vnigrp, vg);
>> +
>> + return 0;
>> +
>> +out:
>> + return ret;
>> +
>> +err_rhtbl:
>> + kfree(vg);
>> +
>> + goto out;
>> +}
>> +
>> +static int vxlan_vnifilter_process(struct sk_buff *skb, struct nlmsghdr *nlh,
>> + struct netlink_ext_ack *extack)
>> +{
>> + struct net *net = sock_net(skb->sk);
>> + struct tunnel_msg *tmsg;
>> + struct vxlan_dev *vxlan;
>> + struct net_device *dev;
>> + struct nlattr *attr;
>> + int err, vnis = 0;
>> + int rem;
>> +
>> + /* this should validate the header and check for remaining bytes */
>> + err = nlmsg_parse(nlh, sizeof(*tmsg), NULL, VXLAN_VNIFILTER_MAX, NULL,
>> + extack);
> Could be useful to provide a policy here, even if it only points to
> single type (entry which is nested). Otherwise we will not reject
> UNSPEC, and validate if ENTRY has NLA_F_NESTED set, no?
yeah, any policy is better than no policy, will fix.
thanks for the review!
Powered by blists - more mailing lists