Re: Netfilter: suspicious RCU usage in __nft_rule_lookup

[Phil Sutter <phil@nwl.cc>]

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@kernel.org> 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@netfilter.org>
> 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