Re: [PATCH iproute2-next v1 1/1] iproute: Extend bonding's "arp_ip_target" parameter to add vlan tags.

[Jay Vosburgh <jv@jvosburgh.net>]

David Wilder <wilder@us.ibm.com> wrote:

>This change extends the "arp_ip_target" parameter format to allow for
>a list of vlan tags to be included for each arp target.
>
>The new format for arp_ip_target is:
>arp_ip_target=ipv4-address[vlan-tag\...],...
>
>Examples:
>arp_ip_target=10.0.0.1[10]
>arp_ip_target=10.0.0.1[100/200]
>
>Signed-off-by: David Wilder <wilder@us.ibm.com>
>---
> ip/iplink_bond.c | 62 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 59 insertions(+), 3 deletions(-)
>
>diff --git a/ip/iplink_bond.c b/ip/iplink_bond.c
>index 19af67d0..bb0b6e84 100644
>--- a/ip/iplink_bond.c
>+++ b/ip/iplink_bond.c
>@@ -173,6 +173,45 @@ static void explain(void)
> 	print_explain(stderr);
> }
> 
>+#define BOND_VLAN_PROTO_NONE htons(0xffff)
>+
>+struct bond_vlan_tag {
>+	__be16	vlan_proto;
>+	__be16	vlan_id;
>+};
>+
>+static inline struct bond_vlan_tag *bond_vlan_tags_parse(char *vlan_list, int level, int *size)
>+{
>+	struct bond_vlan_tag *tags = NULL;
>+	char *vlan;
>+	int n;
>+
>+	if (!vlan_list || strlen(vlan_list) == 0) {
>+		tags = calloc(level + 1, sizeof(*tags));
>+		*size = (level + 1) * (sizeof(*tags));
>+		if (tags)
>+			tags[level].vlan_proto = BOND_VLAN_PROTO_NONE;
>+		return tags;
>+	}
>+
>+	for (vlan = strsep(&vlan_list, "/"); (vlan != 0); level++) {
>+		tags = bond_vlan_tags_parse(vlan_list, level + 1, size);
>+		if (!tags)
>+			continue;
>+
>+		tags[level].vlan_proto = htons(ETH_P_8021Q);
>+		n = sscanf(vlan, "%hu", &(tags[level].vlan_id));
>+
>+		if (n != 1 || tags[level].vlan_id < 1 ||
>+		    tags[level].vlan_id > 4094)
>+			return NULL;

	Two questions:

	1) Do we care about 802.1p priority tags?  If memory serves,
those manifest as VLAN tags with a VLAN ID of 0 and some other bits set
to provide the priority.  The above appears to disallow such tags.

	2) This loop appears to be unbounded, and will process an
unlimited number of VLANs.  Do we need more than 2 (the original 802.1ad
limit)?  Even if we need more than 2, the upper limit should probably be
some reasonably small number.  The addattr loop (below) will conk out if
the whole thing exceeds 1024 bytes, but that would still permit 120 or
so.

	-J

>+
>+		return tags;
>+	}
>+
>+	return NULL;
>+}
>+
> static int bond_parse_opt(struct link_util *lu, int argc, char **argv,
> 			  struct nlmsghdr *n)
> {
>@@ -239,12 +278,29 @@ static int bond_parse_opt(struct link_util *lu, int argc, char **argv,
> 				NEXT_ARG();
> 				char *targets = strdupa(*argv);
> 				char *target = strtok(targets, ",");
>-				int i;
>+				struct bond_vlan_tag *tags;
>+				int size, i;
> 
> 				for (i = 0; target && i < BOND_MAX_ARP_TARGETS; i++) {
>-					__u32 addr = get_addr32(target);
>+					struct Data {
>+						__u32 addr;
>+						struct bond_vlan_tag vlans[];
>+					} data;
>+					char *vlan_list, *dup;
>+
>+					dup = strdupa(target);
>+					data.addr = get_addr32(strsep(&dup, "["));
>+					vlan_list = strsep(&dup, "]");
>+
>+					if (vlan_list) {
>+						tags = bond_vlan_tags_parse(vlan_list, 0, &size);
>+						memcpy(&data.vlans, tags, size);
>+						addattr_l(n, 1024, i, &data,
>+							  sizeof(data.addr)+size);
>+					} else {
>+						addattr32(n, 1024, i, data.addr);
>+					}
> 
>-					addattr32(n, 1024, i, addr);
> 					target = strtok(NULL, ",");
> 				}
> 				addattr_nest_end(n, nest);
>-- 
>2.43.5
>

---
	-Jay Vosburgh, jv@jvosburgh.net