[PATCH nft 1/2] utils: add memory_allocation_check() helper

[PATCH nft 1/2] utils: add memory_allocation_check() helper

[Thomas Haller]

libnftables kills the process on out of memory (xmalloc()), so
when we use libraries that propagate ENOMEM to libnftables, we
also abort the process.

For example:

     nlr = nftnl_rule_alloc();
     if (!nlr)
          memory_allocation_error();

Add memory_allocation_check() macro which can simplify this common
check to:

     nlr = memory_allocation_check(nftnl_rule_alloc());

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/utils.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/utils.h b/include/utils.h
index 36a28f893667..fcd7c598fe9f 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -142,6 +142,16 @@ extern void __memory_allocation_error(const char *filename, uint32_t line) __nor
 #define memory_allocation_error()		\
 	__memory_allocation_error(__FILE__, __LINE__);
 
+#define memory_allocation_check(cmd)               \
+	({                                         \
+		typeof((cmd)) _v = (cmd);          \
+		const void *const _v2 = _v;        \
+                                                   \
+		if (!_v2)                          \
+			memory_allocation_error(); \
+		_v;                                \
+	})
+
 extern void xfree(const void *ptr);
 extern void *xmalloc(size_t size);
 extern void *xmalloc_array(size_t nmemb, size_t size);
-- 
2.41.0

[PATCH nft 2/2] netlink: add and use _nftnl_udata_buf_alloc() helper

[Thomas Haller]

We don't want to handle allocation errors, but crash via memory_allocation_error().
Also, we usually just allocate NFT_USERDATA_MAXLEN buffers.

Add a helper for that and use it.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 include/netlink.h       |  3 +++
 src/mnl.c               | 16 ++++------------
 src/netlink.c           |  7 ++-----
 src/netlink_linearize.c |  4 +---
 4 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/include/netlink.h b/include/netlink.h
index 6766d7e8563f..15cbb332c8dd 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -260,4 +260,7 @@ struct nft_expr_loc *nft_expr_loc_find(const struct nftnl_expr *nle,
 
 struct dl_proto_ctx *dl_proto_ctx(struct rule_pp_ctx *ctx);
 
+#define _nftnl_udata_buf_alloc() \
+	memory_allocation_check(nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN))
+
 #endif /* NFTABLES_NETLINK_H */
