[B.A.T.M.A.N.] [PATCH 0/2 net] Bug fixes for batman-adv 2012-06-23

[B.A.T.M.A.N.] [PATCH 0/2 net] Bug fixes for batman-adv 2012-06-23

[Antonio Quartulli]

Hello David,

here are two fixes intended for net/linux-3.5.

Patch 1 is a fix for the AP-Isolation feature. A wrong check made all the
broadcast packets coming from any client be dropped before delivery to the
interface.
Patch 2 instead fixes a "real" race condition in the TranslationTable code.

Please, tell me if there is any problem.
Thank you,
	Antonio



The following changes since commit a18e08bdcf845efb7344cea146e683df746bbfb4:

  net: sh_eth: fix the condition to fix the cur_tx/dirty_rx (2012-06-22 21:50:37 -0700)

are available in the git repository at:

  git://git.open-mesh.org/linux-merge.git batman-adv/maint

for you to fetch changes up to 8b8e4bc0391f8abbcdb9e1c54415bcc0f4f5a2a0:

  batman-adv: fix race condition in TT full-table replacement (2012-06-23 17:21:35 +0200)

----------------------------------------------------------------
Included changes:
- fix for the AP-isolation feature
- fix for a race condition in the TranslationTable code

----------------------------------------------------------------
Antonio Quartulli (1):
      batman-adv: fix race condition in TT full-table replacement

Marek Lindner (1):
      batman-adv: only drop packets of known wifi clients

 net/batman-adv/translation-table.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

[B.A.T.M.A.N.] [PATCH 1/2 net] batman-adv: only drop packets of known wifi clients

[Antonio Quartulli]

From: Marek Lindner <lindner_marek@yahoo.de>

bug introduced with 59b699cdee039d75915c354da06937102d1f9a84

If the source or destination mac address of an ethernet packet
could not be found in the translation table the packet was
dropped if AP isolation was turned on. This behavior would
make it impossible to send broadcast packets over the mesh as
the broadcast address will never enter the translation table.

Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
Acked-by: Antonio Quartulli <ordex@autistici.org>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 net/batman-adv/translation-table.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index a66c2dc..660c40f 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -2031,10 +2031,10 @@ bool is_ap_isolated(struct bat_priv *bat_priv, uint8_t *src, uint8_t *dst)
 {
 	struct tt_local_entry *tt_local_entry = NULL;
 	struct tt_global_entry *tt_global_entry = NULL;
-	bool ret = true;
+	bool ret = false;
 
 	if (!atomic_read(&bat_priv->ap_isolation))
-		return false;
+		goto out;
 
 	tt_local_entry = tt_local_hash_find(bat_priv, dst);
 	if (!tt_local_entry)
@@ -2044,10 +2044,10 @@ bool is_ap_isolated(struct bat_priv *bat_priv, uint8_t *src, uint8_t *dst)
 	if (!tt_global_entry)
 		goto out;
 
-	if (_is_ap_isolated(tt_local_entry, tt_global_entry))
+	if (!_is_ap_isolated(tt_local_entry, tt_global_entry))
 		goto out;
 
-	ret = false;
+	ret = true;
 
 out:
 	if (tt_global_entry)
-- 
1.7.9.4

[B.A.T.M.A.N.] [PATCH 2/2 net] batman-adv: fix race condition in TT full-table replacement

[Antonio Quartulli]

bug introduced with cea194d90b11aff7fc289149e4c7f305fad3535a

In the current TT code, when a TT_Response containing a full table is received
from an originator, first the node purges all the clients for that originator in
the global translation-table and then merges the newly received table.
During the purging phase each client deletion is done by means of a call_rcu()
invocation and at the end of this phase the global entry counter for that
originator is set to 0. However the invoked rcu function decreases the global
entry counter for that originator by one too and since the rcu invocation is
likely to be postponed, the node will end up in first setting the counter to 0
and then decreasing it one by one for each deleted client.

This bug leads to having a wrong global entry counter for the related node, say
X. Then when the node with the broken counter will answer to a TT_REQUEST on
behalf of node X, it will create faulty TT_RESPONSE that will generate an
unrecoverable situation on the node that asked for the full table recover.

The non-recoverability is given by the fact that the node with the broken
counter will keep answering on behalf of X because its knowledge about X's state
(ttvn + tt_crc) is correct.

To solve this problem the counter is not explicitly set to 0 anymore and the
counter decrement is performed right before the invocation of call_rcu().

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 net/batman-adv/translation-table.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 660c40f..2ab83d7 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -141,13 +141,14 @@ static void tt_orig_list_entry_free_rcu(struct rcu_head *rcu)
 	struct tt_orig_list_entry *orig_entry;
 
 	orig_entry = container_of(rcu, struct tt_orig_list_entry, rcu);
-	atomic_dec(&orig_entry->orig_node->tt_size);
 	orig_node_free_ref(orig_entry->orig_node);
 	kfree(orig_entry);
 }
 
 static void tt_orig_list_entry_free_ref(struct tt_orig_list_entry *orig_entry)
 {
+	/* to avoid race conditions, immediately decrease the tt counter */
+	atomic_dec(&orig_entry->orig_node->tt_size);
 	call_rcu(&orig_entry->rcu, tt_orig_list_entry_free_rcu);
 }
 
