[1/2] 2.6.22-rc7: known regressions

[1/2] 2.6.22-rc7: known regressions

[Michal Piotrowski]

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 <randy.dunlap@oracle.com>
Handled-By : Vladimir V. Saveliev <vs@namesys.com>
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 <dgc@sgi.com>
Status     : unknown



Sparc64

Subject    : random invalid instruction occourances on sparc32 (sun4c)
References : http://lkml.org/lkml/2007/6/17/111
Submitter  : Mark Fortescue <mark@mtfhpc.demon.co.uk>
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 <mikpe@it.uu.se>
Handled-By : David Miller <davem@davemloft.net>
Status     : problem is being debugged



Regards,
Michal

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

Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2404 bytes --]

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.

On Tue, 3 Jul 2007, Michal Piotrowski wrote:

> 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 <randy.dunlap@oracle.com>
> Handled-By : Vladimir V. Saveliev <vs@namesys.com>
> 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 <dgc@sgi.com>
> Status     : unknown
>
>
>
> Sparc64
>
> Subject    : random invalid instruction occourances on sparc32 (sun4c)
> References : http://lkml.org/lkml/2007/6/17/111
> Submitter  : Mark Fortescue <mark@mtfhpc.demon.co.uk>
> 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 <mikpe@it.uu.se>
> Handled-By : David Miller <davem@davemloft.net>
> Status     : problem is being debugged
>
>
>
> Regards,
> Michal
>
> --
> LOG
> http://www.stardust.webpages.pl/log/
> -
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed, Size: 3186 bytes --]

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.

On Tue, 3 Jul 2007, Mark Fortescue wrote:

> 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.
>
> On Tue, 3 Jul 2007, Michal Piotrowski wrote:
>
>> 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 <randy.dunlap@oracle.com>
>> Handled-By : Vladimir V. Saveliev <vs@namesys.com>
>> 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 <dgc@sgi.com>
>> Status     : unknown
>> 
>> 
>> 
>> Sparc64
>> 
>> Subject    : random invalid instruction occourances on sparc32 (sun4c)
>> References : http://lkml.org/lkml/2007/6/17/111
>> Submitter  : Mark Fortescue <mark@mtfhpc.demon.co.uk>
>> 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 <mikpe@it.uu.se>
>> Handled-By : David Miller <davem@davemloft.net>
>> Status     : problem is being debugged
>> 
>> 
>> 
>> Regards,
>> Michal
>> 
>> --
>> LOG
>> http://www.stardust.webpages.pl/log/
>> -
>> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2480 bytes --]

From: Mark Fortescue <mark@mtfhpc.demon.co.uk>

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) 

Signed-off-by: Mark Fortescue <mark@mtfhpc.demon.co.uk>
---
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.
--- linux-2.6/mm/slab.c	2007-07-03 17:35:07.000000000 +0100
+++ linux-test/mm/slab.c	2007-07-03 19:05:19.000000000 +0100
@@ -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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Tue, 2007-07-03 at 19:57 +0100, Mark Fortescue wrote:
> > 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.

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

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.

On Tue, 3 Jul 2007, David Woodhouse wrote:

> On Tue, 2007-07-03 at 19:57 +0100, Mark Fortescue wrote:
>>> 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.
>
> 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
>
> -
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Tue, 2007-07-03 at 22:25 +0100, Mark Fortescue wrote:
> 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.

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.

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

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?

diff --git a/mm/slab.c b/mm/slab.c
index 6d65cf4..3b15671 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

Hi David,

I will try out your patch shortly.

On Tue, 3 Jul 2007, David Woodhouse wrote:

> On Tue, 2007-07-03 at 22:25 +0100, Mark Fortescue wrote:
>> 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.
>
> 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.
>
>> 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.
>
> 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.
>

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.

> Does this fix it?
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 6d65cf4..3b15671 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -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
>
>

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Tue, 2007-07-03 at 23:47 +0100, Mark Fortescue wrote:
> Hi David,
> 
> I will try out your patch shortly.

Thanks.

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

Why do we need more than the existing:

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

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

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

diff --git a/mm/slab.c b/mm/slab.c
index a9c4472..8081c07 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

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.

--- linux-2.6/mm/slab.c	2007-07-03 19:09:48.000000000 +0100
+++ linux-test/mm/slab.c	2007-07-04 04:14:15.000000000 +0100
@@ -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.

On Tue, 3 Jul 2007, David Woodhouse wrote:

