[PATCH net] bonding: fix lladdr finding and confirmation

[PATCH net] bonding: fix lladdr finding and confirmation

[Hangbin Liu]

There are 3 issues when setting lladdr as bonding IPv6 target

1. On the send side. When ns_ip6_target was set, the ipv6_dev_get_saddr()
   will be called to get available src addr and send IPv6 neighbor solicit
   message.

   If the target is global address, ipv6_dev_get_saddr() will get any
   available src address. But if the target is link local address,
   ipv6_dev_get_saddr() will only get available address from out interace,
   i.e. the corresponding bond interface.

   But before bond interface up, all the address is tentative, while
   ipv6_dev_get_saddr() will ignore tentative address. This makes we can't
   find available link local src address, then bond_ns_send() will not be
   called and no NS message was sent. Thus no NA message received and bond
   interface will keep in down state.

   Fix this by sending NS with unspecified address if there is no available
   source address.

2. On the receive side. The slave was set down before enslave to bond.
   This makes slaves remove mcast address 33:33:00:00:00:01( The IPv6
   maddr ff02::1 is kept even when the interface down). When bond set
   slave up, the ipv6_mc_up() was not called due to commit c2edacf80e15
   ("bonding / ipv6: no addrconf for slaves separately from master").
   This makes the slave interface never add the all node mcast address
   33:33:00:00:00:01. So there is no way to accept unsolicited NA with
   dest ff02::1.

   Fix this by adding all node mcast address 33:33:00:00:00:01 back when
   the slave interface up.

3. On the validating side. The NA message with all-nodes multicast dest
   address should also be valid.

   Also rename bond_validate_ns() to bond_validate_na().

Reported-by: LiLiang <liali@redhat.com>
Fixes: 5e1eeef69c0f ("bonding: NS target should accept link local address")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 20 +++++++++++++++-----
 net/ipv6/addrconf.c             |  7 +++++--
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2f4da2c13c0a..5c2febe94428 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3167,6 +3167,9 @@ static void bond_ns_send_all(struct bonding *bond, struct slave *slave)
 found:
 		if (!ipv6_dev_get_saddr(dev_net(dst->dev), dst->dev, &targets[i], 0, &saddr))
 			bond_ns_send(slave, &targets[i], &saddr, tags);
+		else
+			bond_ns_send(slave, &targets[i], &in6addr_any, tags);
+
 		dst_release(dst);
 		kfree(tags);
 	}
@@ -3198,12 +3201,19 @@ static bool bond_has_this_ip6(struct bonding *bond, struct in6_addr *addr)
 	return ret;
 }
 
