[PATCH] extensions: libxt_hashlimit: Do not print default timeout and burst

[PATCH] extensions: libxt_hashlimit: Do not print default timeout and burst

[Harsha Sharma]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 3955 bytes --]

Do not print timeout and burst in case default values are used.
For e.g.
iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit
--hashlimit-above 200/sec --hashlimit-mode srcip,dstport
--hashlimit-name http1 -j DROP

nft add rule ip filter INPUT tcp dport 80 flow table http1 { tcp dport .
ip saddr limit rate over 200/second } counter drop

Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
---
 extensions/libxt_hashlimit.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/extensions/libxt_hashlimit.c b/extensions/libxt_hashlimit.c
index e51ac1ae..3206c0a4 100644
--- a/extensions/libxt_hashlimit.c
+++ b/extensions/libxt_hashlimit.c
@@ -7,7 +7,7 @@
  * Based on ipt_limit.c by
  * Jérôme de Vivie   <devivie@info.enserb.u-bordeaux.fr>
  * Hervé Eychenne    <rv@wallfire.org>
- * 
+ *
  * Error corections by nmalykh@bilim.com (22.01.2005)
  */
 #define _BSD_SOURCE 1
@@ -33,6 +33,8 @@
 /* miliseconds */
 #define XT_HASHLIMIT_GCINTERVAL	1000
 
+int XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT;
+
 struct hashlimit_mt_udata {
 	uint32_t mult;
 };
@@ -678,6 +680,7 @@ static void hashlimit_mt_parse(struct xt_option_call *cb)
 			xtables_param_act(XTF_BAD_VALUE, "hashlimit",
 				"--hashlimit-rate-interval", cb->arg);
 	}
+
 }
 
 static void hashlimit_check(struct xt_fcheck_call *cb)
@@ -762,8 +765,10 @@ static void hashlimit_mt_check(struct xt_fcheck_call *cb)
 	if (!(cb->xflags & (F_UPTO | F_ABOVE)))
 		xtables_error(PARAMETER_PROBLEM,
 				"You have to specify --hashlimit");
