Scheduling in atomic while restoring shm

Scheduling in atomic while restoring shm

[Nikita V. Youshchenko]

Hi

While playing with checkpoint-restart code, version
several-commits-before-0.19, we have faced "scheduling in atomic" issue.

It is still in v0.19, below code is from there.

   247          down_write(&shm_ids->rw_mutex);
   248
   249          /* we are the sole owners/users of this ipc_ns, it can't go away */
   250          perms = ipc_lock(shm_ids, h->perms.id);
   251          BUG_ON(IS_ERR(perms));  /* ipc_ns is private to us */
   252
   253          shp = container_of(perms, struct shmid_kernel, shm_perm);
   254          file = shp->shm_file;
   255          get_file(file);
   256
   257          ret = load_ipc_shm_hdr(ctx, h, shp);
   258          if (ret < 0)
   259                  goto mutex;
   260
   261          /* deposit in objhash and read contents in */
   262          ret = ckpt_obj_insert(ctx, file, h->objref, CKPT_OBJ_FILE);
   263          if (ret < 0)
   264                  goto mutex;
   265          ret = restore_memory_contents(ctx, file->f_dentry->d_inode);
   266   mutex:
   267          fput(file);
   268          if (ret < 0) {
   269                  ckpt_debug("shm: need to remove (%d)\n", ret);
   270                  do_shm_rmid(ns, perms);
   271          } else
   272                  ipc_unlock(perms);
   273          up_write(&shm_ids->rw_mutex);

So restore_ipc_shm() calls ipc_lock() and then restore_memory_contents().
Inside ipc_lock(), a spinlock is taken.
Inside restore_memory_contents(), checkpoint data is read, that results
in vfs_read() and a schedule somewhere below.

Looks like a bug.

Here is a backtrace:

[  145.795810] BUG: scheduling while atomic: multitask/433/0x00000003
[  145.796661] Modules linked in:
[  145.796992] Pid: 433, comm: multitask Not tainted 2.6.33-rc5 #2
[  145.797520] Call Trace:
[  145.797833]  [<c11e096b>] ? schedule+0x80/0x627
[  145.798266]  [<c11e1f6b>] ? _raw_spin_unlock_irqrestore+0x1f/0x29
[  145.798823]  [<c1110c54>] ? debug_check_no_obj_freed+0x11d/0x175
[  145.799451]  [<c11e219d>] ? _raw_spin_lock_irqsave+0x11/0x2a
[  145.800244]  [<c1036623>] ? prepare_to_wait+0x14/0x54
[  145.800872]  [<c108171e>] ? pipe_wait+0x4a/0x61
[  145.801442]  [<c10364a4>] ? autoremove_wake_function+0x0/0x2d
[  145.802113]  [<c1081e39>] ? pipe_read+0x2c4/0x327
[  145.802641]  [<c107b8e5>] ? do_sync_read+0x9c/0xe0
[  145.803176]  [<c110a3b2>] ? radix_tree_insert+0x135/0x16d
[  145.803762]  [<c11e1f42>] ? _raw_spin_unlock_irq+0x1e/0x28
[  145.804561]  [<c1058e97>] ? add_to_page_cache_locked+0xc2/0xca
[  145.805191]  [<c10e60f2>] ? security_file_permission+0xc/0xd
[  145.805798]  [<c107b849>] ? do_sync_read+0x0/0xe0
[  145.806292]  [<c107c127>] ? vfs_read+0x73/0xa1
[  145.806783]  [<c10fd87c>] ? ckpt_kread+0x6e/0xc6
[  145.807297]  [<c1104c54>] ? restore_read_page+0x1a/0x49
[  145.807857]  [<c1104ec0>] ? restore_memory_contents+0x23d/0x2f7
[  145.808727]  [<c10e0231>] ? restore_ipc_shm+0x296/0x32d
[  145.809302]  [<c10df9e9>] ? restore_ipc_any+0xa5/0x119
[  145.809865]  [<c10dfb06>] ? restore_ipc_ns+0xa9/0x112
[  145.810406]  [<c10dff9b>] ? restore_ipc_shm+0x0/0x32d
[  145.810962]  [<c10fe1cc>] ? restore_obj+0x98/0x116
[  145.811483]  [<c10ffe71>] ? ckpt_read_obj_dispatch+0x220/0x246
[  145.812238]  [<c10ffead>] ? ckpt_read_obj+0x16/0xe8
[  145.812857]  [<c107b522>] ? fsnotify_access+0x5a/0x61
[  145.813406]  [<c1100001>] ? ckpt_read_obj_type+0x16/0x70
[  145.813975]  [<c1039a6c>] ? restore_ns+0x18/0x12b
[  145.814483]  [<c10fe1cc>] ? restore_obj+0x98/0x116
[  145.815011]  [<c10ffe71>] ? ckpt_read_obj_dispatch+0x220/0x246
[  145.815636]  [<c10ffead>] ? ckpt_read_obj+0x16/0xe8
[  145.816429]  [<c1100001>] ? ckpt_read_obj_type+0x16/0x70
[  145.817030]  [<c1102abc>] ? restore_task+0x512/0x9fc
[  145.817574]  [<c11011dd>] ? do_restart+0xff4/0x12f3
[  145.818114]  [<c10364a4>] ? autoremove_wake_function+0x0/0x2d
[  145.818735]  [<c10fd1a5>] ? do_sys_restart+0x66/0x77
[  145.819271]  [<c1002795>] ? ptregs_restart+0x15/0x1c
[  145.819816]  [<c1002690>] ? sysenter_do_call+0x12/0x26


Another related bug: if load_ipc_shm_hdr() fails in line 257, control
is transfered to mutex: label with negative ret value; ipc_unlock()
is not called on this path.

[PATCH] c/r: fix "scheduling in atomic" while restoring ipc shm

[Oren Laadan]

"""
  While playing with checkpoint-restart code, version
  several-commits-before-0.19, we have faced "scheduling in atomic" issue.
  ...
  So restore_ipc_shm() calls ipc_lock() and then restore_memory_contents().
  Inside ipc_lock(), a spinlock is taken.
  Inside restore_memory_contents(), checkpoint data is read, that results
  in vfs_read() and a schedule somewhere below.
  ...
  Another related bug: if load_ipc_shm_hdr() fails in line 257, control
  is transfered to mutex: label with negative ret value; ipc_unlock()
  is not called on this path.
"""

Actually, the error could have occurred with other sleeping functions,
for example, security_restore_obj().

The fix is to simply not take ipc_lock(). this relies on the fact that
IPC is always restored to a brand new ipc namespace which is only
accessible to the restarting process(es):

1) The ipc object (id) could not have been deleted between its creation
  and taking the rw_mutex.

2) No unauthorized task will attempt to gain access to it, so it is
  safe to do away with ipc_lock(). This is useful because we can call
  functions that sleep.

3) Likewise, we only restore the security bits further below, so it is
  safe to ignore a security (theoretical only!) race.

If (and when) we allow to restore the ipc state within the parent's
namespace, we will need to re-examine this.

While at it, there is also a cleanup to not restore perm->{id,key,seq}
or the shm->shm_segsz, as they are all set upon creation properly.

Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
Reported-by: "Nikita V. Youshchenko" <yoush-/llMDZXAvAOHXe+LvDLADg@public.gmane.org>
---
 ipc/checkpoint.c     |    9 ++----
 ipc/checkpoint_msg.c |   35 +++++++++++++++++-------
 ipc/checkpoint_sem.c |   33 ++++++++++++++++------
 ipc/checkpoint_shm.c |   74 +++++++++++++++++++++++++++++++++-----------------
 4 files changed, 101 insertions(+), 50 deletions(-)

