Date:   Wed, 23 Feb 2022 20:25:09 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Roopa Prabhu <roopa@...dia.com>
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 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 ?

> +		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()?

> +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?

> +		__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

> +	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?

> +	if (err < 0)
> +		return err;
> +
> +	tmsg = nlmsg_data(nlh);
> +	dev = __dev_get_by_index(net, tmsg->ifindex);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	if (!netif_is_vxlan(dev)) {
> +		NL_SET_ERR_MSG_MOD(extack, "The device is not a vxlan device");
> +		return -EINVAL;
> +	}
> +
> +	vxlan = netdev_priv(dev);
> +
> +	if (!(vxlan->cfg.flags & VXLAN_F_VNIFILTER))
> +		return -EOPNOTSUPP;
> +
> +	nlmsg_for_each_attr(attr, nlh, sizeof(*tmsg), rem) {
> +		switch (nla_type(attr)) {
> +		case VXLAN_VNIFILTER_ENTRY:
> +			err = vxlan_process_vni_filter(vxlan, attr,
> +						       nlh->nlmsg_type, extack);
> +			break;
> +		default:
> +			continue;
> +		}
> +		vnis++;
> +		if (err)
> +			break;
> +	}
> +
> +	if (!vnis) {
> +		NL_SET_ERR_MSG_MOD(extack, "No vnis found to process");
> +		err = -EINVAL;
> +	}
> +
> +	return err;
> +}

Powered by blists - more mailing lists