[lldp-devel] [PATCH 1/3] vdp22 filter interface support

John Fastabend john.fastabend at gmail.com
Mon Nov 25 21:55:48 UTC 2013


On 11/21/2013 01:26 AM, Thomas Richter wrote:
> The VDP22 protocol allows the return of changed filter
> information data from the switch to the originator. This
> requires a change in the netlink message format. Now lldpad
> support 2 netlink message formats:
> 1. The draft 0.2 version. In this version the filter information
>     data is received but not returned to the originator.
>
>     The receive message format is:
>     IFLA_IFNAME
>     IFLA_VFINFO_LIST
>          IFLA_VF_INFO
> 	     IFLA_VF_MAC
> 	     IFLA_VF_VLAN
> 	end
>     IFLA_VF_PORTS
>          IFLA_VF_PORT
> 	     IFLA_PORT_VSI_TYPE
> 	     IFLA_PORT_REQUEST
> 	     IFLA_PORT_INSTANCE_UUID
> 	end
>
>     The returned message format sent as reply from lldpad to the
>     originator is:
>     IFLA_IFNAME
>     IFLA_VF_PORTS
>          IFLA_VF_PORT
> 	     IFLA_PORT_VF
> 	     IFLA_PORT_INSTANCE_UUID
> 	     IFLA_PORT_RESPONSE
> 	end
>
> 2. The ratified standard version VDP22 has the following
>     netlink message format:
>     IFLA_IFNAME
>     IFLA_VF_PORTS
>          IFLA_VF_PORT
> 	     IFLA_PORT_VSI_TYPE22
> 	     IFLA_PORT_VSI_FILTER
> 	     IFLA_PORT_REQUEST
> 	     IFLA_PORT_INSTANCE_UUID
> 	end
>

[...]

> +/*
> + * New version, implemented as library function and header file in lldpad-devel
> + * package.
> + *
> + * Netlink message for QBG 2.2 ratified standard
> + */
> +
> +enum {					/* 802.1Qbg VDP ratified standard */
> +	IFLA_PORT_VSI_TYPE22 = IFLA_PORT_MAX,
> +	IFLA_PORT_VSI_FILTER,
> +	__IFLA_PORT_MAX_NEW
> +};
> +

The problem with pushing the defines to the end like this is you get a
dependency between lldpad and the application pushing the message to
it. For example if you build both libvirt and lldpad at some point in
time then upgrade lldpad or libvirt they may no longer be in sync and
these messages will fail.

We've been going down this path of using the kernel API because it
existed and we presumed at some point there would be a kernel consumer
for this. As it is not a single driver has implemented this API as best
I can tell.

At this point I'm trying to decide how to handle this I really don't
like creating dependencies between separate applications like this.
Any thoughts/ideas? I see you submitted a kernel patch which would
I think would resolve this by making these APIs part of the kernel
kABI. Once Dave Miller opens the net-next merge window again maybe
we need to revisit that patch.


