Re: [PATCH iptables] extensions: hashlimit: fix incorrect burst in translations

[Duncan Roe <duncan_roe@optusnet.com.au>]

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.