diff --git a/ipc/checkpoint.c b/ipc/checkpoint.c
index 4322dea..06027c2 100644
--- a/ipc/checkpoint.c
+++ b/ipc/checkpoint.c
@@ -203,9 +203,6 @@ int restore_load_ipc_perms(struct ckpt_ctx *ctx,
 
 	/* FIX: verify the ->mode field makes sense */
 
-	perm->id = h->id;
-	perm->key = h->key;
-
 	if (!validate_created_perms(h))
 		return -EPERM;
 	perm->uid = h->uid;
@@ -213,10 +210,10 @@ int restore_load_ipc_perms(struct ckpt_ctx *ctx,
 	perm->cuid = h->cuid;
 	perm->cgid = h->cgid;
 	perm->mode = h->mode;
-	perm->seq = h->seq;
 
-	return security_restore_obj(ctx, (void *)perm, CKPT_SECURITY_IPC,
-				   h->sec_ref);
+	return security_restore_obj(ctx, (void *)perm,
+				    CKPT_SECURITY_IPC,
+				    h->sec_ref);
 }
 
 static int restore_ipc_any(struct ckpt_ctx *ctx, struct ipc_namespace *ipc_ns,
diff --git a/ipc/checkpoint_msg.c b/ipc/checkpoint_msg.c
index 61b3d78..073a893 100644
--- a/ipc/checkpoint_msg.c
+++ b/ipc/checkpoint_msg.c
@@ -306,7 +306,7 @@ static int restore_msg_contents(struct ckpt_ctx *ctx, struct list_head *queue,
 int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 {
 	struct ckpt_hdr_ipc_msg *h;
-	struct kern_ipc_perm *perms;
+	struct kern_ipc_perm *ipc;
 	struct msg_queue *msq;
 	struct ipc_ids *msg_ids = &ns->ids[IPC_MSG_IDS];
 	struct list_head messages;
@@ -344,12 +344,26 @@ int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 
 	down_write(&msg_ids->rw_mutex);
 
-	/* we are the sole owners/users of this ipc_ns, it can't go away */
-	perms = ipc_lock(msg_ids, h->perms.id);
-	BUG_ON(IS_ERR(perms));	/* ipc_ns is private to us */
+	/*
+	 * We are the sole owners/users of this brand new ipc-ns, so:
+	 *
+	 * 1) The msgid could not have been deleted between its creation
+	 *   and taking the rw_mutex above.
+	 * 2) No unauthorized task will attempt to gain access to it,
+	 *   so it is safe to do away with ipc_lock(). This is useful
+	 *   because we can call functions that sleep.
+	 * 3) Likewise, we only restore the security bits further below,
+	 *   so it is safe to ignore this (theoretical only!) race.
+	 *
+	 * If/when we allow to restore the ipc state within the parent's
+	 * ipc-ns, we will need to re-examine this.
+	 */
+
+	ipc = idr_find(&msg_ids->ipcs_idr, h->perms.id);
+	BUG_ON(!ipc);
 
-	msq = container_of(perms, struct msg_queue, q_perm);
-	BUG_ON(!list_empty(&msq->q_messages));	/* ipc_ns is private to us */
+	msq = container_of(ipc, struct msg_queue, q_perm);
+	BUG_ON(!list_empty(&msq->q_messages));
 
 	/* attach queued messages we read before */
 	list_splice_init(&messages, &msq->q_messages);
@@ -362,13 +376,14 @@ int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 	msq->q_qnum = h->q_qnum;
 
 	ret = load_ipc_msg_hdr(ctx, h, msq);
-
 	if (ret < 0) {
 		ckpt_debug("msq: need to remove (%d)\n", ret);
-		freeque(ns, perms);
-	} else
-		ipc_unlock(perms);
+		ipc_lock_by_ptr(&msq->q_perm);
+		freeque(ns, ipc);
+		ipc_unlock(ipc);
+	}
 	up_write(&msg_ids->rw_mutex);
+
  out:
 	free_msg_list(&messages);  /* no-op if all ok, else cleanup msgs */
 	ckpt_hdr_put(ctx, h);
diff --git a/ipc/checkpoint_sem.c b/ipc/checkpoint_sem.c
index 395c84d..78c1932 100644
--- a/ipc/checkpoint_sem.c
+++ b/ipc/checkpoint_sem.c
@@ -166,7 +166,7 @@ static struct sem *restore_sem_array(struct ckpt_ctx *ctx, int nsems)
 int restore_ipc_sem(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 {
 	struct ckpt_hdr_ipc_sem *h;
-	struct kern_ipc_perm *perms;
+	struct kern_ipc_perm *ipc;
 	struct sem_array *sem;
 	struct sem *sma = NULL;
 	struct ipc_ids *sem_ids = &ns->ids[IPC_SEM_IDS];
@@ -201,19 +201,34 @@ int restore_ipc_sem(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 
 	down_write(&sem_ids->rw_mutex);
 
-	/* we are the sole owners/users of this ipc_ns, it can't go away */
-	perms = ipc_lock(sem_ids, h->perms.id);
-	BUG_ON(IS_ERR(perms));  /* ipc_ns is private to us */
-
-	sem = container_of(perms, struct sem_array, sem_perm);
+	/*
+	 * We are the sole owners/users of this brand new ipc-ns, so:
+	 *
+	 * 1) The semid could not have been deleted between its creation
+	 *   and taking the rw_mutex above.
+	 * 2) No unauthorized task will attempt to gain access to it,
+	 *   so it is safe to do away with ipc_lock(). This is useful
+	 *   because we can call functions that sleep.
+	 * 3) Likewise, we only restore the security bits further below,
+	 *   so it is safe to ignore this (theoretical only!) race.
+	 *
+	 * If/when we allow to restore the ipc state within the parent's
+	 * ipc-ns, we will need to re-examine this.
+	 */
+
+	ipc = idr_find(&sem_ids->ipcs_idr, h->perms.id);
+	BUG_ON(!ipc);
+
+	sem = container_of(ipc, struct sem_array, sem_perm);
 	memcpy(sem->sem_base, sma, sem->sem_nsems * sizeof(*sma));
 
 	ret = load_ipc_sem_hdr(ctx, h, sem);
 	if (ret < 0) {
+		ipc_lock_by_ptr(&sem->sem_perm);
 		ckpt_debug("sem: need to remove (%d)\n", ret);
-		freeary(ns, perms);
-	} else
-		ipc_unlock(perms);
+		freeary(ns, ipc);
+		ipc_unlock(ipc);
+	}
 	up_write(&sem_ids->rw_mutex);
  out:
 	kfree(sma);
diff --git a/ipc/checkpoint_shm.c b/ipc/checkpoint_shm.c
index 01091d9..6329245 100644
--- a/ipc/checkpoint_shm.c
+++ b/ipc/checkpoint_shm.c
@@ -144,18 +144,27 @@ struct dq_ipcshm_del {
 	int id;
 };
 
-static int ipc_shm_delete(void *data)
+static int _ipc_shm_delete(struct ipc_namespace *ns, int id)
 {
-	struct dq_ipcshm_del *dq = (struct dq_ipcshm_del *) data;
 	mm_segment_t old_fs;
 	int ret;
 
 	old_fs = get_fs();
 	set_fs(get_ds());
-	ret = shmctl_down(dq->ipcns, dq->id, IPC_RMID, NULL, 0);
+	ret = shmctl_down(ns, id, IPC_RMID, NULL, 0);
 	set_fs(old_fs);
 
+	return ret;
+}
+
+static int ipc_shm_delete(void *data)
+{
+	struct dq_ipcshm_del *dq = (struct dq_ipcshm_del *) data;
+	int ret;
+
+	ret = _ipc_shm_delete(dq->ipcns, dq->id);
 	put_ipc_ns(dq->ipcns);
+
 	return ret;
 }
 
@@ -175,7 +184,6 @@ static int load_ipc_shm_hdr(struct ckpt_ctx *ctx,
 	if (h->shm_cprid < 0 || h->shm_lprid < 0)
 		return -EINVAL;
 
-	shp->shm_segsz = h->shm_segsz;
 	shp->shm_atim = h->shm_atim;
 	shp->shm_dtim = h->shm_dtim;
 	shp->shm_ctim = h->shm_ctim;
@@ -188,7 +196,7 @@ static int load_ipc_shm_hdr(struct ckpt_ctx *ctx,
 int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 {
 	struct ckpt_hdr_ipc_shm *h;
-	struct kern_ipc_perm *perms;
+	struct kern_ipc_perm *ipc;
 	struct shmid_kernel *shp;
 	struct ipc_ids *shm_ids = &ns->ids[IPC_SHM_IDS];
 	struct file *file;
@@ -213,6 +221,14 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 	if (h->flags & SHM_HUGETLB)	/* FIXME: support SHM_HUGETLB */
 		goto out;
 
+	shmflag = h->flags | h->perms.mode | IPC_CREAT | IPC_EXCL;
+	ckpt_debug("shm: do_shmget size %lld flag %#x id %d\n",
+		 h->shm_segsz, shmflag, h->perms.id);
+	ret = do_shmget(ns, h->perms.key, h->shm_segsz, shmflag, h->perms.id);
+	ckpt_debug("shm: do_shmget ret %d\n", ret);
+	if (ret < 0)
+		goto out;
+
 	/*
 	 * SHM_DEST means that the shm is to be deleted after creation.
 	 * However, deleting before it's actually attached is quite silly.
@@ -228,29 +244,36 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 		dq.ipcns = ns;
 		get_ipc_ns(dq.ipcns);
 
-		/* XXX can safely use put_ipc_ns() as dtor, see above */
 		ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
 				     (deferqueue_func_t) ipc_shm_delete,
-				     (deferqueue_func_t) put_ipc_ns);
-		if (ret < 0)
+				     (deferqueue_func_t) ipc_shm_delete);
+		if (ret < 0) {
+			ipc_shm_delete((void *) &dq);
 			goto out;
+		}
 	}
 
-	shmflag = h->flags | h->perms.mode | IPC_CREAT | IPC_EXCL;
-	ckpt_debug("shm: do_shmget size %lld flag %#x id %d\n",
-		 h->shm_segsz, shmflag, h->perms.id);
-	ret = do_shmget(ns, h->perms.key, h->shm_segsz, shmflag, h->perms.id);
-	ckpt_debug("shm: do_shmget ret %d\n", ret);
-	if (ret < 0)
-		goto out;
-
 	down_write(&shm_ids->rw_mutex);
 
-	/* we are the sole owners/users of this ipc_ns, it can't go away */
-	perms = ipc_lock(shm_ids, h->perms.id);
-	BUG_ON(IS_ERR(perms));  /* ipc_ns is private to us */
+	/*
+	 * We are the sole owners/users of this brand new ipc-ns, so:
+	 *
+	 * 1) The shmid could not have been deleted between its creation
+	 *   and taking the rw_mutex above.
+	 * 2) No unauthorized task will attempt to gain access to it,
+	 *   so it is safe to do away with ipc_lock(). This is useful
+	 *   because we can call functions that sleep.
+	 * 3) Likewise, we only restore the security bits further below,
+	 *   so it is safe to ignore this (theoretical only!) race.
+	 *
+	 * If/when we allow to restore the ipc state within the parent's
+	 * ipc-ns, we will need to re-examine this.
+	 */
+
+	ipc = idr_find(&shm_ids->ipcs_idr, h->perms.id);
+	BUG_ON(!ipc);
 
-	shp = container_of(perms, struct shmid_kernel, shm_perm);
+	shp = container_of(ipc, struct shmid_kernel, shm_perm);
 	file = shp->shm_file;
 	get_file(file);
 
@@ -265,12 +288,13 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 	ret = restore_memory_contents(ctx, file->f_dentry->d_inode);
  mutex:
 	fput(file);
-	if (ret < 0) {
-		ckpt_debug("shm: need to remove (%d)\n", ret);
-		do_shm_rmid(ns, perms);
-	} else
-		ipc_unlock(perms);
 	up_write(&shm_ids->rw_mutex);
+
+	/* discard this shmid if error and deferqueue wasn't set */
+	if (ret < 0 && !(h->perms.mode & SHM_DEST)) {
+		ckpt_debug("shm: need to remove (%d)\n", ret);
+		_ipc_shm_delete(ns, h->perms.id);
+	}
  out:
 	ckpt_hdr_put(ctx, h);
 	return ret;
-- 
1.6.3.3

Re: [PATCH] c/r: fix "scheduling in atomic" while restoring ipc shm

[Oren Laadan]

Hi Nikita,

Thanks for the report and the analysis. It actually helped to
pinpoint a couple of other minor issues in the code. This patch
should fix all of these.

Oren.


Oren Laadan wrote:
> """
>   While playing with checkpoint-restart code, version
>   several-commits-before-0.19, we have faced "scheduling in atomic" issue.
>   ...
>   So restore_ipc_shm() calls ipc_lock() and then restore_memory_contents().
>   Inside ipc_lock(), a spinlock is taken.
>   Inside restore_memory_contents(), checkpoint data is read, that results
>   in vfs_read() and a schedule somewhere below.
>   ...
>   Another related bug: if load_ipc_shm_hdr() fails in line 257, control
>   is transfered to mutex: label with negative ret value; ipc_unlock()
>   is not called on this path.
> """
> 
> Actually, the error could have occurred with other sleeping functions,
> for example, security_restore_obj().
> 
> The fix is to simply not take ipc_lock(). this relies on the fact that
> IPC is always restored to a brand new ipc namespace which is only
> accessible to the restarting process(es):
> 
> 1) The ipc object (id) could not have been deleted between its creation
>   and taking the rw_mutex.
> 
> 2) No unauthorized task will attempt to gain access to it, so it is
>   safe to do away with ipc_lock(). This is useful because we can call
>   functions that sleep.
> 
> 3) Likewise, we only restore the security bits further below, so it is
>   safe to ignore a security (theoretical only!) race.
> 
> If (and when) we allow to restore the ipc state within the parent's
> namespace, we will need to re-examine this.
> 
> While at it, there is also a cleanup to not restore perm->{id,key,seq}
> or the shm->shm_segsz, as they are all set upon creation properly.
> 
> Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
> Reported-by: "Nikita V. Youshchenko" <yoush-/llMDZXAvAOHXe+LvDLADg@public.gmane.org>
> ---
>  ipc/checkpoint.c     |    9 ++----
>  ipc/checkpoint_msg.c |   35 +++++++++++++++++-------
>  ipc/checkpoint_sem.c |   33 ++++++++++++++++------
>  ipc/checkpoint_shm.c |   74 +++++++++++++++++++++++++++++++++-----------------
>  4 files changed, 101 insertions(+), 50 deletions(-)
> 
> diff --git a/ipc/checkpoint.c b/ipc/checkpoint.c
> index 4322dea..06027c2 100644
> --- a/ipc/checkpoint.c
> +++ b/ipc/checkpoint.c
> @@ -203,9 +203,6 @@ int restore_load_ipc_perms(struct ckpt_ctx *ctx,
>  
>  	/* FIX: verify the ->mode field makes sense */
>  
> -	perm->id = h->id;
> -	perm->key = h->key;
> -
>  	if (!validate_created_perms(h))
>  		return -EPERM;
>  	perm->uid = h->uid;
> @@ -213,10 +210,10 @@ int restore_load_ipc_perms(struct ckpt_ctx *ctx,
>  	perm->cuid = h->cuid;
>  	perm->cgid = h->cgid;
>  	perm->mode = h->mode;
> -	perm->seq = h->seq;
>  
> -	return security_restore_obj(ctx, (void *)perm, CKPT_SECURITY_IPC,
> -				   h->sec_ref);
> +	return security_restore_obj(ctx, (void *)perm,
> +				    CKPT_SECURITY_IPC,
> +				    h->sec_ref);
>  }
>  
>  static int restore_ipc_any(struct ckpt_ctx *ctx, struct ipc_namespace *ipc_ns,
> diff --git a/ipc/checkpoint_msg.c b/ipc/checkpoint_msg.c
> index 61b3d78..073a893 100644
> --- a/ipc/checkpoint_msg.c
> +++ b/ipc/checkpoint_msg.c
> @@ -306,7 +306,7 @@ static int restore_msg_contents(struct ckpt_ctx *ctx, struct list_head *queue,
>  int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  {
>  	struct ckpt_hdr_ipc_msg *h;
> -	struct kern_ipc_perm *perms;
> +	struct kern_ipc_perm *ipc;
>  	struct msg_queue *msq;
>  	struct ipc_ids *msg_ids = &ns->ids[IPC_MSG_IDS];
>  	struct list_head messages;
> @@ -344,12 +344,26 @@ int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  
>  	down_write(&msg_ids->rw_mutex);
>  
> -	/* we are the sole owners/users of this ipc_ns, it can't go away */
> -	perms = ipc_lock(msg_ids, h->perms.id);
> -	BUG_ON(IS_ERR(perms));	/* ipc_ns is private to us */
> +	/*
> +	 * We are the sole owners/users of this brand new ipc-ns, so:
> +	 *
> +	 * 1) The msgid could not have been deleted between its creation
> +	 *   and taking the rw_mutex above.
> +	 * 2) No unauthorized task will attempt to gain access to it,
> +	 *   so it is safe to do away with ipc_lock(). This is useful
> +	 *   because we can call functions that sleep.
> +	 * 3) Likewise, we only restore the security bits further below,
> +	 *   so it is safe to ignore this (theoretical only!) race.
> +	 *
> +	 * If/when we allow to restore the ipc state within the parent's
> +	 * ipc-ns, we will need to re-examine this.
> +	 */
> +
> +	ipc = idr_find(&msg_ids->ipcs_idr, h->perms.id);
> +	BUG_ON(!ipc);
>  
> -	msq = container_of(perms, struct msg_queue, q_perm);
> -	BUG_ON(!list_empty(&msq->q_messages));	/* ipc_ns is private to us */
> +	msq = container_of(ipc, struct msg_queue, q_perm);
> +	BUG_ON(!list_empty(&msq->q_messages));
>  
>  	/* attach queued messages we read before */
>  	list_splice_init(&messages, &msq->q_messages);
> @@ -362,13 +376,14 @@ int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  	msq->q_qnum = h->q_qnum;
>  
>  	ret = load_ipc_msg_hdr(ctx, h, msq);
> -
>  	if (ret < 0) {
>  		ckpt_debug("msq: need to remove (%d)\n", ret);
> -		freeque(ns, perms);
> -	} else
> -		ipc_unlock(perms);
> +		ipc_lock_by_ptr(&msq->q_perm);
> +		freeque(ns, ipc);
> +		ipc_unlock(ipc);
> +	}
>  	up_write(&msg_ids->rw_mutex);
> +
>   out:
>  	free_msg_list(&messages);  /* no-op if all ok, else cleanup msgs */
>  	ckpt_hdr_put(ctx, h);
> diff --git a/ipc/checkpoint_sem.c b/ipc/checkpoint_sem.c
> index 395c84d..78c1932 100644
> --- a/ipc/checkpoint_sem.c
> +++ b/ipc/checkpoint_sem.c
> @@ -166,7 +166,7 @@ static struct sem *restore_sem_array(struct ckpt_ctx *ctx, int nsems)
>  int restore_ipc_sem(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  {
>  	struct ckpt_hdr_ipc_sem *h;
> -	struct kern_ipc_perm *perms;
> +	struct kern_ipc_perm *ipc;
>  	struct sem_array *sem;
>  	struct sem *sma = NULL;
>  	struct ipc_ids *sem_ids = &ns->ids[IPC_SEM_IDS];
> @@ -201,19 +201,34 @@ int restore_ipc_sem(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  
>  	down_write(&sem_ids->rw_mutex);
>  
> -	/* we are the sole owners/users of this ipc_ns, it can't go away */
> -	perms = ipc_lock(sem_ids, h->perms.id);
> -	BUG_ON(IS_ERR(perms));  /* ipc_ns is private to us */
> -
> -	sem = container_of(perms, struct sem_array, sem_perm);
> +	/*
> +	 * We are the sole owners/users of this brand new ipc-ns, so:
> +	 *
> +	 * 1) The semid could not have been deleted between its creation
> +	 *   and taking the rw_mutex above.
> +	 * 2) No unauthorized task will attempt to gain access to it,
> +	 *   so it is safe to do away with ipc_lock(). This is useful
> +	 *   because we can call functions that sleep.
> +	 * 3) Likewise, we only restore the security bits further below,
> +	 *   so it is safe to ignore this (theoretical only!) race.
> +	 *
> +	 * If/when we allow to restore the ipc state within the parent's
> +	 * ipc-ns, we will need to re-examine this.
> +	 */
> +
> +	ipc = idr_find(&sem_ids->ipcs_idr, h->perms.id);
> +	BUG_ON(!ipc);
> +
> +	sem = container_of(ipc, struct sem_array, sem_perm);
>  	memcpy(sem->sem_base, sma, sem->sem_nsems * sizeof(*sma));
>  
>  	ret = load_ipc_sem_hdr(ctx, h, sem);
>  	if (ret < 0) {
> +		ipc_lock_by_ptr(&sem->sem_perm);
>  		ckpt_debug("sem: need to remove (%d)\n", ret);
> -		freeary(ns, perms);
> -	} else
> -		ipc_unlock(perms);
> +		freeary(ns, ipc);
> +		ipc_unlock(ipc);
> +	}
>  	up_write(&sem_ids->rw_mutex);
>   out:
>  	kfree(sma);
> diff --git a/ipc/checkpoint_shm.c b/ipc/checkpoint_shm.c
> index 01091d9..6329245 100644
> --- a/ipc/checkpoint_shm.c
> +++ b/ipc/checkpoint_shm.c
> @@ -144,18 +144,27 @@ struct dq_ipcshm_del {
>  	int id;
>  };
>  
> -static int ipc_shm_delete(void *data)
> +static int _ipc_shm_delete(struct ipc_namespace *ns, int id)
>  {
> -	struct dq_ipcshm_del *dq = (struct dq_ipcshm_del *) data;
>  	mm_segment_t old_fs;
>  	int ret;
>  
>  	old_fs = get_fs();
>  	set_fs(get_ds());
> -	ret = shmctl_down(dq->ipcns, dq->id, IPC_RMID, NULL, 0);
> +	ret = shmctl_down(ns, id, IPC_RMID, NULL, 0);
>  	set_fs(old_fs);
>  
> +	return ret;
> +}
> +
> +static int ipc_shm_delete(void *data)
> +{
> +	struct dq_ipcshm_del *dq = (struct dq_ipcshm_del *) data;
> +	int ret;
> +
> +	ret = _ipc_shm_delete(dq->ipcns, dq->id);
>  	put_ipc_ns(dq->ipcns);
> +
>  	return ret;
>  }
>  
> @@ -175,7 +184,6 @@ static int load_ipc_shm_hdr(struct ckpt_ctx *ctx,
>  	if (h->shm_cprid < 0 || h->shm_lprid < 0)
>  		return -EINVAL;
>  
> -	shp->shm_segsz = h->shm_segsz;
>  	shp->shm_atim = h->shm_atim;
>  	shp->shm_dtim = h->shm_dtim;
>  	shp->shm_ctim = h->shm_ctim;
> @@ -188,7 +196,7 @@ static int load_ipc_shm_hdr(struct ckpt_ctx *ctx,
>  int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  {
>  	struct ckpt_hdr_ipc_shm *h;
> -	struct kern_ipc_perm *perms;
> +	struct kern_ipc_perm *ipc;
>  	struct shmid_kernel *shp;
>  	struct ipc_ids *shm_ids = &ns->ids[IPC_SHM_IDS];
>  	struct file *file;
> @@ -213,6 +221,14 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  	if (h->flags & SHM_HUGETLB)	/* FIXME: support SHM_HUGETLB */
>  		goto out;
>  
> +	shmflag = h->flags | h->perms.mode | IPC_CREAT | IPC_EXCL;
> +	ckpt_debug("shm: do_shmget size %lld flag %#x id %d\n",
> +		 h->shm_segsz, shmflag, h->perms.id);
> +	ret = do_shmget(ns, h->perms.key, h->shm_segsz, shmflag, h->perms.id);
> +	ckpt_debug("shm: do_shmget ret %d\n", ret);
> +	if (ret < 0)
> +		goto out;
> +
>  	/*
>  	 * SHM_DEST means that the shm is to be deleted after creation.
>  	 * However, deleting before it's actually attached is quite silly.
> @@ -228,29 +244,36 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  		dq.ipcns = ns;
>  		get_ipc_ns(dq.ipcns);
>  
> -		/* XXX can safely use put_ipc_ns() as dtor, see above */
>  		ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
>  				     (deferqueue_func_t) ipc_shm_delete,
> -				     (deferqueue_func_t) put_ipc_ns);
> -		if (ret < 0)
> +				     (deferqueue_func_t) ipc_shm_delete);
> +		if (ret < 0) {
> +			ipc_shm_delete((void *) &dq);
>  			goto out;
> +		}
>  	}
>  
> -	shmflag = h->flags | h->perms.mode | IPC_CREAT | IPC_EXCL;
> -	ckpt_debug("shm: do_shmget size %lld flag %#x id %d\n",
> -		 h->shm_segsz, shmflag, h->perms.id);
> -	ret = do_shmget(ns, h->perms.key, h->shm_segsz, shmflag, h->perms.id);
> -	ckpt_debug("shm: do_shmget ret %d\n", ret);
> -	if (ret < 0)
> -		goto out;
> -
>  	down_write(&shm_ids->rw_mutex);
>  
> -	/* we are the sole owners/users of this ipc_ns, it can't go away */
> -	perms = ipc_lock(shm_ids, h->perms.id);
> -	BUG_ON(IS_ERR(perms));  /* ipc_ns is private to us */
> +	/*
> +	 * We are the sole owners/users of this brand new ipc-ns, so:
> +	 *
> +	 * 1) The shmid could not have been deleted between its creation
> +	 *   and taking the rw_mutex above.
> +	 * 2) No unauthorized task will attempt to gain access to it,
> +	 *   so it is safe to do away with ipc_lock(). This is useful
> +	 *   because we can call functions that sleep.
> +	 * 3) Likewise, we only restore the security bits further below,
> +	 *   so it is safe to ignore this (theoretical only!) race.
> +	 *
> +	 * If/when we allow to restore the ipc state within the parent's
> +	 * ipc-ns, we will need to re-examine this.
> +	 */
> +
> +	ipc = idr_find(&shm_ids->ipcs_idr, h->perms.id);
> +	BUG_ON(!ipc);
>  
> -	shp = container_of(perms, struct shmid_kernel, shm_perm);
> +	shp = container_of(ipc, struct shmid_kernel, shm_perm);
>  	file = shp->shm_file;
>  	get_file(file);
>  
> @@ -265,12 +288,13 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  	ret = restore_memory_contents(ctx, file->f_dentry->d_inode);
>   mutex:
>  	fput(file);
> -	if (ret < 0) {
> -		ckpt_debug("shm: need to remove (%d)\n", ret);
> -		do_shm_rmid(ns, perms);
> -	} else
> -		ipc_unlock(perms);
>  	up_write(&shm_ids->rw_mutex);
> +
> +	/* discard this shmid if error and deferqueue wasn't set */
> +	if (ret < 0 && !(h->perms.mode & SHM_DEST)) {
> +		ckpt_debug("shm: need to remove (%d)\n", ret);
> +		_ipc_shm_delete(ns, h->perms.id);
> +	}
>   out:
>  	ckpt_hdr_put(ctx, h);
>  	return ret;

Re: [PATCH] c/r: fix "scheduling in atomic" while restoring ipc shm

[Nikita V. Youshchenko]

> Hi Nikita,
>
> Thanks for the report and the analysis. It actually helped to
> pinpoint a couple of other minor issues in the code. This patch
> should fix all of these.
>
> Oren.

Hi Oren.

With ckpt-v19 plus this patch applied, we still are getting a kernel
crash, with BUG() fired at
+       ipc = idr_find(&msg_ids->ipcs_idr, h->perms.id);
+       BUG_ON(!ipc);
added by the patch.

By looking at the code, I can't understand how this idr_find() can at
all succeed, if the namespace it is looking in was just created and
is empty.

What code adds object in question into this idr?

Any hints?

Nikita

...
[   60.321860] [430:430:c/r:ckpt_read_obj_dispatch:254] type 502 len 120
[   60.322489] [430:430:c/r:ckpt_read_obj:383] type 502 len 120(120,120)
[   60.323140] [430:430:c/r:restore_ipc_shm:226] shm: do_shmget size 790528 flag 0x7a4 id 32769
[   60.324257] [430:430:c/r:restore_ipc_shm:228] shm: do_shmget ret 32769
[   60.325573] ------------[ cut here ]------------
[   60.326059] kernel BUG at ipc/checkpoint_shm.c:274!
[   60.326564] invalid opcode: 0000 [#1] PREEMPT SMP
[   60.327124] last sysfs file:
[   60.327480] Modules linked in:
[   60.327903]
[   60.328104] Pid: 430, comm: bash Not tainted 2.6.33-rc8 #2 /
[   60.328104] EIP: 0060:[<c10e0abe>] EFLAGS: 00000246 CPU: 0
[   60.328104] EIP is at restore_ipc_shm+0x1a0/0x35a
[   60.328104] EAX: 00000000 EBX: 00000000 ECX: 00000005 EDX: c789ba58
[   60.328104] ESI: 00008001 EDI: c793d640 EBP: c79ac000 ESP: c7991dbc
[   60.328104]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
[   60.328104] Process bash (pid: 430, ti=c7990000 task=c7855b70 task.ti=c7990000)
[   60.328104] Stack:
[   60.328104]  c129da9c c79ac000 000c1000 00000000 c129db00 000001ae 000001b4 000c1000
[   60.328104] <0> 00000000 c124dde1 000001ae 00000001 00000000 c7940c00 c79ac000 c10e036d
[   60.328104] <0> 00000002 c129da9c ffffffef c129da9c c799ac60 c79ac000 c10e048a 000001f6
[   60.328104] Call Trace:
[   60.328104]  [<c10e036d>] ? restore_ipc_any+0xa5/0x119
[   60.328104]  [<c10e048a>] ? restore_ipc_ns+0xa9/0x112
[   60.328104]  [<c10e091e>] ? restore_ipc_shm+0x0/0x35a
[   60.328104]  [<c10feb48>] ? restore_obj+0x98/0x116
[   60.328104]  [<c11007ed>] ? ckpt_read_obj_dispatch+0x220/0x246
[   60.328104]  [<c1100829>] ? ckpt_read_obj+0x16/0xe8
[   60.328104]  [<c107b866>] ? fsnotify_access+0x5a/0x61
[   60.328104]  [<c110097d>] ? ckpt_read_obj_type+0x16/0x70
[   60.328104]  [<c1039ab8>] ? restore_ns+0x18/0x12b
[   60.328104]  [<c10feb48>] ? restore_obj+0x98/0x116
[   60.328104]  [<c11007ed>] ? ckpt_read_obj_dispatch+0x220/0x246
[   60.328104]  [<c1100829>] ? ckpt_read_obj+0x16/0xe8
[   60.328104]  [<c110097d>] ? ckpt_read_obj_type+0x16/0x70
[   60.328104]  [<c11033fb>] ? restore_task+0x512/0x9fc
[   60.328104]  [<c1101b59>] ? do_restart+0xff4/0x12f3
[   60.328104]  [<c10364f0>] ? autoremove_wake_function+0x0/0x2d
[   60.328104]  [<c10fdb21>] ? do_sys_restart+0x66/0x77
[   60.328104]  [<c10027d5>] ? ptregs_restart+0x15/0x1c
[   60.328104]  [<c10026d0>] ? sysenter_do_call+0x12/0x26
[   60.328104] Code: fe ff ff e9 c8 01 00 00 8b 04 24 83 c0 64 89 44 24 10 e8 dd 16 10 00 8b 57 10 8b 04 24 83 c0 
74 e8 24 71 02 00 85 c0 89 c3 75 04 <0f> 0b eb fe 8b 68 2c 8d 45 18 3e ff 45 18 8b 44 24 04 8d 57 08
[   60.328104] EIP: [<c10e0abe>] restore_ipc_shm+0x1a0/0x35a SS:ESP 0068:c7991dbc
[   60.351332] ---[ end trace 9660dfa05be59307 ]---

Re: [PATCH] c/r: fix "scheduling in atomic" while restoring ipc shm

[Serge E. Hallyn]

Quoting Nikita V. Youshchenko (yoush-/llMDZXAvAOHXe+LvDLADg@public.gmane.org):
> > Hi Nikita,
> >
> > Thanks for the report and the analysis. It actually helped to
> > pinpoint a couple of other minor issues in the code. This patch
> > should fix all of these.
> >
> > Oren.
> 
> Hi Oren.
> 
> With ckpt-v19 plus this patch applied, we still are getting a kernel
> crash, with BUG() fired at
> +       ipc = idr_find(&msg_ids->ipcs_idr, h->perms.id);
> +       BUG_ON(!ipc);
> added by the patch.
> 
> By looking at the code, I can't understand how this idr_find() can at
> all succeed, if the namespace it is looking in was just created and
> is empty.
> 
> What code adds object in question into this idr?

The do_semget() in ipc/checkpoint_sem.c:restore_ipc_sem() does that.
At the very least Oren's comment was wrong in that perms->seq will
not get restored unless we continue to do it explicitly.  With the
following trivial patch I sometimes get success, but usually not.
Still trying to find what's up...

From 809cc2f1eccc39d45a145e280b6d992e4fa1e683 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Tue, 2 Mar 2010 05:43:21 -0600
Subject: [PATCH 1/1] restore perm->seq

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 ipc/checkpoint.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/ipc/checkpoint.c b/ipc/checkpoint.c
index 06027c2..f865471 100644
--- a/ipc/checkpoint.c
+++ b/ipc/checkpoint.c
@@ -210,6 +210,7 @@ int restore_load_ipc_perms(struct ckpt_ctx *ctx,
 	perm->cuid = h->cuid;
 	perm->cgid = h->cgid;
 	perm->mode = h->mode;
+	perm->seq = h->seq;
 
 	return security_restore_obj(ctx, (void *)perm,
 				    CKPT_SECURITY_IPC,
-- 
1.6.0.6


-serge

Re: [PATCH] c/r: fix "scheduling in atomic" while restoring ipc shm

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Nikita V. Youshchenko (yoush-/llMDZXAvAOHXe+LvDLADg@public.gmane.org):
>>> Hi Nikita,
>>>
>>> Thanks for the report and the analysis. It actually helped to
>>> pinpoint a couple of other minor issues in the code. This patch
>>> should fix all of these.
>>>
>>> Oren.
>> Hi Oren.
>>
>> With ckpt-v19 plus this patch applied, we still are getting a kernel
>> crash, with BUG() fired at
>> +       ipc = idr_find(&msg_ids->ipcs_idr, h->perms.id);
>> +       BUG_ON(!ipc);
>> added by the patch.
>>
>> By looking at the code, I can't understand how this idr_find() can at
>> all succeed, if the namespace it is looking in was just created and
>> is empty.
>>
>> What code adds object in question into this idr?
> 
> The do_semget() in ipc/checkpoint_sem.c:restore_ipc_sem() does that.
> At the very least Oren's comment was wrong in that perms->seq will
> not get restored unless we continue to do it explicitly.  With the
> following trivial patch I sometimes get success, but usually not.
> Still trying to find what's up...

Actually, perms->seq (ipc->seq) is set correctly when the object
is allocated in ipc_addid(). It was redundant to explicitly restore
it in restore_load_ipc_perms().

Oren.

> 
> From 809cc2f1eccc39d45a145e280b6d992e4fa1e683 Mon Sep 17 00:00:00 2001
> From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 2 Mar 2010 05:43:21 -0600
> Subject: [PATCH 1/1] restore perm->seq
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  ipc/checkpoint.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/ipc/checkpoint.c b/ipc/checkpoint.c
> index 06027c2..f865471 100644
> --- a/ipc/checkpoint.c
> +++ b/ipc/checkpoint.c
> @@ -210,6 +210,7 @@ int restore_load_ipc_perms(struct ckpt_ctx *ctx,
>  	perm->cuid = h->cuid;
>  	perm->cgid = h->cgid;
>  	perm->mode = h->mode;
> +	perm->seq = h->seq;
>  
>  	return security_restore_obj(ctx, (void *)perm,
>  				    CKPT_SECURITY_IPC,

Re: [PATCH] c/r: fix "scheduling in atomic" while restoring ipc shm

[Oren Laadan]

Nikita V. Youshchenko wrote:
>> Hi Nikita,
>>
>> Thanks for the report and the analysis. It actually helped to
>> pinpoint a couple of other minor issues in the code. This patch
>> should fix all of these.
>>
>> Oren.
> 
> Hi Oren.
> 
> With ckpt-v19 plus this patch applied, we still are getting a kernel
> crash, with BUG() fired at
> +       ipc = idr_find(&msg_ids->ipcs_idr, h->perms.id);
> +       BUG_ON(!ipc);
> added by the patch.
> 
> By looking at the code, I can't understand how this idr_find() can at
> all succeed, if the namespace it is looking in was just created and
> is empty.
> 
> What code adds object in question into this idr?

As Serge pointed out, the call to do_msgget(), if succeeded, should
have created the object, and if it didn't succeed then we would have
returned with an error message.

You can see in your log, that we request id 32769 (h->prems.id) and
that is what do_shmget() returned. So I'm quite confused...

Can you post your test program so I can try to reproduce it here ?
Also, can you add a debug output before and after the call to idr_find
that prints the h->perms.id ?

Thanks,

Oren.


> 
> Any hints?
> 


> Nikita
> 
> ...
> [   60.321860] [430:430:c/r:ckpt_read_obj_dispatch:254] type 502 len 120
> [   60.322489] [430:430:c/r:ckpt_read_obj:383] type 502 len 120(120,120)
> [   60.323140] [430:430:c/r:restore_ipc_shm:226] shm: do_shmget size 790528 flag 0x7a4 id 32769
> [   60.324257] [430:430:c/r:restore_ipc_shm:228] shm: do_shmget ret 32769
> [   60.325573] ------------[ cut here ]------------
> [   60.326059] kernel BUG at ipc/checkpoint_shm.c:274!
> [   60.326564] invalid opcode: 0000 [#1] PREEMPT SMP
> [   60.327124] last sysfs file:
> [   60.327480] Modules linked in:
> [   60.327903]
> [   60.328104] Pid: 430, comm: bash Not tainted 2.6.33-rc8 #2 /
> [   60.328104] EIP: 0060:[<c10e0abe>] EFLAGS: 00000246 CPU: 0
> [   60.328104] EIP is at restore_ipc_shm+0x1a0/0x35a
> [   60.328104] EAX: 00000000 EBX: 00000000 ECX: 00000005 EDX: c789ba58
> [   60.328104] ESI: 00008001 EDI: c793d640 EBP: c79ac000 ESP: c7991dbc
> [   60.328104]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> [   60.328104] Process bash (pid: 430, ti=c7990000 task=c7855b70 task.ti=c7990000)
> [   60.328104] Stack:
> [   60.328104]  c129da9c c79ac000 000c1000 00000000 c129db00 000001ae 000001b4 000c1000
> [   60.328104] <0> 00000000 c124dde1 000001ae 00000001 00000000 c7940c00 c79ac000 c10e036d
> [   60.328104] <0> 00000002 c129da9c ffffffef c129da9c c799ac60 c79ac000 c10e048a 000001f6
> [   60.328104] Call Trace:
> [   60.328104]  [<c10e036d>] ? restore_ipc_any+0xa5/0x119
> [   60.328104]  [<c10e048a>] ? restore_ipc_ns+0xa9/0x112
> [   60.328104]  [<c10e091e>] ? restore_ipc_shm+0x0/0x35a
> [   60.328104]  [<c10feb48>] ? restore_obj+0x98/0x116
> [   60.328104]  [<c11007ed>] ? ckpt_read_obj_dispatch+0x220/0x246
> [   60.328104]  [<c1100829>] ? ckpt_read_obj+0x16/0xe8
> [   60.328104]  [<c107b866>] ? fsnotify_access+0x5a/0x61
> [   60.328104]  [<c110097d>] ? ckpt_read_obj_type+0x16/0x70
> [   60.328104]  [<c1039ab8>] ? restore_ns+0x18/0x12b
> [   60.328104]  [<c10feb48>] ? restore_obj+0x98/0x116
> [   60.328104]  [<c11007ed>] ? ckpt_read_obj_dispatch+0x220/0x246
> [   60.328104]  [<c1100829>] ? ckpt_read_obj+0x16/0xe8
> [   60.328104]  [<c110097d>] ? ckpt_read_obj_type+0x16/0x70
> [   60.328104]  [<c11033fb>] ? restore_task+0x512/0x9fc
> [   60.328104]  [<c1101b59>] ? do_restart+0xff4/0x12f3
> [   60.328104]  [<c10364f0>] ? autoremove_wake_function+0x0/0x2d
> [   60.328104]  [<c10fdb21>] ? do_sys_restart+0x66/0x77
> [   60.328104]  [<c10027d5>] ? ptregs_restart+0x15/0x1c
> [   60.328104]  [<c10026d0>] ? sysenter_do_call+0x12/0x26
> [   60.328104] Code: fe ff ff e9 c8 01 00 00 8b 04 24 83 c0 64 89 44 24 10 e8 dd 16 10 00 8b 57 10 8b 04 24 83 c0 
> 74 e8 24 71 02 00 85 c0 89 c3 75 04 <0f> 0b eb fe 8b 68 2c 8d 45 18 3e ff 45 18 8b 44 24 04 8d 57 08
> [   60.328104] EIP: [<c10e0abe>] restore_ipc_shm+0x1a0/0x35a SS:ESP 0068:c7991dbc
> [   60.351332] ---[ end trace 9660dfa05be59307 ]---
> 
> 

Re: [PATCH] c/r: fix "scheduling in atomic" while restoring ipc shm

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> 
> 
> Nikita V. Youshchenko wrote:
> >> Hi Nikita,
> >>
> >> Thanks for the report and the analysis. It actually helped to
> >> pinpoint a couple of other minor issues in the code. This patch
> >> should fix all of these.
> >>
> >> Oren.
> > 
> > Hi Oren.
> > 
> > With ckpt-v19 plus this patch applied, we still are getting a kernel
> > crash, with BUG() fired at
> > +       ipc = idr_find(&msg_ids->ipcs_idr, h->perms.id);
> > +       BUG_ON(!ipc);
> > added by the patch.
> > 
> > By looking at the code, I can't understand how this idr_find() can at
> > all succeed, if the namespace it is looking in was just created and
> > is empty.
> > 
> > What code adds object in question into this idr?
> 
> As Serge pointed out, the call to do_msgget(), if succeeded, should
> have created the object, and if it didn't succeed then we would have
> returned with an error message.

Should have, but didn't :)  I get the same BUG_ON.

> You can see in your log, that we request id 32769 (h->prems.id) and
> that is what do_shmget() returned. So I'm quite confused...
> 
> Can you post your test program so I can try to reproduce it here ?

You can just

	cd cr_tests/ipc; sh test-sem.sh

to reliably reproduce.

> Also, can you add a debug output before and after the call to idr_find
> that prints the h->perms.id ?
> 
> Thanks,
> 
> Oren.
> 
> 
> > 
> > Any hints?
> > 
> 
> 
> > Nikita
> > 
> > ...
> > [   60.321860] [430:430:c/r:ckpt_read_obj_dispatch:254] type 502 len 120
> > [   60.322489] [430:430:c/r:ckpt_read_obj:383] type 502 len 120(120,120)
> > [   60.323140] [430:430:c/r:restore_ipc_shm:226] shm: do_shmget size 790528 flag 0x7a4 id 32769
> > [   60.324257] [430:430:c/r:restore_ipc_shm:228] shm: do_shmget ret 32769
> > [   60.325573] ------------[ cut here ]------------
> > [   60.326059] kernel BUG at ipc/checkpoint_shm.c:274!
> > [   60.326564] invalid opcode: 0000 [#1] PREEMPT SMP
> > [   60.327124] last sysfs file:
> > [   60.327480] Modules linked in:
> > [   60.327903]
> > [   60.328104] Pid: 430, comm: bash Not tainted 2.6.33-rc8 #2 /
> > [   60.328104] EIP: 0060:[<c10e0abe>] EFLAGS: 00000246 CPU: 0
> > [   60.328104] EIP is at restore_ipc_shm+0x1a0/0x35a
> > [   60.328104] EAX: 00000000 EBX: 00000000 ECX: 00000005 EDX: c789ba58
> > [   60.328104] ESI: 00008001 EDI: c793d640 EBP: c79ac000 ESP: c7991dbc
> > [   60.328104]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> > [   60.328104] Process bash (pid: 430, ti=c7990000 task=c7855b70 task.ti=c7990000)
> > [   60.328104] Stack:
> > [   60.328104]  c129da9c c79ac000 000c1000 00000000 c129db00 000001ae 000001b4 000c1000
> > [   60.328104] <0> 00000000 c124dde1 000001ae 00000001 00000000 c7940c00 c79ac000 c10e036d
> > [   60.328104] <0> 00000002 c129da9c ffffffef c129da9c c799ac60 c79ac000 c10e048a 000001f6
> > [   60.328104] Call Trace:
> > [   60.328104]  [<c10e036d>] ? restore_ipc_any+0xa5/0x119
> > [   60.328104]  [<c10e048a>] ? restore_ipc_ns+0xa9/0x112
> > [   60.328104]  [<c10e091e>] ? restore_ipc_shm+0x0/0x35a
> > [   60.328104]  [<c10feb48>] ? restore_obj+0x98/0x116
> > [   60.328104]  [<c11007ed>] ? ckpt_read_obj_dispatch+0x220/0x246
> > [   60.328104]  [<c1100829>] ? ckpt_read_obj+0x16/0xe8
> > [   60.328104]  [<c107b866>] ? fsnotify_access+0x5a/0x61
> > [   60.328104]  [<c110097d>] ? ckpt_read_obj_type+0x16/0x70
> > [   60.328104]  [<c1039ab8>] ? restore_ns+0x18/0x12b
> > [   60.328104]  [<c10feb48>] ? restore_obj+0x98/0x116
> > [   60.328104]  [<c11007ed>] ? ckpt_read_obj_dispatch+0x220/0x246
> > [   60.328104]  [<c1100829>] ? ckpt_read_obj+0x16/0xe8
> > [   60.328104]  [<c110097d>] ? ckpt_read_obj_type+0x16/0x70
> > [   60.328104]  [<c11033fb>] ? restore_task+0x512/0x9fc
> > [   60.328104]  [<c1101b59>] ? do_restart+0xff4/0x12f3
> > [   60.328104]  [<c10364f0>] ? autoremove_wake_function+0x0/0x2d
> > [   60.328104]  [<c10fdb21>] ? do_sys_restart+0x66/0x77
> > [   60.328104]  [<c10027d5>] ? ptregs_restart+0x15/0x1c
> > [   60.328104]  [<c10026d0>] ? sysenter_do_call+0x12/0x26
> > [   60.328104] Code: fe ff ff e9 c8 01 00 00 8b 04 24 83 c0 64 89 44 24 10 e8 dd 16 10 00 8b 57 10 8b 04 24 83 c0 
> > 74 e8 24 71 02 00 85 c0 89 c3 75 04 <0f> 0b eb fe 8b 68 2c 8d 45 18 3e ff 45 18 8b 44 24 04 8d 57 08
> > [   60.328104] EIP: [<c10e0abe>] restore_ipc_shm+0x1a0/0x35a SS:ESP 0068:c7991dbc
> > [   60.351332] ---[ end trace 9660dfa05be59307 ]---
> > 
> > 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH] c/r: fix "scheduling in atomic" while restoring ipc shm

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> > 
> > 
> > Nikita V. Youshchenko wrote:
> > >> Hi Nikita,
> > >>
> > >> Thanks for the report and the analysis. It actually helped to
> > >> pinpoint a couple of other minor issues in the code. This patch
> > >> should fix all of these.
> > >>
> > >> Oren.
> > > 
> > > Hi Oren.
> > > 
> > > With ckpt-v19 plus this patch applied, we still are getting a kernel
> > > crash, with BUG() fired at
> > > +       ipc = idr_find(&msg_ids->ipcs_idr, h->perms.id);
> > > +       BUG_ON(!ipc);
> > > added by the patch.
> > > 
> > > By looking at the code, I can't understand how this idr_find() can at
> > > all succeed, if the namespace it is looking in was just created and
> > > is empty.
> > > 
> > > What code adds object in question into this idr?
> > 
> > As Serge pointed out, the call to do_msgget(), if succeeded, should
> > have created the object, and if it didn't succeed then we would have
> > returned with an error message.
> 
> Should have, but didn't :)  I get the same BUG_ON.
> 
> > You can see in your log, that we request id 32769 (h->prems.id) and
> > that is what do_shmget() returned. So I'm quite confused...
> > 
> > Can you post your test program so I can try to reproduce it here ?
> 
> You can just
> 
> 	cd cr_tests/ipc; sh test-sem.sh
> 
> to reliably reproduce.
> 
> > Also, can you add a debug output before and after the call to idr_find
> > that prints the h->perms.id ?

[root@oracer4b linux-2.6]# git diff
diff --git a/ipc/checkpoint.c b/ipc/checkpoint.c
index f865471..1c53581 100644
--- a/ipc/checkpoint.c
+++ b/ipc/checkpoint.c
@@ -210,7 +210,11 @@ int restore_load_ipc_perms(struct ckpt_ctx *ctx,
perm->cuid = h->cuid;
perm->cgid = h->cgid;
perm->mode = h->mode;
-       perm->seq = h->seq;
+       if (perm->seq != h->seq) {
+               ckpt_err(ctx, -EINVAL, "bad kern_ipc_perm->seq (%d not %d)\n",
+                       perm->mode, h->mode);
+               return -EINVAL;
+       }

return security_restore_obj(ctx, (void *)perm,
CKPT_SECURITY_IPC,
diff --git a/ipc/checkpoint_sem.c b/ipc/checkpoint_sem.c
index 78c1932..c4012c9 100644
--- a/ipc/checkpoint_sem.c
+++ b/ipc/checkpoint_sem.c
@@ -216,7 +216,10 @@ int restore_ipc_sem(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
* ipc-ns, we will need to re-examine this.
*/

+       printk(KERN_NOTICE "XXX h->perms.id before is %lx\n", h->perms.id);
ipc = idr_find(&sem_ids->ipcs_idr, h->perms.id);
+       printk(KERN_NOTICE "XXX h->perms.id after is %lx\n", h->perms.id);
+       printk(KERN_NOTICE "XXX and i got back %lx\n", ipc);
BUG_ON(!ipc);

sem = container_of(ipc, struct sem_array, sem_perm);

[root@oracer4b linux-2.6]# dmesg|grep XXX
XXX h->perms.id before is 0
XXX h->perms.id after is 0
XXX and i got back ffff88007e51b0d0
XXX h->perms.id before is 8001
XXX h->perms.id after is 8001
XXX and i got back 0

[root@oracer4b linux-2.6]# dmesg|grep sem
[2410:2410:c/r:checkpoint_ipc_any:76] ipc-sem count 2
[2410:2410:c/r:fill_ipc_sem_hdr:50] sem: nsems 1
[2410:2410:c/r:fill_ipc_sem_hdr:50] sem: nsems 1
[2410:2410:c/r:checkpoint_ipc_any:84] ipc-sem ret 0
[2410:2410:c/r:checkpoint_file_common:188] file create-sem credref 11 secref 0
[2417:2406:c/r:restore_ipc_any:236] ipc-sem: count 2
[2417:2406:c/r:restore_ipc_sem:196] sem: do_semget key 0 flag 0x780 id 0
[2417:2406:c/r:restore_ipc_sem:198] sem: do_semget ret 0
[2417:2406:c/r:load_ipc_sem_hdr:120] sem: nsems 1
[2417:2406:c/r:restore_ipc_sem:196] sem: do_semget key 180146447 flag 0x780 id 32769
[2417:2406:c/r:restore_ipc_sem:198] sem: do_semget ret 32769
kernel BUG at ipc/checkpoint_sem.c:223!


-serge

[PATCH] c/r: fix ipc scheduling while atomic - take 3

[Oren Laadan]

This patch applies to the current head of ckpt-v19-dev.

While the previous fix was correct, it was incomplete in the sense that
a similar problem exists during checkpoint. So here is a better attempt
at fixing both.

The main idea is that holding the {shm,msg,sem}ids->rw_mutex is enough
at checkpoint when calling checkpoint_fill_ipc_perms() because the data
accessed is either immutable or protected against change with the mutex.
For restart, the same argument as before works - we are the sole users
of a new ipc-ns, and no unaothorized accessed is possible (still, in
this version the code is a bit cleaner).

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


---
 ipc/checkpoint.c     |   26 ++++++++++++++++++++++++++
 ipc/checkpoint_msg.c |   29 +++++++++++++++--------------
 ipc/checkpoint_sem.c |   18 ++++++++++--------
 ipc/checkpoint_shm.c |   18 +++++++++++-------
 4 files changed, 62 insertions(+), 29 deletions(-)

diff --git a/ipc/checkpoint.c b/ipc/checkpoint.c
index 06027c2..ca181ae 100644
--- a/ipc/checkpoint.c
+++ b/ipc/checkpoint.c
@@ -33,6 +33,19 @@ static char *ipc_ind_to_str[] = { "sem", "msg", "shm" };
  * Checkpoint
  */
 
+/*
+ * Requires that ids->rw_mutex be held; this is sufficient because:
+ *
+ * (a) The data accessed either may not change at all (e.g. id, key,
+ * sqe), or may only change by ipc_update_perm() (e.g. uid, cuid, gid,
+ * cgid, mode), which is only called with the mutex write-held.
+ *
+ * (b) The function ipcperms() relies solely on the latter (uid, vuid,
+ * gid, cgid, mode)
+ *
+ * (c) The security context perm->security also may only change when the
+ * mutex is taken.
+ */
 int checkpoint_fill_ipc_perms(struct ckpt_ctx *ctx,
 			      struct ckpt_hdr_ipc_perms *h,
 			      struct kern_ipc_perm *perm)
@@ -48,12 +61,14 @@ int checkpoint_fill_ipc_perms(struct ckpt_ctx *ctx,
 	h->cgid = perm->cgid;
 	h->mode = perm->mode & S_IRWXUGO;
 	h->seq = perm->seq;
+
 	h->sec_ref = security_checkpoint_obj(ctx, perm->security,
 					     CKPT_SECURITY_IPC);
 	if (h->sec_ref < 0) {
 		ckpt_err(ctx, h->sec_ref, "%(T)ipc_perm->security\n");
 		return h->sec_ref;
 	}
+
 	return 0;
 }
 
@@ -184,6 +199,17 @@ static int validate_created_perms(struct ckpt_hdr_ipc_perms *h)
 	return 1;
 }
 
+/*
+ * Requires that ids->rw_mutex be held; this is sufficient because:
+ *
+ * (a) The data accessed either may only change by ipc_update_perm()
+ * or by security hooks (perm->security), all of which are only called
+ * with the mutex write-held.
+ *
+ * (b) During restart, we are guarantted to be using a brand new
+ * ipc-ns, only accessible to us, so there will be no attempt for
+ * access validation while we restore the state (by other tasks).
+ */
 int restore_load_ipc_perms(struct ckpt_ctx *ctx,
 			   struct ckpt_hdr_ipc_perms *h,
 			   struct kern_ipc_perm *perm)
diff --git a/ipc/checkpoint_msg.c b/ipc/checkpoint_msg.c
index 1933121..7b9a984 100644
--- a/ipc/checkpoint_msg.c
+++ b/ipc/checkpoint_msg.c
@@ -29,18 +29,18 @@
  * ipc checkpoint
  */
 
+/* called with the msgids->rw_mutex is read-held */
 static int fill_ipc_msg_hdr(struct ckpt_ctx *ctx,
 			    struct ckpt_hdr_ipc_msg *h,
 			    struct msg_queue *msq)
 {
-	int ret = 0;
-
-	ipc_lock_by_ptr(&msq->q_perm);
+	int ret;
 
 	ret = checkpoint_fill_ipc_perms(ctx, &h->perms, &msq->q_perm);
 	if (ret < 0)
-		goto unlock;
+		return ret;
 
+	ipc_lock_by_ptr(&msq->q_perm);
 	h->q_stime = msq->q_stime;
 	h->q_rtime = msq->q_rtime;
 	h->q_ctime = msq->q_ctime;
@@ -49,13 +49,12 @@ static int fill_ipc_msg_hdr(struct ckpt_ctx *ctx,
 	h->q_qbytes = msq->q_qbytes;
 	h->q_lspid = msq->q_lspid;
 	h->q_lrpid = msq->q_lrpid;
-
- unlock:
 	ipc_unlock(&msq->q_perm);
+
 	ckpt_debug("msg: lspid %d rspid %d qnum %lld qbytes %lld\n",
 		 h->q_lspid, h->q_lrpid, h->q_qnum, h->q_qbytes);
 
-	return ret;
+	return 0;
 }
 
 static int checkpoint_msg_contents(struct ckpt_ctx *ctx, struct msg_msg *msg)
@@ -144,6 +143,7 @@ static int checkpoint_msg_queue(struct ckpt_ctx *ctx, struct msg_queue *msq)
 	return ret;
 }
 
+/* called with the msgids->rw_mutex is read-held */
 int checkpoint_ipc_msg(int id, void *p, void *data)
 {
 	struct ckpt_hdr_ipc_msg *h;
@@ -178,6 +178,7 @@ int checkpoint_ipc_msg(int id, void *p, void *data)
  * ipc restart
  */
 
+/* called with the msgids->rw_mutex is write-held */
 static int load_ipc_msg_hdr(struct ckpt_ctx *ctx,
 			    struct ckpt_hdr_ipc_msg *h,
 			    struct msg_queue *msq)
@@ -349,19 +350,16 @@ int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 	 *
 	 * 1) The msgid could not have been deleted between its creation
 	 *   and taking the rw_mutex above.
-	 * 2) No unauthorized task will attempt to gain access to it,
-	 *   so it is safe to do away with ipc_lock(). This is useful
-	 *   because we can call functions that sleep.
-	 * 3) Likewise, we only restore the security bits further below,
-	 *   so it is safe to ignore this (theoretical only!) race.
+	 *
+	 * 2) No unauthorized task will have attempted to gain access
+	 *   to it either, not even until we restore the security bit
+	 *   further below, so the theoretical security race is void.
 	 *
 	 * If/when we allow to restore the ipc state within the parent's
 	 * ipc-ns, we will need to re-examine this.
 	 */
-
 	ipc = ipc_lock(msg_ids, h->perms.id);
 	BUG_ON(IS_ERR(ipc));
-	ipc_unlock(ipc);
 
 	msq = container_of(ipc, struct msg_queue, q_perm);
 	BUG_ON(!list_empty(&msq->q_messages));
@@ -376,6 +374,9 @@ int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 	msq->q_qbytes = h->q_qbytes;
 	msq->q_qnum = h->q_qnum;
 
+	/* this is safe because no unauthorized access is possible */
+	ipc_unlock(ipc);
+
 	ret = load_ipc_msg_hdr(ctx, h, msq);
 	if (ret < 0) {
 		ckpt_debug("msq: need to remove (%d)\n", ret);
diff --git a/ipc/checkpoint_sem.c b/ipc/checkpoint_sem.c
index ac28592..890374d 100644
--- a/ipc/checkpoint_sem.c
+++ b/ipc/checkpoint_sem.c
@@ -29,27 +29,26 @@ struct msg_msg;
  * ipc checkpoint
  */
 
+/* called with the msgids->rw_mutex is read-held */
 static int fill_ipc_sem_hdr(struct ckpt_ctx *ctx,
 			       struct ckpt_hdr_ipc_sem *h,
 			       struct sem_array *sem)
 {
 	int ret = 0;
 
-	ipc_lock_by_ptr(&sem->sem_perm);
-
 	ret = checkpoint_fill_ipc_perms(ctx, &h->perms, &sem->sem_perm);
 	if (ret < 0)
-		goto unlock;
+		return ret;
 
+	ipc_lock_by_ptr(&sem->sem_perm);
 	h->sem_otime = sem->sem_otime;
 	h->sem_ctime = sem->sem_ctime;
 	h->sem_nsems = sem->sem_nsems;
-
- unlock:
 	ipc_unlock(&sem->sem_perm);
+
 	ckpt_debug("sem: nsems %u\n", h->sem_nsems);
 
-	return ret;
+	return 0;
 }
 
 /**
@@ -74,6 +73,7 @@ static int checkpoint_sem_array(struct ckpt_ctx *ctx, struct sem_array *sem)
 			       sem->sem_nsems * sizeof(*sem->sem_base));
 }
 
+/* called with the msgids->rw_mutex is read-held */
 int checkpoint_ipc_sem(int id, void *p, void *data)
 {
 	struct ckpt_hdr_ipc_sem *h;
@@ -107,6 +107,7 @@ int checkpoint_ipc_sem(int id, void *p, void *data)
  * ipc restart
  */
 
+/* called with the msgids->rw_mutex is write-held */
 static int load_ipc_sem_hdr(struct ckpt_ctx *ctx,
 			       struct ckpt_hdr_ipc_sem *h,
 			       struct sem_array *sem)
@@ -215,14 +216,15 @@ int restore_ipc_sem(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 	 * If/when we allow to restore the ipc state within the parent's
 	 * ipc-ns, we will need to re-examine this.
 	 */
-
 	ipc = ipc_lock(sem_ids, h->perms.id);
 	BUG_ON(IS_ERR(ipc));
-	ipc_unlock(ipc);
 
 	sem = container_of(ipc, struct sem_array, sem_perm);
 	memcpy(sem->sem_base, sma, sem->sem_nsems * sizeof(*sma));
 
+	/* this is safe because no unauthorized access is possible */
+	ipc_unlock(ipc);
+
 	ret = load_ipc_sem_hdr(ctx, h, sem);
 	if (ret < 0) {
 		ipc_lock_by_ptr(&sem->sem_perm);
diff --git a/ipc/checkpoint_shm.c b/ipc/checkpoint_shm.c
index 62eacaf..bfba5dc 100644
--- a/ipc/checkpoint_shm.c
+++ b/ipc/checkpoint_shm.c
@@ -33,17 +33,18 @@
  * ipc checkpoint
  */
 
+/* called with the msgids->rw_mutex is read-held */
 static int fill_ipc_shm_hdr(struct ckpt_ctx *ctx,
 			    struct ckpt_hdr_ipc_shm *h,
 			    struct shmid_kernel *shp)
 {
 	int ret = 0;
 
-	ipc_lock_by_ptr(&shp->shm_perm);
-
 	ret = checkpoint_fill_ipc_perms(ctx, &h->perms, &shp->shm_perm);
 	if (ret < 0)
-		goto unlock;
+		return ret;
+
+	ipc_lock_by_ptr(&shp->shm_perm);
 
 	h->shm_segsz = shp->shm_segsz;
 	h->shm_atim = shp->shm_atim;
@@ -67,14 +68,15 @@ static int fill_ipc_shm_hdr(struct ckpt_ctx *ctx,
 		ret = -ENOSYS;
 	}
 
- unlock:
 	ipc_unlock(&shp->shm_perm);
+
 	ckpt_debug("shm: cprid %d lprid %d segsz %lld mlock %d\n",
 		 h->shm_cprid, h->shm_lprid, h->shm_segsz, h->mlock_uid);
 
 	return ret;
 }
 
+/* called with the msgids->rw_mutex is read-held */
 int checkpoint_ipc_shm(int id, void *p, void *data)
 {
 	struct ckpt_hdr_ipc_shm *h;
@@ -168,6 +170,7 @@ static int ipc_shm_delete(void *data)
 	return ret;
 }
 
+/* called with the msgids->rw_mutex is write-held */
 static int load_ipc_shm_hdr(struct ckpt_ctx *ctx,
 			    struct ckpt_hdr_ipc_shm *h,
 			    struct shmid_kernel *shp)
@@ -242,7 +245,7 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 
 		dq.id = h->perms.id;
 		dq.ipcns = ns;
-		get_ipc_ns(dq.ipcns);
+		get_ipc_ns(ns);
 
 		ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
 				     (deferqueue_func_t) ipc_shm_delete,
@@ -269,15 +272,16 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
 	 * If/when we allow to restore the ipc state within the parent's
 	 * ipc-ns, we will need to re-examine this.
 	 */
-
 	ipc = ipc_lock(shm_ids, h->perms.id);
 	BUG_ON(IS_ERR(ipc));
-	ipc_unlock(ipc);
 
 	shp = container_of(ipc, struct shmid_kernel, shm_perm);
 	file = shp->shm_file;
 	get_file(file);
 
+	/* this is safe because no unauthorized access is possible */
+	ipc_unlock(ipc);
+
 	ret = load_ipc_shm_hdr(ctx, h, shp);
 	if (ret < 0)
 		goto mutex;
-- 
1.6.3.3

Re: [PATCH] c/r: fix ipc scheduling while atomic - take 3

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> This patch applies to the current head of ckpt-v19-dev.
> 
> While the previous fix was correct, it was incomplete in the sense that
> a similar problem exists during checkpoint. So here is a better attempt
> at fixing both.
> 
> The main idea is that holding the {shm,msg,sem}ids->rw_mutex is enough
> at checkpoint when calling checkpoint_fill_ipc_perms() because the data
> accessed is either immutable or protected against change with the mutex.
> For restart, the same argument as before works - we are the sole users
> of a new ipc-ns, and no unaothorized accessed is possible (still, in
> this version the code is a bit cleaner).
> 
> Signed-off-by: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>

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

On my fedora 10 x86-64 partition with selinux enabled, all tests
(except futex - huh?) including the selinux tests pass.

> 
> 
> ---
>  ipc/checkpoint.c     |   26 ++++++++++++++++++++++++++
>  ipc/checkpoint_msg.c |   29 +++++++++++++++--------------
>  ipc/checkpoint_sem.c |   18 ++++++++++--------
>  ipc/checkpoint_shm.c |   18 +++++++++++-------
>  4 files changed, 62 insertions(+), 29 deletions(-)
> 
> diff --git a/ipc/checkpoint.c b/ipc/checkpoint.c
> index 06027c2..ca181ae 100644
> --- a/ipc/checkpoint.c
> +++ b/ipc/checkpoint.c
> @@ -33,6 +33,19 @@ static char *ipc_ind_to_str[] = { "sem", "msg", "shm" };
>   * Checkpoint
>   */
> 
> +/*
> + * Requires that ids->rw_mutex be held; this is sufficient because:
> + *
> + * (a) The data accessed either may not change at all (e.g. id, key,
> + * sqe), or may only change by ipc_update_perm() (e.g. uid, cuid, gid,
> + * cgid, mode), which is only called with the mutex write-held.
> + *
> + * (b) The function ipcperms() relies solely on the latter (uid, vuid,
> + * gid, cgid, mode)
> + *
> + * (c) The security context perm->security also may only change when the
> + * mutex is taken.
> + */
>  int checkpoint_fill_ipc_perms(struct ckpt_ctx *ctx,
>  			      struct ckpt_hdr_ipc_perms *h,
>  			      struct kern_ipc_perm *perm)
> @@ -48,12 +61,14 @@ int checkpoint_fill_ipc_perms(struct ckpt_ctx *ctx,
>  	h->cgid = perm->cgid;
>  	h->mode = perm->mode & S_IRWXUGO;
>  	h->seq = perm->seq;
> +
>  	h->sec_ref = security_checkpoint_obj(ctx, perm->security,
>  					     CKPT_SECURITY_IPC);
>  	if (h->sec_ref < 0) {
>  		ckpt_err(ctx, h->sec_ref, "%(T)ipc_perm->security\n");
>  		return h->sec_ref;
>  	}
> +
>  	return 0;
>  }
> 
> @@ -184,6 +199,17 @@ static int validate_created_perms(struct ckpt_hdr_ipc_perms *h)
>  	return 1;
>  }
> 
> +/*
> + * Requires that ids->rw_mutex be held; this is sufficient because:
> + *
> + * (a) The data accessed either may only change by ipc_update_perm()
> + * or by security hooks (perm->security), all of which are only called
> + * with the mutex write-held.
> + *
> + * (b) During restart, we are guarantted to be using a brand new
> + * ipc-ns, only accessible to us, so there will be no attempt for
> + * access validation while we restore the state (by other tasks).
> + */
>  int restore_load_ipc_perms(struct ckpt_ctx *ctx,
>  			   struct ckpt_hdr_ipc_perms *h,
>  			   struct kern_ipc_perm *perm)
> diff --git a/ipc/checkpoint_msg.c b/ipc/checkpoint_msg.c
> index 1933121..7b9a984 100644
> --- a/ipc/checkpoint_msg.c
> +++ b/ipc/checkpoint_msg.c
> @@ -29,18 +29,18 @@
>   * ipc checkpoint
>   */
> 
> +/* called with the msgids->rw_mutex is read-held */
>  static int fill_ipc_msg_hdr(struct ckpt_ctx *ctx,
>  			    struct ckpt_hdr_ipc_msg *h,
>  			    struct msg_queue *msq)
>  {
> -	int ret = 0;
> -
> -	ipc_lock_by_ptr(&msq->q_perm);
> +	int ret;
> 
>  	ret = checkpoint_fill_ipc_perms(ctx, &h->perms, &msq->q_perm);
>  	if (ret < 0)
> -		goto unlock;
> +		return ret;
> 
> +	ipc_lock_by_ptr(&msq->q_perm);
>  	h->q_stime = msq->q_stime;
>  	h->q_rtime = msq->q_rtime;
>  	h->q_ctime = msq->q_ctime;
> @@ -49,13 +49,12 @@ static int fill_ipc_msg_hdr(struct ckpt_ctx *ctx,
>  	h->q_qbytes = msq->q_qbytes;
>  	h->q_lspid = msq->q_lspid;
>  	h->q_lrpid = msq->q_lrpid;
> -
> - unlock:
>  	ipc_unlock(&msq->q_perm);
> +
>  	ckpt_debug("msg: lspid %d rspid %d qnum %lld qbytes %lld\n",
>  		 h->q_lspid, h->q_lrpid, h->q_qnum, h->q_qbytes);
> 
> -	return ret;
> +	return 0;
>  }
> 
>  static int checkpoint_msg_contents(struct ckpt_ctx *ctx, struct msg_msg *msg)
> @@ -144,6 +143,7 @@ static int checkpoint_msg_queue(struct ckpt_ctx *ctx, struct msg_queue *msq)
>  	return ret;
>  }
> 
> +/* called with the msgids->rw_mutex is read-held */
>  int checkpoint_ipc_msg(int id, void *p, void *data)
>  {
>  	struct ckpt_hdr_ipc_msg *h;
> @@ -178,6 +178,7 @@ int checkpoint_ipc_msg(int id, void *p, void *data)
>   * ipc restart
>   */
> 
> +/* called with the msgids->rw_mutex is write-held */
>  static int load_ipc_msg_hdr(struct ckpt_ctx *ctx,
>  			    struct ckpt_hdr_ipc_msg *h,
>  			    struct msg_queue *msq)
> @@ -349,19 +350,16 @@ int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  	 *
>  	 * 1) The msgid could not have been deleted between its creation
>  	 *   and taking the rw_mutex above.
> -	 * 2) No unauthorized task will attempt to gain access to it,
> -	 *   so it is safe to do away with ipc_lock(). This is useful
> -	 *   because we can call functions that sleep.
> -	 * 3) Likewise, we only restore the security bits further below,
> -	 *   so it is safe to ignore this (theoretical only!) race.
> +	 *
> +	 * 2) No unauthorized task will have attempted to gain access
> +	 *   to it either, not even until we restore the security bit
> +	 *   further below, so the theoretical security race is void.
>  	 *
>  	 * If/when we allow to restore the ipc state within the parent's
>  	 * ipc-ns, we will need to re-examine this.
>  	 */
> -
>  	ipc = ipc_lock(msg_ids, h->perms.id);
>  	BUG_ON(IS_ERR(ipc));
> -	ipc_unlock(ipc);
> 
>  	msq = container_of(ipc, struct msg_queue, q_perm);
>  	BUG_ON(!list_empty(&msq->q_messages));
> @@ -376,6 +374,9 @@ int restore_ipc_msg(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  	msq->q_qbytes = h->q_qbytes;
>  	msq->q_qnum = h->q_qnum;
> 
> +	/* this is safe because no unauthorized access is possible */
> +	ipc_unlock(ipc);
> +
>  	ret = load_ipc_msg_hdr(ctx, h, msq);
>  	if (ret < 0) {
>  		ckpt_debug("msq: need to remove (%d)\n", ret);
> diff --git a/ipc/checkpoint_sem.c b/ipc/checkpoint_sem.c
> index ac28592..890374d 100644
> --- a/ipc/checkpoint_sem.c
> +++ b/ipc/checkpoint_sem.c
> @@ -29,27 +29,26 @@ struct msg_msg;
>   * ipc checkpoint
>   */
> 
> +/* called with the msgids->rw_mutex is read-held */
>  static int fill_ipc_sem_hdr(struct ckpt_ctx *ctx,
>  			       struct ckpt_hdr_ipc_sem *h,
>  			       struct sem_array *sem)
>  {
>  	int ret = 0;
> 
> -	ipc_lock_by_ptr(&sem->sem_perm);
> -
>  	ret = checkpoint_fill_ipc_perms(ctx, &h->perms, &sem->sem_perm);
>  	if (ret < 0)
> -		goto unlock;
> +		return ret;
> 
> +	ipc_lock_by_ptr(&sem->sem_perm);
>  	h->sem_otime = sem->sem_otime;
>  	h->sem_ctime = sem->sem_ctime;
>  	h->sem_nsems = sem->sem_nsems;
> -
> - unlock:
>  	ipc_unlock(&sem->sem_perm);
> +
>  	ckpt_debug("sem: nsems %u\n", h->sem_nsems);
> 
> -	return ret;
> +	return 0;
>  }
> 
>  /**
> @@ -74,6 +73,7 @@ static int checkpoint_sem_array(struct ckpt_ctx *ctx, struct sem_array *sem)
>  			       sem->sem_nsems * sizeof(*sem->sem_base));
>  }
> 
> +/* called with the msgids->rw_mutex is read-held */
>  int checkpoint_ipc_sem(int id, void *p, void *data)
>  {
>  	struct ckpt_hdr_ipc_sem *h;
> @@ -107,6 +107,7 @@ int checkpoint_ipc_sem(int id, void *p, void *data)
>   * ipc restart
>   */
> 
> +/* called with the msgids->rw_mutex is write-held */
>  static int load_ipc_sem_hdr(struct ckpt_ctx *ctx,
>  			       struct ckpt_hdr_ipc_sem *h,
>  			       struct sem_array *sem)
> @@ -215,14 +216,15 @@ int restore_ipc_sem(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  	 * If/when we allow to restore the ipc state within the parent's
>  	 * ipc-ns, we will need to re-examine this.
>  	 */
> -
>  	ipc = ipc_lock(sem_ids, h->perms.id);
>  	BUG_ON(IS_ERR(ipc));
> -	ipc_unlock(ipc);
> 
>  	sem = container_of(ipc, struct sem_array, sem_perm);
>  	memcpy(sem->sem_base, sma, sem->sem_nsems * sizeof(*sma));
> 
> +	/* this is safe because no unauthorized access is possible */
> +	ipc_unlock(ipc);
> +
>  	ret = load_ipc_sem_hdr(ctx, h, sem);
>  	if (ret < 0) {
>  		ipc_lock_by_ptr(&sem->sem_perm);
> diff --git a/ipc/checkpoint_shm.c b/ipc/checkpoint_shm.c
> index 62eacaf..bfba5dc 100644
> --- a/ipc/checkpoint_shm.c
> +++ b/ipc/checkpoint_shm.c
> @@ -33,17 +33,18 @@
>   * ipc checkpoint
>   */
> 
> +/* called with the msgids->rw_mutex is read-held */
>  static int fill_ipc_shm_hdr(struct ckpt_ctx *ctx,
>  			    struct ckpt_hdr_ipc_shm *h,
>  			    struct shmid_kernel *shp)
>  {
>  	int ret = 0;
> 
> -	ipc_lock_by_ptr(&shp->shm_perm);
> -
>  	ret = checkpoint_fill_ipc_perms(ctx, &h->perms, &shp->shm_perm);
>  	if (ret < 0)
> -		goto unlock;
> +		return ret;
> +
> +	ipc_lock_by_ptr(&shp->shm_perm);
> 
>  	h->shm_segsz = shp->shm_segsz;
>  	h->shm_atim = shp->shm_atim;
> @@ -67,14 +68,15 @@ static int fill_ipc_shm_hdr(struct ckpt_ctx *ctx,
>  		ret = -ENOSYS;
>  	}
> 
> - unlock:
>  	ipc_unlock(&shp->shm_perm);
> +
>  	ckpt_debug("shm: cprid %d lprid %d segsz %lld mlock %d\n",
>  		 h->shm_cprid, h->shm_lprid, h->shm_segsz, h->mlock_uid);
> 
>  	return ret;
>  }
> 
> +/* called with the msgids->rw_mutex is read-held */
>  int checkpoint_ipc_shm(int id, void *p, void *data)
>  {
>  	struct ckpt_hdr_ipc_shm *h;
> @@ -168,6 +170,7 @@ static int ipc_shm_delete(void *data)
>  	return ret;
>  }
> 
> +/* called with the msgids->rw_mutex is write-held */
>  static int load_ipc_shm_hdr(struct ckpt_ctx *ctx,
>  			    struct ckpt_hdr_ipc_shm *h,
>  			    struct shmid_kernel *shp)
> @@ -242,7 +245,7 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
> 
>  		dq.id = h->perms.id;
>  		dq.ipcns = ns;
> -		get_ipc_ns(dq.ipcns);
> +		get_ipc_ns(ns);
> 
>  		ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
>  				     (deferqueue_func_t) ipc_shm_delete,
> @@ -269,15 +272,16 @@ int restore_ipc_shm(struct ckpt_ctx *ctx, struct ipc_namespace *ns)
>  	 * If/when we allow to restore the ipc state within the parent's
>  	 * ipc-ns, we will need to re-examine this.
>  	 */
> -
>  	ipc = ipc_lock(shm_ids, h->perms.id);
>  	BUG_ON(IS_ERR(ipc));
> -	ipc_unlock(ipc);
> 
>  	shp = container_of(ipc, struct shmid_kernel, shm_perm);
>  	file = shp->shm_file;
>  	get_file(file);
> 
> +	/* this is safe because no unauthorized access is possible */
> +	ipc_unlock(ipc);
> +
>  	ret = load_ipc_shm_hdr(ctx, h, shp);
>  	if (ret < 0)
>  		goto mutex;
> -- 
> 1.6.3.3