| Message ID | 1336397946.4325.27.camel@jlt3.sipsolutions.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2012-05-07 at 15:39 +0200, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > Neither compare_ether_addr() nor compare_ether_addr_64bits() > (as it can fall back to the former) have comparison semantics > like memcmp() where the sign of the return value indicates sort > order. We had a bug in the wireless code due to a blind memcmp > replacement because of this. > > A cursory look suggests that the wireless bug was the only one > due to this semantic difference. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > include/linux/etherdevice.h | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) The right way to avoid this kind of problems is to change these functions to return a bool -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-05-07 at 15:53 +0200, Eric Dumazet wrote: > On Mon, 2012-05-07 at 15:39 +0200, Johannes Berg wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > > > Neither compare_ether_addr() nor compare_ether_addr_64bits() > > (as it can fall back to the former) have comparison semantics > > like memcmp() where the sign of the return value indicates sort > > order. We had a bug in the wireless code due to a blind memcmp > > replacement because of this. > > > > A cursory look suggests that the wireless bug was the only one > > due to this semantic difference. > > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > > --- > > include/linux/etherdevice.h | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > The right way to avoid this kind of problems is to change these > functions to return a bool Well, I guess so, but that'd be a weird thing for a compare_ function... should probably be named equal_... then, but I'm not really able to do such a huge change on the first day after my vacation :-) johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Johannes Berg <johannes@sipsolutions.net> Date: Mon, 07 May 2012 16:12:41 +0200 > On Mon, 2012-05-07 at 15:53 +0200, Eric Dumazet wrote: >> On Mon, 2012-05-07 at 15:39 +0200, Johannes Berg wrote: >> > From: Johannes Berg <johannes.berg@intel.com> >> > >> > Neither compare_ether_addr() nor compare_ether_addr_64bits() >> > (as it can fall back to the former) have comparison semantics >> > like memcmp() where the sign of the return value indicates sort >> > order. We had a bug in the wireless code due to a blind memcmp >> > replacement because of this. >> > >> > A cursory look suggests that the wireless bug was the only one >> > due to this semantic difference. >> > >> > Signed-off-by: Johannes Berg <johannes.berg@intel.com> >> > --- >> > include/linux/etherdevice.h | 11 ++++++----- >> > 1 file changed, 6 insertions(+), 5 deletions(-) >> >> The right way to avoid this kind of problems is to change these >> functions to return a bool > > Well, I guess so, but that'd be a weird thing for a compare_ function... > should probably be named equal_... then, but I'm not really able to do > such a huge change on the first day after my vacation :-) It's true the name could be improved, but changing the name is quite a large undertaking even with automated scripts. Even the bool change is slightly painful, since all of the explicit tests against integers (%99.999 of these are in wireless BTW :-) would need to be adjusted. For now, I'll just apply Johannes's comment fix. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-05-07 at 19:20 -0400, David Miller wrote: > >> > Neither compare_ether_addr() nor compare_ether_addr_64bits() > >> > (as it can fall back to the former) have comparison semantics > >> > like memcmp() where the sign of the return value indicates sort > >> > order. We had a bug in the wireless code due to a blind memcmp > >> > replacement because of this. > >> > > >> > A cursory look suggests that the wireless bug was the only one > >> > due to this semantic difference. > >> > > >> > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > >> > --- > >> > include/linux/etherdevice.h | 11 ++++++----- > >> > 1 file changed, 6 insertions(+), 5 deletions(-) > >> > >> The right way to avoid this kind of problems is to change these > >> functions to return a bool > > > > Well, I guess so, but that'd be a weird thing for a compare_ function... > > should probably be named equal_... then, but I'm not really able to do > > such a huge change on the first day after my vacation :-) > > It's true the name could be improved, but changing the name is quite > a large undertaking even with automated scripts. > > Even the bool change is slightly painful, since all of the explicit > tests against integers (%99.999 of these are in wireless BTW :-) would > need to be adjusted. I suppose I could fix those first and then later change the type, but I think having a "compare_ether_addr" function that returns *false* when they *match* would be rather confusing. I'd rather have "equal_ether_addr()" that returns *true* when they match. I guess we could introduce equal_ether_addr() though and slowly convert, keeping compare_ether_addr() as a sort of wrapper around it. johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Johannes Berg <johannes@sipsolutions.net> Date: Tue, 08 May 2012 07:25:44 +0200 > I suppose I could fix those first and then later change the type, but I > think having a "compare_ether_addr" function that returns *false* when > they *match* would be rather confusing. I'd rather have > "equal_ether_addr()" that returns *true* when they match. > > I guess we could introduce equal_ether_addr() though and slowly convert, > keeping compare_ether_addr() as a sort of wrapper around it. Indeed, this is one way to proceed. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-05-08 at 02:26 -0400, David Miller wrote: > From: Johannes Berg <johannes@sipsolutions.net> > Date: Tue, 08 May 2012 07:25:44 +0200 > > > I suppose I could fix those first and then later change the type, but I > > think having a "compare_ether_addr" function that returns *false* when > > they *match* would be rather confusing. I'd rather have > > "equal_ether_addr()" that returns *true* when they match. > > > > I guess we could introduce equal_ether_addr() though and slowly convert, > > keeping compare_ether_addr() as a sort of wrapper around it. > > Indeed, this is one way to proceed. perhaps is_equal_ether_addr or is_same_ether_addr instead? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Joe Perches <joe@perches.com> Date: Mon, 07 May 2012 23:35:36 -0700 > On Tue, 2012-05-08 at 02:26 -0400, David Miller wrote: >> From: Johannes Berg <johannes@sipsolutions.net> >> Date: Tue, 08 May 2012 07:25:44 +0200 >> >> > I suppose I could fix those first and then later change the type, but I >> > think having a "compare_ether_addr" function that returns *false* when >> > they *match* would be rather confusing. I'd rather have >> > "equal_ether_addr()" that returns *true* when they match. >> > >> > I guess we could introduce equal_ether_addr() though and slowly convert, >> > keeping compare_ether_addr() as a sort of wrapper around it. >> >> Indeed, this is one way to proceed. > > perhaps is_equal_ether_addr or is_same_ether_addr instead? Hmmm, my first choice would have been "eth_addr_equal()" -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-05-08 at 03:31 -0400, David Miller wrote: > From: Joe Perches <joe@perches.com> > Date: Mon, 07 May 2012 23:35:36 -0700 > > > On Tue, 2012-05-08 at 02:26 -0400, David Miller wrote: > >> From: Johannes Berg <johannes@sipsolutions.net> > >> Date: Tue, 08 May 2012 07:25:44 +0200 > >> > >> > I suppose I could fix those first and then later change the type, but I > >> > think having a "compare_ether_addr" function that returns *false* when > >> > they *match* would be rather confusing. I'd rather have > >> > "equal_ether_addr()" that returns *true* when they match. > >> > > >> > I guess we could introduce equal_ether_addr() though and slowly convert, > >> > keeping compare_ether_addr() as a sort of wrapper around it. > >> > >> Indeed, this is one way to proceed. > > > > perhaps is_equal_ether_addr or is_same_ether_addr instead? > > Hmmm, my first choice would have been "eth_addr_equal()" Perhaps ether_addr_equal for some API naming semi-consistency. $ grep "\bint.*is_.*ether_addr" include/linux/etherdevice.h static inline int is_zero_ether_addr(const u8 *addr) static inline int is_multicast_ether_addr(const u8 *addr) static inline int is_local_ether_addr(const u8 *addr) static inline int is_broadcast_ether_addr(const u8 *addr) static inline int is_unicast_ether_addr(const u8 *addr) static inline int is_valid_ether_addr(const u8 *addr) Perhaps all of these should be bool too (patch in a separate email) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/include/linux/etherdevice.h 2012-04-12 05:40:35.000000000 +0200 +++ b/include/linux/etherdevice.h 2012-05-07 15:34:28.000000000 +0200 @@ -159,7 +159,8 @@ static inline void eth_hw_addr_random(st * @addr1: Pointer to a six-byte array containing the Ethernet address * @addr2: Pointer other six-byte array containing the Ethernet address * - * Compare two ethernet addresses, returns 0 if equal + * Compare two ethernet addresses, returns 0 if equal, non-zero otherwise. + * Unlike memcmp(), it doesn't return a value suitable for sorting. */ static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2) { @@ -184,10 +185,10 @@ static inline unsigned long zap_last_2by * @addr1: Pointer to an array of 8 bytes * @addr2: Pointer to an other array of 8 bytes * - * Compare two ethernet addresses, returns 0 if equal. - * Same result than "memcmp(addr1, addr2, ETH_ALEN)" but without conditional - * branches, and possibly long word memory accesses on CPU allowing cheap - * unaligned memory reads. + * Compare two ethernet addresses, returns 0 if equal, non-zero otherwise. + * Unlike memcmp(), it doesn't return a value suitable for sorting. + * The function doesn't need any conditional branches and possibly uses + * word memory accesses on CPU allowing cheap unaligned memory reads. * arrays = { byte1, byte2, byte3, byte4, byte6, byte7, pad1, pad2} * * Please note that alignment of addr1 & addr2 is only guaranted to be 16 bits.