| Message ID | 20231108182431.4005745-2-thaller@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft,1/2] utils: add memory_allocation_check() helper | expand |
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.
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
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 >
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
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.
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))
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(-)