On Fri, Oct 25, 2024 at 12:26:47PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 25, 2024 at 11:46:33AM +0200, Phil Sutter wrote:
> > On Fri, Oct 25, 2024 at 11:23:56AM +0200, Florian Westphal wrote:
> > > Matthieu Baerts <matttbe@xxxxxxxxxx> wrote:
> > > > While at it, I had a question related to the rules' list: in
> > > > __nft_release_basechain() from the same nf_tables_api.c file, list's
> > > > entries are not removed with the _rcu variant → is it OK to do that
> > > > because this function is only called last at the cleanup time, when no
> > > > other readers can iterate over the list? So similar to what is done in
> > > > __nft_release_table()?
> > >
> > > Looks like __nft_release_basechain() is broken from start, I don't see
> > > how it can work, it doesn't call synchronize_rcu or anything like that
> > > afaics.
> > >
> > > No idea what to do here.
> >
> > It will vanish with my name-based netdev hooks series (the second part).
> > I could prepare a patch for nf/stable which merely kills that function -
> > dropping netdev-family chains upon removal of last interface is
> > inconsistent wrt. flowtables which remain in place.
>
> I like the idea of keeping the basechain in place. With chain update
> support, it makes sense to add a basechain then update it with the
> devices to hook in.
>
> But chain device updates are only recently supported:
>
> b9703ed44ffbfba85c103b9de01886a225e14b38
> Author: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> Date: Fri Apr 21 00:34:31 2023 +0200
>
> netfilter: nf_tables: support for adding new devices to an existing netdev chain
>
> that is, in older kernels, this chain would remain unused because
> updates are not be possible.
No idea how many nftables "service" scripts out there actually do it,
but I keep considering the hypothetical case of SAVE_RULESET_ON_STOP=1
which does 'nft list ruleset >/etc/nftables/nftables.conf'. If it runs
after the "network" service shuts down and removes all virtual
interfaces, it would drop any hooked chains from user's config.
> > Another alternative might be to call synchronize_rcu() in there, but it
> > slows down interface teardown AIUI.
>
> Else unregister objects from lists under mutex then call_rcu() to
> release them.
This requires to add an rcu_head to nft_chain or nft_base_chain, right?
Or is it possible to allocate a new object just for the purpose, like:
| struct nft_chain_rcu_call {
| struct rcu_head rcu;
| struct nft_chain *chain;
| };
And free both base chain and this temporary object in the callback?
> Then, take you patch so new kernel don't remove the basechain.
We would not change behaviour in stable this way, also not the worst
thing to do. Your call!
Thanks, Phil