> On Tue, 2007-07-03 at 23:47 +0100, Mark Fortescue wrote:
>> Hi David,
>>
>> I will try out your patch shortly.
>
> Thanks.
>
>> 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.
>
> Why do we need more than the existing:
>
> 	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER)
> 		ralign = __alignof__(unsigned long long);
>
>> 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.
>
> 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...
>
> diff --git a/mm/slab.c b/mm/slab.c
> index a9c4472..8081c07 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -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
>
>

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Wed, 2007-07-04 at 04:27 +0100, Mark Fortescue wrote:
> 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.

That's almost certain to be an unrelated issue.

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

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

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.

  On Tue, 3 Jul 2007, David Woodhouse wrote:

> On Wed, 2007-07-04 at 04:27 +0100, Mark Fortescue wrote:
>> 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.
>
> That's almost certain to be an unrelated issue.
>
>> 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.
>
> 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
>
> -
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Wed, 2007-07-04 at 11:27 +0100, Mark Fortescue wrote:
> 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).

No, it's always 64 bits.

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

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 206 bytes --]

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.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3129 bytes --]

From: Mark Fortescue <mark@mtfhpc.demon.co.uk>, David Woodhouse <dwmw2@infradead.org>

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.

Signed-off-by: Mark Fortescue <mark@mtfhpc.demon.co.uk>
---
Tested on Sparc32:sun4c
Additional testing desirable on PowerPC32, Sparc64, PowerPC64 and any
other platforms where alignment changes may be of concirn.
--- linux-2.6/mm/slab.c	2007-07-03 19:09:48.000000000 +0100
+++ linux-test/mm/slab.c	2007-07-04 04:14:15.000000000 +0100
@@ -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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Tue, 03 Jul 2007 15:26:18 -0400

> On Tue, 2007-07-03 at 19:57 +0100, Mark Fortescue wrote:
> > > 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.
> 
> 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;

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.

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Tue, 2007-07-03 at 14:41 -0700, David Miller wrote:
> 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

Re: [1/2] 2.6.22-rc7: known regressions

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday 03 July 2007, Michal Piotrowski wrote:

> IDE
> 
> Subject    : 2.6.22-rcX: hda: lost interrupt
> References : http://lkml.org/lkml/2007/6/29/121
> Submitter  : David Chinner <dgc@sgi.com>
> Status     : unknown

David, any news on this one?

Have you tried libata as suggested by Jeff?

[ would exclude IRQ routing issue or broken hardware ]

Thanks,
Bart

Re: [1/2] 2.6.22-rc7: known regressions

[David Chinner]

On Tue, Jul 03, 2007 at 07:50:26PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Tuesday 03 July 2007, Michal Piotrowski wrote:
> 
> > IDE
> > 
> > Subject    : 2.6.22-rcX: hda: lost interrupt
> > References : http://lkml.org/lkml/2007/6/29/121
> > Submitter  : David Chinner <dgc@sgi.com>
> > Status     : unknown
> 
> David, any news on this one?
> 
> Have you tried libata as suggested by Jeff?

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

Re: [1/2] 2.6.22-rc7: known regressions

[David Woodhouse]

On Tue, 2007-07-03 at 18:45 +0200, Michal Piotrowski wrote:
> Subject    : random invalid instruction occourances on sparc32 (sun4c)
> References : http://lkml.org/lkml/2007/6/17/111
> Submitter  : Mark Fortescue <mark@mtfhpc.demon.co.uk>
> Status     : problem is being debugged 

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

[PATCH 2.6.22 REGRESSION] Fix slab redzone alignment

[David Woodhouse]

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.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/mm/slab.c b/mm/slab.c
index a9c4472..b344e67 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -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

Re: [1/2] 2.6.22-rc7: known regressions

[David Woodhouse]

On Tue, 2007-07-03 at 18:45 +0200, Michal Piotrowski wrote:
> Hi all,
> 
> Here is a list of some known regressions in 2.6.22-rc7.

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

Re: [1/2] 2.6.22-rc7: known regressions

[Linus Torvalds]

On Wed, 4 Jul 2007, David Woodhouse wrote:
> 
> Oh, and here's another one for you. My Bluetooth mouse just stopped
> working and hidd is deadlocked...

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

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

Re: [1/2] 2.6.22-rc7: known regressions

[David Woodhouse]

On Thu, 2007-07-05 at 09:28 -0700, Linus Torvalds wrote:
> 
> On Wed, 4 Jul 2007, David Woodhouse wrote:
> > 
> > Oh, and here's another one for you. My Bluetooth mouse just stopped
> > working and hidd is deadlocked...
> 
> 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?

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.

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

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

Re: [1/2] 2.6.22-rc7: known regressions

[David Woodhouse]

On Thu, 2007-07-05 at 09:28 -0700, Linus Torvalds wrote:
> 
> On Wed, 4 Jul 2007, David Woodhouse wrote:
> > 
> > Oh, and here's another one for you. My Bluetooth mouse just stopped
> > working and hidd is deadlocked...
> 
> Looks like it is stuck on hidp_session_sem.

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

