Re: [PATCH v5 nf-next 1/3] Revert nf_tables commit_mutex in reset path

[Florian Westphal <fw@strlen.de>]

Brian Witte <brianwitte@mailfence.com> wrote:

TL;DR: I plan to queue this series up for the nf tree next week.
Not directly related to this patchset:

[ CC Phil ]

> Revert mutex-based locking for reset requests. It caused a circular
> lock dependency between commit_mutex, nfnl_subsys_ipset, and
> nlk_cb_mutex when nft reset, ipset list, and iptables-nft with set
> match ran concurrently.
> 
> This reverts bd662c4218f9, 3d483faa6663, 3cb03edb4de3.

Phil, Pablo, the reset infra is broken in the sense that it cannot
guarantee a correct dump+reset:

        nft_rule_for_each_expr(expr, next, rule) {
                if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
                        goto nla_put_failure;
        }
        nla_nest_end(skb, list);

-> when a single ->dump callback fails because netlink skb is full,
the dump is trimmed and resumed.

But, the reset side effects are already visible.
Hence, while dump may be complete, it can contain already-zeroed
counters without userspace ever getting the pre-reset value.

Maybe we should add a cushion in the relevant dump callbacks to
bail out before calling counter/quota->dump() when we run low on
remaining space?  What do you think?