Hi,
On Mon, 25 Jul 2016 14:20:29 +0900, YOSHIFUJI Hideaki/吉藤英明
<hideaki.yoshif...@miraclelinux.com> wrote:
> Hi,
>
> Chunhui He wrote:
>> Hi,
>>
>> On Fri, 22 Jul 2016 10:20:01 +0300 (EEST), Julian Anastasov <j...@ssi.bg>
>> 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 <hchun...@mail.ustc.edu.cn>
>>>> ---
>>>> 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.
>
> IPv6 depends on WEAK_OVERRIDE. Please do not change.
>
It seems like IPv6 always sets WEAK_OVERRIDE.
As Julian said, maybe there is no good reason to set NUD_STALE for any state
that is NUD_VALID if address is not changed, even WEAK_OVERRIDE is not set.
So we may eliminate WEAK_OVERRIDE in that branch.
I think this change should not break IPv6.
>>
>>> 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 <j...@ssi.bg>
>>
>> Regards,
>> Chunhui He
>>
>
> --
> 吉藤英明 <hideaki.yoshif...@miraclelinux.com>
> ミラクル・リナックス株式会社 技術本部 サポート部
Regards,
Chunhui
