Messages in this thread | | | From | Jay Vosburgh <> | Subject | Re: [PATCH] net: bonding: Add support for IPV6 ns/na | Date | Thu, 09 Dec 2021 12:22:33 -0800 |
| |
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
| |