[PATCH iproute2 0/3] iplink: bond: fix partner LACP state decoding

[PATCH iproute2 0/3] iplink: bond: fix partner LACP state decoding

[Dave Seddon]

I have been using an LLM to help port iproute2's netlink handling to
Go. While comparing the two decoders it flagged that ip reads
IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE one byte wide. I may well be
missing something; details below in case it is real.

Patch 1 is the one-line fix and stands on its own. Patches 2 and 3 add
a test for it. Please take just patch 1 if you would rather not have
the test.

Issue
- kernel sends u16: nla_put_u16() in bond_fill_slave_info(),
  drivers/net/bonding/bond_netlink.c:79. The field is
  struct port_params.port_state, u16, include/net/bond_3ad.h:205.
- iproute2 reads u8: rta_getattr_u8() in bond_slave_print_opt(),
  ip/iplink_bond_slave.c:148.
- little endian: first byte is the low byte. Output is right because
  the kernel never sets bits above 0xff (the value comes from the
  one-byte LACPDU actor_state, bond_3ad.c:562).
- big endian: first byte is the always-zero high byte. Every bond port
  prints:
    ad_partner_oper_port_state 0 ad_partner_oper_port_state_str <>
  Reproduced on s390x under qemu-user by replaying a generated message.
- regressed in 39ac2d2b802b (v5.6.0). That commit added the _str
  output and copied the actor (u8) block for the partner. The code
  before it used rta_getattr_u16().
- the kernel width is old: u16 in the struct since 1055c9aba3b6
  (2008), exported as u16 since 46ea297ed67c (2015). sysfs and the
  YNL spec agree with it, so the fix belongs in iproute2.
- /proc/net/bonding and sysfs read the struct directly and are fine,
  which is probably why nobody noticed.
- the other bond and bond slave attributes match bond_netlink.c; this
  is the only width mismatch.

Test
- tools/generate_nlmsg writes four RTM_NEWLINK blobs for a bond slave
  with partner state 0x003f, 0x0000, 0x0100 and absent.
- tests/ip/link/show_bond_slave_lacp_state.t decodes them with
  "ip -d monitor file" and checks the printed value. No root needed.
- 0x0100 cannot occur in real LACP. A one-byte reader prints 0 for it
  on little endian and 1 on big endian; only a u16 reader prints 256.
  That makes the bug visible on x86_64 too.

How it was tested
- x86_64, native: new test red with patch 1 reverted (0x0100 prints 0),
  green with it.
- s390x, qemu-user: the blobs were written by an s390x build of
  generate_nlmsg, so they are big endian (partner payload 01 00 for
  0x0100). ip built with and without patch 1:
    without: 0x3f prints "0 <>", 0x0100 prints "1 <active>"
    with:    0x3f prints "63 <active,...>", 0x0100 prints "256"
- results: without patch 1, 1 of 6 checks fails on x86_64 and 2 of 6
  on s390x. With it, all pass on both.
- whole testsuite, rootless, with the series: no change. add_tunnel.t
  and the two netns/set_nsid tests fail (no module load, no
  /var/run/netns), the same as on unpatched e11870d5.
- test_on_not fix checked against matching, mixed, absent and empty
  output, a bad regex and a missing file.
- builds with no warnings. checkpatch --strict is clean apart from the
  exec bit on the new .t, which matches the existing .t files.
- the new .t is shellcheck clean and passes under dash, busybox sh and
  bash --posix, so it does not depend on what /bin/sh points to.

Fix
- patch 1: rta_getattr_u8 -> rta_getattr_u16.
- patch 2: lib/generic.sh test_on_not uses "grep -vqE", which passes
  whenever any line does not match. Fixed so the new test can use it.
  Its one caller, tests/bridge/vlan/show.t, still passes.
- patch 3: the test.

LLM use
- The LLM spotted the mismatch, wrote the patches and drafted this
  letter. Each patch carries Assisted-by: LLM.
- Every result above comes from running the code, natively and under
  qemu-s390x. I have read all of it and can answer for it.

Dave Seddon (3):
  iplink_bond_slave: read ad_partner_oper_port_state as u16
  testsuite: fix test_on_not on multi-line output
  testsuite: add bond slave LACP port state test

 .gitignore                                    |   1 +
 ip/iplink_bond_slave.c                        |   2 +-
 testsuite/Makefile                            |   1 +
 testsuite/lib/generic.sh                      |   5 +-
 .../ip/link/show_bond_slave_lacp_state.t      |  33 ++++++
 testsuite/tools/generate_nlmsg.c              | 103 ++++++++++++++++--
 6 files changed, 133 insertions(+), 12 deletions(-)
 create mode 100755 testsuite/tests/ip/link/show_bond_slave_lacp_state.t

-- 
2.55.0

[PATCH iproute2 1/3] iplink_bond_slave: read ad_partner_oper_port_state as u16

[Dave Seddon]

The kernel emits IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE with
nla_put_u16() in bond_fill_slave_info(), but iproute2 reads only the
first payload byte with rta_getattr_u8().

On big-endian systems, valid LACP states have a zero high byte, so
both the numeric partner state and its decoded flags are displayed
as zero/empty. Little-endian systems display the low byte correctly
for the eight defined LACP state bits.

Restore the u16 accessor used before the string formatting change.
The actor state remains u8, matching the kernel attribute.

Fixes: 39ac2d2b802b ("iplink: bond: print lacp actor/partner oper states as strings")
Assisted-by: LLM
Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
---
 ip/iplink_bond_slave.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/iplink_bond_slave.c b/ip/iplink_bond_slave.c
index f4b28fbc..24e73575 100644
--- a/ip/iplink_bond_slave.c
+++ b/ip/iplink_bond_slave.c
@@ -145,7 +145,7 @@ static void bond_slave_print_opt(struct link_util *lu, FILE *f, struct rtattr *t
 	}
 
 	if (tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE]) {
-		__u16 state = rta_getattr_u8(tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE]);
+		__u16 state = rta_getattr_u16(tb[IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE]);
 
 		print_int(PRINT_ANY,
 			  "ad_partner_oper_port_state",
-- 
2.55.0

[PATCH iproute2 2/3] testsuite: fix test_on_not on multi-line output

[Dave Seddon]

grep -vqE succeeds when any line does not match, even if another line
contains the forbidden pattern. It also fails on empty output.

Accept only exit status 1 from a non-inverted grep: the pattern was
not found. A matching line or a grep error must fail the assertion.

Assisted-by: LLM
Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
---
 testsuite/lib/generic.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/testsuite/lib/generic.sh b/testsuite/lib/generic.sh
index 8b339ec1..ac48f023 100644
--- a/testsuite/lib/generic.sh
+++ b/testsuite/lib/generic.sh
@@ -113,8 +113,9 @@ test_on()
 
 test_on_not()
 {
-	echo -n "test on: \"$1\""
-	if cat "$STD_OUT" | grep -vqE "$1"
+	echo -n "test on absence: \"$1\""
+	grep -qE -- "$1" "$STD_OUT"
+	if [ "$?" -eq 1 ]
 	then
 		pr_success
 	else
-- 
2.55.0

[PATCH iproute2 3/3] testsuite: add bond slave LACP port state test

[Dave Seddon]

Decode generated RTM_NEWLINK messages with ip -d monitor file and
check the partner state value and flags, the u8 actor state, and
zero and absent partner attributes.

Use a synthetic 0x0100 partner state to detect a one-byte reader on
both endian types: it returns 0 on little-endian and 1 on big-endian
instead of 256. Real LACP states use only the low eight bits.

Check the actor state before asserting partner attribute absence,
so empty output cannot satisfy that test.

Assisted-by: LLM
Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
---
 .gitignore                                    |   1 +
 testsuite/Makefile                            |   1 +
 .../ip/link/show_bond_slave_lacp_state.t      |  33 ++++++
 testsuite/tools/generate_nlmsg.c              | 103 ++++++++++++++++--
 4 files changed, 129 insertions(+), 9 deletions(-)
 create mode 100755 testsuite/tests/ip/link/show_bond_slave_lacp_state.t

diff --git a/.gitignore b/.gitignore
index e5234a3d..a16f6362 100644
--- a/.gitignore
+++ b/.gitignore
@@ -39,3 +39,4 @@ testsuite/results
 testsuite/iproute2/iproute2-this
 testsuite/tools/generate_nlmsg
 testsuite/tests/ip/link/dev_wo_vf_rate.nl
+testsuite/tests/ip/link/bond_slave_partner_*.nl
diff --git a/testsuite/Makefile b/testsuite/Makefile
index fb50f618..bef66002 100644
--- a/testsuite/Makefile
+++ b/testsuite/Makefile
@@ -48,6 +48,7 @@ testclean:
 clean: testclean
 	@rm -f iproute2/iproute2-this
 	@rm -f tests/ip/link/dev_wo_vf_rate.nl
+	@rm -f tests/ip/link/bond_slave_partner_*.nl
 	$(MAKE) -C tools clean
 
 distclean: clean
diff --git a/testsuite/tests/ip/link/show_bond_slave_lacp_state.t b/testsuite/tests/ip/link/show_bond_slave_lacp_state.t
new file mode 100755
index 00000000..4ccf3960
--- /dev/null
+++ b/testsuite/tests/ip/link/show_bond_slave_lacp_state.t
@@ -0,0 +1,33 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+. lib/generic.sh
+
+# IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE is a u16 on the wire; the
+# actor state next to it is a u8. Each row decodes one blob written by
+# tools/generate_nlmsg and checks the printed value.
+#
+# The synthetic 0x0100 state checks the full attribute width on either
+# endian: a u8 reader returns 0 on little-endian and 1 on big-endian.
+#
+# row: file suffix | description | expected regex (!regex: must be absent)
+while IFS='|' read -r suffix desc expect; do
+	[ -z "$suffix" ] && continue
+	NL_FILE="tests/ip/link/bond_slave_partner_${suffix}.nl"
+	ts_ip "$0" "$desc" -d monitor file "$NL_FILE"
+	case "$expect" in
+	!*)
+		test_on "ad_actor_oper_port_state 63[^0-9]"
+		test_on_not "${expect#!}"
+		;;
+	*)
+		test_on "$expect"
+		;;
+	esac
+done <<EOF
+fast|partner state 0x3f|ad_partner_oper_port_state 63 ad_partner_oper_port_state_str <active,short_timeout,aggregating,in_sync,collecting,distributing>
+fast|actor state stays u8|ad_actor_oper_port_state 63[^0-9]
+zero|partner state 0|ad_partner_oper_port_state 0 ad_partner_oper_port_state_str <>
+hionly|partner state 0x0100, high byte only|ad_partner_oper_port_state 256[^0-9]
+absent|no partner attribute|!ad_partner_oper_port_state
+EOF
diff --git a/testsuite/tools/generate_nlmsg.c b/testsuite/tools/generate_nlmsg.c
index 106dca63..7c0aed3a 100644
--- a/testsuite/tools/generate_nlmsg.c
+++ b/testsuite/tools/generate_nlmsg.c
@@ -11,6 +11,9 @@
 #include <linux/if.h>
 #include <errno.h>
 #include <stdio.h>
