| Message ID | 1440600113-21305-3-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 E9E458024C for <b.a.t.m.a.n@lists.open-mesh.org>; Wed, 26 Aug 2015 16:39:15 +0200 (CEST) Received: from kero.packetmixer.de (p549BDBE2.dip0.t-ipconnect.de [84.155.219.226]) (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 33A81174001; Wed, 26 Aug 2015 16:39:16 +0200 (CEST) From: Simon Wunderlich <sw@simonwunderlich.de> To: b.a.t.m.a.n@lists.open-mesh.org Date: Wed, 26 Aug 2015 16:41:52 +0200 Message-Id: <1440600113-21305-3-git-send-email-sw@simonwunderlich.de> X-Mailer: git-send-email 2.5.0 In-Reply-To: <1440600113-21305-1-git-send-email-sw@simonwunderlich.de> References: <1440600113-21305-1-git-send-email-sw@simonwunderlich.de> Cc: alessandro@mediaspot.net Subject: [B.A.T.M.A.N.] [PATCH-maintv2 2/3] batman-adv: avoid keeping false temporary entry 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: Wed, 26 Aug 2015 14:39:16 -0000 |
Commit Message
Simon Wunderlich
Aug. 26, 2015, 2:41 p.m. UTC
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(-)
Comments
On 26/08/15 16:41, 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. When can this really happen? When a temporary client has roamed to a new originator before it could be claimed by the first one ? Or are there other cases ? I think it is important to make this clear, because the original logic was such as the expected behaviour was to receive an ADD event from the same originator where the temporary client was seen. Other than this, the patch looks good.
On Tuesday 01 September 2015 11:00:29 Antonio Quartulli wrote: > On 26/08/15 16:41, 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. > > When can this really happen? When a temporary client has roamed to a new > originator before it could be claimed by the first one ? Or are there > other cases ? I think it is important to make this clear, because the > original logic was such as the expected behaviour was to receive an ADD > event from the same originator where the temporary client was seen. > > Other than this, the patch looks good. I've seen the problem in that problematic case which was fixed in PATCHv1. Practically, it is unlikely to happen unless there is a malicious attacker or another bug like the one described earlier - that is, speedy join is triggered without any actual TT update. The main problem I've perceived here was that the TT entry got the temporary flag removed even if the original sender didn't supply a proper TT announcement yet. The reason for this was that it got a proper reply from another orig node. The roaming case you've described would also cause the temporary client to be removed, but might be added later through a ''proper'' TT announcement. Do you think I should include any of this in the commit message? Cheers, Simon
On 01/09/15 13:00, Simon Wunderlich wrote: > On Tuesday 01 September 2015 11:00:29 Antonio Quartulli wrote: >> On 26/08/15 16:41, 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. >> >> When can this really happen? When a temporary client has roamed to a new >> originator before it could be claimed by the first one ? Or are there >> other cases ? I think it is important to make this clear, because the >> original logic was such as the expected behaviour was to receive an ADD >> event from the same originator where the temporary client was seen. >> >> Other than this, the patch looks good. > > I've seen the problem in that problematic case which was fixed in PATCHv1. > Practically, it is unlikely to happen unless there is a malicious attacker or > another bug like the one described earlier - that is, speedy join is triggered > without any actual TT update. > > The main problem I've perceived here was that the TT entry got the temporary > flag removed even if the original sender didn't supply a proper TT > announcement yet. The reason for this was that it got a proper reply from > another orig node. > > The roaming case you've described would also cause the temporary client to be > removed, but might be added later through a ''proper'' TT announcement. > > Do you think I should include any of this in the commit message? I think you should mention something that makes this case "real": I'd just say that it is possible that a client detected behind a given originator moves before an actual claim is made, so triggering this particular configuration. I hope you think this is fine. Cheers,
On Tuesday 01 September 2015 13:30:43 Antonio Quartulli wrote: > On 01/09/15 13:00, Simon Wunderlich wrote: > > On Tuesday 01 September 2015 11:00:29 Antonio Quartulli wrote: > >> On 26/08/15 16:41, 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. > >> > >> When can this really happen? When a temporary client has roamed to a new > >> originator before it could be claimed by the first one ? Or are there > >> other cases ? I think it is important to make this clear, because the > >> original logic was such as the expected behaviour was to receive an ADD > >> event from the same originator where the temporary client was seen. > >> > >> Other than this, the patch looks good. > > > > I've seen the problem in that problematic case which was fixed in PATCHv1. > > Practically, it is unlikely to happen unless there is a malicious attacker > > or another bug like the one described earlier - that is, speedy join is > > triggered without any actual TT update. > > > > The main problem I've perceived here was that the TT entry got the > > temporary flag removed even if the original sender didn't supply a proper > > TT announcement yet. The reason for this was that it got a proper reply > > from another orig node. > > > > The roaming case you've described would also cause the temporary client to > > be removed, but might be added later through a ''proper'' TT > > announcement. > > > > Do you think I should include any of this in the commit message? > > I think you should mention something that makes this case "real": I'd > just say that it is possible that a client detected behind a given > originator moves before an actual claim is made, so triggering this > particular configuration. Sounds good, I'll extend the commit message. Thanks! Simon
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