Add support for SEM_UNDO, round three

Add support for SEM_UNDO, round three

[Dan Smith]

In this (hopefully final) version of the set, I integrated the round
two feedback and changed the build bug to use the new macros.

[PATCH 1/2] Add ipc_namespace to struct sem_undo_list

[Dan Smith]

Checkpoint/Restart needs to have a pointer to the ipc_namespace that the
sem_undo_list applies to in order to properly bring up and tear down
the object hash.  This patch adds a pointer to the namespace to the list
structure, as well as breaks out the allocation of the undo list to a
separate function (which is needed in a later C/R patch anyway).

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 include/linux/sem.h |    2 ++
 ipc/sem.c           |   30 +++++++++++++++++++++---------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/include/linux/sem.h b/include/linux/sem.h
index 8a4adbe..8cf9636 100644
--- a/include/linux/sem.h
+++ b/include/linux/sem.h
@@ -127,12 +127,14 @@ struct sem_undo {
 	short *			semadj;		/* array of adjustments, one per semaphore */
 };
 
+struct ipc_namespace;
 /* sem_undo_list controls shared access to the list of sem_undo structures
  * that may be shared among all a CLONE_SYSVSEM task group.
  */ 
 struct sem_undo_list {
 	atomic_t		refcnt;
 	spinlock_t		lock;
+	struct ipc_namespace	*ipc_ns;
 	struct list_head	list_proc;
 };
 
diff --git a/ipc/sem.c b/ipc/sem.c
index 37da85e..e439b73 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -983,6 +983,21 @@ asmlinkage long SyS_semctl(int semid, int semnum, int cmd, union semun arg)
 SYSCALL_ALIAS(sys_semctl, SyS_semctl);
 #endif
 
+static struct sem_undo_list *alloc_undo_list(struct ipc_namespace *ipc_ns)
+{
+	struct sem_undo_list *undo_list;
+
+	undo_list = kzalloc(sizeof(*undo_list), GFP_KERNEL);
+	if (undo_list == NULL)
+		return NULL;
+	spin_lock_init(&undo_list->lock);
+	atomic_set(&undo_list->refcnt, 1);
+	INIT_LIST_HEAD(&undo_list->list_proc);
+	undo_list->ipc_ns = ipc_ns;
+
+	return undo_list;
+}
+
 /* If the task doesn't already have a undo_list, then allocate one
  * here.  We guarantee there is only one thread using this undo list,
  * and current is THE ONE
@@ -994,19 +1009,16 @@ SYSCALL_ALIAS(sys_semctl, SyS_semctl);
  *
  * This can block, so callers must hold no locks.
  */
