All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] netfilter: Cleanup nft_net->module_list from nf_tables_exit_net()
@ 2022-11-03 13:08 Shigeru Yoshida
  2022-11-03 13:18 ` Florian Westphal
  0 siblings, 1 reply; 2+ messages in thread
From: Shigeru Yoshida @ 2022-11-03 13:08 UTC (permalink / raw)
  To: pablo, kadlec, fw
  Cc: netfilter-devel, coreteam, linux-kernel, syzkaller-bugs,
	Shigeru Yoshida, syzbot+178efee9e2d7f87f5103

syzbot reported a warning like below [1]:

WARNING: CPU: 3 PID: 9 at net/netfilter/nf_tables_api.c:10096 nf_tables_exit_net+0x71c/0x840
Modules linked in:
CPU: 2 PID: 9 Comm: kworker/u8:0 Tainted: G        W          6.1.0-rc3-00072-g8e5423e991e8 #47
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-1.fc36 04/01/2014
Workqueue: netns cleanup_net
RIP: 0010:nf_tables_exit_net+0x71c/0x840
...
Call Trace:
 <TASK>
 ? __nft_release_table+0xfc0/0xfc0
 ops_exit_list+0xb5/0x180
 cleanup_net+0x506/0xb10
 ? unregister_pernet_device+0x80/0x80
 process_one_work+0xa38/0x1730
 ? pwq_dec_nr_in_flight+0x2b0/0x2b0
 ? rwlock_bug.part.0+0x90/0x90
 ? _raw_spin_lock_irq+0x46/0x50
 worker_thread+0x67e/0x10e0
 ? process_one_work+0x1730/0x1730
 kthread+0x2e5/0x3a0
 ? kthread_complete_and_exit+0x40/0x40
 ret_from_fork+0x1f/0x30
 </TASK>

In nf_tables_exit_net(), there is a case where nft_net->commit_list is
empty but nft_net->module_list is not empty.  Such a case occurs with
the following scenario:

1. nfnetlink_rcv_batch() is called
2. nf_tables_newset() returns -EAGAIN and NFNL_BATCH_FAILURE bit is
   set to status
3. nf_tables_abort() is called with NFNL_ABORT_AUTOLOAD
   (nft_net->commit_list is released, but nft_net->module_list is not
   because of NFNL_ABORT_AUTOLOAD flag)
4. Jump to replay label
5. netlink_skb_clone() fails and returns from the function (this is
   caused by fault injection in the reproducer of syzbot)

This patch fixes this issue by calling __nf_tables_abort() when
nft_net->module_list is not empty in nf_tables_exit_net().

Link: https://syzkaller.appspot.com/bug?id=802aba2422de4218ad0c01b46c9525cc9d4e4aa3 [1]
Reported-by: syzbot+178efee9e2d7f87f5103@syzkaller.appspotmail.com
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 net/netfilter/nf_tables_api.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 58d9cbc9ccdc..a7579d16f59f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -10088,7 +10088,8 @@ 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);
-	if (!list_empty(&nft_net->commit_list))
+	if (!list_empty(&nft_net->commit_list) ||
+	    !list_empty(&nft_net->module_list))
 		__nf_tables_abort(net, NFNL_ABORT_NONE);
 	__nft_release_tables(net);
 	mutex_unlock(&nft_net->commit_mutex);
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] netfilter: Cleanup nft_net->module_list from nf_tables_exit_net()
  2022-11-03 13:08 [PATCH] netfilter: Cleanup nft_net->module_list from nf_tables_exit_net() Shigeru Yoshida
@ 2022-11-03 13:18 ` Florian Westphal
  0 siblings, 0 replies; 2+ messages in thread
From: Florian Westphal @ 2022-11-03 13:18 UTC (permalink / raw)
  To: Shigeru Yoshida
  Cc: pablo, kadlec, fw, netfilter-devel, coreteam, linux-kernel,
	syzkaller-bugs, syzbot+178efee9e2d7f87f5103

Shigeru Yoshida <syoshida@redhat.com> wrote:
> syzbot reported a warning like below [1]:
> 
> This patch fixes this issue by calling __nf_tables_abort() when
> nft_net->module_list is not empty in nf_tables_exit_net().

Fixes: eb014de4fd41 ("netfilter: nf_tables: autoload modules from the abort path")

> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 58d9cbc9ccdc..a7579d16f59f 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -10088,7 +10088,8 @@ 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);
> -	if (!list_empty(&nft_net->commit_list))
> +	if (!list_empty(&nft_net->commit_list) ||
> +	    !list_empty(&nft_net->module_list))
>  		__nf_tables_abort(net, NFNL_ABORT_NONE);

Maybe just an unconditionall call to nf_tables_module_autoload_cleanup().

Or, alternatively, unconditionally call __nf_tables_abort().
The downside is that we will need to change __nf_tables_abort to elide
synchronize_rcu() unless needed -- else netns dismantling might become
too expensive.

OTOH, module_list should always be empty here, so this is ok.

Reviewed-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2022-11-03 13:18 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-03 13:08 [PATCH] netfilter: Cleanup nft_net->module_list from nf_tables_exit_net() Shigeru Yoshida
2022-11-03 13:18 ` Florian Westphal

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.