Re: [PATCH v3 01/21] grace: replace BUG_ON by WARN_ONCE in exit_net hook

Re: [PATCH v3 01/21] grace: replace BUG_ON by WARN_ONCE in exit_net hook

[Alexey Dobriyan]

> -	BUG_ON(!list_empty(grace_list));
> +	WARN_ONCE(!list_empty(grace_list),
> +		  "net %x %s: grace_list is not empty\n",
> +		  net->ns.inum, __func__);

* printing __func__ is unnecessary as it will be on top of the stacktrace anyway,
* message duplicates condition
* printing netns id in netns ->exit hook also looks like pointless
  information: for kernel dumps you want struct net pointer right away.
  You wrote admins want to know which container triggered the message
  but what will they do with that information?


I'd say WARN_ONCE(!list_empty()) is enough.
BUG_ON if you ship with crashdumps by default.

[PATCH v2 00/21] exit_net checks for objects initialized in net_init hook

[Vasily Averin]

This patch set checks that lists initialized in net_init hooks were
return to initial state at end of net_exit hooks.

I hope such checks allows to detect leaked per-netns objects.
Also I hope that all new pernet_operations will inherit such checks too.

I assume that elements added into per-net lists should not live longer than net namespace,
and should be deleted from the list. I think exit_net hook is good place for such check.

Recently I've found lost list_entry and enabled timer on stop of net namespace.
Then I've reviewed all existing pernet_operations and found that many drivers
have such checks already. So I decided to complete this task and add such checks
into all affected subsystems.

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

Vasily Averin (21):
  exit_net cleanup: geneve sock_list check
  ppp: exit_net cleanup checks added
  vxlan: exit_net cleanup checks added
  netdev: exit_net cleanup check added
  nfs4blocklayout: exit_net cleanup check added
  nfs client: exit_net cleanup check added
  fib_notifier: exit_net cleanup check added
  fib_rules: exit_net cleanup check added
  clusterip: exit_net cleanup check added
  xfrm6_tunnel: exit_net cleanup check added
  af_key: replace BUG_ON on WARN_ON in net_exit hook
  l2tp: 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
  hashlimit: exit_net cleanup check added
  recent: exit_net cleanup check added
  packet: exit_net cleanup check added
  phonet: exit_net cleanup check added
  sunrpc: exit_net cleanup check added

 drivers/net/geneve.c               |  2 ++
 drivers/net/ppp/ppp_generic.c      |  4 ++++
 drivers/net/vxlan.c                |  6 ++++++
 fs/nfs/blocklayout/rpc_pipefs.c    |  2 ++
 fs/nfs/client.c                    |  4 ++++
 net/core/dev.c                     |  3 +++
 net/core/fib_notifier.c            |  7 +++++++
 net/core/fib_rules.c               |  7 +++++++
 net/ipv4/netfilter/ipt_CLUSTERIP.c |  2 ++
 net/ipv6/xfrm6_tunnel.c            | 12 ++++++++++++
 net/key/af_key.c                   |  2 +-
 net/l2tp/l2tp_core.c               |  6 ++++++
 net/netfilter/nf_tables_api.c      |  9 +++++++++
 net/netfilter/nfnetlink_log.c      |  6 ++++++
 net/netfilter/nfnetlink_queue.c    |  7 +++++++
 net/netfilter/x_tables.c           | 10 ++++++++++
 net/netfilter/xt_hashlimit.c       |  4 ++++
 net/netfilter/xt_recent.c          |  4 ++++
 net/packet/af_packet.c             |  2 ++
 net/phonet/pn_dev.c                |  4 ++++
 net/sunrpc/sunrpc_syms.c           |  4 ++++
 21 files changed, 106 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v3 01/21] grace: replace BUG_ON by WARN_ONCE in exit_net hook

[Vasily Averin]

Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
---
 fs/nfs_common/grace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfs_common/grace.c b/fs/nfs_common/grace.c
index 420d3a0..1bd6599 100644
--- a/fs/nfs_common/grace.c
+++ b/fs/nfs_common/grace.c
@@ -104,7 +104,9 @@ grace_exit_net(struct net *net)
 {
 	struct list_head *grace_list = net_generic(net, grace_net_id);
 
-	BUG_ON(!list_empty(grace_list));
+	WARN_ONCE(!list_empty(grace_list),
+		  "net %x %s: grace_list is not empty\n",
+		  net->ns.inum, __func__);
 }
 
 static struct pernet_operations grace_net_ops = {
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 01/21] grace: replace BUG_ON by WARN_ONCE in exit_net hook

[J. Bruce Fields]

I'm assuming you want me to apply this (and the following lockd patch),
but I'm not clear why this is part of a bigger series going to netdev.

--b.

On Mon, Nov 06, 2017 at 04:22:48PM +0300, Vasily Averin wrote:
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  fs/nfs_common/grace.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs_common/grace.c b/fs/nfs_common/grace.c
> index 420d3a0..1bd6599 100644
> --- a/fs/nfs_common/grace.c
> +++ b/fs/nfs_common/grace.c
> @@ -104,7 +104,9 @@ grace_exit_net(struct net *net)
>  {
>  	struct list_head *grace_list = net_generic(net, grace_net_id);
>  
> -	BUG_ON(!list_empty(grace_list));
> +	WARN_ONCE(!list_empty(grace_list),
> +		  "net %x %s: grace_list is not empty\n",
> +		  net->ns.inum, __func__);
>  }
>  
>  static struct pernet_operations grace_net_ops = {
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 01/21] grace: replace BUG_ON by WARN_ONCE in exit_net hook

[J. Bruce Fields]

Applied for 4.15, thanks.--b.

On Mon, Nov 06, 2017 at 04:22:48PM +0300, Vasily Averin wrote:
> Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
> ---
>  fs/nfs_common/grace.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs_common/grace.c b/fs/nfs_common/grace.c
> index 420d3a0..1bd6599 100644
> --- a/fs/nfs_common/grace.c
> +++ b/fs/nfs_common/grace.c
> @@ -104,7 +104,9 @@ grace_exit_net(struct net *net)
>  {
>  	struct list_head *grace_list = net_generic(net, grace_net_id);
>  
> -	BUG_ON(!list_empty(grace_list));
> +	WARN_ONCE(!list_empty(grace_list),
> +		  "net %x %s: grace_list is not empty\n",
> +		  net->ns.inum, __func__);
>  }
>  
>  static struct pernet_operations grace_net_ops = {
> -- 
> 2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html