[Bridge] [PATCH 00/13] net: Add and use ether_addr_equal

[Bridge] [PATCH 00/13] net: Add and use ether_addr_equal

[Joe Perches]

Add a boolean function to test 2 ethernet addresses for equality
Convert compare_ether_addr uses to ether_addr_equal

Joe Perches (13):
  etherdevice.h: Add ether_addr_equal
  802: Convert compare_ether_addr to ether_addr_equal
  8021q: Convert compare_ether_addr to ether_addr_equal
  bridge: netfilter: Convert compare_ether_addr to ether_addr_equal
  bridge: Convert compare_ether_addr to ether_addr_equal
  atm: Convert compare_ether_addr to ether_addr_equal
  bluetooth: Convert compare_ether_addr to ether_addr_equal
  mac80211: Convert compare_ether_addr to ether_addr_equal
  mac80211: Convert compare_ether_addr to ether_addr_equal by hand
  netfilter: Convert compare_ether_addr to ether_addr_equal
  wireless: Convert compare_ether_addr to ether_addr_equal
  wireless: Convert compare_ether_addr to ether_addr_equal by hand
  dsa: Convert compare_ether_addr to ether_addr_equal

 include/linux/etherdevice.h               |   12 +++++++++
 net/802/stp.c                             |    2 +-
 net/8021q/vlan.c                          |   10 +++---
 net/8021q/vlan_core.c                     |    3 +-
 net/8021q/vlan_dev.c                      |   10 +++---
 net/atm/lec.c                             |    6 ++--
 net/atm/mpc.c                             |    3 +-
 net/bluetooth/bnep/core.c                 |    6 ++--
 net/bridge/br_device.c                    |    2 +-
 net/bridge/br_fdb.c                       |   14 +++++-----
 net/bridge/br_input.c                     |    2 +-
 net/bridge/br_stp_bpdu.c                  |    2 +-
 net/bridge/br_stp_if.c                    |   11 +++----
 net/bridge/netfilter/ebt_stp.c            |    4 +-
 net/dsa/slave.c                           |   10 +++---
 net/mac80211/cfg.c                        |    2 +-
 net/mac80211/ibss.c                       |   12 ++++----
 net/mac80211/ieee80211_i.h                |    2 +-
 net/mac80211/iface.c                      |    2 +-
 net/mac80211/mesh.c                       |    4 +-
 net/mac80211/mesh_hwmp.c                  |   14 +++++-----
 net/mac80211/mesh_pathtbl.c               |   12 ++++----
 net/mac80211/mlme.c                       |   29 +++++++++------------
 net/mac80211/rx.c                         |   39 +++++++++++++---------------
 net/mac80211/scan.c                       |    2 +-
 net/mac80211/sta_info.c                   |    8 +++---
 net/mac80211/sta_info.h                   |    2 +-
 net/mac80211/status.c                     |    2 +-
 net/mac80211/tx.c                         |   11 +++----
 net/netfilter/ipset/ip_set_bitmap_ipmac.c |    4 +-
 net/netfilter/xt_mac.c                    |    2 +-
 net/wireless/ibss.c                       |    2 +-
 net/wireless/mlme.c                       |   31 +++++++++++------------
 net/wireless/scan.c                       |    2 +-
 net/wireless/util.c                       |   11 +++----
 net/wireless/wext-sme.c                   |    2 +-
 net/wireless/wext-spy.c                   |    2 +-
 37 files changed, 147 insertions(+), 147 deletions(-)

-- 
1.7.8.111.gad25c.dirty

[Bridge] [PATCH 04/13] bridge: netfilter: Convert compare_ether_addr to ether_addr_equal

[Joe Perches]

Use the new bool function ether_addr_equal to add
some clarity and reduce the likelihood for misuse
of compare_ether_addr for sorting.

Done via cocci script:

$ cat compare_ether_addr.cocci
@@
expression a,b;
@@
-	!compare_ether_addr(a, b)
+	ether_addr_equal(a, b)

@@
expression a,b;
@@
-	compare_ether_addr(a, b)
+	!ether_addr_equal(a, b)

