[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2c9efd36-f1f6-b77b-d4eb-f65932cfaba@netfilter.org>
Date: Thu, 5 Oct 2023 16:50:42 +0200 (CEST)
From: Jozsef Kadlecsik <kadlec@...filter.org>
To: Florian Westphal <fw@...len.de>
cc: xiaolinkui <xiaolinkui@....com>,
Pablo Neira Ayuso <pablo@...filter.org>,
David Miller <davem@...emloft.net>, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, justinstitt@...gle.com,
kuniyu@...zon.com, netfilter-devel@...r.kernel.org,
coreteam@...filter.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, Linkui Xiao <xiaolinkui@...inos.cn>
Subject: Re: [PATCH] netfilter: ipset: wait for xt_recseq on all cpus
On Thu, 5 Oct 2023, Florian Westphal wrote:
> xiaolinkui <xiaolinkui@....com> wrote:
> > From: Linkui Xiao <xiaolinkui@...inos.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@...r.kernel.org
first in order to review netfilter and ipset related patches there.
Best regards,
Jozsef
-
E-mail : kadlec@...ckhole.kfki.hu, kadlecsik.jozsef@...ner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
H-1525 Budapest 114, POB. 49, Hungary
Powered by blists - more mailing lists