-static void bond_validate_ns(struct bonding *bond, struct slave *slave,
+static void bond_validate_na(struct bonding *bond, struct slave *slave,
 			     struct in6_addr *saddr, struct in6_addr *daddr)
 {
 	int i;
 
-	if (ipv6_addr_any(saddr) || !bond_has_this_ip6(bond, daddr)) {
+	/* Ignore NAs that:
+	 * 1. Source address is unspecified address.
+	 * 2. Dest address is neither all-nodes multicast address nor
+	 *    exist on bond interface.
+	 */
+	if (ipv6_addr_any(saddr) ||
+	    (!ipv6_addr_equal(daddr, &in6addr_linklocal_allnodes) &&
+	     !bond_has_this_ip6(bond, daddr))) {
 		slave_dbg(bond->dev, slave->dev, "%s: sip %pI6c tip %pI6c not found\n",
 			  __func__, saddr, daddr);
 		return;
@@ -3246,14 +3256,14 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
 	 * see bond_arp_rcv().
 	 */
 	if (bond_is_active_slave(slave))
-		bond_validate_ns(bond, slave, saddr, daddr);
+		bond_validate_na(bond, slave, saddr, daddr);
 	else if (curr_active_slave &&
 		 time_after(slave_last_rx(bond, curr_active_slave),
 			    curr_active_slave->last_link_up))
-		bond_validate_ns(bond, slave, saddr, daddr);
+		bond_validate_na(bond, slave, saddr, daddr);
 	else if (curr_arp_slave &&
 		 bond_time_in_interval(bond, slave_last_tx(curr_arp_slave), 1))
-		bond_validate_ns(bond, slave, saddr, daddr);
+		bond_validate_na(bond, slave, saddr, daddr);
 
 out:
 	return RX_HANDLER_ANOTHER;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index e15f64f22fa8..77750b6327e7 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3557,11 +3557,14 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 		fallthrough;
 	case NETDEV_UP:
 	case NETDEV_CHANGE:
-		if (dev->flags & IFF_SLAVE)
+		if (idev && idev->cnf.disable_ipv6)
 			break;
 
-		if (idev && idev->cnf.disable_ipv6)
+		if (dev->flags & IFF_SLAVE) {
+			if (event == NETDEV_UP && !IS_ERR_OR_NULL(idev))
+				ipv6_mc_up(idev);
 			break;
+		}
 
 		if (event == NETDEV_UP) {
 			/* restore routes for permanent addresses */
-- 
2.37.1

Re: [PATCH net] bonding: fix lladdr finding and confirmation

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>There are 3 issues when setting lladdr as bonding IPv6 target
>
>1. On the send side. When ns_ip6_target was set, the ipv6_dev_get_saddr()
>   will be called to get available src addr and send IPv6 neighbor solicit
>   message.
>
>   If the target is global address, ipv6_dev_get_saddr() will get any
>   available src address. But if the target is link local address,
>   ipv6_dev_get_saddr() will only get available address from out interace,

	Should this be "our interface"?

>   i.e. the corresponding bond interface.
>
>   But before bond interface up, all the address is tentative, while
>   ipv6_dev_get_saddr() will ignore tentative address. This makes we can't
>   find available link local src address, then bond_ns_send() will not be
>   called and no NS message was sent. Thus no NA message received and bond
>   interface will keep in down state.
>
>   Fix this by sending NS with unspecified address if there is no available
>   source address.
>
>2. On the receive side. The slave was set down before enslave to bond.
>   This makes slaves remove mcast address 33:33:00:00:00:01( The IPv6
>   maddr ff02::1 is kept even when the interface down). When bond set
>   slave up, the ipv6_mc_up() was not called due to commit c2edacf80e15
>   ("bonding / ipv6: no addrconf for slaves separately from master").
>   This makes the slave interface never add the all node mcast address
>   33:33:00:00:00:01. So there is no way to accept unsolicited NA with
>   dest ff02::1.
>
>   Fix this by adding all node mcast address 33:33:00:00:00:01 back when
>   the slave interface up.
>
>3. On the validating side. The NA message with all-nodes multicast dest
>   address should also be valid.
>
>   Also rename bond_validate_ns() to bond_validate_na().

	I'm not exactly sure which change matches which of the three
above fixes; should this be three separate patches?

>Reported-by: LiLiang <liali@redhat.com>
>Fixes: 5e1eeef69c0f ("bonding: NS target should accept link local address")

	Is this fixes tag correct for all the fixes?  Number 2 cites a
different commit (c2edacf80e15).  Again, should these be three separate
patches?

>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> drivers/net/bonding/bond_main.c | 20 +++++++++++++++-----
> net/ipv6/addrconf.c             |  7 +++++--
> 2 files changed, 20 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 2f4da2c13c0a..5c2febe94428 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3167,6 +3167,9 @@ static void bond_ns_send_all(struct bonding *bond, struct slave *slave)
> found:
> 		if (!ipv6_dev_get_saddr(dev_net(dst->dev), dst->dev, &targets[i], 0, &saddr))
> 			bond_ns_send(slave, &targets[i], &saddr, tags);
>+		else
>+			bond_ns_send(slave, &targets[i], &in6addr_any, tags);
>+
> 		dst_release(dst);
> 		kfree(tags);
> 	}
>@@ -3198,12 +3201,19 @@ static bool bond_has_this_ip6(struct bonding *bond, struct in6_addr *addr)
> 	return ret;
> }
> 
>-static void bond_validate_ns(struct bonding *bond, struct slave *slave,
>+static void bond_validate_na(struct bonding *bond, struct slave *slave,
> 			     struct in6_addr *saddr, struct in6_addr *daddr)
> {
> 	int i;
> 
>-	if (ipv6_addr_any(saddr) || !bond_has_this_ip6(bond, daddr)) {
>+	/* Ignore NAs that:
>+	 * 1. Source address is unspecified address.
>+	 * 2. Dest address is neither all-nodes multicast address nor
>+	 *    exist on bond interface.
>+	 */
>+	if (ipv6_addr_any(saddr) ||
>+	    (!ipv6_addr_equal(daddr, &in6addr_linklocal_allnodes) &&
>+	     !bond_has_this_ip6(bond, daddr))) {
> 		slave_dbg(bond->dev, slave->dev, "%s: sip %pI6c tip %pI6c not found\n",
> 			  __func__, saddr, daddr);
> 		return;
>@@ -3246,14 +3256,14 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
> 	 * see bond_arp_rcv().
> 	 */
> 	if (bond_is_active_slave(slave))
>-		bond_validate_ns(bond, slave, saddr, daddr);
>+		bond_validate_na(bond, slave, saddr, daddr);
> 	else if (curr_active_slave &&
> 		 time_after(slave_last_rx(bond, curr_active_slave),
> 			    curr_active_slave->last_link_up))
>-		bond_validate_ns(bond, slave, saddr, daddr);
>+		bond_validate_na(bond, slave, saddr, daddr);
> 	else if (curr_arp_slave &&
> 		 bond_time_in_interval(bond, slave_last_tx(curr_arp_slave), 1))
>-		bond_validate_ns(bond, slave, saddr, daddr);
>+		bond_validate_na(bond, slave, saddr, daddr);

	Is this logic correct?  If I'm not mistaken, there are two
receive cases:

	1- We receive a reply (Neighbor Advertisement) to our request
(Neighbor Solicitation).

	2- We receive a copy of our request (NS), which passed through
the switch and was received by another interface of the bond.

	For the ARP monitor implementation, in the second case, the
source and target IP addresses are swapped for the validation.

	Is such a swap necessary for the NS/NA monitor implementation?
I would expect this to be in the second block of the if (inside the
"else if (curr_active_slave &&" block).

	-J

> out:
> 	return RX_HANDLER_ANOTHER;
>diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>index e15f64f22fa8..77750b6327e7 100644
>--- a/net/ipv6/addrconf.c
>+++ b/net/ipv6/addrconf.c
>@@ -3557,11 +3557,14 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
> 		fallthrough;
> 	case NETDEV_UP:
> 	case NETDEV_CHANGE:
>-		if (dev->flags & IFF_SLAVE)
>+		if (idev && idev->cnf.disable_ipv6)
> 			break;
> 
>-		if (idev && idev->cnf.disable_ipv6)
>+		if (dev->flags & IFF_SLAVE) {
>+			if (event == NETDEV_UP && !IS_ERR_OR_NULL(idev))
>+				ipv6_mc_up(idev);
> 			break;
>+		}
> 
> 		if (event == NETDEV_UP) {
> 			/* restore routes for permanent addresses */
>-- 
>2.37.1
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: fix lladdr finding and confirmation

[Hangbin Liu]

On Sun, Aug 28, 2022 at 04:20:43PM -0700, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> >There are 3 issues when setting lladdr as bonding IPv6 target
> >
> >1. On the send side. When ns_ip6_target was set, the ipv6_dev_get_saddr()
> >   will be called to get available src addr and send IPv6 neighbor solicit
> >   message.
> >
> >   If the target is global address, ipv6_dev_get_saddr() will get any
> >   available src address. But if the target is link local address,
> >   ipv6_dev_get_saddr() will only get available address from out interace,
> 
> 	Should this be "our interface"?

Ah, yes.

> >
> >2. On the receive side. The slave was set down before enslave to bond.
> >   This makes slaves remove mcast address 33:33:00:00:00:01( The IPv6
> >   maddr ff02::1 is kept even when the interface down). When bond set
> >   slave up, the ipv6_mc_up() was not called due to commit c2edacf80e15
> >   ("bonding / ipv6: no addrconf for slaves separately from master").
> >   This makes the slave interface never add the all node mcast address
> >   33:33:00:00:00:01. So there is no way to accept unsolicited NA with
> >   dest ff02::1.
> >
> >   Fix this by adding all node mcast address 33:33:00:00:00:01 back when
> >   the slave interface up.
> >
> >3. On the validating side. The NA message with all-nodes multicast dest
> >   address should also be valid.
> >
> >   Also rename bond_validate_ns() to bond_validate_na().
> 
> 	I'm not exactly sure which change matches which of the three
> above fixes; should this be three separate patches?

The 1st case(send side) is fixed in function bond_ns_send_all().
The 2nd case(receive side) is fixed in addrconf_notify().
The 3rd case(validating side) is fixed in bond_validate_ns/na()

> 
> >Reported-by: LiLiang <liali@redhat.com>
> >Fixes: 5e1eeef69c0f ("bonding: NS target should accept link local address")
> 
> 	Is this fixes tag correct for all the fixes?  Number 2 cites a
> different commit (c2edacf80e15). 

Before we support link local target for bonding. Commit (c2edacf80e15) works
as bond device could up and add the all node multicast correctly.

After we adding the link local target for bonding. The bond could not up
and not able to add node multicast address. So I think the fixes tag should
not be commit (c2edacf80e15).

> Again, should these be three separate patches?

I thought these 3 parts are all to fix lladdr target. So I put them together.
If you think it's easier to review. I can separate the patches of course.

> 
> >@@ -3246,14 +3256,14 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
> > 	 * see bond_arp_rcv().
> > 	 */
> > 	if (bond_is_active_slave(slave))
> >-		bond_validate_ns(bond, slave, saddr, daddr);
> >+		bond_validate_na(bond, slave, saddr, daddr);
> > 	else if (curr_active_slave &&
> > 		 time_after(slave_last_rx(bond, curr_active_slave),
> > 			    curr_active_slave->last_link_up))
> >-		bond_validate_ns(bond, slave, saddr, daddr);
> >+		bond_validate_na(bond, slave, saddr, daddr);
> > 	else if (curr_arp_slave &&
> > 		 bond_time_in_interval(bond, slave_last_tx(curr_arp_slave), 1))
> >-		bond_validate_ns(bond, slave, saddr, daddr);
> >+		bond_validate_na(bond, slave, saddr, daddr);
> 
> 	Is this logic correct?  If I'm not mistaken, there are two
> receive cases:
> 
> 	1- We receive a reply (Neighbor Advertisement) to our request
> (Neighbor Solicitation).
> 
> 	2- We receive a copy of our request (NS), which passed through
> the switch and was received by another interface of the bond.

No, we don't have this case for IPv6 because I did a check in

static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
                       struct slave *slave)
{
	[...]

        if (skb->pkt_type == PACKET_OTHERHOST ||
            skb->pkt_type == PACKET_LOOPBACK ||
            hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
                goto out;

Here we will ignore none NA messages.

Thanks
Hangbin
> 
> 	For the ARP monitor implementation, in the second case, the
> source and target IP addresses are swapped for the validation.
> 
> 	Is such a swap necessary for the NS/NA monitor implementation?
> I would expect this to be in the second block of the if (inside the
> "else if (curr_active_slave &&" block).
> 
> 	-J
> 
> > out:
> > 	return RX_HANDLER_ANOTHER;
> >diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> >index e15f64f22fa8..77750b6327e7 100644
> >--- a/net/ipv6/addrconf.c
> >+++ b/net/ipv6/addrconf.c
> >@@ -3557,11 +3557,14 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
> > 		fallthrough;
> > 	case NETDEV_UP:
> > 	case NETDEV_CHANGE:
> >-		if (dev->flags & IFF_SLAVE)
> >+		if (idev && idev->cnf.disable_ipv6)
> > 			break;
> > 
> >-		if (idev && idev->cnf.disable_ipv6)
> >+		if (dev->flags & IFF_SLAVE) {
> >+			if (event == NETDEV_UP && !IS_ERR_OR_NULL(idev))
> >+				ipv6_mc_up(idev);
> > 			break;
> >+		}
> > 
> > 		if (event == NETDEV_UP) {
> > 			/* restore routes for permanent addresses */
> >-- 
> >2.37.1
> >
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: fix lladdr finding and confirmation

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

[...]
>> 	I'm not exactly sure which change matches which of the three
>> above fixes; should this be three separate patches?
>
>The 1st case(send side) is fixed in function bond_ns_send_all().
>The 2nd case(receive side) is fixed in addrconf_notify().
>The 3rd case(validating side) is fixed in bond_validate_ns/na()
>
>> 
>> >Reported-by: LiLiang <liali@redhat.com>
>> >Fixes: 5e1eeef69c0f ("bonding: NS target should accept link local address")
>> 
>> 	Is this fixes tag correct for all the fixes?  Number 2 cites a
>> different commit (c2edacf80e15). 
>
>Before we support link local target for bonding. Commit (c2edacf80e15) works
>as bond device could up and add the all node multicast correctly.
>
>After we adding the link local target for bonding. The bond could not up
>and not able to add node multicast address. So I think the fixes tag should
>not be commit (c2edacf80e15).
>
>> Again, should these be three separate patches?
>
>I thought these 3 parts are all to fix lladdr target. So I put them together.
>If you think it's easier to review. I can separate the patches of course.

	I see the set of three posted separately, thanks.

>> 
>> >@@ -3246,14 +3256,14 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
>> > 	 * see bond_arp_rcv().
>> > 	 */
>> > 	if (bond_is_active_slave(slave))
>> >-		bond_validate_ns(bond, slave, saddr, daddr);
>> >+		bond_validate_na(bond, slave, saddr, daddr);
>> > 	else if (curr_active_slave &&
>> > 		 time_after(slave_last_rx(bond, curr_active_slave),
>> > 			    curr_active_slave->last_link_up))
>> >-		bond_validate_ns(bond, slave, saddr, daddr);
>> >+		bond_validate_na(bond, slave, saddr, daddr);
>> > 	else if (curr_arp_slave &&
>> > 		 bond_time_in_interval(bond, slave_last_tx(curr_arp_slave), 1))
>> >-		bond_validate_ns(bond, slave, saddr, daddr);
>> >+		bond_validate_na(bond, slave, saddr, daddr);
>> 
>> 	Is this logic correct?  If I'm not mistaken, there are two
>> receive cases:
>> 
>> 	1- We receive a reply (Neighbor Advertisement) to our request
>> (Neighbor Solicitation).
>> 
>> 	2- We receive a copy of our request (NS), which passed through
>> the switch and was received by another interface of the bond.
>
>No, we don't have this case for IPv6 because I did a check in
>
>static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
>                       struct slave *slave)
>{
>	[...]
>
>        if (skb->pkt_type == PACKET_OTHERHOST ||
>            skb->pkt_type == PACKET_LOOPBACK ||
>            hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
>                goto out;
>
>Here we will ignore none NA messages.

	Is there a reason to implement this differently from the ARP
monitor with regard to the "passed request through switch to backup
interface" logic?  Are the backup interfaces then always down until the
active interface fails (in other words, what do they receive)?

	Assuming that there is a good reason, the commentary in
bond_na_rcv() is misleading, as it says to "see bond_arp_rcv()" for the
logic.  Again, assuming there's a good reason for it, can you amend this
comment to mention that the "Note: for (b)" in the bond_arp_rcv()
commentary does not apply to bond_na_rcv() for whatever the good reason
is?

	-J

>Thanks
>Hangbin
>> 
>> 	For the ARP monitor implementation, in the second case, the
>> source and target IP addresses are swapped for the validation.
>> 
>> 	Is such a swap necessary for the NS/NA monitor implementation?
>> I would expect this to be in the second block of the if (inside the
>> "else if (curr_active_slave &&" block).
>> 
>> 	-J
>> 
>> > out:
>> > 	return RX_HANDLER_ANOTHER;
>> >diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> >index e15f64f22fa8..77750b6327e7 100644
>> >--- a/net/ipv6/addrconf.c
>> >+++ b/net/ipv6/addrconf.c
>> >@@ -3557,11 +3557,14 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
>> > 		fallthrough;
>> > 	case NETDEV_UP:
>> > 	case NETDEV_CHANGE:
>> >-		if (dev->flags & IFF_SLAVE)
>> >+		if (idev && idev->cnf.disable_ipv6)
>> > 			break;
>> > 
>> >-		if (idev && idev->cnf.disable_ipv6)
>> >+		if (dev->flags & IFF_SLAVE) {
>> >+			if (event == NETDEV_UP && !IS_ERR_OR_NULL(idev))
>> >+				ipv6_mc_up(idev);
>> > 			break;
>> >+		}
>> > 
>> > 		if (event == NETDEV_UP) {
>> > 			/* restore routes for permanent addresses */
>> >-- 
>> >2.37.1

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: fix lladdr finding and confirmation

[Hangbin Liu]

On Tue, Aug 30, 2022 at 07:53:39PM -0700, Jay Vosburgh wrote:
> >> >@@ -3246,14 +3256,14 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
> >> > 	 * see bond_arp_rcv().
> >> > 	 */
> >> > 	if (bond_is_active_slave(slave))
> >> >-		bond_validate_ns(bond, slave, saddr, daddr);
> >> >+		bond_validate_na(bond, slave, saddr, daddr);
> >> > 	else if (curr_active_slave &&
> >> > 		 time_after(slave_last_rx(bond, curr_active_slave),
> >> > 			    curr_active_slave->last_link_up))
> >> >-		bond_validate_ns(bond, slave, saddr, daddr);
> >> >+		bond_validate_na(bond, slave, saddr, daddr);
> >> > 	else if (curr_arp_slave &&
> >> > 		 bond_time_in_interval(bond, slave_last_tx(curr_arp_slave), 1))
> >> >-		bond_validate_ns(bond, slave, saddr, daddr);
> >> >+		bond_validate_na(bond, slave, saddr, daddr);
> >> 
> >> 	Is this logic correct?  If I'm not mistaken, there are two
> >> receive cases:
> >> 
> >> 	1- We receive a reply (Neighbor Advertisement) to our request
> >> (Neighbor Solicitation).
> >> 
> >> 	2- We receive a copy of our request (NS), which passed through
> >> the switch and was received by another interface of the bond.
> >
> >No, we don't have this case for IPv6 because I did a check in
> >
> >static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
> >                       struct slave *slave)
> >{
> >	[...]
> >
> >        if (skb->pkt_type == PACKET_OTHERHOST ||
> >            skb->pkt_type == PACKET_LOOPBACK ||
> >            hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
> >                goto out;
> >
> >Here we will ignore none NA messages.
> 
> 	Is there a reason to implement this differently from the ARP
> monitor with regard to the "passed request through switch to backup
> interface" logic?  Are the backup interfaces then always down until the
> active interface fails (in other words, what do they receive)?

Hmm... There do have some differences between the ARP monitor and NS/NA monitor.

For ARP monitor
"""
	For an active slave, the validation checks ARP replies to confirm
	that they were generated by an arp_ip_target.  Since backup slaves
	do not typically receive these replies, the validation performed
	for backup slaves is on the broadcast ARP request sent out via the
	active slave.  
"""

But IPv6 NS message is a little different. For our solicited NS message, the
dest mac is set to correspond solicited-node multicast address instead of
broadcast address. So the backup slave will not receive the NS message that
send from active slave.

When we send unsolicited NS with in6addr_any. The target will reply NA with
dest addr ff02::1. This is the only time the backup salve could receive NA
message.

If you think this is OK, I can update the comments.

Thanks
Hangbin

> 
> 	Assuming that there is a good reason, the commentary in
> bond_na_rcv() is misleading, as it says to "see bond_arp_rcv()" for the
> logic.  Again, assuming there's a good reason for it, can you amend this
> comment to mention that the "Note: for (b)" in the bond_arp_rcv()
> commentary does not apply to bond_na_rcv() for whatever the good reason
> is?
> 
> 	-J
> 
> >Thanks
> >Hangbin
> >> 
> >> 	For the ARP monitor implementation, in the second case, the
> >> source and target IP addresses are swapped for the validation.
> >> 
> >> 	Is such a swap necessary for the NS/NA monitor implementation?
> >> I would expect this to be in the second block of the if (inside the
> >> "else if (curr_active_slave &&" block).
> >> 
> >> 	-J
> >> 
> >> > out:
> >> > 	return RX_HANDLER_ANOTHER;
> >> >diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> >> >index e15f64f22fa8..77750b6327e7 100644
> >> >--- a/net/ipv6/addrconf.c
> >> >+++ b/net/ipv6/addrconf.c
> >> >@@ -3557,11 +3557,14 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
> >> > 		fallthrough;
> >> > 	case NETDEV_UP:
> >> > 	case NETDEV_CHANGE:
> >> >-		if (dev->flags & IFF_SLAVE)
> >> >+		if (idev && idev->cnf.disable_ipv6)
> >> > 			break;
> >> > 
> >> >-		if (idev && idev->cnf.disable_ipv6)
> >> >+		if (dev->flags & IFF_SLAVE) {
> >> >+			if (event == NETDEV_UP && !IS_ERR_OR_NULL(idev))
> >> >+				ipv6_mc_up(idev);
> >> > 			break;
> >> >+		}
> >> > 
> >> > 		if (event == NETDEV_UP) {
> >> > 			/* restore routes for permanent addresses */
> >> >-- 
> >> >2.37.1
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com