c/r: Support open INET connections

c/r: Support open INET connections

[Dan Smith]

This set adds support for open INET connection migration.  To be tested
with the recently-posted demonstration script.

[PATCH 1/2] Add socket list helper functions

[Dan Smith]

The INET patch needs to maintain a list of listening sockets in order to
support post-restart socket hashing.  The ckpt_ctx_free() function thus
needs to clean up that list after processing the deferqueue, and needs to
know something about what is on that list.  Instead of calling into a
single cleanup function in the checkpoint code, I decided to just make
the list itself a little more abstract and separate it here.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 include/linux/checkpoint.h |    5 +++++
 net/checkpoint.c           |   42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index dd75bc2..89e41c2 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -100,6 +100,11 @@ extern int ckpt_sock_getnames(struct ckpt_ctx *ctx,
 			      struct socket *socket,
 			      struct sockaddr *loc, unsigned *loc_len,
 			      struct sockaddr *rem, unsigned *rem_len);
+int sock_list_add(struct list_head *list, struct sock *sk);
+typedef int (*sock_compare_fn)(struct sock *, struct sock *);
+struct sock *sock_list_find(struct list_head *list, struct sock *sk,
+			    sock_compare_fn cmp);
+void sock_list_free(struct list_head *list);
 
 /* ckpt kflags */
 #define ckpt_set_ctx_kflag(__ctx, __kflag)  \
diff --git a/net/checkpoint.c b/net/checkpoint.c
index 9a72aae..e7e8e75 100644
--- a/net/checkpoint.c
+++ b/net/checkpoint.c
@@ -758,3 +758,45 @@ struct file *sock_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
 
 	return file;
 }
+
+struct ckpt_sock_list_item {
+       struct sock *sk;
+       struct list_head list;
+};
+
+int sock_list_add(struct list_head *list, struct sock *sk)
+{
+	struct ckpt_sock_list_item *item;
+
+	item = kmalloc(sizeof(*item), GFP_KERNEL);
+	if (!item)
+		return -ENOMEM;
+
+	item->sk = sk;
+	list_add(&item->list, list);
+
+	return 0;
+}
+
+void sock_list_free(struct list_head *list)
+{
+	struct ckpt_sock_list_item *item, *tmp;
+
+	list_for_each_entry_safe(item, tmp, list, list) {
+		list_del(&item->list);
+		kfree(item);
+	}
+}
+
+struct sock *sock_list_find(struct list_head *list, struct sock *sk,
+			    sock_compare_fn cmp)
+{
+	struct ckpt_sock_list_item *item;
+
+	list_for_each_entry(item, list, list) {
+		if (cmp(sk, item->sk))
+			return item->sk;
+	}
+
+	return NULL;
+}
-- 
1.6.2.5

[PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Dan Smith]

This patch adds basic support for C/R of open INET sockets.  I think that
all the important bits of the TCP and ICSK socket structures is saved,
but I think there is still some additional IPv6 stuff that needs to be
handled.

With this patch applied, the following script can be used to demonstrate
the functionality:

  https://lists.linux-foundation.org/pipermail/containers/2009-October/021239.html

It shows that this enables migration of a sendmail process with open
connections from one machine to another without dropping.

Now that listening socket support is in the c/r tree, I think it is
a good time to start fielding comments and suggestions on the
connected part, as I think lots of folks have input on how to make it
better, safer, etc.

Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>
Cc: John Dykstra <jdykstra72-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/sys.c                 |    4 +
 include/linux/checkpoint_hdr.h   |   97 +++++++++++++++++++
 include/linux/checkpoint_types.h |    2 +
 net/checkpoint.c                 |   25 ++----
 net/ipv4/checkpoint.c            |  192 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 303 insertions(+), 17 deletions(-)

diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index 260a1ee..4ec4dd9 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -221,6 +221,8 @@ static void ckpt_ctx_free(struct ckpt_ctx *ctx)
 
 	kfree(ctx->pids_arr);
 
+	sock_list_free(&ctx->listen_sockets);
+
 	kfree(ctx);
 }
 
@@ -249,6 +251,8 @@ static struct ckpt_ctx *ckpt_ctx_alloc(int fd, unsigned long uflags,
 	spin_lock_init(&ctx->lock);
 #endif
 
+	INIT_LIST_HEAD(&ctx->listen_sockets);
+
 	err = -EBADF;
 	ctx->file = fget(fd);
 	if (!ctx->file)
diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
index b5f958e..2693a5d 100644
--- a/include/linux/checkpoint_hdr.h
+++ b/include/linux/checkpoint_hdr.h
@@ -16,6 +16,7 @@
 #include <linux/socket.h>
 #include <linux/un.h>
 #include <linux/in.h>
+#include <linux/in6.h>
 #else
 #include <sys/socket.h>
 #include <sys/un.h>
@@ -475,6 +476,102 @@ struct ckpt_hdr_socket_unix {
 
 struct ckpt_hdr_socket_inet {
 	struct ckpt_hdr h;
+	__u32 daddr;
+	__u32 rcv_saddr;
+	__u32 saddr;
+	__u16 dport;
+	__u16 num;
+	__u16 sport;
+	__s16 uc_ttl;
+	__u16 cmsg_flags;
+
+	struct {
+		__u64 timeout;
+		__u32 ato;
+		__u32 lrcvtime;
+		__u16 last_seg_size;
+		__u16 rcv_mss;
+		__u8 pending;
+		__u8 quick;
+		__u8 pingpong;
+		__u8 blocked;
+	} icsk_ack __attribute__ ((aligned(8)));
+
+	/* FIXME: Skipped opt, tos, multicast, cork settings */
+
+	struct {
+		__u64 last_synq_overflow;
+
+		__u32 rcv_nxt;
+		__u32 copied_seq;
+		__u32 rcv_wup;
+		__u32 snd_nxt;
+		__u32 snd_una;
+		__u32 snd_sml;
+		__u32 rcv_tstamp;
+		__u32 lsndtime;
+
+		__u32 snd_wl1;
+		__u32 snd_wnd;
+		__u32 max_window;
+		__u32 mss_cache;
+		__u32 window_clamp;
+		__u32 rcv_ssthresh;
+		__u32 frto_highmark;
+
+		__u32 srtt;
+		__u32 mdev;
+		__u32 mdev_max;
+		__u32 rttvar;
+		__u32 rtt_seq;
+
+		__u32 packets_out;
+		__u32 retrans_out;
+
+		__u32 snd_up;
+		__u32 rcv_wnd;
+		__u32 write_seq;
+		__u32 pushed_seq;
+		__u32 lost_out;
+		__u32 sacked_out;
+		__u32 fackets_out;
+		__u32 tso_deferred;
+		__u32 bytes_acked;
+
+		__s32 lost_cnt_hint;
+		__u32 retransmit_high;
+
+		__u32 lost_retrans_low;
+
+		__u32 prior_ssthresh;
+		__u32 high_seq;
+
+		__u32 retrans_stamp;
+		__u32 undo_marker;
+		__s32 undo_retrans;
+		__u32 total_retrans;
+
+		__u32 urg_seq;
+		__u32 keepalive_time;
+		__u32 keepalive_intvl;
+
+		__u16 urg_data;
+		__u16 advmss;
+		__u8 frto_counter;
+		__u8 nonagle;
+
+		__u8 ecn_flags;
+		__u8 reordering;
+
+		__u8 keepalive_probes;
+	} tcp __attribute__ ((aligned(8)));
+
+	struct {
+		struct in6_addr saddr;
+		struct in6_addr rcv_saddr;
+		struct in6_addr daddr;
+	} inet6 __attribute__ ((aligned(8)));
+
 	__u32 laddr_len;
 	__u32 raddr_len;
 	struct sockaddr_in laddr;
diff --git a/include/linux/checkpoint_types.h b/include/linux/checkpoint_types.h
index fa57cdc..91c141b 100644
--- a/include/linux/checkpoint_types.h
+++ b/include/linux/checkpoint_types.h
@@ -65,6 +65,8 @@ struct ckpt_ctx {
 	struct list_head pgarr_list;	/* page array to dump VMA contents */
 	struct list_head pgarr_pool;	/* pool of empty page arrays chain */
 
+	struct list_head listen_sockets;/* listening parent sockets */
+
 	/* [multi-process checkpoint] */
 	struct task_struct **tasks_arr; /* array of all tasks [checkpoint] */
 	int nr_tasks;                   /* size of tasks array */
diff --git a/net/checkpoint.c b/net/checkpoint.c
index e7e8e75..3d6da68 100644
--- a/net/checkpoint.c
+++ b/net/checkpoint.c
@@ -90,6 +90,7 @@ static int sock_copy_buffers(struct sk_buff_head *from,
 
 static int __sock_write_buffers(struct ckpt_ctx *ctx,
 				struct sk_buff_head *queue,
+				uint16_t family,
 				int dst_objref)
 {
 	struct sk_buff *skb;
@@ -98,11 +99,7 @@ static int __sock_write_buffers(struct ckpt_ctx *ctx,
 		struct ckpt_hdr_socket_buffer *h;
 		int ret = 0;
 
-		/* FIXME: This could be a false positive for non-unix
-		 *        buffers, so add a type check here in the
-		 *        future
-		 */
-		if (UNIXCB(skb).fp) {
+		if ((family == AF_UNIX) && UNIXCB(skb).fp) {
 			ckpt_write_err(ctx, "TE", "af_unix: pass fd", -EBUSY);
 			return -EBUSY;
 		}
@@ -141,6 +138,7 @@ static int __sock_write_buffers(struct ckpt_ctx *ctx,
 
 static int sock_write_buffers(struct ckpt_ctx *ctx,
 			      struct sk_buff_head *queue,
+			      uint16_t family,
 			      int dst_objref)
 {
 	struct ckpt_hdr_socket_queue *h;
@@ -160,7 +158,7 @@ static int sock_write_buffers(struct ckpt_ctx *ctx,
 	h->skb_count = ret;
 	ret = ckpt_write_obj(ctx, (struct ckpt_hdr *) h);
 	if (!ret)
-		ret = __sock_write_buffers(ctx, &tmpq, dst_objref);
+		ret = __sock_write_buffers(ctx, &tmpq, family, dst_objref);
 
  out:
 	ckpt_hdr_put(ctx, h);
@@ -182,12 +180,14 @@ int sock_deferred_write_buffers(void *data)
 		return dst_objref;
 	}
 
-	ret = sock_write_buffers(ctx, &dq->sk->sk_receive_queue, dst_objref);
+	ret = sock_write_buffers(ctx, &dq->sk->sk_receive_queue,
+				 dq->sk->sk_family, dst_objref);
 	ckpt_debug("write recv buffers: %i\n", ret);
 	if (ret < 0)
 		return ret;
 
-	ret = sock_write_buffers(ctx, &dq->sk->sk_write_queue, dst_objref);
+	ret = sock_write_buffers(ctx, &dq->sk->sk_write_queue,
+				 dq->sk->sk_family, dst_objref);
 	ckpt_debug("write send buffers: %i\n", ret);
 
 	return ret;
@@ -710,15 +710,6 @@ struct sock *do_sock_restore(struct ckpt_ctx *ctx)
 	if (ret < 0)
 		goto err;
 
-	if ((h->sock_common.family == AF_INET) &&
-	    (h->sock.state != TCP_LISTEN)) {
-		/* Temporary hack to enable restore of TCP_LISTEN sockets
-		 * while forcing anything else to a closed state
-		 */
-		sock->sk->sk_state = TCP_CLOSE;
-		sock->state = SS_UNCONNECTED;
-	}
-
 	ckpt_hdr_put(ctx, h);
 	return sock->sk;
  err:
diff --git a/net/ipv4/checkpoint.c b/net/ipv4/checkpoint.c
index 9cbbf5e..0edfa3e 100644
--- a/net/ipv4/checkpoint.c
+++ b/net/ipv4/checkpoint.c
@@ -17,6 +17,7 @@
 #include <linux/deferqueue.h>
 #include <net/tcp_states.h>
 #include <net/tcp.h>
+#include <net/ipv6.h>
 
 struct dq_sock {
 	struct ckpt_ctx *ctx;
@@ -28,6 +29,176 @@ struct dq_buffers {
 	struct sock *sk;
 };
 
+static int sock_is_parent(struct sock *sk, struct sock *parent)
+{
+	return inet_sk(sk)->sport == inet_sk(parent)->sport;
+}
+
+static struct sock *sock_get_parent(struct ckpt_ctx *ctx, struct sock *sk)
+{
+	return sock_list_find(&ctx->listen_sockets, sk, sock_is_parent);
+}
+
+static int sock_hash_parent(void *data)
+{
+	struct dq_sock *dq = (struct dq_sock *)data;
+	struct sock *parent;
+
+	printk("Doing post-restart hash\n");
+
+	dq->sk->sk_prot->hash(dq->sk);
+
+	parent = sock_get_parent(dq->ctx, dq->sk);
+	if (parent) {
+		inet_sk(dq->sk)->num = ntohs(inet_sk(dq->sk)->sport);
+		local_bh_disable();
+		__inet_inherit_port(parent, dq->sk);
+		local_bh_enable();
+	} else {
+		inet_sk(dq->sk)->num = 0;
+		inet_hash_connect(&tcp_death_row, dq->sk);
+		inet_sk(dq->sk)->num = ntohs(inet_sk(dq->sk)->sport);
+	}
+
+	return 0;
+}
+
+static int sock_defer_hash(struct ckpt_ctx *ctx, struct sock *sock)
+{
+	struct dq_sock dq;
+
+	dq.sk = sock;
+	dq.ctx = ctx;
+
+	return deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
+			      sock_hash_parent, NULL);
+}
+
+static int sock_inet_tcp_cptrst(struct ckpt_ctx *ctx,
+				struct tcp_sock *sk,
+				struct ckpt_hdr_socket_inet *hh,
+				int op)
+{
+	CKPT_COPY(op, hh->tcp.rcv_nxt, sk->rcv_nxt);
+	CKPT_COPY(op, hh->tcp.copied_seq, sk->copied_seq);
+	CKPT_COPY(op, hh->tcp.rcv_wup, sk->rcv_wup);
+	CKPT_COPY(op, hh->tcp.snd_nxt, sk->snd_nxt);
+	CKPT_COPY(op, hh->tcp.snd_una, sk->snd_una);
+	CKPT_COPY(op, hh->tcp.snd_sml, sk->snd_sml);
+	CKPT_COPY(op, hh->tcp.rcv_tstamp, sk->rcv_tstamp);
+	CKPT_COPY(op, hh->tcp.lsndtime, sk->lsndtime);
+
+	CKPT_COPY(op, hh->tcp.snd_wl1, sk->snd_wl1);
+	CKPT_COPY(op, hh->tcp.snd_wnd, sk->snd_wnd);
+	CKPT_COPY(op, hh->tcp.max_window, sk->max_window);
+	CKPT_COPY(op, hh->tcp.mss_cache, sk->mss_cache);
+	CKPT_COPY(op, hh->tcp.window_clamp, sk->window_clamp);
+	CKPT_COPY(op, hh->tcp.rcv_ssthresh, sk->rcv_ssthresh);
+	CKPT_COPY(op, hh->tcp.frto_highmark, sk->frto_highmark);
+	CKPT_COPY(op, hh->tcp.advmss, sk->advmss);
+	CKPT_COPY(op, hh->tcp.frto_counter, sk->frto_counter);
+	CKPT_COPY(op, hh->tcp.nonagle, sk->nonagle);
+
+	CKPT_COPY(op, hh->tcp.srtt, sk->srtt);
+	CKPT_COPY(op, hh->tcp.mdev, sk->mdev);
+	CKPT_COPY(op, hh->tcp.mdev_max, sk->mdev_max);
+	CKPT_COPY(op, hh->tcp.rttvar, sk->rttvar);
+	CKPT_COPY(op, hh->tcp.rtt_seq, sk->rtt_seq);
+
+	CKPT_COPY(op, hh->tcp.packets_out, sk->packets_out);
+	CKPT_COPY(op, hh->tcp.retrans_out, sk->retrans_out);
+
+	CKPT_COPY(op, hh->tcp.urg_data, sk->urg_data);
+	CKPT_COPY(op, hh->tcp.ecn_flags, sk->ecn_flags);
+	CKPT_COPY(op, hh->tcp.reordering, sk->reordering);
+	CKPT_COPY(op, hh->tcp.snd_up, sk->snd_up);
+
+	CKPT_COPY(op, hh->tcp.keepalive_probes, sk->keepalive_probes);
+
+	CKPT_COPY(op, hh->tcp.rcv_wnd, sk->rcv_wnd);
+	CKPT_COPY(op, hh->tcp.write_seq, sk->write_seq);
+	CKPT_COPY(op, hh->tcp.pushed_seq, sk->pushed_seq);
+	CKPT_COPY(op, hh->tcp.lost_out, sk->lost_out);
+	CKPT_COPY(op, hh->tcp.sacked_out, sk->sacked_out);
+	CKPT_COPY(op, hh->tcp.fackets_out, sk->fackets_out);
+	CKPT_COPY(op, hh->tcp.tso_deferred, sk->tso_deferred);
+	CKPT_COPY(op, hh->tcp.bytes_acked, sk->bytes_acked);
+
+	CKPT_COPY(op, hh->tcp.lost_cnt_hint, sk->lost_cnt_hint);
+	CKPT_COPY(op, hh->tcp.retransmit_high, sk->retransmit_high);
+
+	CKPT_COPY(op, hh->tcp.lost_retrans_low, sk->lost_retrans_low);
+
+	CKPT_COPY(op, hh->tcp.prior_ssthresh, sk->prior_ssthresh);
+	CKPT_COPY(op, hh->tcp.high_seq, sk->high_seq);
+
+	CKPT_COPY(op, hh->tcp.retrans_stamp, sk->retrans_stamp);
+	CKPT_COPY(op, hh->tcp.undo_marker, sk->undo_marker);
+	CKPT_COPY(op, hh->tcp.undo_retrans, sk->undo_retrans);
+	CKPT_COPY(op, hh->tcp.total_retrans, sk->total_retrans);
+
+	CKPT_COPY(op, hh->tcp.urg_seq, sk->urg_seq);
+	CKPT_COPY(op, hh->tcp.keepalive_time, sk->keepalive_time);
+	CKPT_COPY(op, hh->tcp.keepalive_intvl, sk->keepalive_intvl);
+
+	return 0;
+}
+
+static int sock_inet_cptrst(struct ckpt_ctx *ctx,
+			    struct sock *sock,
+			    struct ckpt_hdr_socket_inet *hh,
+			    int op)
+{
+	struct inet_sock *sk = inet_sk(sock);
+	struct inet_connection_sock *icsk = inet_csk(sock);
+	int ret;
+
+	CKPT_COPY(op, hh->daddr, sk->daddr);
+	CKPT_COPY(op, hh->rcv_saddr, sk->rcv_saddr);
+	CKPT_COPY(op, hh->dport, sk->dport);
+	CKPT_COPY(op, hh->num, sk->num);
+	CKPT_COPY(op, hh->saddr, sk->saddr);
+	CKPT_COPY(op, hh->sport, sk->sport);
+	CKPT_COPY(op, hh->uc_ttl, sk->uc_ttl);
+	CKPT_COPY(op, hh->cmsg_flags, sk->cmsg_flags);
+
+	CKPT_COPY(op, hh->icsk_ack.pending, icsk->icsk_ack.pending);
+	CKPT_COPY(op, hh->icsk_ack.quick, icsk->icsk_ack.quick);
+	CKPT_COPY(op, hh->icsk_ack.pingpong, icsk->icsk_ack.pingpong);
+	CKPT_COPY(op, hh->icsk_ack.blocked, icsk->icsk_ack.blocked);
+	CKPT_COPY(op, hh->icsk_ack.ato, icsk->icsk_ack.ato);
+	CKPT_COPY(op, hh->icsk_ack.timeout, icsk->icsk_ack.timeout);
+	CKPT_COPY(op, hh->icsk_ack.lrcvtime, icsk->icsk_ack.lrcvtime);
+	CKPT_COPY(op,
+		  hh->icsk_ack.last_seg_size, icsk->icsk_ack.last_seg_size);
+	CKPT_COPY(op, hh->icsk_ack.rcv_mss, icsk->icsk_ack.rcv_mss);
+
+	if (sock->sk_protocol == IPPROTO_TCP)
+		ret = sock_inet_tcp_cptrst(ctx, tcp_sk(sock), hh, op);
+	else if (sock->sk_protocol == IPPROTO_UDP)
+		ret = 0;
+	else {
+		ckpt_write_err(ctx, "T", "unknown socket protocol %d",
+			       sock->sk_protocol);
+		ret = -EINVAL;
+	}
+
+	if (sock->sk_family == AF_INET6) {
+		struct ipv6_pinfo *inet6 = inet6_sk(sock);
+		if (op == CKPT_CPT) {
+			ipv6_addr_copy(&hh->inet6.saddr, &inet6->saddr);
+			ipv6_addr_copy(&hh->inet6.rcv_saddr, &inet6->rcv_saddr);
+			ipv6_addr_copy(&hh->inet6.daddr, &inet6->daddr);
+		} else {
+			ipv6_addr_copy(&inet6->saddr, &hh->inet6.saddr);
+			ipv6_addr_copy(&inet6->rcv_saddr, &hh->inet6.rcv_saddr);
+			ipv6_addr_copy(&inet6->daddr, &hh->inet6.daddr);
+		}
+	}
+
+	return ret;
+}
+
 int inet_checkpoint(struct ckpt_ctx *ctx, struct socket *sock)
 {
 	struct ckpt_hdr_socket_inet *in;
@@ -43,6 +214,10 @@ int inet_checkpoint(struct ckpt_ctx *ctx, struct socket *sock)
 	if (ret)
 		goto out;
 
+	ret = sock_inet_cptrst(ctx, sock->sk, in, CKPT_CPT);
+	if (ret < 0)
+		goto out;
+
 	ret = ckpt_write_obj(ctx, (struct ckpt_hdr *) in);
  out:
 	ckpt_hdr_put(ctx, in);
@@ -209,8 +384,25 @@ int inet_restore(struct ckpt_ctx *ctx,
 			ckpt_debug("inet listen: %i\n", ret);
 			if (ret < 0)
 				goto out;
+
+			ret = sock_list_add(&ctx->listen_sockets, sock->sk);
+			if (ret < 0)
+				goto out;
 		}
 	} else {
+		ret = sock_inet_cptrst(ctx, sock->sk, in, CKPT_RST);
+		printk("sock_inet_cptrst: %i\n", ret);
+		if (ret)
+			goto out;
+
+		if ((h->sock.state == TCP_ESTABLISHED) &&
+		    (h->sock.protocol == IPPROTO_TCP)) {
+			/* Delay hashing this sock until the end so we can
+			 * hook it up with its parent (if appropriate)
+			 */
+			ret = sock_defer_hash(ctx, sock->sk);
+		}
+
 		if (!sock_flag(sock->sk, SOCK_DEAD))
 			ret = inet_defer_restore_buffers(ctx, sock->sk);
 	}
-- 
1.6.2.5

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> This patch adds basic support for C/R of open INET sockets.  I think that
> all the important bits of the TCP and ICSK socket structures is saved,
> but I think there is still some additional IPv6 stuff that needs to be
> handled.
> 
> With this patch applied, the following script can be used to demonstrate
> the functionality:
> 
>   https://lists.linux-foundation.org/pipermail/containers/2009-October/021239.html
> 
> It shows that this enables migration of a sendmail process with open
> connections from one machine to another without dropping.

neato

> Now that listening socket support is in the c/r tree, I think it is
> a good time to start fielding comments and suggestions on the
> connected part, as I think lots of folks have input on how to make it
> better, safer, etc.

One thing:

> +static int sock_inet_cptrst(struct ckpt_ctx *ctx,
> +			    struct sock *sock,
> +			    struct ckpt_hdr_socket_inet *hh,
> +			    int op)
> +{
> +	struct inet_sock *sk = inet_sk(sock);
> +	struct inet_connection_sock *icsk = inet_csk(sock);
> +	int ret;
> +
> +	CKPT_COPY(op, hh->daddr, sk->daddr);
> +	CKPT_COPY(op, hh->rcv_saddr, sk->rcv_saddr);
> +	CKPT_COPY(op, hh->dport, sk->dport);
> +	CKPT_COPY(op, hh->num, sk->num);
> +	CKPT_COPY(op, hh->saddr, sk->saddr);
> +	CKPT_COPY(op, hh->sport, sk->sport);

This becomes an easy way around CAP_NET_BIND_SERVICE right?  Or
will that be caught by something already done in your listen
patch after this step?

thanks,
-serge

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Dan Smith]

>> +	CKPT_COPY(op, hh->daddr, sk->daddr);
>> +	CKPT_COPY(op, hh->rcv_saddr, sk->rcv_saddr);
>> +	CKPT_COPY(op, hh->dport, sk->dport);
>> +	CKPT_COPY(op, hh->num, sk->num);
>> +	CKPT_COPY(op, hh->saddr, sk->saddr);
>> +	CKPT_COPY(op, hh->sport, sk->sport);

SH> This becomes an easy way around CAP_NET_BIND_SERVICE right?  Or
SH> will that be caught by something already done in your listen patch
SH> after this step?

Actually, yeah, you're right.  I was going to say that we'd catch it
because we also do a bind(), but there's no guarantee that the
sockaddr_in we use for bind() is the same as this :D

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

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[John Dykstra]

On Wed, 2009-10-07 at 09:29 -0700, Dan Smith wrote:
> This patch adds basic support for C/R of open INET sockets.  I think
> that
> all the important bits of the TCP and ICSK socket structures is saved,
> but I think there is still some additional IPv6 stuff that needs to be
> handled.

I think this patch breaks code that was already in do_sock_restore():

        struct sock *do_sock_restore(struct ckpt_ctx *ctx)
        {
        	struct ckpt_hdr_socket *h;
        	struct socket *sock;
        	int ret;
        
        	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_SOCKET);
        	if (IS_ERR(h))
        		return ERR_PTR(PTR_ERR(h));
        
        	/* silently clear flags, e.g. SOCK_NONBLOCK or SOCK_CLOEXEC */
        	h->sock.type &= SOCK_TYPE_MASK;
        
        	ret = sock_create(h->sock_common.family, h->sock.type, 0, &sock);
        	if (ret < 0)
        		goto err;
        

You're passing 0 as the protocol value to sock_create().  This
ultimately gets passed to the address family's create() function.  

inet_create() (and its IPv6 companion) use that protocol value as the
key when they search for the proper inet_protosw, which in turn gets
mapped to the struct proto and passed to sk_prot_alloc().

In address families INET and AF_INET6, the struct sock is different
sizes for different protocols.  This is implemented by the struct proto
specifying which cache the struct sock comes from.

So by passing in 0 all the time to sock_create(), you're getting a
struct sock that may not be the right size.  Memory corruption and
madness follow.

  --  John

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Dan Smith]

JD> You're passing 0 as the protocol value to sock_create().  This
JD> ultimately gets passed to the address family's create() function.

The fix is to pass in the previous version of sk_protocol instead of
zero, right?  It looks like inet_create() has enough intelligence to
weed out invalid values for us too...

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

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[John Dykstra]

On Thu, 2009-10-08 at 08:41 -0700, Dan Smith wrote:
> JD> You're passing 0 as the protocol value to sock_create().  This
> JD> ultimately gets passed to the address family's create() function.
> 
> The fix is to pass in the previous version of sk_protocol instead of
> zero, right?

Yep.  It sort of messes up your separation between layers, though.

  --  John

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Dan Smith]

JD> Yep.  It sort of messes up your separation between layers, though.

In what way?  It's just this:

diff --git a/net/checkpoint.c b/net/checkpoint.c
index 3d6da68..f74ebe4 100644
--- a/net/checkpoint.c
+++ b/net/checkpoint.c
@@ -687,7 +687,8 @@ struct sock *do_sock_restore(struct ckpt_ctx *ctx)
        /* silently clear flags, e.g. SOCK_NONBLOCK or SOCK_CLOEXEC */
        h->sock.type &= SOCK_TYPE_MASK;
 
-       ret = sock_create(h->sock_common.family, h->sock.type, 0, &sock);
+       ret = sock_create(h->sock_common.family, h->sock.type,
+                         h->sock.protocol, &sock);
        if (ret < 0)
                goto err;

Which seems like a why-didn't-I-do-that-in-the-first-place sort of
thing...   Am I missing something?

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

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[John Dykstra]

On Thu, 2009-10-08 at 10:34 -0700, Dan Smith wrote:
> JD> Yep.  It sort of messes up your separation between layers, though.
> 
> In what way?  It's just this:

I just meant that you're peeking into the checkpointed sk data before
you create the new sk.  In so much of your code, you manage to restore
structs as a whole, without peeking first.

> Which seems like a why-didn't-I-do-that-in-the-first-place sort of
> thing...   Am I missing something?

Not anything that I see.  

  --  John

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Dan Smith]

JD> I just meant that you're peeking into the checkpointed sk data
JD> before you create the new sk.  In so much of your code, you manage
JD> to restore structs as a whole, without peeking first.

Ah, okay, gotcha.

Thanks!

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

Re: [PATCH 1/2] Add socket list helper functions

[Oren Laadan]

Dan Smith wrote:
> The INET patch needs to maintain a list of listening sockets in order to
> support post-restart socket hashing.  The ckpt_ctx_free() function thus
> needs to clean up that list after processing the deferqueue, and needs to
> know something about what is on that list.  Instead of calling into a
> single cleanup function in the checkpoint code, I decided to just make
> the list itself a little more abstract and separate it here.

IMHO this is over-engineered. This way, you place net-related knowledge
in ckpt_ctx_free(), instead of isolating that sort of smarts under net/
subdir. I'd rather see the clean-up logic near the "litter" logic, so
to speak.

Or, make it entirely generic, use container_of etc and put in, for
instance, in include/linux/list2.h. Anyway, I don't think it belongs
in checkpoint.h.

Thanks,

Oren.

> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/checkpoint.h |    5 +++++
>  net/checkpoint.c           |   42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
> index dd75bc2..89e41c2 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -100,6 +100,11 @@ extern int ckpt_sock_getnames(struct ckpt_ctx *ctx,
>  			      struct socket *socket,
>  			      struct sockaddr *loc, unsigned *loc_len,
>  			      struct sockaddr *rem, unsigned *rem_len);
> +int sock_list_add(struct list_head *list, struct sock *sk);
> +typedef int (*sock_compare_fn)(struct sock *, struct sock *);
> +struct sock *sock_list_find(struct list_head *list, struct sock *sk,
> +			    sock_compare_fn cmp);
> +void sock_list_free(struct list_head *list);
>  
>  /* ckpt kflags */
>  #define ckpt_set_ctx_kflag(__ctx, __kflag)  \
> diff --git a/net/checkpoint.c b/net/checkpoint.c
> index 9a72aae..e7e8e75 100644
> --- a/net/checkpoint.c
> +++ b/net/checkpoint.c
> @@ -758,3 +758,45 @@ struct file *sock_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
>  
>  	return file;
>  }
> +
> +struct ckpt_sock_list_item {
> +       struct sock *sk;
> +       struct list_head list;
> +};
> +
> +int sock_list_add(struct list_head *list, struct sock *sk)
> +{
> +	struct ckpt_sock_list_item *item;
> +
> +	item = kmalloc(sizeof(*item), GFP_KERNEL);
> +	if (!item)
> +		return -ENOMEM;
> +
> +	item->sk = sk;
> +	list_add(&item->list, list);
> +
> +	return 0;
> +}
> +
> +void sock_list_free(struct list_head *list)
> +{
> +	struct ckpt_sock_list_item *item, *tmp;
> +
> +	list_for_each_entry_safe(item, tmp, list, list) {
> +		list_del(&item->list);
> +		kfree(item);
> +	}
> +}
> +
> +struct sock *sock_list_find(struct list_head *list, struct sock *sk,
> +			    sock_compare_fn cmp)
> +{
> +	struct ckpt_sock_list_item *item;
> +
> +	list_for_each_entry(item, list, list) {
> +		if (cmp(sk, item->sk))
> +			return item->sk;
> +	}
> +
> +	return NULL;
> +}

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Oren Laadan]

Dan Smith wrote:
> This patch adds basic support for C/R of open INET sockets.  I think that
> all the important bits of the TCP and ICSK socket structures is saved,
> but I think there is still some additional IPv6 stuff that needs to be
> handled.
> 
> With this patch applied, the following script can be used to demonstrate
> the functionality:
> 
>   https://lists.linux-foundation.org/pipermail/containers/2009-October/021239.html
> 
> It shows that this enables migration of a sendmail process with open
> connections from one machine to another without dropping.
> 
> Now that listening socket support is in the c/r tree, I think it is
> a good time to start fielding comments and suggestions on the
> connected part, as I think lots of folks have input on how to make it
> better, safer, etc.

Nice !

> 
> Cc: netdev@vger.kernel.org
> Cc: Oren Laadan <orenl@librato.com>
> Cc: John Dykstra <jdykstra72@gmail.com>
> Signed-off-by: Dan Smith <danms@us.ibm.com>
> ---

A few comments/questions before the review:

* Did you test this with UDP too ?

* What happens if the the clock on the target machine differs from the
clock on the origin machine ?   (TCP timestamps)

* How confident are we that "bad" input in one or more fields, that
you don't currently sanitize, cannot create "bad" behavior ?  (bad can
be kernel crash, unauthorized behavior, DoS etc)

* How much does TCP rely on the validity of the info in the protocol
control block, and what sorts of bads can happen if it isn't ?  Would
TCP be still happy if the URG point is bogus, would it allow the user
to sent packets otherwise disallowed (to that user?), or maybe it
could crash the kernel ?

* In other words, how much input-sanity-logic is missing (if any) ?

* Can you please document (brief description) how the restart logic
works (listening parent socket etc) ?

* Do you intend to checkpoint (and collect) lingering sockets, that
is they are closed by the application so not references by any task,
but still sending data from their buffers ?

* I'd like to also preserve the "older" behavior - so the user can
choose to restart and reset all previous connections, keep listening
sockets (e.g. RESTART_DISCONNET).

Also, a couple nits inside below.

>  checkpoint/sys.c                 |    4 +
>  include/linux/checkpoint_hdr.h   |   97 +++++++++++++++++++
>  include/linux/checkpoint_types.h |    2 +
>  net/checkpoint.c                 |   25 ++----
>  net/ipv4/checkpoint.c            |  192 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 303 insertions(+), 17 deletions(-)
> 

[...]

> @@ -475,6 +476,102 @@ struct ckpt_hdr_socket_unix {
>  
>  struct ckpt_hdr_socket_inet {
>  	struct ckpt_hdr h;
> +	__u32 daddr;
> +	__u32 rcv_saddr;
> +	__u32 saddr;
> +	__u16 dport;
> +	__u16 num;
> +	__u16 sport;
> +	__s16 uc_ttl;
> +	__u16 cmsg_flags;
> +

Alignment ?

> +	struct {
> +		__u64 timeout;
> +		__u32 ato;
> +		__u32 lrcvtime;
> +		__u16 last_seg_size;
> +		__u16 rcv_mss;
> +		__u8 pending;
> +		__u8 quick;
> +		__u8 pingpong;
> +		__u8 blocked;
> +	} icsk_ack __attribute__ ((aligned(8)));
> +

[...]


> @@ -710,15 +710,6 @@ struct sock *do_sock_restore(struct ckpt_ctx *ctx)
>  	if (ret < 0)
>  		goto err;
>  
> -	if ((h->sock_common.family == AF_INET) &&
> -	    (h->sock.state != TCP_LISTEN)) {
> -		/* Temporary hack to enable restore of TCP_LISTEN sockets
> -		 * while forcing anything else to a closed state
> -		 */
> -		sock->sk->sk_state = TCP_CLOSE;
> -		sock->state = SS_UNCONNECTED;
> -	}
> -
>  	ckpt_hdr_put(ctx, h);
>  	return sock->sk;
>   err:

How about leaving this inside if the user explicitly requests so,
e.g. using a RESTART_DISCONNECT or something ?

> diff --git a/net/ipv4/checkpoint.c b/net/ipv4/checkpoint.c
> index 9cbbf5e..0edfa3e 100644
> --- a/net/ipv4/checkpoint.c
> +++ b/net/ipv4/checkpoint.c
> @@ -17,6 +17,7 @@
>  #include <linux/deferqueue.h>
>  #include <net/tcp_states.h>
>  #include <net/tcp.h>
> +#include <net/ipv6.h>
>  
>  struct dq_sock {
>  	struct ckpt_ctx *ctx;
> @@ -28,6 +29,176 @@ struct dq_buffers {
>  	struct sock *sk;
>  };
>  
> +static int sock_is_parent(struct sock *sk, struct sock *parent)
> +{
> +	return inet_sk(sk)->sport == inet_sk(parent)->sport;
> +}
> +
> +static struct sock *sock_get_parent(struct ckpt_ctx *ctx, struct sock *sk)
> +{
> +	return sock_list_find(&ctx->listen_sockets, sk, sock_is_parent);
> +}
> +
> +static int sock_hash_parent(void *data)
> +{
> +	struct dq_sock *dq = (struct dq_sock *)data;
> +	struct sock *parent;
> +
> +	printk("Doing post-restart hash\n");
> +
> +	dq->sk->sk_prot->hash(dq->sk);
> +
> +	parent = sock_get_parent(dq->ctx, dq->sk);
> +	if (parent) {
> +		inet_sk(dq->sk)->num = ntohs(inet_sk(dq->sk)->sport);
> +		local_bh_disable();
> +		__inet_inherit_port(parent, dq->sk);
> +		local_bh_enable();
> +	} else {
> +		inet_sk(dq->sk)->num = 0;
> +		inet_hash_connect(&tcp_death_row, dq->sk);
> +		inet_sk(dq->sk)->num = ntohs(inet_sk(dq->sk)->sport);
> +	}
> +
> +	return 0;
> +}

I wonder if a user can use this to convince TCP to send some nasty
packets to some arbitrary destination, with specific seq-number or
what not ?

> +
> +static int sock_defer_hash(struct ckpt_ctx *ctx, struct sock *sock)
> +{
> +	struct dq_sock dq;
> +
> +	dq.sk = sock;
> +	dq.ctx = ctx;
> +
> +	return deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
> +			      sock_hash_parent, NULL);
> +}
> +
> +static int sock_inet_tcp_cptrst(struct ckpt_ctx *ctx,
> +				struct tcp_sock *sk,
> +				struct ckpt_hdr_socket_inet *hh,
> +				int op)
> +{

Which of the following list is safe to restore *as is*, and which
requires sanitation tests ?

> +	CKPT_COPY(op, hh->tcp.rcv_nxt, sk->rcv_nxt);
> +	CKPT_COPY(op, hh->tcp.copied_seq, sk->copied_seq);
> +	CKPT_COPY(op, hh->tcp.rcv_wup, sk->rcv_wup);
> +	CKPT_COPY(op, hh->tcp.snd_nxt, sk->snd_nxt);
> +	CKPT_COPY(op, hh->tcp.snd_una, sk->snd_una);
> +	CKPT_COPY(op, hh->tcp.snd_sml, sk->snd_sml);
> +	CKPT_COPY(op, hh->tcp.rcv_tstamp, sk->rcv_tstamp);
> +	CKPT_COPY(op, hh->tcp.lsndtime, sk->lsndtime);
> +
> +	CKPT_COPY(op, hh->tcp.snd_wl1, sk->snd_wl1);
> +	CKPT_COPY(op, hh->tcp.snd_wnd, sk->snd_wnd);
> +	CKPT_COPY(op, hh->tcp.max_window, sk->max_window);
> +	CKPT_COPY(op, hh->tcp.mss_cache, sk->mss_cache);
> +	CKPT_COPY(op, hh->tcp.window_clamp, sk->window_clamp);
> +	CKPT_COPY(op, hh->tcp.rcv_ssthresh, sk->rcv_ssthresh);
> +	CKPT_COPY(op, hh->tcp.frto_highmark, sk->frto_highmark);
> +	CKPT_COPY(op, hh->tcp.advmss, sk->advmss);
> +	CKPT_COPY(op, hh->tcp.frto_counter, sk->frto_counter);
> +	CKPT_COPY(op, hh->tcp.nonagle, sk->nonagle);
> +
> +	CKPT_COPY(op, hh->tcp.srtt, sk->srtt);
> +	CKPT_COPY(op, hh->tcp.mdev, sk->mdev);
> +	CKPT_COPY(op, hh->tcp.mdev_max, sk->mdev_max);
> +	CKPT_COPY(op, hh->tcp.rttvar, sk->rttvar);
> +	CKPT_COPY(op, hh->tcp.rtt_seq, sk->rtt_seq);
> +
> +	CKPT_COPY(op, hh->tcp.packets_out, sk->packets_out);
> +	CKPT_COPY(op, hh->tcp.retrans_out, sk->retrans_out);
> +
> +	CKPT_COPY(op, hh->tcp.urg_data, sk->urg_data);
> +	CKPT_COPY(op, hh->tcp.ecn_flags, sk->ecn_flags);
> +	CKPT_COPY(op, hh->tcp.reordering, sk->reordering);
> +	CKPT_COPY(op, hh->tcp.snd_up, sk->snd_up);
> +
> +	CKPT_COPY(op, hh->tcp.keepalive_probes, sk->keepalive_probes);
> +
> +	CKPT_COPY(op, hh->tcp.rcv_wnd, sk->rcv_wnd);
> +	CKPT_COPY(op, hh->tcp.write_seq, sk->write_seq);
> +	CKPT_COPY(op, hh->tcp.pushed_seq, sk->pushed_seq);
> +	CKPT_COPY(op, hh->tcp.lost_out, sk->lost_out);
> +	CKPT_COPY(op, hh->tcp.sacked_out, sk->sacked_out);
> +	CKPT_COPY(op, hh->tcp.fackets_out, sk->fackets_out);
> +	CKPT_COPY(op, hh->tcp.tso_deferred, sk->tso_deferred);
> +	CKPT_COPY(op, hh->tcp.bytes_acked, sk->bytes_acked);
> +
> +	CKPT_COPY(op, hh->tcp.lost_cnt_hint, sk->lost_cnt_hint);
> +	CKPT_COPY(op, hh->tcp.retransmit_high, sk->retransmit_high);
> +
> +	CKPT_COPY(op, hh->tcp.lost_retrans_low, sk->lost_retrans_low);
> +
> +	CKPT_COPY(op, hh->tcp.prior_ssthresh, sk->prior_ssthresh);
> +	CKPT_COPY(op, hh->tcp.high_seq, sk->high_seq);
> +
> +	CKPT_COPY(op, hh->tcp.retrans_stamp, sk->retrans_stamp);
> +	CKPT_COPY(op, hh->tcp.undo_marker, sk->undo_marker);
> +	CKPT_COPY(op, hh->tcp.undo_retrans, sk->undo_retrans);
> +	CKPT_COPY(op, hh->tcp.total_retrans, sk->total_retrans);
> +
> +	CKPT_COPY(op, hh->tcp.urg_seq, sk->urg_seq);
> +	CKPT_COPY(op, hh->tcp.keepalive_time, sk->keepalive_time);
> +	CKPT_COPY(op, hh->tcp.keepalive_intvl, sk->keepalive_intvl);
> +
> +	return 0;
> +}
> +
> +static int sock_inet_cptrst(struct ckpt_ctx *ctx,
> +			    struct sock *sock,
> +			    struct ckpt_hdr_socket_inet *hh,
> +			    int op)
> +{
> +	struct inet_sock *sk = inet_sk(sock);
> +	struct inet_connection_sock *icsk = inet_csk(sock);
> +	int ret;
> +
> +	CKPT_COPY(op, hh->daddr, sk->daddr);
> +	CKPT_COPY(op, hh->rcv_saddr, sk->rcv_saddr);
> +	CKPT_COPY(op, hh->dport, sk->dport);
> +	CKPT_COPY(op, hh->num, sk->num);
> +	CKPT_COPY(op, hh->saddr, sk->saddr);
> +	CKPT_COPY(op, hh->sport, sk->sport);
> +	CKPT_COPY(op, hh->uc_ttl, sk->uc_ttl);
> +	CKPT_COPY(op, hh->cmsg_flags, sk->cmsg_flags);
> +
> +	CKPT_COPY(op, hh->icsk_ack.pending, icsk->icsk_ack.pending);
> +	CKPT_COPY(op, hh->icsk_ack.quick, icsk->icsk_ack.quick);
> +	CKPT_COPY(op, hh->icsk_ack.pingpong, icsk->icsk_ack.pingpong);
> +	CKPT_COPY(op, hh->icsk_ack.blocked, icsk->icsk_ack.blocked);
> +	CKPT_COPY(op, hh->icsk_ack.ato, icsk->icsk_ack.ato);
> +	CKPT_COPY(op, hh->icsk_ack.timeout, icsk->icsk_ack.timeout);
> +	CKPT_COPY(op, hh->icsk_ack.lrcvtime, icsk->icsk_ack.lrcvtime);
> +	CKPT_COPY(op,
> +		  hh->icsk_ack.last_seg_size, icsk->icsk_ack.last_seg_size);
> +	CKPT_COPY(op, hh->icsk_ack.rcv_mss, icsk->icsk_ack.rcv_mss);
> +
> +	if (sock->sk_protocol == IPPROTO_TCP)
> +		ret = sock_inet_tcp_cptrst(ctx, tcp_sk(sock), hh, op);
> +	else if (sock->sk_protocol == IPPROTO_UDP)
> +		ret = 0;
> +	else {
> +		ckpt_write_err(ctx, "T", "unknown socket protocol %d",
> +			       sock->sk_protocol);
> +		ret = -EINVAL;
> +	}
> +
> +	if (sock->sk_family == AF_INET6) {
> +		struct ipv6_pinfo *inet6 = inet6_sk(sock);
> +		if (op == CKPT_CPT) {
> +			ipv6_addr_copy(&hh->inet6.saddr, &inet6->saddr);
> +			ipv6_addr_copy(&hh->inet6.rcv_saddr, &inet6->rcv_saddr);
> +			ipv6_addr_copy(&hh->inet6.daddr, &inet6->daddr);
> +		} else {
> +			ipv6_addr_copy(&inet6->saddr, &hh->inet6.saddr);
> +			ipv6_addr_copy(&inet6->rcv_saddr, &hh->inet6.rcv_saddr);
> +			ipv6_addr_copy(&inet6->daddr, &hh->inet6.daddr);
> +		}
> +	}
> +
> +	return ret;
> +}
> +

[...]

Oren.

Re: [PATCH 1/2] Add socket list helper functions

[Dan Smith]

OL> IMHO this is over-engineered. This way, you place net-related
OL> knowledge in ckpt_ctx_free(), instead of isolating that sort of
OL> smarts under net/ subdir. I'd rather see the clean-up logic near
OL> the "litter" logic, so to speak.

Well, I still need to clean up the list after the deferqueue has run,
so something has to go into ckpt_ctx_free() to do that.  I can make it
a specific call like ckpt_sock_cleanup_listening() or something, but
that will still need to go in checkpoint.h so I can call it from
ctx_free()...

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

Re: [PATCH 1/2] Add socket list helper functions

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> OL> IMHO this is over-engineered. This way, you place net-related
> OL> knowledge in ckpt_ctx_free(), instead of isolating that sort of
> OL> smarts under net/ subdir. I'd rather see the clean-up logic near
> OL> the "litter" logic, so to speak.
> 
> Well, I still need to clean up the list after the deferqueue has run,
> so something has to go into ckpt_ctx_free() to do that.  I can make it
> a specific call like ckpt_sock_cleanup_listening() or something, but
> that will still need to go in checkpoint.h so I can call it from
> ctx_free()...

Heh, but that sounds like over-engineering  :)

-serge

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Dan Smith]

OL> * Did you test this with UDP too ?

Not sendmail of course, but I have a little test program that
maintains a DGRAM connection to the echo service on a remote node,
yeah.

OL> * What happens if the the clock on the target machine differs from
OL> the clock on the origin machine ?  (TCP timestamps)

I guess maybe we should canonicalize the timeout values to something
like "milliseconds after checkpoint start"?  This would allow the
remote system to reset the timers to something reasonable.  It would
also cause non-migration restarts to restore the timers appropriately
for a coordinated restart of multiple machines.

OL> * How confident are we that "bad" input in one or more fields,
OL> that you don't currently sanitize, cannot create "bad" behavior ?
OL> (bad can be kernel crash, unauthorized behavior, DoS etc)

I'm going to say 0.052.

I haven't evaluated much of it, no :)

OL> * How much does TCP rely on the validity of the info in the
OL> protocol control block, and what sorts of bads can happen if it
OL> isn't ?  Would TCP be still happy if the URG point is bogus, would
OL> it allow the user to sent packets otherwise disallowed (to that
OL> user?), or maybe it could crash the kernel ?

Good question, I'll have to look.

OL> * Can you please document (brief description) how the restart
OL> logic works (listening parent socket etc) ?

Sure.

OL> * Do you intend to checkpoint (and collect) lingering sockets,
OL> that is they are closed by the application so not references by
OL> any task, but still sending data from their buffers ?

Yeah, I expect that will be important :)

OL> * I'd like to also preserve the "older" behavior - so the user can
OL> choose to restart and reset all previous connections, keep
OL> listening sockets (e.g. RESTART_DISCONNET).

Sure, sounds good to me.

>> +	printk("Doing post-restart hash\n");

(oops, looks like I left some debug messages in place)

OL> I wonder if a user can use this to convince TCP to send some nasty
OL> packets to some arbitrary destination, with specific seq-number or
OL> what not ?

I'm not sure what you mean.  The sk->num value comes from the sport
which should have been refused during the bind() if it's in use or not
permitted, no?

Thanks!

-- 
Dan Smith
IBM Linux Technology Center
email: danms@us.ibm.com

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Oren Laadan]

Dan Smith wrote:
> OL> * Did you test this with UDP too ?
> 
> Not sendmail of course, but I have a little test program that
> maintains a DGRAM connection to the echo service on a remote node,
> yeah.
> 
> OL> * What happens if the the clock on the target machine differs from
> OL> the clock on the origin machine ?  (TCP timestamps)
> 
> I guess maybe we should canonicalize the timeout values to something
> like "milliseconds after checkpoint start"?  This would allow the
> remote system to reset the timers to something reasonable.  It would
> also cause non-migration restarts to restore the timers appropriately
> for a coordinated restart of multiple machines.

IIRC, the TCP stack takes the timestamp for each packet directly from
jiffies. So you need to teach TCP to add a per-container (or you can
make it per-socket) delta to that timestamp.

> 
> OL> * How confident are we that "bad" input in one or more fields,
> OL> that you don't currently sanitize, cannot create "bad" behavior ?
> OL> (bad can be kernel crash, unauthorized behavior, DoS etc)
> 
> I'm going to say 0.052.

Ah ... sure ...
To avoid confusion, can you state the units :p

> 
> I haven't evaluated much of it, no :)

I guess my point is that we want to ask the networking people this
question in an explicit way.

> 
> OL> * How much does TCP rely on the validity of the info in the
> OL> protocol control block, and what sorts of bads can happen if it
> OL> isn't ?  Would TCP be still happy if the URG point is bogus, would
> OL> it allow the user to sent packets otherwise disallowed (to that
> OL> user?), or maybe it could crash the kernel ?
> 
> Good question, I'll have to look.

Ditto.

So I'm thinking, for both, do (1) put a big fat comment in the code
saying that sanity-tests are needed, and what for, and (2) send a
separate mail to the networking people with these two scenarios and
request comments ?

> 
> OL> * Can you please document (brief description) how the restart
> OL> logic works (listening parent socket etc) ?
> 
> Sure.
> 
> OL> * Do you intend to checkpoint (and collect) lingering sockets,
> OL> that is they are closed by the application so not references by
> OL> any task, but still sending data from their buffers ?
> 
> Yeah, I expect that will be important :)

Cool. How about a TODO comment somewhere to convince everyone ( = me)
that you have it in your plans :)

