pull request [net]: batman-adv 20150616

pull request [net]: batman-adv 20150616

[Antonio Quartulli]

Hello David,

this is a pull request intended for net/linux-4.1.
I know it is rather late in the release cycle, but the fixes are
pretty small and they were worth being sent now.

Patch 1/2 fixes an overflow of a u32 variable that may happen while
computing the metric to select the best batman-GW in the network.
** It would be great if this patch could be queued for inclusion in
any stable release >= 3.2


Patch 2/2 prevents the Distributed ARP Table from forging unwanted
ARP replies on behalf of another host that may fool an Ethernet switch
sitting behind batman-adv.
** It would be great if this patch could be queued for inclusion in
any stable release >= 3.8


Please pull or let me know if something is wrong

Thanks a lot,
	Antonio



The following changes since commit ac0a72a3e6e8d817f60ce4d9a8f3b43dc256d847:

  net/mlx4_core: Disable Granular QoS per VF under IB/Eth VPI configuration (2015-06-15 16:42:57 -0700)

are available in the git repository at:

  git://git.open-mesh.org/linux-merge.git tags/batman-adv-fix-for-davem

for you to fetch changes up to c065d51055924f6daad8e16307c364602b0f9805:

  batman-adv: avoid DAT to mess up LAN state (2015-06-16 11:13:12 +0200)

----------------------------------------------------------------
Included changes:
- fix gateway selection metric overflow
- avoid the Distriubted ARP Table to fool Ethernet switches by forging
  unexpected packets

----------------------------------------------------------------
Antonio Quartulli (1):
      batman-adv: avoid DAT to mess up LAN state

Ruben Wisniewski (1):
      batman-adv: Avoid u32 overflow during gateway select

 net/batman-adv/distributed-arp-table.c | 18 +++++++++++++-----
 net/batman-adv/gateway_client.c        |  2 +-
 2 files changed, 14 insertions(+), 6 deletions(-)

[PATCH 1/2] batman-adv: Avoid u32 overflow during gateway select

[Antonio Quartulli]

From: Ruben Wisniewski <ruben@freifunk-nrw.de>

The gateway selection based on fast connections is using a single value
calculated from the average tq (0-255) and the download bandwidth (in
100Kibit). The formula for the first step (tq ** 2 * 10000 * bandwidth)
tends to overflow a u32 with low bandwidth settings like 50 [100KiBit]
and a tq value of over 92.

Changing this to a 64 bit unsigned integer allows to support a
bandwidth_down with up to ~2.8e10 [100KiBit] and a perfect tq of 255. This
is ~6.6 times higher than the maximum possible value of the gateway
announcement TVLV.

This problem only affects the non-default gw_sel_class 1.

Signed-off-by: Ruben Wisniewsi <ruben@vfn-nrw.de>
[sven@narfation.org: rewritten commit message, changed to kernel type]
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>

Signed-off-by: Antonio Quartulli <antonio@meshcoding.com>
---
 net/batman-adv/gateway_client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/batman-adv/gateway_client.c b/net/batman-adv/gateway_client.c
index 090828c..ca734f8 100644
--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
@@ -133,7 +133,7 @@ batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)
 	struct batadv_neigh_node *router;
 	struct batadv_neigh_ifinfo *router_ifinfo;
 	struct batadv_gw_node *gw_node, *curr_gw = NULL;
-	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
+	uint64_t max_gw_factor = 0, tmp_gw_factor = 0;
 	uint32_t gw_divisor;
 	uint8_t max_tq = 0;
 	uint8_t tq_avg;
-- 
2.4.3

Re: [PATCH 1/2] batman-adv: Avoid u32 overflow during gateway select

[David Miller]

From: Antonio Quartulli <antonio@meshcoding.com>
Date: Tue, 16 Jun 2015 21:06:23 +0200

> @@ -133,7 +133,7 @@ batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)
>  	struct batadv_neigh_node *router;
>  	struct batadv_neigh_ifinfo *router_ifinfo;
>  	struct batadv_gw_node *gw_node, *curr_gw = NULL;
> -	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
> +	uint64_t max_gw_factor = 0, tmp_gw_factor = 0;
>  	uint32_t gw_divisor;

Divides are performed using these variables, so if you make
them 64-bit the build will fail.  You have to use one of the
routines in include/linux/math64.h in this situation, but
realize that this operation is now going to be quite expensive.

Re: [PATCH 1/2] batman-adv: Avoid u32 overflow during gateway select

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

On Sunday 21 June 2015 09:37:13 David Miller wrote:
> From: Antonio Quartulli <antonio@meshcoding.com>
> Date: Tue, 16 Jun 2015 21:06:23 +0200
> 
> > @@ -133,7 +133,7 @@ batadv_gw_get_best_gw_node(struct batadv_priv
> > *bat_priv)> 
> >  	struct batadv_neigh_node *router;
> >  	struct batadv_neigh_ifinfo *router_ifinfo;
> >  	struct batadv_gw_node *gw_node, *curr_gw = NULL;
> > 
> > -	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
> > +	uint64_t max_gw_factor = 0, tmp_gw_factor = 0;
> > 
> >  	uint32_t gw_divisor;
> 
> Divides are performed using these variables, so if you make
> them 64-bit the build will fail.  You have to use one of the
> routines in include/linux/math64.h in this situation, but
> realize that this operation is now going to be quite expensive.