@@
expression a,b;
@@
-	!ether_addr_equal(a, b) == 0
+	ether_addr_equal(a, b)

@@
expression a,b;
@@
-	!ether_addr_equal(a, b) != 0
+	!ether_addr_equal(a, b)

@@
expression a,b;
@@
-	ether_addr_equal(a, b) == 0
+	!ether_addr_equal(a, b)

@@
expression a,b;
@@
-	ether_addr_equal(a, b) != 0
+	ether_addr_equal(a, b)

@@
expression a,b;
@@
-	!!ether_addr_equal(a, b)
+	ether_addr_equal(a, b)

Signed-off-by: Joe Perches <joe@perches.com>
---
 net/bridge/netfilter/ebt_stp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c
index 5b33a2e..071d872 100644
--- a/net/bridge/netfilter/ebt_stp.c
+++ b/net/bridge/netfilter/ebt_stp.c
@@ -164,8 +164,8 @@ static int ebt_stp_mt_check(const struct xt_mtchk_param *par)
 	    !(info->bitmask & EBT_STP_MASK))
 		return -EINVAL;
 	/* Make sure the match only receives stp frames */
-	if (compare_ether_addr(e->destmac, bridge_ula) ||
-	    compare_ether_addr(e->destmsk, msk) || !(e->bitmask & EBT_DESTMAC))
+	if (!ether_addr_equal(e->destmac, bridge_ula) ||
+	    !ether_addr_equal(e->destmsk, msk) || !(e->bitmask & EBT_DESTMAC))
 		return -EINVAL;
 
 	return 0;
-- 
1.7.8.111.gad25c.dirty

Re: [Bridge] [PATCH 04/13] bridge: netfilter: Convert compare_ether_addr to ether_addr_equal

[Stephen Hemminger]

On Tue,  8 May 2012 21:56:48 -0700
Joe Perches <joe@perches.com> wrote:

> Use the new bool function ether_addr_equal to add
> some clarity and reduce the likelihood for misuse
> of compare_ether_addr for sorting.
> 
> Done via cocci script:
> 
> $ cat compare_ether_addr.cocci
> @@
> expression a,b;
> @@
> -	!compare_ether_addr(a, b)
> +	ether_addr_equal(a, b)
> 
> @@
> expression a,b;
> @@
> -	compare_ether_addr(a, b)
> +	!ether_addr_equal(a, b)
> 
> @@
> expression a,b;
> @@
> -	!ether_addr_equal(a, b) == 0
> +	ether_addr_equal(a, b)
> 
> @@
> expression a,b;
> @@
> -	!ether_addr_equal(a, b) != 0
> +	!ether_addr_equal(a, b)
> 
> @@
> expression a,b;
> @@
> -	ether_addr_equal(a, b) == 0
> +	!ether_addr_equal(a, b)
> 
> @@
> expression a,b;
> @@
> -	ether_addr_equal(a, b) != 0
> +	ether_addr_equal(a, b)
> 
> @@
> expression a,b;
> @@
> -	!!ether_addr_equal(a, b)
> +	ether_addr_equal(a, b)
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  net/bridge/netfilter/ebt_stp.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/netfilter/ebt_stp.c b/net/bridge/netfilter/ebt_stp.c
> index 5b33a2e..071d872 100644
> --- a/net/bridge/netfilter/ebt_stp.c
> +++ b/net/bridge/netfilter/ebt_stp.c
> @@ -164,8 +164,8 @@ static int ebt_stp_mt_check(const struct xt_mtchk_param *par)
>  	    !(info->bitmask & EBT_STP_MASK))
>  		return -EINVAL;
>  	/* Make sure the match only receives stp frames */
> -	if (compare_ether_addr(e->destmac, bridge_ula) ||
> -	    compare_ether_addr(e->destmsk, msk) || !(e->bitmask & EBT_DESTMAC))
> +	if (!ether_addr_equal(e->destmac, bridge_ula) ||
> +	    !ether_addr_equal(e->destmsk, msk) || !(e->bitmask & EBT_DESTMAC))
>  		return -EINVAL;
>  
>  	return 0;

