Re: [nf-next PATCH v3 3/3] netfilter: nf_tables: Add locking for NFT_MSG_GETRULE_RESET requests

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Oct 19, 2023 at 02:33:50PM +0200, Phil Sutter wrote:
> 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@nwl.cc>
> > > ---
> > > 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.

I'd suggest you add comment to this code, feel free to add better
wording:

        /* Mutex is held is to prevent that two concurrent dump-and-reset calls
         * do not underrun counters and quotas. The commit_mutex is used for the
         * lack a better lock, this is not transaction path.
         */
        mutex_lock(&nft_net->commit_mutex);
        ret = nf_tables_dump_rules(skb, cb);
        mutex_unlock(&nft_net->commit_mutex);