-static inline int get_undo_list(struct sem_undo_list **undo_listp)
+static inline int get_undo_list(struct sem_undo_list **undo_listp,
+				struct ipc_namespace *ipc_ns)
 {
 	struct sem_undo_list *undo_list;
 
 	undo_list = current->sysvsem.undo_list;
 	if (!undo_list) {
-		undo_list = kzalloc(sizeof(*undo_list), GFP_KERNEL);
-		if (undo_list == NULL)
+		undo_list = alloc_undo_list(ipc_ns);
+		if (!undo_list)
 			return -ENOMEM;
-		spin_lock_init(&undo_list->lock);
-		atomic_set(&undo_list->refcnt, 1);
-		INIT_LIST_HEAD(&undo_list->list_proc);
-
 		current->sysvsem.undo_list = undo_list;
 	}
 	*undo_listp = undo_list;
@@ -1057,7 +1069,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
 	int nsems;
 	int error;
 
-	error = get_undo_list(&ulp);
+	error = get_undo_list(&ulp, ns);
 	if (error)
 		return ERR_PTR(error);
 
@@ -1328,7 +1340,7 @@ int copy_semundo(unsigned long clone_flags, struct task_struct *tsk)
 	int error;
 
 	if (clone_flags & CLONE_SYSVSEM) {
-		error = get_undo_list(&undo_list);
+		error = get_undo_list(&undo_list, tsk->nsproxy->ipc_ns);
 		if (error)
 			return error;
 		atomic_inc(&undo_list->refcnt);
-- 
1.7.1.1

[PATCH 2/2] Add support for the per-task sem_undo list (v3.01)

[Dan Smith]

The semaphore undo list is a set of adjustments to be made to semaphores
held by a task on exit.  Right now, we do not checkpoint or restore this
list which could cause undesirable behavior by a restarted process on exit.

Changes in v3:
 - Move taking of the refcount for the first process to restore_sem_undo()
   and make restore_obj_sem_undo() take the reference for second-and-
   later tasks
 - Fix uses of __u16 to represent a short
 - Fix potential for overrunning the un->semadj buffer in restore
 - Move the checkpoint object and init functions to the bottom of sem.c
 - Use ckpt_read_payload() instead of allocating our own semadj buffer
 - Change the build bug macro to use the new one introduced in the previous
   patch

Changes in v2:
 - Remove collect operation
 - Add a BUILD_BUG_ON() to ensure sizeof(short) == sizeof(__u16)
 - Use sizeof(__u16) when copying to/from checkpoint header
 - Fix a couple of leaked hdr objects
 - Avoid reading the semadj buffer with rcu_read_lock() held
 - Set the sem_undo pointer on tasks other than the first to restore a list
 - Fix refcounting on restart
 - Pull out the guts of exit_sem() into put_undo_list() and call that
   from our drop() function in case we're the last one.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 include/linux/checkpoint.h     |    4 +
 include/linux/checkpoint_hdr.h |   18 ++
 ipc/sem.c                      |  344 ++++++++++++++++++++++++++++++++++++++--
 kernel/checkpoint/process.c    |   13 ++
 4 files changed, 363 insertions(+), 16 deletions(-)

diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index 4e25042..a11d40e 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -271,6 +271,10 @@ extern int ckpt_collect_fs(struct ckpt_ctx *ctx, struct task_struct *t);
 extern int checkpoint_obj_fs(struct ckpt_ctx *ctx, struct task_struct *t);
 extern int restore_obj_fs(struct ckpt_ctx *ctx, int fs_objref);
 
+/* per-task semaphore undo */
+extern int checkpoint_obj_sem_undo(struct ckpt_ctx *ctx, struct task_struct *t);
+extern int restore_obj_sem_undo(struct ckpt_ctx *ctx, int sem_undo_objref);
+
 /* memory */
 extern void ckpt_pgarr_free(struct ckpt_ctx *ctx);
 
diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
index 75d863d..005cd8d 100644
--- a/include/linux/checkpoint_hdr.h
+++ b/include/linux/checkpoint_hdr.h
@@ -182,6 +182,10 @@ enum {
 #define CKPT_HDR_IPC_MSG_MSG CKPT_HDR_IPC_MSG_MSG
 	CKPT_HDR_IPC_SEM,
 #define CKPT_HDR_IPC_SEM CKPT_HDR_IPC_SEM
+	CKPT_HDR_TASK_SEM_UNDO_LIST,
+#define CKPT_HDR_TASK_SEM_UNDO_LIST CKPT_HDR_TASK_SEM_UNDO_LIST
+	CKPT_HDR_TASK_SEM_UNDO,
+#define CKPT_HDR_TASK_SEM_UNDO CKPT_HDR_TASK_SEM_UNDO
 
 	CKPT_HDR_SIGHAND = 601,
 #define CKPT_HDR_SIGHAND CKPT_HDR_SIGHAND
@@ -288,6 +292,8 @@ enum obj_type {
 #define CKPT_OBJ_NET_NS CKPT_OBJ_NET_NS
 	CKPT_OBJ_NETDEV,
 #define CKPT_OBJ_NETDEV CKPT_OBJ_NETDEV
+	CKPT_OBJ_SEM_UNDO,
+#define CKPT_OBJ_SEM_UNDO CKPT_OBJ_SEM_UNDO
 	CKPT_OBJ_MAX
 #define CKPT_OBJ_MAX CKPT_OBJ_MAX
 };
@@ -476,6 +482,17 @@ struct ckpt_hdr_ns {
 	__s32 net_objref;
 } __attribute__((aligned(8)));
 
+struct ckpt_hdr_task_sem_undo_list {
+	struct ckpt_hdr h;
+	__u32 count;
+};
+
+struct ckpt_hdr_task_sem_undo {
+	struct ckpt_hdr h;
+	__u32 semid;
+	__u32 semadj_count;
+};
+
 /* cannot include <linux/tty.h> from userspace, so define: */
 #define CKPT_NEW_UTS_LEN  64
 #ifdef __KERNEL__
@@ -502,6 +519,7 @@ struct ckpt_hdr_task_objs {
 	__s32 files_objref;
 	__s32 mm_objref;
 	__s32 fs_objref;
+	__s32 sem_undo_objref;
 	__s32 sighand_objref;
 	__s32 signal_objref;
 } __attribute__((aligned(8)));
diff --git a/ipc/sem.c b/ipc/sem.c
index e439b73..ee5b386 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -132,14 +132,6 @@ void sem_exit_ns(struct ipc_namespace *ns)
 }
 #endif
 
-void __init sem_init (void)
-{
-	sem_init_ns(&init_ipc_ns);
-	ipc_init_proc_interface("sysvipc/sem",
-				"       key      semid perms      nsems   uid   gid  cuid  cgid      otime      ctime\n",
-				IPC_SEM_IDS, sysvipc_sem_proc_show);
-}
-
 /*
  * sem_lock_(check_) routines are called in the paths where the rw_mutex
  * is not held.
@@ -1363,14 +1355,8 @@ int copy_semundo(unsigned long clone_flags, struct task_struct *tsk)
  * The current implementation does not do so. The POSIX standard
  * and SVID should be consulted to determine what behavior is mandated.
  */
-void exit_sem(struct task_struct *tsk)
+static void put_undo_list(struct sem_undo_list *ulp)
 {
-	struct sem_undo_list *ulp;
-
-	ulp = tsk->sysvsem.undo_list;
-	if (!ulp)
-		return;
-	tsk->sysvsem.undo_list = NULL;
 
 	if (!atomic_dec_and_test(&ulp->refcnt))
 		return;
@@ -1393,7 +1379,7 @@ void exit_sem(struct task_struct *tsk)
 		if (semid == -1)
 			break;
 
-		sma = sem_lock_check(tsk->nsproxy->ipc_ns, un->semid);
+		sma = sem_lock_check(ulp->ipc_ns, un->semid);
 
 		/* exit_sem raced with IPC_RMID, nothing to do */
 		if (IS_ERR(sma))
@@ -1451,6 +1437,16 @@ void exit_sem(struct task_struct *tsk)
 	kfree(ulp);
 }
 
+void exit_sem(struct task_struct *tsk)
+{
+	struct sem_undo_list *ulp = tsk->sysvsem.undo_list;
+
+	if (ulp) {
+		put_undo_list(ulp);
+		tsk->sysvsem.undo_list = NULL;
+	}
+}
+
 #ifdef CONFIG_PROC_FS
 static int sysvipc_sem_proc_show(struct seq_file *s, void *it)
 {
@@ -1470,3 +1466,319 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it)
 			  sma->sem_ctime);
 }
 #endif
