Collect sockets for leak detection

Collect sockets for leak detection

[Dan Smith]

The objhash doesn't currently collect sockets for leak detection.  The
second patch in this set adds that support for general sockets (collecting
the socket itself and any DEAD sockets referenced in their read/write
queues) as well as a proto_ops operation for a per-protocol collection
routine.

I separated out the first patch in this set to make sure it's called out
for discussion as I expect it will be contentious.  Since sockets can
have a refcount of zero while remaining valid, the assumption that the
objhash makes about the refcount (which is correct in other cases) doesn't
hold true for sockets that are DEAD and only hanging around because of
an outstanding socket buffer.

[PATCH 1/2] Adjust socket reference count during leak detection phase

[Dan Smith]

Sockets that have been disconnected from their struct file have
a reference count one less than normal sockets (potentially zero if the
socket is only in the system because of an outstanding skb).  Since the
objhash assumes a minimum reference count of 2, skb's in such a situation
will always be counted as a leak.

This (probably contentious) patch adds a fixup function during the leak
detection phase to adjust SOCK_DEAD sockets' reference counts down by
one to match reality.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/objhash.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index b85aa77..ee9dbfd 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -816,6 +816,26 @@ static void ckpt_obj_users_inc(struct ckpt_ctx *ctx, void *ptr, int increment)
  */
 
 /**
+ * obj_sock_adjust_users - remove implicit reference on DEAD sockets
+ * @obj: CKPT_OBJ_SOCK object to adjust
+ *
+ * Sockets that have been disconnected from their struct file have
+ * a reference count one less than normal sockets.  The objhash's
+ * assumption of such a reference is therefore incorrect, so we correct
+ * it here.
+ */
+static inline void obj_sock_adjust_users(struct ckpt_obj *obj)
+{
+	struct sock *sk = (struct sock *)obj->ptr;
+
+	if (sock_flag(sk, SOCK_DEAD)) {
+		obj->users--;
+		ckpt_debug("Adjusting SOCK %i count to %i\n",
+			   obj->objref, obj->users);
+	}
+}
+
+/**
  * ckpt_obj_contained - test if shared objects are contained in checkpoint
  * @ctx: checkpoint context
  *
@@ -838,6 +858,10 @@ int ckpt_obj_contained(struct ckpt_ctx *ctx)
 	hlist_for_each_entry(obj, node, &ctx->obj_hash->list, next) {
 		if (!obj->ops->ref_users)
 			continue;
+
+		if (obj->ops->obj_type == CKPT_OBJ_SOCK)
+			obj_sock_adjust_users(obj);
+
 		if (obj->ops->ref_users(obj->ptr) != obj->users) {
 			ckpt_debug("usage leak: %s\n", obj->ops->obj_name);
 			ckpt_write_err(ctx, "OP", "leak: usage (%d != %d (%s)",
-- 
1.6.2.5

[PATCH 2/2] Collect sockets

[Dan Smith]

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/objhash.c  |    6 +++++
 include/net/af_unix.h |    2 +
 include/net/sock.h    |    1 +
 net/checkpoint.c      |   59 +++++++++++++++++++++++++++++++++++++++++++++++++
 net/socket.c          |    1 +
 net/unix/af_unix.c    |    3 ++
 net/unix/checkpoint.c |   15 ++++++++++++
 7 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index ee9dbfd..96634b0 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -280,6 +280,11 @@ static int obj_tty_users(void *ptr)
 	return atomic_read(&((struct tty_struct *) ptr)->kref.refcount);
 }
 
+static int obj_sock_users(void *ptr)
+{
+	return atomic_read(&((struct sock *) ptr)->sk_refcnt);
+}
+
 static struct ckpt_obj_ops ckpt_obj_ops[] = {
 	/* ignored object */
 	{
@@ -414,6 +419,7 @@ static struct ckpt_obj_ops ckpt_obj_ops[] = {
 		.obj_type = CKPT_OBJ_SOCK,
 		.ref_drop = obj_sock_drop,
 		.ref_grab = obj_sock_grab,
+		.ref_users = obj_sock_users,
 		.checkpoint = checkpoint_sock,
 		.restore = restore_sock,
 	},
diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 61f666b..e42a714 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -75,6 +75,8 @@ struct ckpt_hdr_socket;
 extern int unix_checkpoint(struct ckpt_ctx *ctx, struct socket *sock);
 extern int unix_restore(struct ckpt_ctx *ctx, struct socket *sock,
 			struct ckpt_hdr_socket *h);
+extern int unix_collect(struct ckpt_ctx *ctx, struct socket *sock);
+
 #else
 #define unix_checkpoint NULL
 #define unix_restore NULL
diff --git a/include/net/sock.h b/include/net/sock.h
index 1100a5c..ec351f9 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1655,6 +1655,7 @@ extern void *restore_sock(struct ckpt_ctx *ctx);
 extern int sock_file_checkpoint(struct ckpt_ctx *ctx, struct file *file);
 extern struct file *sock_file_restore(struct ckpt_ctx *ctx,
 				      struct ckpt_hdr_file *h);
+extern int sock_file_collect(struct ckpt_ctx *ctx, struct file *file);
 #endif
 
 #endif	/* _SOCK_H */
diff --git a/net/checkpoint.c b/net/checkpoint.c
index 626b8ef..a11ec7a 100644
--- a/net/checkpoint.c
+++ b/net/checkpoint.c
@@ -592,6 +592,65 @@ int sock_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
 	return ret;
 }
 
