Re: [PATCH] Limit req: r/h and r/d support

[Maxim Dounin]

Hello!

On Sat, Feb 17, 2018 at 10:07:27PM +0100, Bernhard Reutner-Fischer wrote:

> On 16 February 2018 at 17:52, Maxim Dounin  wrote:
> 
> >> Limit req: r/h and r/d support
> 
> > Do you use it yourself, or your patch is based on the feature
> > requests in question?  If yes, what is your use case?
> 
> I have to limit certain use-cases to a handful of requests per day (don't 
> ask).
> Mentioned feature-requests just show that this oddish corner case is a
> real knock-out criterion in the real world so i mentioned them to
> emphasis that it wasn't just /me but that you're losing users due to
> this. Often this is a hard requirement. You don't seem to offer any
> sensible, off the shelve (or documented) way to handle "long" request
> limits anyway -- think shm -- at least none that i was able to find?

The limit_req module was created mostly to limit high rates - in 
particular, to limit requests to costly services to fight DoS, and 
to limit requests to password checking to fight bruteforce 
attempts.  It was never meant to limit normal user activity, and 
hence the data format choosen does not support rates less than 
1r/m.  Yet it works effectively with high rates the module was 
created for.

While it is understood that some people might want to use longer 
limits, it's not something the module tries to provide now.  Also, 
it might not actually be a good idea to provide such limits in the 
module, as any nginx restart / upgrade will mean that limits won't 
work as expected for significant time.  If you want to limits 
users to several requests per day, it might be a better idea to 
keep these limits outside of nginx.

> > Style: please add a trailing dot.
> 
> Sure, please excuse my sloppy reading of previous logs, i honestly
> failed to notice that, added.
> 
> > On 32-bit platforms this means that the maximum supported rate
> > would be about ~40kr/s.  And I suspect this can hurt real setups.
> 
> agreed. Not sure about current typical HTTP server rates, admittedly.
> Getting esoteric, but current servers usually satisfy millions of
> requests but i'm not sure if you'd usually limit those to let's say
> 100k/s, but of course i see your point.

I doubt rates like 100kr/s are needed in practice (especially on 
32-bit platforms), but I'm pretty sure that configurations with 
something like this are real.  In particular, googling "limit_req 
rate site:nginx.org/pipermail/" shows examples like 
"rate=99r/s" pretty quickly.

[...]

> > Note that I have no good solution for 32-bit platforms and the
> > 40kr/s (20kr/s) limit.
> 
> my notes read:
> disentangle rate. Should have rate(per unit) and unit(in seconds?
> minutes? calc!) where excess would have same base as rate; common
> timer conversion helper for rate/unit?
> 
> This has much more impact and hence is a much larger patch but would
> certainly be way easier to grok compared to the current handling.
> Would you be willing to fix the existing rate mess^whandling?

Current handling is pretty clear and simple: all calculations are 
done in requests per millisecond, and this requires just one value 
in various structures.  We can tolerate switching to something 
different, though it would be a good idea to keep code logic close 
to the current implementation.

-- 
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH] Limit req: r/h and r/d support

[Bernhard Reutner-Fischer]

On 16 February 2018 at 17:52, Maxim Dounin  wrote:

>> Limit req: r/h and r/d support

> Do you use it yourself, or your patch is based on the feature
> requests in question?  If yes, what is your use case?

I have to limit certain use-cases to a handful of requests per day (don't ask).
Mentioned feature-requests just show that this oddish corner case is a
real knock-out criterion in the real world so i mentioned them to
emphasis that it wasn't just /me but that you're losing users due to
this. Often this is a hard requirement. You don't seem to offer any
sensible, off the shelve (or documented) way to handle "long" request
limits anyway -- think shm -- at least none that i was able to find?

> Style: please add a trailing dot.

Sure, please excuse my sloppy reading of previous logs, i honestly
failed to notice that, added.

> On 32-bit platforms this means that the maximum supported rate
> would be about ~40kr/s.  And I suspect this can hurt real setups.

agreed. Not sure about current typical HTTP server rates, admittedly.
Getting esoteric, but current servers usually satisfy millions of
requests but i'm not sure if you'd usually limit those to let's say
100k/s, but of course i see your point.
[]
> The ctx->rate is unsigned, and the ctx->rate check will only fail
> if ctx->rate is 0.  While this might happen in some cases, it
> certainly won't catch all overflows (which are quite likely in
> real configurations on 32-bit platforms, see above).
>
> Also, the "rate" type is ngx_int_t, so (rate * 10) will cause
> integer overflow, which is not defined.  It might be a better idea
> to avoid overflows at all.

