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@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.
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);
