On Thu, Oct 19, 2023 at 01:38:04PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 19, 2023 at 01:33:47PM +0200, Phil Sutter wrote:
> > Rule reset is not concurrency-safe per-se, so multiple CPUs may reset
> > the same rule at the same time. At least counter and quota expressions
> > will suffer from value underruns in this case.
> >
> > Prevent this by introducing dedicated locking callbacks for nfnetlink
> > and the asynchronous dump handling to serialize access.
> >
> > Signed-off-by: Phil Sutter <phil@xxxxxx>
> > ---
> > Changes since v2:
> > - Keep local variable 'nft_net' in nf_tables_getrule_reset()
> > - No need for local variable 'family' in same function (used only once
> > after all the churn)
> > ---
> > net/netfilter/nf_tables_api.c | 74 ++++++++++++++++++++++++++++-------
> > 1 file changed, 60 insertions(+), 14 deletions(-)
> >
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 584d3b204372..fbb688c9903c 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> [...]
> > +static int nf_tables_dumpreset_rules(struct sk_buff *skb,
> > + struct netlink_callback *cb)
> > +{
> > + struct nftables_pernet *nft_net = nft_pernet(sock_net(skb->sk));
> > + int ret;
> > +
> > + mutex_lock(&nft_net->commit_mutex);
> > + ret = nf_tables_dump_rules(skb, cb);
> > + mutex_unlock(&nft_net->commit_mutex);
>
> NACK.
>
> This just mitigates the problem we are discussing, when there is an
> interference with an ongoing transaction.
This fixes for user space's ability to underrun counters and quotas
because expressions' dump callbacks are not concurrency safe in reset
mode.
What you're concerned with is a different issue.