yea, see below.
>
> Also, the "rate" type is ngx_int_t, not ngx_uint_t, so proper
> format would be "%i".

right.

> In general, it might be a better idea to move the check to
> ngx_atoi(), and test something like NGX_MAX_INT_T_VALUE / 10.
> This will ensure no overflow can happen during calculations below,
> and will allow logging of the original value.  This will further
> limit maximum rate to about 20kr/s on 32-bit platforms though.
>
> Note that I have no good solution for 32-bit platforms and the
> 40kr/s (20kr/s) limit.

my notes read:
disentangle rate. Should have rate(per unit) and unit(in seconds?
minutes? calc!) where excess would have same base as rate; common
timer conversion helper for rate/unit?

This has much more impact and hence is a much larger patch but would
certainly be way easier to grok compared to the current handling.
Would you be willing to fix the existing rate mess^whandling?

cheers,
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH] Limit req: r/h and r/d support

[Maxim Dounin]

Hello!

On Fri, Feb 16, 2018 at 03:57:05PM +0100, Bernhard Reutner-Fischer wrote:

> Attached is a patch to add r/h and r/d limit_req support.
> 
> There are at least two feature requests that i found asking for this:
> - https://trac.nginx.org/nginx/ticket/68
> - https://trac.nginx.org/nginx/ticket/1060
> 
> Ok?
> 
> Please consider applying.

Do you use it yourself, or your patch is based on the feature 
requests in question?  If yes, what is your use case?

Some obvious problems with the patch outlined below.

> # HG changeset patch
> # User Bernhard Reutner-Fischer 
> # Date 1518790589 -3600
> #  Fri Feb 16 15:16:29 2018 +0100
> # Node ID 4f5981016b6d3b4b9745f90ddf9521fa7a0d375f
> # Parent  a49af443656f2b65ca5de9d8cad5594f44e18ff7
> Limit req: r/h and r/d support

Style: please add a trailing dot.

> 
> diff -r a49af443656f -r 4f5981016b6d 
> src/http/modules/ngx_http_limit_req_module.c
> --- a/src/http/modules/ngx_http_limit_req_module.cThu Feb 08 12:11:30 
> 2018 +0300
> +++ b/src/http/modules/ngx_http_limit_req_module.cFri Feb 16 15:16:29 
> 2018 +0100
> @@ -16,7 +16,7 @@
>  u_short  len;
>  ngx_queue_t  queue;
>  ngx_msec_t   last;
> -/* integer value, 1 corresponds to 0.001 r/s */
> +/* integer value, 1 corresponds to 0.1 r/s */
>  ngx_uint_t   excess;
>  ngx_uint_t   count;
>  u_char   data[1];
> @@ -33,7 +33,7 @@
>  typedef struct {
>  ngx_http_limit_req_shctx_t  *sh;
>  ngx_slab_pool_t *shpool;
> -/* integer value, 1 corresponds to 0.001 r/s */
> +/* integer value, 1 corresponds to 0.1 r/s */
>  ngx_uint_t   rate;

On 32-bit platforms this means that the maximum supported rate 
would be about ~40kr/s.  And I suspect this can hurt real setups.

[...]

> @@ -213,7 +213,7 @@
>  
>  ngx_log_debug4(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> "limit_req[%ui]: %i %ui.%03ui",
> -   n, rc, excess / 1000, excess % 1000);
> +   n, rc, excess / 10, excess % 10);

The "%ui.%03ui" format needs to be adjusted as well, here and in 
all other places.

[...]

> @@ -398,8 +398,9 @@
>  ngx_queue_insert_head(>sh->queue, >queue);
>  
>  ms = (ngx_msec_int_t) (now - lr->last);
> +ms = ngx_abs(ms);
>  
> -excess = lr->excess - ctx->rate * ngx_abs(ms) / 1000 + 1000;
> +excess = lr->excess - ctx->rate * ms * 100 / 10 + 10;