+
+static int __get_task_semids(struct sem_undo_list *ulp, int *semids, int max)
+{
+	int count = 0;
+	struct sem_undo *un;
+
+	if (!ulp)
+		return 0;
+
+	spin_lock(&ulp->lock);
+	list_for_each_entry_rcu(un, &ulp->list_proc, list_proc) {
+		if (count >= max) {
+			count = -E2BIG;
+			break;
+		}
+		semids[count++] = un->semid;
+	}
+	spin_unlock(&ulp->lock);
+
+	return count;
+}
+
+static int get_task_semids(struct sem_undo_list *ulp, int **semid_listp)
+{
+	int ret;
+	int max = 32;
+	int *semid_list = NULL;
+ retry:
+	*semid_listp = krealloc(semid_list, max * sizeof(int), GFP_KERNEL);
+	if (!*semid_listp) {
+		kfree(semid_list);
+		return -ENOMEM;
+	}
+	semid_list = *semid_listp;
+
+	ret = __get_task_semids(ulp, semid_list, max);
+	if (ret == -E2BIG) {
+		max *= 2;
+		goto retry;
+	} else if (ret < 0) {
+		kfree(semid_list);
+		*semid_listp = NULL;
+	}
+
+	return ret;
+}
+
+int checkpoint_sem_undo_adj(struct ckpt_ctx *ctx, struct sem_undo *un)
+{
+	int nsems;
+	int ret;
+	short *semadj = NULL;
+	struct sem_array *sma;
+	struct ckpt_hdr_task_sem_undo *h = NULL;
+
+	sma = sem_lock(ctx->root_nsproxy->ipc_ns, un->semid);
+	if (IS_ERR(sma)) {
+		ckpt_debug("unable to lock semid %i (wrong ns?)\n", un->semid);
+		return PTR_ERR(sma);
+	}
+
+	nsems = sma->sem_nsems;
+	sem_getref_and_unlock(sma);
+
+	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO);
+	if (!h)
+		goto putref_abort;
+
+	semadj = kzalloc(nsems * sizeof(short), GFP_KERNEL);
+	if (!semadj)
+		goto putref_abort;
+
+	sem_lock_and_putref(sma);
+
+	h->semid = un->semid;
+	h->semadj_count = nsems;
+	memcpy(semadj, un->semadj, h->semadj_count * sizeof(__s16));
+
+	sem_unlock(sma);
+
+	ret = ckpt_write_obj(ctx, (struct ckpt_hdr *)h);
+	if (ret == 0)
+		ret = ckpt_write_buffer(ctx, semadj, nsems * sizeof(__s16));
+
+	kfree(semadj);
+	ckpt_hdr_put(ctx, h);
+
+	return ret;
+
+ putref_abort:
+	sem_putref(sma);
+	if (h)
+		ckpt_hdr_put(ctx, h);
+
+	return -ENOMEM;
+}
+
+int write_sem_undo_list(struct ckpt_ctx *ctx, struct sem_undo_list *ulp,
+			int count, int *semids)
+{
+	int i;
+	int ret;
+
+	for (i = 0; i < count; i++) {
+		struct sem_undo *un;
+
+		spin_lock(&ulp->lock);
+		un = lookup_undo(ulp, semids[i]);
+		spin_unlock(&ulp->lock);
+
+		if (!un) {
+			ckpt_debug("unable to lookup semid %i\n", semids[i]);
+			return -EINVAL;
+		}
+
+		ret = checkpoint_sem_undo_adj(ctx, un);
+		ckpt_debug("checkpoint_sem_undo: %i\n", ret);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int checkpoint_sem_undo(struct ckpt_ctx *ctx, void *ptr)
+{
+	int ret;
+	int *semids = NULL;
+	struct sem_undo_list *ulp = ptr;
+	struct ckpt_hdr_task_sem_undo_list *h;
+
+	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO_LIST);
+	if (!h)
+		return -ENOMEM;
+
+	ret = get_task_semids(ulp, &semids);
+	if (ret < 0)
+		goto out;
+	h->count = ret;
+
+	ret = ckpt_write_obj(ctx, (struct ckpt_hdr *)h);
+	if (ret < 0)
+		goto out;
+
+	ret = write_sem_undo_list(ctx, ulp, h->count, semids);
+ out:
+	ckpt_hdr_put(ctx, h);
+	kfree(semids);
+
+	return ret;
+}
+
+int checkpoint_obj_sem_undo(struct ckpt_ctx *ctx, struct task_struct *t)
+{
+	struct sem_undo_list *ulp;
+
+	ulp = t->sysvsem.undo_list;
+	if (ulp)
+		return checkpoint_obj(ctx, ulp, CKPT_OBJ_SEM_UNDO);
+
+	return 0;
+}
+
+/* Count the number of sems for the given sem_undo->semid */
+static int sem_undo_nsems(struct sem_undo *un, struct ipc_namespace *ns)
+{
+	struct sem_array *sma;
+	int nsems;
+
+	sma = sem_lock(ns, un->semid);
+	if (IS_ERR(sma))
+		return PTR_ERR(sma);
+
+	nsems = sma->sem_nsems;
+	sem_unlock(sma);
+
+	return nsems;
+}
+
+static int restore_task_sem_undo_adj(struct ckpt_ctx *ctx)
+{
+	struct ckpt_hdr_task_sem_undo *h;
+	int len;
+	int ret = -ENOMEM;
+	struct sem_undo *un;
+	int nsems;
+	__s16 *semadj = NULL;
+
+	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO);
+	if (IS_ERR(h))
+		return PTR_ERR(h);
+
+	len = sizeof(__s16) * h->semadj_count;
+	ret = ckpt_read_payload(ctx, (void **)&semadj, len, CKPT_HDR_BUFFER);
+	if (ret < 0)
+		goto out;
+
+	un = find_alloc_undo(current->nsproxy->ipc_ns, h->semid);
+	if (IS_ERR(un)) {
+		ret = PTR_ERR(un);
+		ckpt_debug("unable to find semid %i\n", h->semid);
+		goto out;
+	}
+
+	nsems = sem_undo_nsems(un, current->nsproxy->ipc_ns);
+	len = sizeof(__s16) * nsems;
+	if (h->semadj_count == nsems)
+		memcpy(un->semadj, semadj, len);
+	rcu_read_unlock();
+
+	if (nsems != h->semadj_count)
+		ckpt_err(ctx, -EINVAL,
+			 "semid %i has nmsems=%i but %i undo adjustments\n",
+			 h->semid, nsems, h->semadj_count);
+	else
+		ckpt_debug("semid %i restored with %i adjustments\n",
+			   h->semid, h->semadj_count);
+ out:
+	ckpt_hdr_put(ctx, h);
+	kfree(semadj);
+
+	return ret;
+}
+
+static void *restore_sem_undo(struct ckpt_ctx *ctx)
+{
+	struct ckpt_hdr_task_sem_undo_list *h;
+	struct sem_undo_list *ulp = NULL;
+	int i;
+	int ret = 0;
+
+	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO_LIST);
+	if (IS_ERR(h))
+		return ERR_PTR(PTR_ERR(h));
+
+	ulp = alloc_undo_list(current->nsproxy->ipc_ns);
+	if (!ulp) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < h->count; i++) {
+		ret = restore_task_sem_undo_adj(ctx);
+		if (ret < 0)
+			goto out;
+	}
+
+	atomic_inc(&ulp->refcnt);
+ out:
+	ckpt_hdr_put(ctx, h);
+	if (ret < 0)
+		return ERR_PTR(ret);
+	else
+		return ulp;
+}
+
+int restore_obj_sem_undo(struct ckpt_ctx *ctx, int sem_undo_objref)
+{
+	struct sem_undo_list *ulp;
+
+	if (!sem_undo_objref)
+		return 0; /* Task had no undo list */
+
+	ulp = ckpt_obj_try_fetch(ctx, sem_undo_objref, CKPT_OBJ_SEM_UNDO);
+	if (IS_ERR(ulp))
+		return PTR_ERR(ulp);
+
+	/* The first task to restore a shared list should already have this,
+	 * but subsequent ones won't, so attach to current in that case and
+	 * take our reference.
+	 */
+	if (!current->sysvsem.undo_list) {
+		current->sysvsem.undo_list = ulp;
+		atomic_inc(&ulp->refcnt);
+	}
+
+	return 0;
+}
+
+static int obj_sem_undo_grab(void *ptr)
+{
+	struct sem_undo_list *ulp = ptr;
+
+	atomic_inc(&ulp->refcnt);
+	return 0;
+}
+
+static void obj_sem_undo_drop(void *ptr, int lastref)
+{
+	struct sem_undo_list *ulp = ptr;
+
+	put_undo_list(ulp);
+}
+
+static const struct ckpt_obj_ops ckpt_obj_sem_undo_ops = {
+	.obj_name = "IPC_SEM_UNDO",
+	.obj_type = CKPT_OBJ_SEM_UNDO,
+	.ref_drop = obj_sem_undo_drop,
+	.ref_grab = obj_sem_undo_grab,
+	.checkpoint = checkpoint_sem_undo,
+	.restore = restore_sem_undo,
+};
+
+void __init sem_init (void)
+{
+	sem_init_ns(&init_ipc_ns);
+	ipc_init_proc_interface("sysvipc/sem",
+				"       key      semid perms      nsems   uid   gid  cuid  cgid      otime      ctime\n",
+				IPC_SEM_IDS, sysvipc_sem_proc_show);
+
+	/* sem_undo_list uses a short but we write a __s16 */
+	CKPT_BUILD_BUG_ON_MISMATCH(*CKPT_STRUCT_MEMBER(sem_undo, semadj),
+				   __s16);
+
+	register_checkpoint_obj(&ckpt_obj_sem_undo_ops);
+}
diff --git a/kernel/checkpoint/process.c b/kernel/checkpoint/process.c
index 936675a..4ec9cdd 100644
--- a/kernel/checkpoint/process.c
+++ b/kernel/checkpoint/process.c
@@ -236,6 +236,7 @@ static int checkpoint_task_objs(struct ckpt_ctx *ctx, struct task_struct *t)
 	int files_objref;
 	int mm_objref;
 	int fs_objref;
+	int sem_undo_objref;
 	int sighand_objref;
 	int signal_objref;
 	int first, ret;
@@ -283,6 +284,12 @@ static int checkpoint_task_objs(struct ckpt_ctx *ctx, struct task_struct *t)
 		return fs_objref;
 	}
 
+	sem_undo_objref = checkpoint_obj_sem_undo(ctx, t);
+	if (sem_undo_objref < 0) {
+		ckpt_err(ctx, sem_undo_objref, "%(T)process sem_undo\n");
+		return sem_undo_objref;
+	}
+
 	sighand_objref = checkpoint_obj_sighand(ctx, t);
 	ckpt_debug("sighand: objref %d\n", sighand_objref);
 	if (sighand_objref < 0) {
@@ -311,6 +318,7 @@ static int checkpoint_task_objs(struct ckpt_ctx *ctx, struct task_struct *t)
 	h->files_objref = files_objref;
 	h->mm_objref = mm_objref;
 	h->fs_objref = fs_objref;
+	h->sem_undo_objref = sem_undo_objref;
 	h->sighand_objref = sighand_objref;
 	h->signal_objref = signal_objref;
 	ret = ckpt_write_obj(ctx, &h->h);
@@ -679,6 +687,11 @@ static int restore_task_objs(struct ckpt_ctx *ctx)
 	if (ret < 0)
 		return ret;
 
+	ret = restore_obj_sem_undo(ctx, h->sem_undo_objref);
+	ckpt_debug("sem_undo: ret %d\n", ret);
+	if (ret < 0)
+		return ret;
+
 	ret = restore_obj_sighand(ctx, h->sighand_objref);
 	ckpt_debug("sighand: ret %d (%p)\n", ret, current->sighand);
 	if (ret < 0)
-- 
1.7.1.1

Re: [PATCH 2/2] Add support for the per-task sem_undo list (v3.01)

[Matt Helsley]

On Thu, Aug 05, 2010 at 10:57:06AM -0700, Dan Smith wrote:
> The semaphore undo list is a set of adjustments to be made to semaphores
> held by a task on exit.  Right now, we do not checkpoint or restore this
> list which could cause undesirable behavior by a restarted process on exit.

<snip>

> diff --git a/ipc/sem.c b/ipc/sem.c
> index e439b73..ee5b386 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c

<snip>

> @@ -1470,3 +1466,319 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it)
>  			  sma->sem_ctime);
>  }
>  #endif
> +
> +static int __get_task_semids(struct sem_undo_list *ulp, int *semids, int max)

Why "task"? I'd name it __get_undo_list_semids or something like that
instead.

> +{
> +	int count = 0;
> +	struct sem_undo *un;
> +
> +	if (!ulp)
> +		return 0;
> +
> +	spin_lock(&ulp->lock);
> +	list_for_each_entry_rcu(un, &ulp->list_proc, list_proc) {
> +		if (count >= max) {
> +			count = -E2BIG;
> +			break;
> +		}
> +		semids[count++] = un->semid;
> +	}
> +	spin_unlock(&ulp->lock);
> +
> +	return count;
> +}

<snip>

> +int checkpoint_sem_undo_adj(struct ckpt_ctx *ctx, struct sem_undo *un)

Should be static.

<snip>

> +
> +int write_sem_undo_list(struct ckpt_ctx *ctx, struct sem_undo_list *ulp,
> +			int count, int *semids)

Should be static.

> +{
> +	int i;
> +	int ret;
> +
> +	for (i = 0; i < count; i++) {
> +		struct sem_undo *un;
> +
> +		spin_lock(&ulp->lock);
> +		un = lookup_undo(ulp, semids[i]);
> +		spin_unlock(&ulp->lock);
> +
> +		if (!un) {
> +			ckpt_debug("unable to lookup semid %i\n", semids[i]);

Or should this be a ckpt_err() ?

> +			return -EINVAL;
> +		}
> +
> +		ret = checkpoint_sem_undo_adj(ctx, un);
> +		ckpt_debug("checkpoint_sem_undo: %i\n", ret);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

<snip>

> +static int restore_task_sem_undo_adj(struct ckpt_ctx *ctx)
> +{
> +	struct ckpt_hdr_task_sem_undo *h;
> +	int len;
> +	int ret = -ENOMEM;
> +	struct sem_undo *un;
> +	int nsems;
> +	__s16 *semadj = NULL;
> +
> +	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO);
> +	if (IS_ERR(h))
> +		return PTR_ERR(h);
> +
> +	len = sizeof(__s16) * h->semadj_count;
> +	ret = ckpt_read_payload(ctx, (void **)&semadj, len, CKPT_HDR_BUFFER);
> +	if (ret < 0)
> +		goto out;
> +

As best I can tell exit_sem() does not do permission checking -- it
relies on semop and semtimedop to check permissions on the operation
that created the undo in the first place. Thus I think we need to check
permissions here with ipc_check_perms() or ipcperms(). Otherwise it may
be possible to manipulate semaphores we do not have permission to
manipulate.

> +	un = find_alloc_undo(current->nsproxy->ipc_ns, h->semid);
> +	if (IS_ERR(un)) {
> +		ret = PTR_ERR(un);
> +		ckpt_debug("unable to find semid %i\n", h->semid);
> +		goto out;
> +	}
> +
> +	nsems = sem_undo_nsems(un, current->nsproxy->ipc_ns);
> +	len = sizeof(__s16) * nsems;
> +	if (h->semadj_count == nsems)
> +		memcpy(un->semadj, semadj, len);
> +	rcu_read_unlock();
> +
> +	if (nsems != h->semadj_count)
> +		ckpt_err(ctx, -EINVAL,
> +			 "semid %i has nmsems=%i but %i undo adjustments\n",
> +			 h->semid, nsems, h->semadj_count);
> +	else
> +		ckpt_debug("semid %i restored with %i adjustments\n",
> +			   h->semid, h->semadj_count);
> + out:
> +	ckpt_hdr_put(ctx, h);
> +	kfree(semadj);
> +
> +	return ret;
> +}
> +
> +static void *restore_sem_undo(struct ckpt_ctx *ctx)
> +{
> +	struct ckpt_hdr_task_sem_undo_list *h;
> +	struct sem_undo_list *ulp = NULL;
> +	int i;
> +	int ret = 0;
> +
> +	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO_LIST);
> +	if (IS_ERR(h))
> +		return ERR_PTR(PTR_ERR(h));
> +
> +	ulp = alloc_undo_list(current->nsproxy->ipc_ns);
> +	if (!ulp) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < h->count; i++) {
> +		ret = restore_task_sem_undo_adj(ctx);
> +		if (ret < 0)
> +			goto out;
> +	}
> +
> +	atomic_inc(&ulp->refcnt);
> + out:
> +	ckpt_hdr_put(ctx, h);
> +	if (ret < 0)

Don't we need to check ulp and clean up the alloc'd undo list here?

> +		return ERR_PTR(ret);
> +	else
> +		return ulp;
> +}

Cheers,
	-Matt Helsley

Re: [PATCH 1/2] Add ipc_namespace to struct sem_undo_list

[Matt Helsley]

On Thu, Aug 05, 2010 at 10:57:05AM -0700, Dan Smith wrote:
> Checkpoint/Restart needs to have a pointer to the ipc_namespace that the
> sem_undo_list applies to in order to properly bring up and tear down
> the object hash.  This patch adds a pointer to the namespace to the list
> structure, as well as breaks out the allocation of the undo list to a
> separate function (which is needed in a later C/R patch anyway).
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Reviewed-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  include/linux/sem.h |    2 ++
>  ipc/sem.c           |   30 +++++++++++++++++++++---------
>  2 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/sem.h b/include/linux/sem.h
> index 8a4adbe..8cf9636 100644
> --- a/include/linux/sem.h
> +++ b/include/linux/sem.h
> @@ -127,12 +127,14 @@ struct sem_undo {
>  	short *			semadj;		/* array of adjustments, one per semaphore */
>  };
> 
> +struct ipc_namespace;
>  /* sem_undo_list controls shared access to the list of sem_undo structures
>   * that may be shared among all a CLONE_SYSVSEM task group.
>   */ 
>  struct sem_undo_list {
>  	atomic_t		refcnt;
>  	spinlock_t		lock;
> +	struct ipc_namespace	*ipc_ns;
>  	struct list_head	list_proc;
>  };
> 
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 37da85e..e439b73 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -983,6 +983,21 @@ asmlinkage long SyS_semctl(int semid, int semnum, int cmd, union semun arg)
>  SYSCALL_ALIAS(sys_semctl, SyS_semctl);
>  #endif
> 
> +static struct sem_undo_list *alloc_undo_list(struct ipc_namespace *ipc_ns)
> +{
> +	struct sem_undo_list *undo_list;
> +
> +	undo_list = kzalloc(sizeof(*undo_list), GFP_KERNEL);
> +	if (undo_list == NULL)
> +		return NULL;
> +	spin_lock_init(&undo_list->lock);
> +	atomic_set(&undo_list->refcnt, 1);
> +	INIT_LIST_HEAD(&undo_list->list_proc);
> +	undo_list->ipc_ns = ipc_ns;
> +
> +	return undo_list;
> +}
> +
>  /* If the task doesn't already have a undo_list, then allocate one
>   * here.  We guarantee there is only one thread using this undo list,
>   * and current is THE ONE
> @@ -994,19 +1009,16 @@ SYSCALL_ALIAS(sys_semctl, SyS_semctl);
>   *
>   * This can block, so callers must hold no locks.
>   */
> -static inline int get_undo_list(struct sem_undo_list **undo_listp)
> +static inline int get_undo_list(struct sem_undo_list **undo_listp,
> +				struct ipc_namespace *ipc_ns)
>  {
>  	struct sem_undo_list *undo_list;
> 
>  	undo_list = current->sysvsem.undo_list;
>  	if (!undo_list) {
> -		undo_list = kzalloc(sizeof(*undo_list), GFP_KERNEL);
> -		if (undo_list == NULL)
> +		undo_list = alloc_undo_list(ipc_ns);
> +		if (!undo_list)
>  			return -ENOMEM;
> -		spin_lock_init(&undo_list->lock);
> -		atomic_set(&undo_list->refcnt, 1);
> -		INIT_LIST_HEAD(&undo_list->list_proc);
> -
>  		current->sysvsem.undo_list = undo_list;
>  	}
>  	*undo_listp = undo_list;
> @@ -1057,7 +1069,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
>  	int nsems;
>  	int error;
> 
> -	error = get_undo_list(&ulp);
> +	error = get_undo_list(&ulp, ns);
>  	if (error)
>  		return ERR_PTR(error);
> 
> @@ -1328,7 +1340,7 @@ int copy_semundo(unsigned long clone_flags, struct task_struct *tsk)
>  	int error;
> 
>  	if (clone_flags & CLONE_SYSVSEM) {
> -		error = get_undo_list(&undo_list);
> +		error = get_undo_list(&undo_list, tsk->nsproxy->ipc_ns);
>  		if (error)
>  			return error;
>  		atomic_inc(&undo_list->refcnt);
> -- 
> 1.7.1.1
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH 2/2] Add support for the per-task sem_undo list (v3.01)

[Oren Laadan]

On 08/05/2010 05:48 PM, Matt Helsley wrote:
> On Thu, Aug 05, 2010 at 10:57:06AM -0700, Dan Smith wrote:
>> The semaphore undo list is a set of adjustments to be made to semaphores
>> held by a task on exit.  Right now, we do not checkpoint or restore this
>> list which could cause undesirable behavior by a restarted process on exit.

[...]

>> +{
>> +	int i;
>> +	int ret;
>> +
>> +	for (i = 0; i<  count; i++) {
>> +		struct sem_undo *un;
>> +
>> +		spin_lock(&ulp->lock);
>> +		un = lookup_undo(ulp, semids[i]);
>> +		spin_unlock(&ulp->lock);
>> +
>> +		if (!un) {
>> +			ckpt_debug("unable to lookup semid %i\n", semids[i]);
>
> Or should this be a ckpt_err() ?

This is not supposed to happen: the semdis[] array was
extracted just before, and it should not be possible for
them to disappear (at least in the container-checkpoint
case, where tasks are frozen and ipcns does not leak).

>
>> +			return -EINVAL;
>> +		}
>> +
>> +		ret = checkpoint_sem_undo_adj(ctx, un);
>> +		ckpt_debug("checkpoint_sem_undo: %i\n", ret);
>> +		if (ret<  0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>
> <snip>
>
>> +static int restore_task_sem_undo_adj(struct ckpt_ctx *ctx)

Now that Matt said it ... I suggest to rename this too to
something like "restore_sem_undo_adj()" :)

(Interestingly, the checkpoint counterpart is named so !)

>> +{
>> +	struct ckpt_hdr_task_sem_undo *h;
>> +	int len;
>> +	int ret = -ENOMEM;
>> +	struct sem_undo *un;
>> +	int nsems;
>> +	__s16 *semadj = NULL;
>> +
>> +	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO);
>> +	if (IS_ERR(h))
>> +		return PTR_ERR(h);
>> +
>> +	len = sizeof(__s16) * h->semadj_count;
>> +	ret = ckpt_read_payload(ctx, (void **)&semadj, len, CKPT_HDR_BUFFER);
>> +	if (ret<  0)
>> +		goto out;
>> +
>
> As best I can tell exit_sem() does not do permission checking -- it
> relies on semop and semtimedop to check permissions on the operation
> that created the undo in the first place. Thus I think we need to check
> permissions here with ipc_check_perms() or ipcperms(). Otherwise it may
> be possible to manipulate semaphores we do not have permission to
> manipulate.

Good catch !

>
>> +	un = find_alloc_undo(current->nsproxy->ipc_ns, h->semid);
>> +	if (IS_ERR(un)) {
>> +		ret = PTR_ERR(un);
>> +		ckpt_debug("unable to find semid %i\n", h->semid);
>> +		goto out;
>> +	}
>> +
>> +	nsems = sem_undo_nsems(un, current->nsproxy->ipc_ns);
>> +	len = sizeof(__s16) * nsems;
>> +	if (h->semadj_count == nsems)
>> +		memcpy(un->semadj, semadj, len);
>> +	rcu_read_unlock();
>> +
>> +	if (nsems != h->semadj_count)
>> +		ckpt_err(ctx, -EINVAL,
>> +			 "semid %i has nmsems=%i but %i undo adjustments\n",
>> +			 h->semid, nsems, h->semadj_count);
>> +	else
>> +		ckpt_debug("semid %i restored with %i adjustments\n",
>> +			   h->semid, h->semadj_count);
>> + out:
>> +	ckpt_hdr_put(ctx, h);
>> +	kfree(semadj);
>> +
>> +	return ret;
>> +}
>> +
>> +static void *restore_sem_undo(struct ckpt_ctx *ctx)
>> +{
>> +	struct ckpt_hdr_task_sem_undo_list *h;
>> +	struct sem_undo_list *ulp = NULL;
>> +	int i;
>> +	int ret = 0;
>> +
>> +	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO_LIST);
>> +	if (IS_ERR(h))
>> +		return ERR_PTR(PTR_ERR(h));
>> +

Maybe add this:
	/*
	 * On success, alloc_undo_list() attaches the new @ulp
	 * to current task - so no need for explicit cleanup
	 */
>> +	ulp = alloc_undo_list(current->nsproxy->ipc_ns);
>> +	if (!ulp) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	for (i = 0; i<  h->count; i++) {
>> +		ret = restore_task_sem_undo_adj(ctx);
>> +		if (ret<  0)
>> +			goto out;
>> +	}
>> +

Maybe add this:

	/* success: account for reference in the objhash*/
>> +	atomic_inc(&ulp->refcnt);
>> + out:
>> +	ckpt_hdr_put(ctx, h);
>> +	if (ret<  0)
>
> Don't we need to check ulp and clean up the alloc'd undo list here?

No: the ulp is attached to current ask (a side effect of
alloc_undo_list) so will be freed when the task exits.
The addition atomic_inc() is only performed on success,
to account for it being added to the hash once we return.

Dan: I suggest to add the comment above to make it clear.

Oren.

Re: [PATCH 1/2] Add ipc_namespace to struct sem_undo_list

[Oren Laadan]

On 08/05/2010 01:57 PM, Dan Smith wrote:
> Checkpoint/Restart needs to have a pointer to the ipc_namespace that the
> sem_undo_list applies to in order to properly bring up and tear down
> the object hash.  This patch adds a pointer to the namespace to the list
> structure, as well as breaks out the allocation of the undo list to a
> separate function (which is needed in a later C/R patch anyway).
>
> Signed-off-by: Dan Smith<danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Acked-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>

Re: [PATCH 2/2] Add support for the per-task sem_undo list (v3.01)

[Matt Helsley]

On Thu, Aug 05, 2010 at 06:40:57PM -0400, Oren Laadan wrote:
> 
> 
> On 08/05/2010 05:48 PM, Matt Helsley wrote:
> >On Thu, Aug 05, 2010 at 10:57:06AM -0700, Dan Smith wrote:
<snip>
> ><snip>
> >
> >>+static int restore_task_sem_undo_adj(struct ckpt_ctx *ctx)
> 
> Now that Matt said it ... I suggest to rename this too to
> something like "restore_sem_undo_adj()" :)
> 
> (Interestingly, the checkpoint counterpart is named so !)
> 
> >>+{
> >>+	struct ckpt_hdr_task_sem_undo *h;
> >>+	int len;
> >>+	int ret = -ENOMEM;
> >>+	struct sem_undo *un;
> >>+	int nsems;
> >>+	__s16 *semadj = NULL;
> >>+
> >>+	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO);
> >>+	if (IS_ERR(h))
> >>+		return PTR_ERR(h);
> >>+
> >>+	len = sizeof(__s16) * h->semadj_count;
> >>+	ret = ckpt_read_payload(ctx, (void **)&semadj, len, CKPT_HDR_BUFFER);
> >>+	if (ret<  0)
> >>+		goto out;
> >>+
> >
> >As best I can tell exit_sem() does not do permission checking -- it
> >relies on semop and semtimedop to check permissions on the operation
> >that created the undo in the first place. Thus I think we need to check
> >permissions here with ipc_check_perms() or ipcperms(). Otherwise it may
> >be possible to manipulate semaphores we do not have permission to
> >manipulate.
> 
> Good catch !

Perhaps instead of mimicking the kernel data structure in the checkpoint image
and then restoring that kernel structure during restart we could create
a set of semops to write in the checkpoint image and then perform them
during restart. By effectively calling sys_semop during restart we would ensure
that we're doing the proper security checks, make the checkpoint image more
portable (since the semop interface can't change), and greatly reduce the
probability that c/r of the semaphores and their undo lists will bitrot
(IOW make maintenance easy for everyone).

Cheers,
	-Matt Helsley

Re: [PATCH 2/2] Add support for the per-task sem_undo list (v3.01)

[Dan Smith]

MH> Perhaps instead of mimicking the kernel data structure in the
MH> checkpoint image and then restoring that kernel structure during
MH> restart we could create a set of semops to write in the checkpoint
MH> image and then perform them during restart. By effectively calling
MH> sys_semop during restart we would ensure that we're doing the
MH> proper security checks, make the checkpoint image more portable
MH> (since the semop interface can't change), and greatly reduce the
MH> probability that c/r of the semaphores and their undo lists will
MH> bitrot (IOW make maintenance easy for everyone).

I actually started thinking down that road at one point.  However, the
way you get something into the undo list is you take action on a
semaphore, specifying that it should be undone later.  Unless we add a
semop to allow you to arbitrarily adjust the undo list (bad idea,
IMHO), using the semop interface would require us to modify the actual
semaphore.  We could potentially try to replay the semops that led to
the semaphore's value at checkpoint, but that seems more likely to
break to me.

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