[PATCH] net: Fix for_each_netdev_feature on Big endian

[PATCH] net: Fix for_each_netdev_feature on Big endian

[Hauke Mehrtens]

The features attribute is of type u64 and stored in the native endianes on
the system. The for_each_set_bit() macro takes a pointer to a 32 bit array
and goes over the bits in this area. On little Endian systems this also
works with an u64 as the most significant bit is on the highest address,
but on big endian the words are swapped. When we expect bit 15 here we get
bit 47 (15 + 32).

This patch converts it more or less to its own for_each_set_bit()
implementation which works on 64 bit integers directly. This is then
completely in host endianness and should work like expected.

Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
Cc: stable@vger.kernel.org
Signed-off-by: Hauke Mehrtens <hauke.mehrtens@intel.com>
---
 include/linux/netdev_features.h | 23 +++++++++++++++++++++--
 net/core/dev.c                  |  4 ++--
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 2b2a6dc..fce2856 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -11,6 +11,7 @@
 #define _LINUX_NETDEV_FEATURES_H
 
 #include <linux/types.h>
+#include <asm/byteorder.h>
 
 typedef u64 netdev_features_t;
 
@@ -154,8 +155,26 @@ enum {
 #define NETIF_F_HW_TLS_TX	__NETIF_F(HW_TLS_TX)
 #define NETIF_F_HW_TLS_RX	__NETIF_F(HW_TLS_RX)
 
-#define for_each_netdev_feature(mask_addr, bit)	\
-	for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
+/* Finds the next feature with the highest number of the range of start till 0.
+ */
+static inline int find_next_netdev_feature(u64 feature, unsigned long start)
+{
+	/* like BITMAP_LAST_WORD_MASK() for u64
+	 * this sets the most significant 64 - start to 0.
+	 */
+	feature &= ~0ULL >> (-start & ((sizeof(feature) * 8) - 1));
+
+	return fls64(feature) - 1;
+}
+
+/* This goes for the MSB to the LSB through the set feature bits,
+ * mask_addr should be a u64 and bit an int
+ */
+#define for_each_netdev_feature(mask_addr, bit)				\
+	for ((bit) = find_next_netdev_feature((mask_addr),		\
+					      NETDEV_FEATURE_COUNT);	\
+	     (bit) >= 0;						\
+	     (bit) = find_next_netdev_feature((mask_addr), (bit) - 1))
 
 /* Features valid for ethtool to change */
 /* = all defined minus driver/device-class-related */
diff --git a/net/core/dev.c b/net/core/dev.c
index 8e276e0..5d03889 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8152,7 +8152,7 @@ static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
 	netdev_features_t feature;
 	int feature_bit;
 
-	for_each_netdev_feature(&upper_disables, feature_bit) {
+	for_each_netdev_feature(upper_disables, feature_bit) {
 		feature = __NETIF_F_BIT(feature_bit);
 		if (!(upper->wanted_features & feature)
 		    && (features & feature)) {
@@ -8172,7 +8172,7 @@ static void netdev_sync_lower_features(struct net_device *upper,
 	netdev_features_t feature;
 	int feature_bit;
 
-	for_each_netdev_feature(&upper_disables, feature_bit) {
+	for_each_netdev_feature(upper_disables, feature_bit) {
 		feature = __NETIF_F_BIT(feature_bit);
 		if (!(features & feature) && (lower->features & feature)) {
 			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
-- 
2.10.1

Re: [PATCH] net: Fix for_each_netdev_feature on Big endian

[David Miller]

From: Hauke Mehrtens <hauke.mehrtens@intel.com>
Date: Fri, 15 Feb 2019 17:58:54 +0100

> The features attribute is of type u64 and stored in the native endianes on
> the system. The for_each_set_bit() macro takes a pointer to a 32 bit array
> and goes over the bits in this area. On little Endian systems this also
> works with an u64 as the most significant bit is on the highest address,
> but on big endian the words are swapped. When we expect bit 15 here we get
> bit 47 (15 + 32).
> 
> This patch converts it more or less to its own for_each_set_bit()
> implementation which works on 64 bit integers directly. This is then
> completely in host endianness and should work like expected.
> 
> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hauke Mehrtens <hauke.mehrtens@intel.com>

Applied and queued up for -stable.

Please do not CC: stable for networking fixes, I handle -stable submissions
myself manually and this is documented in the netdev FAQ.

Re: [PATCH] net: Fix for_each_netdev_feature on Big endian

[Eric Dumazet]

On 02/15/2019 08:24 PM, David Miller wrote:
> From: Hauke Mehrtens <hauke.mehrtens@intel.com>
> Date: Fri, 15 Feb 2019 17:58:54 +0100
> 
>> The features attribute is of type u64 and stored in the native endianes on
>> the system. The for_each_set_bit() macro takes a pointer to a 32 bit array
>> and goes over the bits in this area. On little Endian systems this also
>> works with an u64 as the most significant bit is on the highest address,
>> but on big endian the words are swapped. When we expect bit 15 here we get
>> bit 47 (15 + 32).
>>
>> This patch converts it more or less to its own for_each_set_bit()
>> implementation which works on 64 bit integers directly. This is then
>> completely in host endianness and should work like expected.
>>
>> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hauke Mehrtens <hauke.mehrtens@intel.com>
> 
> Applied and queued up for -stable.
> 
> Please do not CC: stable for networking fixes, I handle -stable submissions
> myself manually and this is documented in the netdev FAQ.
> 

This commit added a call to fls64(), but not the needed include.

This might break some uses I think.

I suggest the following fix :

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index c50cedb65cf56fe7d722a5a321b714ed83f449a0..d3f61011f4346e4ea80b61f88bd24541dd006014 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -11,6 +11,7 @@
 #define _LINUX_NETDEV_FEATURES_H
 
 #include <linux/types.h>
+#include <linux/bitops.h>
 #include <asm/byteorder.h>
 
 typedef u64 netdev_features_t;

Re: [PATCH] net: Fix for_each_netdev_feature on Big endian

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 16 Feb 2019 13:01:28 -0800

> This commit added a call to fls64(), but not the needed include.
> 
> This might break some uses I think.
> 
> I suggest the following fix :
> 
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index c50cedb65cf56fe7d722a5a321b714ed83f449a0..d3f61011f4346e4ea80b61f88bd24541dd006014 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -11,6 +11,7 @@
>  #define _LINUX_NETDEV_FEATURES_H
>  
>  #include <linux/types.h>
> +#include <linux/bitops.h>
>  #include <asm/byteorder.h>
>  
>  typedef u64 netdev_features_t;

Ok I'm build testing that right now, thanks.

====================
From 8681ef1f3d295bd3600315325f3b3396d76d02f6 Mon Sep 17 00:00:00 2001
From: "David S. Miller" <davem@davemloft.net>
Date: Sat, 16 Feb 2019 13:44:39 -0800
Subject: [PATCH] net: Add header for usage of fls64()

Fixes: 3b89ea9c5902 ("net: Fix for_each_netdev_feature on Big endian")
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 include/linux/netdev_features.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index fce28562bed2..4c76fe2c8488 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -11,6 +11,7 @@
 #define _LINUX_NETDEV_FEATURES_H
 
 #include <linux/types.h>
+#include <linux/bitops.h>
 #include <asm/byteorder.h>
 
 typedef u64 netdev_features_t;
-- 
2.20.1