Just keeping "/ 1000" might be a better option, given it 
corresponds to milliseconds in the "ms" variable.

[...]

> @@ -527,7 +529,7 @@
>  continue;
>  }
>  
> -delay = excess * 1000 / ctx->rate;
> +delay = excess * 10 / ctx->rate;
>  
>  if (delay > max_delay) {
>  max_delay = delay;
> @@ -535,7 +537,7 @@
>  *limit = [n];
>  }
>  }
> -
> +max_delay /= 100;
>  return max_delay;

It is not clear why calculate delay in different units instead of 
milliseconds as specified by the type used, and then convert it 
back to milliseconds.

[...]

> @@ -825,7 +835,11 @@
>  return NGX_CONF_ERROR;
>  }
>  
> -ctx->rate = rate * 1000 / scale;
> +ctx->rate = rate * 10 / scale;
> +if (ctx->rate < 1) {
> +ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "invalid rate \"%ui\"", 
> rate);
> +return NGX_CONF_ERROR;
> +}

The ctx->rate is unsigned, and the ctx->rate check will only fail 
if ctx->rate is 0.  While this might happen in some cases, it 
certainly won't catch all overflows (which are quite likely in 
real configurations on 32-bit platforms, see above).

Also, the "rate" type is ngx_int_t, so (rate * 10) will cause 
integer overflow, which is not defined.  It might be a better idea 
to avoid overflows at all.

Also, the "rate" type is ngx_int_t, not ngx_uint_t, so proper 
format would be "%i". 

In general, it might be a better idea to move the check to 
ngx_atoi(), and test something like NGX_MAX_INT_T_VALUE / 10.  
This will ensure no overflow can happen during calculations below, 
and will allow logging of the original value.  This will further 
limit maximum rate to about 20kr/s on 32-bit platforms though.

Note that I have no good solution for 32-bit platforms and the 
40kr/s (20kr/s) limit.

-- 
Maxim Dounin
http://mdounin.ru/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

Re: [PATCH] Limit req: r/h and r/d support

[Bernhard Reutner-Fischer]

On 16 February 2018 at 15:57, Bernhard Reutner-Fischer
 wrote:
> Hi!
>
> Attached is a patch to add r/h and r/d limit_req support.
>
> There are at least two feature requests that i found asking for this:
> - https://trac.nginx.org/nginx/ticket/68
> - https://trac.nginx.org/nginx/ticket/1060
>
I forgot to mention that it regression-tests cleanly for nginx-tests
and seems to work as intended.

> Ok?
>
> Please consider applying.
>
> thanks and cheers,
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[PATCH] Limit req: r/h and r/d support

[Bernhard Reutner-Fischer]

Hi!

Attached is a patch to add r/h and r/d limit_req support.

There are at least two feature requests that i found asking for this:
- https://trac.nginx.org/nginx/ticket/68
- https://trac.nginx.org/nginx/ticket/1060

Ok?

Please consider applying.

thanks and cheers,
# HG changeset patch
# User Bernhard Reutner-Fischer 
# Date 1518790589 -3600
#  Fri Feb 16 15:16:29 2018 +0100
# Node ID 4f5981016b6d3b4b9745f90ddf9521fa7a0d375f
# Parent  a49af443656f2b65ca5de9d8cad5594f44e18ff7
Limit req: r/h and r/d support

diff -r a49af443656f -r 4f5981016b6d src/http/modules/ngx_http_limit_req_module.c
--- a/src/http/modules/ngx_http_limit_req_module.c	Thu Feb 08 12:11:30 2018 +0300
+++ b/src/http/modules/ngx_http_limit_req_module.c	Fri Feb 16 15:16:29 2018 +0100
@@ -16,7 +16,7 @@
 u_short  len;
 ngx_queue_t  queue;
 ngx_msec_t   last;
-/* integer value, 1 corresponds to 0.001 r/s */
+/* integer value, 1 corresponds to 0.1 r/s */
 ngx_uint_t   excess;
 ngx_uint_t   count;
 u_char   data[1];
