[PATCH nft 0/4] add operation cache for timestamp

[PATCH nft 0/4] add operation cache for timestamp

[Thomas Haller]

Add a cache for "time(NULL)" and tm_gmtoff from localtime_r(time(NULL), &tm).
The point is to ensure that one parse/output operation fetches the current time
and GMT offset at most once.

Follow up to ([1])

  Subject:  Re: [nft PATCH 2/2] meta: use reentrant localtime_r()/gmtime_r() functions
  Date:     Tue, 22 Aug 2023 17:15:14 +0200

[1] https://marc.info/?l=netfilter-devel&m=169271724629901&w=4

Thomas Haller (4):
  evaluate: add and use parse_ctx_init() helper method
  src: add ops_cache struct for caching information during parsing
  src: cache result of time() during parsing/output
  src: cache GMT offset for current time during parsing/output

 include/datatype.h | 22 ++++++++++++++++++++
 include/nftables.h |  3 +++
 src/datatype.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 src/evaluate.c     | 29 +++++++++++++++++++-------
 src/libnftables.c  | 17 +++++++++++++++
 src/meta.c         | 43 +++++++++++++++++++-------------------
 6 files changed, 136 insertions(+), 30 deletions(-)

-- 
2.41.0

[PATCH nft 1/4] evaluate: add and use parse_ctx_init() helper method

[Thomas Haller]

Another field to parse_ctx will be added, that should be initialized.
As initializing the parse_ctx struct gets more involved, move the
duplicated code to a separate function.

Having a dedicated function, also  makes it easier to grep of all the
places where a parse context gets set up.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/evaluate.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 1ae2ef0de10c..fdd2433b4780 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -40,6 +40,20 @@
 #include <utils.h>
 #include <xt.h>
 