> 
> OL> * I'd like to also preserve the "older" behavior - so the user can
> OL> choose to restart and reset all previous connections, keep
> OL> listening sockets (e.g. RESTART_DISCONNET).
> 
> Sure, sounds good to me.
> 
>>> +	printk("Doing post-restart hash\n");
> 
> (oops, looks like I left some debug messages in place)
> 
> OL> I wonder if a user can use this to convince TCP to send some nasty
> OL> packets to some arbitrary destination, with specific seq-number or
> OL> what not ?
> 
> I'm not sure what you mean.  The sk->num value comes from the sport
> which should have been refused during the bind() if it's in use or not
> permitted, no?

I think Serge already pointed in his review that this should not permit
a user to bind inconsistent or restricted ports.

I actually meant the contrary: suppose a malicious user on your machine
wants to attack a target machine/connection. can that user provide such
destination-address data and protocol parameters to build a connection
that locally seems valid, but is malicious ?

For example, now, if a user wants to send a TCP packet with arbitrary
protocol parameters, he needs to use raw IP sockets, which require
privilege. Would restarting a connection with the desired parameters
become a way to bypass that restriction ?  (e.g. assume the user
restarts while using the host's network namespace).

Oren.

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Dan Smith]

OL> IIRC, the TCP stack takes the timestamp for each packet directly
OL> from jiffies. So you need to teach TCP to add a per-container (or
OL> you can make it per-socket) delta to that timestamp.

