Avoid leaking unattached socket objects on restore

Avoid leaking unattached socket objects on restore

[Dan Smith]

When we restore socket objects that are not attached to a file, we leak
the struct socket object because only an fput(file) results in the
necessary sock_release().

This set provides a proposed solution to that problem by adding a cleanup
operation to the objhash operations structure which is called at objhash
teardown time.  This gives us the opportunity to call sock_release() on
sockets that we have put into the hash that were never attached to a file
and thus avoid the leak.

[PATCH 1/2] Add a 'cleanup' function to objhash object operations

[Dan Smith]

This patch adds a 'cleanup' function to the object operations structure,
which is called during objhash teardown before the final reference is
dropped.  It provides a way to perform some object cleanup when checkpoint
or restart operation is finished, in scenarios where the final reference
count cannot be anticipated.

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

diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index a410346..9750483 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -34,6 +34,7 @@ struct ckpt_obj_ops {
 	int (*ref_users)(void *ptr);
 	int (*checkpoint)(struct ckpt_ctx *ctx, void *ptr);
 	void *(*restore)(struct ckpt_ctx *ctx);
+	void (*cleanup)(void *ptr);
 };
 
 struct ckpt_obj {
@@ -399,6 +400,8 @@ static void obj_hash_clear(struct ckpt_obj_hash *obj_hash)
 
 	for (i = 0; i < CKPT_OBJ_HASH_TOTAL; i++) {
 		hlist_for_each_entry_safe(obj, n, t, &h[i], hash) {
+			if (obj->ops->cleanup)
+				obj->ops->cleanup(obj->ptr);
 			obj->ops->ref_drop(obj->ptr);
 			kfree(obj);
 		}
-- 
1.6.2.5

[PATCH 2/2] Add a cleanup routine for objhash socket objects

[Dan Smith]

This cleanup routine checks for unattached sockets that we instantiated
and calls sock_release() on them to avoid leaking the struct socket when
their buffers are consumed and the struct sock is free'd.

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

diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index 9750483..5626707 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -247,6 +247,18 @@ static void obj_sock_drop(void *ptr)
 	sock_put((struct sock *) ptr);
 }
 
+static void cleanup_sock(void *ptr)
+{
+	struct sock *sk = (struct sock *) ptr;
+
+	if (sk->sk_socket && !sk->sk_socket->file) {
+		struct socket *sock = sk->sk_socket;
+		sock_orphan(sk);
+		sock->sk = NULL;
+		sock_release(sock);
+	}
+}
+
 static struct ckpt_obj_ops ckpt_obj_ops[] = {
 	/* ignored object */
 	{
@@ -384,6 +396,7 @@ static struct ckpt_obj_ops ckpt_obj_ops[] = {
 		.ref_grab = obj_sock_grab,
 		.checkpoint = checkpoint_sock,
 		.restore = restore_sock,
+		.cleanup = cleanup_sock,
 	},
 };
 
-- 
1.6.2.5

Re: [PATCH 2/2] Add a cleanup routine for objhash socket objects

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> This cleanup routine checks for unattached sockets that we instantiated
> and calls sock_release() on them to avoid leaking the struct socket when
> their buffers are consumed and the struct sock is free'd.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

I realize it's being discussed on irc, but it looks good to me, so
whatever variation you end up doing,

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

> ---
>  checkpoint/objhash.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
> index 9750483..5626707 100644
> --- a/checkpoint/objhash.c
> +++ b/checkpoint/objhash.c
> @@ -247,6 +247,18 @@ static void obj_sock_drop(void *ptr)
>  	sock_put((struct sock *) ptr);
>  }
> 
> +static void cleanup_sock(void *ptr)
> +{
> +	struct sock *sk = (struct sock *) ptr;
> +
> +	if (sk->sk_socket && !sk->sk_socket->file) {
> +		struct socket *sock = sk->sk_socket;
> +		sock_orphan(sk);
> +		sock->sk = NULL;
> +		sock_release(sock);
> +	}
> +}
> +
>  static struct ckpt_obj_ops ckpt_obj_ops[] = {
>  	/* ignored object */
>  	{
> @@ -384,6 +396,7 @@ static struct ckpt_obj_ops ckpt_obj_ops[] = {
>  		.ref_grab = obj_sock_grab,
>  		.checkpoint = checkpoint_sock,
>  		.restore = restore_sock,
> +		.cleanup = cleanup_sock,
>  	},
>  };
> 
> -- 
> 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] Add a cleanup routine for objhash socket objects

[Matt Helsley]

On Tue, Sep 15, 2009 at 09:52:11AM -0700, Dan Smith wrote:
> This cleanup routine checks for unattached sockets that we instantiated
> and calls sock_release() on them to avoid leaking the struct socket when
> their buffers are consumed and the struct sock is free'd.

Does it make sense to add a generic (cleanup) operation when only one object
type will make use of it? If we add generic mechanisms for things
without having multiple uses then are we just obfuscating the special
cases of the code? (Another example of this dilemma was my file table
deferqueue before you introduced a second use of it...) Or do you
anticipate other particular uses of "cleanup"?

As an alternative, would it work if we  kept a list of unattached sockets in
the ckpt context, removed them whenever they become attached, and then use the
generic "end of restart" deferqueue to cleanup unattached sockets?

Cheers,
	-Matt Helsley

> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/objhash.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
> index 9750483..5626707 100644
> --- a/checkpoint/objhash.c
> +++ b/checkpoint/objhash.c
> @@ -247,6 +247,18 @@ static void obj_sock_drop(void *ptr)
>  	sock_put((struct sock *) ptr);
>  }
> 
> +static void cleanup_sock(void *ptr)
> +{
> +	struct sock *sk = (struct sock *) ptr;
> +
> +	if (sk->sk_socket && !sk->sk_socket->file) {

Would it make sense to add a little helper to explain this?

	if (!is_sk_attached(sk)) {
...

examples where this _might_ be nicely re-used:

net/core/sock.c (sk_send_sigurg())
ipv4/netfilter/ipt_LOG.c (dump_packet())
ipv6/netfilter/ip6t_LOG.c (dump_packet())
net/netfilter/nfnetlink_log.c (__build_packet_message())
net/sctp/socket.c (__sctp_connect() -- though this adds a redundant
					sk_socket NULL check...)

> +		struct socket *sock = sk->sk_socket;
> +		sock_orphan(sk);
> +		sock->sk = NULL;
> +		sock_release(sock);
> +	}
> +}
> +
>  static struct ckpt_obj_ops ckpt_obj_ops[] = {
>  	/* ignored object */
>  	{
> @@ -384,6 +396,7 @@ static struct ckpt_obj_ops ckpt_obj_ops[] = {
>  		.ref_grab = obj_sock_grab,
>  		.checkpoint = checkpoint_sock,
>  		.restore = restore_sock,
> +		.cleanup = cleanup_sock,
>  	},
>  };

Re: [PATCH 2/2] Add a cleanup routine for objhash socket objects

[Dan Smith]

MH> Does it make sense to add a generic (cleanup) operation when only
MH> one object type will make use of it?

In general, I agree with you, but I don't think this is an obscure
case.

MH> If we add generic mechanisms for things without having multiple
MH> uses then are we just obfuscating the special cases of the code?

MH> As an alternative, would it work if we kept a list of unattached
MH> sockets in the ckpt context, removed them whenever they become
MH> attached, and then use the generic "end of restart" deferqueue to
MH> cleanup unattached sockets?

It would be more code to do it that way, it would inflate the context
with another list, and would cause us to iterate these objects more
than we already do.

>> +	if (sk->sk_socket && !sk->sk_socket->file) {

MH> Would it make sense to add a little helper to explain this?

MH> 	if (!is_sk_attached(sk)) {

Does that make it more clear?  What's the socket attached to?  Another
socket?  I could add more words to the helper to make it more obvious
but IMHO, it's clearer to have it spelled out.

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH 2/2] Add a cleanup routine for objhash socket objects

[Oren Laadan]

Dan Smith wrote:
> MH> Does it make sense to add a generic (cleanup) operation when only
> MH> one object type will make use of it?
> 
> In general, I agree with you, but I don't think this is an obscure
> case.
> 
> MH> If we add generic mechanisms for things without having multiple
> MH> uses then are we just obfuscating the special cases of the code?
> 
> MH> As an alternative, would it work if we kept a list of unattached
> MH> sockets in the ckpt context, removed them whenever they become
> MH> attached, and then use the generic "end of restart" deferqueue to
> MH> cleanup unattached sockets?
> 
> It would be more code to do it that way, it would inflate the context
> with another list, and would cause us to iterate these objects more
> than we already do.

I agree with Dan.

So the problem happens because the ref_drop() method cannot tell
whether it's called from restore_obj() to drop an extra reference, or
from obj_hash_clear(), to drop the last reference (from the objhash).

[Actually, the way it is now there is still a leak: if the call to
obj_new() from restore_obj() fails, then the subsequent ref_drop()
*is* intended to drop the last reference, but it will not].

I think that the root of this is that ref_drop() doesn't have enough
information about what's going on. Dan's patch suggested to solve it
by adding a specialized ref_drop() - a cleanup method.

However, now I think that it's probably better to modify the prototype
of ref_drop() to be, e.g.:   void ref_drop(void *ptr, int clean), so
it can be smarter.

>>> +	if (sk->sk_socket && !sk->sk_socket->file) {
> 
> MH> Would it make sense to add a little helper to explain this?
> 
> MH> 	if (!is_sk_attached(sk)) {
> 
> Does that make it more clear?  What's the socket attached to?  Another
> socket?  I could add more words to the helper to make it more obvious
> but IMHO, it's clearer to have it spelled out.
> 

Perhaps a comment to spell it out :)

Oren.