[PATCH nft 1/2] libnftables: drop gmp_init() and mp_set_memory_functions()

[PATCH nft 1/2] libnftables: drop gmp_init() and mp_set_memory_functions()

[Thomas Haller]

Setting global handles for libgmp via mp_set_memory_functions() is very
ugly. When we don't use mini-gmp, then potentially there are other users
of the library in the same process, and every process fighting about the
allocation functions is not gonna work.

It also means, we must not reset the allocation functions after somebody
already allocated GMP data with them. Which we cannot ensure, as we
don't know what other parts of the process are doing.

It's also unnecessary. The default allocation functions for gmp and
mini-gmp already abort the process on allocation failure ([1], [2]),
just like our xmalloc().

Just don't do this.

[1] https://gmplib.org/repo/gmp/file/8225bdfc499f/memory.c#l37
[2] https://git.netfilter.org/nftables/tree/src/mini-gmp.c?id=6d19a902c1d77cb51b940b1ce65f31b1cad38b74#n286

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/nftables.h |  1 -
 src/gmputil.c      | 10 ----------
 src/libnftables.c  |  1 -
 3 files changed, 12 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index b9b2b01c2689..4b7c335928da 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -224,7 +224,6 @@ struct input_descriptor {
 
 void ct_label_table_init(struct nft_ctx *ctx);
 void mark_table_init(struct nft_ctx *ctx);
-void gmp_init(void);
 void realm_table_rt_init(struct nft_ctx *ctx);
 void devgroup_table_init(struct nft_ctx *ctx);
 void xt_init(void);
diff --git a/src/gmputil.c b/src/gmputil.c
index 7f65630db59c..bf472c65de48 100644
--- a/src/gmputil.c
+++ b/src/gmputil.c
@@ -197,13 +197,3 @@ int mpz_vfprintf(FILE *fp, const char *f, va_list args)
 	return n;
 }
 #endif
-
-static void *gmp_xrealloc(void *ptr, size_t old_size, size_t new_size)
-{
-	return xrealloc(ptr, new_size);
-}
-
-void gmp_init(void)
-{
-	mp_set_memory_functions(xmalloc, gmp_xrealloc, NULL);
-}
diff --git a/src/libnftables.c b/src/libnftables.c
index c5f5729409d1..c34ee43de1fa 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -196,7 +196,6 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
 
 	if (!init_once) {
 		init_once = true;
-		gmp_init();
 #ifdef HAVE_LIBXTABLES
 		xt_init();
 #endif
-- 
2.41.0

[PATCH nft 2/2] libnftables: move init-once guard inside xt_init()

[Thomas Haller]

A library should not restrict being used by multiple threads or make
assumptions about how it's being used. Hence a "init_once" pattern
without no locking is racy, a code smell and should be avoided.

Note that libxtables is full of global variables and when linking against
it, libnftables cannot be used from multiple threads either. That is not
easy to fix.

Move the ugliness of "init_once" away from nft_ctx_new(), so that the
problem is concentrated closer to libxtables.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/libnftables.c |  6 +-----
 src/xt.c          | 15 +++++++++++++--
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index c34ee43de1fa..ed6b7fb5554c 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -191,15 +191,11 @@ void nft_ctx_clear_include_paths(struct nft_ctx *ctx)
 EXPORT_SYMBOL(nft_ctx_new);
 struct nft_ctx *nft_ctx_new(uint32_t flags)
 {
-	static bool init_once;
 	struct nft_ctx *ctx;
 
-	if (!init_once) {
-		init_once = true;
 #ifdef HAVE_LIBXTABLES
-		xt_init();
+	xt_init();
 #endif
-	}
 
 	ctx = xzalloc(sizeof(struct nft_ctx));
 	nft_init(ctx);
diff --git a/src/xt.c b/src/xt.c
index d774e07395a6..bb87e86e02af 100644
--- a/src/xt.c
+++ b/src/xt.c
@@ -361,7 +361,18 @@ static struct xtables_globals xt_nft_globals = {
 
 void xt_init(void)
 {
-	/* Default to IPv4, but this changes in runtime */
-	xtables_init_all(&xt_nft_globals, NFPROTO_IPV4);
+	static bool init_once;
+
+	if (!init_once) {
+		/* libxtables is full of global variables and cannot be used
+		 * concurrently by multiple threads. Hence, it's fine that the
+		 * "init_once" guard is not thread-safe either.
+		 * Don't link against xtables if you want thread safety.
+		 */
+		init_once = true;
+
+		/* Default to IPv4, but this changes in runtime */
+		xtables_init_all(&xt_nft_globals, NFPROTO_IPV4);
+	}
 }
 #endif
-- 
2.41.0

Re: [PATCH nft 2/2] libnftables: move init-once guard inside xt_init()

[Pablo Neira Ayuso]

On Tue, Sep 19, 2023 at 02:36:17PM +0200, Thomas Haller wrote:
> A library should not restrict being used by multiple threads or make
> assumptions about how it's being used. Hence a "init_once" pattern
> without no locking is racy, a code smell and should be avoided.
> 
> Note that libxtables is full of global variables and when linking against
> it, libnftables cannot be used from multiple threads either. That is not
> easy to fix.
> 
> Move the ugliness of "init_once" away from nft_ctx_new(), so that the
> problem is concentrated closer to libxtables.

Also applied, thanks

Re: [PATCH nft 1/2] libnftables: drop gmp_init() and mp_set_memory_functions()

[Pablo Neira Ayuso]

On Tue, Sep 19, 2023 at 02:36:16PM +0200, Thomas Haller wrote:
> Setting global handles for libgmp via mp_set_memory_functions() is very
> ugly. When we don't use mini-gmp, then potentially there are other users
> of the library in the same process, and every process fighting about the
> allocation functions is not gonna work.
> 
> It also means, we must not reset the allocation functions after somebody
> already allocated GMP data with them. Which we cannot ensure, as we
> don't know what other parts of the process are doing.
> 
> It's also unnecessary. The default allocation functions for gmp and
> mini-gmp already abort the process on allocation failure ([1], [2]),
> just like our xmalloc().
> 
> Just don't do this.

Applied, thanks