lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230227124402.GA30043@breakpoint.cc> Date: Mon, 27 Feb 2023 13:44:02 +0100 From: Florian Westphal <fw@...len.de> To: Alexander Atanasov <alexander.atanasov@...tuozzo.com> Cc: netdev@...r.kernel.org Subject: Re: [PATCH] netfilter: nf_tables: always synchronize with readers before releasing tables Alexander Atanasov <alexander.atanasov@...tuozzo.com> wrote: > general protection fault, probably for non-canonical > address 0xdead000000000115: 0000 [#1] PREEMPT SMP NOPTI > RIP: 0010:__nf_tables_dump_rules+0x10d/0x170 [nf_tables] > > __nf_tables_dump_rules runs under rcu_read_lock while __nft_release_table > is called from nf_tables_exit_net. commit_mutex is held inside > nf_tables_exit_net but this is not enough to guard against > lockless readers. When __nft_release_table does list_del(&rule->list) > next ptr is poisoned and it crashes while walking the list. > > Before calling __nft_release_tables all lockless readers must be done - > to ensure this a call to synchronize_rcu() is required. > > nf_tables_exit_net does this in case there is something to abort > inside __nf_tables_abort but it does not do so otherwise. > Fix this by add the missing synchronize_rcu() call before calling > __nft_release_table in the nothing to abort case. > > Fixes: 6001a930ce03 ("netfilter: nftables: introduce table ownership") > Signed-off-by: Alexander Atanasov <alexander.atanasov@...tuozzo.com> > --- > net/netfilter/nf_tables_api.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index d73edbd4eec4..849523ece109 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -10333,9 +10333,15 @@ static void __net_exit nf_tables_exit_net(struct > net *net) > struct nftables_pernet *nft_net = nft_pernet(net); > mutex_lock(&nft_net->commit_mutex); > + /* Need to call synchronize_rcu() to let any active rcu lockless > + * readers to finish. __nf_tables_abort does this internaly so > + * only call it here if there is nothing to abort. > + */ > if (!list_empty(&nft_net->commit_list) || > !list_empty(&nft_net->module_list)) > __nf_tables_abort(net, NFNL_ABORT_NONE); > + else > + synchronize_rcu(); Wouldn't it be better to just drop those list_empty() checks? AFAICS __nf_tables_abort will DTRT in that case. You can still add a comment like the one you added above to make it clear that we also need to wait for those readers to finish. Lastly, that list_del() in __nft_release_basechain should probably be list_del_rcu()?
Powered by blists - more mailing lists