[PATCHv2 net 0/2] bonding: fix incorrect mac address setting

[PATCHv2 net 0/2] bonding: fix incorrect mac address setting

[Hangbin Liu]

The mac address on backup slave should be convert from Solicited-Node
Multicast address, not from bonding unicast target address.

v2: fix patch 01's subject

Hangbin Liu (2):
  bonding: fix incorrect MAC address setting to receive NS messages
  selftests: bonding: fix incorrect mac address

 drivers/net/bonding/bond_options.c                          | 4 +++-
 tools/testing/selftests/drivers/net/bonding/bond_options.sh | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.46.0

[PATCHv2 net 1/2] bonding: fix incorrect MAC address setting to receive NS messages

[Hangbin Liu]

When validation on the backup slave is enabled, we need to validate the
Neighbor Solicitation (NS) messages received on the backup slave. To
receive these messages, the correct destination MAC address must be added
to the slave. However, the target in bonding is a unicast address, which
we cannot use directly. Instead, we should first convert it to a
Solicited-Node Multicast Address and then derive the corresponding MAC
address.

Fixes: 8eb36164d1a6 ("bonding: add ns target multicast address to slave device")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_options.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 327b6ecdc77e..63cf209dcdc9 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1246,6 +1246,7 @@ static void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool
 {
 	struct in6_addr *targets = bond->params.ns_targets;
 	char slot_maddr[MAX_ADDR_LEN];
+	struct in6_addr mcaddr;
 	int i;
 
 	if (!slave_can_set_ns_maddr(bond, slave))
@@ -1255,7 +1256,8 @@ static void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool
 		if (ipv6_addr_any(&targets[i]))
 			break;
 
-		if (!ndisc_mc_map(&targets[i], slot_maddr, slave->dev, 0)) {
+		addrconf_addr_solict_mult(&targets[i], &mcaddr);
+		if (!ndisc_mc_map(&mcaddr, slot_maddr, slave->dev, 0)) {
 			if (add)
 				dev_mc_add(slave->dev, slot_maddr);
 			else
-- 
2.46.0

[PATCHv2 net 2/2] selftests: bonding: fix incorrect mac address

[Hangbin Liu]

The correct mac address for NS target 2001:db8::254 is 33:33:ff:00:02:54,
not 33:33:00:00:02:54. The same with client maddress.

Fixes: 86fb6173d11e ("selftests: bonding: add ns multicast group testing")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/testing/selftests/drivers/net/bonding/bond_options.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/bonding/bond_options.sh b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
index edc56e2cc606..7bc148889ca7 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond_options.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
@@ -11,8 +11,8 @@ ALL_TESTS="
 
 lib_dir=$(dirname "$0")
 source ${lib_dir}/bond_topo_3d1c.sh
-c_maddr="33:33:00:00:00:10"
-g_maddr="33:33:00:00:02:54"
+c_maddr="33:33:ff:00:00:10"
+g_maddr="33:33:ff:00:02:54"
 
 skip_prio()
 {
-- 
2.46.0

Re: [PATCHv2 net 1/2] bonding: fix incorrect MAC address setting to receive NS messages

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>When validation on the backup slave is enabled, we need to validate the
>Neighbor Solicitation (NS) messages received on the backup slave. To
>receive these messages, the correct destination MAC address must be added
>to the slave. However, the target in bonding is a unicast address, which
>we cannot use directly. Instead, we should first convert it to a
>Solicited-Node Multicast Address and then derive the corresponding MAC
>address.
>
>Fixes: 8eb36164d1a6 ("bonding: add ns target multicast address to slave device")
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

	I think this now deserves some commentary in the code.  Not
because this function itself is unclear, but because there's the
similarly-named slave_set_ns_maddr() (singular, not plural as in this
patch) that will behave in a subtly different manner after this patch is
applied.

	The "maddrs" version here will convert the provided IPv6 address
to the IPv6 solicited-nodes multicast address (RFC 4291 section 2.7.1)
and thence to the MAC address, via ndisc_mc_map(), whereas the "maddr"
version uses the supplied IPv6 address directly for multicast MAC
address conversion.

	Assuming that I'm following that correctly, I think this
distinction deserves explanation.  And if I'm getting it wrong, then it
definitely deserves some explanation.

	FWIW, functionally, I think it's doing the correct thing.

	-J

>---
> drivers/net/bonding/bond_options.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 327b6ecdc77e..63cf209dcdc9 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -1246,6 +1246,7 @@ static void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool
> {
> 	struct in6_addr *targets = bond->params.ns_targets;
> 	char slot_maddr[MAX_ADDR_LEN];
>+	struct in6_addr mcaddr;
> 	int i;
> 
> 	if (!slave_can_set_ns_maddr(bond, slave))
>@@ -1255,7 +1256,8 @@ static void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool
> 		if (ipv6_addr_any(&targets[i]))
> 			break;
> 
>-		if (!ndisc_mc_map(&targets[i], slot_maddr, slave->dev, 0)) {
>+		addrconf_addr_solict_mult(&targets[i], &mcaddr);
>+		if (!ndisc_mc_map(&mcaddr, slot_maddr, slave->dev, 0)) {
> 			if (add)
> 				dev_mc_add(slave->dev, slot_maddr);
> 			else
>-- 
>2.46.0
>

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

Re: [PATCHv2 net 1/2] bonding: fix incorrect MAC address setting to receive NS messages

[Hangbin Liu]

On Thu, Feb 06, 2025 at 05:19:34PM -0800, Jay Vosburgh wrote:
> 	I think this now deserves some commentary in the code.  Not
> because this function itself is unclear, but because there's the
> similarly-named slave_set_ns_maddr() (singular, not plural as in this
> patch) that will behave in a subtly different manner after this patch is
> applied.

Yes, slave_set_ns_maddr() also need to be changed. The names are similar.
I will add some comments for the functions.

Thanks
Hangbin