Hi Florian, On Tue, Feb 05, 2019 at 07:19:59AM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote: > > OK. So after this chunk, this works like this: > > > > * listcnt is set to 1 and match/target is added to the list from .select_ops > > * in case of error: match/target is left in the list, so it can be "recovered". > > * in case of success: match/target is removed from the list, so this > > nft_xt is unique. > > Its supposed to be this: > * listcnt is set to 0 and match/target is added to list from select_ops > * in case of error: match/target is left in the list, so it can be > "recovered" > * in case of success: listcnt/refcnt get increased, so nft_xt is not removed > from lists when the expression is destroyed OK. Thanks for explaining. With your chunk in place, I don't see any listcnt++, now the ->activate path is gone, so we never bump it again? Refering to this patch. > > So listcnt is there to catch the error case, right? > > listcnt is there to remove the expression from the list via > ->deactivate, ->destroy is called too late (after commit phase/no mutex > held). We need the listcnt to know when we remove the last instance of > the expression. OK, now with this chunk we remove the match/target from the abort/commit phase. And nft_xt_put() from the destroy path deals with removing the extension (BTW, probably no need for kfree_rcu() anymore?). Right? > > We can probably add something like a flag to nft_expr_ops, eg. > > NFT_EXPR_F_DYNAMIC, then call a release function for this > > .release_ops from the error path to solve this problem in nft_compat. > > Or just check if there's a .release_ops and call it. > > That would make it possible to init both listcnt and recnt to 1. I think we probably just need refcnt after this. > We could also consider removing the entire list handling and always > use a 1:1 mapping of nft_expr <-> nft_compat_ops > > (we don't need to "Recover" in case of error). You mean, no reuse at all of the nft_xt object.