Just a minor comment: The division is currently "max_gw_factor >> 18" which 
the compiler also outputs as such (shrd + shr on i586). This is the reason why 
it currently also works on architectures without a native 64 bit div.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] batman-adv: Avoid u32 overflow during gateway select

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

On 21/06/15 20:27, Sven Eckelmann wrote:
> On Sunday 21 June 2015 09:37:13 David Miller wrote:
>> From: Antonio Quartulli <antonio@meshcoding.com>
>> Date: Tue, 16 Jun 2015 21:06:23 +0200
>>
>>> @@ -133,7 +133,7 @@ batadv_gw_get_best_gw_node(struct batadv_priv
>>> *bat_priv)> 
>>>  	struct batadv_neigh_node *router;
>>>  	struct batadv_neigh_ifinfo *router_ifinfo;
>>>  	struct batadv_gw_node *gw_node, *curr_gw = NULL;
>>>
>>> -	uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
>>> +	uint64_t max_gw_factor = 0, tmp_gw_factor = 0;
>>>
>>>  	uint32_t gw_divisor;
>>
>> Divides are performed using these variables, so if you make
>> them 64-bit the build will fail.  You have to use one of the
>> routines in include/linux/math64.h in this situation, but
>> realize that this operation is now going to be quite expensive.
> 
> Just a minor comment: The division is currently "max_gw_factor >> 18" which 
> the compiler also outputs as such (shrd + shr on i586). This is the reason why 
> it currently also works on architectures without a native 64 bit div.


Thanks for the comment David and Sven.


David you can drop this pull request.
I'll send a new one later with a modified patch that explicitly
introduces the shift.



Thanks!

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] batman-adv: avoid DAT to mess up LAN state

[Antonio Quartulli]

When a node running DAT receives an ARP request from the LAN for the
first time, it is likely that this node will request the ARP entry
through the distributed ARP table (DAT) in the mesh.

Once a DAT reply is received the asking node must check if the MAC
address for which the IP address has been asked is local. If it is, the
node must drop the ARP reply bceause the client should have replied on
its own locally.

Forwarding this reply means fooling any L2 bridge (e.g. Ethernet
switches) lying between the batman-adv node and the LAN. This happens
because the L2 bridge will think that the client sending the ARP reply
lies somewhere in the mesh, while this node is sitting in the same LAN.

Reported-by: Simon Wunderlich <sw@simonwunderlich.de>
Signed-off-by: Antonio Quartulli <antonio@meshcoding.com>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
---
 net/batman-adv/distributed-arp-table.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/batman-adv/distributed-arp-table.c b/net/batman-adv/distributed-arp-table.c
index aad022d..5a4d45a 100644
--- a/net/batman-adv/distributed-arp-table.c
+++ b/net/batman-adv/distributed-arp-table.c
@@ -1107,6 +1107,9 @@ void batadv_dat_snoop_outgoing_arp_reply(struct batadv_priv *bat_priv,
  * @bat_priv: the bat priv with all the soft interface information
  * @skb: packet to check
  * @hdr_size: size of the encapsulation header
+ *
+ * Returns true if the packet was snooped and consumed by DAT. False if the
+ * packet has to be delivered to the interface
  */
 bool batadv_dat_snoop_incoming_arp_reply(struct batadv_priv *bat_priv,
 					 struct sk_buff *skb, int hdr_size)
@@ -1114,7 +1117,7 @@ bool batadv_dat_snoop_incoming_arp_reply(struct batadv_priv *bat_priv,
 	uint16_t type;
 	__be32 ip_src, ip_dst;
 	uint8_t *hw_src, *hw_dst;
-	bool ret = false;
+	bool dropped = false;
 	unsigned short vid;
 
 	if (!atomic_read(&bat_priv->distributed_arp_table))
@@ -1143,12 +1146,17 @@ bool batadv_dat_snoop_incoming_arp_reply(struct batadv_priv *bat_priv,
 	/* if this REPLY is directed to a client of mine, let's deliver the
 	 * packet to the interface
 	 */
-	ret = !batadv_is_my_client(bat_priv, hw_dst, vid);
+	dropped = !batadv_is_my_client(bat_priv, hw_dst, vid);
+
+	/* if this REPLY is sent on behalf of a client of mine, let's drop the
+	 * packet because the client will reply by itself
+	 */
+	dropped |= batadv_is_my_client(bat_priv, hw_src, vid);
 out:
-	if (ret)
+	if (dropped)
 		kfree_skb(skb);
-	/* if ret == false -> packet has to be delivered to the interface */
-	return ret;
+	/* if dropped == false -> deliver to the interface */
+	return dropped;
 }
 
 /**
-- 
2.4.3