Skip to contentSkip to conversation

extensions: hashlimit: always print timeout on translations

PATCH In review Pablo Neira Ayuso 2 participants 2 messages Jan 4, 2018 netfilter

lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

PATCH 1/1 extensions: hashlimit: always print timeout on translations

Meters need an explicit timeout that we cannot skip, otherwise entries remain in the set forever.

This fixes the following translation:

$ 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

that was skipping the timeout option:

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

---
 extensions/libxt_hashlimit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
extensions/libxt_hashlimit.c +1 -2
index 3fa5719127db..f85f2d3a179a 100644
@@ -1341,8 +1341,7 @@ 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);
-	if (cfg->expire != 1000)
-		xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
+	xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
 	xt_xlate_add(xl, " limit rate");
 
 	if (cfg->mode & XT_HASHLIMIT_INVERT)
Signature · 1 line
2.11.0
#1 lore raw
Quoted 37 lines · Pablo Neira Ayuso

Meters need an explicit timeout that we cannot skip, otherwise entries remain in the set forever.

This fixes the following translation:

$ 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

that was skipping the timeout option:

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

---
 extensions/libxt_hashlimit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
extensions/libxt_hashlimit.c +1 -3
index 3fa5719127db..f85f2d3a179a 100644
@@ -1341,8 +1341,7 @@ 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);
-	if (cfg->expire != 1000)
-		xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
+	xt_xlate_add(xl, " timeout %us", cfg->expire / 1000);
 	xt_xlate_add(xl, " limit rate");
 
 	if (cfg->mode & XT_HASHLIMIT_INVERT)
--
Quoted 2 lines

2.11.0

Hey Pablo,

After applying this latest patch:

Quoted 2 lines
$ 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 timeout 1s limit rate over 200/second } counter drop

The spurious "timeout 1s" has re-appeared.

Sorry but ignoring default values is just a Bad Idea (tm).

You can either force a recognisable nonsense default (like 0, in the patch below), or use flags in cfg (would need an extra __u8 in struct hashlimit_cfg3 to hold them).

This is the patch I submitted the other night (my time), but diffed against commit 27de281d8aca84e3c841b3ae72a17616b1382ac4 (git diff -w). This is a simpler diff than against HEAD:

Quoted 44 lines
extensions/libxt_hashlimit.c +6 -5
@@ -759,15 +759,15 @@ static void hashlimit_mt_check(struct xt_fcheck_call *cb)
 {
        const struct hashlimit_mt_udata *udata = cb->udata;
        struct xt_hashlimit_mtinfo3 *info = cb->data;
+       uint32_t burst = 0;
 
        if (!(cb->xflags & (F_UPTO | F_ABOVE)))
                xtables_error(PARAMETER_PROBLEM,
                                "You have to specify --hashlimit");
        if (!(cb->xflags & F_HTABLE_EXPIRE))
-               info->cfg.expire = udata->mult * 1000; /* from s to msec */
+               info->cfg.expire = 0;
 
        if (info->cfg.mode & XT_HASHLIMIT_BYTES) {
-               uint32_t burst = 0;
                if (cb->xflags & F_BURST) {
                        if (info->cfg.burst < cost_to_bytes(info->cfg.avg))
                                xtables_error(PARAMETER_PROBLEM,
@@ -780,9 +780,9 @@ static void hashlimit_mt_check(struct xt_fcheck_call *cb)
                        if (!(cb->xflags & F_HTABLE_EXPIRE))
                                info->cfg.expire = XT_HASHLIMIT_BYTE_EXPIRE_BURST * 1000;
                }
-               info->cfg.burst = burst;
        } else if (info->cfg.burst > XT_HASHLIMIT_BURST_MAX)
                burst_error();
+       info->cfg.burst = burst;
 
        if (cb->xflags & F_RATEMATCH) {
                if (!(info->cfg.mode & XT_HASHLIMIT_BYTES))
@@ -1220,8 +1220,9 @@ 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);
+       if (burst)
+               xt_xlate_add(xl, " burst %lu packets", burst);
 }
 
 static void print_bytes_rate_xlate(struct xt_xlate *xl,

extensions/libxt_hashlimit.c static int hashlimit_mt_xlate(struct xt_xlate *xl, const char *name, · line 1350

Quoted 10 lines
extensions/libxt_hashlimit.c +3 -1
@@ -1341,7 +1342,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 (cfg->expire)
+               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");

If you would rather leave default values as before, I can re-work it to use flags instead.

Cheers ... Duncan.