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

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

v3: also fix the mac setting for slave_set_ns_maddr. (Jay)
    Add function description for slave_set_ns_maddr/slave_set_ns_maddrs (Jay)
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            | 55 ++++++++++++++++---
 .../drivers/net/bonding/bond_options.sh       |  4 +-
 2 files changed, 49 insertions(+), 10 deletions(-)

-- 
2.46.0

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

Fix the incorrect MAC address setting on both slave_set_ns_maddr() and
slave_set_ns_maddrs(). Since the two function names are similar. Add
some description for the functions. Also only use one mac_addr variable
in slave_set_ns_maddr() to save some code and logic.

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 | 55 +++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 327b6ecdc77e..d1b095af253b 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1242,10 +1242,28 @@ static bool slave_can_set_ns_maddr(const struct bonding *bond, struct slave *sla
 	       slave->dev->flags & IFF_MULTICAST;
 }
 
+/**
+ * slave_set_ns_maddrs - add/del all NS mac addresses for slave
+ * @bond: bond device
+ * @slave: slave device
+ * @add: add or remove all the NS mac addresses
+ *
+ * This function tries to add or delete all the NS mac addresses on the slave
+ *
+ * Note, the IPv6 NS target address is the unicast address in Neighbor
+ * Solicitation (NS) message. The dest address of NS message should be
+ * solicited-node multicast address of the target. The dest mac of NS message
+ * is converted from the solicited-node multicast address.
+ *
+ * This function is called when
+ *   * arp_validate changes
+ *   * enslaving, releasing new slaves
+ */
 static void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool add)
 {
 	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 +1273,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
@@ -1278,23 +1297,43 @@ void bond_slave_ns_maddrs_del(struct bonding *bond, struct slave *slave)
 	slave_set_ns_maddrs(bond, slave, false);
 }
 
+/**
+ * slave_set_ns_maddr - set new NS mac address for slave
+ * @bond: bond device
+ * @slave: slave device
+ * @target: the new IPv6 target
+ * @slot: the old IPv6 target in the slot
+ *
+ * This function tries to replace the old mac address to new one on the slave.
+ *
+ * Note, the target/slot IPv6 address is the unicast address in Neighbor
+ * Solicitation (NS) message. The dest address of NS message should be
+ * solicited-node multicast address of the target. The dest mac of NS message
+ * is converted from the solicited-node multicast address.
+ *
+ * This function is called when
+ *   * An IPv6 NS target is added or removed.
+ */
 static void slave_set_ns_maddr(struct bonding *bond, struct slave *slave,
 			       struct in6_addr *target, struct in6_addr *slot)
 {
-	char target_maddr[MAX_ADDR_LEN], slot_maddr[MAX_ADDR_LEN];
+	char mac_addr[MAX_ADDR_LEN];
+	struct in6_addr mcast_addr;
 
 	if (!bond->params.arp_validate || !slave_can_set_ns_maddr(bond, slave))
 		return;
 
-	/* remove the previous maddr from slave */
+	/* remove the previous mac addr from slave */
+	addrconf_addr_solict_mult(slot, &mcast_addr);
 	if (!ipv6_addr_any(slot) &&
-	    !ndisc_mc_map(slot, slot_maddr, slave->dev, 0))
-		dev_mc_del(slave->dev, slot_maddr);
+	    !ndisc_mc_map(&mcast_addr, mac_addr, slave->dev, 0))
+		dev_mc_del(slave->dev, mac_addr);
 