> +/*
> + * Parse the IFLA_VF_PORT block of the netlink message.
>    * Return zero on success and errno else.
> + *
> + * Code to parse netlink message format 1. May contain IFLA_PORT_VSI_TYPE22
> + * and IFLA_PORT_VSI_FILTER attributes which make it netlink message format 2.

If it may contain a IFLA_PORT_VSI_TYPE22 type why is it not in the
nla_policy for pc_vfport?

>    */
> -static int vdpnl_vfports(struct nlattr *vfports, struct vdpnl_vsi *vsi)
> +static struct nla_policy  pc_vfport[IFLA_MAX + 1] = {
> +	[IFLA_PORT_VF] = {
> +				.type = NLA_U32
> +			},
> +	[IFLA_PORT_VSI_TYPE] = {
> +				.minlen = sizeof(struct ifla_port_vsi)
> +			},

I expected to see another entry for type22 here like this,

	[IFLA_PORT_VSI_TYPE22] = { ... },

> +	[IFLA_PORT_INSTANCE_UUID] = {
> +				.minlen = PORT_UUID_MAX

should this be '.len = PORT_UUID_MAX' I think that this is a max length
not a minimum length?

> +			},
> +	[IFLA_PORT_HOST_UUID] = {
> +				.minlen = PORT_UUID_MAX

Same here max?

> +			},
> +	[IFLA_PORT_REQUEST] = {
> +				.type = NLA_U8
> +			},

Also same question for IFLA_PORT_VSI_FILTER?

> +};
> +
> +static int parse_vf_port(struct vdpnl_vsi *vsi, struct nlattr *tb)
>   {
>   	char instance[VDP_UUID_STRLEN + 2];
> -	struct nlattr *tb_vf_ports, *tb3[IFLA_PORT_MAX + 1];
> -	int rem;
> +	struct nlattr *vf[IFLA_PORT_MAX + 1];
> +	size_t dim = sizeof(vf) / sizeof(vf[0]);
> +	int rc;
>
> -	if (!vfports) {
> -		LLDPAD_DBG("%s:FOUND NO IFLA_VF_PORTS\n", __func__);
> +	memset(vf, 0, sizeof(vf));
> +	rc = nla_parse(vf, dim, nla_data(tb), nla_len(tb), pc_vfport);

Because you don't have the IFLA_PORT_VSI_FILTER and IFLA_PORT_VSI_TYPE22
in the policy these attributes are not validated.


> +	if (rc) {
> +		LLDPAD_ERR("%s:IFLA_VF_PORT parsing error\n", __func__);
>   		return -EINVAL;
>   	}
> -
> -	nla_for_each_nested(tb_vf_ports, vfports, rem) {
> -		if (nla_type(tb_vf_ports) != IFLA_VF_PORT) {
> -			LLDPAD_DBG("%s:not a IFLA_VF_PORT skipping\n",
> +	if (vf[IFLA_PORT_VF])
> +		vsi->vf = nla_get_u32(vf[IFLA_PORT_VF]);
> +	if (vf[IFLA_PORT_HOST_UUID]) {
> +		unsigned char *uuid;
> +
> +		uuid = (unsigned char *)nla_data(vf[IFLA_PORT_HOST_UUID]);
> +		vdp_uuid2str(uuid, instance, sizeof(instance));
> +		LLDPAD_DBG("%s:IFLA_PORT_HOST_UUID:%s\n", __func__, instance);
> +	}
> +	if (vf[IFLA_PORT_RESPONSE]) {
> +		vsi->response = nla_get_u16(vf[IFLA_PORT_RESPONSE]);
> +		LLDPAD_DBG("%s:IFLA_PORT_RESPONSE:%d\n", __func__,
> +			    vsi->response);
> +	}
> +	if (vf[IFLA_PORT_VSI_TYPE]) {
> +		struct ifla_port_vsi *p;
> +
> +		p = (struct ifla_port_vsi *)nla_data(vf[IFLA_PORT_VSI_TYPE]);
> +		vsi->vsi_typeid = p->vsi_type_id[2] << 16
> +				| p->vsi_type_id[1] << 8 | p->vsi_type_id[0];
> +		vsi->vsi_mgrid = p->vsi_mgr_id;
> +		vsi->vsi_typeversion = p->vsi_type_version;
> +		vsi->nl_version = vdpnl_nlf1;
> +		if (!vsi->maclist) {
> +			LLDPAD_ERR("%s:VDP 0.2/2.2 filter error\n",
>   				   __func__);
> -			continue;
> +			return -EINVAL;
>   		}
> -		if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb_vf_ports,
> -			ifla_port_policy)) {
> -			LLDPAD_ERR("%s:IFLA_PORT_MAX parsing failed\n",
> +	} else if(vf[IFLA_PORT_VSI_TYPE22] && vf[IFLA_PORT_VSI_FILTER]) {
> +		struct ifla_port_vsi22 *p;
> +
> +		if (vsi->maclist) {
> +			LLDPAD_ERR("%s:VDP 0.2/2.2 filter error\n",
>   				   __func__);
>   			return -EINVAL;


[...]

> +		p = (struct ifla_port_vsi22 *)
> +					nla_data(vf[IFLA_PORT_VSI_TYPE22]);
> +		vsi->vsi_typeid = p->vsi_type_id[2] << 16
> +				| p->vsi_type_id[1] << 8 | p->vsi_type_id[0];
> +		vsi->vsi_typeversion = p->vsi_type_version;
> +		vsi->hints = p->vsi_hints;
> +		vsi->filter_fmt = p->vsi_filter_fmt;
> +		vsi->vsiid_fmt = p->vsi_uuidfmt;
> +		memcpy(vsi->vsi_mgrid2, p->vsi_mgrid, PORT_UUID_MAX);
> +		vsi->nl_version = vdpnl_nlf2;
> +		vsi->macsz = p->vsi_filter_num;
> +		vsi->maclist = calloc(vsi->macsz, sizeof(*vsi->maclist));
> +		if (!vsi->maclist)
> +			return -ENOMEM;
> +		parse_filter_data(vsi, vf[IFLA_PORT_VSI_FILTER]);
> +	} else {
> +		LLDPAD_ERR("%s:IFLA_PORT_VSI_TYPE/FILTER missing\n", __func__);
> +		return -EINVAL;
> +	}
> +	if (vf[IFLA_PORT_INSTANCE_UUID])
> +		nla_memcpy(vsi->vsi_uuid, vf[IFLA_PORT_INSTANCE_UUID],
> +		       sizeof(vsi->vsi_uuid));
> +	else {
> +		LLDPAD_ERR("%s:IFLA_PORT_INSTANCE_UUID missing\n", __func__);

do you need to free vsi->maclist at this point?

> +		return -EINVAL;
> +	}
> +	if (vf[IFLA_PORT_REQUEST])
> +		vsi->request = nla_get_u8(vf[IFLA_PORT_REQUEST]);
> +	else {

same here free vsi->maclist?

> +		LLDPAD_ERR("%s:IFLA_PORT_REQUEST missing\n", __func__);
> +		return -EINVAL;
>   	}
>   	return 0;
>   }
>

[...]

> +/*
> + * Parse the IFLA_VF_INFO block of the netlink message.
> + * Return zero on success and errno else.
> + *
> + * Code and parse netlink message format 1.
> + */
> +static struct nla_policy  pc_vlanmac[IFLA_MAX + 1] = {
> +	[IFLA_VF_MAC] = {
> +				.minlen = sizeof(struct ifla_vf_mac)
> +			},
> +	[IFLA_VF_VLAN] = {
> +				.minlen = sizeof(struct ifla_vf_vlan)
> +			}
> +};
>
> -		if (vf[IFLA_VF_MAC]) {
> -			struct ifla_vf_mac *mac = RTA_DATA(vf[IFLA_VF_MAC]);
> +static int parse_vf_info(struct vdpnl_vsi *vsi, struct nlattr *tb)
> +{
> +	struct nlattr *vf[IFLA_VF_MAX + 1];
> +	size_t dim = sizeof(vf) / sizeof(vf[0]);
> +	int rc;
> +	bool have_mac = false, have_vid = false;
>
> -			memcpy(vsi->maclist->mac, mac->mac, ETH_ALEN);
> -			have_mac = true;
> -		}
> +	memset(vf, 0, sizeof(vf));
> +	rc = nla_parse(vf, dim, nla_data(tb), nla_len(tb), pc_vlanmac);
> +	if (rc || !vf[IFLA_VF_VLAN]) {
> +		LLDPAD_ERR("%s:IFLA_VF_VLAN missing\n", __func__);
> +		return -EINVAL;
> +	}
> +	vsi->nl_version = vdpnl_nlf1;
> +	vsi->macsz = 1;
> +	vsi->maclist = calloc(1, sizeof(*vsi->maclist));
> +	if (!vsi->maclist)
> +		return -ENOMEM;
> +	if (vf[IFLA_VF_MAC]) {
> +		struct ifla_vf_mac *mac = nla_data(vf[IFLA_VF_MAC]);
> +
> +		memcpy(vsi->maclist->mac, mac->mac, ETH_ALEN);
> +		have_mac = true;
> +	}
>
> -		if (vf[IFLA_VF_VLAN]) {
> -			struct ifla_vf_vlan *vlan = RTA_DATA(vf[IFLA_VF_VLAN]);
> +	if (vf[IFLA_VF_VLAN]) {
> +		struct ifla_vf_vlan *vlan = nla_data(vf[IFLA_VF_VLAN]);
>
> -			vsi->maclist->vlan = vlan->vlan;
> -			vsi->maclist->qos = vlan->qos;
> +		vsi->maclist->vlan = vlan->vlan;
> +		vsi->maclist->qos = vlan->qos;
> +		if (vlan->vlan <= 4095 && vlan->qos <= 7)
>   			have_vid = true;
> -		}
> -		LLDPAD_DBG("%s:have_vid:%d have_mac:%d\n", __func__, have_vid,
> +	}
> +	LLDPAD_DBG("%s:have_vid:%d have_mac:%d\n", __func__, have_vid,
>   			   have_mac);
> -		if (have_vid && have_mac)
> -			vsi->filter_fmt = VDP22_FFMT_MACVID;
> -		else if (have_vid)
> -			vsi->filter_fmt = VDP22_FFMT_VID;
> -		else
> -			return -EINVAL;
> +	if (have_vid && have_mac)
> +		vsi->filter_fmt = VDP22_FFMT_MACVID;
> +	else if (have_vid)
> +		vsi->filter_fmt = VDP22_FFMT_VID;
> +	else

free vsi->maclist?

> +		rc = -EINVAL;
> +	return rc;
> +}
> +

[...]
>   /*
>    * Build the variable part of the netlink reply message for status inquiry.
>    * It contains the UUID and the response field for the VSI profile.
> + *
> + * If the vlan number changed, return the new vlan/qos to the caller.
>    */
> -static void vdpnl_reply2(struct vdpnl_vsi *p, struct nlmsghdr *nlh)
> +static void vdpnl_reply2(struct vdpnl_vsi *p, struct nl_msg *nlh)
>   {
>   	char instance[VDP_UUID_STRLEN + 2];
>
> -	mynla_put(nlh, IFLA_PORT_INSTANCE_UUID, sizeof p->vsi_uuid,
> +	nla_put(nlh, IFLA_PORT_INSTANCE_UUID, sizeof p->vsi_uuid,
>   		  p->vsi_uuid);
>   	vdp_uuid2str(p->vsi_uuid, instance, sizeof instance);
>   	LLDPAD_DBG("%s:IFLA_PORT_INSTANCE_UUID:%s\n", __func__, instance);
> -	mynla_put_u32(nlh, IFLA_PORT_VF, PORT_SELF_VF);
> +	nla_put_u32(nlh, IFLA_PORT_VF, PORT_SELF_VF);
>   	LLDPAD_DBG("%s:IFLA_PORT_VF:%d\n", __func__,  PORT_SELF_VF);
>   	if (p->response != VDP_RESPONSE_NO_RESPONSE) {
> -		mynla_put_u16(nlh, IFLA_PORT_RESPONSE, p->response);
> +		nla_put_u16(nlh, IFLA_PORT_RESPONSE, p->response);

Also we should start checking the return values from nla_put_*. They
can fail if the payload becomes bigger than the default message size.

>   		LLDPAD_DBG("%s:IFLA_PORT_RESPONSE:%d\n", __func__,
>   			   p->response);
> +		if (p->macsz && p->maclist->changed)
> +			vdpnl_reply3(p, nlh);
> +	}
> +}
> +

[...]

Besides the list of comments the big issue is the compatibilite with
the kernel header files.

Thanks,
John


-- 
John Fastabend         Intel Corporation


More information about the lldp-devel mailing list