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

[Jay Vosburgh <jay.vosburgh@canonical.com>]

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