All look good.
Acked-by: Stephen Hemminger <shemminger@vyatta.com>

[Bridge] [PATCH 05/13] bridge: Convert compare_ether_addr to ether_addr_equal

[Joe Perches]

Use the new bool function ether_addr_equal to add
some clarity and reduce the likelihood for misuse
of compare_ether_addr for sorting.

Done via cocci script:

$ cat compare_ether_addr.cocci
@@
expression a,b;
@@
-	!compare_ether_addr(a, b)
+	ether_addr_equal(a, b)

@@
expression a,b;
@@
-	compare_ether_addr(a, b)
+	!ether_addr_equal(a, b)

@@
expression a,b;
@@
-	!ether_addr_equal(a, b) == 0
+	ether_addr_equal(a, b)

@@
expression a,b;
@@
-	!ether_addr_equal(a, b) != 0
+	!ether_addr_equal(a, b)

@@
expression a,b;
@@
-	ether_addr_equal(a, b) == 0
+	!ether_addr_equal(a, b)

@@
expression a,b;
@@
-	ether_addr_equal(a, b) != 0
+	ether_addr_equal(a, b)

@@
expression a,b;
@@
-	!!ether_addr_equal(a, b)
+	ether_addr_equal(a, b)

Signed-off-by: Joe Perches <joe@perches.com>
---
 net/bridge/br_device.c   |    2 +-
 net/bridge/br_fdb.c      |   14 +++++++-------
 net/bridge/br_input.c    |    2 +-
 net/bridge/br_stp_bpdu.c |    2 +-
 net/bridge/br_stp_if.c   |   11 +++++------
 5 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index d6e5929..929e48aed 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -170,7 +170,7 @@ static int br_set_mac_address(struct net_device *dev, void *p)
 		return -EADDRNOTAVAIL;
 
 	spin_lock_bh(&br->lock);
