* [B.A.T.M.A.N.] [PATCH-maint 0/4] Some fixes for DAT/TT/Speedy join corner cases
@ 2015-08-21 15:15 Simon Wunderlich
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 1/4] batman-adv: fix speedy join for DAT cache replies Simon Wunderlich
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-21 15:15 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: alessandro
Upon debugging a network with dangling, bogus TT entries, I found a
couple of bugs for which I would like to propose fixes. The network showed
the following symptoms:
* DAT was enabled
* VLANs were used, but not every originator used the same VLANs
* I've found global entries assigned to originators which actually
never had the client in question connected to them. These false target
originators didn't even had the VLANs in use or bridged for which
the entry was done. This caused packets sent to be sent to these wrong
originators.
* The wrong entries also didn't get purged automatically, since they
didn't announce the VLAN in question through their TT TLVLs, and the
TT code didn't check for excess VLANs.
* Furthermore, the temp flag was removed too early from the TT entries
so that the purge function was not removing the entry after a timeout
as well.
* I've found that with DAT, the cached ARP replies may cause these
TT entries to be created on behalf of the answering host. This is wrong,
since the answering host (usually) doesn't actually has the client
connected.
Three of these four patches fix various issues connected with the problem
described above. The fourth one is merely a style fix which does not neccessarily
have to be adopted to maint.
We have not yet tested these patches in the network, and the bug also appears
only once in a couple of days. Therefore I'd like to ask to review these patches
thoroughly, and if you agree to the fixes we will apply them in this production
network.
Thanks,
Simon
Simon Wunderlich (4):
batman-adv: fix speedy join for DAT cache replies
batman-adv: avoid keeping false temporary entry
batman-adv: unify flags access style in tt global add
batman-adv: detect local excess vlans in TT request
net/batman-adv/routing.c | 17 +++++++++++++----
net/batman-adv/translation-table.c | 33 ++++++++++++++++++++++++++++++---
2 files changed, 43 insertions(+), 7 deletions(-)
--
2.5.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [B.A.T.M.A.N.] [PATCH-maint 1/4] batman-adv: fix speedy join for DAT cache replies
2015-08-21 15:15 [B.A.T.M.A.N.] [PATCH-maint 0/4] Some fixes for DAT/TT/Speedy join corner cases Simon Wunderlich
@ 2015-08-21 15:15 ` Simon Wunderlich
2015-08-25 9:42 ` Antonio Quartulli
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 2/4] batman-adv: avoid keeping false temporary entry Simon Wunderlich
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-21 15:15 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: alessandro
DAT Cache replies are answered on behalf of other clients which are not
connected to the answering originator. Therefore, we shouldn't add these
clients to the answering originators TT table through speed join to
avoid bogus entries.
Reported-by: Alessandro Bolletta <alessandro@mediaspot.net>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
---
net/batman-adv/routing.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index c360c0c..f2779b6 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -836,6 +836,7 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
uint8_t *orig_addr;
struct batadv_orig_node *orig_node = NULL;
int check, hdr_size = sizeof(*unicast_packet);
+ enum batadv_subtype subtype;
bool is4addr;
unicast_packet = (struct batadv_unicast_packet *)skb->data;
@@ -863,10 +864,18 @@ int batadv_recv_unicast_packet(struct sk_buff *skb,
/* packet for me */
if (batadv_is_my_mac(bat_priv, unicast_packet->dest)) {
if (is4addr) {
- batadv_dat_inc_counter(bat_priv,
- unicast_4addr_packet->subtype);
- orig_addr = unicast_4addr_packet->src;
- orig_node = batadv_orig_hash_find(bat_priv, orig_addr);
+ subtype = unicast_4addr_packet->subtype;
+ batadv_dat_inc_counter(bat_priv, subtype);
+
+ /* Cache replies should not be considered for speedy
+ * join, since the clients do not actually reside at
+ * the originator.
+ */
+ if (subtype != BATADV_P_DAT_CACHE_REPLY) {
+ orig_addr = unicast_4addr_packet->src;
+ orig_node = batadv_orig_hash_find(bat_priv,
+ orig_addr);
+ }
}
if (batadv_dat_snoop_incoming_arp_request(bat_priv, skb,
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [B.A.T.M.A.N.] [PATCH-maint 2/4] batman-adv: avoid keeping false temporary entry
2015-08-21 15:15 [B.A.T.M.A.N.] [PATCH-maint 0/4] Some fixes for DAT/TT/Speedy join corner cases Simon Wunderlich
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 1/4] batman-adv: fix speedy join for DAT cache replies Simon Wunderlich
@ 2015-08-21 15:15 ` Simon Wunderlich
2015-08-25 9:49 ` Antonio Quartulli
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 3/4] batman-adv: unify flags access style in tt global add Simon Wunderlich
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request Simon Wunderlich
3 siblings, 1 reply; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-21 15:15 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: alessandro
In the case when a temporary entry is added first and a proper tt entry
is added after that, the temporary tt entry is kept in the orig list.
However the temporary flag is removed at this point, and therefore the
purge function can not find this temporary entry anymore.
Therefore, remove the previous temp entry before adding the new proper
one.
Reported-by: Alessandro Bolletta <alessandro@mediaspot.net>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
---
net/batman-adv/translation-table.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 7986ec5..f629c21 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -1416,9 +1416,15 @@ static bool batadv_tt_global_add(struct batadv_priv *bat_priv,
}
/* if the client was temporary added before receiving the first
- * OGM announcing it, we have to clear the TEMP flag
+ * OGM announcing it, we have to clear the TEMP flag. Also,
+ * remove the previous temporary orig node and re-add it
+ * if required. If the orig entry changed, the new one which
+ * is a non-temporary entry is preferred.
*/
- common->flags &= ~BATADV_TT_CLIENT_TEMP;
+ if (common->flags & BATADV_TT_CLIENT_TEMP) {
+ batadv_tt_global_del_orig_list(tt_global_entry);
+ common->flags &= ~BATADV_TT_CLIENT_TEMP;
+ }
/* the change can carry possible "attribute" flags like the
* TT_CLIENT_WIFI, therefore they have to be copied in the
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [B.A.T.M.A.N.] [PATCH-maint 3/4] batman-adv: unify flags access style in tt global add
2015-08-21 15:15 [B.A.T.M.A.N.] [PATCH-maint 0/4] Some fixes for DAT/TT/Speedy join corner cases Simon Wunderlich
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 1/4] batman-adv: fix speedy join for DAT cache replies Simon Wunderlich
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 2/4] batman-adv: avoid keeping false temporary entry Simon Wunderlich
@ 2015-08-21 15:15 ` Simon Wunderlich
2015-08-25 9:51 ` Antonio Quartulli
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request Simon Wunderlich
3 siblings, 1 reply; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-21 15:15 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: alessandro
This should slightly improve readability
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
---
net/batman-adv/translation-table.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index f629c21..dced2da 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -1430,7 +1430,7 @@ static bool batadv_tt_global_add(struct batadv_priv *bat_priv,
* TT_CLIENT_WIFI, therefore they have to be copied in the
* client entry
*/
- tt_global_entry->common.flags |= flags;
+ common->flags |= flags;
/* If there is the BATADV_TT_CLIENT_ROAM flag set, there is only
* one originator left in the list and we previously received a
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request
2015-08-21 15:15 [B.A.T.M.A.N.] [PATCH-maint 0/4] Some fixes for DAT/TT/Speedy join corner cases Simon Wunderlich
` (2 preceding siblings ...)
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 3/4] batman-adv: unify flags access style in tt global add Simon Wunderlich
@ 2015-08-21 15:15 ` Simon Wunderlich
2015-08-25 9:59 ` Antonio Quartulli
3 siblings, 1 reply; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-21 15:15 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: alessandro
If the local representation of the global TT table of one originator has
more VLAN entries than the respective TT update, there is some
inconsistency present. By detecting and reporting this inconsistency,
the global table gets updated and the excess VLAN will get removed in
the process.
Reported-by: Alessandro Bolletta <alessandro@mediaspot.net>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
---
net/batman-adv/translation-table.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index dced2da..1adb72e 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -2392,6 +2392,7 @@ static bool batadv_tt_global_check_crc(struct batadv_orig_node *orig_node,
struct batadv_tvlv_tt_vlan_data *tt_vlan_tmp;
struct batadv_orig_node_vlan *vlan;
uint32_t crc;
+ bool found;
int i;
/* check if each received CRC matches the locally stored one */
@@ -2418,6 +2419,26 @@ static bool batadv_tt_global_check_crc(struct batadv_orig_node *orig_node,
return false;
}
+ /* check if any excess VLANs exist locally for the originator
+ * which are not mentioned in the TVLV from the originator.
+ */
+ rcu_read_lock();
+ list_for_each_entry_rcu(vlan, &orig_node->vlan_list, list) {
+ found = false;
+
+ for (i = 0; i < num_vlan; i++) {
+ tt_vlan_tmp = tt_vlan + i;
+ if (ntohs(tt_vlan_tmp->vid) == vlan->vid) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ return false;
+ }
+ rcu_read_unlock();
+
return true;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 1/4] batman-adv: fix speedy join for DAT cache replies
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 1/4] batman-adv: fix speedy join for DAT cache replies Simon Wunderlich
@ 2015-08-25 9:42 ` Antonio Quartulli
2015-08-25 15:24 ` Simon Wunderlich
0 siblings, 1 reply; 16+ messages in thread
From: Antonio Quartulli @ 2015-08-25 9:42 UTC (permalink / raw)
To: Simon Wunderlich
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 667 bytes --]
On 21/08/15 17:15, Simon Wunderlich wrote:
> + /* Cache replies should not be considered for speedy
> + * join, since the clients do not actually reside at
> + * the originator.
> + */
> + if (subtype != BATADV_P_DAT_CACHE_REPLY) {
Tha patch looks good but this should really be:
if (subtype == BATADV_P_DATA) {
because speedy join is supposed to work with any payload packet.
Cheers,
> + orig_addr = unicast_4addr_packet->src;
> + orig_node = batadv_orig_hash_find(bat_priv,
> + orig_addr);
> + }
> }
>
> if (batadv_dat_snoop_incoming_arp_request(bat_priv, skb,
>
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 2/4] batman-adv: avoid keeping false temporary entry
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 2/4] batman-adv: avoid keeping false temporary entry Simon Wunderlich
@ 2015-08-25 9:49 ` Antonio Quartulli
2015-08-25 15:27 ` Simon Wunderlich
0 siblings, 1 reply; 16+ messages in thread
From: Antonio Quartulli @ 2015-08-25 9:49 UTC (permalink / raw)
To: Simon Wunderlich
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]
On 21/08/15 17:15, Simon Wunderlich wrote:
> In the case when a temporary entry is added first and a proper tt entry
> is added after that, the temporary tt entry is kept in the orig list.
> However the temporary flag is removed at this point, and therefore the
> purge function can not find this temporary entry anymore.
>
> Therefore, remove the previous temp entry before adding the new proper
> one.
>
[..]
> /* if the client was temporary added before receiving the first
> - * OGM announcing it, we have to clear the TEMP flag
> + * OGM announcing it, we have to clear the TEMP flag. Also,
> + * remove the previous temporary orig node and re-add it
> + * if required. If the orig entry changed, the new one which
> + * is a non-temporary entry is preferred.
> */
> - common->flags &= ~BATADV_TT_CLIENT_TEMP;
> + if (common->flags & BATADV_TT_CLIENT_TEMP) {
> + batadv_tt_global_del_orig_list(tt_global_entry);
> + common->flags &= ~BATADV_TT_CLIENT_TEMP;
> + }
mh...interesting..and nice catch. Have you tested this with a client
roaming from A to B during the "speedy join" period?
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 3/4] batman-adv: unify flags access style in tt global add
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 3/4] batman-adv: unify flags access style in tt global add Simon Wunderlich
@ 2015-08-25 9:51 ` Antonio Quartulli
2015-08-25 15:28 ` Simon Wunderlich
0 siblings, 1 reply; 16+ messages in thread
From: Antonio Quartulli @ 2015-08-25 9:51 UTC (permalink / raw)
To: Simon Wunderlich
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 194 bytes --]
On 21/08/15 17:15, Simon Wunderlich wrote:
> This should slightly improve readability
>
Patch looks good, but it is definitely *not* for maint.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request Simon Wunderlich
@ 2015-08-25 9:59 ` Antonio Quartulli
2015-08-25 15:31 ` Simon Wunderlich
0 siblings, 1 reply; 16+ messages in thread
From: Antonio Quartulli @ 2015-08-25 9:59 UTC (permalink / raw)
To: Simon Wunderlich
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 2239 bytes --]
On 21/08/15 17:15, Simon Wunderlich wrote:
> If the local representation of the global TT table of one originator has
> more VLAN entries than the respective TT update, there is some
> inconsistency present. By detecting and reporting this inconsistency,
> the global table gets updated and the excess VLAN will get removed in
> the process.
>
> Reported-by: Alessandro Bolletta <alessandro@mediaspot.net>
> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
> ---
> net/batman-adv/translation-table.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
> index dced2da..1adb72e 100644
> --- a/net/batman-adv/translation-table.c
> +++ b/net/batman-adv/translation-table.c
> @@ -2392,6 +2392,7 @@ static bool batadv_tt_global_check_crc(struct batadv_orig_node *orig_node,
> struct batadv_tvlv_tt_vlan_data *tt_vlan_tmp;
> struct batadv_orig_node_vlan *vlan;
> uint32_t crc;
> + bool found;
> int i;
>
> /* check if each received CRC matches the locally stored one */
> @@ -2418,6 +2419,26 @@ static bool batadv_tt_global_check_crc(struct batadv_orig_node *orig_node,
> return false;
> }
>
> + /* check if any excess VLANs exist locally for the originator
> + * which are not mentioned in the TVLV from the originator.
> + */
> + rcu_read_lock();
> + list_for_each_entry_rcu(vlan, &orig_node->vlan_list, list) {
> + found = false;
> +
> + for (i = 0; i < num_vlan; i++) {
> + tt_vlan_tmp = tt_vlan + i;
> + if (ntohs(tt_vlan_tmp->vid) == vlan->vid) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found)
> + return false;
> + }
> + rcu_read_unlock();
> +
NAK.
we already do this check slightly above in this function with the
following code:
2426 vlan = batadv_orig_node_vlan_get(orig_node,
2427
ntohs(tt_vlan_tmp->vid));
2428 if (!vlan)
2429 return false;
batadv_orig_node_vlan_get() returns NULL if we don't know this VLAN for
that Originator, therefore the CRC check fails here.
Cheers,
> return true;
> }
>
>
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 1/4] batman-adv: fix speedy join for DAT cache replies
2015-08-25 9:42 ` Antonio Quartulli
@ 2015-08-25 15:24 ` Simon Wunderlich
0 siblings, 0 replies; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-25 15:24 UTC (permalink / raw)
To: Antonio Quartulli
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 553 bytes --]
Hi Antonio,
On Tuesday 25 August 2015 11:42:05 Antonio Quartulli wrote:
> On 21/08/15 17:15, Simon Wunderlich wrote:
> > + /* Cache replies should not be considered for speedy
> > + * join, since the clients do not actually reside at
> > + * the originator.
> > + */
> > + if (subtype != BATADV_P_DAT_CACHE_REPLY) {
>
> Tha patch looks good but this should really be:
>
> if (subtype == BATADV_P_DATA) {
>
> because speedy join is supposed to work with any payload packet.
>
OK, will make that change in PATCH v2.
Thanks,
Simon
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 2/4] batman-adv: avoid keeping false temporary entry
2015-08-25 9:49 ` Antonio Quartulli
@ 2015-08-25 15:27 ` Simon Wunderlich
0 siblings, 0 replies; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-25 15:27 UTC (permalink / raw)
To: Antonio Quartulli
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]
Hi Antonio,
On Tuesday 25 August 2015 11:49:17 Antonio Quartulli wrote:
> On 21/08/15 17:15, Simon Wunderlich wrote:
> > In the case when a temporary entry is added first and a proper tt entry
> > is added after that, the temporary tt entry is kept in the orig list.
> > However the temporary flag is removed at this point, and therefore the
> > purge function can not find this temporary entry anymore.
> >
> > Therefore, remove the previous temp entry before adding the new proper
> > one.
>
> [..]
>
> > /* if the client was temporary added before receiving the first
> >
> > - * OGM announcing it, we have to clear the TEMP flag
> > + * OGM announcing it, we have to clear the TEMP flag. Also,
> > + * remove the previous temporary orig node and re-add it
> > + * if required. If the orig entry changed, the new one which
> > + * is a non-temporary entry is preferred.
> >
> > */
> >
> > - common->flags &= ~BATADV_TT_CLIENT_TEMP;
> > + if (common->flags & BATADV_TT_CLIENT_TEMP) {
> > + batadv_tt_global_del_orig_list(tt_global_entry);
> > + common->flags &= ~BATADV_TT_CLIENT_TEMP;
> > + }
>
> mh...interesting..and nice catch. Have you tested this with a client
> roaming from A to B during the "speedy join" period?
No I didn't. I've just seen bad entries dangling and never getting cleaned up,
and suspected that "speedy join" added TT the entry falsely. However, in that
case it should only have been temporarily and getting removed later, which
wasn't the case. Therefore this patch ...
I haven't tested this patch on the production network because its very hard to
reproduce there, and the bug is probably squashed already with the first
patch.
Thanks for reviewing!
Simon
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 3/4] batman-adv: unify flags access style in tt global add
2015-08-25 9:51 ` Antonio Quartulli
@ 2015-08-25 15:28 ` Simon Wunderlich
2015-08-25 16:14 ` Antonio Quartulli
0 siblings, 1 reply; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-25 15:28 UTC (permalink / raw)
To: Antonio Quartulli
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 376 bytes --]
On Tuesday 25 August 2015 11:51:24 Antonio Quartulli wrote:
> On 21/08/15 17:15, Simon Wunderlich wrote:
> > This should slightly improve readability
>
> Patch looks good, but it is definitely *not* for maint.
>
> Cheers,
Shall I keep it in the patchset, and our friendly maintainer can merge it on
master instead? Or do you prefer to send it separately?
Thanks,
Simo
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request
2015-08-25 9:59 ` Antonio Quartulli
@ 2015-08-25 15:31 ` Simon Wunderlich
2015-08-25 18:28 ` Antonio Quartulli
0 siblings, 1 reply; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-25 15:31 UTC (permalink / raw)
To: Antonio Quartulli
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 2861 bytes --]
On Tuesday 25 August 2015 11:59:01 Antonio Quartulli wrote:
> On 21/08/15 17:15, Simon Wunderlich wrote:
> > If the local representation of the global TT table of one originator has
> > more VLAN entries than the respective TT update, there is some
> > inconsistency present. By detecting and reporting this inconsistency,
> > the global table gets updated and the excess VLAN will get removed in
> > the process.
> >
> > Reported-by: Alessandro Bolletta <alessandro@mediaspot.net>
> > Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
> > ---
> >
> > net/batman-adv/translation-table.c | 21 +++++++++++++++++++++
> > 1 file changed, 21 insertions(+)
> >
> > diff --git a/net/batman-adv/translation-table.c
> > b/net/batman-adv/translation-table.c index dced2da..1adb72e 100644
> > --- a/net/batman-adv/translation-table.c
> > +++ b/net/batman-adv/translation-table.c
> > @@ -2392,6 +2392,7 @@ static bool batadv_tt_global_check_crc(struct
> > batadv_orig_node *orig_node,>
> > struct batadv_tvlv_tt_vlan_data *tt_vlan_tmp;
> > struct batadv_orig_node_vlan *vlan;
> > uint32_t crc;
> >
> > + bool found;
> >
> > int i;
> >
> > /* check if each received CRC matches the locally stored one */
> >
> > @@ -2418,6 +2419,26 @@ static bool batadv_tt_global_check_crc(struct
> > batadv_orig_node *orig_node,>
> > return false;
> >
> > }
> >
> > + /* check if any excess VLANs exist locally for the originator
> > + * which are not mentioned in the TVLV from the originator.
> > + */
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(vlan, &orig_node->vlan_list, list) {
> > + found = false;
> > +
> > + for (i = 0; i < num_vlan; i++) {
> > + tt_vlan_tmp = tt_vlan + i;
> > + if (ntohs(tt_vlan_tmp->vid) == vlan->vid) {
> > + found = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!found)
> > + return false;
> > + }
> > + rcu_read_unlock();
> > +
>
> NAK.
>
> we already do this check slightly above in this function with the
> following code:
>
> 2426 vlan = batadv_orig_node_vlan_get(orig_node,
> 2427
> ntohs(tt_vlan_tmp->vid));
> 2428 if (!vlan)
> 2429 return false;
>
> batadv_orig_node_vlan_get() returns NULL if we don't know this VLAN for
> that Originator, therefore the CRC check fails here.
That's right, however it only sweeps through the VLANs announced within the
TT-TVLV. However, my addition tries to check if there are any excess VLAN
locally which are NOT in that TT-TVLV. I think this patch doesn't take care of
that, or am I missing something?
For example, think of having VLAN 6 locally with a couple of global entries at
the originator, but the TT-TVLV only announces VLANs 3,4,5. Then the fact that
we also have VLAN 6 is not detected, and these (probably wrong) entries are
never cleaned up.
Thanks!
Simon
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 3/4] batman-adv: unify flags access style in tt global add
2015-08-25 15:28 ` Simon Wunderlich
@ 2015-08-25 16:14 ` Antonio Quartulli
0 siblings, 0 replies; 16+ messages in thread
From: Antonio Quartulli @ 2015-08-25 16:14 UTC (permalink / raw)
To: Simon Wunderlich
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 564 bytes --]
On 25/08/15 17:28, Simon Wunderlich wrote:
> On Tuesday 25 August 2015 11:51:24 Antonio Quartulli wrote:
>> On 21/08/15 17:15, Simon Wunderlich wrote:
>>> This should slightly improve readability
>>
>> Patch looks good, but it is definitely *not* for maint.
>>
>> Cheers,
>
> Shall I keep it in the patchset, and our friendly maintainer can merge it on
> master instead? Or do you prefer to send it separately?
Up to the friendly maintainer. but if you ask me it is better to send
this patch separately.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request
2015-08-25 15:31 ` Simon Wunderlich
@ 2015-08-25 18:28 ` Antonio Quartulli
2015-08-25 21:12 ` Simon Wunderlich
0 siblings, 1 reply; 16+ messages in thread
From: Antonio Quartulli @ 2015-08-25 18:28 UTC (permalink / raw)
To: Simon Wunderlich
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 990 bytes --]
On 25/08/15 17:31, Simon Wunderlich wrote:
>> batadv_orig_node_vlan_get() returns NULL if we don't know this VLAN for
>> that Originator, therefore the CRC check fails here.
>
> That's right, however it only sweeps through the VLANs announced within the
> TT-TVLV. However, my addition tries to check if there are any excess VLAN
> locally which are NOT in that TT-TVLV. I think this patch doesn't take care of
> that, or am I missing something?
>
> For example, think of having VLAN 6 locally with a couple of global entries at
> the originator, but the TT-TVLV only announces VLANs 3,4,5. Then the fact that
> we also have VLAN 6 is not detected, and these (probably wrong) entries are
> never cleaned up.
Right, this check is required, but what about just checking the VLAN
count in the tt packet and in the originator struct ? if the number is
different it means that there must be an excess in the local struct.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request
2015-08-25 18:28 ` Antonio Quartulli
@ 2015-08-25 21:12 ` Simon Wunderlich
0 siblings, 0 replies; 16+ messages in thread
From: Simon Wunderlich @ 2015-08-25 21:12 UTC (permalink / raw)
To: Antonio Quartulli
Cc: The list for a Better Approach To Mobile Ad-hoc Networking, alessandro
[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]
On Tuesday 25 August 2015 20:28:48 Antonio Quartulli wrote:
> On 25/08/15 17:31, Simon Wunderlich wrote:
> >> batadv_orig_node_vlan_get() returns NULL if we don't know this VLAN for
> >> that Originator, therefore the CRC check fails here.
> >
> > That's right, however it only sweeps through the VLANs announced within
> > the
> > TT-TVLV. However, my addition tries to check if there are any excess VLAN
> > locally which are NOT in that TT-TVLV. I think this patch doesn't take
> > care of that, or am I missing something?
> >
> > For example, think of having VLAN 6 locally with a couple of global
> > entries at the originator, but the TT-TVLV only announces VLANs 3,4,5.
> > Then the fact that we also have VLAN 6 is not detected, and these
> > (probably wrong) entries are never cleaned up.
>
> Right, this check is required, but what about just checking the VLAN
> count in the tt packet and in the originator struct ? if the number is
> different it means that there must be an excess in the local struct.
You are right, just counting would be way simpler than comparing for the right
VLANs (since we don't use that information anyway). And this patch also did
not unlock the rcu-lock properly ... I'll resend this one with the simpler
method.
Thanks!
Simon
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-08-25 21:12 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-21 15:15 [B.A.T.M.A.N.] [PATCH-maint 0/4] Some fixes for DAT/TT/Speedy join corner cases Simon Wunderlich
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 1/4] batman-adv: fix speedy join for DAT cache replies Simon Wunderlich
2015-08-25 9:42 ` Antonio Quartulli
2015-08-25 15:24 ` Simon Wunderlich
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 2/4] batman-adv: avoid keeping false temporary entry Simon Wunderlich
2015-08-25 9:49 ` Antonio Quartulli
2015-08-25 15:27 ` Simon Wunderlich
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 3/4] batman-adv: unify flags access style in tt global add Simon Wunderlich
2015-08-25 9:51 ` Antonio Quartulli
2015-08-25 15:28 ` Simon Wunderlich
2015-08-25 16:14 ` Antonio Quartulli
2015-08-21 15:15 ` [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request Simon Wunderlich
2015-08-25 9:59 ` Antonio Quartulli
2015-08-25 15:31 ` Simon Wunderlich
2015-08-25 18:28 ` Antonio Quartulli
2015-08-25 21:12 ` Simon Wunderlich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.