* [PATCH] inetpeer: ensure to set the maximum tokens the first time
@ 2012-09-27 12:33 Nicolas Dichtel
2012-09-27 12:53 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Dichtel @ 2012-09-27 12:33 UTC (permalink / raw)
To: netdev, davem; +Cc: Nicolas Dichtel
When jiffies wraps around (for example, 5 minutes after the boot, see
INITIAL_JIFFIES) and peer has just been created, now - peer->rate_last can be
< XRLIM_BURST_FACTOR * timeout, so token is not set to the maximum value, thus
some icmp packets can be unexpectedly dropped.
With this patch, it's still possible that last_rate and rate_tokens are 0 at the
same time after jiffies wraps round, but the probability is very low and the
only consequence is to let some ICMP packets bypass the filter.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/ipv4/inetpeer.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index e1e0a4e..92fec02 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -559,10 +559,14 @@ bool inet_peer_xrlim_allow(struct inet_peer *peer, int timeout)
token = peer->rate_tokens;
now = jiffies;
- token += now - peer->rate_last;
- peer->rate_last = now;
- if (token > XRLIM_BURST_FACTOR * timeout)
+ if (!peer->rate_last && !token)
token = XRLIM_BURST_FACTOR * timeout;
+ else {
+ token += now - peer->rate_last;
+ if (token > XRLIM_BURST_FACTOR * timeout)
+ token = XRLIM_BURST_FACTOR * timeout;
+ }
+ peer->rate_last = now;
if (token >= timeout) {
token -= timeout;
rc = true;
--
1.7.12
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] inetpeer: ensure to set the maximum tokens the first time
2012-09-27 12:33 [PATCH] inetpeer: ensure to set the maximum tokens the first time Nicolas Dichtel
@ 2012-09-27 12:53 ` Eric Dumazet
2012-09-27 13:21 ` Nicolas Dichtel
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2012-09-27 12:53 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: netdev, davem
On Thu, 2012-09-27 at 14:33 +0200, Nicolas Dichtel wrote:
> When jiffies wraps around (for example, 5 minutes after the boot, see
> INITIAL_JIFFIES) and peer has just been created, now - peer->rate_last can be
> < XRLIM_BURST_FACTOR * timeout, so token is not set to the maximum value, thus
> some icmp packets can be unexpectedly dropped.
>
> With this patch, it's still possible that last_rate and rate_tokens are 0 at the
> same time after jiffies wraps round, but the probability is very low and the
> only consequence is to let some ICMP packets bypass the filter.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> net/ipv4/inetpeer.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
> index e1e0a4e..92fec02 100644
> --- a/net/ipv4/inetpeer.c
> +++ b/net/ipv4/inetpeer.c
> @@ -559,10 +559,14 @@ bool inet_peer_xrlim_allow(struct inet_peer *peer, int timeout)
>
> token = peer->rate_tokens;
> now = jiffies;
> - token += now - peer->rate_last;
> - peer->rate_last = now;
> - if (token > XRLIM_BURST_FACTOR * timeout)
> + if (!peer->rate_last && !token)
> token = XRLIM_BURST_FACTOR * timeout;
> + else {
> + token += now - peer->rate_last;
> + if (token > XRLIM_BURST_FACTOR * timeout)
> + token = XRLIM_BURST_FACTOR * timeout;
> + }
> + peer->rate_last = now;
> if (token >= timeout) {
> token -= timeout;
> rc = true;
I am sorry I dont understand your patch at all.
Why not init rate_last to a more sensible value ?
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index e1e0a4e..25ed555 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -510,7 +510,7 @@ relookup:
secure_ipv6_id(daddr->addr.a6));
p->metrics[RTAX_LOCK-1] = INETPEER_METRICS_NEW;
p->rate_tokens = 0;
- p->rate_last = 0;
+ p->rate_last = jiffies;
INIT_LIST_HEAD(&p->gc_list);
/* Link the node. */
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] inetpeer: ensure to set the maximum tokens the first time
2012-09-27 12:53 ` Eric Dumazet
@ 2012-09-27 13:21 ` Nicolas Dichtel
2012-09-27 13:30 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Dichtel @ 2012-09-27 13:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem
Le 27/09/2012 14:53, Eric Dumazet a écrit :
> On Thu, 2012-09-27 at 14:33 +0200, Nicolas Dichtel wrote:
>> When jiffies wraps around (for example, 5 minutes after the boot, see
>> INITIAL_JIFFIES) and peer has just been created, now - peer->rate_last can be
>> < XRLIM_BURST_FACTOR * timeout, so token is not set to the maximum value, thus
>> some icmp packets can be unexpectedly dropped.
>>
>> With this patch, it's still possible that last_rate and rate_tokens are 0 at the
>> same time after jiffies wraps round, but the probability is very low and the
>> only consequence is to let some ICMP packets bypass the filter.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>> net/ipv4/inetpeer.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
>> index e1e0a4e..92fec02 100644
>> --- a/net/ipv4/inetpeer.c
>> +++ b/net/ipv4/inetpeer.c
>> @@ -559,10 +559,14 @@ bool inet_peer_xrlim_allow(struct inet_peer *peer, int timeout)
>>
>> token = peer->rate_tokens;
>> now = jiffies;
>> - token += now - peer->rate_last;
>> - peer->rate_last = now;
>> - if (token > XRLIM_BURST_FACTOR * timeout)
>> + if (!peer->rate_last && !token)
>> token = XRLIM_BURST_FACTOR * timeout;
>> + else {
>> + token += now - peer->rate_last;
>> + if (token > XRLIM_BURST_FACTOR * timeout)
>> + token = XRLIM_BURST_FACTOR * timeout;
>> + }
>> + peer->rate_last = now;
>> if (token >= timeout) {
>> token -= timeout;
>> rc = true;
>
>
> I am sorry I dont understand your patch at all.
>
> Why not init rate_last to a more sensible value ?
>
> diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
> index e1e0a4e..25ed555 100644
> --- a/net/ipv4/inetpeer.c
> +++ b/net/ipv4/inetpeer.c
> @@ -510,7 +510,7 @@ relookup:
> secure_ipv6_id(daddr->addr.a6));
> p->metrics[RTAX_LOCK-1] = INETPEER_METRICS_NEW;
> p->rate_tokens = 0;
> - p->rate_last = 0;
> + p->rate_last = jiffies;
inet_getpeer(...,1) is called just before inet_peer_xrlim_allow().
So the result in inet_peer_xrlim_allow():
token = peer->rate_tokens; => 0
now = jiffies;
token += now - peer->rate_last; => token += jiffies - jiffies => 0
So we have no token and packet is dropped.
Am I wrong?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] inetpeer: ensure to set the maximum tokens the first time
2012-09-27 13:21 ` Nicolas Dichtel
@ 2012-09-27 13:30 ` Eric Dumazet
2012-09-27 13:34 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2012-09-27 13:30 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev, davem
On Thu, 2012-09-27 at 15:21 +0200, Nicolas Dichtel wrote:
> Le 27/09/2012 14:53, Eric Dumazet a écrit :
> > On Thu, 2012-09-27 at 14:33 +0200, Nicolas Dichtel wrote:
> >> When jiffies wraps around (for example, 5 minutes after the boot, see
> >> INITIAL_JIFFIES) and peer has just been created, now - peer->rate_last can be
> >> < XRLIM_BURST_FACTOR * timeout, so token is not set to the maximum value, thus
> >> some icmp packets can be unexpectedly dropped.
> >>
> >> With this patch, it's still possible that last_rate and rate_tokens are 0 at the
> >> same time after jiffies wraps round, but the probability is very low and the
> >> only consequence is to let some ICMP packets bypass the filter.
> >>
> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> >> ---
> >> net/ipv4/inetpeer.c | 10 +++++++---
> >> 1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
> >> index e1e0a4e..92fec02 100644
> >> --- a/net/ipv4/inetpeer.c
> >> +++ b/net/ipv4/inetpeer.c
> >> @@ -559,10 +559,14 @@ bool inet_peer_xrlim_allow(struct inet_peer *peer, int timeout)
> >>
> >> token = peer->rate_tokens;
> >> now = jiffies;
> >> - token += now - peer->rate_last;
> >> - peer->rate_last = now;
> >> - if (token > XRLIM_BURST_FACTOR * timeout)
> >> + if (!peer->rate_last && !token)
> >> token = XRLIM_BURST_FACTOR * timeout;
> >> + else {
> >> + token += now - peer->rate_last;
> >> + if (token > XRLIM_BURST_FACTOR * timeout)
> >> + token = XRLIM_BURST_FACTOR * timeout;
> >> + }
> >> + peer->rate_last = now;
> >> if (token >= timeout) {
> >> token -= timeout;
> >> rc = true;
> >
> >
> > I am sorry I dont understand your patch at all.
> >
> > Why not init rate_last to a more sensible value ?
> >
> > diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
> > index e1e0a4e..25ed555 100644
> > --- a/net/ipv4/inetpeer.c
> > +++ b/net/ipv4/inetpeer.c
> > @@ -510,7 +510,7 @@ relookup:
> > secure_ipv6_id(daddr->addr.a6));
> > p->metrics[RTAX_LOCK-1] = INETPEER_METRICS_NEW;
> > p->rate_tokens = 0;
> > - p->rate_last = 0;
> > + p->rate_last = jiffies;
> inet_getpeer(...,1) is called just before inet_peer_xrlim_allow().
> So the result in inet_peer_xrlim_allow():
> token = peer->rate_tokens; => 0
> now = jiffies;
> token += now - peer->rate_last; => token += jiffies - jiffies => 0
> So we have no token and packet is dropped.
>
> Am I wrong?
So find the right initializer ?
p->rate_last = jiffies;
p->rate_tokens = TOKENS_INIT;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] inetpeer: ensure to set the maximum tokens the first time
2012-09-27 13:30 ` Eric Dumazet
@ 2012-09-27 13:34 ` Eric Dumazet
2012-09-27 13:39 ` Nicolas Dichtel
2012-09-27 14:11 ` [PATCH v2] inetpeer: fix token initialization Nicolas Dichtel
0 siblings, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2012-09-27 13:34 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev, davem
On Thu, 2012-09-27 at 15:30 +0200, Eric Dumazet wrote:
> So find the right initializer ?
>
> p->rate_last = jiffies;
> p->rate_tokens = TOKENS_INIT;
>
>
Or :
p->rate_last = jiffies - 60*HZ;
since the rates_tokens initial value is protocol/family dependent
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] inetpeer: ensure to set the maximum tokens the first time
2012-09-27 13:34 ` Eric Dumazet
@ 2012-09-27 13:39 ` Nicolas Dichtel
2012-09-27 14:11 ` [PATCH v2] inetpeer: fix token initialization Nicolas Dichtel
1 sibling, 0 replies; 9+ messages in thread
From: Nicolas Dichtel @ 2012-09-27 13:39 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem
Le 27/09/2012 15:34, Eric Dumazet a écrit :
> On Thu, 2012-09-27 at 15:30 +0200, Eric Dumazet wrote:
>
>> So find the right initializer ?
>>
>> p->rate_last = jiffies;
>> p->rate_tokens = TOKENS_INIT;
>>
>>
>
> Or :
>
> p->rate_last = jiffies - 60*HZ;
>
> since the rates_tokens initial value is protocol/family dependent
Yes, this will avoid to pass the timeout arg to inet_getpeer()
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] inetpeer: fix token initialization
2012-09-27 13:34 ` Eric Dumazet
2012-09-27 13:39 ` Nicolas Dichtel
@ 2012-09-27 14:11 ` Nicolas Dichtel
2012-09-27 14:18 ` Eric Dumazet
1 sibling, 1 reply; 9+ messages in thread
From: Nicolas Dichtel @ 2012-09-27 14:11 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, davem, Nicolas Dichtel
When jiffies wraps around (for example, 5 minutes after the boot, see
INITIAL_JIFFIES) and peer has just been created, now - peer->rate_last can be
< XRLIM_BURST_FACTOR * timeout, so token is not set to the maximum value, thus
some icmp packets can be unexpectedly dropped.
Fix this case by initializing last_rate to 60 seconds in the past.
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
v2: fix initialization of peer instead of adding some tests in
inet_peer_xrlim_allow()
net/ipv4/inetpeer.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index e1e0a4e..c7527f6 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -510,7 +510,10 @@ relookup:
secure_ipv6_id(daddr->addr.a6));
p->metrics[RTAX_LOCK-1] = INETPEER_METRICS_NEW;
p->rate_tokens = 0;
- p->rate_last = 0;
+ /* 60*HZ is arbitrary, but chosen enough high so that the first
+ * calculation of tokens is at its maximum.
+ */
+ p->rate_last = jiffies - 60*HZ;
INIT_LIST_HEAD(&p->gc_list);
/* Link the node. */
--
1.7.12
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] inetpeer: fix token initialization
2012-09-27 14:11 ` [PATCH v2] inetpeer: fix token initialization Nicolas Dichtel
@ 2012-09-27 14:18 ` Eric Dumazet
2012-09-27 23:28 ` David Miller
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2012-09-27 14:18 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: netdev, davem
On Thu, 2012-09-27 at 16:11 +0200, Nicolas Dichtel wrote:
> When jiffies wraps around (for example, 5 minutes after the boot, see
> INITIAL_JIFFIES) and peer has just been created, now - peer->rate_last can be
> < XRLIM_BURST_FACTOR * timeout, so token is not set to the maximum value, thus
> some icmp packets can be unexpectedly dropped.
>
> Fix this case by initializing last_rate to 60 seconds in the past.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> v2: fix initialization of peer instead of adding some tests in
> inet_peer_xrlim_allow()
>
> net/ipv4/inetpeer.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
> index e1e0a4e..c7527f6 100644
> --- a/net/ipv4/inetpeer.c
> +++ b/net/ipv4/inetpeer.c
> @@ -510,7 +510,10 @@ relookup:
> secure_ipv6_id(daddr->addr.a6));
> p->metrics[RTAX_LOCK-1] = INETPEER_METRICS_NEW;
> p->rate_tokens = 0;
> - p->rate_last = 0;
> + /* 60*HZ is arbitrary, but chosen enough high so that the first
> + * calculation of tokens is at its maximum.
> + */
> + p->rate_last = jiffies - 60*HZ;
> INIT_LIST_HEAD(&p->gc_list);
>
> /* Link the node. */
Signed-off-by: Eric Dumazet <edumazet@google.com>
Thanks !
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] inetpeer: fix token initialization
2012-09-27 14:18 ` Eric Dumazet
@ 2012-09-27 23:28 ` David Miller
0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2012-09-27 23:28 UTC (permalink / raw)
To: eric.dumazet; +Cc: nicolas.dichtel, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 27 Sep 2012 16:18:36 +0200
> On Thu, 2012-09-27 at 16:11 +0200, Nicolas Dichtel wrote:
>> When jiffies wraps around (for example, 5 minutes after the boot, see
>> INITIAL_JIFFIES) and peer has just been created, now - peer->rate_last can be
>> < XRLIM_BURST_FACTOR * timeout, so token is not set to the maximum value, thus
>> some icmp packets can be unexpectedly dropped.
>>
>> Fix this case by initializing last_rate to 60 seconds in the past.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>> v2: fix initialization of peer instead of adding some tests in
>> inet_peer_xrlim_allow()
...
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, thanks everyone.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-09-27 23:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-27 12:33 [PATCH] inetpeer: ensure to set the maximum tokens the first time Nicolas Dichtel
2012-09-27 12:53 ` Eric Dumazet
2012-09-27 13:21 ` Nicolas Dichtel
2012-09-27 13:30 ` Eric Dumazet
2012-09-27 13:34 ` Eric Dumazet
2012-09-27 13:39 ` Nicolas Dichtel
2012-09-27 14:11 ` [PATCH v2] inetpeer: fix token initialization Nicolas Dichtel
2012-09-27 14:18 ` Eric Dumazet
2012-09-27 23:28 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).