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:

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