* [PATCH RESEND nf] netfilter: avoid a race between nf_register_hook() and cleanup_net()
[not found] <20160729150033.E0250A0BD9@unicorn.suse.cz>
@ 2016-07-29 16:19 ` Michal Kubecek
2016-07-30 13:24 ` Eric W. Biederman
0 siblings, 1 reply; 4+ messages in thread
From: Michal Kubecek @ 2016-07-29 16:19 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Patrick McHardy, Jozsef Kadlecsik, netfilter-devel, coreteam,
netdev, linux-kernel, Eric W. Biederman
There is a race condition between nf_{,un}register_hook() and
cleanup_net() which can either trigger WARN check or cause a memory
leak. The scenario is like this (2a and 2b are alternatives):
1. cleanup_net() removes one or more struct net from net_namespace_list
2a. nf_register_hook() adds per-netns hooks to all netns (but not those
removed in step 1) and adds the hook to global nf_hook_list
2b. nf_unregister_hook() deletes per-netns hooks from all netns (but not
those removed in step 1) and removes the hook from nf_hook_list
3. cleanup_net() calls pernet subsystem exit functions for netns being
removed; one of them is netfilter_net_exit() which (among others)
calls nf_unregister_net_hook() to unregister per-netns hooks for all
hooks in nf_hook_list.
In case (a), per-netns hooks are never added as the namespace was
already invisible to for_each_net() in step 2a but an attempt to remove
them in step 3 (the hook is already in nf_hook_list) triggers a WARN
check in nf_unregister_net_hook() (no real harm done, however). In case
(b), the per-netns hook is removed neither in step 2b (netns is already
invisible to for_each_net()) nor in step 3 (the hook is already removed
from nf_hook_list), causing a memory leak.
Prevent the race by protecting the for_each_net() loop in
nf_{,un}register_hook() (also) by net_mutex. There is already a
precendens for this in rtnl_link_unregister() which addresses similar
race.
Fixes: 085db2c04557 ("netfilter: Per network namespace netfilter hooks.")
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
net/netfilter/core.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index f39276d1c2d7..860978c9f82e 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -193,6 +193,8 @@ int nf_register_hook(struct nf_hook_ops *reg)
struct net *net, *last;
int ret;
+ /* prevent race with cleanup_net() */
+ mutex_lock(&net_mutex);
rtnl_lock();
for_each_net(net) {
ret = nf_register_net_hook(net, reg);
@@ -201,6 +203,7 @@ int nf_register_hook(struct nf_hook_ops *reg)
}
list_add_tail(®->list, &nf_hook_list);
rtnl_unlock();
+ mutex_unlock(&net_mutex);
return 0;
rollback:
@@ -211,6 +214,7 @@ rollback:
nf_unregister_net_hook(net, reg);
}
rtnl_unlock();
+ mutex_unlock(&net_mutex);
return ret;
}
EXPORT_SYMBOL(nf_register_hook);
@@ -219,11 +223,14 @@ void nf_unregister_hook(struct nf_hook_ops *reg)
{
struct net *net;
+ /* prevent race with cleanup_net() */
+ mutex_lock(&net_mutex);
rtnl_lock();
list_del(®->list);
for_each_net(net)
nf_unregister_net_hook(net, reg);
rtnl_unlock();
+ mutex_unlock(&net_mutex);
}
EXPORT_SYMBOL(nf_unregister_hook);
--
2.9.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RESEND nf] netfilter: avoid a race between nf_register_hook() and cleanup_net()
2016-07-29 16:19 ` [PATCH RESEND nf] netfilter: avoid a race between nf_register_hook() and cleanup_net() Michal Kubecek
@ 2016-07-30 13:24 ` Eric W. Biederman
2016-08-01 12:34 ` Pablo Neira Ayuso
2016-08-26 17:31 ` Pablo Neira Ayuso
0 siblings, 2 replies; 4+ messages in thread
From: Eric W. Biederman @ 2016-07-30 13:24 UTC (permalink / raw)
To: Michal Kubecek
Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
netfilter-devel, coreteam, netdev, linux-kernel
Michal Kubecek <mkubecek@suse.cz> writes:
> There is a race condition between nf_{,un}register_hook() and
> cleanup_net() which can either trigger WARN check or cause a memory
> leak. The scenario is like this (2a and 2b are alternatives):
>
> 1. cleanup_net() removes one or more struct net from net_namespace_list
> 2a. nf_register_hook() adds per-netns hooks to all netns (but not those
> removed in step 1) and adds the hook to global nf_hook_list
> 2b. nf_unregister_hook() deletes per-netns hooks from all netns (but not
> those removed in step 1) and removes the hook from nf_hook_list
> 3. cleanup_net() calls pernet subsystem exit functions for netns being
> removed; one of them is netfilter_net_exit() which (among others)
> calls nf_unregister_net_hook() to unregister per-netns hooks for all
> hooks in nf_hook_list.
>
> In case (a), per-netns hooks are never added as the namespace was
> already invisible to for_each_net() in step 2a but an attempt to remove
> them in step 3 (the hook is already in nf_hook_list) triggers a WARN
> check in nf_unregister_net_hook() (no real harm done, however). In case
> (b), the per-netns hook is removed neither in step 2b (netns is already
> invisible to for_each_net()) nor in step 3 (the hook is already removed
> from nf_hook_list), causing a memory leak.
>
> Prevent the race by protecting the for_each_net() loop in
> nf_{,un}register_hook() (also) by net_mutex. There is already a
> precendens for this in rtnl_link_unregister() which addresses similar
> race.
So this analysis of a problem appears to be spot on.
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
I really really want there to be a better way to do this, but it is
really not ok for a hook to continue it's life past
nf_unregister_net_hook as after that point the code may be removed
from the kernel (sigh).
Although keeping with the precedent and minimizing net_mutex
we could remove the WARN and keep nf_register_hook as it is.
But that sounds entirely too clever for a fix that will
probably be backported.
But that sounds entirely too clever for a fix that likely needs to be
backported.
Eric
> Fixes: 085db2c04557 ("netfilter: Per network namespace netfilter hooks.")
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
> net/netfilter/core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index f39276d1c2d7..860978c9f82e 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -193,6 +193,8 @@ int nf_register_hook(struct nf_hook_ops *reg)
> struct net *net, *last;
> int ret;
>
> + /* prevent race with cleanup_net() */
> + mutex_lock(&net_mutex);
> rtnl_lock();
> for_each_net(net) {
> ret = nf_register_net_hook(net, reg);
> @@ -201,6 +203,7 @@ int nf_register_hook(struct nf_hook_ops *reg)
> }
> list_add_tail(®->list, &nf_hook_list);
> rtnl_unlock();
> + mutex_unlock(&net_mutex);
>
> return 0;
> rollback:
> @@ -211,6 +214,7 @@ rollback:
> nf_unregister_net_hook(net, reg);
> }
> rtnl_unlock();
> + mutex_unlock(&net_mutex);
> return ret;
> }
> EXPORT_SYMBOL(nf_register_hook);
> @@ -219,11 +223,14 @@ void nf_unregister_hook(struct nf_hook_ops *reg)
> {
> struct net *net;
>
> + /* prevent race with cleanup_net() */
> + mutex_lock(&net_mutex);
> rtnl_lock();
> list_del(®->list);
> for_each_net(net)
> nf_unregister_net_hook(net, reg);
> rtnl_unlock();
> + mutex_unlock(&net_mutex);
> }
> EXPORT_SYMBOL(nf_unregister_hook);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RESEND nf] netfilter: avoid a race between nf_register_hook() and cleanup_net()
2016-07-30 13:24 ` Eric W. Biederman
@ 2016-08-01 12:34 ` Pablo Neira Ayuso
2016-08-26 17:31 ` Pablo Neira Ayuso
1 sibling, 0 replies; 4+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-01 12:34 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Michal Kubecek, Patrick McHardy, Jozsef Kadlecsik,
netfilter-devel, coreteam, netdev, linux-kernel
On Sat, Jul 30, 2016 at 08:24:37AM -0500, Eric W. Biederman wrote:
> Michal Kubecek <mkubecek@suse.cz> writes:
>
> > There is a race condition between nf_{,un}register_hook() and
> > cleanup_net() which can either trigger WARN check or cause a memory
> > leak. The scenario is like this (2a and 2b are alternatives):
> >
> > 1. cleanup_net() removes one or more struct net from net_namespace_list
> > 2a. nf_register_hook() adds per-netns hooks to all netns (but not those
> > removed in step 1) and adds the hook to global nf_hook_list
> > 2b. nf_unregister_hook() deletes per-netns hooks from all netns (but not
> > those removed in step 1) and removes the hook from nf_hook_list
> > 3. cleanup_net() calls pernet subsystem exit functions for netns being
> > removed; one of them is netfilter_net_exit() which (among others)
> > calls nf_unregister_net_hook() to unregister per-netns hooks for all
> > hooks in nf_hook_list.
> >
> > In case (a), per-netns hooks are never added as the namespace was
> > already invisible to for_each_net() in step 2a but an attempt to remove
> > them in step 3 (the hook is already in nf_hook_list) triggers a WARN
> > check in nf_unregister_net_hook() (no real harm done, however). In case
> > (b), the per-netns hook is removed neither in step 2b (netns is already
> > invisible to for_each_net()) nor in step 3 (the hook is already removed
> > from nf_hook_list), causing a memory leak.
> >
> > Prevent the race by protecting the for_each_net() loop in
> > nf_{,un}register_hook() (also) by net_mutex. There is already a
> > precendens for this in rtnl_link_unregister() which addresses similar
> > race.
>
> So this analysis of a problem appears to be spot on.
>
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
>
> I really really want there to be a better way to do this, but it is
> really not ok for a hook to continue it's life past
> nf_unregister_net_hook as after that point the code may be removed
> from the kernel (sigh).
>
> Although keeping with the precedent and minimizing net_mutex
> we could remove the WARN and keep nf_register_hook as it is.
> But that sounds entirely too clever for a fix that will
> probably be backported.
>
> But that sounds entirely too clever for a fix that likely needs to be
> backported.
Please, propagate up to the caller to register and unregister the
hooks from init_net and exit_net instead as I suggested time ago.
I understand that this is not as small as this patch, and that this
will require a bit more boiler plate code in iptable_*.c and nftables
itself, but we'll avoid the dependencies with both rtnl_lock and
net_lock.
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RESEND nf] netfilter: avoid a race between nf_register_hook() and cleanup_net()
2016-07-30 13:24 ` Eric W. Biederman
2016-08-01 12:34 ` Pablo Neira Ayuso
@ 2016-08-26 17:31 ` Pablo Neira Ayuso
1 sibling, 0 replies; 4+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-26 17:31 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Michal Kubecek, Patrick McHardy, Jozsef Kadlecsik,
netfilter-devel, coreteam, netdev, linux-kernel
Hi Eric,
On Sat, Jul 30, 2016 at 08:24:37AM -0500, Eric W. Biederman wrote:
> Michal Kubecek <mkubecek@suse.cz> writes:
>
> > There is a race condition between nf_{,un}register_hook() and
> > cleanup_net() which can either trigger WARN check or cause a memory
> > leak. The scenario is like this (2a and 2b are alternatives):
> >
> > 1. cleanup_net() removes one or more struct net from net_namespace_list
> > 2a. nf_register_hook() adds per-netns hooks to all netns (but not those
> > removed in step 1) and adds the hook to global nf_hook_list
> > 2b. nf_unregister_hook() deletes per-netns hooks from all netns (but not
> > those removed in step 1) and removes the hook from nf_hook_list
> > 3. cleanup_net() calls pernet subsystem exit functions for netns being
> > removed; one of them is netfilter_net_exit() which (among others)
> > calls nf_unregister_net_hook() to unregister per-netns hooks for all
> > hooks in nf_hook_list.
> >
> > In case (a), per-netns hooks are never added as the namespace was
> > already invisible to for_each_net() in step 2a but an attempt to remove
> > them in step 3 (the hook is already in nf_hook_list) triggers a WARN
> > check in nf_unregister_net_hook() (no real harm done, however). In case
> > (b), the per-netns hook is removed neither in step 2b (netns is already
> > invisible to for_each_net()) nor in step 3 (the hook is already removed
> > from nf_hook_list), causing a memory leak.
> >
> > Prevent the race by protecting the for_each_net() loop in
> > nf_{,un}register_hook() (also) by net_mutex. There is already a
> > precendens for this in rtnl_link_unregister() which addresses similar
> > race.
>
> So this analysis of a problem appears to be spot on.
>
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> I really really want there to be a better way to do this, but it is
> really not ok for a hook to continue it's life past
> nf_unregister_net_hook as after that point the code may be removed
> from the kernel (sigh).
>
> Although keeping with the precedent and minimizing net_mutex
> we could remove the WARN and keep nf_register_hook as it is.
> But that sounds entirely too clever for a fix that will
> probably be backported.
>
> But that sounds entirely too clever for a fix that likely needs to be
> backported.
OK... I'm going to place this in the nf.git tree... but this is very ugly.
So Eric, I'd really appreciate if you can follow up once this has hit
nf-next.git and we get rid of the rtnl_lock and net_lock mutex by
propagating up to the the caller the hook registration from init_net()
and unregistering this from exit_net(). So we don't need to loop on
the existing netns but we use the existing netns init and exit
callbacks.
Let me know, thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-08-26 17:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20160729150033.E0250A0BD9@unicorn.suse.cz>
2016-07-29 16:19 ` [PATCH RESEND nf] netfilter: avoid a race between nf_register_hook() and cleanup_net() Michal Kubecek
2016-07-30 13:24 ` Eric W. Biederman
2016-08-01 12:34 ` Pablo Neira Ayuso
2016-08-26 17:31 ` Pablo Neira Ayuso
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.