Skip to contentSkip to conversation

mmiotrace broken in linux-next (8-bit writes only)

PATCH Applied Pekka Paalanen 3 participants 5 messages Jun 30, 2008 – Jul 26, 2008 2 messages not in the archive

Subsystems (2)
x86 x86 MM
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Pekka Paalanen

author
#2 lore raw
Quoted 3 lines · On Mon, 30 Jun 2008 14:37:36 +0200 Ingo Molnar <[email protected]> wrote:

it would also be nice if you could check whether ftrace as integrated into linux-next has a functional mmiotrace. (and work with me in -tip on fixing it if it doesnt)

I tested it and it is broken in two ways. First is the NULL pointer dereference, for which I made a quick patch (in the end of this email) and I don't know what is going on in there. The patch allows me to run my simple test. However, the test shows, that all 8-bit MMIO writes are recorded with datum 0x80, which is incorrect. I've no idea what could have changed. All the 16 and 32-bit ops are ok, and so are 8-bit reads, which means the writes really do happen with the correct data, but just mmiotrace gets it wrong. Mmiotrace source files are unchanged.

Has there been changes in compiler flags or iowrite8() that would cause different instructions to be used in implementing the MMIO write? I'm completely baffled.

I guess I should try to bisect, but I don't know when I can do that. I tested on Athlon64, 64-bit UP kernel.

Vegard, have you experienced any weird issues in instruction decoding? But I guess sort of bug would not affect you, since kmemcheck is not interested in the data.

- pq

Quoted 1 line

From 79e6a8eaf73b778dc76eb3c55bb82b18817c34df Mon Sep 17 00:00:00 2001

From: Pekka Paalanen <[email protected]>
Date: Mon, 30 Jun 2008 22:02:25 +0300
Subject: [PATCH] ftrace: do not call ftrace_special() from down().

The moment mmiotrace is enabled, I hit a NULL deref in:

IP: [<ffffffff80256e71>] __trace_special+0x17c/0x23a

Call Trace:
 [<ffffffff802573cc>] ftrace_special+0x6f/0x9a
 [<ffffffff8023e3e4>] down+0x19/0x4a
 [<ffffffff80228adc>] acquire_console_sem+0x42/0x58
 [<ffffffff8035d273>] con_flush_chars+0x28/0x43
 [<ffffffff80354a70>] write_chan+0x22e/0x334
 [<ffffffff802244e9>] ? default_wake_function+0x0/0xf
 [<ffffffff8035236d>] tty_write+0x195/0x228
 [<ffffffff80354842>] ? write_chan+0x0/0x334
 [<ffffffff8027c23a>] vfs_write+0xae/0x137
 [<ffffffff8027c6e3>] sys_write+0x47/0x70
 [<ffffffff8020b1db>] system_call_after_swapgs+0x7b/0x80

which means 'entry' in __trace_special() is NULL.

Signed-off-by Pekka Paalanen <[email protected]>
---
 kernel/semaphore.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
kernel/semaphore.c +0 -1
index bbab232..62f30c2 100644
@@ -54,7 +54,6 @@ void down(struct semaphore *sem)
 {
 	unsigned long flags;
 
-	ftrace_special(sem->count, 0, __LINE__);
 	spin_lock_irqsave(&sem->lock, flags);
 	if (likely(sem->count > 0))
 		sem->count--;
Signature · 1 line
1.5.4.5
#3 lore raw
Quoted 20 lines · Pekka Paalanen
Quoted 3 lines · On Mon, 30 Jun 2008 14:37:36 +0200 Ingo Molnar <[email protected]> wrote:

it would also be nice if you could check whether ftrace as integrated into linux-next has a functional mmiotrace. (and work with me in -tip on fixing it if it doesnt)

I tested it and it is broken in two ways. First is the NULL pointer dereference, for which I made a quick patch (in the end of this email) and I don't know what is going on in there. The patch allows me to run my simple test. However, the test shows, that all 8-bit MMIO writes are recorded with datum 0x80, which is incorrect. I've no idea what could have changed. All the 16 and 32-bit ops are ok, and so are 8-bit reads, which means the writes really do happen with the correct data, but just mmiotrace gets it wrong. Mmiotrace source files are unchanged.

Has there been changes in compiler flags or iowrite8() that would cause different instructions to be used in implementing the MMIO write? I'm completely baffled.

Quoted 6 lines

I guess I should try to bisect, but I don't know when I can do that. I tested on Athlon64, 64-bit UP kernel.

Vegard, have you experienced any weird issues in instruction decoding? But I guess sort of bug would not affect you, since kmemcheck is not interested in the data.

No. But kmemcheck is not in -next yet either AFAIK :-)

