Re: [nf PATCH v2 8/8] netfilter: nf_tables: Add locking for NFT_MSG_GETSETELEM_RESET requests

[Phil Sutter <phil@nwl.cc>]

On Thu, Sep 28, 2023 at 10:07:51PM +0200, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > I'd say its semantically the same thing, we alter state.
> > > > 
> > > > We even emit audit records to tell userspace that there is state
> > > > change.
> > > 
> > > This is a netlink event, how does the mutex help in that regard?
> > 
> > It will prevent two concurrent 'reset dumps' from messing
> > up internal state of counters, quota, etc.
> > > > Also, are you sure spinlock is appropriate here?
> > > > 
> > > > For full-ruleset resets we might be doing quite some
> > > > traverals.
> > > 
> > > I said several times, we grab and release this for each
> > > netlink_recvmsg(), commit_mutex helps us in no way to fix the "two
> > > concurrent dump-and-reset problem".
> > 
> > It does, any lock prevents the 'concurrent reset problem'.
> 
> Reading Jozsefs email:
> 
> The locking prevents problems with concurrent resets,
> but not concurrent modifcation with one (or more) resets.
> 
> Phil, what is the intention with the reset?
> If the idea is to atomically reset AND list everything
> (old values are shown) then yes, this approach doesn't work
> either and something ipset-alike has to be done, i.e.
> completely preventing any new transaction while a reset
> "dump" is in progress.

Sure, the functionality is to fetch old values while resetting so they
are not lost (and may be used for accounting, etc.). The question is
what we want to guarantee. There's still the non-dump path, so if
someone wants transactional safety they could still reset rules
individually.

The whole "what if my other hand pulls the trigger while I'm still
drawing the gun" scenario is a bit too academic for my taste. ;)

> Pablo, do you think we should combine this with the
> ealier-discussed "perpetual dump restart" problem?
> 
> Userspace could request 'stable' dump that locks
> out writers for the entire duration, and kernel could
> force it automatically for resets.
> 
> I don't really like it though because misbehaving userspace
> can lock out writers.

Make them inactive and free only after the dump is done? IIUC,
nft_active_genmask() will return true again though after the second
update, right?

Cheers, Phil