| Message ID | 20171219122716.19710-1-harshasharmaiitr@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | extensions: libxt_hashlimit: Do not print default timeout and burst | expand |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)