Hi Florian,
Thanks a lot for your reviews.
On Sun, Mar 10, 2024 at 06:47:54PM +0100, Florian Westphal wrote:
> Quan Tian <tianquan23@xxxxxxxxx> wrote:
> > @@ -10129,14 +10154,12 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
> > switch (trans->msg_type) {
> > case NFT_MSG_NEWTABLE:
> > if (nft_trans_table_update(trans)) {
> > - if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
> > - nft_trans_destroy(trans);
> > - break;
> > + if (trans->ctx.table->flags & __NFT_TABLE_F_UPDATE) {
> > + if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
> > + nf_tables_table_disable(net, trans->ctx.table);
> > + trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
> > }
> > - if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
> > - nf_tables_table_disable(net, trans->ctx.table);
> > -
> > - trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
> > + swap(trans->ctx.table->udata, nft_trans_table_udata(trans));
> > } else {
> > nft_clear(net, trans->ctx.table);
> > }
>
> There is a call to nft_trans_destroy() below here.
> You could add a "break" after the swap() to avoid it.
>
> Otherwise kmemleak should report old udata being lost
> on update.
Thanks for pointing it out. kmemleak indeed reported the leak.
I found "break" after swap() would skip sending the table update event,
so I changed to execute different code paths for the two branches in v3.
Please let me know if it makes sense to you.
Thanks,
Quan