I'll test the latest tip/master and see if I can try out mmiotrace as well. But you are right, kmemcheck is not interested in the data.

(BTW, Ingo, Pekka E., I had an additional idea for kmemcheck: We can record a checksum of bytes that are written and compare them with the actual value when they are next read. If they differ, we are either dealing with DMA memory or bad RAM. But this is long into the future.. :-))

Can you please post a few IPs that give you byte-writes with 0x80 value, then post the disassemblies of those IPs? Also, a chunk of the log would be nice, just so we know what it looks like.

Vegard

Signature · 4 lines
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036
author
#4 lore raw

PATCH 1/1 x86: fix mmiotrace 8-bit register decoding

Quoted 1 line

From 8321edde04244cc91470da5a7a71b79b704b0da1 Mon Sep 17 00:00:00 2001

From: Pekka Paalanen <[email protected]>
Date: Mon, 21 Jul 2008 18:49:56 +0300
Subject: [PATCH] x86: fix mmiotrace 8-bit register decoding

When SIL, DIL, BPL or SPL registers were used in MMIO, the datum was extracted from AH, BH, CH, or DH, which are incorrect.

Signed-off-by Pekka Paalanen <[email protected]>

---

This patch was cooked in Linus' tree because I couldn't get linux-next to boot properly at the time. It applies cleanly to tip/master though. Another problem I noticed is with non-page-aligned mappings, but that's a different story.

 arch/x86/mm/pf_in.c |  121 +++++++++++++++++++++++++++++++++++----------------
 1 files changed, 84 insertions(+), 37 deletions(-)
arch/x86/mm/pf_in.c +84 -37
index efa1911..4d0bf36 100644
@@ -79,25 +79,34 @@ static unsigned int mw32[] = { 0xC7 };
 static unsigned int mw64[] = { 0x89, 0x8B };
 #endif /* not __i386__ */
 
