[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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