[PATCH iproute2-next v1 0/1] iproute2-next: Extending bonding's arp_ip_target to include a list of vlan tags.

[PATCH iproute2-next v1 0/1] iproute2-next: Extending bonding's arp_ip_target to include a list of vlan tags.

[David Wilder]

This change extends the "arp_ip_target" option format to allow for a list of
vlan tags to be included for each arp target. This new list of tags is optional
and may be omitted to preserve the current format and process of discovering
vlans.

This change is dependent on this bonding driver patch set:
https://www.spinics.net/lists/netdev/msg1103232.html

Merge only after the above patch set has been merged.

Thank you for your time and reviews.

Signed-off-by: David Wilder <wilder@us.ibm.com>

David Wilder (1):
  iproute: Extend bonding's "arp_ip_target" parameter to add vlan tags.

 ip/iplink_bond.c | 62 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 3 deletions(-)

-- 
2.43.5

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

[David Wilder]

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

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

[Jay Vosburgh]

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

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

[David Wilder]

________________________________________
From: Jay Vosburgh <jv@jvosburgh.net>
Sent: Wednesday, July 2, 2025 7:04 PM
To: David Wilder
Cc: netdev@vger.kernel.org; pradeeps@linux.vnet.ibm.com; Pradeep Satyanarayana; i.maximets@ovn.org; Adrian Moreno Zapata; Hangbin Liu; stephen@networkplumber.org; dsahern@gmail.com
Subject: [EXTERNAL] Re: [PATCH iproute2-next v1 1/1] iproute: Extend bonding's "arp_ip_target" parameter to add vlan tags.

>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.

802.1p should be ok, the tpid is the same just an added priority control field.
Could not the priority (PCP) in a 802.1p be different between
flows to the same target?  In this case we are only defining the tag for
arp requests the default priority 0 should be ok.

However, Do we need to support other tag types? For example 802.1ad?
I would like to avoided making the configuration complicated with too many
options.

>
>        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.

I will add a limit.  The existing implementation has no limit,
although bond_arp_send() gets the order wrong with more than two vlan headers.
That's a bug I hoped to look at. I found I could workaround that by specifying
the vlan tags in the wrong order :)

David Wilder

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

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

[Jay Vosburgh]

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;
>+
>+		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);

	Another question occurred to me: is this method for sending the
VLAN tags going to break compatibility?  New versions of iproute2 need
to work on older kernels, so we can't simply change existing APIs in
ways that require a lockstep change of iproute versions (going either
forwards or backwards).

	The above looks like it changes the structure being conveyed
into the kernel, which I don't think we can do.  In the kernel, the old
API will need to continue to function, and therefore the new "with VLAN
tag" case will need to use a new API.

	The existing IFLA_BOND_ARP_IP_TARGET type doesn't use nested
netlink types, it just sends binary data, so I'm thinking we can't just
change that binary data, and would need a new IFLA_BOND_ type.

	-J

> 					target = strtok(NULL, ",");
> 				}
> 				addattr_nest_end(n, nest);
>-- 
>2.43.5
>

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

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

[David Wilder]

________________________________________
From: Jay Vosburgh <jv@jvosburgh.net>
Sent: Tuesday, July 8, 2025 9:51 AM
To: David Wilder
Cc: netdev@vger.kernel.org; pradeeps@linux.vnet.ibm.com; Pradeep Satyanarayana; i.maximets@ovn.org; Adrian Moreno Zapata; Hangbin Liu; stephen@networkplumber.org; dsahern@gmail.com
Subject: [EXTERNAL] Re: [PATCH iproute2-next v1 1/1] iproute: Extend bonding's "arp_ip_target" parameter to add vlan tags.


>>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;
>>+
>>+              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);

Answering your last question first,  IFLA_BOND_ARP_IP_TARGET was
already NLA_NESTED (see: bond_netlink.c).

>
>        Another question occurred to me: is this method for sending the
>VLAN tags going to break compatibility?  New versions of iproute2 need
>to work on older kernels, so we can't simply change existing APIs in
>ways that require a lockstep change of iproute versions (going either
>forwards or backwards).

I manage to preserve compatibility forward and backward.  Both a new
kernel and old kernel will work with both an new and old iproute2.

>        The above looks like it changes the structure being conveyed
>into the kernel, which I don't think we can do.  In the kernel, the old
>API will need to continue to function, and therefore the new "with VLAN
>tag" case will need to use a new API.

