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

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

[Pekka Paalanen]

On Mon, 30 Jun 2008 14:37:36 +0200
Ingo Molnar <mingo@elte.hu> 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


>From 79e6a8eaf73b778dc76eb3c55bb82b18817c34df Mon Sep 17 00:00:00 2001
From: Pekka Paalanen <pq@iki.fi>
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 <pq@iki.fi>
---
 kernel/semaphore.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/kernel/semaphore.c b/kernel/semaphore.c
index bbab232..62f30c2 100644
--- a/kernel/semaphore.c
+++ b/kernel/semaphore.c
@@ -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--;
-- 
1.5.4.5

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

[Vegard Nossum]

On Mon, Jun 30, 2008 at 10:48 PM, Pekka Paalanen <pq@iki.fi> wrote:
> On Mon, 30 Jun 2008 14:37:36 +0200
> Ingo Molnar <mingo@elte.hu> 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.

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

-- 
"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

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

[Ingo Molnar]

* Pekka Paalanen <pq@iki.fi> wrote:

> 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.

>  
> -	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

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

[Pekka Paalanen]

>From 8321edde04244cc91470da5a7a71b79b704b0da1 Mon Sep 17 00:00:00 2001
From: Pekka Paalanen <pq@iki.fi>
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 <pq@iki.fi>
---

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(-)

diff --git a/arch/x86/mm/pf_in.c b/arch/x86/mm/pf_in.c
index efa1911..4d0bf36 100644
--- a/arch/x86/mm/pf_in.c
+++ b/arch/x86/mm/pf_in.c
@@ -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) {
-- 
1.5.4.5

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

[Ingo Molnar]

* Pekka Paalanen <pq@iki.fi> wrote:

> From 8321edde04244cc91470da5a7a71b79b704b0da1 Mon Sep 17 00:00:00 2001
> From: Pekka Paalanen <pq@iki.fi>
> 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 <pq@iki.fi>
> ---
> 
> 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