Message ID | 1440170118-10876-5-git-send-email-sw@simonwunderlich.de (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <sw@simonwunderlich.de> Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=79.140.42.25; helo=mail.mail.packetmixer.de; envelope-from=sw@simonwunderlich.de; receiver=b.a.t.m.a.n@lists.open-mesh.org Received: from mail.mail.packetmixer.de (packetmixer.de [79.140.42.25]) by open-mesh.org (Postfix) with ESMTPS id 342F480E64 for <b.a.t.m.a.n@lists.open-mesh.org>; Fri, 21 Aug 2015 17:15:26 +0200 (CEST) Received: from kero.packetmixer.de (unknown [IPv6:2a02:3100:260b:5600:221:ccff:fe73:b665]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.mail.packetmixer.de (Postfix) with ESMTPSA id 6D979174005; Fri, 21 Aug 2015 17:15:26 +0200 (CEST) From: Simon Wunderlich <sw@simonwunderlich.de> To: b.a.t.m.a.n@lists.open-mesh.org Date: Fri, 21 Aug 2015 17:15:18 +0200 Message-Id: <1440170118-10876-5-git-send-email-sw@simonwunderlich.de> X-Mailer: git-send-email 2.5.0 In-Reply-To: <1440170118-10876-1-git-send-email-sw@simonwunderlich.de> References: <1440170118-10876-1-git-send-email-sw@simonwunderlich.de> Cc: alessandro@mediaspot.net Subject: [B.A.T.M.A.N.] [PATCH-maint 4/4] batman-adv: detect local excess vlans in TT request X-BeenThere: b.a.t.m.a.n@lists.open-mesh.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n.lists.open-mesh.org> List-Unsubscribe: <https://lists.open-mesh.org/mm/options/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=unsubscribe> List-Archive: <http://lists.open-mesh.org/pipermail/b.a.t.m.a.n/> List-Post: <mailto:b.a.t.m.a.n@lists.open-mesh.org> List-Help: <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=help> List-Subscribe: <https://lists.open-mesh.org/mm/listinfo/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=subscribe> X-List-Received-Date: Fri, 21 Aug 2015 15:15:26 -0000 |
Commit Message
Simon Wunderlich
Aug. 21, 2015, 3:15 p.m. UTC
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(+)
Comments
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; > } > >
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
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,
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
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; }