Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> Add .release_ops, that is called in case of error at a later stage in
> the expression initialization path, ie. .select_ops() has been already
> set up operations and that needs to be undone.
>
> This allows us to follow a more simplistic approach, ie. place the
> match/target into the list from the .select_ops path. Otherwise, if
> there is already an extension in the list, bump the reference counter
> and re-use it.
>
> nft_xt_put() is always called from paths where we have guarantees that
> no packets are walking the expression ops. Therefore, there is no need
> for kfree_rcu() anymore.
>
> The atomic reference counter is still needed since the destroy path is
> called away from pernet mutex that protects the transaction.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> Compile tested only, no function test but this is more or less what
> I think we should do here. I still need to resync nf-next to get this
> patch in, so pull request for nf-next will be coming soon.
>
> static bool nft_xt_put(struct nft_xt *xt)
> {
> if (refcount_dec_and_test(&xt->refcnt)) {
> WARN_ON_ONCE(!list_empty(&xt->head));
> - kfree_rcu(xt, rcu_head);
> + list_del(&xt->head);
> + kfree(xt);
This list_del can race with list_add. Probably best to add a private
lock.
> - nft_match->listcnt = 0;
> list_add(&nft_match->head, &cn->nft_match_list);
(This can run in parallel from same netns).
Once a private lock is added I do not see a reason to have pernet lists
anymore, so you can revert cf52572ebbd7189a1966c2b5fc34b97078cd1dce.
With proper error undo, the entry gets inserted with refcnt of 1, so if
another netns can see/use this entry, it would just increment refcount
to 2. We would still need rcu in that case though (and use
refcount_inc_not_zero) so we can search the lists in parallel from
different netns, or grab the private list lock to synchronize with
removal/list_del/refcount_dec.
