[PATCH 00/10] ipc MSG_COPY fixes

[PATCH 00/10] ipc MSG_COPY fixes

[Peter Hurley]

Over the weekend testing with trinity on KVM, I hit a similar oops
(pasted below) to what others have already reported here
     http://lkml.indiana.edu/hypermail/linux/kernel/1302.2/01465.html

While trying to uncover the underlying cause of the list corruption,
I uncovered two other bugs which are addressed in
  ipc: Fix potential oops when src msg > 4k w/ MSG_COPY
  ipc: Don't allocate a copy larger than max

The other cleanup was incidental to trying to uncover the oops (so far
unsuccessfully).

Can the alloc_msg() be further simplified to allocate one block with
vmalloc() and link the msg segments in-place?


[   86.026309] BUG: unable to handle kernel paging request at 0000000000058134
[   86.035004] IP: [<ffffffff813087d0>] testmsg.isra.5+0x30/0x60
[   86.035004] PGD 5ff2d067 PUD 5ee34067 PMD 0
[   86.035004] Oops: 0000 [#1] PREEMPT SMP
[   86.035004] Modules linked in: can_bcm bridge stp dlci af_rxrpc .......
[   86.035004] CPU 5
[   86.035004] Pid: 1736, comm: trinity-child37 Not tainted 3.9.0-next-20130220+ldsem-xeon+lockdep #20130220+ldsem Bochs Bochs
[   86.035004] RIP: 0010:[<ffffffff813087d0>]  [<ffffffff813087d0>] testmsg.isra.5+0x30/0x60
[   86.035004] RSP: 0018:ffff88005ee2fe78  EFLAGS: 00010246
[   86.035004] RAX: 0000000000000000 RBX: 0000000000000004 RCX: 0000000000000001
[   86.035004] RDX: 0000000000000004 RSI: 8000000000000000 RDI: 0000000000058134
[   86.035004] RBP: ffff88005ee2fe78 R08: 0000000b0ff40000 R09: 0000000000000000
[   86.035004] R10: 0000000000000001 R11: 0000000000000000 R12: 8000000000000000
[   86.035004] R13: ffff880061275c20 R14: 0000000000058124 R15: ffff880061275b70
[   86.035004] FS:  00007f9b37442700(0000) GS:ffff88007d400000(0000) knlGS:0000000000000000
[   86.035004] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   86.035004] CR2: 0000000000058134 CR3: 000000005ff2c000 CR4: 00000000000007e0
[   86.035004] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   86.035004] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[   86.035004] Process trinity-child37 (pid: 1736, threadinfo ffff88005ee2e000, task ffff88005edaa440)
[   86.035004] Stack:
[   86.035004]  ffff88005ee2ff68 ffffffff81309c06 00000000001d5b00 ffff88005edaa440
[   86.035004]  ffff88005edaa440 ffff88005edaa440 0000000000000000 ffffffff813085e0
[   86.035004]  0000000000000000 ffff88005ff7e458 0000000000000000 00000000006fe000
[   86.035004] Call Trace:
[   86.035004]  [<ffffffff81309c06>] do_msgrcv+0x1d6/0x6a0
[   86.035004]  [<ffffffff813085e0>] ? load_msg+0x180/0x180
[   86.035004]  [<ffffffff810d473d>] ? trace_hardirqs_on_caller+0x10d/0x1a0
[   86.035004]  [<ffffffff813b52fe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[   86.035004]  [<ffffffff8130a0e5>] sys_msgrcv+0x15/0x20
[   86.035004]  [<ffffffff817aebd9>] system_call_fastpath+0x16/0x1b
[   86.035004] Code: 55 83 fa 02 48 89 e5 74 32 7e 10 83 fa 03 74 3b 83 fa 04 74 16 31 c0 5d c3 66 90 83 fa 01 b8 01 00 00 00 74 f2 31 c0 eb ee 66 90 <48> 39 37 b8 01 00 00 00 7e e2 31 c0 eb de 66 90 48 3b 37 75 d5 
[   86.035004] RIP  [<ffffffff813087d0>] testmsg.isra.5+0x30/0x60
[   86.035004]  RSP <ffff88005ee2fe78>
[   86.035004] CR2: 0000000000058134
[   86.183799] ---[ end trace f8a403aaa782a5b4 ]---



Peter Hurley (10):
  ipc: Fix potential oops when src msg > 4k w/ MSG_COPY
  ipc: Clamp with min()
  ipc: Separate msg allocation from userspace copy
  ipc: Tighten msg copy loops
  ipc: Set EFAULT as default error in load_msg()
  ipc: Don't allocate a copy larger than max
  ipc: Remove msg handling from queue scan
  ipc: Implement MSG_COPY as a new receive mode
  ipc: Simplify msg list search
  ipc: Refactor msg list search into separate function

 ipc/msg.c     |  84 +++++++++++++++++++++-----------------------
 ipc/msgutil.c | 109 +++++++++++++++++++++++++++-------------------------------
 2 files changed, 90 insertions(+), 103 deletions(-)

-- 
1.8.1.2

[PATCH 01/10] ipc: Fix potential oops when src msg > 4k w/ MSG_COPY

[Peter Hurley]

If the src msg is > 4k, then dest->next points to the
next allocated segment; resetting it just prior to dereferencing
is bad.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msgutil.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index ebfcbfa..5df8e4b 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -117,9 +117,6 @@ struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst)
 	if (alen > DATALEN_MSG)
 		alen = DATALEN_MSG;
 
-	dst->next = NULL;
-	dst->security = NULL;
-
 	memcpy(dst + 1, src + 1, alen);
 
 	len -= alen;
-- 
1.8.1.2

[PATCH 02/10] ipc: Clamp with min()

[Peter Hurley]

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msgutil.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index 5df8e4b..98b1c2b 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -41,8 +41,8 @@ struct msg_msgseg {
 	/* the next part of the message follows immediately */
 };
 
-#define DATALEN_MSG	(PAGE_SIZE-sizeof(struct msg_msg))
-#define DATALEN_SEG	(PAGE_SIZE-sizeof(struct msg_msgseg))
+#define DATALEN_MSG	(int)(PAGE_SIZE-sizeof(struct msg_msg))
+#define DATALEN_SEG	(int)(PAGE_SIZE-sizeof(struct msg_msgseg))
 
 struct msg_msg *load_msg(const void __user *src, int len)
 {
@@ -51,10 +51,7 @@ struct msg_msg *load_msg(const void __user *src, int len)
 	int err;
 	int alen;
 
-	alen = len;
-	if (alen > DATALEN_MSG)
-		alen = DATALEN_MSG;
-
+	alen = min(len, DATALEN_MSG);
 	msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL);
 	if (msg == NULL)
 		return ERR_PTR(-ENOMEM);
@@ -72,9 +69,7 @@ struct msg_msg *load_msg(const void __user *src, int len)
 	pseg = &msg->next;
 	while (len > 0) {
 		struct msg_msgseg *seg;
-		alen = len;
-		if (alen > DATALEN_SEG)
-			alen = DATALEN_SEG;
+		alen = min(len, DATALEN_SEG);
 		seg = kmalloc(sizeof(*seg) + alen,
 						 GFP_KERNEL);
 		if (seg == NULL) {
@@ -113,19 +108,14 @@ struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst)
 	if (src->m_ts > dst->m_ts)
 		return ERR_PTR(-EINVAL);
 
-	alen = len;
-	if (alen > DATALEN_MSG)
-		alen = DATALEN_MSG;
-
+	alen = min(len, DATALEN_MSG);
 	memcpy(dst + 1, src + 1, alen);
 
 	len -= alen;
 	dst_pseg = dst->next;
 	src_pseg = src->next;
 	while (len > 0) {
-		alen = len;
-		if (alen > DATALEN_SEG)
-			alen = DATALEN_SEG;
+		alen = min(len, DATALEN_SEG);
 		memcpy(dst_pseg + 1, src_pseg + 1, alen);
 		dst_pseg = dst_pseg->next;
 		len -= alen;
@@ -148,9 +138,7 @@ int store_msg(void __user *dest, struct msg_msg *msg, int len)
 	int alen;
 	struct msg_msgseg *seg;
 
-	alen = len;
-	if (alen > DATALEN_MSG)
-		alen = DATALEN_MSG;
+	alen = min(len, DATALEN_MSG);
 	if (copy_to_user(dest, msg + 1, alen))
 		return -1;
 
@@ -158,9 +146,7 @@ int store_msg(void __user *dest, struct msg_msg *msg, int len)
 	dest = ((char __user *)dest) + alen;
 	seg = msg->next;
 	while (len > 0) {
-		alen = len;
-		if (alen > DATALEN_SEG)
-			alen = DATALEN_SEG;
+		alen = min(len, DATALEN_SEG);
 		if (copy_to_user(dest, seg + 1, alen))
 			return -1;
 		len -= alen;
-- 
1.8.1.2

[PATCH 03/10] ipc: Separate msg allocation from userspace copy

[Peter Hurley]

Separating msg allocation enables single-block vmalloc
allocation instead.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msgutil.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 14 deletions(-)

diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index 98b1c2b..0a5c8a9 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -44,21 +44,54 @@ struct msg_msgseg {
 #define DATALEN_MSG	(int)(PAGE_SIZE-sizeof(struct msg_msg))
 #define DATALEN_SEG	(int)(PAGE_SIZE-sizeof(struct msg_msgseg))
 
-struct msg_msg *load_msg(const void __user *src, int len)
+
+static struct msg_msg *alloc_msg(int len)
 {
 	struct msg_msg *msg;
 	struct msg_msgseg **pseg;
-	int err;
 	int alen;
 
 	alen = min(len, DATALEN_MSG);
 	msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL);
 	if (msg == NULL)
-		return ERR_PTR(-ENOMEM);
+		return NULL;
 
 	msg->next = NULL;
 	msg->security = NULL;
 
+	len -= alen;
+	pseg = &msg->next;
+	while (len > 0) {
+		struct msg_msgseg *seg;
+		alen = min(len, DATALEN_SEG);
+		seg = kmalloc(sizeof(*seg) + alen, GFP_KERNEL);
+		if (seg == NULL)
+			goto out_err;
+		*pseg = seg;
+		seg->next = NULL;
+		pseg = &seg->next;
+		len -= alen;
+	}
+
+	return msg;
+
+out_err:
+	free_msg(msg);
+	return NULL;
+}
+
+struct msg_msg *load_msg(const void __user *src, int len)
+{
+	struct msg_msg *msg;
+	struct msg_msgseg *seg;
+	int err;
+	int alen;
+
+	msg = alloc_msg(len);
+	if (msg == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	alen = min(len, DATALEN_MSG);
 	if (copy_from_user(msg + 1, src, alen)) {
 		err = -EFAULT;
 		goto out_err;
@@ -66,23 +99,14 @@ struct msg_msg *load_msg(const void __user *src, int len)
 
 	len -= alen;
 	src = ((char __user *)src) + alen;
-	pseg = &msg->next;
+	seg = msg->next;
 	while (len > 0) {
-		struct msg_msgseg *seg;
 		alen = min(len, DATALEN_SEG);
-		seg = kmalloc(sizeof(*seg) + alen,
-						 GFP_KERNEL);
-		if (seg == NULL) {
-			err = -ENOMEM;
-			goto out_err;
-		}
-		*pseg = seg;
-		seg->next = NULL;
 		if (copy_from_user(seg + 1, src, alen)) {
 			err = -EFAULT;
 			goto out_err;
 		}
-		pseg = &seg->next;
+		seg = seg->next;
 		len -= alen;
 		src = ((char __user *)src) + alen;
 	}
-- 
1.8.1.2

[PATCH 04/10] ipc: Tighten msg copy loops

[Peter Hurley]

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msgutil.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index 0a5c8a9..b79582d 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -97,18 +97,14 @@ struct msg_msg *load_msg(const void __user *src, int len)
 		goto out_err;
 	}
 
-	len -= alen;
-	src = ((char __user *)src) + alen;
-	seg = msg->next;
-	while (len > 0) {
+	for (seg = msg->next; seg != NULL; seg = seg->next) {
+		len -= alen;
+		src = (char __user *)src + alen;
 		alen = min(len, DATALEN_SEG);
 		if (copy_from_user(seg + 1, src, alen)) {
 			err = -EFAULT;
 			goto out_err;
 		}
-		seg = seg->next;
-		len -= alen;
-		src = ((char __user *)src) + alen;
 	}
 
 	err = security_msg_msg_alloc(msg);
@@ -135,15 +131,13 @@ struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst)
 	alen = min(len, DATALEN_MSG);
 	memcpy(dst + 1, src + 1, alen);
 
-	len -= alen;
-	dst_pseg = dst->next;
-	src_pseg = src->next;
-	while (len > 0) {
+	for (dst_pseg = dst->next, src_pseg = src->next;
+	     src_pseg != NULL;
+	     dst_pseg = dst_pseg->next, src_pseg = src_pseg->next) {
+
+		len -= alen;
 		alen = min(len, DATALEN_SEG);
 		memcpy(dst_pseg + 1, src_pseg + 1, alen);
-		dst_pseg = dst_pseg->next;
-		len -= alen;
-		src_pseg = src_pseg->next;
 	}
 
 	dst->m_type = src->m_type;
@@ -166,16 +160,12 @@ int store_msg(void __user *dest, struct msg_msg *msg, int len)
 	if (copy_to_user(dest, msg + 1, alen))
 		return -1;
 
-	len -= alen;
-	dest = ((char __user *)dest) + alen;
-	seg = msg->next;
-	while (len > 0) {
+	for (seg = msg->next; seg != NULL; seg = seg->next) {
+		len -= alen;
+		dest = (char __user *)dest + alen;
 		alen = min(len, DATALEN_SEG);
 		if (copy_to_user(dest, seg + 1, alen))
 			return -1;
-		len -= alen;
-		dest = ((char __user *)dest) + alen;
-		seg = seg->next;
 	}
 	return 0;
 }
-- 
1.8.1.2

[PATCH 05/10] ipc: Set EFAULT as default error in load_msg()

[Peter Hurley]

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msgutil.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index b79582d..d33fbb2 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -84,7 +84,7 @@ struct msg_msg *load_msg(const void __user *src, int len)
 {
 	struct msg_msg *msg;
 	struct msg_msgseg *seg;
-	int err;
+	int err = -EFAULT;
 	int alen;
 
 	msg = alloc_msg(len);
@@ -92,19 +92,15 @@ struct msg_msg *load_msg(const void __user *src, int len)
 		return ERR_PTR(-ENOMEM);
 
 	alen = min(len, DATALEN_MSG);
-	if (copy_from_user(msg + 1, src, alen)) {
-		err = -EFAULT;
+	if (copy_from_user(msg + 1, src, alen))
 		goto out_err;
-	}
 
 	for (seg = msg->next; seg != NULL; seg = seg->next) {
 		len -= alen;
 		src = (char __user *)src + alen;
 		alen = min(len, DATALEN_SEG);
-		if (copy_from_user(seg + 1, src, alen)) {
-			err = -EFAULT;
+		if (copy_from_user(seg + 1, src, alen))
 			goto out_err;
-		}
 	}
 
 	err = security_msg_msg_alloc(msg);
-- 
1.8.1.2

[PATCH 06/10] ipc: Don't allocate a copy larger than max

[Peter Hurley]

When MSG_COPY is set, a duplicate message must be allocated for
the copy before locking the queue. However, the copy could
not be larger than was sent which is limited to msg_ctlmax.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 950572f..31cd1bf 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -820,15 +820,17 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 	struct msg_msg *copy = NULL;
 	unsigned long copy_number = 0;
 
+	ns = current->nsproxy->ipc_ns;
+
 	if (msqid < 0 || (long) bufsz < 0)
 		return -EINVAL;
 	if (msgflg & MSG_COPY) {
-		copy = prepare_copy(buf, bufsz, msgflg, &msgtyp, &copy_number);
+		copy = prepare_copy(buf, min_t(size_t, bufsz, ns->msg_ctlmax),
+				    msgflg, &msgtyp, &copy_number);
 		if (IS_ERR(copy))
 			return PTR_ERR(copy);
 	}
 	mode = convert_mode(&msgtyp, msgflg);
-	ns = current->nsproxy->ipc_ns;
 
 	msq = msg_lock_check(ns, msqid);
 	if (IS_ERR(msq)) {
-- 
1.8.1.2

[PATCH 07/10] ipc: Remove msg handling from queue scan

[Peter Hurley]

In preparation for refactoring the queue scan into a separate
function, relocate msg copying.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msg.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 31cd1bf..e8d3f15 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -862,16 +862,8 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 						walk_msg->m_type != 1) {
 					msgtyp = walk_msg->m_type - 1;
 				} else if (msgflg & MSG_COPY) {
-					if (copy_number == msg_counter) {
-						/*
-						 * Found requested message.
-						 * Copy it.
-						 */
-						msg = copy_msg(msg, copy);
-						if (IS_ERR(msg))
-							goto out_unlock;
+					if (copy_number == msg_counter)
 						break;
-					}
 				} else
 					break;
 				msg_counter++;
@@ -891,8 +883,10 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 			 * If we are copying, then do not unlink message and do
 			 * not update queue parameters.
 			 */
-			if (msgflg & MSG_COPY)
+			if (msgflg & MSG_COPY) {
+				msg = copy_msg(msg, copy);
 				goto out_unlock;
+			}
 			list_del(&msg->m_list);
 			msq->q_qnum--;
 			msq->q_rtime = get_seconds();
-- 
1.8.1.2

[PATCH 08/10] ipc: Implement MSG_COPY as a new receive mode

[Peter Hurley]

Teach the helper routines about MSG_COPY so that
msgtyp is preserved as the message number to copy.

The security functions affected by this change were audited
and no additional changes are necessary.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msg.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index e8d3f15..418c5a5 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -66,6 +66,7 @@ struct msg_sender {
 #define SEARCH_EQUAL		2
 #define SEARCH_NOTEQUAL		3
 #define SEARCH_LESSEQUAL	4
+#define SEARCH_NUMBER		5
 
 #define msg_ids(ns)	((ns)->ids[IPC_MSG_IDS])
 
@@ -583,6 +584,7 @@ static int testmsg(struct msg_msg *msg, long type, int mode)
 	switch(mode)
 	{
 		case SEARCH_ANY:
+		case SEARCH_NUMBER:
 			return 1;
 		case SEARCH_LESSEQUAL:
 			if (msg->m_type <=type)
@@ -738,6 +740,8 @@ SYSCALL_DEFINE4(msgsnd, int, msqid, struct msgbuf __user *, msgp, size_t, msgsz,
 
 static inline int convert_mode(long *msgtyp, int msgflg)
 {
+	if (msgflg & MSG_COPY)
+		return SEARCH_NUMBER;
 	/*
 	 *  find message of correct type.
 	 *  msgtyp = 0 => get first.
@@ -774,14 +778,10 @@ static long do_msg_fill(void __user *dest, struct msg_msg *msg, size_t bufsz)
  * This function creates new kernel message structure, large enough to store
  * bufsz message bytes.
  */
-static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz,
-					   int msgflg, long *msgtyp,
-					   unsigned long *copy_number)
+static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz)
 {
 	struct msg_msg *copy;
 
-	*copy_number = *msgtyp;
-	*msgtyp = 0;
 	/*
 	 * Create dummy message to copy real message to.
 	 */
@@ -797,9 +797,7 @@ static inline void free_copy(struct msg_msg *copy)
 		free_msg(copy);
 }
 #else
-static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz,
-					   int msgflg, long *msgtyp,
-					   unsigned long *copy_number)
+static inline struct msg_msg *prepare_copy(void __user *buf, size_t bufsz)
 {
 	return ERR_PTR(-ENOSYS);
 }
@@ -818,18 +816,17 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 	int mode;
 	struct ipc_namespace *ns;
 	struct msg_msg *copy = NULL;
-	unsigned long copy_number = 0;
 
 	ns = current->nsproxy->ipc_ns;
 
 	if (msqid < 0 || (long) bufsz < 0)
 		return -EINVAL;
 	if (msgflg & MSG_COPY) {
-		copy = prepare_copy(buf, min_t(size_t, bufsz, ns->msg_ctlmax),
-				    msgflg, &msgtyp, &copy_number);
+		copy = prepare_copy(buf, min_t(size_t, bufsz, ns->msg_ctlmax));
 		if (IS_ERR(copy))
 			return PTR_ERR(copy);
 	}
+
 	mode = convert_mode(&msgtyp, msgflg);
 
 	msq = msg_lock_check(ns, msqid);
@@ -861,8 +858,8 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 				if (mode == SEARCH_LESSEQUAL &&
 						walk_msg->m_type != 1) {
 					msgtyp = walk_msg->m_type - 1;
-				} else if (msgflg & MSG_COPY) {
-					if (copy_number == msg_counter)
+				} else if (mode == SEARCH_NUMBER) {
+					if (msgtyp == msg_counter)
 						break;
 				} else
 					break;
-- 
1.8.1.2

[PATCH 09/10] ipc: Simplify msg list search

[Peter Hurley]

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msg.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 418c5a5..c3867da 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -837,7 +837,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 
 	for (;;) {
 		struct msg_receiver msr_d;
-		struct list_head *tmp;
+		struct msg_msg *walk_msg;
 		long msg_counter = 0;
 
 		msg = ERR_PTR(-EACCES);
@@ -845,11 +845,8 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 			goto out_unlock;
 
 		msg = ERR_PTR(-EAGAIN);
-		tmp = msq->q_messages.next;
-		while (tmp != &msq->q_messages) {
-			struct msg_msg *walk_msg;
+		list_for_each_entry(walk_msg, &msq->qmessages, m_list) {
 
-			walk_msg = list_entry(tmp, struct msg_msg, m_list);
 			if (testmsg(walk_msg, msgtyp, mode) &&
 			    !security_msg_queue_msgrcv(msq, walk_msg, current,
 						       msgtyp, mode)) {
@@ -865,7 +862,6 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 					break;
 				msg_counter++;
 			}
-			tmp = tmp->next;
 		}
 		if (!IS_ERR(msg)) {
 			/*
-- 
1.8.1.2

[PATCH 10/10] ipc: Refactor msg list search into separate function

[Peter Hurley]

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 ipc/msg.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index c3867da..daeca13 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -807,6 +807,30 @@ static inline void free_copy(struct msg_msg *copy)
 }
 #endif
 
+struct msg_msg *find_msg(struct msg_queue *msq, long *msgtyp, int mode)
+{
+	struct msg_msg *msg;
+	long count = 0;
+
+	list_for_each_entry(msg, &msq->q_messages, m_list) {
+		if (testmsg(msg, *msgtyp, mode) &&
+		    !security_msg_queue_msgrcv(msq, msg, current,
+					       *msgtyp, mode)) {
+			if (mode == SEARCH_LESSEQUAL && msg->m_type != 1) {
+				*msgtyp = msg->m_type - 1;
+			} else if (mode == SEARCH_NUMBER) {
+				if (*msgtyp == count)
+					return msg;
+			} else
+				return msg;
+			count++;
+		}
+	}
+
+	return ERR_PTR(-EAGAIN);
+}
+
+
 long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 	       int msgflg,
 	       long (*msg_handler)(void __user *, struct msg_msg *, size_t))
@@ -837,32 +861,13 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 
 	for (;;) {
 		struct msg_receiver msr_d;
-		struct msg_msg *walk_msg;
-		long msg_counter = 0;
 
 		msg = ERR_PTR(-EACCES);
 		if (ipcperms(ns, &msq->q_perm, S_IRUGO))
 			goto out_unlock;
 
-		msg = ERR_PTR(-EAGAIN);
-		list_for_each_entry(walk_msg, &msq->qmessages, m_list) {
-
-			if (testmsg(walk_msg, msgtyp, mode) &&
-			    !security_msg_queue_msgrcv(msq, walk_msg, current,
-						       msgtyp, mode)) {
-
-				msg = walk_msg;
-				if (mode == SEARCH_LESSEQUAL &&
-						walk_msg->m_type != 1) {
-					msgtyp = walk_msg->m_type - 1;
-				} else if (mode == SEARCH_NUMBER) {
-					if (msgtyp == msg_counter)
-						break;
-				} else
-					break;
-				msg_counter++;
-			}
-		}
+		msg = find_msg(msq, &msgtyp, mode);
+
 		if (!IS_ERR(msg)) {
 			/*
 			 * Found a suitable message.
-- 
1.8.1.2

[PATCH v2 09/10] ipc: Simplify msg list search

[Peter Hurley]

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---

v2 - /s/qmessages/q_messages/

 ipc/msg.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 418c5a5..ba431c9 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -837,7 +837,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 
 	for (;;) {
 		struct msg_receiver msr_d;
-		struct list_head *tmp;
+		struct msg_msg *walk_msg;
 		long msg_counter = 0;
 
 		msg = ERR_PTR(-EACCES);
@@ -845,11 +845,8 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 			goto out_unlock;
 
 		msg = ERR_PTR(-EAGAIN);
-		tmp = msq->q_messages.next;
-		while (tmp != &msq->q_messages) {
-			struct msg_msg *walk_msg;
+		list_for_each_entry(walk_msg, &msq->q_messages, m_list) {
 
-			walk_msg = list_entry(tmp, struct msg_msg, m_list);
 			if (testmsg(walk_msg, msgtyp, mode) &&
 			    !security_msg_queue_msgrcv(msq, walk_msg, current,
 						       msgtyp, mode)) {
@@ -865,7 +862,6 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 					break;
 				msg_counter++;
 			}
-			tmp = tmp->next;
 		}
 		if (!IS_ERR(msg)) {
 			/*
-- 
1.8.1.2

[PATCH v2 10/10] ipc: Refactor msg list search into separate function

[Peter Hurley]

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---

v2 - /s/qmessages/q_messages/

 ipc/msg.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index ba431c9..daeca13 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -807,6 +807,30 @@ static inline void free_copy(struct msg_msg *copy)
 }
 #endif
 
+struct msg_msg *find_msg(struct msg_queue *msq, long *msgtyp, int mode)
+{
+	struct msg_msg *msg;
+	long count = 0;
+
+	list_for_each_entry(msg, &msq->q_messages, m_list) {
+		if (testmsg(msg, *msgtyp, mode) &&
+		    !security_msg_queue_msgrcv(msq, msg, current,
+					       *msgtyp, mode)) {
+			if (mode == SEARCH_LESSEQUAL && msg->m_type != 1) {
+				*msgtyp = msg->m_type - 1;
+			} else if (mode == SEARCH_NUMBER) {
+				if (*msgtyp == count)
+					return msg;
+			} else
+				return msg;
+			count++;
+		}
+	}
+
+	return ERR_PTR(-EAGAIN);
+}
+
+
 long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 	       int msgflg,
 	       long (*msg_handler)(void __user *, struct msg_msg *, size_t))
@@ -837,32 +861,13 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp,
 
 	for (;;) {
 		struct msg_receiver msr_d;
-		struct msg_msg *walk_msg;
-		long msg_counter = 0;
 
 		msg = ERR_PTR(-EACCES);
 		if (ipcperms(ns, &msq->q_perm, S_IRUGO))
 			goto out_unlock;
 
-		msg = ERR_PTR(-EAGAIN);
-		list_for_each_entry(walk_msg, &msq->q_messages, m_list) {
-
-			if (testmsg(walk_msg, msgtyp, mode) &&
-			    !security_msg_queue_msgrcv(msq, walk_msg, current,
-						       msgtyp, mode)) {
-
-				msg = walk_msg;
-				if (mode == SEARCH_LESSEQUAL &&
-						walk_msg->m_type != 1) {
-					msgtyp = walk_msg->m_type - 1;
-				} else if (mode == SEARCH_NUMBER) {
-					if (msgtyp == msg_counter)
-						break;
-				} else
-					break;
-				msg_counter++;
-			}
-		}
+		msg = find_msg(msq, &msgtyp, mode);
+
 		if (!IS_ERR(msg)) {
 			/*
 			 * Found a suitable message.
-- 
1.8.1.2

Re: [PATCH 00/10] ipc MSG_COPY fixes

[Stanislav Kinsbursky]

Looks good to me. Thanks you, Peter!

Acked-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

Next time please, add maintainer to "To" list instead of "CC" list (no need to resend - I've added Andrew Morton to "To" list in this reply).

26.02.2013 06:21, Peter Hurley пишет:
> Over the weekend testing with trinity on KVM, I hit a similar oops
> (pasted below) to what others have already reported here
>       http://lkml.indiana.edu/hypermail/linux/kernel/1302.2/01465.html
>
> While trying to uncover the underlying cause of the list corruption,
> I uncovered two other bugs which are addressed in
>    ipc: Fix potential oops when src msg > 4k w/ MSG_COPY
>    ipc: Don't allocate a copy larger than max
>
> The other cleanup was incidental to trying to uncover the oops (so far
> unsuccessfully).
>
> Can the alloc_msg() be further simplified to allocate one block with
> vmalloc() and link the msg segments in-place?
>
>
> [   86.026309] BUG: unable to handle kernel paging request at 0000000000058134
> [   86.035004] IP: [<ffffffff813087d0>] testmsg.isra.5+0x30/0x60
> [   86.035004] PGD 5ff2d067 PUD 5ee34067 PMD 0
> [   86.035004] Oops: 0000 [#1] PREEMPT SMP
> [   86.035004] Modules linked in: can_bcm bridge stp dlci af_rxrpc .......
> [   86.035004] CPU 5
> [   86.035004] Pid: 1736, comm: trinity-child37 Not tainted 3.9.0-next-20130220+ldsem-xeon+lockdep #20130220+ldsem Bochs Bochs
> [   86.035004] RIP: 0010:[<ffffffff813087d0>]  [<ffffffff813087d0>] testmsg.isra.5+0x30/0x60
> [   86.035004] RSP: 0018:ffff88005ee2fe78  EFLAGS: 00010246
> [   86.035004] RAX: 0000000000000000 RBX: 0000000000000004 RCX: 0000000000000001
> [   86.035004] RDX: 0000000000000004 RSI: 8000000000000000 RDI: 0000000000058134
> [   86.035004] RBP: ffff88005ee2fe78 R08: 0000000b0ff40000 R09: 0000000000000000
> [   86.035004] R10: 0000000000000001 R11: 0000000000000000 R12: 8000000000000000
> [   86.035004] R13: ffff880061275c20 R14: 0000000000058124 R15: ffff880061275b70
> [   86.035004] FS:  00007f9b37442700(0000) GS:ffff88007d400000(0000) knlGS:0000000000000000
> [   86.035004] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   86.035004] CR2: 0000000000058134 CR3: 000000005ff2c000 CR4: 00000000000007e0
> [   86.035004] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   86.035004] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [   86.035004] Process trinity-child37 (pid: 1736, threadinfo ffff88005ee2e000, task ffff88005edaa440)
> [   86.035004] Stack:
> [   86.035004]  ffff88005ee2ff68 ffffffff81309c06 00000000001d5b00 ffff88005edaa440
> [   86.035004]  ffff88005edaa440 ffff88005edaa440 0000000000000000 ffffffff813085e0
> [   86.035004]  0000000000000000 ffff88005ff7e458 0000000000000000 00000000006fe000
> [   86.035004] Call Trace:
> [   86.035004]  [<ffffffff81309c06>] do_msgrcv+0x1d6/0x6a0
> [   86.035004]  [<ffffffff813085e0>] ? load_msg+0x180/0x180
> [   86.035004]  [<ffffffff810d473d>] ? trace_hardirqs_on_caller+0x10d/0x1a0
> [   86.035004]  [<ffffffff813b52fe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [   86.035004]  [<ffffffff8130a0e5>] sys_msgrcv+0x15/0x20
> [   86.035004]  [<ffffffff817aebd9>] system_call_fastpath+0x16/0x1b
> [   86.035004] Code: 55 83 fa 02 48 89 e5 74 32 7e 10 83 fa 03 74 3b 83 fa 04 74 16 31 c0 5d c3 66 90 83 fa 01 b8 01 00 00 00 74 f2 31 c0 eb ee 66 90 <48> 39 37 b8 01 00 00 00 7e e2 31 c0 eb de 66 90 48 3b 37 75 d5
> [   86.035004] RIP  [<ffffffff813087d0>] testmsg.isra.5+0x30/0x60
> [   86.035004]  RSP <ffff88005ee2fe78>
> [   86.035004] CR2: 0000000000058134
> [   86.183799] ---[ end trace f8a403aaa782a5b4 ]---
>
>
>
> Peter Hurley (10):
>    ipc: Fix potential oops when src msg > 4k w/ MSG_COPY
>    ipc: Clamp with min()
>    ipc: Separate msg allocation from userspace copy
>    ipc: Tighten msg copy loops
>    ipc: Set EFAULT as default error in load_msg()
>    ipc: Don't allocate a copy larger than max
>    ipc: Remove msg handling from queue scan
>    ipc: Implement MSG_COPY as a new receive mode
>    ipc: Simplify msg list search
>    ipc: Refactor msg list search into separate function
>
>   ipc/msg.c     |  84 +++++++++++++++++++++-----------------------
>   ipc/msgutil.c | 109 +++++++++++++++++++++++++++-------------------------------
>   2 files changed, 90 insertions(+), 103 deletions(-)
>


-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH 00/10] ipc MSG_COPY fixes

[Peter Hurley]

On Tue, 2013-02-26 at 11:53 +0400, Stanislav Kinsbursky wrote:
> Looks good to me. Thanks you, Peter!
> 
> Acked-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> 
> Next time please, add maintainer to "To" list instead of "CC" list (no need to resend - I've added Andrew Morton to "To" list in this reply).

Ok.

> > Can the alloc_msg() be further simplified to allocate one block with
> > vmalloc() and link the msg segments in-place?

Any thoughts on this suggestion?

Regards,
Peter Hurley

Re: [PATCH 00/10] ipc MSG_COPY fixes

[Andrew Morton]

On Mon, 25 Feb 2013 21:21:37 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> Over the weekend testing with trinity on KVM, I hit a similar oops
> (pasted below) to what others have already reported here
>      http://lkml.indiana.edu/hypermail/linux/kernel/1302.2/01465.html
> 
> While trying to uncover the underlying cause of the list corruption,
> I uncovered two other bugs which are addressed in
>   ipc: Fix potential oops when src msg > 4k w/ MSG_COPY
>   ipc: Don't allocate a copy larger than max
> 
> The other cleanup was incidental to trying to uncover the oops (so far
> unsuccessfully).

afacit, only the above two are needed in 3.9 and 3.8.x, agree?

The changelog for "ipc: Don't allocate a copy larger than max" is
rather poor - it doesn't actually describe the bug's effects, so people
will have trouble understanding whether they need the patch in their
kernels.  Can you please send along some additional description of this
one?

Re: [PATCH 00/10] ipc MSG_COPY fixes

[Stanislav Kinsbursky]

26.02.2013 16:00, Peter Hurley пишет:
> On Tue, 2013-02-26 at 11:53 +0400, Stanislav Kinsbursky wrote:
>> Looks good to me. Thanks you, Peter!
>>
>> Acked-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
>>
>> Next time please, add maintainer to "To" list instead of "CC" list (no need to resend - I've added Andrew Morton to "To" list in this reply).
>
> Ok.
>
>>> Can the alloc_msg() be further simplified to allocate one block with
>>> vmalloc() and link the msg segments in-place?
>
> Any thoughts on this suggestion?
>

Emm... You can do so, is you want.
But this will be just an optimisation on a slow-path. I.e. I have nothing
to object, but don't see any other reason except striving for perfection.

> Regards,
> Peter Hurley
>


-- 
Best regards,
Stanislav Kinsbursky