On Fri, Oct 25, 2024 at 12:56:03PM +0200, Phil Sutter wrote:
> 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.
I'm afraid if the chain remains in place with no device it won't be
any better once the system boots up again, because chains cannot
updated in old kernels.
I think chain updates go hand in hand with your proposed approach to
leave the basechain in place.
> > > 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!
I am still exploring how ugly this fix looks, you will see get to know.
