| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <55073522.6010409@emagii.com>
Date: Mon, 16 Mar 2015 20:55:14 +0100
From: Ulf Samuelsson <ulf@...gii.com>
To: YOSHIFUJI Hideaki/吉藤英明
<hideaki.yoshifuji@...aclelinux.com>,
Ulf Samuelsson <netdev@...gii.com>, netdev@...r.kernel.org
CC: YOSHIFUJI Hideaki <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH] neighbour.c: Avoid GC directly after state change
Den 2015-03-16 05:57, YOSHIFUJI Hideaki/吉藤英明 skrev:
> Hello.
>
> Ulf Samuelsson wrote:
>> Den 2015-03-15 09:27, YOSHIFUJI Hideaki skrev:
>>> Hello.
>>>
>>> Ulf Samuelsson wrote:
>>>> From: Ulf Samuelsson <ulf@...gii.com>
>>>>
>>>> The neighbour state is changed in the ARP timer handler.
>>>> If the state is changed to NUD_STALE, then the neighbour
>>>> entry becomes a candidate for garbage collection.
>>>>
>>>> The garbage collection is handled by a "periodic work" routine.
>>>>
>>>> When :
>>>>
>>>> * noone refers to the entry
>>>> * the state is no longer valid (I.E: NUD_STALE).
>>> NUD_STALE is still valid.
>> Yes, my fault.
>> The condition which causes garbage collection to be skipped is.
>>
>>
>> if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {
>>
>> NUD_STALE is not part of that, so GC will not be skipped,
>> and therefore the patch is needed if you want to be able
>> to use the API to modify the neigh statemachine..
>>>
>>>> * the timeout value has been reached or state is FAILED
>>>>
>>>> the "periodic work" routine will notify
>>>> the stack that the entry should be deleted.
>>>>
>>>> A user application monitoring and controlling the neighbour table
>>>> using NETLINK may fail, if the "period work" routine is run
>>>> directly after the state has been changed to NUD_STALE,
>>>> but before the user application has had a chance to change
>>>> the state to something valid.
>>>>
>>>> The "period work" routine will detect the NUD_STALE state
>>>> and if the timeout value has been reached, it will notify the stack
>>>> that the entry should be deleted.
>>>>
>>>> The patch adds a check in the periodic work routine
>>>> which will skip test for garbage collection
>>>> unless a number of ticks has passed since the last time
>>>> the neighbour entry state was changed.
>>>>
>>>> The feature is controlled through Kconfig
>>>>
>>>> The featuree is enabled by setting ARP_GC_APPLY_GUARDBAND
>>>> The guardband time (in ticks) is set in ARP_GC_GUARDBAND
>>>> Default time is 100 ms if HZ_### is set.
>>> We have "lower limit" not to start releasing neighbour entries.
>>> Try increasing gc_thresh1.
>> Why would that work?
>>
>> The only place where this is used is
>>
>> "if (atomic_read(&tbl->entries) < tbl->gc_thresh1)"
>>
>> tbl->entries is related to how many entries there are in the
>> neighbour table.
>>
>> The only way I think this would work, is if this is raised so high that
>> garbage collection does not occur.
>>
>> That is not the intention.
>>
>> It does not solve the race condition between the timer_handler and
>> the periodic_work.
>
> I don't think it is a race.
And I think you are wrong and my logging shows that it is.
>
> You can try increasing gc_staletime to hold each entry based
> on last usage. Plus, you can "confirm" neighbors by
> MSG_CONFIRM.
>
> Note that if the number of entries becomes high, "forced GC" will
> drop valid, "not connected" entries as well.
I can try increasing gc_staletime, but its a waste of time, because it
is not the last usage which is interesting.
What is interesting is the time when the entry state was updated by the
timer handler.
Pls explain further MSG_CONFIRM.
The problem occurs when the periodic_work routine runs immediately
after the timer handler has changes the state to NUD_STALE and
the entry has reached the expiry time.
I suggest you read the code, and you will understand what I mean.
Ulf
>
> --yoshfuji
>
>>
>> BR
>> Ulf Samuelsson
>>
>>>
>>> --yoshfuji
>>>
>>>> Signed-off-by: Ulf Samuelsson <ulf@...gii.com>
>>>> ---
>>>> net/Kconfig | 32 ++++++++++++++++++++++++++++++++
>>>> net/core/neighbour.c | 15 ++++++++++++---
>>>> 2 files changed, 44 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/Kconfig b/net/Kconfig
>>>> index 44dd578..099a5dd 100644
>>>> --- a/net/Kconfig
>>>> +++ b/net/Kconfig
>>>> @@ -77,6 +77,38 @@ config INET
>>>> Short answer: say Y.
>>>> if INET
>>>> +
>>>> +#
>>>> +# Core Network configuration
>>>> +#
>>>> +
>>>> +config ARP_GC_APPLY_GUARDBAND
>>>> + bool "IP: ARP: Avoid garbage collection directly after state
>>>> change"
>>>> + default n
>>>> + ---help---
>>>> + With this item selected, an entry in the neighbour table
>>>> + will not be garbage collected directly after the ARP state
>>>> + has changed to STALE of FAILED
>>>> + This allows an application program change the state to
>>>> something valid
>>>> + before garbage colllection occurs.
>>>> +
>>>> + If unsure, say N.
>>>> +
>>>> +config ARP_GC_GUARDBAND
>>>> + int "Guardband time on garbage collection"
>>>> + depends on ARP_GC_APPLY_GUARDBAND
>>>> + default 10 if HZ_100
>>>> + default 25 if HZ_250
>>>> + default 30 if HZ_300
>>>> + default 100 if HZ_1000
>>>> + default 100
>>>> +
>>>> + ---help---
>>>> + The number of ticks to delay garbage collection
>>>> + after the neighbour entry has been updated
>>>> + A delay of 100 ms is reasonable.
>>>> + With CONFIG_HZ = 250, this value should be 25
>>>> +
>>>> source "net/ipv4/Kconfig"
>>>> source "net/ipv6/Kconfig"
>>>> source "net/netlabel/Kconfig"
>>>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>>>> index 70fe9e1..194195d 100644
>>>> --- a/net/core/neighbour.c
>>>> +++ b/net/core/neighbour.c
>>>> @@ -786,13 +786,23 @@ static void neigh_periodic_work(struct
>>>> work_struct *work)
>>>> state = n->nud_state;
>>>> if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {
>>>> - write_unlock(&n->lock);
>>>> goto next_elt;
>>>> }
>>>> if (time_before(n->used, n->confirmed))
>>>> n->used = n->confirmed;
>>>> +#if defined(CONFIG_ARP_GC_APPLY_GUARDBAND)
>>>> + /* Do not garbage collect directly after we
>>>> + * updated n->state to allow applications to
>>>> + * react to the event
>>>> + */
>>>> + if (time_before(jiffies,
>>>> + n->updated + CONFIG_ARP_GC_GUARDBAND)) {
>>>> + goto next_elt;
>>>> + }
>>>> +#endif
>>>> +
>>>> if (atomic_read(&n->refcnt) == 1 &&
>>>> (state == NUD_FAILED ||
>>>> time_after(jiffies, n->used +
>>>> NEIGH_VAR(n->parms, GC_STALETIME)))) {
>>>> @@ -802,9 +812,8 @@ static void neigh_periodic_work(struct
>>>> work_struct *work)
>>>> neigh_cleanup_and_release(n);
>>>> continue;
>>>> }
>>>> - write_unlock(&n->lock);
>>>> -
>>>> next_elt:
>>>> + write_unlock(&n->lock);
>>>> np = &n->next;
>>>> }
>>>> /*
>>>>
>>
>>
>
--
Best Regards
Ulf Samuelsson
ulf@...gii.com
+46 722 427437
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists