Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650739 > unrolled thread

[PATCH 0/20 V3] Misc cleanups for ipc

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/20 V3] Misc cleanups for ipc Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 05/20] ipc/sem: Do not use ipc_rcu_free() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 08/20] ipc/util: Drop ipc_rcu_free() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 15/20] ipc/msg.c: Avoid ipc_rcu_putref for failed ipc_addid() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 19/20] ipc/sem: Drop __sem_free() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 20/20] ipc/util.h: Update documentation for ipc_getref() and ipc_putref() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 09/20] ipc/sem: Avoid ipc_rcu_alloc() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 17/20] ipc/shm: Remove special shm_alloc/free Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 11/20] ipc/msg: Avoid ipc_rcu_alloc() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 12/20] ipc/util: Drop ipc_rcu_alloc() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 16/20] ipc: Move atomic_set() to where it is needed Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 04/20] ipc: Drop non-RCU allocation Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
      Re: [PATCH 04/20] ipc: Drop non-RCU allocation Kees Cook <keescook@chromium.org> - 2017-05-25 21:40 +0200
    [PATCH 03/20] include/linux/sem.h: Correctly document sem_ctime Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 07/20] ipc/msg: Do not use ipc_rcu_free() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    [PATCH 10/20] ipc/shm: Avoid ipc_rcu_alloc() Manfred Spraul <manfred@colorfullife.com> - 2017-05-25 21:00 +0200
    Re: [PATCH 0/20 V3] Misc cleanups for ipc Kees Cook <keescook@chromium.org> - 2017-05-25 21:50 +0200
      Re: [PATCH 0/20 V3] Misc cleanups for ipc Manfred Spraul <manfred@colorfullife.com> - 2017-05-26 04:00 +0200

#1650739 — [PATCH 0/20 V3] Misc cleanups for ipc

Hi all,

Updated series. The series got longer, because I merged all patches
from Kees.

Main changes are:
- sems[] instead of sem[0].
- Immediately use BUILD_BUG_ON()
- Immediately move the memset() to avoid crashing with SEM_UNDO.
- Use rcu for every security_xx_free(), even if ipc_addid() was not 
  successful

@Andrew: Could you add them again to your tree?

@Michael: 
Should we update man semctl(2)?
Several years ago, I did a review and found that sem_ctime is only
for Coherent the time of the last change...

http://calculix-rpm.sourceforge.net/sysvsem.html

--
        Manfred

[toc] | [next] | [standalone]

#1650740 — [PATCH 05/20] ipc/sem: Do not use ipc_rcu_free()

From: Kees Cook <keescook@chromium.org>

Avoid using ipc_rcu_free, since it just re-finds the original structure
pointer. For the pre-list-init failure path, there is no RCU needed,
since it was just allocated. It can be directly freed.

Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/sem.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 484ccf8..a04c4d6 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -258,13 +258,18 @@ static void merge_queues(struct sem_array *sma)
 	}
 }
 
+static void __sem_free(struct sem_array *sma)
+{
+	kvfree(sma);
+}
+
 static void sem_rcu_free(struct rcu_head *head)
 {
 	struct kern_ipc_perm *p = container_of(head, struct kern_ipc_perm, rcu);
 	struct sem_array *sma = container_of(p, struct sem_array, sem_perm);
 
 	security_sem_free(sma);
-	ipc_rcu_free(head);
+	__sem_free(sma);
 }
 
 /*
@@ -482,7 +487,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
 	sma->sem_perm.security = NULL;
 	retval = security_sem_alloc(sma);
 	if (retval) {
-		ipc_rcu_putref(&sma->sem_perm, ipc_rcu_free);
+		__sem_free(sma);
 		return retval;
 	}
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650741 — [PATCH 08/20] ipc/util: Drop ipc_rcu_free()

From: Kees Cook <keescook@chromium.org>

There are no more callers of ipc_rcu_free(), so remove it.

Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/util.c | 7 -------
 ipc/util.h | 1 -
 2 files changed, 8 deletions(-)

diff --git a/ipc/util.c b/ipc/util.c
index dd73feb..556884b 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -429,13 +429,6 @@ void ipc_rcu_putref(struct kern_ipc_perm *ptr,
 	call_rcu(&ptr->rcu, func);
 }
 
-void ipc_rcu_free(struct rcu_head *h)
-{
-	struct kern_ipc_perm *ptr = container_of(h, struct kern_ipc_perm, rcu);
-
-	kvfree(ptr);
-}
-
 /**
  * ipcperms - check ipc permissions
  * @ns: ipc namespace
diff --git a/ipc/util.h b/ipc/util.h
index 2578fd9..44efbc0 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -119,7 +119,6 @@ struct kern_ipc_perm *ipc_rcu_alloc(int size);
 int ipc_rcu_getref(struct kern_ipc_perm *ptr);
 void ipc_rcu_putref(struct kern_ipc_perm *ptr,
 			void (*func)(struct rcu_head *head));
-void ipc_rcu_free(struct rcu_head *h);
 
 struct kern_ipc_perm *ipc_lock(struct ipc_ids *, int);
 struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650742 — [PATCH 15/20] ipc/msg.c: Avoid ipc_rcu_putref for failed ipc_addid()

Loosely based on a patch from Kees Cook <keescook@chromium.org>:
- id and retval can be merged
- if ipc_addid() fails, then use call_rcu() directly.

The difference is that call_rcu is used for failed ipc_addid() calls,
to continue to guaranteed an rcu delay for security_msg_queue_free().

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: Kees Cook <keescook@chromium.org>
---
 ipc/msg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 10094a7..cd90bfd 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -132,7 +132,7 @@ static struct msg_queue *msg_alloc(void)
 static int newque(struct ipc_namespace *ns, struct ipc_params *params)
 {
 	struct msg_queue *msq;
-	int id, retval;
+	int retval;
 	key_t key = params->key;
 	int msgflg = params->flg;
 
@@ -160,10 +160,10 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params)
 	INIT_LIST_HEAD(&msq->q_senders);
 
 	/* ipc_addid() locks msq upon success. */
-	id = ipc_addid(&msg_ids(ns), &msq->q_perm, ns->msg_ctlmni);
-	if (id < 0) {
-		ipc_rcu_putref(&msq->q_perm, msg_rcu_free);
-		return id;
+	retval = ipc_addid(&msg_ids(ns), &msq->q_perm, ns->msg_ctlmni);
+	if (retval < 0) {
+		call_rcu(&msq->q_perm.rcu, msg_rcu_free);
+		return retval;
 	}
 
 	ipc_unlock_object(&msq->q_perm);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650743 — [PATCH 19/20] ipc/sem: Drop __sem_free()

From: Kees Cook <keescook@chromium.org>

The remaining users of __sem_free() can simply call kvfree() instead for
better readability.

Signed-off-by: Kees Cook <keescook@chromium.org>

[manfred@colorfullife.com: Rediff to keep rcu protection for
security_sem_alloc()]
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/sem.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 5f13773..9e70cd7 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -258,18 +258,13 @@ static void merge_queues(struct sem_array *sma)
 	}
 }
 
-static void __sem_free(struct sem_array *sma)
-{
-	kvfree(sma);
-}
-
 static void sem_rcu_free(struct rcu_head *head)
 {
 	struct kern_ipc_perm *p = container_of(head, struct kern_ipc_perm, rcu);
 	struct sem_array *sma = container_of(p, struct sem_array, sem_perm);
 
 	security_sem_free(sma);
-	__sem_free(sma);
+	kvfree(sma);
 }
 
 /*
@@ -500,7 +495,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
 	sma->sem_perm.security = NULL;
 	retval = security_sem_alloc(sma);
 	if (retval) {
-		__sem_free(sma);
+		kvfree(sma);
 		return retval;
 	}
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650744 — [PATCH 20/20] ipc/util.h: Update documentation for ipc_getref() and ipc_putref()

Now that ipc_rcu_alloc() and ipc_rcu_free() are removed,
document when it is valid to use ipc_getref() and ipc_putref().

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/util.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ipc/util.h b/ipc/util.h
index 77336c2b..c692010 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -112,6 +112,9 @@ int ipcperms(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp, short flg);
  * Objects are reference counted, they start with reference count 1.
  * getref increases the refcount, the putref call that reduces the recount
  * to 0 schedules the rcu destruction. Caller must guarantee locking.
+ *
+ * refcount is initialized by ipc_addid(), before that point call_rcu()
+ * must be used.
  */
 int ipc_rcu_getref(struct kern_ipc_perm *ptr);
 void ipc_rcu_putref(struct kern_ipc_perm *ptr,
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650745 — [PATCH 09/20] ipc/sem: Avoid ipc_rcu_alloc()

From: Kees Cook <keescook@chromium.org>

Instead of using ipc_rcu_alloc() which only performs the refcount
bump, open code it to perform better sem-specific checks. This
also allows for sem_array structure layout to be randomized in the
future.

Signed-off-by: Kees Cook <keescook@chromium.org>

[manfred@colorfullife.com: Rediff, because the memset was
temporarily inside ipc_rcu_alloc()]

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/sem.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index a04c4d6..445a5b5 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -451,6 +451,25 @@ static inline void sem_rmid(struct ipc_namespace *ns, struct sem_array *s)
 	ipc_rmid(&sem_ids(ns), &s->sem_perm);
 }
 