+static int sock_collect_skbs(struct ckpt_ctx *ctx, struct sk_buff_head *queue)
+{
+	struct sk_buff_head tmpq;
+	struct sk_buff *skb;
+	int ret = 0;
+	int bytes;
+
+	skb_queue_head_init(&tmpq);
+
+	ret = sock_copy_buffers(queue, &tmpq, &bytes);
+	if (ret < 0)
+		return ret;
+
+	skb_queue_walk(&tmpq, skb) {
+		/* Socket buffers do not maintain a ref count on their
+		 * owning sock because they're counted in sock_wmem_alloc.
+		 * So, we only need to collect sockets from the queue that
+		 * won't be collected any other way (i.e. DEAD sockets that
+		 * are hanging around only because they're waiting for us
+		 * to process their skb.
+		 */
+
+		if (!ckpt_obj_lookup(ctx, skb->sk, CKPT_OBJ_SOCK) &&
+		    sock_flag(skb->sk, SOCK_DEAD)) {
+			ret = ckpt_obj_collect(ctx, skb->sk, CKPT_OBJ_SOCK);
+			if (ret < 0)
+				break;
+		}
+	}
+
+	__skb_queue_purge(&tmpq);
+
+	return ret;
+}
+
+int sock_file_collect(struct ckpt_ctx *ctx, struct file *file)
+{
+	struct socket *sock = file->private_data;
+	struct sock *sk = sock->sk;
+	int ret;
+
+	ret = sock_collect_skbs(ctx, &sk->sk_write_queue);
+	if (ret < 0)
+		return ret;
+
+	ret = sock_collect_skbs(ctx, &sk->sk_receive_queue);
+	if (ret < 0)
+		return ret;
+
+	ret = ckpt_obj_collect(ctx, sk, CKPT_OBJ_SOCK);
+	if (ret < 0)
+		return ret;
+
+	if (sock->ops->collect)
+		ret = sock->ops->collect(ctx, sock);
+
+	return ret;
+}
+
 static struct file *sock_alloc_attach_fd(struct socket *sock)
 {
 	struct file *file;
diff --git a/net/socket.c b/net/socket.c
index 2de0861..0a4d539 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -142,6 +142,7 @@ static const struct file_operations socket_file_ops = {
 	.splice_read =	sock_splice_read,
 #ifdef CONFIG_CHECKPOINT
 	.checkpoint =   sock_file_checkpoint,
+	.collect = sock_file_collect,
 #endif
 };
 
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index da6405d..b3d4f16 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -525,6 +525,7 @@ static const struct proto_ops unix_stream_ops = {
 	.sendpage =	sock_no_sendpage,
 	.checkpoint =	unix_checkpoint,
 	.restore =	unix_restore,
+	.collect =      unix_collect,
 };
 
 static const struct proto_ops unix_dgram_ops = {
@@ -548,6 +549,7 @@ static const struct proto_ops unix_dgram_ops = {
 	.sendpage =	sock_no_sendpage,
 	.checkpoint =	unix_checkpoint,
 	.restore =	unix_restore,
+	.collect =      unix_collect,
 };
 
 static const struct proto_ops unix_seqpacket_ops = {
@@ -571,6 +573,7 @@ static const struct proto_ops unix_seqpacket_ops = {
 	.sendpage =	sock_no_sendpage,
 	.checkpoint =	unix_checkpoint,
 	.restore =	unix_restore,
+	.collect =      unix_collect,
 };
 
 static struct proto unix_proto = {
diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
index 6a6114b..fd6d944 100644
--- a/net/unix/checkpoint.c
+++ b/net/unix/checkpoint.c
@@ -173,6 +173,21 @@ int unix_checkpoint(struct ckpt_ctx *ctx, struct socket *sock)
 	return ret;
 }
 
+int unix_collect(struct ckpt_ctx *ctx, struct socket *sock)
+{
+	struct unix_sock *sk = unix_sk(sock->sk);
+	int ret;
+
+	ret = ckpt_obj_collect(ctx, sock->sk, CKPT_OBJ_SOCK);
+	if (ret < 0)
+		return ret;
+
+	if (sk->peer)
+		ret = ckpt_obj_collect(ctx, sk->peer, CKPT_OBJ_SOCK);
+
+	return 0;
+}
+
 static int sock_read_buffer_sendmsg(struct ckpt_ctx *ctx,
 				    struct sockaddr *addr,
 				    unsigned int addrlen)
-- 
1.6.2.5

Re: [PATCH 1/2] Adjust socket reference count during leak detection phase

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Sockets that have been disconnected from their struct file have
> a reference count one less than normal sockets (potentially zero if the
> socket is only in the system because of an outstanding skb).  Since the
> objhash assumes a minimum reference count of 2, skb's in such a situation
> will always be counted as a leak.
> 
> This (probably contentious) patch adds a fixup function during the leak

This is IMO the right place for it though.  If we have to play
games with refcounts to pacify leak detection, then all such
games should be played together in ckpt_obj_contained.

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>


> detection phase to adjust SOCK_DEAD sockets' reference counts down by
> one to match reality.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/objhash.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
> index b85aa77..ee9dbfd 100644
> --- a/checkpoint/objhash.c
> +++ b/checkpoint/objhash.c
> @@ -816,6 +816,26 @@ static void ckpt_obj_users_inc(struct ckpt_ctx *ctx, void *ptr, int increment)
>   */
> 
>  /**
> + * obj_sock_adjust_users - remove implicit reference on DEAD sockets
> + * @obj: CKPT_OBJ_SOCK object to adjust
> + *
> + * Sockets that have been disconnected from their struct file have
> + * a reference count one less than normal sockets.  The objhash's
> + * assumption of such a reference is therefore incorrect, so we correct
> + * it here.
> + */
> +static inline void obj_sock_adjust_users(struct ckpt_obj *obj)
> +{
> +	struct sock *sk = (struct sock *)obj->ptr;
> +
> +	if (sock_flag(sk, SOCK_DEAD)) {
> +		obj->users--;
> +		ckpt_debug("Adjusting SOCK %i count to %i\n",
> +			   obj->objref, obj->users);
> +	}
> +}
> +
> +/**
>   * ckpt_obj_contained - test if shared objects are contained in checkpoint
>   * @ctx: checkpoint context
>   *
> @@ -838,6 +858,10 @@ int ckpt_obj_contained(struct ckpt_ctx *ctx)
>  	hlist_for_each_entry(obj, node, &ctx->obj_hash->list, next) {
>  		if (!obj->ops->ref_users)
>  			continue;
> +
> +		if (obj->ops->obj_type == CKPT_OBJ_SOCK)
> +			obj_sock_adjust_users(obj);
> +
>  		if (obj->ops->ref_users(obj->ptr) != obj->users) {
>  			ckpt_debug("usage leak: %s\n", obj->ops->obj_name);
>  			ckpt_write_err(ctx, "OP", "leak: usage (%d != %d (%s)",
> -- 
> 1.6.2.5
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH 2/2] Collect sockets

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Don't see any gotchas here...

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  checkpoint/objhash.c  |    6 +++++
>  include/net/af_unix.h |    2 +
>  include/net/sock.h    |    1 +
>  net/checkpoint.c      |   59 +++++++++++++++++++++++++++++++++++++++++++++++++
>  net/socket.c          |    1 +
>  net/unix/af_unix.c    |    3 ++
>  net/unix/checkpoint.c |   15 ++++++++++++
>  7 files changed, 87 insertions(+), 0 deletions(-)
> 
> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
> index ee9dbfd..96634b0 100644
> --- a/checkpoint/objhash.c
> +++ b/checkpoint/objhash.c
> @@ -280,6 +280,11 @@ static int obj_tty_users(void *ptr)
>  	return atomic_read(&((struct tty_struct *) ptr)->kref.refcount);
>  }
> 
> +static int obj_sock_users(void *ptr)
> +{
> +	return atomic_read(&((struct sock *) ptr)->sk_refcnt);
> +}
> +
>  static struct ckpt_obj_ops ckpt_obj_ops[] = {
>  	/* ignored object */
>  	{
> @@ -414,6 +419,7 @@ static struct ckpt_obj_ops ckpt_obj_ops[] = {
>  		.obj_type = CKPT_OBJ_SOCK,
>  		.ref_drop = obj_sock_drop,
>  		.ref_grab = obj_sock_grab,
> +		.ref_users = obj_sock_users,
>  		.checkpoint = checkpoint_sock,
>  		.restore = restore_sock,
>  	},
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index 61f666b..e42a714 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -75,6 +75,8 @@ struct ckpt_hdr_socket;
>  extern int unix_checkpoint(struct ckpt_ctx *ctx, struct socket *sock);
>  extern int unix_restore(struct ckpt_ctx *ctx, struct socket *sock,
>  			struct ckpt_hdr_socket *h);
> +extern int unix_collect(struct ckpt_ctx *ctx, struct socket *sock);
> +
>  #else
>  #define unix_checkpoint NULL
>  #define unix_restore NULL
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 1100a5c..ec351f9 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1655,6 +1655,7 @@ extern void *restore_sock(struct ckpt_ctx *ctx);
>  extern int sock_file_checkpoint(struct ckpt_ctx *ctx, struct file *file);
>  extern struct file *sock_file_restore(struct ckpt_ctx *ctx,
>  				      struct ckpt_hdr_file *h);
> +extern int sock_file_collect(struct ckpt_ctx *ctx, struct file *file);
>  #endif
> 
>  #endif	/* _SOCK_H */
> diff --git a/net/checkpoint.c b/net/checkpoint.c
> index 626b8ef..a11ec7a 100644
> --- a/net/checkpoint.c
> +++ b/net/checkpoint.c
> @@ -592,6 +592,65 @@ int sock_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
>  	return ret;
>  }
> 
> +static int sock_collect_skbs(struct ckpt_ctx *ctx, struct sk_buff_head *queue)
> +{
> +	struct sk_buff_head tmpq;
> +	struct sk_buff *skb;
> +	int ret = 0;
> +	int bytes;
> +
> +	skb_queue_head_init(&tmpq);
> +
> +	ret = sock_copy_buffers(queue, &tmpq, &bytes);
> +	if (ret < 0)
> +		return ret;
> +
> +	skb_queue_walk(&tmpq, skb) {
> +		/* Socket buffers do not maintain a ref count on their
> +		 * owning sock because they're counted in sock_wmem_alloc.
> +		 * So, we only need to collect sockets from the queue that
> +		 * won't be collected any other way (i.e. DEAD sockets that
> +		 * are hanging around only because they're waiting for us
> +		 * to process their skb.
> +		 */
> +
> +		if (!ckpt_obj_lookup(ctx, skb->sk, CKPT_OBJ_SOCK) &&
> +		    sock_flag(skb->sk, SOCK_DEAD)) {
> +			ret = ckpt_obj_collect(ctx, skb->sk, CKPT_OBJ_SOCK);
> +			if (ret < 0)
> +				break;
> +		}
> +	}
> +
> +	__skb_queue_purge(&tmpq);
> +
> +	return ret;
> +}
> +
> +int sock_file_collect(struct ckpt_ctx *ctx, struct file *file)
> +{
> +	struct socket *sock = file->private_data;
> +	struct sock *sk = sock->sk;
> +	int ret;
> +
> +	ret = sock_collect_skbs(ctx, &sk->sk_write_queue);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sock_collect_skbs(ctx, &sk->sk_receive_queue);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ckpt_obj_collect(ctx, sk, CKPT_OBJ_SOCK);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (sock->ops->collect)
> +		ret = sock->ops->collect(ctx, sock);
> +
> +	return ret;
> +}
> +
>  static struct file *sock_alloc_attach_fd(struct socket *sock)
>  {
>  	struct file *file;
> diff --git a/net/socket.c b/net/socket.c
> index 2de0861..0a4d539 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -142,6 +142,7 @@ static const struct file_operations socket_file_ops = {
>  	.splice_read =	sock_splice_read,
>  #ifdef CONFIG_CHECKPOINT
>  	.checkpoint =   sock_file_checkpoint,
> +	.collect = sock_file_collect,
>  #endif
>  };
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index da6405d..b3d4f16 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -525,6 +525,7 @@ static const struct proto_ops unix_stream_ops = {
>  	.sendpage =	sock_no_sendpage,
>  	.checkpoint =	unix_checkpoint,
>  	.restore =	unix_restore,
> +	.collect =      unix_collect,
>  };
> 
>  static const struct proto_ops unix_dgram_ops = {
> @@ -548,6 +549,7 @@ static const struct proto_ops unix_dgram_ops = {
>  	.sendpage =	sock_no_sendpage,
>  	.checkpoint =	unix_checkpoint,
>  	.restore =	unix_restore,
> +	.collect =      unix_collect,
>  };
> 
>  static const struct proto_ops unix_seqpacket_ops = {
> @@ -571,6 +573,7 @@ static const struct proto_ops unix_seqpacket_ops = {
>  	.sendpage =	sock_no_sendpage,
>  	.checkpoint =	unix_checkpoint,
>  	.restore =	unix_restore,
> +	.collect =      unix_collect,
>  };
> 
>  static struct proto unix_proto = {
> diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
> index 6a6114b..fd6d944 100644
> --- a/net/unix/checkpoint.c
> +++ b/net/unix/checkpoint.c
> @@ -173,6 +173,21 @@ int unix_checkpoint(struct ckpt_ctx *ctx, struct socket *sock)
>  	return ret;
>  }
> 
> +int unix_collect(struct ckpt_ctx *ctx, struct socket *sock)
> +{
> +	struct unix_sock *sk = unix_sk(sock->sk);
> +	int ret;
> +
> +	ret = ckpt_obj_collect(ctx, sock->sk, CKPT_OBJ_SOCK);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (sk->peer)
> +		ret = ckpt_obj_collect(ctx, sk->peer, CKPT_OBJ_SOCK);
> +
> +	return 0;
> +}
> +
>  static int sock_read_buffer_sendmsg(struct ckpt_ctx *ctx,
>  				    struct sockaddr *addr,
>  				    unsigned int addrlen)
> -- 
> 1.6.2.5
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH 2/2] Collect sockets

[Oren Laadan]

Looks good, pulled for v18.

Dan Smith wrote:
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/objhash.c  |    6 +++++
>  include/net/af_unix.h |    2 +
>  include/net/sock.h    |    1 +
>  net/checkpoint.c      |   59 +++++++++++++++++++++++++++++++++++++++++++++++++
>  net/socket.c          |    1 +
>  net/unix/af_unix.c    |    3 ++
>  net/unix/checkpoint.c |   15 ++++++++++++
>  7 files changed, 87 insertions(+), 0 deletions(-)
> 
> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
> index ee9dbfd..96634b0 100644
> --- a/checkpoint/objhash.c
> +++ b/checkpoint/objhash.c
> @@ -280,6 +280,11 @@ static int obj_tty_users(void *ptr)
>  	return atomic_read(&((struct tty_struct *) ptr)->kref.refcount);
>  }
>  
> +static int obj_sock_users(void *ptr)
> +{
> +	return atomic_read(&((struct sock *) ptr)->sk_refcnt);
> +}
> +
>  static struct ckpt_obj_ops ckpt_obj_ops[] = {
>  	/* ignored object */
>  	{
> @@ -414,6 +419,7 @@ static struct ckpt_obj_ops ckpt_obj_ops[] = {
>  		.obj_type = CKPT_OBJ_SOCK,
>  		.ref_drop = obj_sock_drop,
>  		.ref_grab = obj_sock_grab,
> +		.ref_users = obj_sock_users,
>  		.checkpoint = checkpoint_sock,
>  		.restore = restore_sock,
>  	},
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index 61f666b..e42a714 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -75,6 +75,8 @@ struct ckpt_hdr_socket;
>  extern int unix_checkpoint(struct ckpt_ctx *ctx, struct socket *sock);
>  extern int unix_restore(struct ckpt_ctx *ctx, struct socket *sock,
>  			struct ckpt_hdr_socket *h);
> +extern int unix_collect(struct ckpt_ctx *ctx, struct socket *sock);
> +
>  #else
>  #define unix_checkpoint NULL
>  #define unix_restore NULL
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 1100a5c..ec351f9 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1655,6 +1655,7 @@ extern void *restore_sock(struct ckpt_ctx *ctx);
>  extern int sock_file_checkpoint(struct ckpt_ctx *ctx, struct file *file);
>  extern struct file *sock_file_restore(struct ckpt_ctx *ctx,
>  				      struct ckpt_hdr_file *h);
> +extern int sock_file_collect(struct ckpt_ctx *ctx, struct file *file);
>  #endif
>  
>  #endif	/* _SOCK_H */
> diff --git a/net/checkpoint.c b/net/checkpoint.c
> index 626b8ef..a11ec7a 100644
> --- a/net/checkpoint.c
> +++ b/net/checkpoint.c
> @@ -592,6 +592,65 @@ int sock_file_checkpoint(struct ckpt_ctx *ctx, struct file *file)
>  	return ret;
>  }
>  
> +static int sock_collect_skbs(struct ckpt_ctx *ctx, struct sk_buff_head *queue)
> +{
> +	struct sk_buff_head tmpq;
> +	struct sk_buff *skb;
> +	int ret = 0;
> +	int bytes;
> +
> +	skb_queue_head_init(&tmpq);
> +
> +	ret = sock_copy_buffers(queue, &tmpq, &bytes);
> +	if (ret < 0)
> +		return ret;
> +
> +	skb_queue_walk(&tmpq, skb) {
> +		/* Socket buffers do not maintain a ref count on their
> +		 * owning sock because they're counted in sock_wmem_alloc.
> +		 * So, we only need to collect sockets from the queue that
> +		 * won't be collected any other way (i.e. DEAD sockets that
> +		 * are hanging around only because they're waiting for us
> +		 * to process their skb.
> +		 */
> +
> +		if (!ckpt_obj_lookup(ctx, skb->sk, CKPT_OBJ_SOCK) &&
> +		    sock_flag(skb->sk, SOCK_DEAD)) {
> +			ret = ckpt_obj_collect(ctx, skb->sk, CKPT_OBJ_SOCK);
> +			if (ret < 0)
> +				break;
> +		}
> +	}
> +
> +	__skb_queue_purge(&tmpq);
> +
> +	return ret;
> +}
> +
> +int sock_file_collect(struct ckpt_ctx *ctx, struct file *file)
> +{
> +	struct socket *sock = file->private_data;
> +	struct sock *sk = sock->sk;
> +	int ret;
> +
> +	ret = sock_collect_skbs(ctx, &sk->sk_write_queue);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sock_collect_skbs(ctx, &sk->sk_receive_queue);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ckpt_obj_collect(ctx, sk, CKPT_OBJ_SOCK);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (sock->ops->collect)
> +		ret = sock->ops->collect(ctx, sock);
> +
> +	return ret;
> +}
> +
>  static struct file *sock_alloc_attach_fd(struct socket *sock)
>  {
>  	struct file *file;
> diff --git a/net/socket.c b/net/socket.c
> index 2de0861..0a4d539 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -142,6 +142,7 @@ static const struct file_operations socket_file_ops = {
>  	.splice_read =	sock_splice_read,
>  #ifdef CONFIG_CHECKPOINT
>  	.checkpoint =   sock_file_checkpoint,
> +	.collect = sock_file_collect,
>  #endif
>  };
>  
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index da6405d..b3d4f16 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -525,6 +525,7 @@ static const struct proto_ops unix_stream_ops = {
>  	.sendpage =	sock_no_sendpage,
>  	.checkpoint =	unix_checkpoint,
>  	.restore =	unix_restore,
> +	.collect =      unix_collect,
>  };
>  
>  static const struct proto_ops unix_dgram_ops = {
> @@ -548,6 +549,7 @@ static const struct proto_ops unix_dgram_ops = {
>  	.sendpage =	sock_no_sendpage,
>  	.checkpoint =	unix_checkpoint,
>  	.restore =	unix_restore,
> +	.collect =      unix_collect,
>  };
>  
>  static const struct proto_ops unix_seqpacket_ops = {
> @@ -571,6 +573,7 @@ static const struct proto_ops unix_seqpacket_ops = {
>  	.sendpage =	sock_no_sendpage,
>  	.checkpoint =	unix_checkpoint,
>  	.restore =	unix_restore,
> +	.collect =      unix_collect,
>  };
>  
>  static struct proto unix_proto = {
> diff --git a/net/unix/checkpoint.c b/net/unix/checkpoint.c
> index 6a6114b..fd6d944 100644
> --- a/net/unix/checkpoint.c
> +++ b/net/unix/checkpoint.c
> @@ -173,6 +173,21 @@ int unix_checkpoint(struct ckpt_ctx *ctx, struct socket *sock)
>  	return ret;
>  }
>  
> +int unix_collect(struct ckpt_ctx *ctx, struct socket *sock)
> +{
> +	struct unix_sock *sk = unix_sk(sock->sk);
> +	int ret;
> +
> +	ret = ckpt_obj_collect(ctx, sock->sk, CKPT_OBJ_SOCK);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (sk->peer)
> +		ret = ckpt_obj_collect(ctx, sk->peer, CKPT_OBJ_SOCK);
> +
> +	return 0;
> +}
> +
>  static int sock_read_buffer_sendmsg(struct ckpt_ctx *ctx,
>  				    struct sockaddr *addr,
>  				    unsigned int addrlen)

Re: [PATCH 1/2] Adjust socket reference count during leak detection phase

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
>> Sockets that have been disconnected from their struct file have
>> a reference count one less than normal sockets (potentially zero if the
>> socket is only in the system because of an outstanding skb).  Since the
>> objhash assumes a minimum reference count of 2, skb's in such a situation
>> will always be counted as a leak.
>>
>> This (probably contentious) patch adds a fixup function during the leak
> 
> This is IMO the right place for it though.  If we have to play
> games with refcounts to pacify leak detection, then all such
> games should be played together in ckpt_obj_contained.
> 
> Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

I concur, it's a leak-detection specific code.
Pulled for v18.

Oren.

> 
> 
>> detection phase to adjust SOCK_DEAD sockets' reference counts down by
>> one to match reality.
>>
>> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>> ---
>>  checkpoint/objhash.c |   24 ++++++++++++++++++++++++
>>  1 files changed, 24 insertions(+), 0 deletions(-)
>>
>> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
>> index b85aa77..ee9dbfd 100644
>> --- a/checkpoint/objhash.c
>> +++ b/checkpoint/objhash.c
>> @@ -816,6 +816,26 @@ static void ckpt_obj_users_inc(struct ckpt_ctx *ctx, void *ptr, int increment)
>>   */
>>
>>  /**
>> + * obj_sock_adjust_users - remove implicit reference on DEAD sockets
>> + * @obj: CKPT_OBJ_SOCK object to adjust
>> + *
>> + * Sockets that have been disconnected from their struct file have
>> + * a reference count one less than normal sockets.  The objhash's
>> + * assumption of such a reference is therefore incorrect, so we correct
>> + * it here.
>> + */
>> +static inline void obj_sock_adjust_users(struct ckpt_obj *obj)
>> +{
>> +	struct sock *sk = (struct sock *)obj->ptr;
>> +
>> +	if (sock_flag(sk, SOCK_DEAD)) {
>> +		obj->users--;
>> +		ckpt_debug("Adjusting SOCK %i count to %i\n",
>> +			   obj->objref, obj->users);
>> +	}
>> +}
>> +
>> +/**
>>   * ckpt_obj_contained - test if shared objects are contained in checkpoint
>>   * @ctx: checkpoint context
>>   *
>> @@ -838,6 +858,10 @@ int ckpt_obj_contained(struct ckpt_ctx *ctx)
>>  	hlist_for_each_entry(obj, node, &ctx->obj_hash->list, next) {
>>  		if (!obj->ops->ref_users)
>>  			continue;
>> +
>> +		if (obj->ops->obj_type == CKPT_OBJ_SOCK)
>> +			obj_sock_adjust_users(obj);
>> +
>>  		if (obj->ops->ref_users(obj->ptr) != obj->users) {
>>  			ckpt_debug("usage leak: %s\n", obj->ops->obj_name);
>>  			ckpt_write_err(ctx, "OP", "leak: usage (%d != %d (%s)",
>> -- 
>> 1.6.2.5
>>
>> _______________________________________________
>> Containers mailing list
>> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>> https://lists.linux-foundation.org/mailman/listinfo/containers
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers