2.6.22-rc7: known regressions

Hi all,

Here is a list of some known regressions in 2.6.22-rc7.

Feel free to add new regressions/remove fixed etc.
http://kernelnewbies.org/known_regressions

List of Aces

Name Regressions fixed since 21-Jun-2007

Hugh Dickins                           2
Andi Kleen                             1
Andrew Morton                          1
Benjamin Herrenschmidt                 1
Björn Steinbrink                       1
Bjorn Helgaas                          1
Jean Delvare                           1
Olaf Hering                            1
Siddha, Suresh B                       1
Trent Piepho                           1

Ville Syrjälä 1

FS

Subject : 2.6.22-rc4-git5 reiserfs: null ptr deref.
References : http://lkml.org/lkml/2007/6/13/322
Submitter : Randy Dunlap <[email protected]>

Status : problem is being debugged

IDE

Subject : 2.6.22-rcX: hda: lost interrupt
References : http://lkml.org/lkml/2007/6/29/121
Submitter : David Chinner <[email protected]>
Status : unknown

Sparc64

Subject : random invalid instruction occourances on sparc32 (sun4c)
References : http://lkml.org/lkml/2007/6/17/111
Submitter : Mark Fortescue <[email protected]>
Status : problem is being debugged

Subject : 2.6.22-rc broke X on Ultra5
References : http://lkml.org/lkml/2007/5/22/78
Submitter : Mikael Pettersson <[email protected]>

Status : problem is being debugged

Regards,
Michal

--
LOG
http://www.stardust.webpages.pl/log/

Hi all,

I think I have found the cause of the problem.

Commit b46b8f19c9cd435ecac4d9d12b39d78c137ecd66 partially fixed alignment issues but does not ensure that all 64bit alignment requirements of sparc32 are met. Tests have shown that the redzone2 word can become misallignd.

I am currently working on a posible fix.

Regards
Mark Fortescue.

Hi all,

I have tested a solution to the random invalid instruction occourances on sparc32 (sun4c). I have attached the patch as my mail client messes up patch files.

The problem apears to be an alignment error of the redzone2 word, caused by the userword not being forced onto a 64bit boundary. My solution is to increase the size of the user word (BYTES_PER_WORD) to 64bits in order to force the correct alignment of the user word (and hence the redzone2 word). My solution also works on platforms where sizeof(unsigned long long) is 128bits and requires 128bit alignment.

Regards
Mark Fortescue.

From: Mark Fortescue <[email protected]>

Verious alignment fixes in the SLAB alocator that increased the size of the RedZone words failed to ensure that RedZone word 2 is aligned on a 64bit boundary. This has resulted in random invalid instruction occourances on Sparc32 (sun4c).
By increasing the size of the User Word (BYTES_PER_WORD) to 64bits seams to ensure that correct alignment is maintained but assumes that:
sizeof (void *) <= sizeof (unsigned dlong long)

---
Alternative solutions would involve correcting the size caculations on lines 2175 to 2275 and may also involve additional changes to the calculations to get a pointer to the RedZone Word 2.

@@ -136,7 +136,8 @@
 #endif
 
 /* Shouldn't this be in a header file somewhere? */
-#define	BYTES_PER_WORD		sizeof(void *)
+/* Fix alignment of redzone2. Assumes sizeof (void*) <= sizeof (unsigned long long) */
+#define	BYTES_PER_WORD		sizeof(unsigned long long)
 
 #ifndef cache_line_size
 #define cache_line_size()	L1_CACHE_BYTES

@@ -538,7 +539,7 @@ static unsigned long long *dbg_redzone1(
 {
 	BUG_ON(!(cachep->flags & SLAB_RED_ZONE));
 	return (unsigned long long*) (objp + obj_offset(cachep) -
-				      sizeof(unsigned long long));
+				      BYTES_PER_WORD);
 }
 
 static unsigned long long *dbg_redzone2(struct kmem_cache *cachep, void *objp)

@@ -546,10 +547,9 @@ static unsigned long long *dbg_redzone2(
 	BUG_ON(!(cachep->flags & SLAB_RED_ZONE));
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
-					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      2 * BYTES_PER_WORD);
 	return (unsigned long long *) (objp + cachep->buffer_size -
-				       sizeof(unsigned long long));
+				       BYTES_PER_WORD);
 }
 
 static void **dbg_userword(struct kmem_cache *cachep, void *objp)

