netdev - Re: [PATCH net-next v3 2/2] vxlan: allow specifying multiple default destinations

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: <20130530110948.GA10532@casper.infradead.org>
Date:	Thu, 30 May 2013 12:09:48 +0100
From:	Thomas Graf <tgraf@...g.ch>
To:	Mike Rapoport <mike.rapoport@...ellosystems.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/2] vxlan: allow specifying multiple default
 destinations

Looks much better, thanks for taking the feedback. Some additional
feedback inlined:

On 05/29/13 at 01:00pm, Mike Rapoport wrote:
> +static int vxlan_remote_add(struct vxlan_dev *vxlan, struct nlattr *attr)
> +{
> +	struct nlattr *i;
> +	__be32 ip = htonl(INADDR_NONE);
> +	__be16 port;
> +	u32 ifindex, vni;
> +	int rem, err = 0;
> +
> +	port = vxlan->dst_port;
> +	vni = vxlan->default_dst.remote_vni;
> +	ifindex = vxlan->default_dst.remote_ifindex;
> +
> +	nla_for_each_nested(i, attr, rem) {
> +		switch (nla_type(i)) {
> +		case IFLA_VXLAN_REMOTE_ADDR:
> +			ip = nla_get_be32(i);
> +			break;
> +		case IFLA_VXLAN_REMOTE_PORT:
> +			port = nla_get_be16(i);
> +			break;
> +		case IFLA_VXLAN_REMOTE_VNI:
> +			vni = nla_get_u32(i);
> +			break;
> +		case IFLA_VXLAN_REMOTE_IFINDEX:
> +			ifindex = nla_get_u32(i);
> +			break;
> +		default:
> +			err = -EINVAL;
> +			break;
> +		};
> +
> +		if (err)
> +			return err;
> +	}

The above construct is not forward compatible. If we want to add
more attributes in the future any older kernel will break with
EINVAL. We typically support partial requests in older kernels.

> @@ -1380,6 +1478,13 @@ static void vxlan_setup(struct net_device *dev)
>  		INIT_HLIST_HEAD(&vxlan->fdb_head[h]);
>  }
>  
> +static const struct nla_policy vxlan_remotes_policy[IFLA_VXLAN_REMOTE_MAX + 1] = {
> +	[IFLA_VXLAN_REMOTE_ADDR]	= { .len = FIELD_SIZEOF(struct iphdr, daddr) },

I would just make this a u32 for now.

> @@ -1512,6 +1646,46 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port)
>  	return vs;
>  }
>  
> +static int vxlan_remotes_update(struct vxlan_dev *vxlan, struct nlattr *attr)
> +{
> +	struct nlattr *i;
> +	int rem, err = 0;
> +
> +	nla_for_each_nested(i, attr, rem) {
> +		switch (nla_type(i)) {
> +		case IFLA_VXLAN_REMOTE_NEW:
> +			err = vxlan_remote_add(vxlan, i);
> +			break;
> +		case IFLA_VXLAN_REMOTE_DEL:
> +			err = vxlan_remote_delete(vxlan, i);
> +			break;
> +		default:
> +			err = -EINVAL;
> +			break;

I would return EOPNOTSUPP here

> +static int vxlan_fill_remotes_info(struct sk_buff *skb,
> +				   const struct vxlan_dev *vxlan)
> +{
> +	struct vxlan_rdst *rdst;
> +	struct nlattr *nest, *rdst_nest;
> +	__be32 ip;
> +	int i;
> +
> +	if (vxlan->remote_cnt) {
> +		nest = nla_nest_start(skb, IFLA_VXLAN_REMOTES);
> +		if (nest == NULL)
> +			goto nla_put_failure;
> +
> +		for (rdst = vxlan->default_dst.remote_next, i = 0; rdst;
> +		     rdst = rdst->remote_next, i++) {
> +			ip = rdst->remote_ip;
> +
> +			rdst_nest = nla_nest_start(skb, i);

Attribute type '0' is reserved, please don't use it. Start with '1'.

> +enum {
> +	IFLA_VXLAN_REMOTE_NEW,
> +	IFLA_VXLAN_REMOTE_DEL,
> +};

Same here, attribute type '0' is reserved.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html