-static int skip_prefix(unsigned char *addr, int *shorted, int *enlarged,
-								int *rexr)
+struct prefix_bits {
+	unsigned shorted:1;
+	unsigned enlarged:1;
+	unsigned rexr:1;
+	unsigned rex:1;
+};
+
+static int skip_prefix(unsigned char *addr, struct prefix_bits *prf)
 {
 	int i;
 	unsigned char *p = addr;
-	*shorted = 0;
-	*enlarged = 0;
-	*rexr = 0;
+	prf->shorted = 0;
+	prf->enlarged = 0;
+	prf->rexr = 0;
+	prf->rex = 0;
 
 restart:
 	for (i = 0; i < ARRAY_SIZE(prefix_codes); i++) {
 		if (*p == prefix_codes[i]) {
 			if (*p == 0x66)
-				*shorted = 1;
+				prf->shorted = 1;
 #ifdef __amd64__
 			if ((*p & 0xf8) == 0x48)
-				*enlarged = 1;
+				prf->enlarged = 1;
 			if ((*p & 0xf4) == 0x44)
-				*rexr = 1;
+				prf->rexr = 1;
+			if ((*p & 0xf0) == 0x40)
+				prf->rex = 1;
 #endif
 			p++;
 			goto restart;

@@ -135,12 +144,12 @@ enum reason_type get_ins_type(unsigned long ins_addr)
 {
 	unsigned int opcode;
 	unsigned char *p;
-	int shorted, enlarged, rexr;
+	struct prefix_bits prf;
 	int i;
 	enum reason_type rv = OTHERS;
 
 	p = (unsigned char *)ins_addr;
-	p += skip_prefix(p, &shorted, &enlarged, &rexr);
+	p += skip_prefix(p, &prf);
 	p += get_opcode(p, &opcode);
 
 	CHECK_OP_TYPE(opcode, reg_rop, REG_READ);

@@ -156,10 +165,11 @@ static unsigned int get_ins_reg_width(unsigned long ins_addr)
 {
 	unsigned int opcode;
 	unsigned char *p;
-	int i, shorted, enlarged, rexr;
+	struct prefix_bits prf;
+	int i;
 
 	p = (unsigned char *)ins_addr;
-	p += skip_prefix(p, &shorted, &enlarged, &rexr);
+	p += skip_prefix(p, &prf);
 	p += get_opcode(p, &opcode);
 
 	for (i = 0; i < ARRAY_SIZE(rw8); i++)

@@ -168,7 +178,7 @@ static unsigned int get_ins_reg_width(unsigned long ins_addr)
 
 	for (i = 0; i < ARRAY_SIZE(rw32); i++)
 		if (rw32[i] == opcode)
-			return (shorted ? 2 : (enlarged ? 8 : 4));
+			return (prf.shorted ? 2 : (prf.enlarged ? 8 : 4));
 
 	printk(KERN_ERR "mmiotrace: Unknown opcode 0x%02x\n", opcode);
 	return 0;

@@ -178,10 +188,11 @@ unsigned int get_ins_mem_width(unsigned long ins_addr)
 {
 	unsigned int opcode;
 	unsigned char *p;
-	int i, shorted, enlarged, rexr;
+	struct prefix_bits prf;
+	int i;
 
 	p = (unsigned char *)ins_addr;
-	p += skip_prefix(p, &shorted, &enlarged, &rexr);
+	p += skip_prefix(p, &prf);
 	p += get_opcode(p, &opcode);
 
 	for (i = 0; i < ARRAY_SIZE(mw8); i++)

@@ -194,11 +205,11 @@ unsigned int get_ins_mem_width(unsigned long ins_addr)
 
 	for (i = 0; i < ARRAY_SIZE(mw32); i++)
 		if (mw32[i] == opcode)
-			return shorted ? 2 : 4;
+			return prf.shorted ? 2 : 4;
 
 	for (i = 0; i < ARRAY_SIZE(mw64); i++)
 		if (mw64[i] == opcode)
-			return shorted ? 2 : (enlarged ? 8 : 4);
+			return prf.shorted ? 2 : (prf.enlarged ? 8 : 4);
 
 	printk(KERN_ERR "mmiotrace: Unknown opcode 0x%02x\n", opcode);
 	return 0;

@@ -238,7 +249,7 @@ enum {
 #endif
 };
 
-static unsigned char *get_reg_w8(int no, struct pt_regs *regs)
+static unsigned char *get_reg_w8(int no, int rex, struct pt_regs *regs)
 {
 	unsigned char *rv = NULL;
 

@@ -255,18 +266,6 @@ static unsigned char *get_reg_w8(int no, struct pt_regs *regs)
 	case arg_DL:
 		rv = (unsigned char *)&regs->dx;
 		break;
-	case arg_AH:
-		rv = 1 + (unsigned char *)&regs->ax;
-		break;
-	case arg_BH:
-		rv = 1 + (unsigned char *)&regs->bx;
-		break;
-	case arg_CH:
-		rv = 1 + (unsigned char *)&regs->cx;
-		break;
-	case arg_DH:
-		rv = 1 + (unsigned char *)&regs->dx;
-		break;
 #ifdef __amd64__
 	case arg_R8:
 		rv = (unsigned char *)&regs->r8;

@@ -294,9 +293,55 @@ static unsigned char *get_reg_w8(int no, struct pt_regs *regs)
 		break;
 #endif
 	default:
-		printk(KERN_ERR "mmiotrace: Error reg no# %d\n", no);
 		break;
 	}
+
+	if (rv)
+		return rv;
+
+	if (rex) {
+		/*
+		 * If REX prefix exists, access low bytes of SI etc.
+		 * instead of AH etc.
+		 */
+		switch (no) {
+		case arg_SI:
+			rv = (unsigned char *)&regs->si;
+			break;
+		case arg_DI:
+			rv = (unsigned char *)&regs->di;
+			break;
+		case arg_BP:
+			rv = (unsigned char *)&regs->bp;
+			break;
+		case arg_SP:
+			rv = (unsigned char *)&regs->sp;
+			break;
+		default:
+			break;
+		}
+	} else {
+		switch (no) {
+		case arg_AH:
+			rv = 1 + (unsigned char *)&regs->ax;
+			break;
+		case arg_BH:
+			rv = 1 + (unsigned char *)&regs->bx;
+			break;
+		case arg_CH:
+			rv = 1 + (unsigned char *)&regs->cx;
+			break;
+		case arg_DH:
+			rv = 1 + (unsigned char *)&regs->dx;
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (!rv)
+		printk(KERN_ERR "mmiotrace: Error reg no# %d\n", no);
+
 	return rv;
 }
 

@@ -368,11 +413,12 @@ unsigned long get_ins_reg_val(unsigned long ins_addr, struct pt_regs *regs)
 	unsigned char mod_rm;
 	int reg;
 	unsigned char *p;
-	int i, shorted, enlarged, rexr;
+	struct prefix_bits prf;
+	int i;
 	unsigned long rv;
 
 	p = (unsigned char *)ins_addr;
-	p += skip_prefix(p, &shorted, &enlarged, &rexr);
+	p += skip_prefix(p, &prf);
 	p += get_opcode(p, &opcode);
 	for (i = 0; i < ARRAY_SIZE(reg_rop); i++)
 		if (reg_rop[i] == opcode) {

@@ -392,10 +438,10 @@ unsigned long get_ins_reg_val(unsigned long ins_addr, struct pt_regs *regs)
 
 do_work:
 	mod_rm = *p;
-	reg = ((mod_rm >> 3) & 0x7) | (rexr << 3);
+	reg = ((mod_rm >> 3) & 0x7) | (prf.rexr << 3);
 	switch (get_ins_reg_width(ins_addr)) {
 	case 1:
-		return *get_reg_w8(reg, regs);
+		return *get_reg_w8(reg, prf.rex, regs);
 
 	case 2:
 		return *(unsigned short *)get_reg_w32(reg, regs);

@@ -422,11 +468,12 @@ unsigned long get_ins_imm_val(unsigned long ins_addr)
 	unsigned char mod_rm;
 	unsigned char mod;
 	unsigned char *p;
-	int i, shorted, enlarged, rexr;
+	struct prefix_bits prf;
+	int i;
 	unsigned long rv;
 
 	p = (unsigned char *)ins_addr;
-	p += skip_prefix(p, &shorted, &enlarged, &rexr);
+	p += skip_prefix(p, &prf);
 	p += get_opcode(p, &opcode);
 	for (i = 0; i < ARRAY_SIZE(imm_wop); i++)
 		if (imm_wop[i] == opcode) {
Signature · 1 line
1.5.4.5
Applied
#5 lore raw

Re: [PATCH] x86: fix mmiotrace 8-bit register decoding

Quoted 8 lines · Pekka Paalanen

From 8321edde04244cc91470da5a7a71b79b704b0da1 Mon Sep 17 00:00:00 2001
From: Pekka Paalanen <[email protected]>
Date: Mon, 21 Jul 2008 18:49:56 +0300
Subject: [PATCH] x86: fix mmiotrace 8-bit register decoding

When SIL, DIL, BPL or SPL registers were used in MMIO, the datum was extracted from AH, BH, CH, or DH, which are incorrect.

Quoted 7 lines
Signed-off-by Pekka Paalanen <[email protected]>

---

This patch was cooked in Linus' tree because I couldn't get linux-next to boot properly at the time. It applies cleanly to tip/master though. Another problem I noticed is with non-page-aligned mappings, but that's a different story.

applied to tip/tracing/mmiotrace - thanks Pekka!

Ingo

Applied
#6 lore raw
In reply to Pekka Paalanen
Quoted 19 lines · Pekka Paalanen

The moment mmiotrace is enabled, I hit a NULL deref in:

IP: [<ffffffff80256e71>] __trace_special+0x17c/0x23a

Call Trace:
 [<ffffffff802573cc>] ftrace_special+0x6f/0x9a
 [<ffffffff8023e3e4>] down+0x19/0x4a
 [<ffffffff80228adc>] acquire_console_sem+0x42/0x58
 [<ffffffff8035d273>] con_flush_chars+0x28/0x43
 [<ffffffff80354a70>] write_chan+0x22e/0x334
 [<ffffffff802244e9>] ? default_wake_function+0x0/0xf
 [<ffffffff8035236d>] tty_write+0x195/0x228
 [<ffffffff80354842>] ? write_chan+0x0/0x334
 [<ffffffff8027c23a>] vfs_write+0xae/0x137
 [<ffffffff8027c6e3>] sys_write+0x47/0x70
 [<ffffffff8020b1db>] system_call_after_swapgs+0x7b/0x80

which means 'entry' in __trace_special() is NULL.

kernel/semaphore.c void down(struct semaphore *sem) · line 57

Quoted 2 lines
kernel/semaphore.c +0 -1
-	ftrace_special(sem->count, 0, __LINE__);
 	spin_lock_irqsave(&sem->lock, flags);

applied to tip/tracing/ftrace - thanks Pekka. This was a leftover line from semaphore debugging.

Ingo