Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Thu, 5 Oct 2023 16:50:42 +0200 (CEST)
From:   Jozsef Kadlecsik <kadlec@netfilter.org>
To:     Florian Westphal <fw@strlen.de>
cc:     xiaolinkui <xiaolinkui@126.com>,
        Pablo Neira Ayuso <pablo@netfilter.org>,
        David Miller <davem@davemloft.net>, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, justinstitt@google.com,
        kuniyu@amazon.com, netfilter-devel@vger.kernel.org,
        coreteam@netfilter.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, Linkui Xiao <xiaolinkui@kylinos.cn>
Subject: Re: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus
In-Reply-To: <20231005123107.GB9350@breakpoint.cc>
Message-ID: <2c9efd36-f1f6-b77b-d4eb-f65932cfaba@netfilter.org>
References: <20231005115022.12902-1-xiaolinkui@126.com> <20231005123107.GB9350@breakpoint.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 5 Oct 2023, Florian Westphal wrote:

> xiaolinkui <xiaolinkui@126.com> wrote:
> > From: Linkui Xiao <xiaolinkui@kylinos.cn>
> > 
> > Before destroying the ipset, take a check on sequence to ensure that the
> > ip_set_test operation of this ipset has been completed.
> > 
> > The code of set_match_v4 is protected by addend=xt_write_recseq_begin() and
> > xt_write_recseq_end(addend). So we can ensure that the test operation is
> > completed by reading seqcount.
> 
> Nope, please don't do this, the xt_set can also be used from nft_compat
> which doesn't use the xtables packet traversers.
> 
> I'd rather use synchonize_rcu() once in ip_set_destroy(), that will
> make sure all concurrent traversers are gone.

But ip_set_destroy() can be called only when there's no reference to the 
set in the kernel and thus there's no ipset function whatsoever in the 
packet path which would access it.

> That said, I still do not understand this fix, the
> match / target destroy hooks are called after the table has
> been completely replaced, i.e., while packets can still be in flight
> no packets should be within the ipset lookup functions when
> this happens, and no more packets should be able to enter them.
> 
> AFAICS the request to delete the set will fail if its still referenced
> via any rule. xt_set holds references to the sets.
> 
> So:
> 1. set have dropped all references
> 2. userspace *can* delete the set
> 3. we get crash because xt_set was still within a sets eval
>   function.
> 
> I don't see how 3) can happen, xt table replace isn't supposed
> to call the xt_set destroy functions until after table replace.
> 
> We even release the entire x_table blob right afterwards.

I'd expect the author to send patches to netfilter-devel@vger.kernel.org 
first in order to review netfilter and ipset related patches there.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary