"slab: Fix missing DEBUG_SLAB last user" breaks ARM

"slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Russell King]

commit 5c5e3b33 appears to break ARM thusly:

| Mount-cache hash table entries: 512
| slab error in verify_redzone_free(): cache `idr_layer_cache': memory outside object was overwritten
| Backtrace:               
| [<c0227088>] (dump_backtrace+0x0/0x110) from [<c0431afc>] (dump_stack+0x18/0x1c)
| [<c0431ae4>] (dump_stack+0x0/0x1c) from [<c0293304>] (__slab_error+0x28/0x30)
| [<c02932dc>] (__slab_error+0x0/0x30) from [<c0293a74>] (cache_free_debugcheck+0x1c0/0x2b8)               
| [<c02938b4>] (cache_free_debugcheck+0x0/0x2b8) from [<c0293f78>] (kmem_cache_free+0x3c/0xc0)             
| [<c0293f3c>] (kmem_cache_free+0x0/0xc0) from [<c032b1c8>] (ida_get_new_above+0x19c/0x1c0)                
| [<c032b02c>] (ida_get_new_above+0x0/0x1c0) from [<c02af7ec>] (alloc_vfsmnt+0x54/0x144)                   
| [<c02af798>] (alloc_vfsmnt+0x0/0x144) from [<c0299830>] (vfs_kern_mount+0x30/0xec)                       
| [<c0299800>] (vfs_kern_mount+0x0/0xec) from [<c0299908>] (kern_mount_data+0x1c/0x20)                     
| [<c02998ec>] (kern_mount_data+0x0/0x20) from [<c02146c4>] (sysfs_init+0x68/0xc8)
| [<c021465c>] (sysfs_init+0x0/0xc8) from [<c02137d4>] (mnt_init+0x90/0x1b0)
| [<c0213744>] (mnt_init+0x0/0x1b0) from [<c0213388>] (vfs_caches_init+0x100/0x140)
| [<c0213288>] (vfs_caches_init+0x0/0x140) from [<c0208c0c>] (start_kernel+0x2e8/0x368)                    
| [<c0208924>] (start_kernel+0x0/0x368) from [<c0208034>] (__enable_mmu+0x0/0x2c)
| c0113268: redzone 1:0xd84156c5c032b3ac, redzone 2:0xd84156c5635688c0.
| slab error in cache_alloc_debugcheck_after(): cache `idr_layer_cache': double free, or memory outside object was overwritten
| ...
| c011307c: redzone 1:0x9f91102ffffffff, redzone 2:0x9f911029d74e35b
| slab: Internal list corruption detected in cache 'idr_layer_cache'(24), slabp c0113000(16). Hexdump:     
|                          
| 000: 20 4f 10 c0 20 4f 10 c0 7c 00 00 00 7c 30 11 c0
| 010: 10 00 00 00 10 00 00 00 00 00 c9 17 fe ff ff ff
| 020: fe ff ff ff fe ff ff ff fe ff ff ff fe ff ff ff
| 030: fe ff ff ff fe ff ff ff fe ff ff ff fe ff ff ff
| 040: fe ff ff ff fe ff ff ff fe ff ff ff fe ff ff ff
| 050: fe ff ff ff fe ff ff ff fe ff ff ff 11 00 00 00
| 060: 12 00 00 00 13 00 00 00 14 00 00 00 15 00 00 00
| 070: 16 00 00 00 17 00 00 00 c0 88 56 63
| kernel BUG at /home/rmk/git/linux-2.6-rmk/mm/slab.c:2928!

This hasn't been noticed as I guess not many people use SLAB on ARM
anymore, and even less people probably have SLAB debugging enabled.
With SLAB debugging disabled, the system appears to behave correctly -
or maybe the problem is just hidden.

--
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Pekka Enberg]

Hi!

On Mon, Feb 7, 2011 at 8:06 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> commit 5c5e3b33 appears to break ARM thusly:
>
> | Mount-cache hash table entries: 512
> | slab error in verify_redzone_free(): cache `idr_layer_cache': memory outside object was overwritten
> | Backtrace:
> | [<c0227088>] (dump_backtrace+0x0/0x110) from [<c0431afc>] (dump_stack+0x18/0x1c)
> | [<c0431ae4>] (dump_stack+0x0/0x1c) from [<c0293304>] (__slab_error+0x28/0x30)
> | [<c02932dc>] (__slab_error+0x0/0x30) from [<c0293a74>] (cache_free_debugcheck+0x1c0/0x2b8)
> | [<c02938b4>] (cache_free_debugcheck+0x0/0x2b8) from [<c0293f78>] (kmem_cache_free+0x3c/0xc0)
> | [<c0293f3c>] (kmem_cache_free+0x0/0xc0) from [<c032b1c8>] (ida_get_new_above+0x19c/0x1c0)
> | [<c032b02c>] (ida_get_new_above+0x0/0x1c0) from [<c02af7ec>] (alloc_vfsmnt+0x54/0x144)
> | [<c02af798>] (alloc_vfsmnt+0x0/0x144) from [<c0299830>] (vfs_kern_mount+0x30/0xec)
> | [<c0299800>] (vfs_kern_mount+0x0/0xec) from [<c0299908>] (kern_mount_data+0x1c/0x20)
> | [<c02998ec>] (kern_mount_data+0x0/0x20) from [<c02146c4>] (sysfs_init+0x68/0xc8)
> | [<c021465c>] (sysfs_init+0x0/0xc8) from [<c02137d4>] (mnt_init+0x90/0x1b0)
> | [<c0213744>] (mnt_init+0x0/0x1b0) from [<c0213388>] (vfs_caches_init+0x100/0x140)
> | [<c0213288>] (vfs_caches_init+0x0/0x140) from [<c0208c0c>] (start_kernel+0x2e8/0x368)
> | [<c0208924>] (start_kernel+0x0/0x368) from [<c0208034>] (__enable_mmu+0x0/0x2c)
> | c0113268: redzone 1:0xd84156c5c032b3ac, redzone 2:0xd84156c5635688c0.
> | slab error in cache_alloc_debugcheck_after(): cache `idr_layer_cache': double free, or memory outside object was overwritten
> | ...
> | c011307c: redzone 1:0x9f91102ffffffff, redzone 2:0x9f911029d74e35b
> | slab: Internal list corruption detected in cache 'idr_layer_cache'(24), slabp c0113000(16). Hexdump:
> |
> | 000: 20 4f 10 c0 20 4f 10 c0 7c 00 00 00 7c 30 11 c0
> | 010: 10 00 00 00 10 00 00 00 00 00 c9 17 fe ff ff ff
> | 020: fe ff ff ff fe ff ff ff fe ff ff ff fe ff ff ff
> | 030: fe ff ff ff fe ff ff ff fe ff ff ff fe ff ff ff
> | 040: fe ff ff ff fe ff ff ff fe ff ff ff fe ff ff ff
> | 050: fe ff ff ff fe ff ff ff fe ff ff ff 11 00 00 00
> | 060: 12 00 00 00 13 00 00 00 14 00 00 00 15 00 00 00
> | 070: 16 00 00 00 17 00 00 00 c0 88 56 63
> | kernel BUG at /home/rmk/git/linux-2.6-rmk/mm/slab.c:2928!
>
> This hasn't been noticed as I guess not many people use SLAB on ARM
> anymore, and even less people probably have SLAB debugging enabled.
> With SLAB debugging disabled, the system appears to behave correctly -
> or maybe the problem is just hidden.

I thought the fix was for ARM, actually, but unfortunately the
changelog is somewhat terse (shame on me). Hmm?

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Russell King]

On Tue, Feb 08, 2011 at 07:44:56AM +0200, Pekka Enberg wrote:
> Hi!
> 
> On Mon, Feb 7, 2011 at 8:06 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> > commit 5c5e3b33 appears to break ARM thusly:
> >
> > | Mount-cache hash table entries: 512
> > | slab error in verify_redzone_free(): cache `idr_layer_cache': memory outside object was overwritten
> > | Backtrace:
> > | [<c0227088>] (dump_backtrace+0x0/0x110) from [<c0431afc>] (dump_stack+0x18/0x1c)
> > | [<c0431ae4>] (dump_stack+0x0/0x1c) from [<c0293304>] (__slab_error+0x28/0x30)
> > | [<c02932dc>] (__slab_error+0x0/0x30) from [<c0293a74>] (cache_free_debugcheck+0x1c0/0x2b8)
> > | [<c02938b4>] (cache_free_debugcheck+0x0/0x2b8) from [<c0293f78>] (kmem_cache_free+0x3c/0xc0)
> > | [<c0293f3c>] (kmem_cache_free+0x0/0xc0) from [<c032b1c8>] (ida_get_new_above+0x19c/0x1c0)
> > | [<c032b02c>] (ida_get_new_above+0x0/0x1c0) from [<c02af7ec>] (alloc_vfsmnt+0x54/0x144)
> > | [<c02af798>] (alloc_vfsmnt+0x0/0x144) from [<c0299830>] (vfs_kern_mount+0x30/0xec)
> > | [<c0299800>] (vfs_kern_mount+0x0/0xec) from [<c0299908>] (kern_mount_data+0x1c/0x20)
> > | [<c02998ec>] (kern_mount_data+0x0/0x20) from [<c02146c4>] (sysfs_init+0x68/0xc8)
> > | [<c021465c>] (sysfs_init+0x0/0xc8) from [<c02137d4>] (mnt_init+0x90/0x1b0)
> > | [<c0213744>] (mnt_init+0x0/0x1b0) from [<c0213388>] (vfs_caches_init+0x100/0x140)
> > | [<c0213288>] (vfs_caches_init+0x0/0x140) from [<c0208c0c>] (start_kernel+0x2e8/0x368)
> > | [<c0208924>] (start_kernel+0x0/0x368) from [<c0208034>] (__enable_mmu+0x0/0x2c)
> > | c0113268: redzone 1:0xd84156c5c032b3ac, redzone 2:0xd84156c5635688c0.
> > | slab error in cache_alloc_debugcheck_after(): cache `idr_layer_cache': double free, or memory outside object was overwritten
> > | ...
> > | c011307c: redzone 1:0x9f91102ffffffff, redzone 2:0x9f911029d74e35b
> > | slab: Internal list corruption detected in cache 'idr_layer_cache'(24), slabp c0113000(16). Hexdump:
> > |
> > | 000: 20 4f 10 c0 20 4f 10 c0 7c 00 00 00 7c 30 11 c0
> > | 010: 10 00 00 00 10 00 00 00 00 00 c9 17 fe ff ff ff
> > | 020: fe ff ff ff fe ff ff ff fe ff ff ff fe ff ff ff
> > | 030: fe ff ff ff fe ff ff ff fe ff ff ff fe ff ff ff
> > | 040: fe ff ff ff fe ff ff ff fe ff ff ff fe ff ff ff
> > | 050: fe ff ff ff fe ff ff ff fe ff ff ff 11 00 00 00
> > | 060: 12 00 00 00 13 00 00 00 14 00 00 00 15 00 00 00
> > | 070: 16 00 00 00 17 00 00 00 c0 88 56 63
> > | kernel BUG at /home/rmk/git/linux-2.6-rmk/mm/slab.c:2928!
> >
> > This hasn't been noticed as I guess not many people use SLAB on ARM
> > anymore, and even less people probably have SLAB debugging enabled.
> > With SLAB debugging disabled, the system appears to behave correctly -
> > or maybe the problem is just hidden.
> 
> I thought the fix was for ARM, actually, but unfortunately the
> changelog is somewhat terse (shame on me). Hmm?

As you say, the changelog is soo terse that we now have no idea why this
commit was created.

So, what's the fix here?  It looks to me like a revert is in order.

I suggest that if it does get reverted, a copy of my original bug report
is included in the revert commit so that all the details are available if
it needs revisiting.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Christoph Lameter]

On Tue, 8 Feb 2011, Russell King wrote:

> > I thought the fix was for ARM, actually, but unfortunately the
> > changelog is somewhat terse (shame on me). Hmm?
>
> As you say, the changelog is soo terse that we now have no idea why this
> commit was created.
>
> So, what's the fix here?  It looks to me like a revert is in order.
>
> I suggest that if it does get reverted, a copy of my original bug report
> is included in the revert commit so that all the details are available if
> it needs revisiting.

Uhh.. That first hunk in commit 5c5e3b33b7cb959a401f823707bee006caadd76e
looks wrong to me. ralign is usually a power of two and greater than
alignof(unsigned long long). The & operation will result in 0 and never
exempt any caches.