@@ -2256,8 +2256,8 @@ kmem_cache_create (const char *name, siz
 	 */
 	if (flags & SLAB_RED_ZONE) {
 		/* add space for red zone words */
-		cachep->obj_offset += sizeof(unsigned long long);
-		size += 2 * sizeof(unsigned long long);
+		cachep->obj_offset += BYTES_PER_WORD;
+		size += 2 * BYTES_PER_WORD;
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of

Oops, sorry about that. I'm not sure about your patch though -- I think I'd prefer to keep the redzone misaligned (and hence _right_ next to the real data), and just deal with it.

typedef unsigned long long __aligned__((BYTES_PER_WORD)) redzone_t;

dwmw2

Hi David,

The problem is that sun4c Sparc32 can't handle un-aligned variables so having a 64bit readzone word that is not aligned on a 64bit boundary is a problem.

In addition, having looked at the size calculations, it looks to me as if not all of them got updated to handle 64bit redzone words. This may be part of the problem. By making BYTES_PER_WORD 64bit aligned (Sparc32) this is nolonger an issue.

Regards
Mark Fortescue.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Surely, it can. You just have to tell the compiler that it's not properly aligned, and it'll emit code to cope. Hence the suggestion that you use 'unsigned long long __attribute__((aligned(BYTES_PER_WORD))'. But it's probably better just to make sure it remains aligned; you're right.

Really? Other than the alignment of the second redzone, what's wrong? Remember that the 'user word' is still not necessarily 64-bit. And in fact I suspect that's what is causing the problem -- your object _size_ will be aligned to 8 bytes, including the user word, and then we look for the second redzone word 12 bytes before the end of the object.

Does this fix it?

index 6d65cf4..3b15671 100644
@@ -547,7 +547,7 @@ static unsigned long long *dbg_redzone2(struct kmem_cache *cachep, void *objp)
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
 					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
 	return (unsigned long long *) (objp + cachep->buffer_size -
 				       sizeof(unsigned long long));
 }

@@ -2262,9 +2262,14 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of
-		 * the real object.
+		 * the real object. But if the second red zone must be
+		 * aligned 'better' than that, allow for it.
 		 */
-		size += BYTES_PER_WORD;
+		if (flags & SLAB_RED_ZONE
+		    && BYTES_PER_WORD < __alignof__(unsigned long long))
+			size += __alignof__(unsigned long long);
+		else
+			size += BYTES_PER_WORD;
 	}
 #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size

dwmw2

Hi David,

I will try out your patch shortly.

Yes, the user word is a 32bit word and this is part of the issue.

I may be wrong about the size calculations but if you take a look at lines 2174 to 2188 and 2207 to 2203, reading the comments suggest to me that these need to be changed to match the changes to the RedZone words. Failing to change these means that 32bit aligned access of the 64bit RedZone words is still posible and this will kill sun4c.

For the 64bit RedZone word to be 64bit aligned (required by sun4c), the User word must be 64bit aligned. I don't see where in your patch, this is enforced.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Thanks.

Why do we need more than the existing:

	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER)
		ralign = __alignof__(unsigned long long);

Where __alignof__(long long) > BYTES_PER_WORD my patch should lead to this layout (32-bit words):

    [ redzone1 bits 63-32 ]
    [ redzone1 bits 31-0  ]
    [    ... object ...   ]
    [    ... object ...   ]
    [ redzone2 bits 63-32 ]
    [ redzone2 bits 31-0  ]
    [        unused       ]
    [      user word      ]

The user word is a 32-bit value; there's no requirement for _it_ to be aligned.

Hm, actually I think my patch may be incomplete -- I need to adjust the size of the actual object too. This patch should be better...

index a9c4472..8081c07 100644
@@ -547,7 +547,7 @@ static unsigned long long *dbg_redzone2(struct kmem_cache *cachep, void *objp)
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
 					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
 	return (unsigned long long *) (objp + cachep->buffer_size -
 				       sizeof(unsigned long long));
 }

@@ -2223,8 +2223,11 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	 * overridden by architecture or caller mandated alignment if either
 	 * is greater than BYTES_PER_WORD.
 	 */
-	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER)
+	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER) {
 		ralign = __alignof__(unsigned long long);
+		size += (__alignof__(unsigned long long) - 1);
+		size &= ~(__alignof__(unsigned long long) - 1);
+	}
 
 	/* 2) arch mandated alignment */
 	if (ralign < ARCH_SLAB_MINALIGN) {

@@ -2261,9 +2264,14 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of
-		 * the real object.
+		 * the real object. But if the second red zone must be
+		 * aligned 'better' than that, allow for it.
 		 */
-		size += BYTES_PER_WORD;
+		if (flags & SLAB_RED_ZONE
+		    && BYTES_PER_WORD < __alignof__(unsigned long long))
+			size += __alignof__(unsigned long long);
+		else
+			size += BYTES_PER_WORD;
 	}
 #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size

dwmw2

Hi David,

I tried the previous patch and it looks like it fixes the issue however one of the test builds I did caused depmod to use up all available memory (40M - kernel memory) before taking out the kernel with the oom killer. At present, I do not know if it is a depmod issue or a kernel issue.
I will have to do some more tests later on to day.

I have looked at the latest patch below and am I am still not sure about two areas. Please take a look at my offering based on your latest patch (included here to it will probably get mangled).

Note the change to lines 2178 to 2185. I have also changed/moved the alignment of size (see lines 2197 to 2206) based on your changes.

@@ -137,6 +137,7 @@
 
 /* Shouldn't this be in a header file somewhere? */
 #define	BYTES_PER_WORD		sizeof(void *)
+#define RED_ZONE_ALIGN_MASK	(max(__alignof__(void *), __alignof(unsigned long long)) - 1)
 
 #ifndef cache_line_size
 #define cache_line_size()	L1_CACHE_BYTES

@@ -547,7 +548,7 @@ static unsigned long long *dbg_redzone2(
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
 					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
 	return (unsigned long long *) (objp + cachep->buffer_size -
 				       sizeof(unsigned long long));
 }

@@ -2178,7 +2179,8 @@ kmem_cache_create (const char *name, siz
 	 * above the next power of two: caches with object sizes just above a
 	 * power of two have a significant amount of internal fragmentation.
 	 */
-	if (size < 4096 || fls(size - 1) == fls(size-1 + 3 * BYTES_PER_WORD))
+	if (size < 4096 || fls(size - 1) == fls(size-1 + 2 * sizeof(unsigned long long) +
+						max(BYTES_PER_WORD, __alignof__(unsigned long long))))
 		flags |= SLAB_RED_ZONE | SLAB_STORE_USER;
 	if (!(flags & SLAB_DESTROY_BY_RCU))
 		flags |= SLAB_POISON;

@@ -2197,9 +2199,9 @@ kmem_cache_create (const char *name, siz
 	 * unaligned accesses for some archs when redzoning is used, and makes
 	 * sure any on-slab bufctl's are also correctly aligned.
 	 */
-	if (size & (BYTES_PER_WORD - 1)) {
-		size += (BYTES_PER_WORD - 1);
-		size &= ~(BYTES_PER_WORD - 1);
+	if (size & RED_ZONE_ALIGN_MASK) {
+		size += RED_ZONE_ALIGN_MASK;
+		size &= ~RED_ZONE_ALIGN_MASK;
 	}
 
 	/* calculate the final buffer alignment: */

@@ -2261,9 +2263,14 @@ kmem_cache_create (const char *name, siz
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of
-		 * the real object.
+		 * the real object. But if the second red zone must be
+		 * aligned 'better' than that, allow for it.
 		 */
-		size += BYTES_PER_WORD;
+		if (flags & SLAB_RED_ZONE
+		    && BYTES_PER_WORD < __alignof__(unsigned long long))
+			size += __alignof__(unsigned long long);
+		else
+			size += BYTES_PER_WORD;
 	}
 #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size

---

Let me know if you would like an un-mangled copy of the patch as an attachement.

Regards
Mark Fortescue.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

That's almost certain to be an unrelated issue.

The first looks correct; well spotted. The second is just cosmetic but also seems correct. I might be inclined to make the #define 'RED_ZONE_ALIGN' and use it in the other places too.

dwmw2

Hi David,

Another related point that may also need to be considered is that I think I am correct in saying that on ARM and on the 64bit platforms, sizeof (unsigned long long) is 16 (128bits).

Should the RedZone words be specified as __u64 not the unsigned long long used or does the alignment need to be that of unsigned long long ?

Regards
Mark Fortescue.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No, it's always 64 bits.

You have to play silly buggers with printk formats (%lx vs. %llx) if you do that. And you have to make a choice about using proper C types or the Linux-specific nonsense. 'unsigned long long' is just easier all round.

dwmw2

Hi David,

I have sucessfully tested the attached patch on Sparc32:sun4c.

Is there any chance of this getting sufficiantly tested on other architectures to get in into v2.6.22 ?

Regards
Mark Fortescue.

From: Mark Fortescue <[email protected]>, David Woodhouse <[email protected]>

Verious alignment fixes in the SLAB alocator that increased the size of the RedZone words failed to ensure that RedZone word 2 is aligned on a 64bit boundary. This resulted in random invalid instruction occourances on Sparc32 (sun4c).
These additional changes should ensure that the RedZone words are correctly aligned on a 64bit boundary for architectures that require this.

---
Tested on Sparc32:sun4c
Additional testing desirable on PowerPC32, Sparc64, PowerPC64 and any other platforms where alignment changes may be of concirn.

@@ -137,6 +137,7 @@
 
 /* Shouldn't this be in a header file somewhere? */
 #define	BYTES_PER_WORD		sizeof(void *)
+#define	RED_ZONE_ALIGN		(max(__alignof__(void *), __alignof(unsigned long long)) - 1)
 
 #ifndef cache_line_size
 #define cache_line_size()	L1_CACHE_BYTES

@@ -547,7 +548,7 @@ static unsigned long long *dbg_redzone2(
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
 					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
 	return (unsigned long long *) (objp + cachep->buffer_size -
 				       sizeof(unsigned long long));
 }

@@ -2178,7 +2179,8 @@ kmem_cache_create (const char *name, siz
 	 * above the next power of two: caches with object sizes just above a
 	 * power of two have a significant amount of internal fragmentation.
 	 */
-	if (size < 4096 || fls(size - 1) == fls(size-1 + 3 * BYTES_PER_WORD))
+	if (size < 4096 || fls(size - 1) == fls(size-1 + 2 * sizeof(unsigned long long) +
+						max(BYTES_PER_WORD, __alignof__(unsigned long long))))
 		flags |= SLAB_RED_ZONE | SLAB_STORE_USER;
 	if (!(flags & SLAB_DESTROY_BY_RCU))
 		flags |= SLAB_POISON;

@@ -2197,9 +2199,9 @@ kmem_cache_create (const char *name, siz
 	 * unaligned accesses for some archs when redzoning is used, and makes
 	 * sure any on-slab bufctl's are also correctly aligned.
 	 */
-	if (size & (BYTES_PER_WORD - 1)) {
-		size += (BYTES_PER_WORD - 1);
-		size &= ~(BYTES_PER_WORD - 1);
+	if (size & RED_ZONE_ALIGN) {
+		size += RED_ZONE_ALIGN;
+		size &= ~RED_ZONE_ALIGN;
 	}
 
 	/* calculate the final buffer alignment: */

@@ -2261,9 +2263,14 @@ kmem_cache_create (const char *name, siz
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of
-		 * the real object.
+		 * the real object. But if the second red zone must be
+		 * aligned 'better' than that, allow for it.
 		 */
-		size += BYTES_PER_WORD;
+		if (flags & SLAB_RED_ZONE
+		    && BYTES_PER_WORD < __alignof__(unsigned long long))
+			size += __alignof__(unsigned long long);
+		else
+			size += BYTES_PER_WORD;
 	}
 #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size

From: David Woodhouse <[email protected]>
Date: Tue, 03 Jul 2007 15:26:18 -0400

Please don't use get_unaligned() or whatever to fix this, it's going to generate the byte-at-a-time accesses on sparc64 which doesn't need it since the redzone will be aligned.

Yes, get_unaligned() would suck. But 'u64 __aligned__((BYTES_PER_WORD))' as I suggested should result in a single 64-bit load on 64-bit architectures, and two 32-bit loads on 32-bit architectures.

But I think the patch I just sent is a better option than that anyway.

dwmw2

Hi,

David, any news on this one?

Have you tried libata as suggested by Jeff?

[ would exclude IRQ routing issue or broken hardware ]

Thanks,
Bart

Not yet - I've been flat out and haven't got back to it yet. I'll try to get to it today.

Cheers,

Dave.

Dave Chinner
Principal Engineer
SGI Australian Software Group

Hm, when testing the fix for that on ppc32, I stupidly built with Slub instead, and got this...

radeonfb: Monitor 1 type LCD found
radeonfb: EDID probed
radeonfb: Monitor 2 type no found
radeonfb: Using Firmware dividers 0x00040080 from PPLL 0
radeonfb: Dynamic Clock Power Management enabled
*** SLUB kmalloc-32768: Poison check failed@0xc1e20000 slab 0xc04de400 [Not tainted]

    offset=0 flags=0x40c3 inuse=0 freelist=0xc1e20000
    Object 0xc1e20000:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
    Object 0xc1e20010:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
    Object 0xc1e20020:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
    Object 0xc1e20030:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
    Object 0xc1e20040:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
    Object 0xc1e20050:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
    Object 0xc1e20060:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
    Object 0xc1e20070:  6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk

Redzone 0xc1e28000: bb bb bb bb <BB><BB><BB><BB>
FreePointer 0xc1e28004 -> 0x00000000
Last alloc: malloc+0x14/0x24 jiffies_ago=1382 cpu=0 pid=1 Last free : free+0x10/0x20 jiffies_ago=837 cpu=0 pid=1

    Filler 0xc1e28028:  5a 5a 5a 5a 5a 5a 5a 5a                         ZZZZZZZZ        
Call Trace:
[effc7b80] [c000893c] show_stack+0x50/0x184 (unreliable)
[effc7ba0] [c009705c] object_err+0x178/0x18c
[effc7bc0] [c0097380] check_object+0x180/0x2ec
[effc7be0] [c0098320] __slab_alloc+0x5c8/0x5f4
[effc7c10] [c0098aa4] __kmalloc+0x64/0x9c
[effc7c30] [c015f5dc] fbcon_startup+0x154/0x2c0
[effc7c60] [c01bb8ec] register_con_driver+0x94/0x164
[effc7c90] [c01bedc8] take_over_console+0x24/0x58
[effc7cb0] [c015b41c] fbcon_takeover+0x8c/0xec
[effc7cc0] [c015d31c] fbcon_event_notify+0x1e0/0x6c8
[effc7d90] [c02d9490] notifier_call_chain+0x3c/0x94
[effc7db0] [c0045468] __blocking_notifier_call_chain+0x50/0x74
[effc7dd0] [c014f514] fb_notifier_call_chain+0x24/0x34
[effc7de0] [c0150590] register_framebuffer+0x190/0x1a8
[effc7e40] [c0185450] radeonfb_pci_register+0xe54/0xf50
[effc7e70] [c0145b04] pci_device_probe+0x6c/0xa0
[effc7e90] [c01d4108] driver_probe_device+0xfc/0x1a0
[effc7eb0] [c01d436c] __driver_attach+0xac/0x110
[effc7ed0] [c01d32f0] bus_for_each_dev+0x50/0x94
[effc7f00] [c01d3efc] driver_attach+0x24/0x34
[effc7f10] [c01d3710] bus_add_driver+0x78/0x1a0
[effc7f30] [c01d468c] driver_register+0x88/0x9c
[effc7f40] [c0145900] __pci_register_driver+0x6c/0xb8
[effc7f60] [c03e8e4c] radeonfb_init+0x20c/0x220
[effc7f80] [c03c82e4] kernel_init+0xc8/0x284
[effc7ff0] [c0013e28] kernel_thread+0x44/0x60

@@@ SLUB kmalloc-32768: Restoring Poison (0x6b) from 0xc1e20000-0xc1e27ffe @@@ SLUB kmalloc-32768: Restoring Poison (0xa5) from 0xc1e27fff-0xc1e27fff @@@ SLUB: kmalloc-32768 slab 0xc04de400. Marking all objects used.
Console: switching to colour frame buffer device 180x56

dwmw2

Commit b46b8f19c9cd435ecac4d9d12b39d78c137ecd66 fixed a couple of bugs by switching the redzone to 64 bits. Unfortunately, it neglected to ensure that the _second_ redzone, after the slab object, is aligned correctly. This caused illegal instruction faults on sparc32, which for some reason not entirely clear to me are not trapped and fixed up.

Two things need to be done to fix this:
- increase the object size, rounding up to alignof(long long) so that the second redzone can be aligned correctly.
- If SLAB_STORE_USER is set but alignof(long long)==8, allow a full 64 bits of space for the user word at the end of the buffer, even though we may not _use_ the whole 64 bits.

This patch should be a no-op on any 64-bit architecture or any 32-bit architecture where alignof(long long) == 4. Of the others, it's tested on ppc32 by myself and a very similar patch was tested on sparc32 by Mark Fortescue, who reported the new problem.

Also, fix the conditions for FORCED_DEBUG, which hadn't been adjusted to the new sizes. Again noticed by Mark.

index a9c4472..b344e67 100644
@@ -137,6 +137,7 @@
 
 /* Shouldn't this be in a header file somewhere? */
 #define	BYTES_PER_WORD		sizeof(void *)
+#define	REDZONE_ALIGN		max(BYTES_PER_WORD, __alignof__(unsigned long long))
 
 #ifndef cache_line_size
 #define cache_line_size()	L1_CACHE_BYTES

@@ -547,7 +548,7 @@ static unsigned long long *dbg_redzone2(struct kmem_cache *cachep, void *objp)
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
 					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      REDZONE_ALIGN);
 	return (unsigned long long *) (objp + cachep->buffer_size -
 				       sizeof(unsigned long long));
 }

@@ -2178,7 +2179,8 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	 * above the next power of two: caches with object sizes just above a
 	 * power of two have a significant amount of internal fragmentation.
 	 */
-	if (size < 4096 || fls(size - 1) == fls(size-1 + 3 * BYTES_PER_WORD))
+	if (size < 4096 || fls(size - 1) == fls(size-1 + REDZONE_ALIGN +
+						2 * sizeof(unsigned long long)))
 		flags |= SLAB_RED_ZONE | SLAB_STORE_USER;
 	if (!(flags & SLAB_DESTROY_BY_RCU))
 		flags |= SLAB_POISON;

@@ -2219,12 +2221,20 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	}
 
 	/*
-	 * Redzoning and user store require word alignment. Note this will be
-	 * overridden by architecture or caller mandated alignment if either
-	 * is greater than BYTES_PER_WORD.
+	 * Redzoning and user store require word alignment or possibly larger.
+	 * Note this will be overridden by architecture or caller mandated
+	 * alignment if either is greater than BYTES_PER_WORD.
 	 */
-	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER)
-		ralign = __alignof__(unsigned long long);
+	if (flags & SLAB_STORE_USER)
+		ralign = BYTES_PER_WORD;
+
+	if (flags & SLAB_RED_ZONE) {
+		ralign = REDZONE_ALIGN;
+		/* If redzoning, ensure that the second redzone is suitably
+		 * aligned, by adjusting the object size accordingly. */
+		size += REDZONE_ALIGN - 1;
+		size &= ~(REDZONE_ALIGN - 1);
+	}
 
 	/* 2) arch mandated alignment */
 	if (ralign < ARCH_SLAB_MINALIGN) {

@@ -2261,9 +2271,13 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of
-		 * the real object.
+		 * the real object. But if the second red zone needs to be
+		 * aligned to 64 bits, we must allow that much space.
 		 */
-		size += BYTES_PER_WORD;
+		if (flags & SLAB_RED_ZONE)
+			size += REDZONE_ALIGN;
+		else
+			size += BYTES_PER_WORD;
 	}
 #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size

dwmw2

Oh, and here's another one for you. My Bluetooth mouse just stopped working and hidd is deadlocked...

hidd D 1FE27798 5940 1695 1 (NOTLB)

Call Trace:

[ef3ddb70] [00000004] 0x4 (unreliable)
[ef3ddc30] [c0008e7c] __switch_to+0x50/0x68 [ef3ddc50] [c02d5998] schedule+0x3cc/0x480 [ef3ddc80] [c0137a20] rwsem_down_failed_common+0x1c4/0x1f4 [ef3ddcb0] [c02d7454] rwsem_down_write_failed+0x28/0x40 [ef3ddce0] [c004ff60] down_write+0x50/0x64 [ef3ddd00] [f27f2068] hidp_add_connection+0x168/0x75c [hidp] [ef3ddd40] [f27f2e44] hidp_sock_ioctl+0x140/0x414 [hidp] [ef3ddeb0] [c024da6c] sock_ioctl+0x248/0x284 [ef3dded0] [c00ab02c] do_ioctl+0x38/0x84 [ef3ddee0] [c00ab448] vfs_ioctl+0x3d0/0x404 [ef3ddf10] [c00ab4e4] sys_ioctl+0x68/0x98

dwmw2

Looks like it is stuck on hidp_session_sem.

Nothing after 2.6.21 seems to have even touched that semaphore usage, and in fact there's not a whole lot of changes to the hidp code at all (and none of them look even remotely interesting).

So I suspect it's something lower down in the bluetooth stack, or it's a long-standing problem that you are somehow able to trigger more easily now. Is it consistent?

Can you showo the traces for the _other_ processes that are in bluetooth functions? Because there should be other processes there, holding that hidp_session_sem rwsem.

[ Alternatively, there is some process that doesn't release it in an error
case, but that is definitely not a regression if so: the changes to net/bluetooth/hidp/core.c since 2.6.21 really are trivial. ]

IOW, more info needed, I think.

Linus

---

It happened twice before I gave up on my 2.6.22-rc7 test kernel and went back to something earlier. I suppose I should double-check that it wasn't my slab changes, but I really don't think that's it.

There was nothing, apart from a later 'hidd -l' which got stuck on the same semaphore. I have an hcidump of it happening, at
http://david.woodhou.se/hidd-lockup-dump.txt -- it doesn't seem particularly enlightening though. There's just a disconnection and reconnect, as happens quite frequently with this mouse, and then we're deadlocked. I'll build with hidp debugging.

dwmw2

Oh, I suck. I failed to noticed that it had oopsed earlier, in slab debugging. I shall look at my 'obviously correct' slab patch a little harder, now that I'm not distracted by the fireworks.

dwmw2

Hm, it's not something new. It's an oops I saw occasionally in 2.6.21-rc too, whenever we had CONFIG_SYSFS_DEPRECATED set.

 Unable to handle kernel paging request for data at address 0x6b6b6b6b
 Faulting instruction address: 0xc001870c
 Oops: Kernel access of bad area, sig: 11 [#1]
 PowerMac
 Modules linked in: radeon(U) drm(U) hidp(U) hci_usb(U) rfcomm(U) l2cap(U) bluetooth(U) ipv6(U) nls_utf8(U) hfsplus(U) dm_mirror(U) dm_mod(U) therm_adt746x(U) parport_pc(U) lp(U) parport(U) loop(U) snd_aoa_i2sbus(U) snd_powermac(U) snd_seq_dummy(U) snd_seq_oss(U) snd_seq_midi_event(U) snd_seq(U) ide_cd(U) cdrom(U) snd_seq_device(U) pmac_zilog(U) snd_pcm_oss(U) snd_mixer_oss(U) snd_pcm(U) snd_timer(U) snd_page_alloc(U) snd(U) soundcore(U) snd_aoa_soundbus(U) firewire_ohci(U) firewire_core(U) crc_itu_t(U) sungem(U) sungem_phy(U) bcm43xx(U) ieee80211softmac(U) ieee80211(U) ieee80211_crypt(U) ext3(U) jbd(U) mbcache(U) ehci_hcd(U) ohci_hcd(U) uhci_hcd(U)
 NIP: c001870c LR: c0134fec CTR: c01d5078
 REGS: eed5bdc0 TRAP: 0300   Not tainted  (2.6.22-0.9.rc7.git3.fc8)
 MSR: 00009032 <EE,ME,IR,DR>  CR: 22000224  XER: 20000000
 DAR: 6b6b6b6b, DSISR: 40000000
 TASK = ef72a950[1753] 'khidpd_00000000' THREAD: eed5a000
 GPR00: 6b6b6b6b eed5be70 ef72a950 6b6b6b6b 6b6b6b6a ef18a3a0 0000001a ec8701c6 
 GPR08: 000007aa 00000014 00000000 00000005 00000000 2002163c 100d0000 00000000 
 GPR16: 00000000 7fb9a006 00000003 00000000 ee12cb08 c038cf20 ec870170 c037bf38 
 GPR24: ef6bab08 ec8701c6 0000001a 000007aa 00000001 ef6babb0 ef6babb0 000000d0 
 NIP [c001870c] strlen+0x4/0x18
 LR [c0134fec] kobject_get_path+0x34/0xc4
 Call Trace:
 [eed5be70] [c0098030] __kmalloc_track_caller+0x13c/0x164 (unreliable)
 [eed5be90] [c01d5124] class_uevent+0xac/0x1bc
 [eed5bed0] [c01357e4] kobject_uevent_env+0x23c/0x460
 [eed5bf20] [c01d485c] class_device_del+0x178/0x1a0
 [eed5bf40] [c01d489c] class_device_unregister+0x18/0x30
 [eed5bf60] [c021f820] input_unregister_device+0xf4/0x130
 [eed5bf70] [c0242f4c] hidinput_disconnect+0x2c/0x60
 [eed5bf90] [f27f2bac] hidp_session+0x550/0x584 [hidp]
 [eed5bff0] [c0013e28] kernel_thread+0x44/0x60
 Instruction dump:
 4082fff4 4e800020 38a3ffff 3884ffff 8c650001 2c830000 8c040001 7c601851 
 4d860020 4182ffec 4e800020 3883ffff <8c040001> 2c000000 4082fff8 7c632050 

dwmw2

Looks input-related..

Ok, that 0x6b is obviously the kfree() poisoning, ie it looks like a use-after-free problem with a pointer being loaded from a structure that had been free'd-

And the trace seems to be (ignore the unreliable one):

Where we have a few missing functions due to inlining, ie the real sequence seems to be:

class_device_del ->
kobject_uevent_env ->

    class_uevent ->
      kobject_get_path ->
	get_kobj_path_length ->
	  parent = kobj;
	  do {
	    strlen(parent->k_name /* kobject_name(parent) */);
	    parent = parent->parent;
	  } while (parent);

so either the kobj or one of it's parents had already been freed when it was unregistered due to the disconnect.

I'm not seeing any reference counting or other protection for the device ("input") on "hid->inputs" list. But I don't know the code. Dmitry? Jiri?

Linus

In hidinput_connect we do:

         input_dev->dev.parent = hid->dev;

This should pin hid object untill all inputs are released. However bluetooth does not use driver model and does not have hid->dev set up and so it looks like we are simply trying to unregister an input device that is already gone... I still don't quite get how we unregister the same device twice - it is done from a per-hid-device thread in hidp...

Dmitry

(CC list trimmed)

[...]

This should be automatically done by proper dev.parent setting of the corresponding input device, as already mentioned by Dmitry.

Actually even bluetooth HID seems to set up hid->dev correctly in hidp_setup_hid() and also sets properly dev.parent in hidp_setup_input().

Marcel, what is please the point behind this code in hidp_add_connection():

        if (!session->hid) {
                session->input = input_allocate_device();
                if (!session->input) {
                        kfree(session);
                        return -ENOMEM;
                }
        }

I suspect that the oops happens during freeing this extra device which is not allocated inside hid core, but I can't immediately see from the code what is the exact purpose of this 'extra' input device and why don't the input devices allocated and registered in hidinput_connect() suffice? usbhid doesn't need to register any extra input devices, everything is handled solely in hid-input core.

Seems like it could be triggered by ioctl() with ca->req->rd_size set to 0, but I am not that familiar with the bluetooth code to see it immediately.

Thanks,

Jiri Kosina

Ah, I missed that. It did not use to do that last time I looked closely there.

This is for devices using boot protocol, not full HID as far as I understand.

Dmitry

David,

could you please try with the trivial patch below and report the output? I'd be interested to know whether we are by any chance really seeing double input_unregister_device() for the same input_dev somehow. Thanks.

index ccd8aba..ed5adb4 100644
@@ -1160,6 +1160,9 @@ void input_unregister_device(struct input_dev *dev)
 	struct input_handle *handle, *next;
 	int code;
 
+	printk(KERN_DEBUG "input_unregister_device for %p\n", dev);
+	dump_stack();
+
 	for (code = 0; code <= KEY_MAX; code++)
 		if (test_bit(code, dev->key))
 			input_report_key(dev, code, 0);

I suspect it's more likely that hci_conn_del_sysfs() is running and removing the object representing the ACL connection. That's what the input device's remove event is reporting as 'PHYSDEVPATH' on the occasions that it _doesn't_ oops.

dwmw2

Yes, that seems to be it. It happens when we hit the idle_timeout and the kernel tears down the connection. Adding a mdelay(5000) into hidp_session() just before calling input_unregister_device(), and hard-coding idle_to to 1 second, makes it nice and easy to reproduce...

Marcel? Can we deregister the input devices earlier...?

@@ -581,15 +581,6 @@ static int hidp_session(void *arg)
 
 	hidp_del_timer(session);
 
-	fput(session->intr_sock->file);
-
-	wait_event_timeout(*(ctrl_sk->sk_sleep),
-		(ctrl_sk->sk_state == BT_CLOSED), msecs_to_jiffies(500));
-
-	fput(session->ctrl_sock->file);
-
-	__hidp_unlink_session(session);
-
 	if (session->input) {
 		input_unregister_device(session->input);
 		session->input = NULL;

@@ -601,6 +592,15 @@ static int hidp_session(void *arg)
 		hid_free_device(session->hid);
 	}
 
+	fput(session->intr_sock->file);
+
+	wait_event_timeout(*(ctrl_sk->sk_sleep),
+		(ctrl_sk->sk_state == BT_CLOSED), msecs_to_jiffies(500));
+
+	fput(session->ctrl_sock->file);
+
+	__hidp_unlink_session(session);
+
 	up_write(&hidp_session_sem);
 
 	kfree(session);

dwmw2

Hi David,

looks good to me.

Regards

Marcel

David, can you add an explanation about the thing (and your sign-off too), and I can apply it and get this issue out of the way?

Linus

When cleaning up HIDP sessions, we currently close the ACL connection before deregistering the input device. Closing the ACL connection schedules a workqueue to remove the associated objects from sysfs, but the input device still refers to them -- and if the workqueue happens to run before the input device removal, the kernel will oops when trying to look up PHYSDEVPATH for the removed input device.

Fix this by deregistering the input device before closing the connections.

@@ -581,15 +581,6 @@ static int hidp_session(void *arg)
 
 	hidp_del_timer(session);
 
-	fput(session->intr_sock->file);
-
-	wait_event_timeout(*(ctrl_sk->sk_sleep),
-		(ctrl_sk->sk_state == BT_CLOSED), msecs_to_jiffies(500));
-
-	fput(session->ctrl_sock->file);
-
-	__hidp_unlink_session(session);
-
 	if (session->input) {
 		input_unregister_device(session->input);
 		session->input = NULL;

@@ -601,6 +592,15 @@ static int hidp_session(void *arg)
 		hid_free_device(session->hid);
 	}
 
+	fput(session->intr_sock->file);
+
+	wait_event_timeout(*(ctrl_sk->sk_sleep),
+		(ctrl_sk->sk_state == BT_CLOSED), msecs_to_jiffies(500));
+
+	fput(session->ctrl_sock->file);
+
+	__hidp_unlink_session(session);
+
 	up_write(&hidp_session_sem);
 
 	kfree(session);

dwmw2

Thanks.

Btw, and totally unrelated to the patch itself..

I changed that second "Signed-off-by:" into an "Acked-by:", since that more accurately reflects what happened.

Not that it really matters, but it's nice to see what chain the patch actually took (from you to me - sign-off path), and independently of that see who looked at it and approved it (acked-by), and then often we additionally end up having a set of people who might have tested it ("Tested-by:") or maybe should just have been aware of it (the "Cc:") ones.

So the "Signed-off-by:" thing is one of many possible thigns that you can do, but it's somewhat special in that it has some "legal" meaning in that it is supposed to show the people who either wrote it or who forwarded it (possibly with changes, but quite often just unchanged).

Linus

I think it will work ok for 2.6.22 but I don't think this is a final
solution: input_unregister_device might not free the device right away. If there is a process that hangs on to one of the input interfaces (evdev or mousedev) then freeing of the device structure may be delayed and we may still run into the case when session is wiped out before device is freed.

Dmitry