Re: [PATCH] net: bonding: Add support for IPV6 ns/na

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

Sun Shouxin <sunshouxin@chinatelecom.cn> wrote:

>Since ipv6 neighbor solicitation and advertisement messages
>isn't handled gracefully in bonding6 driver, we can see packet
>drop due to inconsistency bewteen mac address in the option
>message and source MAC .

	Could you provide a specific example where this occurs?

>Another examples is ipv6 neighbor solicitation and advertisement
>messages from VM via tap attached to host brighe, the src mac
>mighe be changed through balance-alb mode, but it is not synced
>with Link-layer address in the option message.

	What happens if the MAC assignment changes because alb does a
rebalance?

>The patch implements bond6's tx handle for ipv6 neighbor
>solicitation and advertisement messages.

	A few additional minor comments below.

>Suggested-by: Hu Yadi <huyd12@chinatelecom.cn>
>Signed-off-by: Sun Shouxin <sunshouxin@chinatelecom.cn>
>---
> drivers/net/bonding/bond_alb.c | 127 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 127 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index 2ec8e01..01566ba 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -22,6 +22,7 @@
> #include <asm/byteorder.h>
> #include <net/bonding.h>
> #include <net/bond_alb.h>
>+#include <net/ndisc.h>
> 
> static const u8 mac_v6_allmcast[ETH_ALEN + 2] __long_aligned = {
> 	0x33, 0x33, 0x00, 0x00, 0x00, 0x01
>@@ -1269,6 +1270,112 @@ static int alb_set_mac_address(struct bonding *bond, void *addr)
> 	return res;
> }
> 
>+static void alb_change_nd_option(struct sk_buff *skb, void *data)
>+{
>+	struct nd_msg *msg = (struct nd_msg *)skb_transport_header(skb);
>+	struct nd_opt_hdr *nd_opt = (struct nd_opt_hdr *)msg->opt;
>+	struct net_device *dev = skb->dev;
>+	struct icmp6hdr *icmp6h = icmp6_hdr(skb);
>+	struct ipv6hdr *ip6hdr = ipv6_hdr(skb);
>+	u8 *lladdr = NULL;
>+	u32 ndoptlen = skb_tail_pointer(skb) - (skb_transport_header(skb) +
>+				offsetof(struct nd_msg, opt));
>+
>+	while (ndoptlen) {
>+		int l;
>+
>+		switch (nd_opt->nd_opt_type) {
>+		case ND_OPT_SOURCE_LL_ADDR:
>+		case ND_OPT_TARGET_LL_ADDR:
>+		lladdr = ndisc_opt_addr_data(nd_opt, dev);
>+		break;
>+
>+		default:
>+		break;
>+		}
>+
>+		l = nd_opt->nd_opt_len << 3;
>+
>+		if (ndoptlen < l || l == 0)
>+			return;
>+
>+		if (lladdr) {
>+			memcpy(lladdr, data, dev->addr_len);
>+			lladdr = NULL;
>+			icmp6h->icmp6_cksum = 0;
>+
>+			icmp6h->icmp6_cksum = csum_ipv6_magic(&ip6hdr->saddr,
>+							      &ip6hdr->daddr,
>+						ntohs(ip6hdr->payload_len),
>+						IPPROTO_ICMPV6,
>+						csum_partial(icmp6h,
>+							     ntohs(ip6hdr->payload_len), 0));
>+			lladdr = NULL;

	"lladdr = NULL" could be in the default: case, above, instead of
being done here (and it's here twice).

>+		}
>+		ndoptlen -= l;
>+		nd_opt = ((void *)nd_opt) + l;
>+	}
>+}
>+
>+static u8 *alb_get_lladdr(struct sk_buff *skb)
>+{
>+	struct nd_msg *msg = (struct nd_msg *)skb_transport_header(skb);
>+	struct nd_opt_hdr *nd_opt = (struct nd_opt_hdr *)msg->opt;
>+	struct net_device *dev = skb->dev;
>+	u8 *lladdr = NULL;
>+	u32 ndoptlen = skb_tail_pointer(skb) - (skb_transport_header(skb) +
>+				offsetof(struct nd_msg, opt));
>+
>+	while (ndoptlen) {
>+		int l;
>+
>+		switch (nd_opt->nd_opt_type) {
>+		case ND_OPT_SOURCE_LL_ADDR:
>+		case ND_OPT_TARGET_LL_ADDR:
>+			lladdr = ndisc_opt_addr_data(nd_opt, dev);
>+			break;
>+
>+		default:
>+			break;
>+		}
>+
>+		l = nd_opt->nd_opt_len << 3;
>+
>+		if (ndoptlen < l || l == 0)
>+			return lladdr;
>+
>+		if (lladdr)
>+			return lladdr;
>+
>+		ndoptlen -= l;
>+		nd_opt = ((void *)nd_opt) + l;
>+	}
>+
>+	return lladdr;
>+}
>+
>+static void alb_set_nd_option(struct sk_buff *skb, struct bonding *bond,
>+			      struct slave *tx_slave)
>+{
>+	struct ipv6hdr *ip6hdr;
>+	struct icmp6hdr *hdr = NULL;
>+
>+	if (tx_slave && tx_slave != rcu_access_pointer(bond->curr_active_slave)) {
>+		if (ntohs(skb->protocol) == ETH_P_IPV6) {

	Nit: use "skb->protocol == htons(ETH_P_IPV6)" as the compiler
should optimize the htons() of a constant.  Also, you may want to
consider reordering the tests here, as IPv6 NA/NS traffic is likely to
be the vast minority.

>+			ip6hdr = ipv6_hdr(skb);
>+			if (ip6hdr->nexthdr == IPPROTO_ICMPV6) {
>+				hdr = icmp6_hdr(skb);
>+				if (hdr->icmp6_type ==
>+				    NDISC_NEIGHBOUR_ADVERTISEMENT ||
>+				     hdr->icmp6_type ==
>+				     NDISC_NEIGHBOUR_SOLICITATION) {

	This construct appears twice, perhaps it deserves its own
boolean-return function?

	-J

>+					alb_change_nd_option(skb, tx_slave->dev->dev_addr);
>+				}
>+			}
>+		}
>+	}
>+}
>+
> /************************ exported alb functions ************************/
> 
> int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>@@ -1415,6 +1522,7 @@ struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
> 	}
> 	case ETH_P_IPV6: {
> 		const struct ipv6hdr *ip6hdr;
>+		struct icmp6hdr *hdr = NULL;
> 
> 		/* IPv6 doesn't really use broadcast mac address, but leave
> 		 * that here just in case.
>@@ -1446,6 +1554,24 @@ struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
> 			break;
> 		}
> 
>+		if (ip6hdr->nexthdr == IPPROTO_ICMPV6) {
>+			hdr = icmp6_hdr(skb);
>+			if (hdr->icmp6_type ==
>+			    NDISC_NEIGHBOUR_ADVERTISEMENT ||
>+			    hdr->icmp6_type ==
>+			    NDISC_NEIGHBOUR_SOLICITATION) {
>+				u8 *lladdr = NULL;
>+
>+				lladdr = alb_get_lladdr(skb);
>+				if (lladdr) {
>+					if (!bond_slave_has_mac_rx(bond, lladdr)) {
>+						do_tx_balance = false;
>+						break;
>+					}
>+				}
>+			}
>+		}
>+
> 		hash_start = (char *)&ip6hdr->daddr;
> 		hash_size = sizeof(ip6hdr->daddr);
> 		break;
>@@ -1489,6 +1615,7 @@ netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> 	struct slave *tx_slave = NULL;
> 
> 	tx_slave = bond_xmit_alb_slave_get(bond, skb);
>+	alb_set_nd_option(skb, bond, tx_slave);
> 	return bond_do_alb_xmit(skb, bond, tx_slave);
> }
> 
>-- 
>1.8.3.1
>

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