[PATCH nf-next 0/8] nf_tables: support for updating set element timeout

[PATCH nf-next 0/8] nf_tables: support for updating set element timeout

[Pablo Neira Ayuso]

Hi,

This patchset adds support for updating set element timeouts. This
includes 5 fixes, then one patch to consolidate set element timeout
extensions, and finally the new marker for elements that never expire
and support for element timeout updates.

Patch #1 fixes a bug with timeouts less than HZ/10, assuming CONFIG_HZ=100

        add element ip x y { 1.2.3.4 timeout 9ms)

   results in an element that never expires. This happens because this
   timeout results in jiffies64 == 0, hence, the timeout extension is not
   allocated.

Patch #2 rejects element expiration with no timeout, this is currently
   silently ignore in case no default set timeout is specified, e.g.

        table ip x {
        	set y {
			typeof ip saddr
			flags timeout
			elements = { 1.2.3.4 expires 30s }
		}
        }

Patch #3 remove unnecessary read_once notation when accessing default
   set timeout while holding mutex.

Patch #4 adds read-write_once notations for lockless access to default set
   timeout policy that are missing in dynset.

Patch #5 adds read-write_once notations for element expiration, again dynset
   could update this while netlink dump is in progress.

Patch #6 consolidates the timeout extensions: timeout and expiration
   are tightly coupled, use a single extension for both. This simplifies
   set element timeout updates coming in the next patches.

Patch #7 adds a marker for elements that never update.

        table ip x {
        	set y {
			typeof ip saddr
			timeout 1h
			elements = { 1.2.3.4 timeout never, 1.2.3.5 }
		}
        }

   In this case, 1.2.3.4 never expires and 1.2.3.5 gets a timeout of 1h
   as per the default set timeout.

   Note that it is already possible to define set elements that never
   expire by declaring a set with the timeout flag set on, but with no
   default set policy. In this case, no timeout extension is allocated.

        table ip x {
        	set y {
			typeof ip saddr
			flags timeout
			elements = { 1.2.3.4, 1.2.3.5 timeout 1h }
		}
        }

   In this example above, 1.2.3.4 never expires [*]. The new marker prepares
   for set element timeout updates, where the timeout extension needs to
   be allocated. This marker also allows for elements that never expire
   when default timeout policy is specified, which was not supported.

   [*] Note that sets with no default timeout do not display timeout
   never to retain backward compatibility in the listing.

Patch #8 allows to update set timeout/expiration.

        table ip x {
        	set y {
			typeof ip saddr
			timeout 1h
			elements = { 1.2.3.4, 1.2.3.5 }
		}
        }

   which use default 1h set timeout. Then, updating timeout is possible via:

        add element x y { 1.2.3.4 timeout 30s }
        add element x y { 1.2.3.5 timeout 25s }

No tests/shell yet available, still working on this.

Pablo Neira Ayuso (8):
  netfilter: nf_tables: elements with timeout less than HZ/10 never expire
  netfilter: nf_tables: reject element expiration with no timeout
  netfilter: nf_tables: remove annotation to access set timeout while holding lock
  netfilter: nft_dynset: annotate data-races around set timeout
  netfilter: nf_tables: annotate data-races around element expiration
  netfilter: nf_tables: consolidate timeout extension for elements
  netfilter: nf_tables: add never expires marker to elements
  netfilter: nf_tables: set element timeout update support

 include/net/netfilter/nf_tables.h        |  32 +++--
 include/uapi/linux/netfilter/nf_tables.h |   3 +
 net/netfilter/nf_tables_api.c            | 144 ++++++++++++++++-------
 net/netfilter/nft_dynset.c               |  21 ++--
 net/netfilter/nft_last.c                 |   3 +-
 5 files changed, 139 insertions(+), 64 deletions(-)

--
2.30.2

[PATCH nf-next 1/8] netfilter: nf_tables: elements with timeout less than HZ/10 never expire

[Pablo Neira Ayuso]

Elements with less than HZ/10 milliseconds timeout never expire because
the element timeout extension is not allocated given that
nf_msecs_to_jiffies64() returns 0. Round up this timeout to HZ/10 to let
them time out.

Fixes: 8e1102d5a159 ("netfilter: nf_tables: support timeouts larger than 23 days")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 481ee78e77bc..0fb8f8f1ef66 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4586,6 +4586,9 @@ int nf_msecs_to_jiffies64(const struct nlattr *nla, u64 *result)
 	if (ms >= max)
 		return -ERANGE;
 
+	if (ms < HZ/10)
+		ms = HZ/10;
+
 	ms *= NSEC_PER_MSEC;
 	*result = nsecs_to_jiffies64(ms);
 	return 0;
-- 
2.30.2

[PATCH nf-next 2/8] netfilter: nf_tables: reject element expiration with no timeout

[Pablo Neira Ayuso]

If element timeout is unset and set provides no default timeout, the
element expiration is silently ignored, reject this instead to let user
know this is unsupported.

While at it, remove unnecesary notation to read default set timeout
under mutex.

Fixes: 8e1102d5a159 ("netfilter: nf_tables: support timeouts larger than 23 days")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 0fb8f8f1ef66..79ab90069b84 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -6920,6 +6920,9 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	if (nla[NFTA_SET_ELEM_EXPIRATION] != NULL) {
 		if (!(set->flags & NFT_SET_TIMEOUT))
 			return -EINVAL;
+		if (timeout == 0)
+			return -EOPNOTSUPP;
+
 		err = nf_msecs_to_jiffies64(nla[NFTA_SET_ELEM_EXPIRATION],
 					    &expiration);
 		if (err)
-- 
2.30.2

[PATCH nf-next 3/8] netfilter: nf_tables: remove annotation to access set timeout while holding lock

[Pablo Neira Ayuso]

Mutex is held when adding an element, no need for READ_ONCE, remove it.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 79ab90069b84..488c07eed296 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -6913,7 +6913,7 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 			return err;
 	} else if (set->flags & NFT_SET_TIMEOUT &&
 		   !(flags & NFT_SET_ELEM_INTERVAL_END)) {
-		timeout = READ_ONCE(set->timeout);
+		timeout = set->timeout;
 	}
 
 	expiration = 0;
@@ -7017,7 +7017,7 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 		if (err < 0)
 			goto err_parse_key_end;
 
