netdev - Re: [PATCHv2 net] bonding: add ns target multicast address to slave device

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZyL0OgXVAEUxthbq@fedora>
Date: Thu, 31 Oct 2024 03:06:34 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Jay Vosburgh <jv@...sburgh.net>
Cc: netdev@...r.kernel.org, Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 net] bonding: add ns target multicast address to slave
 device

Hi Jay,
On Wed, Oct 30, 2024 at 05:21:05PM +0100, Jay Vosburgh wrote:
> 	I suspect the set of multicast addresses involved is likely to
> be small in the usual case, so the question then is whether the
> presumably small amount of traffic that inadvertently passes the filter
> (and is then thrown away by the kernel RX logic) is worth the complexity
> added here.

Yes, while the code and logic may be complex, the "small amount of
traffic", specifically, the IPv6 NS messages, plays a crucial role in
determining whether backup slaves are up or not. Without these messages,
it would be akin to dropping ARP traffic for IPv4, which could lead to
connectivity issues.

> 
> 	That said, I have a few questions below.
> 
> >    arp_validate doesn't support 3ad, tlb, alb. So let's only do it on ab mode.
> >---
> > drivers/net/bonding/bond_main.c    | 18 +++++-
> > drivers/net/bonding/bond_options.c | 95 +++++++++++++++++++++++++++++-
> > include/net/bond_options.h         |  1 +
> > 3 files changed, 112 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index b1bffd8e9a95..d7c1016619f9 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1008,6 +1008,9 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> > 
> > 		if (bond->dev->flags & IFF_UP)
> > 			bond_hw_addr_flush(bond->dev, old_active->dev);
> >+
> >+		/* add target NS maddrs for backup slave */
> >+		slave_set_ns_maddrs(bond, old_active, true);
> > 	}
> > 
> > 	if (new_active) {
> >@@ -1024,6 +1027,9 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
> > 			dev_mc_sync(new_active->dev, bond->dev);
> > 			netif_addr_unlock_bh(bond->dev);
> > 		}
> >+
> >+		/* clear target NS maddrs for active slave */
> >+		slave_set_ns_maddrs(bond, new_active, false);
> > 	}
> > }
> > 
> >@@ -2341,6 +2347,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > 	bond_compute_features(bond);
> > 	bond_set_carrier(bond);
> > 
> >+	/* set target NS maddrs for new slave, need to be called before
> >+	 * bond_select_active_slave(), which will remove the maddr if
> >+	 * the slave is selected as active slave
> >+	 */
> >+	slave_set_ns_maddrs(bond, new_slave, true);
> >+
> > 	if (bond_uses_primary(bond)) {
> > 		block_netpoll_tx();
> > 		bond_select_active_slave(bond);
> >@@ -2350,7 +2362,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> > 	if (bond_mode_can_use_xmit_hash(bond))
> > 		bond_update_slave_arr(bond, NULL);
> > 
> >-
> > 	if (!slave_dev->netdev_ops->ndo_bpf ||
> > 	    !slave_dev->netdev_ops->ndo_xdp_xmit) {
> > 		if (bond->xdp_prog) {
> >@@ -2548,6 +2559,11 @@ static int __bond_release_one(struct net_device *bond_dev,
> > 	if (oldcurrent == slave)
> > 		bond_change_active_slave(bond, NULL);
> > 
> >+	/* clear target NS maddrs, must after bond_change_active_slave()
> >+	 * as we need to clear the maddrs on backup slave
> >+	 */
> >+	slave_set_ns_maddrs(bond, slave, false);
> >+
> > 	if (bond_is_lb(bond)) {
> > 		/* Must be called only after the slave has been
> > 		 * detached from the list and the curr_active_slave
> >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> >index 95d59a18c022..2554ba70f092 100644
> >--- a/drivers/net/bonding/bond_options.c
> >+++ b/drivers/net/bonding/bond_options.c
> >@@ -1234,6 +1234,75 @@ static int bond_option_arp_ip_targets_set(struct bonding *bond,
> > }
> > 
> > #if IS_ENABLED(CONFIG_IPV6)
> >+/* convert IPv6 address to link-local solicited-node multicast mac address */
> >+static void ipv6_addr_to_solicited_mac(const struct in6_addr *addr,
> >+				       unsigned char mac[ETH_ALEN])
> >+{
> >+	mac[0] = 0x33;
> >+	mac[1] = 0x33;
> >+	mac[2] = 0xFF;
> >+	mac[3] = addr->s6_addr[13];
> >+	mac[4] = addr->s6_addr[14];
> >+	mac[5] = addr->s6_addr[15];
> >+}
> 
> 	Can we make use of ndisc_mc_map() / ipv6_eth_mc_map() to perform
> this step, instead of creating a new function that's almost the same?

Ah, yes, I think so. Thanks for this tips.


> >+void slave_set_ns_maddrs(struct bonding *bond, struct slave *slave, bool add)
> >+{
> >+	if (!bond->params.arp_validate)
> >+		return;
> >+
> >+	_slave_set_ns_maddrs(bond, slave, add);
> >+}
> 
> 	Why does this need a wrapper function vs. having the
> arp_validate test be first in the larger function?

We have 4 places call slave_set_ns_maddrs(). I think with this wrapper could
save some codes. I'm fine to remove this wrapper if you think the code
would be simpler.

Thanks
Hangbin