Re: [1/2] 2.6.22-rc7: known regressions

[David Woodhouse]

On Thu, 2007-07-05 at 14:46 -0400, David Woodhouse wrote:
> 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.

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

Re: [1/2] 2.6.22-rc7: known regressions

[Linus Torvalds]

Looks input-related..

On Thu, 5 Jul 2007, David Woodhouse wrote:
> 
> 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

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

>  NIP [c001870c] strlen+0x4/0x18
>  LR [c0134fec] kobject_get_path+0x34/0xc4
>  Call Trace:
>  [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

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

Re: [1/2] 2.6.22-rc7: known regressions

[Dmitry Torokhov]

On 7/5/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> Looks input-related..
>
> On Thu, 5 Jul 2007, David Woodhouse wrote:
> >
> > 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
>
> 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):
>
> >  NIP [c001870c] strlen+0x4/0x18
> >  LR [c0134fec] kobject_get_path+0x34/0xc4
> >  Call Trace:
> >  [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
>
> 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?
>

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

Re: [1/2] 2.6.22-rc7: known regressions

[Jiri Kosina]

(CC list trimmed)

On Thu, 5 Jul 2007, Dmitry Torokhov wrote:

> > >  NIP [c001870c] strlen+0x4/0x18
> > >  LR [c0134fec] kobject_get_path+0x34/0xc4
> > >  Call Trace:
> > >  [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
[...]
> > 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?

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

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

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

Re: [1/2] 2.6.22-rc7: known regressions

[Dmitry Torokhov]

On Friday 06 July 2007 18:50, Jiri Kosina wrote:
> (CC list trimmed)
> 
> On Thu, 5 Jul 2007, Dmitry Torokhov wrote:
> 
> > > >  NIP [c001870c] strlen+0x4/0x18
> > > >  LR [c0134fec] kobject_get_path+0x34/0xc4
> > > >  Call Trace:
> > > >  [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
> [...]
> > > 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?
> 
> This should be automatically done by proper dev.parent setting of the 
> corresponding input device, as already mentioned by Dmitry.
> 
> > 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...
> 
> 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().
>

Ah, I missed that. It did not use to do that last time I looked closely there.
 
> 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.
>

This is for devices using boot protocol, not full HID as far as I understand.
 
> 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,
> 

-- 
Dmitry

Re: [1/2] 2.6.22-rc7: known regressions

[Jiri Kosina]

On Fri, 6 Jul 2007, Dmitry Torokhov wrote:

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

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.

diff --git a/drivers/input/input.c b/drivers/input/input.c
index ccd8aba..ed5adb4 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -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);

Re: [1/2] 2.6.22-rc7: known regressions

[David Woodhouse]

On Sat, 2007-07-07 at 03:05 +0200, Jiri Kosina wrote:
> On Fri, 6 Jul 2007, Dmitry Torokhov wrote:
> 
> > This is for devices using boot protocol, not full HID as far as I 
> > understand.
> 
> 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.

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

Re: [1/2] 2.6.22-rc7: known regressions

[David Woodhouse]

On Fri, 2007-07-06 at 21:28 -0400, David Woodhouse wrote:
> 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. 

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

--- net/bluetooth/hidp/core.c~	2007-07-06 21:34:25.000000000 -0400
+++ net/bluetooth/hidp/core.c	2007-07-06 22:06:48.000000000 -0400
@@ -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

Re: [1/2] 2.6.22-rc7: known regressions

[Marcel Holtmann]

Hi David,

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

looks good to me.

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [1/2] 2.6.22-rc7: known regressions

[Linus Torvalds]

On Sat, 7 Jul 2007, Marcel Holtmann wrote:
>
> > Marcel? Can we deregister the input devices earlier...?
> 
> looks good to me.
> 
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>

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

[PATCH] Fix use-after-free oops in Bluetooth HID.

[David Woodhouse]

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.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>

--- net/bluetooth/hidp/core.c~	2007-07-06 21:34:25.000000000 -0400
+++ net/bluetooth/hidp/core.c	2007-07-06 22:06:48.000000000 -0400
@@ -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

Re: [PATCH] Fix use-after-free oops in Bluetooth HID.

[Linus Torvalds]

On Sat, 7 Jul 2007, David Woodhouse wrote:
>
> When cleaning up HIDP sessions [...]

Thanks.

> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>

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

Re: [PATCH] Fix use-after-free oops in Bluetooth HID.

[Dmitry Torokhov]

On Saturday 07 July 2007 14:58, David Woodhouse wrote:
> 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.

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