| Message ID | 20180103144109.8280-1-pablo@netfilter.org |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [iptables] extensions: hashlimit: fix incorrect burst in translations | expand |
On Wed, Jan 03, 2018 at 03:41:08PM +0100, Pablo Neira Ayuso wrote: > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP > > shows: > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop > > which prints burst twice, this is not correct. > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > extensions/libxt_hashlimit.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/extensions/libxt_hashlimit.c b/extensions/libxt_hashlimit.c > index 472d8e7f6cc2..3fa5719127db 100644 > --- a/extensions/libxt_hashlimit.c > +++ b/extensions/libxt_hashlimit.c > @@ -1350,10 +1350,12 @@ 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 > + else { > print_packets_rate_xlate(xl, cfg->avg, revision); > - if (cfg->burst != 5) > - xt_xlate_add(xl, " burst %lu packets", cfg->burst); > + if (cfg->burst != XT_HASHLIMIT_BURST) > + xt_xlate_add(xl, " burst %lu packets", cfg->burst); > + > + } > xt_xlate_add(xl, "}"); > > return ret; > -- > 2.11.0 > This still discards a timeout of 1s (1000ms): > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --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 burst 1 mbytes} counter drop This is especially incorrect, since the code deliberately inserts a default timeout of 1m if no timeout was specified with a burst: > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 -j DROP > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 60s limit rate over 200 kbytes/second burst 1 mbytes} counter drop The patch I suggested doesn't have that problem, because of forcing defaults to zero. Can doing that have any adverse side-effects? 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 Thu, Jan 04, 2018 at 09:26:40AM +1100, Duncan Roe wrote: > On Wed, Jan 03, 2018 at 03:41:08PM +0100, Pablo Neira Ayuso wrote: > > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP > > > > shows: > > > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop > > > > which prints burst twice, this is not correct. > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- Not actually related to the patch, but I happened to notice: > 769 if (info->cfg.mode & XT_HASHLIMIT_BYTES) { > 770 uint32_t burst = 0; > 771 if (cb->xflags & F_BURST) { > 772 if (info->cfg.burst < cost_to_bytes(info->cfg.avg)) > 773 xtables_error(PARAMETER_PROBLEM, > 774 "burst cannot be smaller than %lub", cost_to_bytes(info->cfg.avg)); > 775 > 776 burst = info->cfg.burst; > 777 burst /= cost_to_bytes(info->cfg.avg); > 778 if (info->cfg.burst % cost_to_bytes(info->cfg.avg)) > 779 burst++; > 780 if (!(cb->xflags & F_HTABLE_EXPIRE)) > 781 info->cfg.expire = XT_HASHLIMIT_BYTE_EXPIRE_BURST * 1000; > 782 } > 783 info->cfg.burst = burst; > 784 } else if (info->cfg.burst > XT_HASHLIMIT_BURST_MAX) > 785 burst_error(); What is that final "else" claues there for? No hashlimit was specified so why check its value? 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 Thu, Jan 04, 2018 at 09:26:40AM +1100, Duncan Roe wrote: > On Wed, Jan 03, 2018 at 03:41:08PM +0100, Pablo Neira Ayuso wrote: > > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP > > > > shows: > > > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop > > > > which prints burst twice, this is not correct. > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- > > extensions/libxt_hashlimit.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/extensions/libxt_hashlimit.c b/extensions/libxt_hashlimit.c > > index 472d8e7f6cc2..3fa5719127db 100644 > > --- a/extensions/libxt_hashlimit.c > > +++ b/extensions/libxt_hashlimit.c > > @@ -1350,10 +1350,12 @@ 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 > > + else { > > print_packets_rate_xlate(xl, cfg->avg, revision); > > - if (cfg->burst != 5) > > - xt_xlate_add(xl, " burst %lu packets", cfg->burst); > > + if (cfg->burst != XT_HASHLIMIT_BURST) > > + xt_xlate_add(xl, " burst %lu packets", cfg->burst); > > + > > + } > > xt_xlate_add(xl, "}"); > > > > return ret; > > -- > > 2.11.0 > > > This still discards a timeout of 1s (1000ms): > > > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --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 burst 1 mbytes} counter drop > > This is especially incorrect, since the code deliberately inserts a default > timeout of 1m if no timeout was specified with a burst: > > > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 -j DROP > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 60s limit rate over 200 kbytes/second burst 1 mbytes} counter drop > > The patch I suggested doesn't have that problem, because of forcing defaults to > zero. Can doing that have any adverse side-effects? Yes. Problem is that we cannot assume that hashlimit_mt_check() is called. If you compile nftables with --with-xtables, listing of rules that are added via iptables-compat will be translated to nftables. -- 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 Thu, Jan 04, 2018 at 10:53:28AM +0100, Pablo Neira Ayuso wrote: > On Thu, Jan 04, 2018 at 09:26:40AM +1100, Duncan Roe wrote: > > On Wed, Jan 03, 2018 at 03:41:08PM +0100, Pablo Neira Ayuso wrote: > > > iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP > > > > > > shows: > > > > > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop > > > > > > which prints burst twice, this is not correct. > > > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > --- > > > extensions/libxt_hashlimit.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/extensions/libxt_hashlimit.c b/extensions/libxt_hashlimit.c > > > index 472d8e7f6cc2..3fa5719127db 100644 > > > --- a/extensions/libxt_hashlimit.c > > > +++ b/extensions/libxt_hashlimit.c > > > @@ -1350,10 +1350,12 @@ 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 > > > + else { > > > print_packets_rate_xlate(xl, cfg->avg, revision); > > > - if (cfg->burst != 5) > > > - xt_xlate_add(xl, " burst %lu packets", cfg->burst); > > > + if (cfg->burst != XT_HASHLIMIT_BURST) > > > + xt_xlate_add(xl, " burst %lu packets", cfg->burst); > > > + > > > + } > > > xt_xlate_add(xl, "}"); > > > > > > return ret; > > > -- > > > 2.11.0 > > > > > This still discards a timeout of 1s (1000ms): > > > > > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --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 burst 1 mbytes} counter drop > > > > This is especially incorrect, since the code deliberately inserts a default > > timeout of 1m if no timeout was specified with a burst: > > > > > $ iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 -j DROP > > > nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 60s limit rate over 200 kbytes/second burst 1 mbytes} counter drop > > > > The patch I suggested doesn't have that problem, because of forcing defaults to > > zero. Can doing that have any adverse side-effects? > > Yes. Problem is that we cannot assume that hashlimit_mt_check() is > called. If you compile nftables with --with-xtables, listing of rules > that are added via iptables-compat will be translated to nftables. OK on that. But see my comments to your latest patch. I guess it should be harmless to introduce a flags byte that no other code is aware of? 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 472d8e7f6cc2..3fa5719127db 100644 --- a/extensions/libxt_hashlimit.c +++ b/extensions/libxt_hashlimit.c @@ -1350,10 +1350,12 @@ 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 + else { print_packets_rate_xlate(xl, cfg->avg, revision); - if (cfg->burst != 5) - xt_xlate_add(xl, " burst %lu packets", cfg->burst); + if (cfg->burst != XT_HASHLIMIT_BURST) + xt_xlate_add(xl, " burst %lu packets", cfg->burst); + + } xt_xlate_add(xl, "}"); return ret;
iptables-translate -A INPUT -m tcp -p tcp --dport 80 -m hashlimit --hashlimit-above 200kb/s --hashlimit-burst 1mb --hashlimit-mode srcip,dstport --hashlimit-name http2 --hashlimit-htable-expire 3000 -j DROP shows: nft add rule ip filter INPUT tcp dport 80 flow table http2 { tcp dport . ip saddr timeout 3s limit rate over 200 kbytes/second burst 1 mbytes burst 6 packets} counter drop which prints burst twice, this is not correct. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- extensions/libxt_hashlimit.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)