-	if (!(cb->xflags & F_HTABLE_EXPIRE))
+	if (!(cb->xflags & F_HTABLE_EXPIRE)) {
+		XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT = 1;
 		info->cfg.expire = udata->mult * 1000; /* from s to msec */
+	}
 
 	if (info->cfg.mode & XT_HASHLIMIT_BYTES) {
 		uint32_t burst = 0;
@@ -1208,7 +1213,7 @@ static const struct rates rates_xlate[] = {
 	{ "second", XT_HASHLIMIT_SCALE_v2 } };
 
 static void print_packets_rate_xlate(struct xt_xlate *xl, uint64_t avg,
-				     uint64_t burst, int revision)
+				     int revision)
 {
 	unsigned int i;
 	const struct rates *_rates = (revision == 1) ?
@@ -1219,8 +1224,8 @@ static void print_packets_rate_xlate(struct xt_xlate *xl, uint64_t avg,
 		    _rates[i].mult / avg < _rates[i].mult % avg)
 			break;
 
-	xt_xlate_add(xl, " %llu/%s burst %lu packets",
-		     _rates[i-1].mult / avg, _rates[i-1].name, burst);
+	xt_xlate_add(xl, " %llu/%s ",
+		     _rates[i-1].mult / avg, _rates[i-1].name);
 }
 
 static void print_bytes_rate_xlate(struct xt_xlate *xl,
@@ -1340,7 +1345,9 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
 	xt_xlate_add(xl, "flow table %s {", name);
 	ret = hashlimit_mode_xlate(xl, cfg->mode, family,
 				   cfg->srcmask, cfg->dstmask);
-	xt_xlate_add(xl, " timeout %us limit rate", cfg->expire / 1000);
+	if (!XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT)
+		xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
+	xt_xlate_add(xl, " limit rate");
 
 	if (cfg->mode & XT_HASHLIMIT_INVERT)
 		xt_xlate_add(xl, " over");
@@ -1348,8 +1355,9 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
 	if (cfg->mode & XT_HASHLIMIT_BYTES)
 		print_bytes_rate_xlate(xl, cfg);
 	else
-		print_packets_rate_xlate(xl, cfg->avg, cfg->burst, revision);
-
+		print_packets_rate_xlate(xl, cfg->avg, revision);
+	if (cfg->burst & (1 << XT_HASHLIMIT_BURST))
+		xt_xlate_add(xl, " burst %lu packets", cfg->burst);
 	xt_xlate_add(xl, "}");
 
 	return ret;
@@ -1364,7 +1372,8 @@ static int hashlimit_xlate(struct xt_xlate *xl,
 	xt_xlate_add(xl, "flow table %s {", info->name);
 	ret = hashlimit_mode_xlate(xl, info->cfg.mode, NFPROTO_IPV4, 32, 32);
 	xt_xlate_add(xl, " timeout %us limit rate", info->cfg.expire / 1000);
-	print_packets_rate_xlate(xl, info->cfg.avg, info->cfg.burst, 1);
+	print_packets_rate_xlate(xl, info->cfg.avg, 1);
+	xt_xlate_add(xl, " burst %lu packets", info->cfg.burst);
 	xt_xlate_add(xl, "}");
 
 	return ret;
-- 
2.11.0

Re: [PATCH] extensions: libxt_hashlimit: Do not print default timeout and burst

[Pablo Neira Ayuso]

On Tue, Dec 19, 2017 at 05:57:16PM +0530, Harsha Sharma wrote:
> @@ -1340,7 +1345,9 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
>  	xt_xlate_add(xl, "flow table %s {", name);
>  	ret = hashlimit_mode_xlate(xl, cfg->mode, family,
>  				   cfg->srcmask, cfg->dstmask);
> -	xt_xlate_add(xl, " timeout %us limit rate", cfg->expire / 1000);
> +	if (!XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT)
> +		xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);

Better print cfg->expire if only if the default timeout is set.

        if (cfg->expire != XT_HASHLIMIT_XLATE_DEFAULT_TIMEOUT)
                ...

Same thing for burst. Thanks.

Re: [PATCH] extensions: libxt_hashlimit: Do not print default timeout and burst

[Harsha Sharma]

On Tue, Dec 19, 2017 at 7:31 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Dec 19, 2017 at 05:57:16PM +0530, Harsha Sharma wrote:
>> @@ -1340,7 +1345,9 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
>>       xt_xlate_add(xl, "flow table %s {", name);
>>       ret = hashlimit_mode_xlate(xl, cfg->mode, family,
>>                                  cfg->srcmask, cfg->dstmask);
>> -     xt_xlate_add(xl, " timeout %us limit rate", cfg->expire / 1000);
>> +     if (!XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT)
>> +             xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
>
> Better print cfg->expire if only if the default timeout is set.
>
>         if (cfg->expire != XT_HASHLIMIT_XLATE_DEFAULT_TIMEOUT)
>                 ...
>
> Same thing for burst. Thanks.

This would result into not printing the timeout 1s (default value)
even if user specifies it.
For e.g.
iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit
--hashlimit-above 200kb/s --hashlimit-mode srcip,dstport
--hashlimit-name http2 --hashlimit-htable-expire 1000 -j DROP
nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
. ip saddr limit rate over 200 kbytes/second} counter drop

and the expected output is
nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
. ip saddr timeout 1s limit rate over 200 kbytes/second} counter drop
(This patch allows this.)
I hope this makes sense and same for burst.
Thanks a lot for the review.

Regards,
Harsha Sharma

Re: [PATCH] extensions: libxt_hashlimit: Do not print default timeout and burst

[Duncan Roe]