After wondering what the heck you were talking about, I realized I
assumed you were talking about TCP timeouts and not timestamps :)

I assume you mean the following:

  1. Put a absolute time stamp in the checkpoint stream
  2. Calculate the delta between that and the current time on the
     restoring host
  3. Use that value to offset timestamps from that point on.

Correct?

Since I brought it up, do you agree that the retransmit timers should
be canonicalized to time-after checkpoint values?  It occurs to me
that right now I restore a jiffies value on the receiving host which
is guaranteed to be incorrect :)

OL> So I'm thinking, for both, do (1) put a big fat comment in the
OL> code saying that sanity-tests are needed, and what for, and (2)
OL> send a separate mail to the networking people with these two
OL> scenarios and request comments ?

Yeah, although I would hope that they're seeing this conversation and
would chime in (hence the cc:netdev).  Hopefully I don't have to
disguise a separate email as non-C/R related to get past their
filters! :)

OL> For example, now, if a user wants to send a TCP packet with
OL> arbitrary protocol parameters, he needs to use raw IP sockets,
OL> which require privilege. Would restarting a connection with the
OL> desired parameters become a way to bypass that restriction ?
OL> (e.g. assume the user restarts while using the host's network
OL> namespace).

Um, yeah?  I don't see much way around that if we're going to trust
any of what is in the checkpoint stream.  Perhaps we say CAP_NET_ADMIN
is required to restart a live TCP connection?

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

