From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754308AbcGVMsZ (ORCPT ); Fri, 22 Jul 2016 08:48:25 -0400 Received: from smtp2.ustc.edu.cn ([202.38.64.46]:56732 "EHLO ustc.edu.cn" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753444AbcGVMsX (ORCPT ); Fri, 22 Jul 2016 08:48:23 -0400 Date: Fri, 22 Jul 2016 12:47:57 +0000 (UTC) Message-Id: <20160722.124757.283248020246505940.hchunhui@mail.ustc.edu.cn> To: ja@ssi.bg Cc: davem@davemloft.net, dsa@cumulusnetworks.com, nicolas.dichtel@6wind.com, roopa@cumulusnetworks.com, rshearma@brocade.com, dbarroso@fastly.com, martinbj2008@gmail.com, rick.jones2@hp.com, koct9i@gmail.com, edumazet@google.com, tgraf@suug.ch, ebiederm@xmission.com, yoshfuji@linux-ipv6.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] net: neigh: disallow state transition DELAY->STALE in neigh_update() From: Chunhui He In-Reply-To: References: <1469123890-2715-1-git-send-email-hchunhui@mail.ustc.edu.cn> X-Mailer: Mew version 6.6 on Emacs 24.4 / Mule 6.0 (HANACHIRUSATO) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-CM-TRANSID: LkAmygAngP8EFpJX2JbFBg--.4S2 X-Coremail-Antispam: 1UD129KBjvJXoWxZry5XFyfKw48AF1DZw1UKFg_yoWrZw4kpa yDCFsxKay5Jrs5Zws3Xa1UuF1jkw17AF15uF9Yk3y2kFnYqrW5tFn7Kw15GFyDGFWjkrWI kF42grWUuFWqva7anT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvvb7Iv0xC_KF4lb4IE77IF4wAFF20E14v26r4j6ryUM7CY07I2 0VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rw A2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Ar0_tr1l84ACjcxK6xII jxv20xvEc7CjxVAFwI0_Cr0_Gr1UM28EF7xvwVC2z280aVAFwI0_GcCE3s1l84ACjcxK6I 8E87Iv6xkF7I0E14v26rxl6s0DM2AIxVAIcxkEcVAq07x20xvEncxIr21l5I8CrVACY4xI 64kE6c02F40Ex7xfMcIj6xIIjxv20xvE14v26r1j6r18McIj6I8E87Iv67AKxVWUJVW8Jw Am72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IYc2Ij64vIr41lFIxGxcIEc7CjxVA2Y2ka0xkI wI1lc2xSY4AK67AK6ryUMxAIw28IcxkI7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI 8I3I0E5I8CrVAFwI0_Jr0_Jr4lx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AK xVW8ZVWrXwCIc40Y0x0EwIxGrwCI42IY6xIIjxv20xvE14v26r1j6r1xMIIF0xvE2Ix0cI 8IcVCY1x0267AKxVW8JVWxJwCI42IY6xAIw20EY4v20xvaj40_WFyUJVCq3wCI42IY6I8E 87Iv67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r4j6r4UJbIYCTnIWIevJa73Uj IFyTuYvjxUcWE_DUUUU X-CM-SenderInfo: pkfk30xkxlqzxdloh3xvwfhvlgxou0/ Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Fri, 22 Jul 2016 10:20:01 +0300 (EEST), Julian Anastasov wrote: > > Hello, > > On Thu, 21 Jul 2016, Chunhui He wrote: > >> If neigh entry was CONNECTED and address is not changed, and if new state is >> STALE, entry state will not change. Because DELAY is not in CONNECTED, it's >> possible to change state from DELAY to STALE. >> >> That is bad. Consider a host in IPv4 nerwork, a neigh entry in STALE state >> is referenced to send packets, so goes to DELAY state. If the entry is not >> confirmed by upper layer, it goes to PROBE state, and sends ARP request. >> The neigh host sends ARP reply, then the entry goes to REACHABLE state. >> But the entry state may be reseted to STALE by broadcast ARP packets, before >> the entry goes to PROBE state. So it's possible that the entry will never go >> to REACHABLE state, without external confirmation. >> >> In my case, the gateway refuses to send unicast packets to me, before it sees >> my ARP request. So it's critical to enter REACHABLE state by sending ARP >> request, but not by external confirmation. >> >> This fixes neigh_update() not to change to STALE if old state is CONNECTED or >> DELAY. >> >> Signed-off-by: Chunhui He >> --- >> net/core/neighbour.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/core/neighbour.c b/net/core/neighbour.c >> index 510cd62..29429eb 100644 >> --- a/net/core/neighbour.c >> +++ b/net/core/neighbour.c >> @@ -1152,7 +1152,7 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new, >> } else { >> if (lladdr == neigh->ha && new == NUD_STALE && >> ((flags & NEIGH_UPDATE_F_WEAK_OVERRIDE) || >> - (old & NUD_CONNECTED)) >> + (old & (NUD_CONNECTED | NUD_DELAY))) >> ) >> new = old; >> } > > You change looks correct to me. But this place > has more problems. There is no good reason to set NUD_STALE > for any state that is NUD_VALID if address is not changed. > This matches perfectly the comment above this code: > NUD_STALE should change a NUD_VALID state only when > address changes. It also means that IPv6 does not need > to provide NEIGH_UPDATE_F_WEAK_OVERRIDE anymore when > NEIGH_UPDATE_F_OVERRIDE is also present. > The NEIGH_UPDATE_F_WEAK_OVERRIDE is confusing to me, so I choose not to deal with the flag. > By this way the state machine can continue with > the resolving: NUD_STALE -> NUD_DELAY (traffic) -> > NUD_PROBE (retries) -> NUD_REACHABLE (unicast reply) > while the address is not changed. Your change covers only > NUD_DELAY, not NUD_PROBE, so it is better to allow more > retries to send. We should not give up until success (NUD_REACHABLE). > I have thought about this. The origin code allows NUD_DELAY -> NUD_STALE and NUD_PROBE -> NUD_STALE. This part was imported to kernel since v2.1.79, I don't know clearly why it allows that. My analysis: (1) As shown in my previous mail, NUD_DELAY -> NUD_STALE may cause "dead loop", so it should be fixed. (2) But NUD_PROBE -> NUD_STALE is acceptable, because in NUD_PROBE, ARP request has been sent, it is sufficient to break the "dead loop". More attempts are accomplished by the following sequence: NUD_STALE --> NUD_DELAY -(sent req)-> NUD_PROBE -(reset by neigh_update())-> NUD_STALE --> NUD_DELAY -(send req again)-> ... --> NUD_REACHABLE But I also agree your change. > Second problem: NEIGH_UPDATE_F_WEAK_OVERRIDE has no > priority over NEIGH_UPDATE_F_ADMIN. For example, now I can not > change from NUD_PERMANENT to NUD_STALE: > > # ip neigh add 192.168.168.111 lladdr 00:11:22:33:44:55 nud perm dev wlan0 > # ip neigh show to 192.168.168.111 > 192.168.168.111 dev wlan0 lladdr 00:11:22:33:44:55 PERMANENT > # ip neigh change 192.168.168.111 lladdr 00:11:22:33:44:55 nud stale dev wlan0 > # ip neigh show to 192.168.168.111 > 192.168.168.111 dev wlan0 lladdr 00:11:22:33:44:55 PERMANENT > > IMHO, here is how this place should look: > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c > index 5cdc62a..2b1cb91 100644 > --- a/net/core/neighbour.c > +++ b/net/core/neighbour.c > @@ -1151,10 +1151,8 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new, > goto out; > } else { > if (lladdr == neigh->ha && new == NUD_STALE && > - ((flags & NEIGH_UPDATE_F_WEAK_OVERRIDE) || > - (old & NUD_CONNECTED)) > - ) > - new = old; > + !(flags & NEIGH_UPDATE_F_ADMIN)) > + goto out; > } > } > > Any thoughts? > > Regards > > -- > Julian Anastasov Regards, Chunhui He