On Tue, Dec 19, 2017 at 08:20:31PM +0530, Harsha Sharma wrote:
> On Tue, Dec 19, 2017 at 7:31 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Dec 19, 2017 at 05:57:16PM +0530, Harsha Sharma wrote:
> >> @@ -1340,7 +1345,9 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
> >>       xt_xlate_add(xl, "flow table %s {", name);
> >>       ret = hashlimit_mode_xlate(xl, cfg->mode, family,
> >>                                  cfg->srcmask, cfg->dstmask);
> >> -     xt_xlate_add(xl, " timeout %us limit rate", cfg->expire / 1000);
> >> +     if (!XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT)
> >> +             xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
> >
> > Better print cfg->expire if only if the default timeout is set.
> >
> >         if (cfg->expire != XT_HASHLIMIT_XLATE_DEFAULT_TIMEOUT)
> >                 ...
> >
> > Same thing for burst. Thanks.
>
> This would result into not printing the timeout 1s (default value)
> even if user specifies it.
> For e.g.
> iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit
> --hashlimit-above 200kb/s --hashlimit-mode srcip,dstport
> --hashlimit-name http2 --hashlimit-htable-expire 1000 -j DROP
> nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
> . ip saddr limit rate over 200 kbytes/second} counter drop
>
> and the expected output is
> nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
> . ip saddr timeout 1s limit rate over 200 kbytes/second} counter drop
> (This patch allows this.)
> I hope this makes sense and same for burst.
> Thanks a lot for the review.
>
Have to say I agree with Harsha on this. You need a logical flag to say whether
a timeout was encountered.

Once this goes through, I'll fix the wiki,

Cheers ... Duncan.

Re: [PATCH] extensions: libxt_hashlimit: Do not print default timeout and burst

[Duncan Roe]

On Wed, Dec 27, 2017 at 10:31:04AM +1100, Duncan Roe wrote:
> On Tue, Dec 19, 2017 at 08:20:31PM +0530, Harsha Sharma wrote:
> > On Tue, Dec 19, 2017 at 7:31 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Tue, Dec 19, 2017 at 05:57:16PM +0530, Harsha Sharma wrote:
> > >> @@ -1340,7 +1345,9 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
> > >>       xt_xlate_add(xl, "flow table %s {", name);
> > >>       ret = hashlimit_mode_xlate(xl, cfg->mode, family,
> > >>                                  cfg->srcmask, cfg->dstmask);
> > >> -     xt_xlate_add(xl, " timeout %us limit rate", cfg->expire / 1000);
> > >> +     if (!XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT)
> > >> +             xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
> > >
> > > Better print cfg->expire if only if the default timeout is set.
> > >
> > >         if (cfg->expire != XT_HASHLIMIT_XLATE_DEFAULT_TIMEOUT)
> > >                 ...
> > >
> > > Same thing for burst. Thanks.
> >
> > This would result into not printing the timeout 1s (default value)
> > even if user specifies it.
> > For e.g.
> > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit
> > --hashlimit-above 200kb/s --hashlimit-mode srcip,dstport
> > --hashlimit-name http2 --hashlimit-htable-expire 1000 -j DROP
> > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
> > . ip saddr limit rate over 200 kbytes/second} counter drop
> >
> > and the expected output is
> > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
> > . ip saddr timeout 1s limit rate over 200 kbytes/second} counter drop
> > (This patch allows this.)
> > I hope this makes sense and same for burst.
> > Thanks a lot for the review.
> >
> Have to say I agree with Harsha on this. You need a logical flag to say
> whether a timeout was encountered.
>
> Once this goes through, I'll fix the wiki,
>

In the original patch, I would prefer

 int XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT = 0;
                                     ^^^^
although rather to my surprise there was no compiler warning without the
initialiser, and the 3 iptables-translate examples from the wiki all worked
either way.

Can we maybe fix the int decl and go with it?

Cheers ... Duncan.

Re: [PATCH] extensions: libxt_hashlimit: Do not print default timeout and burst

[Pablo Neira Ayuso]