-	/* add new maddr on slave if target is set */
+	/* add new mac addr on slave if target is set */
+	addrconf_addr_solict_mult(target, &mcast_addr);
 	if (!ipv6_addr_any(target) &&
-	    !ndisc_mc_map(target, target_maddr, slave->dev, 0))
-		dev_mc_add(slave->dev, target_maddr);
+	    !ndisc_mc_map(&mcast_addr, mac_addr, slave->dev, 0))
+		dev_mc_add(slave->dev, mac_addr);
 }
 
 static void _bond_options_ns_ip6_target_set(struct bonding *bond, int slot,
-- 
2.46.0

[PATCHv3 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: [PATCHv3 net 0/2] bonding: fix incorrect mac address setting

[Hangbin Liu]

Please hold on this patch. Our QE reported that with bare NIC, the
backup NIC can't receive the NS messages even after joining the multicast
MAC group. But after remove the backup NIC from bond, the NIC interface
could receive the NS message.

This is weird, it looks the backup NIC dropped the NS message somewhere,
even using tcpdump (the NIC will be in promisc mode) I can't capture the
NS message on backup slave.

I need to debug more.

Thanks
Hangbin
On Fri, Feb 07, 2025 at 09:29:18AM +0000, Hangbin Liu wrote:
> The mac address on backup slave should be convert from Solicited-Node
> Multicast address, not from bonding unicast target address.
> 
> v3: also fix the mac setting for slave_set_ns_maddr. (Jay)
>     Add function description for slave_set_ns_maddr/slave_set_ns_maddrs (Jay)
> 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            | 55 ++++++++++++++++---
>  .../drivers/net/bonding/bond_options.sh       |  4 +-
>  2 files changed, 49 insertions(+), 10 deletions(-)
> 
> -- 
> 2.46.0
> 

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

[Hangbin Liu]

Hi Jay,
On Sat, Feb 08, 2025 at 06:34:21AM +0000, Hangbin Liu wrote:
> Please hold on this patch. Our QE reported that with bare NIC, the
> backup NIC can't receive the NS messages even after joining the multicast
> MAC group. But after remove the backup NIC from bond, the NIC interface
> could receive the NS message.
> 
> This is weird, it looks the backup NIC dropped the NS message somewhere,
> even using tcpdump (the NIC will be in promisc mode) I can't capture the
> NS message on backup slave.
> 
> I need to debug more.

After debug, I find it's a driver issue. The issue exists with ice dirver NIC.
I tried with a mlx5 NIC and the patch works good for me.

So I think you can start review this patch when you have time. I will debug
the ice driver later.

Thanks
Hangbin

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

[Hangbin Liu]

Hi Jay,

Any comments?

Thanks
Hangbin
On Tue, Feb 11, 2025 at 07:31:32AM +0000, Hangbin Liu wrote:
> Hi Jay,
> On Sat, Feb 08, 2025 at 06:34:21AM +0000, Hangbin Liu wrote:
> > Please hold on this patch. Our QE reported that with bare NIC, the
> > backup NIC can't receive the NS messages even after joining the multicast
> > MAC group. But after remove the backup NIC from bond, the NIC interface
> > could receive the NS message.
> > 
> > This is weird, it looks the backup NIC dropped the NS message somewhere,
> > even using tcpdump (the NIC will be in promisc mode) I can't capture the
> > NS message on backup slave.
> > 
> > I need to debug more.
> 
> After debug, I find it's a driver issue. The issue exists with ice dirver NIC.
> I tried with a mlx5 NIC and the patch works good for me.
> 
> So I think you can start review this patch when you have time. I will debug
> the ice driver later.
> 
> Thanks
> Hangbin

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

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>The mac address on backup slave should be convert from Solicited-Node
>Multicast address, not from bonding unicast target address.
>
>v3: also fix the mac setting for slave_set_ns_maddr. (Jay)
>    Add function description for slave_set_ns_maddr/slave_set_ns_maddrs (Jay)
>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

	For the series:

Acked-by: Jay Vosburgh <jv@jvosburgh.net>

	-J

> drivers/net/bonding/bond_options.c            | 55 ++++++++++++++++---
> .../drivers/net/bonding/bond_options.sh       |  4 +-
> 2 files changed, 49 insertions(+), 10 deletions(-)
>
>-- 
>2.46.0
>

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

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

[Nikolay Aleksandrov]

On 2/7/25 11:29, Hangbin Liu wrote:
> The mac address on backup slave should be convert from Solicited-Node
> Multicast address, not from bonding unicast target address.
> 
> v3: also fix the mac setting for slave_set_ns_maddr. (Jay)
>     Add function description for slave_set_ns_maddr/slave_set_ns_maddrs (Jay)
> 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            | 55 ++++++++++++++++---
>  .../drivers/net/bonding/bond_options.sh       |  4 +-
>  2 files changed, 49 insertions(+), 10 deletions(-)
> 

For the set:
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Hangbin Liu]

Hi David,

This patch set was marked as "Changes Requested" due to my initial reply.
The series has now been approved by Jay and Nikolay.

Could you help process it, or should I re-post it?

Thanks
Hangbin

On Fri, Feb 07, 2025 at 09:29:18AM +0000, Hangbin Liu wrote:
> The mac address on backup slave should be convert from Solicited-Node
> Multicast address, not from bonding unicast target address.
> 
> v3: also fix the mac setting for slave_set_ns_maddr. (Jay)
>     Add function description for slave_set_ns_maddr/slave_set_ns_maddrs (Jay)
> 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            | 55 ++++++++++++++++---
>  .../drivers/net/bonding/bond_options.sh       |  4 +-
>  2 files changed, 49 insertions(+), 10 deletions(-)
> 
> -- 
> 2.46.0
> 

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

[Jakub Kicinski]

On Wed, 5 Mar 2025 08:25:07 +0000 Hangbin Liu wrote:
> Could you help process it, or should I re-post it?

Repost.