[PATCH v4 00/18] exit_net checks for objects initialized in net_init hook

[PATCH v4 00/18] exit_net checks for objects initialized in net_init hook

[Vasily Averin]

OpenVz kernel team have a long history of fighting against namespace-related bugs,
some of them could be excluded by using simple checks described below.

One of typical errors is related to live cycle of namespaces:
usually objects created for some namespace should not live longer than namespace itself.

Such kind of issues can be invisible on usual systems where additional namespaces
are not used, because initial namespaces usually lives forever and never destroyed.

However in systems with namespaces it can lead to memory leaks or to use-after-free.
Both of them are critical for systems with running containers.
As you knows it's quite hard to find the reason of such issues,
especially in rarely-triggered scenarios on production nodes on default kernels
without specially enabled debug settings. Any additional hints can be useful here.

This patch set should help to detect some of these issues.
It is based on assumption that objects initialized in init hook of pernet_operations
should return to initial state until end of exit hook.

Many drivers and subsystems already have such checks, however I've found number
of places where list_empty check would be useful at least as smoke test.

These checks are useful for long-term stable kernels,
they allows to detect problems related to incomplete or incorrectly
backported patches.

Also this patch set replaces BUG_ON in existing checks:
memory leaks and possible memory corruptions are bad of course,
however in many cases they are not fatal 
and should not crash production hosts unconditionally.

Changes:
v4:
- excluded grace and lockd patches taken by Bruce Fields
- let's use WARN_ON_ONCE without any extra messages
   adobriyan@ is right, output of net Id gives nothing to host admin,
   and developers in any case will extract information from core dump
- updated description in cover letter
- dropped nfs4blocklayout patch: waitqueue check does not look useful
- patches was reordered to be per-subsystem grouped
- cover letter should be sent to all people included into cc: of any patches
- minor cosmetic changes in some patches

v3:
- use net->ns.inum as net Id
- removed patches for hashlimit and recent,
    they handle tables list in exit_net hook.
- added patches for grace and lockd

v2:
- net pointer removed from output
- fixed compilation for phonet driver


Vasily Averin (18):
  af_key: replace BUG_ON on WARN_ON in net_exit hook
  geneve: exit_net cleanup check added
  packet: exit_net cleanup check added
  vxlan: exit_net cleanup checks added
  netdev: exit_net cleanup check added
  fib_notifier: exit_net cleanup check added
  fib_rules: exit_net cleanup check added
  l2tp: exit_net cleanup check added
  clusterip: exit_net cleanup check added
  nf_tables: exit_net cleanup check added
  nfnetlink_log: exit_net cleanup check added
  nfnetlink_gueue: exit_net cleanup check added
  x_tables: exit_net cleanup check added
  nfs client: exit_net cleanup check added
  sunrpc: exit_net cleanup check added
  phonet: exit_net cleanup check added
  ppp: exit_net cleanup checks added
  xfrm6_tunnel: exit_net cleanup check added

 drivers/net/geneve.c               |  1 +
 drivers/net/ppp/ppp_generic.c      |  2 ++
 drivers/net/vxlan.c                |  5 +++++
 fs/nfs/inode.c                     |  4 ++++
 net/core/dev.c                     |  2 ++
 net/core/fib_notifier.c            |  6 ++++++
 net/core/fib_rules.c               |  6 ++++++
 net/ipv4/netfilter/ipt_CLUSTERIP.c |  1 +
 net/ipv6/xfrm6_tunnel.c            | 10 ++++++++++
 net/key/af_key.c                   |  2 +-
 net/l2tp/l2tp_core.c               |  5 +++++
 net/netfilter/nf_tables_api.c      |  7 +++++++
 net/netfilter/nfnetlink_log.c      |  5 +++++
 net/netfilter/nfnetlink_queue.c    |  6 ++++++
 net/netfilter/x_tables.c           | 10 ++++++++++
 net/packet/af_packet.c             |  1 +
 net/phonet/pn_dev.c                |  3 +++
 net/sunrpc/sunrpc_syms.c           |  3 +++
 18 files changed, 78 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v4 09/18] clusterip: exit_net cleanup check added