+static struct parse_ctx *parse_ctx_init(struct parse_ctx *parse_ctx, const struct eval_ctx *ctx)
+{
+	struct parse_ctx tmp = {
+		.tbl	= &ctx->nft->output.tbl,
+		.input	= &ctx->nft->input,
+	};
+
+	/* "tmp" only exists, so we can search for "/struct parse_ctx .*=/" and find the location
+	 * where the parse context gets initialized. */
+
+	*parse_ctx = tmp;
+	return parse_ctx;
+}
+
 struct proto_ctx *eval_proto_ctx(struct eval_ctx *ctx)
 {
 	uint8_t idx = ctx->inner_desc ? 1 : 0;
@@ -278,15 +292,14 @@ static int flowtable_not_found(struct eval_ctx *ctx, const struct location *loc,
  */
 static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 {
-	struct parse_ctx parse_ctx = {
-		.tbl	= &ctx->nft->output.tbl,
-		.input	= &ctx->nft->input,
-	};
+	struct parse_ctx parse_ctx;
 	struct error_record *erec;
 	struct table *table;
 	struct set *set;
 	struct expr *new;
 
+	parse_ctx_init(&parse_ctx, ctx);
+
 	switch ((*expr)->symtype) {
 	case SYMBOL_VALUE:
 		datatype_set(*expr, ctx->ectx.dtype);
@@ -3454,13 +3467,12 @@ static int stmt_evaluate_reject_default(struct eval_ctx *ctx,
 
 static int stmt_evaluate_reject_icmp(struct eval_ctx *ctx, struct stmt *stmt)
 {
-	struct parse_ctx parse_ctx = {
-		.tbl	= &ctx->nft->output.tbl,
-		.input	= &ctx->nft->input,
-	};
+	struct parse_ctx parse_ctx;
 	struct error_record *erec;
 	struct expr *code;
 
+	parse_ctx_init(&parse_ctx, ctx);
+
 	erec = symbol_parse(&parse_ctx, stmt->reject.expr, &code);
 	if (erec != NULL) {
 		erec_queue(erec, ctx->msgs);
-- 
2.41.0

[PATCH nft 2/4] src: add ops_cache struct for caching information during parsing

[Thomas Haller]

The "ops_cache" will be used for caching the current timestamp
(time(NULL)) for the duration of one operation. It will ensure that all
decisions regarding the time, are based on the same timestamp.

Add the struct for that. The content will be added next.

There is already "struct nft_cache", but that seems to have a
different purpose. Hence, instead of extending "struct nft_cache",
add a new "struct ops_cache".

The difficulty is invalidating the cache and find the right places
to call nft_ctx_reset_ops_cache().

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/datatype.h |  8 ++++++++
 include/nftables.h |  3 +++
 src/evaluate.c     |  5 +++--
 src/libnftables.c  | 17 +++++++++++++++++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index 9ce7359cd340..79d996edd348 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -120,6 +120,13 @@ enum byteorder {
 
 struct expr;
 
+struct ops_cache {
+};
+
+#define CTX_CACHE_INIT() \
+	{ \
+	}
+
 /**
  * enum datatype_flags
  *
@@ -182,6 +189,7 @@ struct datatype *dtype_clone(const struct datatype *orig_dtype);
 struct parse_ctx {
 	struct symbol_tables	*tbl;
 	const struct input_ctx	*input;
+	struct ops_cache	*ops_cache;
 };
 
 extern struct error_record *symbol_parse(struct parse_ctx *ctx,
diff --git a/include/nftables.h b/include/nftables.h
index 219a10100206..b0a7f2f874ca 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -6,6 +6,7 @@
 #include <utils.h>
 #include <cache.h>
 #include <nftables/libnftables.h>
+#include <datatype.h>
 
 struct cookie {
 	FILE *fp;
@@ -47,6 +48,7 @@ struct output_ctx {
 		struct cookie error_cookie;
 	};
 	struct symbol_tables tbl;
+	struct ops_cache *ops_cache;
 };
 
 static inline bool nft_output_reversedns(const struct output_ctx *octx)
@@ -136,6 +138,7 @@ struct nft_ctx {
 	struct output_ctx	output;
 	bool			check;
 	struct nft_cache	cache;
+	struct ops_cache	ops_cache;
 	uint32_t		flags;
 	uint32_t		optimize_flags;
 	struct parser_state	*state;
diff --git a/src/evaluate.c b/src/evaluate.c
index fdd2433b4780..ea910786f3e4 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -43,8 +43,9 @@
 static struct parse_ctx *parse_ctx_init(struct parse_ctx *parse_ctx, const struct eval_ctx *ctx)
 {
 	struct parse_ctx tmp = {
-		.tbl	= &ctx->nft->output.tbl,
-		.input	= &ctx->nft->input,
+		.tbl		= &ctx->nft->output.tbl,
+		.input		= &ctx->nft->input,
+		.ops_cache	= &ctx->nft->ops_cache,
 	};
 
 	/* "tmp" only exists, so we can search for "/struct parse_ctx .*=/" and find the location
diff --git a/src/libnftables.c b/src/libnftables.c
index 9c802ec95f27..e520bac76dfa 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -19,6 +19,15 @@
 #include <stdlib.h>
 #include <string.h>
 
+static void nft_ctx_reset_ops_cache(struct nft_ctx *ctx)
+{
+	ctx->ops_cache = (struct ops_cache) CTX_CACHE_INIT();
+
+	/* The cache is also referenced by the output context. Set
+	 * up the pointer. */
+	ctx->output.ops_cache = &ctx->ops_cache;
+}
+
 static int nft_netlink(struct nft_ctx *nft,
 		       struct list_head *cmds, struct list_head *msgs)
 {
@@ -37,6 +46,8 @@ static int nft_netlink(struct nft_ctx *nft,
 	if (list_empty(cmds))
 		goto out;
 
+	nft_ctx_reset_ops_cache(nft);
+
 	batch_seqnum = mnl_batch_begin(ctx.batch, mnl_seqnum_alloc(&seqnum));
 	list_for_each_entry(cmd, cmds, list) {
 		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
@@ -522,6 +533,8 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 	unsigned int flags;
 	int err = 0;
 
+	nft_ctx_reset_ops_cache(nft);
+
 	filter = nft_cache_filter_init();
 	if (nft_cache_evaluate(nft, cmds, msgs, filter, &flags) < 0) {
 		nft_cache_filter_fini(filter);
@@ -630,6 +643,8 @@ err:
 	if (rc || nft->check)
 		nft_cache_release(&nft->cache);
 
+	nft_ctx_reset_ops_cache(nft);
+
 	return rc;
 }
 
@@ -740,6 +755,8 @@ err:
 
 	scope_release(nft->state->scopes[0]);
 
+	nft_ctx_reset_ops_cache(nft);
+
 	return rc;
 }
 
-- 
2.41.0

Re: [PATCH nft 2/4] src: add ops_cache struct for caching information during parsing

[Pablo Neira Ayuso]

On Fri, Aug 25, 2023 at 03:24:18PM +0200, Thomas Haller wrote:
> The "ops_cache" will be used for caching the current timestamp
> (time(NULL)) for the duration of one operation. It will ensure that all
> decisions regarding the time, are based on the same timestamp.
>
> Add the struct for that. The content will be added next.
> 
> There is already "struct nft_cache", but that seems to have a
> different purpose. Hence, instead of extending "struct nft_cache",
> add a new "struct ops_cache".

There is a "struct nft_cache" which stores objects from the kernel.

This new area is only used to store time related information.  I
prefer you simple call this time_cache or such, so it only wraps time
related information.

If there is a need to cache something else, new structures can be
added, no need to place all in ops_cache.

> The difficulty is invalidating the cache and find the right places
> to call nft_ctx_reset_ops_cache().

Could you explain the rationale to invalidate this time cache?

> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/datatype.h |  8 ++++++++
>  include/nftables.h |  3 +++
>  src/evaluate.c     |  5 +++--
>  src/libnftables.c  | 17 +++++++++++++++++
>  4 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/include/datatype.h b/include/datatype.h
> index 9ce7359cd340..79d996edd348 100644
> --- a/include/datatype.h
> +++ b/include/datatype.h
> @@ -120,6 +120,13 @@ enum byteorder {
>  
>  struct expr;
>  
> +struct ops_cache {
> +};
> +
> +#define CTX_CACHE_INIT() \
> +	{ \
> +	}
> +
>  /**
>   * enum datatype_flags
>   *
> @@ -182,6 +189,7 @@ struct datatype *dtype_clone(const struct datatype *orig_dtype);
>  struct parse_ctx {
>  	struct symbol_tables	*tbl;
>  	const struct input_ctx	*input;
> +	struct ops_cache	*ops_cache;
>  };
>  
>  extern struct error_record *symbol_parse(struct parse_ctx *ctx,
> diff --git a/include/nftables.h b/include/nftables.h
> index 219a10100206..b0a7f2f874ca 100644
> --- a/include/nftables.h
> +++ b/include/nftables.h
> @@ -6,6 +6,7 @@
>  #include <utils.h>
>  #include <cache.h>
>  #include <nftables/libnftables.h>
> +#include <datatype.h>
>  
>  struct cookie {
>  	FILE *fp;
> @@ -47,6 +48,7 @@ struct output_ctx {
>  		struct cookie error_cookie;
>  	};
>  	struct symbol_tables tbl;
> +	struct ops_cache *ops_cache;
>  };
>  
>  static inline bool nft_output_reversedns(const struct output_ctx *octx)
> @@ -136,6 +138,7 @@ struct nft_ctx {
>  	struct output_ctx	output;
>  	bool			check;
>  	struct nft_cache	cache;
> +	struct ops_cache	ops_cache;
>  	uint32_t		flags;
>  	uint32_t		optimize_flags;
>  	struct parser_state	*state;
> diff --git a/src/evaluate.c b/src/evaluate.c
> index fdd2433b4780..ea910786f3e4 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -43,8 +43,9 @@
>  static struct parse_ctx *parse_ctx_init(struct parse_ctx *parse_ctx, const struct eval_ctx *ctx)
>  {
>  	struct parse_ctx tmp = {
> -		.tbl	= &ctx->nft->output.tbl,
> -		.input	= &ctx->nft->input,
> +		.tbl		= &ctx->nft->output.tbl,
> +		.input		= &ctx->nft->input,
> +		.ops_cache	= &ctx->nft->ops_cache,
>  	};
>  
>  	/* "tmp" only exists, so we can search for "/struct parse_ctx .*=/" and find the location
> diff --git a/src/libnftables.c b/src/libnftables.c
> index 9c802ec95f27..e520bac76dfa 100644
> --- a/src/libnftables.c
> +++ b/src/libnftables.c
> @@ -19,6 +19,15 @@
>  #include <stdlib.h>
>  #include <string.h>
>  
> +static void nft_ctx_reset_ops_cache(struct nft_ctx *ctx)
> +{
> +	ctx->ops_cache = (struct ops_cache) CTX_CACHE_INIT();
> +
> +	/* The cache is also referenced by the output context. Set
> +	 * up the pointer. */
> +	ctx->output.ops_cache = &ctx->ops_cache;
> +}
> +
>  static int nft_netlink(struct nft_ctx *nft,
>  		       struct list_head *cmds, struct list_head *msgs)
>  {
> @@ -37,6 +46,8 @@ static int nft_netlink(struct nft_ctx *nft,
>  	if (list_empty(cmds))
>  		goto out;
>  
> +	nft_ctx_reset_ops_cache(nft);
> +
>  	batch_seqnum = mnl_batch_begin(ctx.batch, mnl_seqnum_alloc(&seqnum));
>  	list_for_each_entry(cmd, cmds, list) {
>  		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
> @@ -522,6 +533,8 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
>  	unsigned int flags;
>  	int err = 0;
>  
> +	nft_ctx_reset_ops_cache(nft);
> +
>  	filter = nft_cache_filter_init();
>  	if (nft_cache_evaluate(nft, cmds, msgs, filter, &flags) < 0) {
>  		nft_cache_filter_fini(filter);
> @@ -630,6 +643,8 @@ err:
>  	if (rc || nft->check)
>  		nft_cache_release(&nft->cache);
>  
> +	nft_ctx_reset_ops_cache(nft);
> +
>  	return rc;
>  }
>  
> @@ -740,6 +755,8 @@ err:
>  
>  	scope_release(nft->state->scopes[0]);
>  
> +	nft_ctx_reset_ops_cache(nft);
> +
>  	return rc;
>  }
>  
> -- 
> 2.41.0
> 

[PATCH nft 3/4] src: cache result of time() during parsing/output

[Thomas Haller]

When we parse/output a larger set of data, we should only call time()
once. With every call of time(), the value keeps ticking (and is subject
to time reset). Previously, one parse/output operation will make
decisions on potentially different timestamps.

Add a cache to the parse/output context, and only fetch time() once
per operation.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/datatype.h |  6 ++++++
 src/datatype.c     | 16 ++++++++++++++++
 src/meta.c         |  4 ++--
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index 79d996edd348..abd093765703 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -2,6 +2,7 @@
 #define NFTABLES_DATATYPE_H
 
 #include <json.h>
+#include <time.h>
 
 /**
  * enum datatypes
@@ -121,12 +122,17 @@ enum byteorder {
 struct expr;
 
 struct ops_cache {
+	time_t		time;
+	bool		has_time;
 };
 
 #define CTX_CACHE_INIT() \
 	{ \
+		.has_time = false, \
 	}
 
+extern time_t ops_cache_get_time(struct ops_cache *cache);
+
 /**
  * enum datatype_flags
  *
diff --git a/src/datatype.c b/src/datatype.c
index dd6a5fbf5df8..933d832c4f4d 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -35,6 +35,22 @@
 
 #include <netinet/ip_icmp.h>
 
+time_t ops_cache_get_time(struct ops_cache *cache)
+{
+	time_t t;
+
+	if (!cache || !cache->has_time) {
+		t = time(NULL);
+		if (cache) {
+			cache->has_time = true;
+			cache->time = time(NULL);
+		}
+	} else
+		t = cache->time;
+
+	return t;
+}
+
 static const struct datatype *datatypes[TYPE_MAX + 1] = {
 	[TYPE_INVALID]		= &invalid_type,
 	[TYPE_VERDICT]		= &verdict_type,
diff --git a/src/meta.c b/src/meta.c
index 4f383269d032..1d853b219fe6 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -496,7 +496,7 @@ static void hour_type_print(const struct expr *expr, struct output_ctx *octx)
 	time_t ts;
 
 	/* Obtain current tm, so that we can add tm_gmtoff */
-	ts = time(NULL);
+	ts = ops_cache_get_time(octx->ops_cache);
 	if (ts != ((time_t) -1) && localtime_r(&ts, &cur_tm))
 		seconds = (seconds + cur_tm.tm_gmtoff) % SECONDS_PER_DAY;
 
@@ -534,7 +534,7 @@ static struct error_record *hour_type_parse(struct parse_ctx *ctx,
 	result = 0;
 
 	/* Obtain current tm, so that we can substract tm_gmtoff */
-	ts = time(NULL);
+	ts = ops_cache_get_time(ctx->ops_cache);
 	if (ts != ((time_t) -1) && localtime_r(&ts, &cur_tm_data))
 		cur_tm = &cur_tm_data;
 	else
-- 
2.41.0

Re: [PATCH nft 3/4] src: cache result of time() during parsing/output

[Pablo Neira Ayuso]

On Fri, Aug 25, 2023 at 03:24:19PM +0200, Thomas Haller wrote:
> When we parse/output a larger set of data, we should only call time()
> once. With every call of time(), the value keeps ticking (and is subject
> to time reset). Previously, one parse/output operation will make
> decisions on potentially different timestamps.
> 
> Add a cache to the parse/output context, and only fetch time() once
> per operation.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/datatype.h |  6 ++++++
>  src/datatype.c     | 16 ++++++++++++++++
>  src/meta.c         |  4 ++--
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/datatype.h b/include/datatype.h
> index 79d996edd348..abd093765703 100644
> --- a/include/datatype.h
> +++ b/include/datatype.h
> @@ -2,6 +2,7 @@
>  #define NFTABLES_DATATYPE_H
>  
>  #include <json.h>
> +#include <time.h>
>  
>  /**
>   * enum datatypes
> @@ -121,12 +122,17 @@ enum byteorder {
>  struct expr;
>  
>  struct ops_cache {
> +	time_t		time;
> +	bool		has_time;
>  };
>  
>  #define CTX_CACHE_INIT() \
>  	{ \
> +		.has_time = false, \
>  	}
>  
> +extern time_t ops_cache_get_time(struct ops_cache *cache);
> +
>  /**
>   * enum datatype_flags
>   *
> diff --git a/src/datatype.c b/src/datatype.c
> index dd6a5fbf5df8..933d832c4f4d 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -35,6 +35,22 @@
>  
>  #include <netinet/ip_icmp.h>
>  
> +time_t ops_cache_get_time(struct ops_cache *cache)
> +{
> +	time_t t;
> +
> +	if (!cache || !cache->has_time) {
> +		t = time(NULL);
> +		if (cache) {
> +			cache->has_time = true;
> +			cache->time = time(NULL);
> +		}
> +	} else
> +		t = cache->time;
> +
> +	return t;
> +}
> +
>  static const struct datatype *datatypes[TYPE_MAX + 1] = {
>  	[TYPE_INVALID]		= &invalid_type,
>  	[TYPE_VERDICT]		= &verdict_type,
> diff --git a/src/meta.c b/src/meta.c
> index 4f383269d032..1d853b219fe6 100644
> --- a/src/meta.c
> +++ b/src/meta.c
> @@ -496,7 +496,7 @@ static void hour_type_print(const struct expr *expr, struct output_ctx *octx)
>  	time_t ts;
>  
>  	/* Obtain current tm, so that we can add tm_gmtoff */
> -	ts = time(NULL);
> +	ts = ops_cache_get_time(octx->ops_cache);

Following the idea of adding a specific time cache, I'd suggest:

        ts = nft_time_get(...);

or similar.

>  	if (ts != ((time_t) -1) && localtime_r(&ts, &cur_tm))
>  		seconds = (seconds + cur_tm.tm_gmtoff) % SECONDS_PER_DAY;
>  
> @@ -534,7 +534,7 @@ static struct error_record *hour_type_parse(struct parse_ctx *ctx,
>  	result = 0;
>  
>  	/* Obtain current tm, so that we can substract tm_gmtoff */
> -	ts = time(NULL);
> +	ts = ops_cache_get_time(ctx->ops_cache);
>  	if (ts != ((time_t) -1) && localtime_r(&ts, &cur_tm_data))
>  		cur_tm = &cur_tm_data;
>  	else
> -- 
> 2.41.0
> 

[PATCH nft 4/4] src: cache GMT offset for current time during parsing/output

[Thomas Haller]

localtime_r() is allowed to call tzset() every time. If it would, we
might pick up timezone changes in the middle of processing a larger
operation. Instead, we want that all decisions that we make for one
operation, are based on the same timezone.

In practice, current glibc/musl does not call tzset() for localtime_r(),
if the timezone is already initialized.  That brings the opposite
problem. We want to refresh the timezone before the operation starts.

Not long ago, we used localtime() instead of localtime_r(). localtime()
would always implicitly call tzset(), so that nftables may change the
timezone, was already happening before.

Cache the GMT offset in ops_cache and call tzset(). This ensures that
the timezone is refreshed at least once per operation (and possibly also
at most once).

There are still two calls for localtime_r() left, that are used to
determine the GTM offset for an arbitrary timestamp other than
time(NULL). The GTM offset depends on the timestamp, so we cannot cache
it. Still, also call ops_cache_get_time_gmtoff(), which ensures that tzset()
gets called once.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/datatype.h |  8 ++++++++
 src/datatype.c     | 36 ++++++++++++++++++++++++++++++++++++
 src/meta.c         | 43 +++++++++++++++++++++----------------------
 3 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index abd093765703..00f0a0123326 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -123,15 +123,23 @@ struct expr;
 
 struct ops_cache {
 	time_t		time;
+
+	/* the GMT offset obtained from localtime(time). */
+	long int	gmtoff;
+
 	bool		has_time;
+	bool		has_gmtoff;
+	bool		good_gmtoff;
 };
 
 #define CTX_CACHE_INIT() \
 	{ \
 		.has_time = false, \
+		.has_gmtoff = false, \
 	}
 
 extern time_t ops_cache_get_time(struct ops_cache *cache);
+extern bool ops_cache_get_time_gmtoff(struct ops_cache *cache, long int *out_gmtoff);
 
 /**
  * enum datatype_flags
diff --git a/src/datatype.c b/src/datatype.c
index 933d832c4f4d..07f1e3474b7a 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -51,6 +51,42 @@ time_t ops_cache_get_time(struct ops_cache *cache)
 	return t;
 }
 
+bool ops_cache_get_time_gmtoff(struct ops_cache *cache, long int *out_gmtoff)
+{
+	long int gmtoff = 0;
+	bool good = false;
+
+	if (!cache || !cache->has_gmtoff) {
+		time_t now;
+
+		now = ops_cache_get_time(cache);
+		if (now != (time_t) -1) {
+			struct tm tm;
+
+			/* localtime_r() is not guaranteed to call tzset() (unlike localtime()).
+			 * Call it ourselves, so we possibly have the currently set timezone. */
+			tzset();
+
+			if (localtime_r(&now, &tm)) {
+				gmtoff = tm.tm_gmtoff;
+				good = true;
+			}
+		}
+
+		if (cache) {
+			cache->good_gmtoff = good;
+			cache->gmtoff = gmtoff;
+		}
+	} else {
+		gmtoff = cache->gmtoff;
+		good = cache->good_gmtoff;
+	}
+
+	if (out_gmtoff)
+		*out_gmtoff = gmtoff;
+	return good;
+}
+
 static const struct datatype *datatypes[TYPE_MAX + 1] = {
 	[TYPE_INVALID]		= &invalid_type,
 	[TYPE_VERDICT]		= &verdict_type,
diff --git a/src/meta.c b/src/meta.c
index 1d853b219fe6..881fbc038775 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -391,6 +391,9 @@ static void date_type_print(const struct expr *expr, struct output_ctx *octx)
 	/* Convert from nanoseconds to seconds */
 	tstamp64 /= 1000000000L;
 
+	/* ops_cache_get_time_gmtoff() will call tzset() for us (once). */
+	ops_cache_get_time_gmtoff(octx->ops_cache, NULL);
+
 	/* Obtain current tm, to add tm_gmtoff to the timestamp */
 	tstamp = tstamp64;
 	if (localtime_r(&tstamp, &tm))
@@ -404,7 +407,7 @@ static void date_type_print(const struct expr *expr, struct output_ctx *octx)
 		nft_print(octx, "Error converting timestamp to printed time");
 }
 
-static bool parse_iso_date(uint64_t *tstamp, const char *sym)
+static bool parse_iso_date(struct parse_ctx *ctx, uint64_t *tstamp, const char *sym)
 {
 	struct tm cur_tm;
 	struct tm tm;
@@ -422,6 +425,9 @@ static bool parse_iso_date(uint64_t *tstamp, const char *sym)
 	return false;
 
 success:
+	/* ops_cache_get_time_gmtoff() will call tzset() for us (once). */
+	ops_cache_get_time_gmtoff(ctx->ops_cache, NULL);
+
 	/*
 	 * Overwriting TZ is problematic if we're parsing hour types in this same process,
 	 * hence I'd rather use timegm() which doesn't take into account the TZ env variable,
@@ -449,7 +455,7 @@ static struct error_record *date_type_parse(struct parse_ctx *ctx,
 	const char *endptr = sym->identifier;
 	uint64_t tstamp;
 
-	if (parse_iso_date(&tstamp, sym->identifier))
+	if (parse_iso_date(ctx, &tstamp, sym->identifier))
 		goto success;
 
 	tstamp = strtoul(sym->identifier, (char **) &endptr, 10);
@@ -492,13 +498,11 @@ static void day_type_print(const struct expr *expr, struct output_ctx *octx)
 static void hour_type_print(const struct expr *expr, struct output_ctx *octx)
 {
 	uint32_t seconds = mpz_get_uint32(expr->value), minutes, hours;
-	struct tm cur_tm;
-	time_t ts;
+	long int gmtoff;
 
-	/* Obtain current tm, so that we can add tm_gmtoff */
-	ts = ops_cache_get_time(octx->ops_cache);
-	if (ts != ((time_t) -1) && localtime_r(&ts, &cur_tm))
-		seconds = (seconds + cur_tm.tm_gmtoff) % SECONDS_PER_DAY;
+	/* Add current offset of localtime() to GMT. */
+	if (ops_cache_get_time_gmtoff(octx->ops_cache, &gmtoff))
+		seconds = (seconds + gmtoff) % SECONDS_PER_DAY;
 
 	minutes = seconds / 60;
 	seconds %= 60;
@@ -516,13 +520,12 @@ static struct error_record *hour_type_parse(struct parse_ctx *ctx,
 					    struct expr **res)
 {
 	struct error_record *er;
-	struct tm cur_tm_data;
-	struct tm *cur_tm;
+	long int gmtoff;
+	bool has_gmtoff;
 	uint32_t result;
 	uint64_t tmp;
 	char *endptr;
 	struct tm tm;
-	time_t ts;
 
 	memset(&tm, 0, sizeof(struct tm));
 
@@ -533,12 +536,8 @@ static struct error_record *hour_type_parse(struct parse_ctx *ctx,
 
 	result = 0;
 
-	/* Obtain current tm, so that we can substract tm_gmtoff */
-	ts = ops_cache_get_time(ctx->ops_cache);
-	if (ts != ((time_t) -1) && localtime_r(&ts, &cur_tm_data))
-		cur_tm = &cur_tm_data;
-	else
-		cur_tm = NULL;
+	/* Obtain current offset of localtime() from GTM. */
+	has_gmtoff = ops_cache_get_time_gmtoff(ctx->ops_cache, &gmtoff);
 
 	endptr = strptime(sym->identifier, "%T", &tm);
 	if (endptr && *endptr == '\0')
@@ -563,12 +562,12 @@ convert:
 	if (result == 0)
 		result = tm.tm_hour * 3600 + tm.tm_min * 60 + tm.tm_sec;
 
-	/* Substract tm_gmtoff to get the current time */
-	if (cur_tm) {
-		if ((long int) result >= cur_tm->tm_gmtoff)
-			result = (result - cur_tm->tm_gmtoff) % 86400;
+	/* Substract gmtoff to get the current time */
+	if (has_gmtoff) {
+		if ((long int) result >= gmtoff)
+			result = (result - gmtoff) % 86400;
 		else
-			result = 86400 - cur_tm->tm_gmtoff + result;
+			result = 86400 - gmtoff + result;
 	}
 
 success:
-- 
2.41.0

Re: [PATCH nft 0/4] add operation cache for timestamp

[Pablo Neira Ayuso]

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

On Fri, Aug 25, 2023 at 03:24:16PM +0200, Thomas Haller wrote:
> Add a cache for "time(NULL)" and tm_gmtoff from localtime_r(time(NULL), &tm).
> The point is to ensure that one parse/output operation fetches the current time
> and GMT offset at most once.
> 
> Follow up to ([1])
> 
>   Subject:  Re: [nft PATCH 2/2] meta: use reentrant localtime_r()/gmtime_r() functions
>   Date:     Tue, 22 Aug 2023 17:15:14 +0200
> 
> [1] https://marc.info/?l=netfilter-devel&m=169271724629901&w=4

To extend what I said yesterday. It would be great if you could
validate that we have sufficient tests for time support.

Probably you can use this ruleset that I am attaching as reference and
think of a ruleset to cover this? I am attaching an example ruleset
which is basically a "timetable" using nftables sets/maps.

[-- Attachment #2: schedules.nft --]
[-- Type: text/plain, Size: 768 bytes --]

table netdev filter {
	map ether_to_chain {
		typeof ether saddr : verdict
		elements = { 96:68:97:a7:e8:a7 comment "Device match" : jump fw_p0_dev0 }
	}

	map schedule_time {
		typeof meta time : verdict
		flags interval
		counter
		elements = { "2022-10-09 18:46:50" - "2022-10-09 19:16:50" comment "!Schedule OFFLINE override" : drop }
	}

	map schedule_day {
		typeof meta day . meta hour : verdict
		flags interval
		counter
		elements = { "Tuesday" . "06:00" - "07:00" : drop }
	}

	chain fw_p0_dev0 {
		meta time vmap @schedule_time
		meta day . meta hour vmap @schedule_day
	}

	chain my_devices_rules {
		ether saddr vmap @ether_to_chain
	}

	chain ingress {
		type filter hook ingress device eth0 priority filter; policy accept;
		jump my_devices_rules
	}
}