diff --git a/src/mnl.c b/src/mnl.c
index 0fb36bd588ee..1263c611cd20 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -823,9 +823,7 @@ int mnl_nft_chain_add(struct netlink_ctx *ctx, struct cmd *cmd,
 					    CHAIN_F_HW_OFFLOAD);
 		}
 		if (cmd->chain->comment) {
-			udbuf = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
-			if (!udbuf)
-				memory_allocation_error();
+			udbuf = _nftnl_udata_buf_alloc();
 			if (!nftnl_udata_put_strz(udbuf, NFTNL_UDATA_CHAIN_COMMENT, cmd->chain->comment))
 				memory_allocation_error();
 			nftnl_chain_set_data(nlc, NFTNL_CHAIN_USERDATA, nftnl_udata_buf_data(udbuf),
@@ -1057,9 +1055,7 @@ int mnl_nft_table_add(struct netlink_ctx *ctx, struct cmd *cmd,
 		nftnl_table_set_u32(nlt, NFTNL_TABLE_FLAGS, cmd->table->flags);
 
 		if (cmd->table->comment) {
-			udbuf = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
-			if (!udbuf)
-				memory_allocation_error();
+			udbuf = _nftnl_udata_buf_alloc();
 			if (!nftnl_udata_put_strz(udbuf, NFTNL_UDATA_TABLE_COMMENT, cmd->table->comment))
 				memory_allocation_error();
 			nftnl_table_set_data(nlt, NFTNL_TABLE_USERDATA, nftnl_udata_buf_data(udbuf),
@@ -1256,9 +1252,7 @@ int mnl_nft_set_add(struct netlink_ctx *ctx, struct cmd *cmd,
 		nftnl_set_set_u32(nls, NFTNL_SET_DESC_SIZE, set->init->size);
 	}
 
-	udbuf = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
-	if (!udbuf)
-		memory_allocation_error();
+	udbuf = _nftnl_udata_buf_alloc();
 	if (!nftnl_udata_put_u32(udbuf, NFTNL_UDATA_SET_KEYBYTEORDER,
 				 set->key->byteorder))
 		memory_allocation_error();
@@ -1453,9 +1447,7 @@ int mnl_nft_obj_add(struct netlink_ctx *ctx, struct cmd *cmd,
 	nftnl_obj_set_u32(nlo, NFTNL_OBJ_TYPE, obj->type);
 
 	if (obj->comment) {
-		udbuf = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
-		if (!udbuf)
-			memory_allocation_error();
+		udbuf = _nftnl_udata_buf_alloc();
 		if (!nftnl_udata_put_strz(udbuf, NFTNL_UDATA_OBJ_COMMENT, obj->comment))
 			memory_allocation_error();
 		nftnl_obj_set_data(nlo, NFTNL_OBJ_USERDATA, nftnl_udata_buf_data(udbuf),
diff --git a/src/netlink.c b/src/netlink.c
index 120a8ba9ceb1..0c858065ca15 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -175,11 +175,8 @@ struct nftnl_set_elem *alloc_nftnl_setelem(const struct expr *set,
 						netlink_gen_stmt_stateful(stmt));
 		}
 	}
-	if (elem->comment || expr->elem_flags) {
-		udbuf = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
-		if (!udbuf)
-			memory_allocation_error();
-	}
+	if (elem->comment || expr->elem_flags)
+		udbuf = _nftnl_udata_buf_alloc();
 	if (elem->comment) {
 		if (!nftnl_udata_put_strz(udbuf, NFTNL_UDATA_SET_ELEM_COMMENT,
 					  elem->comment))
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 0c62341112d8..b5adc4d186c8 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -1760,9 +1760,7 @@ void netlink_linearize_rule(struct netlink_ctx *ctx,
 	if (rule->comment) {
 		struct nftnl_udata_buf *udata;
 
-		udata = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
-		if (!udata)
-			memory_allocation_error();
+		udata = _nftnl_udata_buf_alloc();
 
 		if (!nftnl_udata_put_strz(udata, NFTNL_UDATA_RULE_COMMENT,
 					  rule->comment))
-- 
2.41.0

Re: [PATCH nft 2/2] netlink: add and use _nftnl_udata_buf_alloc() helper

[Pablo Neira Ayuso]

On Wed, Nov 08, 2023 at 07:24:25PM +0100, Thomas Haller wrote:
> We don't want to handle allocation errors, but crash via memory_allocation_error().
> Also, we usually just allocate NFT_USERDATA_MAXLEN buffers.
> 
> Add a helper for that and use it.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/netlink.h       |  3 +++
>  src/mnl.c               | 16 ++++------------
>  src/netlink.c           |  7 ++-----
>  src/netlink_linearize.c |  4 +---
>  4 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/include/netlink.h b/include/netlink.h
> index 6766d7e8563f..15cbb332c8dd 100644
> --- a/include/netlink.h
> +++ b/include/netlink.h
> @@ -260,4 +260,7 @@ struct nft_expr_loc *nft_expr_loc_find(const struct nftnl_expr *nle,
>  
>  struct dl_proto_ctx *dl_proto_ctx(struct rule_pp_ctx *ctx);
>  
> +#define _nftnl_udata_buf_alloc() \
> +	memory_allocation_check(nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN))

Add a wrapper function, no macro.

Re: [PATCH nft 2/2] netlink: add and use _nftnl_udata_buf_alloc() helper

[Thomas Haller]

On Thu, 2023-11-09 at 16:14 +0100, Pablo Neira Ayuso wrote:
> On Wed, Nov 08, 2023 at 07:24:25PM +0100, Thomas Haller wrote:
> > We don't want to handle allocation errors, but crash via
> > memory_allocation_error().
> > Also, we usually just allocate NFT_USERDATA_MAXLEN buffers.
> > 
> > Add a helper for that and use it.
> > 
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > ---
> >  include/netlink.h       |  3 +++
> >  src/mnl.c               | 16 ++++------------
> >  src/netlink.c           |  7 ++-----
> >  src/netlink_linearize.c |  4 +---
> >  4 files changed, 10 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/netlink.h b/include/netlink.h
> > index 6766d7e8563f..15cbb332c8dd 100644
> > --- a/include/netlink.h
> > +++ b/include/netlink.h
> > @@ -260,4 +260,7 @@ struct nft_expr_loc *nft_expr_loc_find(const
> > struct nftnl_expr *nle,
> >  
> >  struct dl_proto_ctx *dl_proto_ctx(struct rule_pp_ctx *ctx);
> >  
> > +#define _nftnl_udata_buf_alloc() \
> > +	memory_allocation_check(nftnl_udata_buf_alloc(NFT_USERDATA
> > _MAXLEN))
> 
> Add a wrapper function, no macro.
> 

Hi,

memory_allocation_error() is itself a macro, as it uses
__FILE__,__LINE__

This is also a macro, to preserve those parameters.

Thomas

Re: [PATCH nft 2/2] netlink: add and use _nftnl_udata_buf_alloc() helper

[Pablo Neira Ayuso]

On Thu, Nov 09, 2023 at 04:19:29PM +0100, Thomas Haller wrote:
> On Thu, 2023-11-09 at 16:14 +0100, Pablo Neira Ayuso wrote:
> > On Wed, Nov 08, 2023 at 07:24:25PM +0100, Thomas Haller wrote:
> > > We don't want to handle allocation errors, but crash via
> > > memory_allocation_error().
> > > Also, we usually just allocate NFT_USERDATA_MAXLEN buffers.
> > > 
> > > Add a helper for that and use it.
> > > 
> > > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > > ---
> > >  include/netlink.h       |  3 +++
> > >  src/mnl.c               | 16 ++++------------
> > >  src/netlink.c           |  7 ++-----
> > >  src/netlink_linearize.c |  4 +---
> > >  4 files changed, 10 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/include/netlink.h b/include/netlink.h
> > > index 6766d7e8563f..15cbb332c8dd 100644
> > > --- a/include/netlink.h
> > > +++ b/include/netlink.h
> > > @@ -260,4 +260,7 @@ struct nft_expr_loc *nft_expr_loc_find(const
> > > struct nftnl_expr *nle,
> > >  
> > >  struct dl_proto_ctx *dl_proto_ctx(struct rule_pp_ctx *ctx);
> > >  
> > > +#define _nftnl_udata_buf_alloc() \
> > > +	memory_allocation_check(nftnl_udata_buf_alloc(NFT_USERDATA
> > > _MAXLEN))
> > 
> > Add a wrapper function, no macro.
> > 
> 
> Hi,
> 
> memory_allocation_error() is itself a macro, as it uses
> __FILE__,__LINE__

In this case above, __FILE__ and __LINE__ does not provide much
information?

nftnl_expr_alloc() returns NULL when support for an expression is
missing in libnftnl, that provides a hint on that, this is very rare
and it can only happen when developing support for new expressions.

Maybe simply say __func__ instead to know what function has failed
when performing the memory allocation is a hint that is fine enough.

> This is also a macro, to preserve those parameters.
> 
> Thomas
> 

Re: [PATCH nft 2/2] netlink: add and use _nftnl_udata_buf_alloc() helper

[Thomas Haller]

On Thu, 2023-11-09 at 16:32 +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 09, 2023 at 04:19:29PM +0100, Thomas Haller wrote:
> > On Thu, 2023-11-09 at 16:14 +0100, Pablo Neira Ayuso wrote:
> > > 
> > > Add a wrapper function, no macro.
> > > 
> > 
> > 
> > memory_allocation_error() is itself a macro, as it uses
> > __FILE__,__LINE__
> 
> In this case above, __FILE__ and __LINE__ does not provide much
> information?

In which case? The patch changes a repeated pattern to a macro(),
without changing any behavior and without questioning the use of
__FILE__:__LINE__.
.

> nftnl_expr_alloc() returns NULL when support for an expression is
> missing in libnftnl,

The patch is not about nftnl_expr_alloc(). Do you mean
nftnl_udata_buf_alloc()?

nftnl_udata_buf_alloc() fails exactly when malloc() fails. It's
unrelated to missing "support for an expression".

>  that provides a hint on that, this is very rare
> and it can only happen when developing support for new expressions.
> 
> Maybe simply say __func__ instead to know what function has failed
> when performing the memory allocation is a hint that is fine enough.

I wouldn't use __func__. It consumes more strings in the binary while
providing less exact information.


Thomas

Re: [PATCH nft 2/2] netlink: add and use _nftnl_udata_buf_alloc() helper

[Pablo Neira Ayuso]

On Thu, Nov 09, 2023 at 05:48:57PM +0100, Thomas Haller wrote:
> On Thu, 2023-11-09 at 16:32 +0100, Pablo Neira Ayuso wrote:
> > On Thu, Nov 09, 2023 at 04:19:29PM +0100, Thomas Haller wrote:
> > > On Thu, 2023-11-09 at 16:14 +0100, Pablo Neira Ayuso wrote:
> > > > 
> > > > Add a wrapper function, no macro.
> > > > 
> > > 
> > > 
> > > memory_allocation_error() is itself a macro, as it uses
> > > __FILE__,__LINE__
> > 
> > In this case above, __FILE__ and __LINE__ does not provide much
> > information?
> 
> In which case? The patch changes a repeated pattern to a macro(),
> without changing any behavior and without questioning the use of
> __FILE__:__LINE__.
> .
> 
> > nftnl_expr_alloc() returns NULL when support for an expression is
> > missing in libnftnl,
> 
> The patch is not about nftnl_expr_alloc(). Do you mean
> nftnl_udata_buf_alloc()?
> 
> nftnl_udata_buf_alloc() fails exactly when malloc() fails. It's
> unrelated to missing "support for an expression".
> 
> >  that provides a hint on that, this is very rare
> > and it can only happen when developing support for new expressions.
> > 
> > Maybe simply say __func__ instead to know what function has failed
> > when performing the memory allocation is a hint that is fine enough.
> 
> I wouldn't use __func__. It consumes more strings in the binary while
> providing less exact information.

OK, then if you prefer a generic OOM error message, that's also fine.

Re: [PATCH nft 1/2] utils: add memory_allocation_check() helper

[Pablo Neira Ayuso]

On Wed, Nov 08, 2023 at 07:24:24PM +0100, Thomas Haller wrote:
> libnftables kills the process on out of memory (xmalloc()), so
> when we use libraries that propagate ENOMEM to libnftables, we
> also abort the process.
> 
> For example:
> 
>      nlr = nftnl_rule_alloc();
>      if (!nlr)
>           memory_allocation_error();
> 
> Add memory_allocation_check() macro which can simplify this common
> check to:
> 
>      nlr = memory_allocation_check(nftnl_rule_alloc());
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  include/utils.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/utils.h b/include/utils.h
> index 36a28f893667..fcd7c598fe9f 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -142,6 +142,16 @@ extern void __memory_allocation_error(const char *filename, uint32_t line) __nor
>  #define memory_allocation_error()		\
>  	__memory_allocation_error(__FILE__, __LINE__);
>  
> +#define memory_allocation_check(cmd)               \
> +	({                                         \
> +		typeof((cmd)) _v = (cmd);          \
> +		const void *const _v2 = _v;        \
> +                                                   \
> +		if (!_v2)                          \

please don't hide a if branch inside a macro.

> +			memory_allocation_error(); \
> +		_v;                                \
> +	})
> +
>  extern void xfree(const void *ptr);
>  extern void *xmalloc(size_t size);
>  extern void *xmalloc_array(size_t nmemb, size_t size);
> -- 
> 2.41.0
> 

Re: [PATCH nft 1/2] utils: add memory_allocation_check() helper

[Thomas Haller]

On Thu, 2023-11-09 at 16:24 +0100, Pablo Neira Ayuso wrote:
> On Wed, Nov 08, 2023 at 07:24:24PM +0100, Thomas Haller wrote:
> > 
> > +#define memory_allocation_check(cmd)               \
> > +	({                                         \
> > +		typeof((cmd)) _v = (cmd);          \
> > +		const void *const _v2 = _v;        \
> > +                                                   \
> > +		if (!_v2)                          \
> 
> please don't hide a if branch inside a macro.

What is the reason for this rule? Is there a style guide somewhere?

> 
> > +			memory_allocation_error(); \
> > +		_v;                                \
> > +	})



It could be instead:


static inline void *__memory_allocation_check(const char *file, unsigned line, const void *ptr) {
    if (!ptr)
        __memory_allocation_error(file, line);
    return (void*) ptr;
}

#define memory_allocation_check(cmd) \
   ((typeof(cmd) __memory_allocation_check(__FILE__, __LINE__, (cmd))


Doesn't seem to make a difference either way.



Thomas

Re: [PATCH nft 1/2] utils: add memory_allocation_check() helper

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> static inline void *__memory_allocation_check(const char *file, unsigned line, const void *ptr) {
>     if (!ptr)
>         __memory_allocation_error(file, line);
>     return (void*) ptr;
> }
> 
> #define memory_allocation_check(cmd) \
>    ((typeof(cmd) __memory_allocation_check(__FILE__, __LINE__, (cmd))
> 
> Doesn't seem to make a difference either way.

We seem to be moving in circles.

I suspect your agenda is to avoid repeating the existing

x = alloc()
if (!x)
  barf()

pattern when adding userhandle support?

If so I think its best to just add a specific ubuf alloc wrapper that
can't fail (i.e. like the 'xmalloc' wrappers).

Like Pablo said, I don't see any added value in providing FILE/LINE
errors on stderr here.  It could be as simple as exit().

Re: [PATCH nft 1/2] utils: add memory_allocation_check() helper

[Thomas Haller]

On Wed, 2023-11-15 at 09:52 +0100, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > static inline void *__memory_allocation_check(const char *file,
> > unsigned line, const void *ptr) {
> >     if (!ptr)
> >         __memory_allocation_error(file, line);
> >     return (void*) ptr;
> > }
> > 
> > #define memory_allocation_check(cmd) \
> >    ((typeof(cmd) __memory_allocation_check(__FILE__, __LINE__,
> > (cmd))
> > 
> > Doesn't seem to make a difference either way.
> 
> We seem to be moving in circles.
> 
> I suspect your agenda is to avoid repeating the existing
> 
> x = alloc()
> if (!x)
>   barf()
> 
> pattern when adding userhandle support?

exactly.

> 
> If so I think its best to just add a specific ubuf alloc wrapper that
> can't fail (i.e. like the 'xmalloc' wrappers).

I don't understand.

_nftnl_udata_buf_alloc() *is* that specific wrapper.

Maybe the name is bad... and such wrappers should have a "nft_x_"
prefix (nft_x_nftnl_udata_buf_alloc()).

> 
> Like Pablo said, I don't see any added value in providing FILE/LINE
> errors on stderr here.  It could be as simple as exit().
> 

I don't understand.

The two patches don't change anything about that. There is no change in
behavior -- aside introducing a convenience wrapper for the repeated
ENOMEM check.

If you want to change memory_allocation_error() to not use FILE/LINE or
call exit(), then that is a separate discussion (I'd like to avoid).


Thomas