Re: [nf PATCH 2/5] netfilter: nf_tables: Add locking for NFT_MSG_GETRULE_RESET requests

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

On Tue, Sep 26, 2023 at 02:14:05PM +0200, Phil Sutter wrote:
> On Tue, Sep 26, 2023 at 12:09:35PM +0200, Pablo Neira Ayuso wrote:
> > Hi Phil,
> > 
> > On Tue, Sep 26, 2023 at 11:34:43AM +0200, Phil Sutter wrote:
> > > On Mon, Sep 25, 2023 at 09:53:17PM +0200, Florian Westphal wrote:
> > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > On Sat, Sep 23, 2023 at 06:18:13PM +0200, Florian Westphal wrote:
> > > > > > callback_that_might_reset()
> > > > > > {
> > > > > > 	try_module_get ...
> > > > > > 	rcu_read_unlock()
> > > > > > 	mutex_lock(net->commit_mutex)
> > > > > > 	  dumper();
> > > > > > 	mutex_unlock(net->commit_mutex)
> > > > > > 	rcu_read_lock();
> > > > > > 	module_put()
> > > > > > }
> > > > > >
> > > > > > should do the trick.
> > > > > 
> > > > > Idiom above LGTM, *except for net->commit_mutex*. Please do not use
> > > > > ->commit_mutex: This will stall ruleset updates for no reason, netlink
> > > > > dump would grab and release such mutex for each netlink_recvmsg() call
> > > > > and netlink dump side will always retry because of NLM_F_EINTR.
> > > > 
> > > > It will stall updates, but for good reason: we are making changes to the
> > > > expressions state.
> > > 
> > > This also disqualifies the use of Pablo's suggested reset_lock, right?
> > 
> > Quick summary:
> > 
> > We are currently discussing if it makes sense to add a new lock or
> > not. The commit_mutex stalls updates, but netlink dumps retrieves
> > listings in chunks, that is, one recvmsg() call from userspace (to
> > retrieve one list chunk) will grab the mutex then release it until the
> > next recvmsg() call is done. Between these two calls an update is
> > still possible. The question is if it is worth to stall an ongoing
> > listing or updates.
> 
> Thanks for the summary. Assuming that a blocked commit will only be
> postponed until after the current chunk was filled and is being
> submitted to user space, I don't see how it would make a practical
> difference for reset command if commit_mutex is used instead of
> reset_lock (or a dedicated reset_mutex).

If the problem we are addressing is two processes listing the ruleset
that concur to reset stateful expressions, then there is no difference.
However, this is stalling writers and I don't think we need this
according to the problem description.

Another point to consider is how likely this list-and-reset happens.
If it is unlikely, then commit_mutex should be fine. But if we expect
a process polling to fetch counters very often, this will introduce an
unnecessary interference with writers. In a very dynamic deployment,
with frequent transaction updates, that might stall transactions for
no reason.

Please, note that using commit_mutex will *not* fix either that
userspace has to properly deal with NLM_F_EINTR, and I am 100% sure
you told me last time when you submitted this that you would prefer to
fix that once you get a ticket^H^H^H^H^H^H complain from someone else.
Oh, and I accepted that deal.

Honestly, we already have things to improve in other fronts, such
speeding up set updates and reducing userspace memory consumption,
much of this is userspace work.

> > There is the NLM_F_EINTR mechanism in place that tells that an
> > interference has occured while keeping the listing lockless.
> > 
> > Unless I am missing anything, the goal is to fix two different
> > processes that are listing at the same time, that is, two processes
> > running a netlink dump at the same time that are resetting the
> > stateful expressions in the ruleset.
> 
> Here's a simple repro I use to verify the locking approach (only rule
> reset for now):
> 
> | set -e
> | 
> | RULESET='flush ruleset
> | table t {
> |       chain c {
> |               counter packets 23 bytes 42
> |       }
> | }'
> | 
> | trap "$NFT list ruleset" EXIT
> | for ((i = 0; i < 10000; i++)); do
> |       echo "iter $i"
> |       $NFT -f - <<< "$RULESET"
> |       $NFT list ruleset | grep -q 'packets 23 bytes 42' >/dev/null
> |       $NFT reset rules >/dev/null &
> |       pid=$!
> |       $NFT reset rules >/dev/null
> |       wait $!
> |       #$NFT list ruleset | grep 'packets'
> |       $NFT list ruleset | grep -q 'packets 0 bytes 0' >/dev/null
> | done
> 
> If the two calls clash, the rule will have huge counter values due to
> underflow.

Can you give a try with the reset_lock spinlock approach with this
script that exercises worst case?

Thanks.