diff --git a/mm/slab.c b/mm/slab.c
index bac0f4f..525c664 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2220,8 +2220,8 @@ kmem_cache_create (const char *name, size_t size,
size_t align,
        if (ralign < align) {
                ralign = align;
        }
-       /* disable debug if necessary */
-       if (ralign > __alignof__(unsigned long long))
+       /* disable debug if not aligning with REDZONE_ALIGN */
+       if (ralign & (__alignof__(unsigned long long) - 1))
                flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
        /*
         * 4) Store it.


Did you mean

	if (ralign > REDZONE_ALIGN) ...

?

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Pekka Enberg]

On Tue, Feb 8, 2011 at 5:02 PM, Christoph Lameter <cl@linux.com> wrote:
> On Tue, 8 Feb 2011, Russell King wrote:
>
>> > I thought the fix was for ARM, actually, but unfortunately the
>> > changelog is somewhat terse (shame on me). Hmm?
>>
>> As you say, the changelog is soo terse that we now have no idea why this
>> commit was created.
>>
>> So, what's the fix here?  It looks to me like a revert is in order.
>>
>> I suggest that if it does get reverted, a copy of my original bug report
>> is included in the revert commit so that all the details are available if
>> it needs revisiting.
>
> Uhh.. That first hunk in commit 5c5e3b33b7cb959a401f823707bee006caadd76e
> looks wrong to me. ralign is usually a power of two and greater than
> alignof(unsigned long long). The & operation will result in 0 and never
> exempt any caches.
>
> diff --git a/mm/slab.c b/mm/slab.c
> index bac0f4f..525c664 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2220,8 +2220,8 @@ kmem_cache_create (const char *name, size_t size,
> size_t align,
>        if (ralign < align) {
>                ralign = align;
>        }
> -       /* disable debug if necessary */
> -       if (ralign > __alignof__(unsigned long long))
> +       /* disable debug if not aligning with REDZONE_ALIGN */
> +       if (ralign & (__alignof__(unsigned long long) - 1))
>                flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
>        /*
>         * 4) Store it.
>
>
> Did you mean
>
>        if (ralign > REDZONE_ALIGN) ...
>
> ?

Ping. Any arm-capable tester out there that can verify this fix? OTOH,
since Shiyong Li hasn't showed up, I'm inclined to just revert the
commit...

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Russell King]

On Fri, Feb 11, 2011 at 09:02:45AM +0200, Pekka Enberg wrote:
> On Tue, Feb 8, 2011 at 5:02 PM, Christoph Lameter <cl@linux.com> wrote:
> > Uhh.. That first hunk in commit 5c5e3b33b7cb959a401f823707bee006caadd76e
> > looks wrong to me. ralign is usually a power of two and greater than
> > alignof(unsigned long long). The & operation will result in 0 and never
> > exempt any caches.
> >
> > diff --git a/mm/slab.c b/mm/slab.c
> > index bac0f4f..525c664 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -2220,8 +2220,8 @@ kmem_cache_create (const char *name, size_t size,
> > size_t align,
> >        if (ralign < align) {
> >                ralign = align;
> >        }
> > -       /* disable debug if necessary */
> > -       if (ralign > __alignof__(unsigned long long))
> > +       /* disable debug if not aligning with REDZONE_ALIGN */
> > +       if (ralign & (__alignof__(unsigned long long) - 1))
> >                flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> >        /*
> >         * 4) Store it.
> >
> >
> > Did you mean
> >
> >        if (ralign > REDZONE_ALIGN) ...
> >
> > ?
> 
> Ping. Any arm-capable tester out there that can verify this fix? OTOH,
> since Shiyong Li hasn't showed up, I'm inclined to just revert the
> commit...

What fix?  The above is a quote of the patch in 5c5e3b33.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Christoph Lameter]

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

On Fri, 11 Feb 2011, Russell King wrote:

> On Fri, Feb 11, 2011 at 09:02:45AM +0200, Pekka Enberg wrote:
> > On Tue, Feb 8, 2011 at 5:02 PM, Christoph Lameter <cl@linux.com> wrote:
> > > Uhh.. That first hunk in commit 5c5e3b33b7cb959a401f823707bee006caadd76e
> > > looks wrong to me. ralign is usually a power of two and greater than
> > > alignof(unsigned long long). The & operation will result in 0 and never
> > > exempt any caches.
> > >
> > > diff --git a/mm/slab.c b/mm/slab.c
> > > index bac0f4f..525c664 100644
> > > --- a/mm/slab.c
> > > +++ b/mm/slab.c
> > > @@ -2220,8 +2220,8 @@ kmem_cache_create (const char *name, size_t size,
> > > size_t align,
> > >        if (ralign < align) {
> > >                ralign = align;
> > >        }
> > > -       /* disable debug if necessary */
> > > -       if (ralign > __alignof__(unsigned long long))
> > > +       /* disable debug if not aligning with REDZONE_ALIGN */
> > > +       if (ralign & (__alignof__(unsigned long long) - 1))
> > >                flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> > >        /*
> > >         * 4) Store it.
> > >
> > >
> > > Did you mean
> > >
> > >        if (ralign > REDZONE_ALIGN) ...
> > >
> > > ?
> >
> > Ping. Any arm-capable tester out there that can verify this fix? OTOH,
> > since Shiyong Li hasn't showed up, I'm inclined to just revert the
> > commit...
>
> What fix?  The above is a quote of the patch in 5c5e3b33.

He was referring to the change of the if statement.

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Russell King]

On Fri, Feb 11, 2011 at 09:20:27AM -0600, Christoph Lameter wrote:
> On Fri, 11 Feb 2011, Russell King wrote:
> 
> > On Fri, Feb 11, 2011 at 09:02:45AM +0200, Pekka Enberg wrote:
> > > On Tue, Feb 8, 2011 at 5:02 PM, Christoph Lameter <cl@linux.com> wrote:
> > > > Uhh.. That first hunk in commit 5c5e3b33b7cb959a401f823707bee006caadd76e
> > > > looks wrong to me. ralign is usually a power of two and greater than
> > > > alignof(unsigned long long). The & operation will result in 0 and never
> > > > exempt any caches.
> > > >
> > > > diff --git a/mm/slab.c b/mm/slab.c
> > > > index bac0f4f..525c664 100644
> > > > --- a/mm/slab.c
> > > > +++ b/mm/slab.c
> > > > @@ -2220,8 +2220,8 @@ kmem_cache_create (const char *name, size_t size,
> > > > size_t align,
> > > >        if (ralign < align) {
> > > >                ralign = align;
> > > >        }
> > > > -       /* disable debug if necessary */
> > > > -       if (ralign > __alignof__(unsigned long long))
> > > > +       /* disable debug if not aligning with REDZONE_ALIGN */
> > > > +       if (ralign & (__alignof__(unsigned long long) - 1))
> > > >                flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
> > > >        /*
> > > >         * 4) Store it.
> > > >
> > > >
> > > > Did you mean
> > > >
> > > >        if (ralign > REDZONE_ALIGN) ...
> > > >
> > > > ?
> > >
> > > Ping. Any arm-capable tester out there that can verify this fix? OTOH,
> > > since Shiyong Li hasn't showed up, I'm inclined to just revert the
> > > commit...
> >
> > What fix?  The above is a quote of the patch in 5c5e3b33.
> 
> He was referring to the change of the if statement.

Replacing what with what?

	if (ralign & (__alignof__(unsigned long long) - 1))
with
	if (ralign > __alignof__(unsigned long long))
or
	if (ralign > REDZONE_ALIGN)

?  A patch is a wonderful way to describe the change to be tested.


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Russell King]

On Mon, Feb 14, 2011 at 11:10:45AM +0800, ShiYong LI wrote:
> Hi all,
> 
> Sorry for my late reply.
> 
> My commit is created to enable ARM with 128 byte cache line support last
> information dump while slab corruption occurs. I've verified it works fine
> on ARMv7.

Mainline only supports 32 byte and 64 byte cache lines on ARM.  It does
not support 128 byte cache lines, so maybe your attempts are the wrong
approach.  Can you please post your 128 byte cache line support patches
to the ARM kernel mailing list for review.

In the mean time, because your patch causes an oopsing regression, I
strongly suggest that it is reverted until we start supporting 128 byte
cache lines on ARM in mainline.

> Russell,
> 
> Can you provide more information for the problem? What kind of ARM are you
> using ?

ARM926, which has a cache line size of 32.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Pekka Enberg]

On Mon, Feb 14, 2011 at 5:40 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> Mainline only supports 32 byte and 64 byte cache lines on ARM.  It does
> not support 128 byte cache lines, so maybe your attempts are the wrong
> approach.  Can you please post your 128 byte cache line support patches
> to the ARM kernel mailing list for review.
>
> In the mean time, because your patch causes an oopsing regression, I
> strongly suggest that it is reverted until we start supporting 128 byte
> cache lines on ARM in mainline.

Agreed. I'll queue a revert. Thanks, Russell!

Re: "slab: Fix missing DEBUG_SLAB last user" breaks ARM

[Pekka Enberg]

On Mon, Feb 14, 2011 at 5:43 PM, Pekka Enberg <penberg@kernel.org> wrote:
> On Mon, Feb 14, 2011 at 5:40 PM, Russell King <rmk@arm.linux.org.uk> wrote:
>> Mainline only supports 32 byte and 64 byte cache lines on ARM.  It does
>> not support 128 byte cache lines, so maybe your attempts are the wrong
>> approach.  Can you please post your 128 byte cache line support patches
>> to the ARM kernel mailing list for review.
>>
>> In the mean time, because your patch causes an oopsing regression, I
>> strongly suggest that it is reverted until we start supporting 128 byte
>> cache lines on ARM in mainline.
>
> Agreed. I'll queue a revert. Thanks, Russell!

It's this commit queued for linux-next:

http://git.kernel.org/?p=linux/kernel/git/penberg/slab-2.6.git;a=commit;h=3ff84a7f36554b257cd57325b1a7c1fa4b49fbe3