-		if (timeout != READ_ONCE(set->timeout)) {
+		if (timeout != set->timeout) {
 			err = nft_set_ext_add(&tmpl, NFT_SET_EXT_TIMEOUT);
 			if (err < 0)
 				goto err_parse_key_end;
-- 
2.30.2

[PATCH nf-next 4/8] netfilter: nft_dynset: annotate data-races around set timeout

[Pablo Neira Ayuso]

set timeout can be read locklessly while being updated from control
plane, add annotation.

Fixes: 123b99619cca ("netfilter: nf_tables: honor set timeout and garbage collection updates")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nft_dynset.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index b4ada3ab2167..489a9b34f1ec 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -56,7 +56,7 @@ static struct nft_elem_priv *nft_dynset_new(struct nft_set *set,
 	if (!atomic_add_unless(&set->nelems, 1, set->size))
 		return NULL;
 
-	timeout = priv->timeout ? : set->timeout;
+	timeout = priv->timeout ? : READ_ONCE(set->timeout);
 	elem_priv = nft_set_elem_init(set, &priv->tmpl,
 				      &regs->data[priv->sreg_key], NULL,
 				      &regs->data[priv->sreg_data],
@@ -95,7 +95,7 @@ void nft_dynset_eval(const struct nft_expr *expr,
 			     expr, regs, &ext)) {
 		if (priv->op == NFT_DYNSET_OP_UPDATE &&
 		    nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION)) {
-			timeout = priv->timeout ? : set->timeout;
+			timeout = priv->timeout ? : READ_ONCE(set->timeout);
 			*nft_set_ext_expiration(ext) = get_jiffies_64() + timeout;
 		}
 
@@ -313,7 +313,7 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
 		nft_dynset_ext_add_expr(priv);
 
 	if (set->flags & NFT_SET_TIMEOUT) {
-		if (timeout || set->timeout) {
+		if (timeout || READ_ONCE(set->timeout)) {
 			nft_set_ext_add(&priv->tmpl, NFT_SET_EXT_TIMEOUT);
 			nft_set_ext_add(&priv->tmpl, NFT_SET_EXT_EXPIRATION);
 		}
-- 
2.30.2

[PATCH nf-next 5/8] netfilter: nf_tables: annotate data-races around element expiration

[Pablo Neira Ayuso]

element expiration can be read-write locklessly, it can be written by
dynset and read from netlink dump, add annotation.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_tables.h | 2 +-
 net/netfilter/nf_tables_api.c     | 2 +-
 net/netfilter/nft_dynset.c        | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 1bfdd16890fa..7a2f7417ed9e 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -831,7 +831,7 @@ static inline bool __nft_set_elem_expired(const struct nft_set_ext *ext,
 					  u64 tstamp)
 {
 	return nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION) &&
-	       time_after_eq64(tstamp, *nft_set_ext_expiration(ext));
+	       time_after_eq64(tstamp, READ_ONCE(*nft_set_ext_expiration(ext)));
 }
 
 static inline bool nft_set_elem_expired(const struct nft_set_ext *ext)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 488c07eed296..2b75fbb5e86d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5824,7 +5824,7 @@ static int nf_tables_fill_setelem(struct sk_buff *skb,
 	if (nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION)) {
 		u64 expires, now = get_jiffies_64();
 
-		expires = *nft_set_ext_expiration(ext);
+		expires = READ_ONCE(*nft_set_ext_expiration(ext));
 		if (time_before64(now, expires))
 			expires -= now;
 		else
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 489a9b34f1ec..67474fd002b2 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -96,7 +96,7 @@ void nft_dynset_eval(const struct nft_expr *expr,
 		if (priv->op == NFT_DYNSET_OP_UPDATE &&
 		    nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION)) {
 			timeout = priv->timeout ? : READ_ONCE(set->timeout);
-			*nft_set_ext_expiration(ext) = get_jiffies_64() + timeout;
+			WRITE_ONCE(*nft_set_ext_expiration(ext), get_jiffies_64() + timeout);
 		}
 
 		nft_set_elem_update_expr(ext, regs, pkt);
-- 
2.30.2

[PATCH nf-next 6/8] netfilter: nf_tables: consolidate timeout extension for elements

[Pablo Neira Ayuso]

Expiration and timeout are stored in separated set element extensions,
but they are tightly coupled. Consolidate them in a single extension to
simplify and prepare for set element updates.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_tables.h | 18 ++++++-------
 net/netfilter/nf_tables_api.c     | 43 ++++++++++++-------------------
 net/netfilter/nft_dynset.c        | 13 ++++------
 3 files changed, 30 insertions(+), 44 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 7a2f7417ed9e..a950a1f932bf 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -683,7 +683,6 @@ void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set);
  *	@NFT_SET_EXT_DATA: mapping data
  *	@NFT_SET_EXT_FLAGS: element flags
  *	@NFT_SET_EXT_TIMEOUT: element timeout
- *	@NFT_SET_EXT_EXPIRATION: element expiration time
  *	@NFT_SET_EXT_USERDATA: user data associated with the element
  *	@NFT_SET_EXT_EXPRESSIONS: expressions assiciated with the element
  *	@NFT_SET_EXT_OBJREF: stateful object reference associated with element
@@ -695,7 +694,6 @@ enum nft_set_extensions {
 	NFT_SET_EXT_DATA,
 	NFT_SET_EXT_FLAGS,
 	NFT_SET_EXT_TIMEOUT,
-	NFT_SET_EXT_EXPIRATION,
 	NFT_SET_EXT_USERDATA,
 	NFT_SET_EXT_EXPRESSIONS,
 	NFT_SET_EXT_OBJREF,
@@ -807,14 +805,14 @@ static inline u8 *nft_set_ext_flags(const struct nft_set_ext *ext)
 	return nft_set_ext(ext, NFT_SET_EXT_FLAGS);
 }
 
-static inline u64 *nft_set_ext_timeout(const struct nft_set_ext *ext)
-{
-	return nft_set_ext(ext, NFT_SET_EXT_TIMEOUT);
-}
+struct nft_timeout {
+	u64	timeout;
+	u64	expiration;
+};
 
-static inline u64 *nft_set_ext_expiration(const struct nft_set_ext *ext)
+static inline struct nft_timeout *nft_set_ext_timeout(const struct nft_set_ext *ext)
 {
-	return nft_set_ext(ext, NFT_SET_EXT_EXPIRATION);
+	return nft_set_ext(ext, NFT_SET_EXT_TIMEOUT);
 }
 
 static inline struct nft_userdata *nft_set_ext_userdata(const struct nft_set_ext *ext)
@@ -830,8 +828,8 @@ static inline struct nft_set_elem_expr *nft_set_ext_expr(const struct nft_set_ex
 static inline bool __nft_set_elem_expired(const struct nft_set_ext *ext,
 					  u64 tstamp)
 {
-	return nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION) &&
-	       time_after_eq64(tstamp, READ_ONCE(*nft_set_ext_expiration(ext)));
+	return nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT) &&
+	       time_after_eq64(tstamp, READ_ONCE(nft_set_ext_timeout(ext)->expiration));
 }
 
 static inline bool nft_set_elem_expired(const struct nft_set_ext *ext)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 2b75fbb5e86d..ec9b85dac3a5 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5691,12 +5691,8 @@ const struct nft_set_ext_type nft_set_ext_types[] = {
 		.align	= __alignof__(u8),
 	},
 	[NFT_SET_EXT_TIMEOUT]		= {
-		.len	= sizeof(u64),
-		.align	= __alignof__(u64),
-	},
-	[NFT_SET_EXT_EXPIRATION]	= {
-		.len	= sizeof(u64),
-		.align	= __alignof__(u64),
+		.len	= sizeof(struct nft_timeout),
+		.align	= __alignof__(struct nft_timeout),
 	},
 	[NFT_SET_EXT_USERDATA]		= {
 		.len	= sizeof(struct nft_userdata),
@@ -5815,16 +5811,16 @@ static int nf_tables_fill_setelem(struct sk_buff *skb,
 		         htonl(*nft_set_ext_flags(ext))))
 		goto nla_put_failure;
 
-	if (nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT) &&
-	    nla_put_be64(skb, NFTA_SET_ELEM_TIMEOUT,
-			 nf_jiffies64_to_msecs(*nft_set_ext_timeout(ext)),
-			 NFTA_SET_ELEM_PAD))
-		goto nla_put_failure;
-
-	if (nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION)) {
+	if (nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT)) {
 		u64 expires, now = get_jiffies_64();
 
-		expires = READ_ONCE(*nft_set_ext_expiration(ext));
+		if (nft_set_ext_timeout(ext)->timeout != READ_ONCE(set->timeout) &&
+		    nla_put_be64(skb, NFTA_SET_ELEM_TIMEOUT,
+				 nf_jiffies64_to_msecs(nft_set_ext_timeout(ext)->timeout),
+				 NFTA_SET_ELEM_PAD))
+			goto nla_put_failure;
+
+		expires = READ_ONCE(nft_set_ext_timeout(ext)->expiration);
 		if (time_before64(now, expires))
 			expires -= now;
 		else
@@ -6496,13 +6492,14 @@ struct nft_elem_priv *nft_set_elem_init(const struct nft_set *set,
 			       nft_set_ext_data(ext), data, set->dlen) < 0)
 		goto err_ext_check;
 
-	if (nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION)) {
-		*nft_set_ext_expiration(ext) = get_jiffies_64() + expiration;
+	if (nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT)) {
+		nft_set_ext_timeout(ext)->timeout = timeout;
+
 		if (expiration == 0)
-			*nft_set_ext_expiration(ext) += timeout;
+			expiration = timeout;
+
+		nft_set_ext_timeout(ext)->expiration = get_jiffies_64() + expiration;
 	}
-	if (nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT))
-		*nft_set_ext_timeout(ext) = timeout;
 
 	return elem;
 