+static struct sem_array *sem_alloc(size_t nsems)
+{
+	struct sem_array *sma;
+	size_t size;
+
+	if (nsems > (INT_MAX - sizeof(*sma)) / sizeof(sma->sems[0]))
+		return NULL;
+
+	size = sizeof(*sma) + nsems * sizeof(sma->sems[0]);
+	sma = kvmalloc(size, GFP_KERNEL);
+	if (unlikely(!sma))
+		return NULL;
+
+	memset(sma, 0, size);
+	atomic_set(&sma->sem_perm.refcount, 1);
+
+	return sma;
+}
+
 /**
  * newary - Create a new semaphore set
  * @ns: namespace
@@ -463,7 +482,6 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
 	int id;
 	int retval;
 	struct sem_array *sma;
-	int size;
 	key_t key = params->key;
 	int nsems = params->u.nsems;
 	int semflg = params->flg;
@@ -474,10 +492,7 @@ static int newary(struct ipc_namespace *ns, struct ipc_params *params)
 	if (ns->used_sems + nsems > ns->sc_semmns)
 		return -ENOSPC;
 
-	BUILD_BUG_ON(offsetof(struct sem_array, sem_perm) != 0);
-
-	size = sizeof(*sma) + nsems * sizeof(sma->sems[0]);
-	sma = container_of(ipc_rcu_alloc(size), struct sem_array, sem_perm);
+	sma = sem_alloc(nsems);
 	if (!sma)
 		return -ENOMEM;
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650746 — [PATCH 17/20] ipc/shm: Remove special shm_alloc/free

From: Kees Cook <keescook@chromium.org>

There is nothing special about the shm_alloc/free routines any more,
so remove them to make code more readable.

Signed-off-by: Kees Cook <keescook@chromium.org>

[manfred@colorfullife.com: Rediff, to continue to keep rcu for
free calls after a successful security_shm_alloc()]
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/shm.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index b85db5a..ec5688e 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -172,11 +172,6 @@ static inline void shm_lock_by_ptr(struct shmid_kernel *ipcp)
 	ipc_lock_object(&ipcp->shm_perm);
 }
 
-static void __shm_free(struct shmid_kernel *shp)
-{
-	kvfree(shp);
-}
-
 static void shm_rcu_free(struct rcu_head *head)
 {
 	struct kern_ipc_perm *ptr = container_of(head, struct kern_ipc_perm,
@@ -184,7 +179,7 @@ static void shm_rcu_free(struct rcu_head *head)
 	struct shmid_kernel *shp = container_of(ptr, struct shmid_kernel,
 							shm_perm);
 	security_shm_free(shp);
-	__shm_free(shp);
+	kvfree(shp);
 }
 
 static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s)
@@ -518,17 +513,6 @@ static const struct vm_operations_struct shm_vm_ops = {
 #endif
 };
 
-static struct shmid_kernel *shm_alloc(void)
-{
-	struct shmid_kernel *shp;
-
-	shp = kvmalloc(sizeof(*shp), GFP_KERNEL);
-	if (unlikely(!shp))
-		return NULL;
-
-	return shp;
-}
-
 /**
  * newseg - Create a new shared memory segment
  * @ns: namespace
@@ -558,8 +542,8 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 			ns->shm_tot + numpages > ns->shm_ctlall)
 		return -ENOSPC;
 
-	shp = shm_alloc();
-	if (!shp)
+	shp = kvmalloc(sizeof(*shp), GFP_KERNEL);
+	if (unlikely(!shp))
 		return -ENOMEM;
 
 	shp->shm_perm.key = key;
@@ -569,7 +553,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 	shp->shm_perm.security = NULL;
 	error = security_shm_alloc(shp);
 	if (error) {
-		__shm_free(shp);
+		kvfree(shp);
 		return error;
 	}
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650747 — [PATCH 11/20] ipc/msg: Avoid ipc_rcu_alloc()

From: Kees Cook <keescook@chromium.org>

Instead of using ipc_rcu_alloc() which only performs the refcount
bump, open code it. This also allows for msg_queue structure
layout to be randomized in the future.

Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/msg.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 25d43e2..10094a7 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -109,6 +109,19 @@ static void msg_rcu_free(struct rcu_head *head)
 	__msg_free(msq);
 }
 
+static struct msg_queue *msg_alloc(void)
+{
+	struct msg_queue *msq;
+
+	msq = kvmalloc(sizeof(*msq), GFP_KERNEL);
+	if (unlikely(!msq))
+		return NULL;
+
+	atomic_set(&msq->q_perm.refcount, 1);
+
+	return msq;
+}
+
 /**
  * newque - Create a new msg queue
  * @ns: namespace
@@ -123,10 +136,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params)
 	key_t key = params->key;
 	int msgflg = params->flg;
 
-	BUILD_BUG_ON(offsetof(struct msg_queue, q_perm) != 0);
-
-	msq = container_of(ipc_rcu_alloc(sizeof(*msq)), struct msg_queue,
-				q_perm);
+	msq = msg_alloc();
 	if (!msq)
 		return -ENOMEM;
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650750 — [PATCH 12/20] ipc/util: Drop ipc_rcu_alloc()

From: Kees Cook <keescook@chromium.org>

No callers remain for ipc_rcu_alloc(). Drop the function.

Signed-off-by: Kees Cook <keescook@chromium.org>

[manfred@colorfullife.com: Rediff because the memset was
temporarily inside ipc_rcu_free()]

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/util.c | 21 ---------------------
 ipc/util.h |  3 ---
 2 files changed, 24 deletions(-)

diff --git a/ipc/util.c b/ipc/util.c
index 556884b..2428dd4 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -394,27 +394,6 @@ void ipc_rmid(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
 	ipcp->deleted = true;
 }
 
-/**
- * ipc_rcu_alloc - allocate ipc space
- * @size: size desired
- *
- * Allocate memory for an ipc object.
- * The first member must be struct kern_ipc_perm.
- */
-struct kern_ipc_perm *ipc_rcu_alloc(int size)
-{
-	/*
-	 * We prepend the allocation with the rcu struct
-	 */
-	struct kern_ipc_perm *out = kvmalloc(size, GFP_KERNEL);
-	if (unlikely(!out))
-		return NULL;
-
-	memset(out, 0, size);
-	atomic_set(&out->refcount, 1);
-	return out;
-}
-
 int ipc_rcu_getref(struct kern_ipc_perm *ptr)
 {
 	return atomic_inc_not_zero(&ptr->refcount);
diff --git a/ipc/util.h b/ipc/util.h
index 44efbc0..77336c2b 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -112,10 +112,7 @@ int ipcperms(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp, short flg);
  * Objects are reference counted, they start with reference count 1.
  * getref increases the refcount, the putref call that reduces the recount
  * to 0 schedules the rcu destruction. Caller must guarantee locking.
- *
- * struct kern_ipc_perm must be the first member in the allocated structure.
  */
-struct kern_ipc_perm *ipc_rcu_alloc(int size);
 int ipc_rcu_getref(struct kern_ipc_perm *ptr);
 void ipc_rcu_putref(struct kern_ipc_perm *ptr,
 			void (*func)(struct rcu_head *head));
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650751 — [PATCH 16/20] ipc: Move atomic_set() to where it is needed

From: Kees Cook <keescook@chromium.org>

Only after ipc_addid() has succeeded will refcounting be used, so
move initialization into ipc_addid() and remove from open-coded
*_alloc() routines.

Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/msg.c  | 2 --
 ipc/sem.c  | 1 -
 ipc/shm.c  | 2 --
 ipc/util.c | 1 +
 4 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index cd90bfd..770342e 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -117,8 +117,6 @@ static struct msg_queue *msg_alloc(void)
 	if (unlikely(!msq))
 		return NULL;
 
-	atomic_set(&msq->q_perm.refcount, 1);
-
 	return msq;
 }
 
diff --git a/ipc/sem.c b/ipc/sem.c
index 2b2ed56..5f13773 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -465,7 +465,6 @@ static struct sem_array *sem_alloc(size_t nsems)
 		return NULL;
 
 	memset(sma, 0, size);
-	atomic_set(&sma->sem_perm.refcount, 1);
 
 	return sma;
 }
diff --git a/ipc/shm.c b/ipc/shm.c
index cb1d97e..b85db5a 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -526,8 +526,6 @@ static struct shmid_kernel *shm_alloc(void)
 	if (unlikely(!shp))
 		return NULL;
 
-	atomic_set(&shp->shm_perm.refcount, 1);
-
 	return shp;
 }
 
diff --git a/ipc/util.c b/ipc/util.c
index 2428dd4..1a2cb02 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -232,6 +232,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int size)
 
 	idr_preload(GFP_KERNEL);
 
+	atomic_set(&new->refcount, 1);
 	spin_lock_init(&new->lock);
 	new->deleted = false;
 	rcu_read_lock();
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650752 — [PATCH 04/20] ipc: Drop non-RCU allocation

The only users of ipc_alloc() were ipc_rcu_alloc() and the on-heap
sem_io fall-back memory. Better to just open-code these to make things
easier to read.

Signed-off-by: Kees Cook <keescook@chromium.org>

[manfred@colorfullife.com: Rediff due to inclusion of memset() into
ipc_rcu_alloc().]

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/sem.c  |  8 +++++---
 ipc/util.c | 27 +++------------------------
 ipc/util.h |  6 ------
 3 files changed, 8 insertions(+), 33 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index bdff6d9..484ccf8 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1368,7 +1368,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 			}
 			sem_unlock(sma, -1);
 			rcu_read_unlock();
-			sem_io = ipc_alloc(sizeof(ushort)*nsems);
+			sem_io = kvmalloc_array(nsems, sizeof(ushort),
+						GFP_KERNEL);
 			if (sem_io == NULL) {
 				ipc_rcu_putref(&sma->sem_perm, sem_rcu_free);
 				return -ENOMEM;
@@ -1402,7 +1403,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 		rcu_read_unlock();
 
 		if (nsems > SEMMSL_FAST) {
-			sem_io = ipc_alloc(sizeof(ushort)*nsems);
+			sem_io = kvmalloc_array(nsems, sizeof(ushort),
+						GFP_KERNEL);
 			if (sem_io == NULL) {
 				ipc_rcu_putref(&sma->sem_perm, sem_rcu_free);
 				return -ENOMEM;
@@ -1480,7 +1482,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 	wake_up_q(&wake_q);
 out_free:
 	if (sem_io != fast_sem_io)
-		ipc_free(sem_io);
+		kvfree(sem_io);
 	return err;
 }
 
diff --git a/ipc/util.c b/ipc/util.c
index 9dcc08b..dd73feb 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -395,29 +395,6 @@ void ipc_rmid(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
 }
 
 /**
- * ipc_alloc -	allocate ipc space
- * @size: size desired
- *
- * Allocate memory from the appropriate pools and return a pointer to it.
- * NULL is returned if the allocation fails
- */
-void *ipc_alloc(int size)
-{
-	return kvmalloc(size, GFP_KERNEL);
-}
-
-/**
- * ipc_free - free ipc space
- * @ptr: pointer returned by ipc_alloc
- *
- * Free a block created with ipc_alloc().
- */
-void ipc_free(void *ptr)
-{
-	kvfree(ptr);
-}
-
-/**
  * ipc_rcu_alloc - allocate ipc space
  * @size: size desired
  *
@@ -429,9 +406,11 @@ struct kern_ipc_perm *ipc_rcu_alloc(int size)
 	/*
 	 * We prepend the allocation with the rcu struct
 	 */
-	struct kern_ipc_perm *out = ipc_alloc(size);
+	struct kern_ipc_perm *out = kvmalloc(size, GFP_KERNEL);
 	if (unlikely(!out))
 		return NULL;
+
+	memset(out, 0, size);
 	atomic_set(&out->refcount, 1);
 	return out;
 }
diff --git a/ipc/util.h b/ipc/util.h
index 09d0f91..2578fd9 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -107,12 +107,6 @@ void ipc_rmid(struct ipc_ids *, struct kern_ipc_perm *);
 /* must be called with ipcp locked */
 int ipcperms(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp, short flg);
 
-/* for rare, potentially huge allocations.
- * both function can sleep
- */
-void *ipc_alloc(int size);
-void ipc_free(void *ptr);
-
 /*
  * For allocation that need to be freed by RCU.
  * Objects are reference counted, they start with reference count 1.
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650773 — Re: [PATCH 04/20] ipc: Drop non-RCU allocation

On Thu, May 25, 2017 at 11:50 AM, Manfred Spraul
<manfred@colorfullife.com> wrote:
> The only users of ipc_alloc() were ipc_rcu_alloc() and the on-heap
> sem_io fall-back memory. Better to just open-code these to make things
> easier to read.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> [manfred@colorfullife.com: Rediff due to inclusion of memset() into
> ipc_rcu_alloc().]

Oh! I see, the hunk snuck into this patch, rather that patch 3.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1650753 — [PATCH 03/20] include/linux/sem.h: Correctly document sem_ctime

sem_ctime is initialized to the semget() time and then updated at every
semctl() that changes the array.
Thus it does not represent the time of the last change.

Especially, semop() calls are only stored in sem_otime, not in sem_ctime.

This is already described in ipc/sem.c, I just overlooked that there is
a comment in include/linux/sem.h and man semctl(2) as well.

So: Correct wrong comments.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
Cc: linux-man@vger.kernel.org
---
 include/linux/sem.h      | 2 +-
 include/uapi/linux/sem.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/sem.h b/include/linux/sem.h
index 9db1409..be5cf2e 100644
--- a/include/linux/sem.h
+++ b/include/linux/sem.h
@@ -30,7 +30,7 @@ struct sem {
 /* One sem_array data structure for each set of semaphores in the system. */
 struct sem_array {
 	struct kern_ipc_perm	sem_perm;	/* permissions .. see ipc.h */
-	time_t			sem_ctime;	/* last change time */
+	time_t			sem_ctime;	/* create/last semctl() time */
 	struct list_head	pending_alter;	/* pending operations */
 						/* that alter the array */
 	struct list_head	pending_const;	/* pending complex operations */
diff --git a/include/uapi/linux/sem.h b/include/uapi/linux/sem.h
index dd73b90..67eb903 100644
--- a/include/uapi/linux/sem.h
+++ b/include/uapi/linux/sem.h
@@ -23,7 +23,7 @@
 struct semid_ds {
 	struct ipc_perm	sem_perm;		/* permissions .. see ipc.h */
 	__kernel_time_t	sem_otime;		/* last semop time */
-	__kernel_time_t	sem_ctime;		/* last change time */
+	__kernel_time_t	sem_ctime;		/* create/last semctl() time */
 	struct sem	*sem_base;		/* ptr to first semaphore in array */
 	struct sem_queue *sem_pending;		/* pending operations to be processed */
 	struct sem_queue **sem_pending_last;	/* last pending operation */
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650754 — [PATCH 07/20] ipc/msg: Do not use ipc_rcu_free()

From: Kees Cook <keescook@chromium.org>

Avoid using ipc_rcu_free, since it just re-finds the original structure
pointer. For the pre-list-init failure path, there is no RCU needed,
since it was just allocated. It can be directly freed.

Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/msg.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 0ed7dae..25d43e2 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -95,13 +95,18 @@ static inline void msg_rmid(struct ipc_namespace *ns, struct msg_queue *s)
 	ipc_rmid(&msg_ids(ns), &s->q_perm);
 }
 
+static void __msg_free(struct msg_queue *msq)
+{
+	kvfree(msq);
+}
+
 static void msg_rcu_free(struct rcu_head *head)
 {
 	struct kern_ipc_perm *p = container_of(head, struct kern_ipc_perm, rcu);
 	struct msg_queue *msq = container_of(p, struct msg_queue, q_perm);
 
 	security_msg_queue_free(msq);
-	ipc_rcu_free(head);
+	__msg_free(msq);
 }
 
 /**
@@ -131,7 +136,7 @@ static int newque(struct ipc_namespace *ns, struct ipc_params *params)
 	msq->q_perm.security = NULL;
 	retval = security_msg_queue_alloc(msq);
 	if (retval) {
-		ipc_rcu_putref(&msq->q_perm, ipc_rcu_free);
+		__msg_free(msq);
 		return retval;
 	}
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650755 — [PATCH 10/20] ipc/shm: Avoid ipc_rcu_alloc()

From: Kees Cook <keescook@chromium.org>

Instead of using ipc_rcu_alloc() which only performs the refcount
bump, open code it. This also allows for shmid_kernel structure
layout to be randomized in the future.

Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/shm.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 77e1bff..c9f1f30 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -518,6 +518,19 @@ static const struct vm_operations_struct shm_vm_ops = {
 #endif
 };
 
+static struct shmid_kernel *shm_alloc(void)
+{
+	struct shmid_kernel *shp;
+
+	shp = kvmalloc(sizeof(*shp), GFP_KERNEL);
+	if (unlikely(!shp))
+		return NULL;
+
+	atomic_set(&shp->shm_perm.refcount, 1);
+
+	return shp;
+}
+
 /**
  * newseg - Create a new shared memory segment
  * @ns: namespace
@@ -548,10 +561,7 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
 			ns->shm_tot + numpages > ns->shm_ctlall)
 		return -ENOSPC;
 
-	BUILD_BUG_ON(offsetof(struct shmid_kernel, shm_perm) != 0);
-
-	shp = container_of(ipc_rcu_alloc(sizeof(*shp)), struct shmid_kernel,
-				shm_perm);
+	shp = shm_alloc();
 	if (!shp)
 		return -ENOMEM;
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1650777

On Thu, May 25, 2017 at 11:50 AM, Manfred Spraul
<manfred@colorfullife.com> wrote:
> Hi all,
>
> Updated series. The series got longer, because I merged all patches
> from Kees.
>
> Main changes are:
> - sems[] instead of sem[0].
> - Immediately use BUILD_BUG_ON()
> - Immediately move the memset() to avoid crashing with SEM_UNDO.
> - Use rcu for every security_xx_free(), even if ipc_addid() was not
>   successful

This all looks great (excepting the accidental hunk migration). I like
the change to use call_rcu(), though I still think it's overkill. ;)

Thanks for integrating my changes!

-Kees

> @Andrew: Could you add them again to your tree?
>
> @Michael:
> Should we update man semctl(2)?
> Several years ago, I did a review and found that sem_ctime is only
> for Coherent the time of the last change...
>
> http://calculix-rpm.sourceforge.net/sysvsem.html
>
> --
>         Manfred



-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1651030

Hi Kees,

On 05/25/2017 09:45 PM, Kees Cook wrote:
> On Thu, May 25, 2017 at 11:50 AM, Manfred Spraul
> <manfred@colorfullife.com> wrote:
>> Hi all,
>>
>> Updated series. The series got longer, because I merged all patches
>> from Kees.
>>
>> Main changes are:
>> - sems[] instead of sem[0].
>> - Immediately use BUILD_BUG_ON()
>> - Immediately move the memset() to avoid crashing with SEM_UNDO.
>> - Use rcu for every security_xx_free(), even if ipc_addid() was not
>>    successful
> This all looks great (excepting the accidental hunk migration). I like
> the change to use call_rcu(), though I still think it's overkill. ;)
I still don't see the hunk migration.

What I wanted to do was to merge a fix for the wrong memset into
"[PATCH 02/20] ipc: merge ipc_rcu and kern_ipc_perm"

--
     Manfred

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web