-	if (compare_ether_addr(dev->dev_addr, addr->sa_data)) {
+	if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) {
 		dev->addr_assign_type &= ~NET_ADDR_RANDOM;
 		memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
 		br_fdb_change_mac_address(br, addr->sa_data);
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 5945c54..d21f323 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -107,8 +107,8 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 				struct net_bridge_port *op;
 				list_for_each_entry(op, &br->port_list, list) {
 					if (op != p &&
-					    !compare_ether_addr(op->dev->dev_addr,
-								f->addr.addr)) {
+					    ether_addr_equal(op->dev->dev_addr,
+							     f->addr.addr)) {
 						f->dst = op;
 						goto insert;
 					}
@@ -214,8 +214,8 @@ void br_fdb_delete_by_port(struct net_bridge *br,
 				struct net_bridge_port *op;
 				list_for_each_entry(op, &br->port_list, list) {
 					if (op != p &&
-					    !compare_ether_addr(op->dev->dev_addr,
-								f->addr.addr)) {
+					    ether_addr_equal(op->dev->dev_addr,
+							     f->addr.addr)) {
 						f->dst = op;
 						goto skip_delete;
 					}
@@ -237,7 +237,7 @@ struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
 	struct net_bridge_fdb_entry *fdb;
 
 	hlist_for_each_entry_rcu(fdb, h, &br->hash[br_mac_hash(addr)], hlist) {
-		if (!compare_ether_addr(fdb->addr.addr, addr)) {
+		if (ether_addr_equal(fdb->addr.addr, addr)) {
 			if (unlikely(has_expired(br, fdb)))
 				break;
 			return fdb;
@@ -331,7 +331,7 @@ static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
 	struct net_bridge_fdb_entry *fdb;
 
 	hlist_for_each_entry(fdb, h, head, hlist) {
-		if (!compare_ether_addr(fdb->addr.addr, addr))
+		if (ether_addr_equal(fdb->addr.addr, addr))
 			return fdb;
 	}
 	return NULL;
@@ -344,7 +344,7 @@ static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
 	struct net_bridge_fdb_entry *fdb;
 
 	hlist_for_each_entry_rcu(fdb, h, head, hlist) {
-		if (!compare_ether_addr(fdb->addr.addr, addr))
+		if (ether_addr_equal(fdb->addr.addr, addr))
 			return fdb;
 	}
 	return NULL;
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 5a31731..76f15fd 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -216,7 +216,7 @@ forward:
 		}
 		/* fall through */
 	case BR_STATE_LEARNING:
-		if (!compare_ether_addr(p->br->dev->dev_addr, dest))
+		if (ether_addr_equal(p->br->dev->dev_addr, dest))
 			skb->pkt_type = PACKET_HOST;
 
 		NF_HOOK(NFPROTO_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL,
diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index e16aade..fd30a60 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -167,7 +167,7 @@ void br_stp_rcv(const struct stp_proto *proto, struct sk_buff *skb,
 	if (p->state == BR_STATE_DISABLED)
 		goto out;
 
-	if (compare_ether_addr(dest, br->group_addr) != 0)
+	if (!ether_addr_equal(dest, br->group_addr))
 		goto out;
 
 	buf = skb_pull(skb, 3);
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index f494496..9d5a414 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -178,7 +178,7 @@ void br_stp_set_enabled(struct net_bridge *br, unsigned long val)
 /* called under bridge lock */
 void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *addr)
 {
-	/* should be aligned on 2 bytes for compare_ether_addr() */
+	/* should be aligned on 2 bytes for ether_addr_equal() */
 	unsigned short oldaddr_aligned[ETH_ALEN >> 1];
 	unsigned char *oldaddr = (unsigned char *)oldaddr_aligned;
 	struct net_bridge_port *p;
@@ -191,12 +191,11 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *addr)
 	memcpy(br->dev->dev_addr, addr, ETH_ALEN);
 
 	list_for_each_entry(p, &br->port_list, list) {
-		if (!compare_ether_addr(p->designated_bridge.addr, oldaddr))
+		if (ether_addr_equal(p->designated_bridge.addr, oldaddr))
 			memcpy(p->designated_bridge.addr, addr, ETH_ALEN);
 
-		if (!compare_ether_addr(p->designated_root.addr, oldaddr))
+		if (ether_addr_equal(p->designated_root.addr, oldaddr))
 			memcpy(p->designated_root.addr, addr, ETH_ALEN);
-
 	}
 
 	br_configuration_update(br);
@@ -205,7 +204,7 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *addr)
 		br_become_root_bridge(br);
 }
 
-/* should be aligned on 2 bytes for compare_ether_addr() */
+/* should be aligned on 2 bytes for ether_addr_equal() */
 static const unsigned short br_mac_zero_aligned[ETH_ALEN >> 1];
 
 /* called under bridge lock */
@@ -227,7 +226,7 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br)
 
 	}
 
-	if (compare_ether_addr(br->bridge_id.addr, addr) == 0)
+	if (ether_addr_equal(br->bridge_id.addr, addr))
 		return false;	/* no change */
 
 	br_stp_change_bridge_id(br, addr);
-- 
1.7.8.111.gad25c.dirty

Re: [Bridge] [PATCH 00/13] net: Add and use ether_addr_equal

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue,  8 May 2012 21:56:44 -0700

> Add a boolean function to test 2 ethernet addresses for equality
> Convert compare_ether_addr uses to ether_addr_equal

This series looks great, I'll apply all of it.

Thanks Joe.

That case you didn't convert in mac80211 is probably the
bug Johannes was talking about which started this whole
discussion.

Re: [Bridge] [PATCH 00/13] net: Add and use ether_addr_equal

[Joe Perches]

On Wed, 2012-05-09 at 21:21 -0400, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Tue,  8 May 2012 21:56:44 -0700
> 
> > Add a boolean function to test 2 ethernet addresses for equality
> > Convert compare_ether_addr uses to ether_addr_equal
> 
> This series looks great, I'll apply all of it.

coccinelle is a nifty, better sed, tool.  Thanks Julia et al.

> That case you didn't convert in mac80211 is probably the
> bug Johannes was talking about which started this whole
> discussion.

Looks like it.

Do you want a similar patch/patches for drivers/net?

If I break it out by nominal driver/maintainer,
it'll be a highish number of low density patches.

$ git grep -w compare_ether_addr drivers/net | wc -l
59
$ git grep -w -l compare_ether_addr drivers/net | wc -l
31

Maybe just a single patch?

Also the compare_ether_addr_64bits function is
still used a couple dozen times.  Maybe another
patch for those?  ether_addr_equal_64bits?

Re: [Bridge] [PATCH 00/13] net: Add and use ether_addr_equal

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Wed, 09 May 2012 18:48:04 -0700

> Maybe just a single patch?

Yes, I think high density on this one, it's extremely
mechanical.

> Also the compare_ether_addr_64bits function is
> still used a couple dozen times.  Maybe another
> patch for those?  ether_addr_equal_64bits?

Use your best judgment.

Re: [Bridge] [PATCH 00/13] net: Add and use ether_addr_equal

[Johannes Berg]

On Wed, 2012-05-09 at 21:21 -0400, David Miller wrote:

> That case you didn't convert in mac80211 is probably the
> bug Johannes was talking about which started this whole
> discussion.

The bug case that started it all is in net/wireless/scan.c and Emmanuel
has since changed it back to memcmp(). Not sure if that's the one you
were referring to or not :-)

johannes

Re: [Bridge] [PATCH 00/13] net: Add and use ether_addr_equal

[Joe Perches]

On Thu, 2012-05-10 at 08:48 +0200, Johannes Berg wrote:
> On Wed, 2012-05-09 at 21:21 -0400, David Miller wrote:
> 
> > That case you didn't convert in mac80211 is probably the
> > bug Johannes was talking about which started this whole
> > discussion.
> 
> The bug case that started it all is in net/wireless/scan.c and Emmanuel
> has since changed it back to memcmp(). Not sure if that's the one you
> were referring to or not :-)

That's the one that I left alone.

Post patch:
$ git grep -n -w compare_ether_addr net
net/batman-adv/main.h:198: * note: can't use compare_ether_addr() as it requires aligned memory
net/wireless/scan.c:381:        return compare_ether_addr(a->bssid, b->bssid);

Re: [Bridge] [PATCH 00/13] net: Add and use ether_addr_equal

[Johannes Berg]

On Wed, 2012-05-09 at 23:54 -0700, Joe Perches wrote:
> On Thu, 2012-05-10 at 08:48 +0200, Johannes Berg wrote:
> > On Wed, 2012-05-09 at 21:21 -0400, David Miller wrote:
> > 
> > > That case you didn't convert in mac80211 is probably the
> > > bug Johannes was talking about which started this whole
> > > discussion.
> > 
> > The bug case that started it all is in net/wireless/scan.c and Emmanuel
> > has since changed it back to memcmp(). Not sure if that's the one you
> > were referring to or not :-)
> 
> That's the one that I left alone.
> 
> Post patch:
> $ git grep -n -w compare_ether_addr net
> net/batman-adv/main.h:198: * note: can't use compare_ether_addr() as it requires aligned memory
> net/wireless/scan.c:381:        return compare_ether_addr(a->bssid, b->bssid);

Ok, great, then that means the fix from Emmanuel won't conflict when it
gets in.

johannes

Re: [Bridge] [PATCH 00/13] net: Add and use ether_addr_equal

[Emmanuel Grumbach]

>> >
>> > > That case you didn't convert in mac80211 is probably the
>> > > bug Johannes was talking about which started this whole
>> > > discussion.
>> >
>> > The bug case that started it all is in net/wireless/scan.c and Emmanuel
>> > has since changed it back to memcmp(). Not sure if that's the one you
>> > were referring to or not :-)
>>
>> That's the one that I left alone.
>>
>> Post patch:
>> $ git grep -n -w compare_ether_addr net
>> net/batman-adv/main.h:198: * note: can't use compare_ether_addr() as it requires aligned memory
>> net/wireless/scan.c:381:        return compare_ether_addr(a->bssid, b->bssid);
>
> Ok, great, then that means the fix from Emmanuel won't conflict when it
> gets in.
>

Thanks Joe - as Johannes said this won't conflict with my patch. And
yes the code is now more clear.