@@ -33,7 +33,7 @@
 typedef struct {
 ngx_http_limit_req_shctx_t  *sh;
 ngx_slab_pool_t *shpool;
-/* integer value, 1 corresponds to 0.001 r/s */
+/* integer value, 1 corresponds to 0.1 r/s */
 ngx_uint_t   rate;
 ngx_http_complex_value_t key;
 ngx_http_limit_req_node_t   *node;
@@ -42,7 +42,7 @@
 
 typedef struct {
 ngx_shm_zone_t  *shm_zone;
-/* integer value, 1 corresponds to 0.001 r/s */
+/* integer value, 1 corresponds to 0.1 r/s */
 ngx_uint_t   burst;
 ngx_uint_t   nodelay; /* unsigned  nodelay:1 */
 } ngx_http_limit_req_limit_t;
@@ -213,7 +213,7 @@
 
 ngx_log_debug4(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
"limit_req[%ui]: %i %ui.%03ui",
-   n, rc, excess / 1000, excess % 1000);
+   n, rc, excess / 10, excess % 10);
 
 if (rc != NGX_AGAIN) {
 break;
@@ -231,7 +231,7 @@
 if (rc == NGX_BUSY) {
 ngx_log_error(lrcf->limit_log_level, r->connection->log, 0,
   "limiting requests, excess: %ui.%03ui by zone \"%V\"",
-  excess / 1000, excess % 1000,
+  excess / 10, excess % 10,
   >shm_zone->shm.name);
 }
 
@@ -268,7 +268,7 @@
 
 ngx_log_error(lrcf->delay_log_level, r->connection->log, 0,
   "delaying request, excess: %ui.%03ui, by zone \"%V\"",
-  excess / 1000, excess % 1000, >shm_zone->shm.name);
+  excess / 10, excess % 10, >shm_zone->shm.name);
 
 if (ngx_handle_read_event(r->connection->read, 0) != NGX_OK) {
 return NGX_HTTP_INTERNAL_SERVER_ERROR;
@@ -398,8 +398,9 @@
 ngx_queue_insert_head(>sh->queue, >queue);
 
 ms = (ngx_msec_int_t) (now - lr->last);
+ms = ngx_abs(ms);
 
-excess = lr->excess - ctx->rate * ngx_abs(ms) / 1000 + 1000;
+excess = lr->excess - ctx->rate * ms * 100 / 10 + 10;
 
 if (excess < 0) {
 excess = 0;
@@ -493,7 +494,7 @@
 
 } else {
 ctx = (*limit)->shm_zone->data;
-max_delay = excess * 1000 / ctx->rate;
+max_delay = excess * 10 / ctx->rate;
 }
 
 while (n--) {
@@ -508,8 +509,9 @@
 
 now = ngx_current_msec;
 ms = (ngx_msec_int_t) (now - lr->last);
+ms = ngx_abs(ms);
 
-excess = lr->excess - ctx->rate * ngx_abs(ms) / 1000 + 1000;
+excess = lr->excess - ctx->rate * ms * 100 / 10 + 10;
 
 if (excess < 0) {
 excess = 0;
@@ -527,7 +529,7 @@
 continue;
 }
 
-delay = excess * 1000 / ctx->rate;
+delay = excess * 10 / ctx->rate;
 
 if (delay > max_delay) {
 max_delay = delay;
@@ -535,7 +537,7 @@
 *limit = [n];
 }
 }
-
+max_delay /= 100;
 return max_delay;
 }
 
@@ -587,7 +589,7 @@
 return;
 }
 
-excess = lr->excess - ctx->rate * ms / 1000;
+excess = lr->excess - ctx->rate * ms * 100 / 10;
 
 if (excess > 0) {
 return;
@@ -801,6 +803,14 @@
 } else if (ngx_strncmp(p, "r/m", 3) == 0) {
 scale = 60;
 len -= 3;
+
+} else if (ngx_strncmp(p, "r/h", 3) == 0) {
+scale = 60 * 60;
+len -= 3;
+
+} else if (ngx_strncmp(p, "r/d", 3) == 0) {
+scale = 60 * 60 * 24;
+len -= 3;
 }
 
 rate = ngx_atoi(value[i].data + 5, len - 5);
@@ -825,7 +835,11 @@
 return NGX_CONF_ERROR;
 }
 
-ctx->rate = rate *