[Vasily Averin]

Be sure that configs list initialized in net_init hook was return
to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 net/ipv4/netfilter/ipt_CLUSTERIP.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index 17b4ca5..e35b8d0 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -819,6 +819,7 @@ static void clusterip_net_exit(struct net *net)
 	cn->procdir = NULL;
 #endif
 	nf_unregister_net_hook(net, &cip_arp_ops);
+	WARN_ON_ONCE(!list_empty(&cn->configs));
 }
 
 static struct pernet_operations clusterip_net_ops = {
-- 
2.7.4

[PATCH v4 10/18] nf_tables: exit_net cleanup check added

[Vasily Averin]

Be sure that lists initialized in net_init hook were return
to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 net/netfilter/nf_tables_api.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 64e1ee0..f432b53 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5778,6 +5778,12 @@ static int __net_init nf_tables_init_net(struct net *net)
 	return 0;
 }
 
+static void __net_exit nf_tables_exit_net(struct net *net)
+{
+	WARN_ON_ONCE(!list_empty(&net->nft.af_info));
+	WARN_ON_ONCE(!list_empty(&net->nft.commit_list));
+}
+
 int __nft_release_basechain(struct nft_ctx *ctx)
 {
 	struct nft_rule *rule, *nr;
@@ -5848,6 +5854,7 @@ static void __nft_release_afinfo(struct net *net, struct nft_af_info *afi)
 
 static struct pernet_operations nf_tables_net_ops = {
 	.init	= nf_tables_init_net,
+	.exit	= nf_tables_exit_net,
 };
 
 static int __init nf_tables_module_init(void)
-- 
2.7.4

[PATCH v4 11/18] nfnetlink_log: exit_net cleanup check added

[Vasily Averin]

Be sure that instance_table array initialized in net_init hook
was return to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 net/netfilter/nfnetlink_log.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index cad6498..80236a2 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -1093,10 +1093,15 @@ static int __net_init nfnl_log_net_init(struct net *net)
 
 static void __net_exit nfnl_log_net_exit(struct net *net)
 {
+	unsigned int i;
+	struct nfnl_log_net *log = nfnl_log_pernet(net);
 #ifdef CONFIG_PROC_FS
 	remove_proc_entry("nfnetlink_log", net->nf.proc_netfilter);
 #endif
 	nf_log_unset(net, &nfulnl_logger);
+	for (i = 0; i < INSTANCE_BUCKETS; i++)
+		if (WARN_ON_ONCE(!hlist_empty(&log->instance_table[i])))
+			break;
 }
 
 static struct pernet_operations nfnl_log_net_ops = {
-- 
2.7.4

[PATCH v4 12/18] nfnetlink_gueue: exit_net cleanup check added

[Vasily Averin]

Be sure that instance_table array initialized in net_init hook
was return to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 net/netfilter/nfnetlink_queue.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index c979662..fd41077 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1512,10 +1512,16 @@ static int __net_init nfnl_queue_net_init(struct net *net)
 
 static void __net_exit nfnl_queue_net_exit(struct net *net)
 {
+	unsigned int i;
+	struct nfnl_queue_net *q = nfnl_queue_pernet(net);
+
 	nf_unregister_queue_handler(net);
 #ifdef CONFIG_PROC_FS
 	remove_proc_entry("nfnetlink_queue", net->nf.proc_netfilter);
 #endif
+	for (i = 0; i < INSTANCE_BUCKETS; i++)
+		if (WARN_ON_ONCE(!hlist_empty(&q->instance_table[i])))
+			break;
 }
 
 static void nfnl_queue_net_exit_batch(struct list_head *net_exit_list)
-- 
2.7.4

[PATCH v4 13/18] x_tables: exit_net cleanup check added

[Vasily Averin]

Be sure that xt.tables array initialized in net_init hook was return
to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 net/netfilter/x_tables.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index d8571f4..119b670 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1714,8 +1714,18 @@ static int __net_init xt_net_init(struct net *net)
 	return 0;
 }
 
+static void __net_exit xt_net_exit(struct net *net)
+{
+	int i;
+
+	for (i = 0; i < NFPROTO_NUMPROTO; i++)
+		if (WARN_ON_ONCE(!list_empty(&net->xt.tables[i])))
+			break;
+}
+
 static struct pernet_operations xt_net_ops = {
 	.init = xt_net_init,
+	.exit = xt_net_exit,
 };
 
 static int __init xt_init(void)
-- 
2.7.4

Re: [PATCH v4 12/18] nfnetlink_gueue: exit_net cleanup check added

[Florian Westphal]

Vasily Averin <vvs@virtuozzo.com> wrote:
> Be sure that instance_table array initialized in net_init hook
> was return to initial state.

> +	for (i = 0; i < INSTANCE_BUCKETS; i++)
> +		if (WARN_ON_ONCE(!hlist_empty(&q->instance_table[i])))
> +			break;

This looks strange, why if/break?

Plain WARN_ON_ONCE should be enough, but thats a nit so:

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

Re: [PATCH v4 11/18] nfnetlink_log: exit_net cleanup check added

[Florian Westphal]

Vasily Averin <vvs@virtuozzo.com> wrote:
> Be sure that instance_table array initialized in net_init hook
> was return to initial state.

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

Re: [PATCH v4 12/18] nfnetlink_gueue: exit_net cleanup check added

[Vasily Averin]

On 2017-11-12 11:52, Florian Westphal wrote:
> Vasily Averin <vvs@virtuozzo.com> wrote:
>> Be sure that instance_table array initialized in net_init hook
>> was return to initial state.
> 
>> +	for (i = 0; i < INSTANCE_BUCKETS; i++)
>> +		if (WARN_ON_ONCE(!hlist_empty(&q->instance_table[i])))
>> +			break;
> 
> This looks strange, why if/break?

I did not want to generate huge number of messages on each non-empty hash bucket.

> Plain WARN_ON_ONCE should be enough, but thats a nit so:

Oh, you're right.
In first patch version WARN_ON  was used here.
I've missed that only first message will be printed with _ONCE check.

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

Re: [PATCH v4 11/18] nfnetlink_log: exit_net cleanup check added

[Sergei Shtylyov]

Hello!

On 11/12/2017 11:46 AM, Vasily Averin wrote:

> Be sure that instance_table array initialized in net_init hook
> was return to initial state.
> 
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>   net/netfilter/nfnetlink_log.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> index cad6498..80236a2 100644
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c
> @@ -1093,10 +1093,15 @@ static int __net_init nfnl_log_net_init(struct net *net)
>   
>   static void __net_exit nfnl_log_net_exit(struct net *net)
>   {
> +	unsigned int i;
> +	struct nfnl_log_net *log = nfnl_log_pernet(net);

    Need empty line after the declarations.

>   #ifdef CONFIG_PROC_FS
>   	remove_proc_entry("nfnetlink_log", net->nf.proc_netfilter);
>   #endif
>   	nf_log_unset(net, &nfulnl_logger);
> +	for (i = 0; i < INSTANCE_BUCKETS; i++)
> +		if (WARN_ON_ONCE(!hlist_empty(&log->instance_table[i])))
> +			break;
>   }
[...]

MBR, Sergei

[PATCH v5 0/5] netfilter: exit_net checks for objects initialized in net_init hook

[Vasily Averin]

OpenVz kernel team have a long history of fighting against namespace-related bugs,
some of them could be excluded by using simple checks described below.

One of typical errors is related to live cycle of namespaces:
usually objects created for some namespace should not live longer than namespace itself.

Such kind of issues can be invisible on usual systems where additional namespaces
are not used, because initial namespaces usually lives forever and never destroyed.

However in systems with namespaces it can lead to memory leaks or to use-after-free.
Both of them are critical for systems with running containers.
As you knows it's quite hard to find the reason of such issues,
especially in rarely-triggered scenarios on production nodes on default kernels
without specially enabled debug settings. Any additional hints can be useful here.

This patch set should help to detect some of these issues.
It is based on assumption that objects initialized in init hook of pernet_operations
should return to initial state until end of exit hook.

Many drivers and subsystems already have such checks, however I've found number
of places where list_empty check would be useful at least as smoke test.

These checks are useful for long-term stable kernels,
they allows to detect problems related to incomplete or incorrectly
backported patches.

Changes:
v5:
- fixed nit pointed by Florian Westphal
- netfilter patches are send separately to netfilter-devel@

v4:
- excluded grace and lockd patches taken by Bruce Fields
- let's use WARN_ON_ONCE without any extra messages
   adobriyan@ is right, output of net Id gives nothing to host admin,
   and developers in any case will extract information from core dump
- updated description in cover letter
- dropped nfs4blocklayout patch: waitqueue check does not look useful
- patches was reordered to be per-subsystem grouped
- cover letter should be sent to all people included into cc: of any patches
- minor cosmetic changes in some patches

v3:
- use net->ns.inum as net Id
- removed patches for hashlimit and recent,
    they handle tables list in exit_net hook.
- added patches for grace and lockd

v2:
- net pointer removed from output
- fixed compilation for phonet driver

Vasily Averin (5):
  clusterip: exit_net cleanup check added
  nf_tables: exit_net cleanup check added
  nfnetlink_log: exit_net cleanup check added
  nfnetlink_gueue: exit_net cleanup check added
  x_tables: exit_net cleanup check added

 net/ipv4/netfilter/ipt_CLUSTERIP.c | 1 +
 net/netfilter/nf_tables_api.c      | 7 +++++++
 net/netfilter/nfnetlink_log.c      | 4 ++++
 net/netfilter/nfnetlink_queue.c    | 5 +++++
 net/netfilter/x_tables.c           | 9 +++++++++
 5 files changed, 26 insertions(+)

-- 
2.7.4

[PATCH v5 1/5] clusterip: exit_net cleanup check added

[Vasily Averin]

Be sure that configs list initialized in net_init hook was return
to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 net/ipv4/netfilter/ipt_CLUSTERIP.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index 17b4ca5..e35b8d0 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -819,6 +819,7 @@ static void clusterip_net_exit(struct net *net)
 	cn->procdir = NULL;
 #endif
 	nf_unregister_net_hook(net, &cip_arp_ops);
+	WARN_ON_ONCE(!list_empty(&cn->configs));
 }
 
 static struct pernet_operations clusterip_net_ops = {
-- 
2.7.4

[PATCH v5 2/5] nf_tables: exit_net cleanup check added

[Vasily Averin]

Be sure that lists initialized in net_init hook were return
to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 net/netfilter/nf_tables_api.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 64e1ee0..f432b53 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5778,6 +5778,12 @@ static int __net_init nf_tables_init_net(struct net *net)
 	return 0;
 }
 