On Wed, Dec 27, 2017 at 10:31:04AM +1100, Duncan Roe wrote:
> On Tue, Dec 19, 2017 at 08:20:31PM +0530, Harsha Sharma wrote:
> > On Tue, Dec 19, 2017 at 7:31 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Tue, Dec 19, 2017 at 05:57:16PM +0530, Harsha Sharma wrote:
> > >> @@ -1340,7 +1345,9 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
> > >>       xt_xlate_add(xl, "flow table %s {", name);
> > >>       ret = hashlimit_mode_xlate(xl, cfg->mode, family,
> > >>                                  cfg->srcmask, cfg->dstmask);
> > >> -     xt_xlate_add(xl, " timeout %us limit rate", cfg->expire / 1000);
> > >> +     if (!XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT)
> > >> +             xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
> > >
> > > Better print cfg->expire if only if the default timeout is set.
> > >
> > >         if (cfg->expire != XT_HASHLIMIT_XLATE_DEFAULT_TIMEOUT)
> > >                 ...
> > >
> > > Same thing for burst. Thanks.
> >
> > This would result into not printing the timeout 1s (default value)
> > even if user specifies it.
> > For e.g.
> > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit
> > --hashlimit-above 200kb/s --hashlimit-mode srcip,dstport
> > --hashlimit-name http2 --hashlimit-htable-expire 1000 -j DROP
> > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
> > . ip saddr limit rate over 200 kbytes/second} counter drop
> >
> > and the expected output is
> > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
> > . ip saddr timeout 1s limit rate over 200 kbytes/second} counter drop
> > (This patch allows this.)
> > I hope this makes sense and same for burst.
> > Thanks a lot for the review.
> >
> Have to say I agree with Harsha on this. You need a logical flag to say whether
> a timeout was encountered.

Yes, but she's assuming you can iptables-translate to pass this global
flag between function, and that is not always the case.

So a simple hardcoded "if timeout equals X then don't print it" is
fair enough.

> Once this goes through, I'll fix the wiki,

Thanks!

Re: [PATCH] extensions: libxt_hashlimit: Do not print default timeout and burst

[Duncan Roe]

On Wed, Dec 27, 2017 at 12:13:26PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Dec 27, 2017 at 10:31:04AM +1100, Duncan Roe wrote:
> > On Tue, Dec 19, 2017 at 08:20:31PM +0530, Harsha Sharma wrote:
> > > On Tue, Dec 19, 2017 at 7:31 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Tue, Dec 19, 2017 at 05:57:16PM +0530, Harsha Sharma wrote:
> > > >> @@ -1340,7 +1345,9 @@ static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name,
> > > >>       xt_xlate_add(xl, "flow table %s {", name);
> > > >>       ret = hashlimit_mode_xlate(xl, cfg->mode, family,
> > > >>                                  cfg->srcmask, cfg->dstmask);
> > > >> -     xt_xlate_add(xl, " timeout %us limit rate", cfg->expire / 1000);
> > > >> +     if (!XT_HASHLIMIT_BYTE_EXPIRE_DEFAULT)
> > > >> +             xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
> > > >
> > > > Better print cfg->expire if only if the default timeout is set.
> > > >
> > > >         if (cfg->expire != XT_HASHLIMIT_XLATE_DEFAULT_TIMEOUT)
> > > >                 ...
> > > >
> > > > Same thing for burst. Thanks.
> > >
> > > This would result into not printing the timeout 1s (default value)
> > > even if user specifies it.
> > > For e.g.
> > > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit
> > > --hashlimit-above 200kb/s --hashlimit-mode srcip,dstport
> > > --hashlimit-name http2 --hashlimit-htable-expire 1000 -j DROP
> > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
> > > . ip saddr limit rate over 200 kbytes/second} counter drop
> > >
> > > and the expected output is
> > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport
> > > . ip saddr timeout 1s limit rate over 200 kbytes/second} counter drop
> > > (This patch allows this.)
> > > I hope this makes sense and same for burst.
> > > Thanks a lot for the review.
> > >
> > Have to say I agree with Harsha on this. You need a logical flag to say whether
> > a timeout was encountered.
>
> Yes, but she's assuming you can iptables-translate to pass this global
> flag between function, and that is not always the case.
>
> So a simple hardcoded "if timeout equals X then don't print it" is
> fair enough.
>
Then I would make the default nonsensical, say 0 or -1,

Cheers ... Duncan.