@@ -910,7 +911,6 @@ void tt_global_del_orig(struct bat_priv *bat_priv,
 		}
 		spin_unlock_bh(list_lock);
 	}
-	atomic_set(&orig_node->tt_size, 0);
 	orig_node->tt_initialised = false;
 }
 
-- 
1.7.9.4

Re: [B.A.T.M.A.N.] [PATCH 0/2 net] Bug fixes for batman-adv 2012-06-23

[Antonio Quartulli]

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

On Sat, Jun 23, 2012 at 05:30:57 +0200, Antonio Quartulli wrote:
> Hello David,
> 
> here are two fixes intended for net/linux-3.5.
> 
> Patch 1 is a fix for the AP-Isolation feature. A wrong check made all the
> broadcast packets coming from any client be dropped before delivery to the
> interface.
> Patch 2 instead fixes a "real" race condition in the TranslationTable code.
> 
> Please, tell me if there is any problem.
> Thank you,
> 	Antonio
> 
> 
> 
> The following changes since commit a18e08bdcf845efb7344cea146e683df746bbfb4:
> 
>   net: sh_eth: fix the condition to fix the cur_tx/dirty_rx (2012-06-22 21:50:37 -0700)
> 
> are available in the git repository at:
> 
>   git://git.open-mesh.org/linux-merge.git batman-adv/maint
> 
> for you to fetch changes up to 8b8e4bc0391f8abbcdb9e1c54415bcc0f4f5a2a0:
> 
>   batman-adv: fix race condition in TT full-table replacement (2012-06-23 17:21:35 +0200)
> 

Hello David,

after pulling these patchset in net, you should hit a conflict while trying to
merge net into net-next. The conflict is caused by the renaming patches that you
already have in the next tree.

Here are our instructions about how to solve it. Hope they will help.
Thank you.


Conflict 1:
<<<<<<<
	orig_entry = container_of(rcu, struct batadv_tt_orig_list_entry, rcu);
	atomic_dec(&orig_entry->orig_node->tt_size);
	batadv_orig_node_free_ref(orig_entry->orig_node);
=======
	orig_entry = container_of(rcu, struct tt_orig_list_entry, rcu);
	orig_node_free_ref(orig_entry->orig_node);
>>>>>>>

Resolves to:
=======
	orig_entry = container_of(rcu, struct batadv_tt_orig_list_entry, rcu);
	batadv_orig_node_free_ref(orig_entry->orig_node);
=======


Conflict 2:
<<<<<<<
	call_rcu(&orig_entry->rcu, batadv_tt_orig_list_entry_free_rcu);
=======
	/* to avoid race conditions, immediately decrease the tt counter */
	atomic_dec(&orig_entry->orig_node->tt_size);
	call_rcu(&orig_entry->rcu, batadv_tt_orig_list_entry_free_rcu);
>>>>>>>

Resolves to:
=======
	/* to avoid race conditions, immediately decrease the tt counter */
	atomic_dec(&orig_entry->orig_node->tt_size);
	call_rcu(&orig_entry->rcu, batadv_tt_orig_list_entry_free_rcu);
=======


Conflict 3:
<<<<<<<
	struct batadv_tt_local_entry *tt_local_entry = NULL;
	struct batadv_tt_global_entry *tt_global_entry = NULL;
	bool ret = true;
=======
	struct tt_local_entry *tt_local_entry = NULL;
	struct tt_global_entry *tt_global_entry = NULL;
	bool ret = false;
>>>>>>>

Resolves to:
=======
	struct batadv_tt_local_entry *tt_local_entry = NULL;
	struct batadv_tt_global_entry *tt_global_entry = NULL;
	bool ret = false;
=======


Conflict 4:
<<<<<<<
	if (_batadv_is_ap_isolated(tt_local_entry, tt_global_entry))
=======
	if (!_is_ap_isolated(tt_local_entry, tt_global_entry))
>>>>>>>

Resolves to:
=======
	if (!_batadv_is_ap_isolated(tt_local_entry, tt_global_entry))
=======


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 0/2 net] Bug fixes for batman-adv 2012-06-23

[David Miller]

From: Antonio Quartulli <ordex@autistici.org>
Date: Sat, 23 Jun 2012 17:30:57 +0200

> Patch 1 is a fix for the AP-Isolation feature. A wrong check made all the
> broadcast packets coming from any client be dropped before delivery to the
> interface.
> Patch 2 instead fixes a "real" race condition in the TranslationTable code.
 ...
>   git://git.open-mesh.org/linux-merge.git batman-adv/maint

Pulled, thanks.

Re: [B.A.T.M.A.N.] [PATCH 0/2 net] Bug fixes for batman-adv 2012-06-23

[David Miller]

From: Antonio Quartulli <ordex@autistici.org>
Date: Sat, 23 Jun 2012 17:45:12 +0200

> after pulling these patchset in net, you should hit a conflict while trying to
> merge net into net-next. The conflict is caused by the renaming patches that you
> already have in the next tree.
> 
> Here are our instructions about how to solve it. Hope they will help.

Thanks a lot for this.