On Mon, 21 Feb 2022 17:17:57 +0100 Pablo Neira Ayuso wrote: > From: Florian Westphal <fw@xxxxxxxxx> > > stateful objects can be updated from the control plane. > The transaction logic allocates a temporary object for this purpose. > > The ->init function was called for this object, so plain kfree() leaks > resources. We must call ->destroy function of the object. > > nft_obj_destroy does this, but it also decrements the module refcount, > but the update path doesn't increment it. > > To avoid special-casing the update object release, do module_get for > the update case too and release it via nft_obj_destroy(). > > Fixes: d62d0ba97b58 ("netfilter: nf_tables: Introduce stateful object update operation") > Cc: Fernando Fernandez Mancera <ffmancera@xxxxxxxxxx> > Signed-off-by: Florian Westphal <fw@xxxxxxxxx> > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> > --- > net/netfilter/nf_tables_api.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index 3081c4399f10..49060f281342 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -6553,10 +6553,13 @@ static int nf_tables_updobj(const struct nft_ctx *ctx, > struct nft_trans *trans; > int err; > > + if (!try_module_get(type->owner)) > + return -ENOENT; > + > trans = nft_trans_alloc(ctx, NFT_MSG_NEWOBJ, > sizeof(struct nft_trans_obj)); > if (!trans) > - return -ENOMEM; > + goto err_trans; > > newobj = nft_obj_init(ctx, type, attr); > if (IS_ERR(newobj)) { > @@ -6573,6 +6576,8 @@ static int nf_tables_updobj(const struct nft_ctx *ctx, > > err_free_trans: > kfree(trans); > +err_trans: > + module_put(type->owner); > return err; > } > > @@ -8185,7 +8190,7 @@ static void nft_obj_commit_update(struct nft_trans *trans) > if (obj->ops->update) > obj->ops->update(obj, newobj); > > - kfree(newobj); > + nft_obj_destroy(&trans->ctx, newobj); > } > > static void nft_commit_release(struct nft_trans *trans) > @@ -8976,7 +8981,7 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action) > break; > case NFT_MSG_NEWOBJ: > if (nft_trans_obj_update(trans)) { > - kfree(nft_trans_obj_newobj(trans)); > + nft_obj_destroy(&trans->ctx, nft_trans_obj_newobj(trans)); > nft_trans_destroy(trans); > } else { > trans->ctx.table->use--; net/netfilter/nf_tables_api.c:6561:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (!trans) ^~~~~~ net/netfilter/nf_tables_api.c:6581:9: note: uninitialized use occurs here return err; ^~~ net/netfilter/nf_tables_api.c:6561:2: note: remove the 'if' if its condition is always false if (!trans) ^~~~~~~~~~~ net/netfilter/nf_tables_api.c:6554:9: note: initialize the variable 'err' to silence this warning int err; ^ = 0