I thought adding a new API was what we wanted to avoid. But a new API
is unnecessary as I am extending the existing one in such a way to not
break the original api.

The original code sent 4 bytes of data (the 32bit ip address) in each nested
entry.  The new code sends the same 4 bytes containing the ip address.
If a list of tags has been included the data is appended to the 4byte address.
Type NLA_NESTED has no fixed data size.  If no vlans are supplied then the
data sent to the kernel looks exactly the same as with the old iproute2.
Therefor a new kernel will continue to work with the old iproute2 command,
you just cant add vlan tags until you upgrade iproute2.

An old kernel will continue to work with a new iproute2. The kernel will be
sent the same 4Byte address in each nested entry.  If you try to add a list
of vlan tags the kernel ignores the extra data.

I tested the various combinations.

>
>        The existing IFLA_BOND_ARP_IP_TARGET type doesn't use nested
>netlink types, it just sends binary data, so I'm thinking we can't just
>change that binary data, and would need a new IFLA_BOND_ type.
>
>        -J
>
>>                                       target = strtok(NULL, ",");
>>                               }
>>                               addattr_nest_end(n, nest);
>>--
>>2.43.5
>>
>
>---
>        -Jay Vosburgh, jv@jvosburgh.net

David Wilder  wilder@us.ibm.com

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

[Jay Vosburgh]

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

>________________________________________
>From: Jay Vosburgh <jv@jvosburgh.net>
>Sent: Tuesday, July 8, 2025 9:51 AM
>To: David Wilder
>Cc: netdev@vger.kernel.org; pradeeps@linux.vnet.ibm.com; Pradeep Satyanarayana; i.maximets@ovn.org; Adrian Moreno Zapata; Hangbin Liu; stephen@networkplumber.org; dsahern@gmail.com
>Subject: [EXTERNAL] Re: [PATCH iproute2-next v1 1/1] iproute: Extend bonding's "arp_ip_target" parameter to add vlan tags.
>
>
>>>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;
>>>+
>>>+              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);
>
>Answering your last question first,  IFLA_BOND_ARP_IP_TARGET was
>already NLA_NESTED (see: bond_netlink.c).
>
>>
>>        Another question occurred to me: is this method for sending the
>>VLAN tags going to break compatibility?  New versions of iproute2 need
>>to work on older kernels, so we can't simply change existing APIs in
>>ways that require a lockstep change of iproute versions (going either
>>forwards or backwards).
>
>I manage to preserve compatibility forward and backward.  Both a new
>kernel and old kernel will work with both an new and old iproute2.
>
>>        The above looks like it changes the structure being conveyed
>>into the kernel, which I don't think we can do.  In the kernel, the old
>>API will need to continue to function, and therefore the new "with VLAN
>>tag" case will need to use a new API.
>
>I thought adding a new API was what we wanted to avoid. But a new API
>is unnecessary as I am extending the existing one in such a way to not
>break the original api.
>
>The original code sent 4 bytes of data (the 32bit ip address) in each nested
>entry.  The new code sends the same 4 bytes containing the ip address.
>If a list of tags has been included the data is appended to the 4byte address.
>Type NLA_NESTED has no fixed data size.  If no vlans are supplied then the
>data sent to the kernel looks exactly the same as with the old iproute2.
>Therefor a new kernel will continue to work with the old iproute2 command,
>you just cant add vlan tags until you upgrade iproute2.
>
>An old kernel will continue to work with a new iproute2. The kernel will be
>sent the same 4Byte address in each nested entry.  If you try to add a list
>of vlan tags the kernel ignores the extra data.
>
>I tested the various combinations.

	Excellent, I stand corrected.  Could you include the above in
the commit message somewhere?  Doesn't necessarily need that amount of
detail, just a mention that the new logic is compatible in the proper
ways.

	-J
	
>>
>>        The existing IFLA_BOND_ARP_IP_TARGET type doesn't use nested
>>netlink types, it just sends binary data, so I'm thinking we can't just
>>change that binary data, and would need a new IFLA_BOND_ type.
>>
>>        -J
>>
>>>                                       target = strtok(NULL, ",");
>>>                               }
>>>                               addattr_nest_end(n, nest);
>>>--
>>>2.43.5
>>>
>>
>>---
>>        -Jay Vosburgh, jv@jvosburgh.net
>
>David Wilder  wilder@us.ibm.com

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