+static void __net_exit nf_tables_exit_net(struct net *net)
+{
+	WARN_ON_ONCE(!list_empty(&net->nft.af_info));
+	WARN_ON_ONCE(!list_empty(&net->nft.commit_list));
+}
+
 int __nft_release_basechain(struct nft_ctx *ctx)
 {
 	struct nft_rule *rule, *nr;
@@ -5848,6 +5854,7 @@ static void __nft_release_afinfo(struct net *net, struct nft_af_info *afi)
 
 static struct pernet_operations nf_tables_net_ops = {
 	.init	= nf_tables_init_net,
+	.exit	= nf_tables_exit_net,
 };
 
 static int __init nf_tables_module_init(void)
-- 
2.7.4

[PATCH v5 3/5] nfnetlink_log: exit_net cleanup check added

[Vasily Averin]

Be sure that instance_table array initialized in net_init hook
was return to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
Acked-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nfnetlink_log.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index cad6498..23d75b1 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -1093,10 +1093,14 @@ static int __net_init nfnl_log_net_init(struct net *net)
 
 static void __net_exit nfnl_log_net_exit(struct net *net)
 {
+	unsigned int i;
+	struct nfnl_log_net *log = nfnl_log_pernet(net);
 #ifdef CONFIG_PROC_FS
 	remove_proc_entry("nfnetlink_log", net->nf.proc_netfilter);
 #endif
 	nf_log_unset(net, &nfulnl_logger);
+	for (i = 0; i < INSTANCE_BUCKETS; i++)
+		WARN_ON_ONCE(!hlist_empty(&log->instance_table[i]));
 }
 
 static struct pernet_operations nfnl_log_net_ops = {
-- 
2.7.4

[PATCH v5 4/5] nfnetlink_gueue: exit_net cleanup check added

[Vasily Averin]

Be sure that instance_table array initialized in net_init hook
was return to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
Acked-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nfnetlink_queue.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index c979662..3cae6d8 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1512,10 +1512,15 @@ static int __net_init nfnl_queue_net_init(struct net *net)
 
 static void __net_exit nfnl_queue_net_exit(struct net *net)
 {
+	unsigned int i;
+	struct nfnl_queue_net *q = nfnl_queue_pernet(net);
+
 	nf_unregister_queue_handler(net);
 #ifdef CONFIG_PROC_FS
 	remove_proc_entry("nfnetlink_queue", net->nf.proc_netfilter);
 #endif
+	for (i = 0; i < INSTANCE_BUCKETS; i++)
+		WARN_ON_ONCE(!hlist_empty(&q->instance_table[i]));
 }
 
 static void nfnl_queue_net_exit_batch(struct list_head *net_exit_list)
-- 
2.7.4

[PATCH v5 5/5] x_tables: exit_net cleanup check added

[Vasily Averin]

Be sure that xt.tables array initialized in net_init hook was return
to initial state.

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 net/netfilter/x_tables.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index d8571f4..02c1abb 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1714,8 +1714,17 @@ static int __net_init xt_net_init(struct net *net)
 	return 0;
 }
 
+static void __net_exit xt_net_exit(struct net *net)
+{
+	int i;
+
+	for (i = 0; i < NFPROTO_NUMPROTO; i++)
+		WARN_ON_ONCE(!list_empty(&net->xt.tables[i]));
+}
+
 static struct pernet_operations xt_net_ops = {
 	.init = xt_net_init,
+	.exit = xt_net_exit,
 };
 
 static int __init xt_init(void)
-- 
2.7.4

Re: [PATCH v5 0/5] netfilter: exit_net checks for objects initialized in net_init hook

[Florian Westphal]

Vasily Averin <vvs@virtuozzo.com> wrote:
> OpenVz kernel team have a long history of fighting against namespace-related bugs,
> some of them could be excluded by using simple checks described below.
> 
> One of typical errors is related to live cycle of namespaces:
> usually objects created for some namespace should not live longer than namespace itself.

These changes look good to me, thank you.

Re: [PATCH v5 0/5] netfilter: exit_net checks for objects initialized in net_init hook

[Pablo Neira Ayuso]

Hi Vasily,

On Sun, Nov 12, 2017 at 02:32:14PM +0300, Vasily Averin wrote:
> OpenVz kernel team have a long history of fighting against namespace-related bugs,
> some of them could be excluded by using simple checks described below.

I'm folding this series into one single patch, description looks like
this:

    netfilter: exit_net cleanup check added

    Be sure that lists initialized in net_init hook was return to initial
    state.

I understand your goal is to make it easier for review, but given this
is all part of the same logic change, I just hope you don't mind I
have squashed them into one single patch like I did.

Thanks.