Re: [PATCH 2/2] [RFC] Add c/r support for connected INET sockets

[Oren Laadan]

Dan Smith wrote:
> OL> IIRC, the TCP stack takes the timestamp for each packet directly
> OL> from jiffies. So you need to teach TCP to add a per-container (or
> OL> you can make it per-socket) delta to that timestamp.
> 
> After wondering what the heck you were talking about, I realized I
> assumed you were talking about TCP timeouts and not timestamps :)
> 
> I assume you mean the following:
> 
>   1. Put a absolute time stamp in the checkpoint stream
>   2. Calculate the delta between that and the current time on the
>      restoring host
>   3. Use that value to offset timestamps from that point on.
> 
> Correct?

Sort of. Right now we already record the absolute time-of-checkpoint:
ctx->ktime_begin.  The restart-blocks are saved relative to it. I'd
suggest the same for all time related data from the network - save it
in the checkpoint image as delta's compared to checkpoint time.

At restart, the restart-blocks are restored relative to restart-time,
using the saved delta. That would work for the TCP timestamps too.

> 
> Since I brought it up, do you agree that the retransmit timers should
> be canonicalized to time-after checkpoint values?  It occurs to me
> that right now I restore a jiffies value on the receiving host which
> is guaranteed to be incorrect :)

As for TCP timeouts - I don't think they matter that much in the case
of live migration, whether the timeout after restart happens in the
saved delta relative to original checkpoint-time, or new restart-time.
The difference is likely to be subsecond to a few seconds at most,
not important for most use-cases, I'd think.