+#include <string.h>
+#include <linux/if_link.h>
+#include "utils.h"
 
 int fill_vf_rate_test(void *buf, size_t buflen)
 {
@@ -40,6 +43,7 @@ int fill_vf_rate_test(void *buf, size_t buflen)
 #define ASSERT(x) if (x < 0) return -1
 #define ATTR_L(t, v, l)	ASSERT(addattr_l(h, buflen, t, v, l))
 #define ATTR_8(t, v)	ASSERT(addattr8(h, buflen, t, v))
+#define ATTR_16(t, v)	ASSERT(addattr16(h, buflen, t, v))
 #define ATTR_32(t, v)	ASSERT(addattr32(h, buflen, t, v))
 #define ATTR_STRZ(t, v)	ASSERT(addattrstrz(h, buflen, t, v))
 
@@ -87,26 +91,107 @@ int fill_vf_rate_test(void *buf, size_t buflen)
 	return h->nlmsg_len;
 }
 
+/* The synthetic 0x0100 state detects a u8 reader on either endian. */
+struct bond_slave_case {
+	const char *file;
+	__u16 partner_state;
+	bool has_partner;
+};
+
+static const struct bond_slave_case bond_slave_cases[] = {
+	{ "tests/ip/link/bond_slave_partner_fast.nl",	0x003f, true },
+	{ "tests/ip/link/bond_slave_partner_zero.nl",	0x0000, true },
+	{ "tests/ip/link/bond_slave_partner_hionly.nl",	0x0100, true },
+	{ "tests/ip/link/bond_slave_partner_absent.nl",	0x0000, false },
+};
+
+static int fill_bond_slave_test(void *buf, size_t buflen,
+				const struct bond_slave_case *c)
+{
+	__u8 mac[ETH_ALEN] = { 0x02, 0x00, 0x00, 0x00, 0x00, 0x01 };
+	struct rtattr *linkinfo, *slave_data;
+	struct nlmsghdr *h = buf;
+	struct ifinfomsg *ifi;
+
+	memset(buf, 0, buflen);
+	h->nlmsg_type = RTM_NEWLINK;
+	h->nlmsg_len = NLMSG_LENGTH(sizeof(*ifi));
+
+	ifi = NLMSG_DATA(h);
+	ifi->ifi_type = ARPHRD_ETHER;
+	ifi->ifi_index = 2;
+	ifi->ifi_flags = IFF_RUNNING | IFF_BROADCAST | IFF_MULTICAST |
+			 IFF_UP | IFF_LOWER_UP | IFF_SLAVE;
+
+	ATTR_STRZ(IFLA_IFNAME, "eth1");
+	ATTR_32(IFLA_MTU, 1500);
+	ATTR_L(IFLA_ADDRESS, mac, sizeof(mac));
+
+	linkinfo = NEST(IFLA_LINKINFO);
+	ATTR_STRZ(IFLA_INFO_SLAVE_KIND, "bond");
+	slave_data = NEST(IFLA_INFO_SLAVE_DATA);
+	ATTR_8(IFLA_BOND_SLAVE_STATE, 0);
+	ATTR_8(IFLA_BOND_SLAVE_MII_STATUS, 0);
+	ATTR_32(IFLA_BOND_SLAVE_LINK_FAILURE_COUNT, 0);
+	ATTR_L(IFLA_BOND_SLAVE_PERM_HWADDR, mac, sizeof(mac));
+	ATTR_16(IFLA_BOND_SLAVE_QUEUE_ID, 0);
+	ATTR_16(IFLA_BOND_SLAVE_AD_AGGREGATOR_ID, 1);
+	ATTR_8(IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE, 0x3f);
+	if (c->has_partner)
+		ATTR_16(IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE,
+			c->partner_state);
+	NEST_END(slave_data);
+	NEST_END(linkinfo);
+
+	return h->nlmsg_len;
+}
+
+static int write_nlmsg(const char *file, const void *buf, int msglen)
+{
+	FILE *fp;
+
+	fp = fopen(file, "wb");
+	if (!fp) {
+		perror("fopen()");
+		return -1;
+	}
+	if (fwrite(buf, msglen, 1, fp) != 1) {
+		perror("fwrite()");
+		fclose(fp);
+		return -1;
+	}
+	if (fclose(fp)) {
+		perror("fclose()");
+		return -1;
+	}
+	return 0;
+}
+
 int main(void)
 {
 	char buf[16384] = { 0 };
+	unsigned int i;
 	int msglen;
-	FILE *fp;
 
 	msglen = fill_vf_rate_test(buf, sizeof(buf));
 	if (msglen < 0) {
 		fprintf(stderr, "fill_vf_rate_test() failed!\n");
 		return 1;
 	}
-	fp = fopen("tests/ip/link/dev_wo_vf_rate.nl", "w");
-	if (!fp) {
-		perror("fopen()");
-		return 1;
-	}
-	if (fwrite(buf, msglen, 1, fp) != 1) {
-		perror("fwrite()");
+	if (write_nlmsg("tests/ip/link/dev_wo_vf_rate.nl", buf, msglen))
 		return 1;
+
+	for (i = 0; i < ARRAY_SIZE(bond_slave_cases); i++) {
+		const struct bond_slave_case *c = &bond_slave_cases[i];
+
+		msglen = fill_bond_slave_test(buf, sizeof(buf), c);
+		if (msglen < 0) {
+			fprintf(stderr, "fill_bond_slave_test(%s) failed!\n",
+				c->file);
+			return 1;
+		}
+		if (write_nlmsg(c->file, buf, msglen))
+			return 1;
 	}
-	fclose(fp);
 	return 0;
 }
-- 
2.55.0