@@ -7013,15 +7010,9 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	}
 
 	if (timeout > 0) {
-		err = nft_set_ext_add(&tmpl, NFT_SET_EXT_EXPIRATION);
+		err = nft_set_ext_add(&tmpl, NFT_SET_EXT_TIMEOUT);
 		if (err < 0)
 			goto err_parse_key_end;
-
-		if (timeout != set->timeout) {
-			err = nft_set_ext_add(&tmpl, NFT_SET_EXT_TIMEOUT);
-			if (err < 0)
-				goto err_parse_key_end;
-		}
 	}
 
 	if (num_exprs) {
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 67474fd002b2..88ea2454c6df 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -94,9 +94,9 @@ void nft_dynset_eval(const struct nft_expr *expr,
 	if (set->ops->update(set, &regs->data[priv->sreg_key], nft_dynset_new,
 			     expr, regs, &ext)) {
 		if (priv->op == NFT_DYNSET_OP_UPDATE &&
-		    nft_set_ext_exists(ext, NFT_SET_EXT_EXPIRATION)) {
+		    nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT)) {
 			timeout = priv->timeout ? : READ_ONCE(set->timeout);
-			WRITE_ONCE(*nft_set_ext_expiration(ext), get_jiffies_64() + timeout);
+			WRITE_ONCE(nft_set_ext_timeout(ext)->expiration, get_jiffies_64() + timeout);
 		}
 
 		nft_set_elem_update_expr(ext, regs, pkt);
@@ -312,12 +312,9 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
 	if (priv->num_exprs)
 		nft_dynset_ext_add_expr(priv);
 
-	if (set->flags & NFT_SET_TIMEOUT) {
-		if (timeout || READ_ONCE(set->timeout)) {
-			nft_set_ext_add(&priv->tmpl, NFT_SET_EXT_TIMEOUT);
-			nft_set_ext_add(&priv->tmpl, NFT_SET_EXT_EXPIRATION);
-		}
-	}
+	if (set->flags & NFT_SET_TIMEOUT &&
+	    (timeout || READ_ONCE(set->timeout)))
+		nft_set_ext_add(&priv->tmpl, NFT_SET_EXT_TIMEOUT);
 
 	priv->timeout = timeout;
 
-- 
2.30.2

[PATCH nf-next 7/8] netfilter: nf_tables: add never expires marker to elements

[Pablo Neira Ayuso]

This patch adds a timeout marker for those elements that never expire
when the element are created, so timeout updates are possible.

Note that maximum supported timeout in milliseconds which is conveyed
within a netlink attribute is 0x10c6f7a0b5ec which translates to
0xffffffffffe85300 jiffies64, higher milliseconds values result in an
ERANGE error. Use U64_MAX as an internal marker to be stored in the set
element timeout field for permanent elements.

If userspace provides no timeout for an element, then the default set
timeout applies. However, if no default set timeout is specified and
timeout flag is set on, then such new element gets the never expires
marker.

Note that, in older kernels, it is already possible to define elements
that never expire by declaring a set with the set timeout flag set on
and no global set timeout, in this case, new element with no explicit
timeout never expire do not allocate the timeout extension, hence, they
never expire. This approach makes it complicated to accomodate element
timeout update, because element extensions do not support reallocations.
Therefore, allocate the timeout extension and use the new marker for
this case, but do not expose it to userspace to retain backward
compatibility in the set listing.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_tables.h        |  9 ++--
 include/uapi/linux/netfilter/nf_tables.h |  3 ++
 net/netfilter/nf_tables_api.c            | 65 ++++++++++++++++--------
 net/netfilter/nft_dynset.c               |  6 ++-
 net/netfilter/nft_last.c                 |  3 +-
 5 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index a950a1f932bf..1c218794c936 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -828,8 +828,11 @@ static inline struct nft_set_elem_expr *nft_set_ext_expr(const struct nft_set_ex
 static inline bool __nft_set_elem_expired(const struct nft_set_ext *ext,
 					  u64 tstamp)
 {
-	return nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT) &&
-	       time_after_eq64(tstamp, READ_ONCE(nft_set_ext_timeout(ext)->expiration));
+	if (!nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT) ||
+	    nft_set_ext_timeout(ext)->timeout == NFT_NEVER_EXPIRES)
+		return false;
+
+	return time_after_eq64(tstamp, READ_ONCE(nft_set_ext_timeout(ext)->expiration));
 }
 
 static inline bool nft_set_elem_expired(const struct nft_set_ext *ext)
@@ -1861,7 +1864,7 @@ void nft_chain_route_fini(void);
 
 void nf_tables_trans_destroy_flush_work(void);
 
-int nf_msecs_to_jiffies64(const struct nlattr *nla, u64 *result);
+int nf_msecs_to_jiffies64(const struct nlattr *nla, u64 *result, bool never_expires);
 __be64 nf_jiffies64_to_msecs(u64 input);
 
 #ifdef CONFIG_MODULES
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 639894ed1b97..19ef0acea98b 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -430,6 +430,9 @@ enum nft_set_elem_flags {
 	NFT_SET_ELEM_CATCHALL		= 0x2,
 };
 
+/* Marker value for elements that never expire. */
+#define NFT_NEVER_EXPIRES	U64_MAX
+
 /**
  * enum nft_set_elem_attributes - nf_tables set element netlink attributes
  *
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index ec9b85dac3a5..7fb9a2cc88ca 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4577,11 +4577,17 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
 	return 0;
 }
 
-int nf_msecs_to_jiffies64(const struct nlattr *nla, u64 *result)
+int nf_msecs_to_jiffies64(const struct nlattr *nla, u64 *result,
+			  bool never_expires)
 {
 	u64 ms = be64_to_cpu(nla_get_be64(nla));
 	u64 max = (u64)(~((u64)0));
 
+	if (never_expires && ms == NFT_NEVER_EXPIRES) {
+		*result = NFT_NEVER_EXPIRES;
+		return 0;
+	}
+
 	max = div_u64(max, NSEC_PER_MSEC);
 	if (ms >= max)
 		return -ERANGE;
@@ -5169,7 +5175,8 @@ static int nf_tables_newset(struct sk_buff *skb, const struct nfnl_info *info,
 		if (flags & NFT_SET_ANONYMOUS)
 			return -EOPNOTSUPP;
 
-		err = nf_msecs_to_jiffies64(nla[NFTA_SET_TIMEOUT], &desc.timeout);
+		err = nf_msecs_to_jiffies64(nla[NFTA_SET_TIMEOUT],
+					    &desc.timeout, false);
 		if (err)
 			return err;
 	}
@@ -5812,24 +5819,36 @@ static int nf_tables_fill_setelem(struct sk_buff *skb,
 		goto nla_put_failure;
 
 	if (nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT)) {
-		u64 expires, now = get_jiffies_64();
-
-		if (nft_set_ext_timeout(ext)->timeout != READ_ONCE(set->timeout) &&
-		    nla_put_be64(skb, NFTA_SET_ELEM_TIMEOUT,
-				 nf_jiffies64_to_msecs(nft_set_ext_timeout(ext)->timeout),
+		u64 timeout = nft_set_ext_timeout(ext)->timeout, msecs = 0;
+		u64 set_timeout = READ_ONCE(set->timeout);
+
+		if (set_timeout > 0) {
+			if (timeout == NFT_NEVER_EXPIRES)
+				msecs = NFT_NEVER_EXPIRES;
+			else if (timeout != set_timeout)
+				msecs = nf_jiffies64_to_msecs(timeout);
+		} else if (timeout && timeout != NFT_NEVER_EXPIRES)
+			msecs = nf_jiffies64_to_msecs(timeout);
+
+		if (msecs &&
+		    nla_put_be64(skb, NFTA_SET_ELEM_TIMEOUT, msecs,
 				 NFTA_SET_ELEM_PAD))
 			goto nla_put_failure;
 
-		expires = READ_ONCE(nft_set_ext_timeout(ext)->expiration);
-		if (time_before64(now, expires))
-			expires -= now;
-		else
-			expires = 0;
+		if (timeout != NFT_NEVER_EXPIRES) {
+			u64 expires, now = get_jiffies_64();
 
-		if (nla_put_be64(skb, NFTA_SET_ELEM_EXPIRATION,
-				 nf_jiffies64_to_msecs(expires),
-				 NFTA_SET_ELEM_PAD))
-			goto nla_put_failure;
+			expires = READ_ONCE(nft_set_ext_timeout(ext)->expiration);
+			if (time_before64(now, expires))
+				expires -= now;
+			else
+				expires = 0;
+
+			if (nla_put_be64(skb, NFTA_SET_ELEM_EXPIRATION,
+					 nf_jiffies64_to_msecs(expires),
+					 NFTA_SET_ELEM_PAD))
+				goto nla_put_failure;
+		}
 	}
 
 	if (nft_set_ext_exists(ext, NFT_SET_EXT_USERDATA)) {
@@ -6498,7 +6517,10 @@ struct nft_elem_priv *nft_set_elem_init(const struct nft_set *set,
 		if (expiration == 0)
 			expiration = timeout;
 
-		nft_set_ext_timeout(ext)->expiration = get_jiffies_64() + expiration;
+		if (timeout != NFT_NEVER_EXPIRES)
+			expiration += get_jiffies_64();
+
+		nft_set_ext_timeout(ext)->expiration = expiration;
 	}
 
 	return elem;
@@ -6904,24 +6926,27 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	if (nla[NFTA_SET_ELEM_TIMEOUT] != NULL) {
 		if (!(set->flags & NFT_SET_TIMEOUT))
 			return -EINVAL;
+
 		err = nf_msecs_to_jiffies64(nla[NFTA_SET_ELEM_TIMEOUT],
-					    &timeout);
+					    &timeout, true);
 		if (err)
 			return err;
 	} else if (set->flags & NFT_SET_TIMEOUT &&
 		   !(flags & NFT_SET_ELEM_INTERVAL_END)) {
 		timeout = set->timeout;
+		if (timeout == 0)
+			timeout = NFT_NEVER_EXPIRES;
 	}
 
 	expiration = 0;
 	if (nla[NFTA_SET_ELEM_EXPIRATION] != NULL) {
 		if (!(set->flags & NFT_SET_TIMEOUT))
 			return -EINVAL;
-		if (timeout == 0)
+		if (timeout == 0 || timeout == NFT_NEVER_EXPIRES)
 			return -EOPNOTSUPP;
 
 		err = nf_msecs_to_jiffies64(nla[NFTA_SET_ELEM_EXPIRATION],
-					    &expiration);
+					    &expiration, false);
 		if (err)
 			return err;
 	}
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 88ea2454c6df..39e773b1c612 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -94,7 +94,8 @@ void nft_dynset_eval(const struct nft_expr *expr,
 	if (set->ops->update(set, &regs->data[priv->sreg_key], nft_dynset_new,
 			     expr, regs, &ext)) {
 		if (priv->op == NFT_DYNSET_OP_UPDATE &&
-		    nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT)) {
+		    nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT) &&
+		    nft_set_ext_timeout(ext)->timeout != NFT_NEVER_EXPIRES) {
 			timeout = priv->timeout ? : READ_ONCE(set->timeout);
 			WRITE_ONCE(nft_set_ext_timeout(ext)->expiration, get_jiffies_64() + timeout);
 		}
@@ -210,7 +211,8 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
 		if (!(set->flags & NFT_SET_TIMEOUT))
 			return -EOPNOTSUPP;
 
-		err = nf_msecs_to_jiffies64(tb[NFTA_DYNSET_TIMEOUT], &timeout);
+		err = nf_msecs_to_jiffies64(tb[NFTA_DYNSET_TIMEOUT], &timeout,
+					    false);
 		if (err)
 			return err;
 	}
diff --git a/net/netfilter/nft_last.c b/net/netfilter/nft_last.c
index de1b6066bfa8..9a0faba16d2d 100644
--- a/net/netfilter/nft_last.c
+++ b/net/netfilter/nft_last.c
@@ -38,7 +38,8 @@ static int nft_last_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 		last->set = ntohl(nla_get_be32(tb[NFTA_LAST_SET]));
 
 	if (last->set && tb[NFTA_LAST_MSECS]) {
-		err = nf_msecs_to_jiffies64(tb[NFTA_LAST_MSECS], &last_jiffies);
+		err = nf_msecs_to_jiffies64(tb[NFTA_LAST_MSECS], &last_jiffies,
+					    false);
 		if (err < 0)
 			goto err;
 
-- 
2.30.2

[PATCH nf-next 8/8] netfilter: nf_tables: set element timeout update support

[Pablo Neira Ayuso]

Store new timeout and expiration in transaction object, use them to
update elements from .commit path. Otherwise, discard update if .abort
path is exercised.

Annotate access to timeout extension now that it can be updated while
lockless read access is possible.

Reject timeout updates on elements with no timeout extension.

Element transaction remains in the 96 bytes kmalloc slab on x86_64 after
this update.

This patch requires ("netfilter: nf_tables: use timestamp to check for
set element timeout") to make sure an element does not expire while
transaction is ongoing.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_tables.h | 11 ++++++-
 net/netfilter/nf_tables_api.c     | 48 ++++++++++++++++++++++++++++---
 net/netfilter/nft_dynset.c        |  2 +-
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 1c218794c936..e6e07faed1f7 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -829,7 +829,7 @@ static inline bool __nft_set_elem_expired(const struct nft_set_ext *ext,
 					  u64 tstamp)
 {
 	if (!nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT) ||
-	    nft_set_ext_timeout(ext)->timeout == NFT_NEVER_EXPIRES)
+	    READ_ONCE(nft_set_ext_timeout(ext)->timeout) == NFT_NEVER_EXPIRES)
 		return false;
 
 	return time_after_eq64(tstamp, READ_ONCE(nft_set_ext_timeout(ext)->expiration));
@@ -1749,6 +1749,9 @@ struct nft_trans_elem {
 	struct nft_trans		nft_trans;
 	struct nft_set			*set;
 	struct nft_elem_priv		*elem_priv;
+	u64				timeout;
+	u64				expiration;
+	bool				update;
 	bool				bound;
 };
 
@@ -1758,6 +1761,12 @@ struct nft_trans_elem {
 	nft_trans_container_elem(trans)->set
 #define nft_trans_elem_priv(trans)			\
 	nft_trans_container_elem(trans)->elem_priv
+#define nft_trans_elem_update(trans)			\
+	nft_trans_container_elem(trans)->update
+#define nft_trans_elem_timeout(trans)			\
+	nft_trans_container_elem(trans)->timeout
+#define nft_trans_elem_expiration(trans)		\
+	nft_trans_container_elem(trans)->expiration
 #define nft_trans_elem_set_bound(trans)			\
 	nft_trans_container_elem(trans)->bound
 
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 7fb9a2cc88ca..53cb6c1fdf12 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5819,7 +5819,7 @@ static int nf_tables_fill_setelem(struct sk_buff *skb,
 		goto nla_put_failure;
 
 	if (nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT)) {
-		u64 timeout = nft_set_ext_timeout(ext)->timeout, msecs = 0;
+		u64 timeout = READ_ONCE(nft_set_ext_timeout(ext)->timeout), msecs = 0;
 		u64 set_timeout = READ_ONCE(set->timeout);
 
 		if (set_timeout > 0) {
@@ -6864,6 +6864,7 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	struct nft_data_desc desc;
 	enum nft_registers dreg;
 	struct nft_trans *trans;
+	bool update = false;
 	u64 expiration;
 	u64 timeout;
 	int err, i;
@@ -7175,8 +7176,31 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 			     nft_set_ext_exists(ext2, NFT_SET_EXT_OBJREF) &&
 			     *nft_set_ext_obj(ext) != *nft_set_ext_obj(ext2)))
 				goto err_element_clash;
-			else if (!(nlmsg_flags & NLM_F_EXCL))
+			else if (!(nlmsg_flags & NLM_F_EXCL)) {
 				err = 0;
+				if (timeout) {
+					nft_trans_elem_timeout(trans) = timeout;
+					if (expiration == 0)
+						expiration = timeout;
+
+					update = true;
+				}
+				if (expiration) {
+					nft_trans_elem_expiration(trans) = expiration;
+					update = true;
+				}
+
+				if (update) {
+					if (WARN_ON_ONCE(!nft_set_ext_exists(ext2, NFT_SET_EXT_TIMEOUT))) {
+						err = -EINVAL;
+						goto err_element_clash;
+					}
+					nft_trans_elem_priv(trans) = elem_priv;
+					nft_trans_elem_update(trans) = true;
+					nft_trans_commit_list_add_tail(ctx->net, trans);
+					goto err_elem_free;
+				}
+			}
 		} else if (err == -ENOTEMPTY) {
 			/* ENOTEMPTY reports overlapping between this element
 			 * and an existing one.
@@ -10443,7 +10467,22 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 		case NFT_MSG_NEWSETELEM:
 			te = nft_trans_container_elem(trans);
 
-			nft_setelem_activate(net, te->set, te->elem_priv);
+			if (te->update) {
+				const struct nft_set_ext *ext =
+					nft_set_elem_ext(te->set, te->elem_priv);
+
+				if (te->timeout) {
+					WRITE_ONCE(nft_set_ext_timeout(ext)->timeout,
+						   te->timeout);
+				}
+				if (te->expiration) {
+					WRITE_ONCE(nft_set_ext_timeout(ext)->expiration,
+						   get_jiffies_64() + te->expiration);
+				}
+			} else {
+				nft_setelem_activate(net, te->set, te->elem_priv);
+			}
+
 			nf_tables_setelem_notify(&ctx, te->set,
 						 te->elem_priv,
 						 NFT_MSG_NEWSETELEM);
@@ -10742,7 +10781,8 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_NEWSETELEM:
-			if (nft_trans_elem_set_bound(trans)) {
+			if (nft_trans_elem_update(trans) ||
+			    nft_trans_elem_set_bound(trans)) {
 				nft_trans_destroy(trans);
 				break;
 			}
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 39e773b1c612..6787c07a1b8f 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -95,7 +95,7 @@ void nft_dynset_eval(const struct nft_expr *expr,
 			     expr, regs, &ext)) {
 		if (priv->op == NFT_DYNSET_OP_UPDATE &&
 		    nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT) &&
-		    nft_set_ext_timeout(ext)->timeout != NFT_NEVER_EXPIRES) {
+		    READ_ONCE(nft_set_ext_timeout(ext)->timeout) != NFT_NEVER_EXPIRES) {
 			timeout = priv->timeout ? : READ_ONCE(set->timeout);
 			WRITE_ONCE(nft_set_ext_timeout(ext)->expiration, get_jiffies_64() + timeout);
 		}
-- 
2.30.2

Re: [PATCH nf-next 7/8] netfilter: nf_tables: add never expires marker to elements

[kernel test robot]

Hi Pablo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on netfilter-nf/main]
[also build test WARNING on linus/master v6.11-rc2 next-20240808]
[cannot apply to nf-next/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Pablo-Neira-Ayuso/netfilter-nf_tables-elements-with-timeout-less-than-HZ-10-never-expire/20240807-222806
base:   https://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git main
patch link:    https://lore.kernel.org/r/20240807142357.90493-8-pablo%40netfilter.org
patch subject: [PATCH nf-next 7/8] netfilter: nf_tables: add never expires marker to elements
config: x86_64-randconfig-123-20240808 (https://download.01.org/0day-ci/archive/20240809/202408090046.7kJeFHf4-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240809/202408090046.7kJeFHf4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408090046.7kJeFHf4-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
   net/netfilter/nf_tables_api.c:1905:25: sparse: sparse: cast between address spaces (__percpu -> __rcu)
   net/netfilter/nf_tables_api.c:1905:25: sparse: sparse: incompatible types in comparison expression (different address spaces):
   net/netfilter/nf_tables_api.c:1905:25: sparse:    struct nft_stats [noderef] __rcu *
   net/netfilter/nf_tables_api.c:1905:25: sparse:    struct nft_stats [noderef] __percpu *
   net/netfilter/nf_tables_api.c:2077:31: sparse: sparse: incorrect type in return expression (different address spaces) @@     expected struct nft_stats [noderef] __percpu * @@     got void * @@
   net/netfilter/nf_tables_api.c:2080:31: sparse: sparse: incorrect type in return expression (different address spaces) @@     expected struct nft_stats [noderef] __percpu * @@     got void * @@
   net/netfilter/nf_tables_api.c:2084:31: sparse: sparse: incorrect type in return expression (different address spaces) @@     expected struct nft_stats [noderef] __percpu * @@     got void * @@
   net/netfilter/nf_tables_api.c:2107:17: sparse: sparse: cast between address spaces (__percpu -> __rcu)
   net/netfilter/nf_tables_api.c:2107:17: sparse: sparse: incompatible types in comparison expression (different address spaces):
   net/netfilter/nf_tables_api.c:2107:17: sparse:    struct nft_stats [noderef] __rcu *
   net/netfilter/nf_tables_api.c:2107:17: sparse:    struct nft_stats [noderef] __percpu *
   net/netfilter/nf_tables_api.c:2107:17: sparse: sparse: incompatible types in comparison expression (different address spaces):
   net/netfilter/nf_tables_api.c:2107:17: sparse:    struct nft_stats [noderef] __rcu *
   net/netfilter/nf_tables_api.c:2107:17: sparse:    struct nft_stats [noderef] __percpu *
   net/netfilter/nf_tables_api.c:2150:21: sparse: sparse: cast between address spaces (__percpu -> __rcu)
   net/netfilter/nf_tables_api.c:2150:21: sparse: sparse: incompatible types in comparison expression (different address spaces):
   net/netfilter/nf_tables_api.c:2150:21: sparse:    struct nft_stats [noderef] __rcu *
   net/netfilter/nf_tables_api.c:2150:21: sparse:    struct nft_stats [noderef] __percpu *
   net/netfilter/nf_tables_api.c:2533:25: sparse: sparse: cast between address spaces (__percpu -> __rcu)
   net/netfilter/nf_tables_api.c:2533:25: sparse: sparse: incompatible types in comparison expression (different address spaces):
   net/netfilter/nf_tables_api.c:2533:25: sparse:    struct nft_stats [noderef] __rcu *
   net/netfilter/nf_tables_api.c:2533:25: sparse:    struct nft_stats [noderef] __percpu *
   net/netfilter/nf_tables_api.c:2740:23: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct nft_stats *stats @@     got struct nft_stats [noderef] __percpu * @@
   net/netfilter/nf_tables_api.c:2752:38: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct nft_stats [noderef] __percpu *stats @@     got struct nft_stats *stats @@
   net/netfilter/nf_tables_api.c:2798:21: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void [noderef] __percpu *__pdata @@     got struct nft_stats *stats @@
>> net/netfilter/nf_tables_api.c:5829:39: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned long long [assigned] [usertype] msecs @@     got restricted __be64 @@
   net/netfilter/nf_tables_api.c:5831:31: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned long long [assigned] [usertype] msecs @@     got restricted __be64 @@
>> net/netfilter/nf_tables_api.c:5834:62: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected restricted __be64 [usertype] value @@     got unsigned long long [assigned] [usertype] msecs @@
   net/netfilter/nf_tables_api.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/umh.h, include/linux/kmod.h, ...):
   include/linux/page-flags.h:235:46: sparse: sparse: self-comparison always evaluates to false
   include/linux/page-flags.h:235:46: sparse: sparse: self-comparison always evaluates to false
   net/netfilter/nf_tables_api.c: note: in included file (through include/linux/rbtree.h, include/linux/mm_types.h, include/linux/mmzone.h, ...):
   include/linux/rcupdate.h:867:9: sparse: sparse: context imbalance in 'nft_netlink_dump_start_rcu' - unexpected unlock

vim +5829 net/netfilter/nf_tables_api.c

  5778	
  5779	static int nf_tables_fill_setelem(struct sk_buff *skb,
  5780					  const struct nft_set *set,
  5781					  const struct nft_elem_priv *elem_priv,
  5782					  bool reset)
  5783	{
  5784		const struct nft_set_ext *ext = nft_set_elem_ext(set, elem_priv);
  5785		unsigned char *b = skb_tail_pointer(skb);
  5786		struct nlattr *nest;
  5787	
  5788		nest = nla_nest_start_noflag(skb, NFTA_LIST_ELEM);
  5789		if (nest == NULL)
  5790			goto nla_put_failure;
  5791	
  5792		if (nft_set_ext_exists(ext, NFT_SET_EXT_KEY) &&
  5793		    nft_data_dump(skb, NFTA_SET_ELEM_KEY, nft_set_ext_key(ext),
  5794				  NFT_DATA_VALUE, set->klen) < 0)
  5795			goto nla_put_failure;
  5796	
  5797		if (nft_set_ext_exists(ext, NFT_SET_EXT_KEY_END) &&
  5798		    nft_data_dump(skb, NFTA_SET_ELEM_KEY_END, nft_set_ext_key_end(ext),
  5799				  NFT_DATA_VALUE, set->klen) < 0)
  5800			goto nla_put_failure;
  5801	
  5802		if (nft_set_ext_exists(ext, NFT_SET_EXT_DATA) &&
  5803		    nft_data_dump(skb, NFTA_SET_ELEM_DATA, nft_set_ext_data(ext),
  5804				  nft_set_datatype(set), set->dlen) < 0)
  5805			goto nla_put_failure;
  5806	
  5807		if (nft_set_ext_exists(ext, NFT_SET_EXT_EXPRESSIONS) &&
  5808		    nft_set_elem_expr_dump(skb, set, ext, reset))
  5809			goto nla_put_failure;
  5810	
  5811		if (nft_set_ext_exists(ext, NFT_SET_EXT_OBJREF) &&
  5812		    nla_put_string(skb, NFTA_SET_ELEM_OBJREF,
  5813				   (*nft_set_ext_obj(ext))->key.name) < 0)
  5814			goto nla_put_failure;
  5815	
  5816		if (nft_set_ext_exists(ext, NFT_SET_EXT_FLAGS) &&
  5817		    nla_put_be32(skb, NFTA_SET_ELEM_FLAGS,
  5818			         htonl(*nft_set_ext_flags(ext))))
  5819			goto nla_put_failure;
  5820	
  5821		if (nft_set_ext_exists(ext, NFT_SET_EXT_TIMEOUT)) {
  5822			u64 timeout = nft_set_ext_timeout(ext)->timeout, msecs = 0;
  5823			u64 set_timeout = READ_ONCE(set->timeout);
  5824	
  5825			if (set_timeout > 0) {
  5826				if (timeout == NFT_NEVER_EXPIRES)
  5827					msecs = NFT_NEVER_EXPIRES;
  5828				else if (timeout != set_timeout)
> 5829					msecs = nf_jiffies64_to_msecs(timeout);
  5830			} else if (timeout && timeout != NFT_NEVER_EXPIRES)
  5831				msecs = nf_jiffies64_to_msecs(timeout);
  5832	
  5833			if (msecs &&
> 5834			    nla_put_be64(skb, NFTA_SET_ELEM_TIMEOUT, msecs,
  5835					 NFTA_SET_ELEM_PAD))
  5836				goto nla_put_failure;
  5837	
  5838			if (timeout != NFT_NEVER_EXPIRES) {
  5839				u64 expires, now = get_jiffies_64();
  5840	
  5841				expires = READ_ONCE(nft_set_ext_timeout(ext)->expiration);
  5842				if (time_before64(now, expires))
  5843					expires -= now;
  5844				else
  5845					expires = 0;
  5846	
  5847				if (nla_put_be64(skb, NFTA_SET_ELEM_EXPIRATION,
  5848						 nf_jiffies64_to_msecs(expires),
  5849						 NFTA_SET_ELEM_PAD))
  5850					goto nla_put_failure;
  5851			}
  5852		}
  5853	
  5854		if (nft_set_ext_exists(ext, NFT_SET_EXT_USERDATA)) {
  5855			struct nft_userdata *udata;
  5856	
  5857			udata = nft_set_ext_userdata(ext);
  5858			if (nla_put(skb, NFTA_SET_ELEM_USERDATA,
  5859				    udata->len + 1, udata->data))
  5860				goto nla_put_failure;
  5861		}
  5862	
  5863		nla_nest_end(skb, nest);
  5864		return 0;
  5865	
  5866	nla_put_failure:
  5867		nlmsg_trim(skb, b);
  5868		return -EMSGSIZE;
  5869	}
  5870	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH nf-next 1/8] netfilter: nf_tables: elements with timeout less than HZ/10 never expire

[Phil Sutter]

Hi Pablo,

On Wed, Aug 07, 2024 at 04:23:50PM +0200, Pablo Neira Ayuso wrote:
> Elements with less than HZ/10 milliseconds timeout never expire because
> the element timeout extension is not allocated given that
> nf_msecs_to_jiffies64() returns 0. Round up this timeout to HZ/10 to let
> them time out.
> 
> Fixes: 8e1102d5a159 ("netfilter: nf_tables: support timeouts larger than 23 days")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_tables_api.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 481ee78e77bc..0fb8f8f1ef66 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -4586,6 +4586,9 @@ int nf_msecs_to_jiffies64(const struct nlattr *nla, u64 *result)
>  	if (ms >= max)
>  		return -ERANGE;
>  
> +	if (ms < HZ/10)
> +		ms = HZ/10;
> +

This lower boundary works for HZ=100 only, right? With HZ=1000, the
mininum timeout is calculated to 100ms, but the kernel ticks once per ms
so 1ms is exactly 1 jiffie.

>  	ms *= NSEC_PER_MSEC;
>  	*result = nsecs_to_jiffies64(ms);

Why not simply sanitize *result? E.g. like so:

|	*result = nsecs_to_jiffies64(ms) ?: !!ms;

Cheers, Phil

Re: [PATCH nf-next 2/8] netfilter: nf_tables: reject element expiration with no timeout

[Phil Sutter]

On Wed, Aug 07, 2024 at 04:23:51PM +0200, Pablo Neira Ayuso wrote:
> If element timeout is unset and set provides no default timeout, the
> element expiration is silently ignored, reject this instead to let user
> know this is unsupported.
> 
> While at it, remove unnecesary notation to read default set timeout
> under mutex.

The sentence above is a left-over from splitting patches, right?

> Fixes: 8e1102d5a159 ("netfilter: nf_tables: support timeouts larger than 23 days")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_tables_api.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 0fb8f8f1ef66..79ab90069b84 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -6920,6 +6920,9 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
>  	if (nla[NFTA_SET_ELEM_EXPIRATION] != NULL) {
>  		if (!(set->flags & NFT_SET_TIMEOUT))
>  			return -EINVAL;
> +		if (timeout == 0)
> +			return -EOPNOTSUPP;
> +
>  		err = nf_msecs_to_jiffies64(nla[NFTA_SET_ELEM_EXPIRATION],
>  					    &expiration);
>  		if (err)
> -- 
> 2.30.2
> 
> 
> 

Re: [PATCH nf-next 7/8] netfilter: nf_tables: add never expires marker to elements

[Phil Sutter]

Hi Pablo,

On Wed, Aug 07, 2024 at 04:23:56PM +0200, Pablo Neira Ayuso wrote:
> This patch adds a timeout marker for those elements that never expire
> when the element are created, so timeout updates are possible.
> 
> Note that maximum supported timeout in milliseconds which is conveyed
> within a netlink attribute is 0x10c6f7a0b5ec which translates to
> 0xffffffffffe85300 jiffies64, higher milliseconds values result in an
> ERANGE error. Use U64_MAX as an internal marker to be stored in the set
> element timeout field for permanent elements.
> 
> If userspace provides no timeout for an element, then the default set
> timeout applies. However, if no default set timeout is specified and
> timeout flag is set on, then such new element gets the never expires
> marker.
> 
> Note that, in older kernels, it is already possible to define elements
> that never expire by declaring a set with the set timeout flag set on
> and no global set timeout, in this case, new element with no explicit
> timeout never expire do not allocate the timeout extension, hence, they
> never expire. This approach makes it complicated to accomodate element
> timeout update, because element extensions do not support reallocations.
> Therefore, allocate the timeout extension and use the new marker for
> this case, but do not expose it to userspace to retain backward
> compatibility in the set listing.

I fail to miss the point why this marker is needed at all:

Right now, new set elements receive EXT_TIMEOUT upon creation if either
NFTA_SET_ELEM_TIMEOUT is present (i.e., user specified per-element
timeout) or set->timeout is non-zero (i.e., set has a default timeout).

Why not change this logic and add EXT_TIMEOUT to all elements which will
timeout and initialize it either to the user-defined value or the set's
default? Then, only elements which don't timeout remain without
EXT_TIMEOUT. Which is not a problem, because their expiration value does
not need to be reset and thus they don't need space for one.

The only downside is that elements which stick to the set's default
timeout increase in size (for the copied set->timeout value), but this
patch series merges EXT_EXPIRATION and EXT_TIMEOUT anyway so their size
increases already. OTOH, non-expiring set elements won't need a spurious
EXT_TIMEOUT holding never expires marker.

Cheers, Phil

Re: [PATCH nf-next 7/8] netfilter: nf_tables: add never expires marker to elements

[Pablo Neira Ayuso]

On Tue, Aug 13, 2024 at 08:12:18PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Wed, Aug 07, 2024 at 04:23:56PM +0200, Pablo Neira Ayuso wrote:
> > This patch adds a timeout marker for those elements that never expire
> > when the element are created, so timeout updates are possible.
> > 
> > Note that maximum supported timeout in milliseconds which is conveyed
> > within a netlink attribute is 0x10c6f7a0b5ec which translates to
> > 0xffffffffffe85300 jiffies64, higher milliseconds values result in an
> > ERANGE error. Use U64_MAX as an internal marker to be stored in the set
> > element timeout field for permanent elements.
> > 
> > If userspace provides no timeout for an element, then the default set
> > timeout applies. However, if no default set timeout is specified and
> > timeout flag is set on, then such new element gets the never expires
> > marker.
> > 
> > Note that, in older kernels, it is already possible to define elements
> > that never expire by declaring a set with the set timeout flag set on
> > and no global set timeout, in this case, new element with no explicit
> > timeout never expire do not allocate the timeout extension, hence, they
> > never expire. This approach makes it complicated to accomodate element
> > timeout update, because element extensions do not support reallocations.
> > Therefore, allocate the timeout extension and use the new marker for
> > this case, but do not expose it to userspace to retain backward
> > compatibility in the set listing.
> 
> I fail to miss the point why this marker is needed at all:

Long story short: I did my best to support this without this marker
but I could not find a design that works without it.

> Right now, new set elements receive EXT_TIMEOUT upon creation if either
> NFTA_SET_ELEM_TIMEOUT is present (i.e., user specified per-element
> timeout) or set->timeout is non-zero (i.e., set has a default timeout).

There is one exception though:

 table inet x {
        set y {
                typeof ip saddr
                flags timeout
        }
 }

in this case, there is no default set timeout. Older kernels already
allow to add elements with no EXT_TIMEOUT that never expire with this
approach, however, this is not practical for element updates, because
set element extension reallocation is not supported.

> Why not change this logic and add EXT_TIMEOUT to all elements which will
> timeout and initialize it either to the user-defined value or the set's
> default? Then, only elements which don't timeout remain without
> EXT_TIMEOUT. Which is not a problem, because their expiration value does
> not need to be reset and thus they don't need space for one.

No EXT_TIMEOUT means users cannot update the timeout policy for such
element. I assume users can update from "timeout never" to "timeout 1h"
as a valid usecase.

> The only downside is that elements which stick to the set's default
> timeout increase in size (for the copied set->timeout value), but this
> patch series merges EXT_EXPIRATION and EXT_TIMEOUT anyway so their size
> increases already. OTOH, non-expiring set elements won't need a spurious
> EXT_TIMEOUT holding never expires marker.

Re: [PATCH nf-next 7/8] netfilter: nf_tables: add never expires marker to elements

[Phil Sutter]

On Tue, Aug 13, 2024 at 10:43:44PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 13, 2024 at 08:12:18PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Wed, Aug 07, 2024 at 04:23:56PM +0200, Pablo Neira Ayuso wrote:
> > > This patch adds a timeout marker for those elements that never expire
> > > when the element are created, so timeout updates are possible.
> > > 
> > > Note that maximum supported timeout in milliseconds which is conveyed
> > > within a netlink attribute is 0x10c6f7a0b5ec which translates to
> > > 0xffffffffffe85300 jiffies64, higher milliseconds values result in an
> > > ERANGE error. Use U64_MAX as an internal marker to be stored in the set
> > > element timeout field for permanent elements.
> > > 
> > > If userspace provides no timeout for an element, then the default set
> > > timeout applies. However, if no default set timeout is specified and
> > > timeout flag is set on, then such new element gets the never expires
> > > marker.
> > > 
> > > Note that, in older kernels, it is already possible to define elements
> > > that never expire by declaring a set with the set timeout flag set on
> > > and no global set timeout, in this case, new element with no explicit
> > > timeout never expire do not allocate the timeout extension, hence, they
> > > never expire. This approach makes it complicated to accomodate element
> > > timeout update, because element extensions do not support reallocations.
> > > Therefore, allocate the timeout extension and use the new marker for
> > > this case, but do not expose it to userspace to retain backward
> > > compatibility in the set listing.
> > 
> > I fail to miss the point why this marker is needed at all:
> 
> Long story short: I did my best to support this without this marker
> but I could not find a design that works without it.
> 
> > Right now, new set elements receive EXT_TIMEOUT upon creation if either
> > NFTA_SET_ELEM_TIMEOUT is present (i.e., user specified per-element
> > timeout) or set->timeout is non-zero (i.e., set has a default timeout).
> 
> There is one exception though:
> 
>  table inet x {
>         set y {
>                 typeof ip saddr
>                 flags timeout
>         }
>  }
> 
> in this case, there is no default set timeout. Older kernels already
> allow to add elements with no EXT_TIMEOUT that never expire with this
> approach, however, this is not practical for element updates, because
> set element extension reallocation is not supported.
> 
> > Why not change this logic and add EXT_TIMEOUT to all elements which will
> > timeout and initialize it either to the user-defined value or the set's
> > default? Then, only elements which don't timeout remain without
> > EXT_TIMEOUT. Which is not a problem, because their expiration value does
> > not need to be reset and thus they don't need space for one.
> 
> No EXT_TIMEOUT means users cannot update the timeout policy for such
> element. I assume users can update from "timeout never" to "timeout 1h"
> as a valid usecase.

Ah, that's the missing piece: I somehow assumed this should only support
resetting elements' timeouts, i.e. update only those elements which will
timeout already.

AFAICT, using UINT64_MAX as never-timeout marker is sane but can't one
use 0 instead? Set elems expire if EXT_TIMEOUT is present and 'timeout
!= 0'. This should simplify the code a bit, too because one may default
to set->timeout without checking the actual value.

Thanks, Phil

Re: [PATCH nf-next 7/8] netfilter: nf_tables: add never expires marker to elements

[Pablo Neira Ayuso]

On Wed, Aug 14, 2024 at 10:53:03AM +0200, Phil Sutter wrote:
> On Tue, Aug 13, 2024 at 10:43:44PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Aug 13, 2024 at 08:12:18PM +0200, Phil Sutter wrote:
> > > Hi Pablo,
> > > 
> > > On Wed, Aug 07, 2024 at 04:23:56PM +0200, Pablo Neira Ayuso wrote:
> > > > This patch adds a timeout marker for those elements that never expire
> > > > when the element are created, so timeout updates are possible.
> > > > 
> > > > Note that maximum supported timeout in milliseconds which is conveyed
> > > > within a netlink attribute is 0x10c6f7a0b5ec which translates to
> > > > 0xffffffffffe85300 jiffies64, higher milliseconds values result in an
> > > > ERANGE error. Use U64_MAX as an internal marker to be stored in the set
> > > > element timeout field for permanent elements.
> > > > 
> > > > If userspace provides no timeout for an element, then the default set
> > > > timeout applies. However, if no default set timeout is specified and
> > > > timeout flag is set on, then such new element gets the never expires
> > > > marker.
> > > > 
> > > > Note that, in older kernels, it is already possible to define elements
> > > > that never expire by declaring a set with the set timeout flag set on
> > > > and no global set timeout, in this case, new element with no explicit
> > > > timeout never expire do not allocate the timeout extension, hence, they
> > > > never expire. This approach makes it complicated to accomodate element
> > > > timeout update, because element extensions do not support reallocations.
> > > > Therefore, allocate the timeout extension and use the new marker for
> > > > this case, but do not expose it to userspace to retain backward
> > > > compatibility in the set listing.
> > > 
> > > I fail to miss the point why this marker is needed at all:
> > 
> > Long story short: I did my best to support this without this marker
> > but I could not find a design that works without it.
> > 
> > > Right now, new set elements receive EXT_TIMEOUT upon creation if either
> > > NFTA_SET_ELEM_TIMEOUT is present (i.e., user specified per-element
> > > timeout) or set->timeout is non-zero (i.e., set has a default timeout).
> > 
> > There is one exception though:
> > 
> >  table inet x {
> >         set y {
> >                 typeof ip saddr
> >                 flags timeout
> >         }
> >  }
> > 
> > in this case, there is no default set timeout. Older kernels already
> > allow to add elements with no EXT_TIMEOUT that never expire with this
> > approach, however, this is not practical for element updates, because
> > set element extension reallocation is not supported.
> > 
> > > Why not change this logic and add EXT_TIMEOUT to all elements which will
> > > timeout and initialize it either to the user-defined value or the set's
> > > default? Then, only elements which don't timeout remain without
> > > EXT_TIMEOUT. Which is not a problem, because their expiration value does
> > > not need to be reset and thus they don't need space for one.
> > 
> > No EXT_TIMEOUT means users cannot update the timeout policy for such
> > element. I assume users can update from "timeout never" to "timeout 1h"
> > as a valid usecase.
> 
> Ah, that's the missing piece: I somehow assumed this should only support
> resetting elements' timeouts, i.e. update only those elements which will
> timeout already.
> 
> AFAICT, using UINT64_MAX as never-timeout marker is sane but can't one
> use 0 instead? Set elems expire if EXT_TIMEOUT is present and 'timeout
> != 0'. This should simplify the code a bit, too because one may default
> to set->timeout without checking the actual value.

UINT64_MAX marker always reports ERANGE in older kernels so user knows
this is not supported, assuming new nft and old kernel, then in old
kernels:

- if "flags timeout" is used, that means "never expires" in sets which
  is correct.

- BUT if "timeout 1h" is used, then timeout 0 means use default set timeout
  which is makes behaviour different in old and new kernels for this.

I can explore using 0 as marker as you suggest if you think that
informing the user that this is not supported is not so important.

Note, though, that timeout updates are completely ignored in new
kernels, I don't have a way to report timeout updates are not
implemented. Older kernels follow a lazy approach.

Re: [PATCH nf-next 7/8] netfilter: nf_tables: add never expires marker to elements

[Phil Sutter]

On Wed, Aug 14, 2024 at 10:44:43PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Aug 14, 2024 at 10:53:03AM +0200, Phil Sutter wrote:
> > On Tue, Aug 13, 2024 at 10:43:44PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Aug 13, 2024 at 08:12:18PM +0200, Phil Sutter wrote:
> > > > Hi Pablo,
> > > > 
> > > > On Wed, Aug 07, 2024 at 04:23:56PM +0200, Pablo Neira Ayuso wrote:
> > > > > This patch adds a timeout marker for those elements that never expire
> > > > > when the element are created, so timeout updates are possible.
> > > > > 
> > > > > Note that maximum supported timeout in milliseconds which is conveyed
> > > > > within a netlink attribute is 0x10c6f7a0b5ec which translates to
> > > > > 0xffffffffffe85300 jiffies64, higher milliseconds values result in an
> > > > > ERANGE error. Use U64_MAX as an internal marker to be stored in the set
> > > > > element timeout field for permanent elements.
> > > > > 
> > > > > If userspace provides no timeout for an element, then the default set
> > > > > timeout applies. However, if no default set timeout is specified and
> > > > > timeout flag is set on, then such new element gets the never expires
> > > > > marker.
> > > > > 
> > > > > Note that, in older kernels, it is already possible to define elements
> > > > > that never expire by declaring a set with the set timeout flag set on
> > > > > and no global set timeout, in this case, new element with no explicit
> > > > > timeout never expire do not allocate the timeout extension, hence, they
> > > > > never expire. This approach makes it complicated to accomodate element
> > > > > timeout update, because element extensions do not support reallocations.
> > > > > Therefore, allocate the timeout extension and use the new marker for
> > > > > this case, but do not expose it to userspace to retain backward
> > > > > compatibility in the set listing.
> > > > 
> > > > I fail to miss the point why this marker is needed at all:
> > > 
> > > Long story short: I did my best to support this without this marker
> > > but I could not find a design that works without it.
> > > 
> > > > Right now, new set elements receive EXT_TIMEOUT upon creation if either
> > > > NFTA_SET_ELEM_TIMEOUT is present (i.e., user specified per-element
> > > > timeout) or set->timeout is non-zero (i.e., set has a default timeout).
> > > 
> > > There is one exception though:
> > > 
> > >  table inet x {
> > >         set y {
> > >                 typeof ip saddr
> > >                 flags timeout
> > >         }
> > >  }
> > > 
> > > in this case, there is no default set timeout. Older kernels already
> > > allow to add elements with no EXT_TIMEOUT that never expire with this
> > > approach, however, this is not practical for element updates, because
> > > set element extension reallocation is not supported.
> > > 
> > > > Why not change this logic and add EXT_TIMEOUT to all elements which will
> > > > timeout and initialize it either to the user-defined value or the set's
> > > > default? Then, only elements which don't timeout remain without
> > > > EXT_TIMEOUT. Which is not a problem, because their expiration value does
> > > > not need to be reset and thus they don't need space for one.
> > > 
> > > No EXT_TIMEOUT means users cannot update the timeout policy for such
> > > element. I assume users can update from "timeout never" to "timeout 1h"
> > > as a valid usecase.
> > 
> > Ah, that's the missing piece: I somehow assumed this should only support
> > resetting elements' timeouts, i.e. update only those elements which will
> > timeout already.
> > 
> > AFAICT, using UINT64_MAX as never-timeout marker is sane but can't one
> > use 0 instead? Set elems expire if EXT_TIMEOUT is present and 'timeout
> > != 0'. This should simplify the code a bit, too because one may default
> > to set->timeout without checking the actual value.
> 
> UINT64_MAX marker always reports ERANGE in older kernels so user knows
> this is not supported, assuming new nft and old kernel, then in old
> kernels:
> 
> - if "flags timeout" is used, that means "never expires" in sets which
>   is correct.
> 
> - BUT if "timeout 1h" is used, then timeout 0 means use default set timeout
>   which is makes behaviour different in old and new kernels for this.

When adding an element to a set with default timeout, there are three
cases:

1) use set's default timeout
2) provide per-element timeout
3) add persistent element

I'd suggest to implement these like so:

1) Do not add NFTA_SET_ELEM_TIMEOUT attribute
2) Provide NFTA_SET_ELEM_TIMEOUT attribute with custom value
3) Provide NFTA_SET_ELEM_TIMEOUT attribute with zero value

This should work with current kernel code already, right?

Updating an element's timeout value does not work without kernel
changes, of course. In current kernel code, any changes to EXT_TIMEOUT
(including removal) are ignored. Using UINT64_MAX as value for (3) will
provoke failure, but that only covers for making an existent element
persistent. Changing timeout value is still ignored, right?

> I can explore using 0 as marker as you suggest if you think that
> informing the user that this is not supported is not so important.

Looking at tests/shell/features/*.sh, there are other features we may
detect only by inspecting the resulting ruleset as well. I guess proper
detection would use a separate command requesting kernel nftables
"capabilities" (probably flags which identify distinct functionality).

> Note, though, that timeout updates are completely ignored in new
> kernels, I don't have a way to report timeout updates are not
> implemented. Older kernels follow a lazy approach.

Ah, that summarizes my remarks above. :)
I didn't look at older kernels.

Cheers, Phil