(If we are concerned about a TCP hickup due to a migration, there are
tricks to work around it that; timeouts and retransmits are not the
best way to go, because once you get there you already slowed down
TCP significantly).

Here, too, once we have time virtualization this can be revisited, to
allow the user to choose a policy how to use the deltas.

> 
> OL> So I'm thinking, for both, do (1) put a big fat comment in the
> OL> code saying that sanity-tests are needed, and what for, and (2)
> OL> send a separate mail to the networking people with these two
> OL> scenarios and request comments ?
> 
> Yeah, although I would hope that they're seeing this conversation and
> would chime in (hence the cc:netdev).  Hopefully I don't have to
> disguise a separate email as non-C/R related to get past their
> filters! :)
> 
> OL> For example, now, if a user wants to send a TCP packet with
> OL> arbitrary protocol parameters, he needs to use raw IP sockets,
> OL> which require privilege. Would restarting a connection with the
> OL> desired parameters become a way to bypass that restriction ?
> OL> (e.g. assume the user restarts while using the host's network
> OL> namespace).
> 
> Um, yeah?  I don't see much way around that if we're going to trust
> any of what is in the checkpoint stream.  Perhaps we say CAP_NET_ADMIN
> is required to restart a live TCP connection?
> 

I don't see much way around that either. My point is to bring the issue
to everyone's attention, and see what others say about it.

CAP_NET_ADMIN is one option. CAP_NET_RAW is another option ?

Oren.