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

[Florian Westphal <fw@strlen.de>]

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.

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.