[PATCH nf] netfilter: nft_ct: fix expiration getter

[PATCH nf] netfilter: nft_ct: fix expiration getter

[Florian Westphal]

We need to compute timeout.expires - jiffies, not the other way around.
Add a helper, another patch can then later change more places in
conntrack code where we currently open-code this.

Will allow us to only change one place later when we remove per-ct timer.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_conntrack.h | 8 ++++++++
 net/netfilter/nft_ct.c               | 6 +-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index dd78bea..b6083c3 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -284,6 +284,14 @@ static inline bool nf_is_loopback_packet(const struct sk_buff *skb)
 	return skb->dev && skb->skb_iif && skb->dev->flags & IFF_LOOPBACK;
 }
 
+/* jiffies until ct expires, 0 if already expired */
+static inline unsigned long nf_ct_expires(const struct nf_conn *ct)
+{
+	long timeout = (long)ct->timeout.expires - (long)jiffies;
+
+	return timeout > 0 ? timeout : 0;
+}
+
 struct kernel_param;
 
 int nf_conntrack_set_hashsize(const char *val, struct kernel_param *kp);
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 137e308..81fbb45 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -54,7 +54,6 @@ static void nft_ct_get_eval(const struct nft_expr *expr,
 	const struct nf_conn_help *help;
 	const struct nf_conntrack_tuple *tuple;
 	const struct nf_conntrack_helper *helper;
-	long diff;
 	unsigned int state;
 
 	ct = nf_ct_get(pkt->skb, &ctinfo);
@@ -94,10 +93,7 @@ static void nft_ct_get_eval(const struct nft_expr *expr,
 		return;
 #endif
 	case NFT_CT_EXPIRATION:
-		diff = (long)jiffies - (long)ct->timeout.expires;
-		if (diff < 0)
-			diff = 0;
-		*dest = jiffies_to_msecs(diff);
+		*dest = jiffies_to_msecs(nf_ct_expires(ct));
 		return;
 	case NFT_CT_HELPER:
 		if (ct->master == NULL)
-- 
2.7.3

Re: [PATCH nf] netfilter: nft_ct: fix expiration getter

[Pablo Neira Ayuso]

On Wed, Jul 06, 2016 at 02:53:06PM +0200, Florian Westphal wrote:
> We need to compute timeout.expires - jiffies, not the other way around.
> Add a helper, another patch can then later change more places in
> conntrack code where we currently open-code this.
>
> Will allow us to only change one place later when we remove per-ct timer.

Applied, thanks.

Re: [PATCH nf] netfilter: nft_ct: fix expiration getter

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On Fri, Jul 08, 2016 at 02:56:46PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jul 06, 2016 at 02:53:06PM +0200, Florian Westphal wrote:
> > We need to compute timeout.expires - jiffies, not the other way around.
> > Add a helper, another patch can then later change more places in
> > conntrack code where we currently open-code this.
> >
> > Will allow us to only change one place later when we remove per-ct timer.
> 
> Applied, thanks.

I just realized that this is broken from userspace, look:

# nft --debug=netlink add rule x y ct expiration 10s
ip x y
  [ ct load expiration => reg 1 ]
  [ cmp eq reg 1 0x0000000a ]

<cmdline>:1:1-30: Error: Could not process rule: No such file or
directory
add rule x y ct expiration 10s
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

jiffies_to_msecs() returns milliseconds, but userspace uses seconds.

[-- Attachment #2: 0001-datatype-time_type-should-send-milliseconds-to-users.patch --]
[-- Type: text/x-diff, Size: 3443 bytes --]

>From 899bce830f990cb32206c32f86edeb2f69ad109e Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 8 Jul 2016 15:12:31 +0200
Subject: [PATCH nft] datatype: time_type should send milliseconds to userspace

Kernel expects milliseconds, so fix this datatype to use
milliseconds instead of seconds.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/utils.h           |  1 +
 src/datatype.c            |  3 ++-
 tests/py/any/ct.t.payload | 18 +++++++++---------
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 8a1dc5e..d886764 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -83,6 +83,7 @@
 	(void) (&_max1 == &_max2);		\
 	_max1 > _max2 ? _max1 : _max2; })
 
+#define MSEC_PER_SEC	1000L
 
 /**
  * fls - find last (most-significant) bit set
diff --git a/src/datatype.c b/src/datatype.c
index 40e14c9..002c4c6 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -883,7 +883,7 @@ struct error_record *time_parse(const struct location *loc, const char *str,
 
 static void time_type_print(const struct expr *expr)
 {
-	time_print(mpz_get_uint64(expr->value));
+	time_print(mpz_get_uint64(expr->value) / MSEC_PER_SEC);
 }
 
 static struct error_record *time_type_parse(const struct expr *sym,
@@ -896,6 +896,7 @@ static struct error_record *time_type_parse(const struct expr *sym,
 	if (erec != NULL)
 		return erec;
 
+	s *= MSEC_PER_SEC;
 	if (s > UINT32_MAX)
 		return error(&sym->location, "value too large");
 
diff --git a/tests/py/any/ct.t.payload b/tests/py/any/ct.t.payload
index 7ed3338..8b1a04f 100644
--- a/tests/py/any/ct.t.payload
+++ b/tests/py/any/ct.t.payload
@@ -198,36 +198,36 @@ ip test-ip4 output
 # ct expiration 30
 ip test-ip4 output
   [ ct load expiration => reg 1 ]
-  [ cmp eq reg 1 0x0000001e ]
+  [ cmp eq reg 1 0x00007530 ]
 
 # ct expiration 22
 ip test-ip4 output
   [ ct load expiration => reg 1 ]
-  [ cmp eq reg 1 0x00000016 ]
+  [ cmp eq reg 1 0x000055f0 ]
 
 # ct expiration != 233
 ip test-ip4 output
   [ ct load expiration => reg 1 ]
-  [ cmp neq reg 1 0x000000e9 ]
+  [ cmp neq reg 1 0x00038e28 ]
 
 # ct expiration 33-45
 ip test-ip4 output
   [ ct load expiration => reg 1 ]
   [ byteorder reg 1 = hton(reg 1, 4, 4) ]
-  [ cmp gte reg 1 0x21000000 ]
-  [ cmp lte reg 1 0x2d000000 ]
+  [ cmp gte reg 1 0xe8800000 ]
+  [ cmp lte reg 1 0xc8af0000 ]
 
 # ct expiration != 33-45
 ip test-ip4 output
   [ ct load expiration => reg 1 ]
   [ byteorder reg 1 = hton(reg 1, 4, 4) ]
-  [ cmp lt reg 1 0x21000000 ]
-  [ cmp gt reg 1 0x2d000000 ]
+  [ cmp lt reg 1 0xe8800000 ]
+  [ cmp gt reg 1 0xc8af0000 ]
 
 # ct expiration {33, 55, 67, 88}
 __set%d test-ip4 3
 __set%d test-ip4 0
-	element 00000021  : 0 [end]	element 00000037  : 0 [end]	element 00000043  : 0 [end]	element 00000058  : 0 [end]
+	element 000080e8  : 0 [end]        element 0000d6d8  : 0 [end]    element 000105b8  : 0 [end]      element 000157c0  : 0 [end]
 ip test-ip4 output
   [ ct load expiration => reg 1 ]
   [ lookup reg 1 set __set%d ]
@@ -235,7 +235,7 @@ ip test-ip4 output
 # ct expiration {33-55}
 __set%d test-ip4 7
 __set%d test-ip4 0
-	element 00000000  : 1 [end]	element 21000000  : 0 [end]	element 38000000  : 1 [end]
+	element 00000000  : 1 [end]    element e8800000  : 0 [end]     element d9d60000  : 1 [end]
 ip test-ip4 output
   [ ct load expiration => reg 1 ]
   [ byteorder reg 1 = hton(reg 1, 4, 4) ]
-- 
2.1.4

Re: [PATCH nf] netfilter: nft_ct: fix expiration getter

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Jul 08, 2016 at 02:56:46PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, Jul 06, 2016 at 02:53:06PM +0200, Florian Westphal wrote:
> > > We need to compute timeout.expires - jiffies, not the other way around.
> > > Add a helper, another patch can then later change more places in
> > > conntrack code where we currently open-code this.
> > >
> > > Will allow us to only change one place later when we remove per-ct timer.
> > 
> > Applied, thanks.
> 
> I just realized that this is broken from userspace, look:
> 
> # nft --debug=netlink add rule x y ct expiration 10s
> ip x y
>   [ ct load expiration => reg 1 ]
>   [ cmp eq reg 1 0x0000000a ]
> 
> <cmdline>:1:1-30: Error: Could not process rule: No such file or
> directory
> add rule x y ct expiration 10s
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> jiffies_to_msecs() returns milliseconds, but userspace uses seconds.

Ack, thanks for catching this.
I only tested that 'ct expiration gt 0' was matching :-/