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

2017-12-27 Thread 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  
> > > 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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2017-12-27 Thread 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  
> > 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!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2017-12-27 Thread 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  
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2017-12-26 Thread 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  
> 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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2017-12-19 Thread Harsha Sharma 
On Tue, Dec 19, 2017 at 7:31 PM, Pablo Neira Ayuso  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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2017-12-19 Thread 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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html