netdev - Re: [PATCH net-next 2/4] bonding: use common mac addr checks

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4921.1526072038@famine>
Date:   Fri, 11 May 2018 13:53:58 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Debabrata Banerjee <dbanerje@...mai.com>
cc:     "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [PATCH net-next 2/4] bonding: use common mac addr checks

Debabrata Banerjee <dbanerje@...mai.com> wrote:

>Replace homegrown mac addr checks with faster defs from etherdevice.h
>
>Signed-off-by: Debabrata Banerjee <dbanerje@...mai.com>
>---
> drivers/net/bonding/bond_alb.c | 28 +++++++++-------------------
> 1 file changed, 9 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index c2f6c58e4e6a..180e50f7806f 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -40,11 +40,6 @@
> #include <net/bonding.h>
> #include <net/bond_alb.h>
> 
>-
>-
>-static const u8 mac_bcast[ETH_ALEN + 2] __long_aligned = {
>-	0xff, 0xff, 0xff, 0xff, 0xff, 0xff
>-};
> static const u8 mac_v6_allmcast[ETH_ALEN + 2] __long_aligned = {
> 	0x33, 0x33, 0x00, 0x00, 0x00, 0x01
> };
>@@ -420,9 +415,7 @@ static void rlb_clear_slave(struct bonding *bond, struct slave *slave)
> 
> 			if (assigned_slave) {
> 				rx_hash_table[index].slave = assigned_slave;
>-				if (!ether_addr_equal_64bits(rx_hash_table[index].mac_dst,
>-							     mac_bcast) &&
>-				    !is_zero_ether_addr(rx_hash_table[index].mac_dst)) {
>+				if (is_valid_ether_addr(rx_hash_table[index].mac_dst)) {

	This change and the similar ones below will now fail
non-broadcast multicast Ethernet addresses, where the prior code would
not.  Is this an intentional change?

	-J

> 					bond_info->rx_hashtbl[index].ntt = 1;
> 					bond_info->rx_ntt = 1;
> 					/* A slave has been removed from the
>@@ -525,8 +518,7 @@ static void rlb_req_update_slave_clients(struct bonding *bond, struct slave *sla
> 		client_info = &(bond_info->rx_hashtbl[hash_index]);
> 
> 		if ((client_info->slave == slave) &&
>-		    !ether_addr_equal_64bits(client_info->mac_dst, mac_bcast) &&
>-		    !is_zero_ether_addr(client_info->mac_dst)) {
>+		    is_valid_ether_addr(client_info->mac_dst)) {
> 			client_info->ntt = 1;
> 			ntt = 1;
> 		}
>@@ -567,8 +559,7 @@ static void rlb_req_update_subnet_clients(struct bonding *bond, __be32 src_ip)
> 		if ((client_info->ip_src == src_ip) &&
> 		    !ether_addr_equal_64bits(client_info->slave->dev->dev_addr,
> 					     bond->dev->dev_addr) &&
>-		    !ether_addr_equal_64bits(client_info->mac_dst, mac_bcast) &&
>-		    !is_zero_ether_addr(client_info->mac_dst)) {
>+		    is_valid_ether_addr(client_info->mac_dst)) {
> 			client_info->ntt = 1;
> 			bond_info->rx_ntt = 1;
> 		}
>@@ -596,7 +587,7 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
> 		if ((client_info->ip_src == arp->ip_src) &&
> 		    (client_info->ip_dst == arp->ip_dst)) {
> 			/* the entry is already assigned to this client */
>-			if (!ether_addr_equal_64bits(arp->mac_dst, mac_bcast)) {
>+			if (!is_broadcast_ether_addr(arp->mac_dst)) {
> 				/* update mac address from arp */
> 				ether_addr_copy(client_info->mac_dst, arp->mac_dst);
> 			}
>@@ -644,8 +635,7 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
> 		ether_addr_copy(client_info->mac_src, arp->mac_src);
> 		client_info->slave = assigned_slave;
> 
>-		if (!ether_addr_equal_64bits(client_info->mac_dst, mac_bcast) &&
>-		    !is_zero_ether_addr(client_info->mac_dst)) {
>+		if (is_valid_ether_addr(client_info->mac_dst)) {
> 			client_info->ntt = 1;
> 			bond->alb_info.rx_ntt = 1;
> 		} else {
>@@ -1418,9 +1408,9 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> 	case ETH_P_IP: {
> 		const struct iphdr *iph = ip_hdr(skb);
> 
>-		if (ether_addr_equal_64bits(eth_data->h_dest, mac_bcast) ||
>-		    (iph->daddr == ip_bcast) ||
>-		    (iph->protocol == IPPROTO_IGMP)) {
>+		if (is_broadcast_ether_addr(eth_data->h_dest) ||
>+		    iph->daddr == ip_bcast ||
>+		    iph->protocol == IPPROTO_IGMP) {
> 			do_tx_balance = false;
> 			break;
> 		}
>@@ -1432,7 +1422,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> 		/* IPv6 doesn't really use broadcast mac address, but leave
> 		 * that here just in case.
> 		 */
>-		if (ether_addr_equal_64bits(eth_data->h_dest, mac_bcast)) {
>+		if (is_broadcast_ether_addr(eth_data->h_dest)) {
> 			do_tx_balance = false;
> 			break;
> 		}
>-- 
>2.17.0
>