| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <27832.1639081353@famine>
Date: Thu, 09 Dec 2021 12:22:33 -0800
From: Jay Vosburgh <jay.vosburgh@...onical.com>
To: Sun Shouxin <sunshouxin@...natelecom.cn>
cc: vfalico@...il.com, andy@...yhouse.net, davem@...emloft.net,
kuba@...nel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, huyd12@...natelecom.cn
Subject: Re: [PATCH] net: bonding: Add support for IPV6 ns/na
Sun Shouxin <sunshouxin@...natelecom.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@...natelecom.cn>
>Signed-off-by: Sun Shouxin <sunshouxin@...natelecom.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@...onical.com
Powered by blists - more mailing lists