mm: slub: invalid memory access in setup_object

mm: slub: invalid memory access in setup_object

[Sasha Levin]

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next
kernel I've stumbled on the following spew:

[  791.659908] BUG: unable to handle kernel paging request at ffff880302e12000
[  791.661580] IP: memset (arch/x86/lib/memset_64.S:83)
[  791.661580] PGD 17b7d067 PUD 704947067 PMD 70492f067 PTE 8000000302e12060
[  791.661580] Oops: 0002 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[  791.661580] Dumping ftrace buffer:
[  791.661580]    (ftrace buffer empty)
[  791.667964] Modules linked in:
[  791.667964] CPU: 13 PID: 10630 Comm: trinity-c20 Not tainted 3.16.0-rc2-next-20140624-sasha-00024-g332b58d #726
[  791.669480] task: ffff8803d5123000 ti: ffff8803ba460000 task.ti: ffff8803ba460000
[  791.669480] RIP: memset (arch/x86/lib/memset_64.S:83)
[  791.669480] RSP: 0018:ffff8803ba463b18  EFLAGS: 00010003
[  791.669480] RAX: 6b6b6b6b6b6b6b6b RBX: ffff880036851540 RCX: 0000000000000068
[  791.669480] RDX: 0000000000002a77 RSI: 000000000000006b RDI: ffff880302e12000
[  791.669480] RBP: ffff8803ba463b40 R08: 0000000000000001 R09: 0000000000000000
[  791.669480] R10: ffff880302e11000 R11: ffffffffffffffd8 R12: ffff880302e11000
[  791.669480] R13: 00000000000000bb R14: ffff880302e11000 R15: ffffffffffffffff
[  791.669480] FS:  00007f37693b3700(0000) GS:ffff880334e00000(0000) knlGS:0000000000000000
[  791.669480] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  791.669480] CR2: ffff880302e12000 CR3: 00000003b744f000 CR4: 00000000000006a0
[  791.669480] Stack:
[  791.669480]  ffffffff902f4273 ffff8803ba463b30 ffff880036851540 ffff880302e11000
[  791.669480]  ffffea000c0b8440 ffff8803ba463b60 ffffffff902f48b0 ffff880036851540
[  791.669480]  ffff880302e11000 ffff8803ba463bc0 ffffffff902f6886 00000000000000d0
[  791.669480] Call Trace:
[  791.669480] ? init_object (mm/slub.c:665)
[  791.669480] setup_object.isra.34 (mm/slub.c:1008 mm/slub.c:1373)
[  791.669480] new_slab (mm/slub.c:278 mm/slub.c:1412)
[  791.669480] __slab_alloc (mm/slub.c:2186 mm/slub.c:2344)
[  791.690803] ? kvm_clock_read (./arch/x86/include/asm/preempt.h:90 arch/x86/kernel/kvmclock.c:86)
[  791.690803] ? copy_process (kernel/fork.c:306 kernel/fork.c:1193)
[  791.690803] ? sched_clock (./arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:305)
[  791.690803] ? get_parent_ip (kernel/sched/core.c:2550)
[  791.690803] kmem_cache_alloc_node (mm/slub.c:2417 mm/slub.c:2486)
[  791.690803] ? sched_clock_cpu (kernel/sched/clock.c:311)
[  791.690803] ? copy_process (kernel/fork.c:306 kernel/fork.c:1193)
[  791.690803] copy_process (kernel/fork.c:306 kernel/fork.c:1193)
[  791.690803] ? kvm_clock_read (./arch/x86/include/asm/preempt.h:90 arch/x86/kernel/kvmclock.c:86)
[  791.690803] ? sched_clock (./arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:305)
[  791.690803] ? sched_clock_local (kernel/sched/clock.c:214)
[  791.690803] ? put_lock_stats.isra.12 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
[  791.690803] do_fork (kernel/fork.c:1609)
[  791.690803] ? get_parent_ip (kernel/sched/core.c:2550)
[  791.690803] ? context_tracking_user_exit (./arch/x86/include/asm/paravirt.h:809 (discriminator 2) kernel/context_tracking.c:184 (discriminator 2))
[  791.690803] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63)
[  791.690803] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2557 kernel/locking/lockdep.c:2599)
[  791.690803] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
[  791.690803] SyS_clone (kernel/fork.c:1695)
[  791.690803] stub_clone (arch/x86/kernel/entry_64.S:637)
[  791.690803] ? tracesys (arch/x86/kernel/entry_64.S:542)
[ 791.690803] Code: b8 01 01 01 01 01 01 01 01 48 0f af c1 41 89 f9 41 83 e1 07 75 70 48 89 d1 48 c1 e9 06 74 39 66 0f 1f 84 00 00 00 00 00 48 ff c9 <48> 89 07 48 89 47 08 48 89 47 10 48 89 47 18 48 89 47 20 48 89
All code
========
   0:	b8 01 01 01 01       	mov    $0x1010101,%eax
   5:	01 01                	add    %eax,(%rcx)
   7:	01 01                	add    %eax,(%rcx)
   9:	48 0f af c1          	imul   %rcx,%rax
   d:	41 89 f9             	mov    %edi,%r9d
  10:	41 83 e1 07          	and    $0x7,%r9d
  14:	75 70                	jne    0x86
  16:	48 89 d1             	mov    %rdx,%rcx
  19:	48 c1 e9 06          	shr    $0x6,%rcx
  1d:	74 39                	je     0x58
  1f:	66 0f 1f 84 00 00 00 	nopw   0x0(%rax,%rax,1)
  26:	00 00
  28:	48 ff c9             	dec    %rcx
  2b:*	48 89 07             	mov    %rax,(%rdi)		<-- trapping instruction
  2e:	48 89 47 08          	mov    %rax,0x8(%rdi)
  32:	48 89 47 10          	mov    %rax,0x10(%rdi)
  36:	48 89 47 18          	mov    %rax,0x18(%rdi)
  3a:	48 89 47 20          	mov    %rax,0x20(%rdi)
  3e:	48 89 00             	mov    %rax,(%rax)

Code starting with the faulting instruction
===========================================
   0:	48 89 07             	mov    %rax,(%rdi)
   3:	48 89 47 08          	mov    %rax,0x8(%rdi)
   7:	48 89 47 10          	mov    %rax,0x10(%rdi)
   b:	48 89 47 18          	mov    %rax,0x18(%rdi)
   f:	48 89 47 20          	mov    %rax,0x20(%rdi)
  13:	48 89 00             	mov    %rax,(%rax)
[  791.690803] RIP memset (arch/x86/lib/memset_64.S:83)
[  791.690803]  RSP <ffff8803ba463b18>
[  791.690803] CR2: ffff880302e12000

Thanks,
Sasha

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

Re: mm: slub: invalid memory access in setup_object

[Christoph Lameter]

On Wed, 25 Jun 2014, Sasha Levin wrote:

> [  791.669480] ? init_object (mm/slub.c:665)
> [  791.669480] setup_object.isra.34 (mm/slub.c:1008 mm/slub.c:1373)
> [  791.669480] new_slab (mm/slub.c:278 mm/slub.c:1412)

So we just got a new page from the page allocator but somehow cannot
write to it. This is the first write access to the page.

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

Re: mm: slub: invalid memory access in setup_object

[David Rientjes]

On Wed, 25 Jun 2014, Christoph Lameter wrote:

> On Wed, 25 Jun 2014, Sasha Levin wrote:
> 
> > [  791.669480] ? init_object (mm/slub.c:665)
> > [  791.669480] setup_object.isra.34 (mm/slub.c:1008 mm/slub.c:1373)
> > [  791.669480] new_slab (mm/slub.c:278 mm/slub.c:1412)
> 
> So we just got a new page from the page allocator but somehow cannot
> write to it. This is the first write access to the page.
> 

I'd be inclined to think that this was a result of "slub: reduce duplicate 
creation on the first object" from -mm[*] that was added the day before 
Sasha reported the problem.

It's not at all clear to me that that patch is correct.  Wei?

Sasha, with a revert of that patch, does this reproduce?

 [*] http://ozlabs.org/~akpm/mmotm/broken-out/slub-reduce-duplicate-creation-on-the-first-object.patch

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

Re: mm: slub: invalid memory access in setup_object

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 4354 bytes --]

On Mon, Jun 30, 2014 at 03:03:21PM -0700, David Rientjes wrote:
>On Wed, 25 Jun 2014, Christoph Lameter wrote:
>
>> On Wed, 25 Jun 2014, Sasha Levin wrote:
>> 
>> > [  791.669480] ? init_object (mm/slub.c:665)
>> > [  791.669480] setup_object.isra.34 (mm/slub.c:1008 mm/slub.c:1373)
>> > [  791.669480] new_slab (mm/slub.c:278 mm/slub.c:1412)
>> 
>> So we just got a new page from the page allocator but somehow cannot
>> write to it. This is the first write access to the page.
>> 
>
>I'd be inclined to think that this was a result of "slub: reduce duplicate 
>creation on the first object" from -mm[*] that was added the day before 
>Sasha reported the problem.
>
>It's not at all clear to me that that patch is correct.  Wei?
>
>Sasha, with a revert of that patch, does this reproduce?
>
> [*] http://ozlabs.org/~akpm/mmotm/broken-out/slub-reduce-duplicate-creation-on-the-first-object.patch

David,

So sad to see the error after applying my patch. In which case this is
triggered? The kernel with this patch runs fine on my laptop. Maybe there is
some corner case I missed? If you could tell me the way you reproduce it, I
would have a try on my side.

I did a simple test for this patch, my test code and result is attached.

1. kmem_cache.c
   The test module.
2. kmem_log.txt
   In this log, you can see 26 objects are initialized once exactly, while
   without this patch, the first object will be initialized twice.

     Fetch a cache from kmem_cache
     new_slab: page->objects is 26
     new_slab: setup on ffff880097038000, ffff8800970384e0
       init_once: [00]ffff880097038000 is created
     new_slab: setup on ffff8800970384e0, ffff8800970389c0
       init_once: [01]ffff8800970384e0 is created
     new_slab: setup on ffff8800970389c0, ffff880097038ea0
       init_once: [02]ffff8800970389c0 is created
     new_slab: setup on ffff880097038ea0, ffff880097039380
       init_once: [03]ffff880097038ea0 is created
     new_slab: setup on ffff880097039380, ffff880097039860
       init_once: [04]ffff880097039380 is created
     new_slab: setup on ffff880097039860, ffff880097039d40
       init_once: [05]ffff880097039860 is created
     new_slab: setup on ffff880097039d40, ffff88009703a220
       init_once: [06]ffff880097039d40 is created
     new_slab: setup on ffff88009703a220, ffff88009703a700
       init_once: [07]ffff88009703a220 is created
     new_slab: setup on ffff88009703a700, ffff88009703abe0
       init_once: [08]ffff88009703a700 is created
     new_slab: setup on ffff88009703abe0, ffff88009703b0c0
       init_once: [09]ffff88009703abe0 is created
     new_slab: setup on ffff88009703b0c0, ffff88009703b5a0
       init_once: [10]ffff88009703b0c0 is created
     new_slab: setup on ffff88009703b5a0, ffff88009703ba80
       init_once: [11]ffff88009703b5a0 is created
     new_slab: setup on ffff88009703ba80, ffff88009703bf60
       init_once: [12]ffff88009703ba80 is created
     new_slab: setup on ffff88009703bf60, ffff88009703c440
       init_once: [13]ffff88009703bf60 is created
     new_slab: setup on ffff88009703c440, ffff88009703c920
       init_once: [14]ffff88009703c440 is created
     new_slab: setup on ffff88009703c920, ffff88009703ce00
       init_once: [15]ffff88009703c920 is created
     new_slab: setup on ffff88009703ce00, ffff88009703d2e0
       init_once: [16]ffff88009703ce00 is created
     new_slab: setup on ffff88009703d2e0, ffff88009703d7c0
       init_once: [17]ffff88009703d2e0 is created
     new_slab: setup on ffff88009703d7c0, ffff88009703dca0
       init_once: [18]ffff88009703d7c0 is created
     new_slab: setup on ffff88009703dca0, ffff88009703e180
       init_once: [19]ffff88009703dca0 is created
     new_slab: setup on ffff88009703e180, ffff88009703e660
       init_once: [20]ffff88009703e180 is created
     new_slab: setup on ffff88009703e660, ffff88009703eb40
       init_once: [21]ffff88009703e660 is created
     new_slab: setup on ffff88009703eb40, ffff88009703f020
       init_once: [22]ffff88009703eb40 is created
     new_slab: setup on ffff88009703f020, ffff88009703f500
       init_once: [23]ffff88009703f020 is created
     new_slab: setup on ffff88009703f500, ffff88009703f9e0
       init_once: [24]ffff88009703f500 is created
     new_slab: do it again? ffff88009703f9e0
       init_once: [25]ffff88009703f9e0 is created

-- 
Richard Yang
Help you, Help me

[-- Attachment #2: kmem_cache.c --]
[-- Type: text/x-csrc, Size: 1217 bytes --]

/*
 * =====================================================================================
 *
 *       Filename:  kmem_cache.c
 *
 *    Description:  /proc/slabinfo
 *
 *        Version:  1.0
 *        Created:  04/26/2014 09:12:04 PM
 *       Revision:  none
 *       Compiler:  gcc
 *
 *         Author:  Wei Yang (weiyang), weiyang.kernel@gmail.com
 *        Company:  
 *
 * =====================================================================================
 */

#include <linux/init.h>
#include <linux/module.h>
#include <linux/slab.h>
MODULE_LICENSE("Dual BSD/GPL");

static struct kmem_cache *test_cache;
void *tmp;

static void init_once(void *foo)
{
	static int num;
	printk(KERN_ERR "%s: [%02d]%p is created\n", __func__, num++, foo);
}

static int kmem_cache_test_init(void)
{
	test_cache = kmem_cache_create("test_cache", 1234, 4,
			0, init_once);
	if (test_cache == NULL)
		return -ENOMEM;

	printk(KERN_ERR "Fetch a cache from kmem_cache\n", __func__);
	tmp = kmem_cache_zalloc(test_cache, GFP_KERNEL);

	return 0;
}
static void kmem_cache_test_exit(void)
{
	kmem_cache_free(test_cache, tmp);
	kmem_cache_destroy(test_cache);
}
module_init(kmem_cache_test_init);
module_exit(kmem_cache_test_exit);

Re: mm: slub: invalid memory access in setup_object

[Christoph Lameter]

On Mon, 30 Jun 2014, David Rientjes wrote:

> It's not at all clear to me that that patch is correct.  Wei?

Looks ok to me. But I do not like the convoluted code in new_slab() which
Wei's patch does not make easier to read. Makes it difficult for the
reader to see whats going on.

Lets drop the use of the variable named "last".


Subject: slub: Only call setup_object once for each object

Modify the logic for object initialization to be less convoluted
and initialize an object only once.

Signed-off-by: Christoph Lameter <cl@linux.com>

Index: linux/mm/slub.c
===================================================================
--- linux.orig/mm/slub.c	2014-07-01 09:50:02.486846653 -0500
+++ linux/mm/slub.c	2014-07-01 09:52:07.918802585 -0500
@@ -1409,7 +1409,6 @@ static struct page *new_slab(struct kmem
 {
 	struct page *page;
 	void *start;
-	void *last;
 	void *p;
 	int order;

@@ -1432,15 +1431,11 @@ static struct page *new_slab(struct kmem
 	if (unlikely(s->flags & SLAB_POISON))
 		memset(start, POISON_INUSE, PAGE_SIZE << order);

-	last = start;
 	for_each_object(p, s, start, page->objects) {
-		setup_object(s, page, last);
-		set_freepointer(s, last, p);
-		last = p;
+		setup_object(s, page, p);
+		set_freepointer(s, p, p + s->size);
 	}
-	setup_object(s, page, last);
-	set_freepointer(s, last, NULL);
-
+	set_freepointer(s, start + (page->objects - 1) * s->size, NULL);
 	page->freelist = start;
 	page->inuse = page->objects;
 	page->frozen = 1;

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

Re: mm: slub: invalid memory access in setup_object

[Andrew Morton]

On Tue, 1 Jul 2014 09:58:52 -0500 (CDT) Christoph Lameter <cl@gentwo.org> wrote:

> On Mon, 30 Jun 2014, David Rientjes wrote:
> 
> > It's not at all clear to me that that patch is correct.  Wei?
> 
> Looks ok to me. But I do not like the convoluted code in new_slab() which
> Wei's patch does not make easier to read. Makes it difficult for the
> reader to see whats going on.
> 
> Lets drop the use of the variable named "last".
> 
> 
> Subject: slub: Only call setup_object once for each object
> 
> Modify the logic for object initialization to be less convoluted
> and initialize an object only once.
> 

Well, um.  Wei's changelog was much better:

: When a kmem_cache is created with ctor, each object in the kmem_cache will
: be initialized before use.  In the slub implementation, the first object
: will be initialized twice.
: 
: This patch avoids the duplication of initialization of the first object.
: 
: Fixes commit 7656c72b5a63: ("SLUB: add macros for scanning objects in a
: slab").

I can copy that text over and add the reported-by etc (ho hum) but I
have a tiny feeling that this patch hasn't been rigorously tested? 
Perhaps someone (Wei?) can do that?

And we still don't know why Sasha's kernel went oops.

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

Re: mm: slub: invalid memory access in setup_object

[Sasha Levin]

On 07/01/2014 05:49 PM, Andrew Morton wrote:
> On Tue, 1 Jul 2014 09:58:52 -0500 (CDT) Christoph Lameter <cl@gentwo.org> wrote:
> 
>> On Mon, 30 Jun 2014, David Rientjes wrote:
>>
>>> It's not at all clear to me that that patch is correct.  Wei?
>>
>> Looks ok to me. But I do not like the convoluted code in new_slab() which
>> Wei's patch does not make easier to read. Makes it difficult for the
>> reader to see whats going on.
>>
>> Lets drop the use of the variable named "last".
>>
>>
>> Subject: slub: Only call setup_object once for each object
>>
>> Modify the logic for object initialization to be less convoluted
>> and initialize an object only once.
>>
> 
> Well, um.  Wei's changelog was much better:
> 
> : When a kmem_cache is created with ctor, each object in the kmem_cache will
> : be initialized before use.  In the slub implementation, the first object
> : will be initialized twice.
> : 
> : This patch avoids the duplication of initialization of the first object.
> : 
> : Fixes commit 7656c72b5a63: ("SLUB: add macros for scanning objects in a
> : slab").
> 
> I can copy that text over and add the reported-by etc (ho hum) but I
> have a tiny feeling that this patch hasn't been rigorously tested? 
> Perhaps someone (Wei?) can do that?
> 
> And we still don't know why Sasha's kernel went oops.

I only saw this oops once, and after David's message yesterday I tried reverting
the patch he pointed out, but not much changed.

Is there a better way to stress test slub?


Thanks,
Sasha

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

Re: mm: slub: invalid memory access in setup_object

[Christoph Lameter]

On Tue, 1 Jul 2014, Sasha Levin wrote:

> Is there a better way to stress test slub?

The typical way to test is by stressing the network subsystem
with small packets that require small allocations. Or do a filesystem
test that requires lots of metadata (file creations, removal, renames
etc).

But I also posted some in kernel benchmarks a while back

https://lkml.org/lkml/2009/10/13/459

Pekka had a project going to get these merged.

https://lkml.org/lkml/2009/11/29/17


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

Re: mm: slub: invalid memory access in setup_object

[Wei Yang]

On Tue, Jul 01, 2014 at 02:49:47PM -0700, Andrew Morton wrote:
>On Tue, 1 Jul 2014 09:58:52 -0500 (CDT) Christoph Lameter <cl@gentwo.org> wrote:
>
>> On Mon, 30 Jun 2014, David Rientjes wrote:
>> 
>> > It's not at all clear to me that that patch is correct.  Wei?
>> 
>> Looks ok to me. But I do not like the convoluted code in new_slab() which
>> Wei's patch does not make easier to read. Makes it difficult for the
>> reader to see whats going on.
>> 
>> Lets drop the use of the variable named "last".
>> 
>> 
>> Subject: slub: Only call setup_object once for each object
>> 
>> Modify the logic for object initialization to be less convoluted
>> and initialize an object only once.
>> 
>
>Well, um.  Wei's changelog was much better:
>
>: When a kmem_cache is created with ctor, each object in the kmem_cache will
>: be initialized before use.  In the slub implementation, the first object
>: will be initialized twice.
>: 
>: This patch avoids the duplication of initialization of the first object.
>: 
>: Fixes commit 7656c72b5a63: ("SLUB: add macros for scanning objects in a
>: slab").
>
>I can copy that text over and add the reported-by etc (ho hum) but I
>have a tiny feeling that this patch hasn't been rigorously tested? 
>Perhaps someone (Wei?) can do that?

Ok, I will apply this one and give a shot.

>
>And we still don't know why Sasha's kernel went oops.

Yep, if there is some procedure to reproduce it, I'd like to do it at my side.

-- 
Richard Yang
Help you, Help me

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

Re: mm: slub: invalid memory access in setup_object

[Christoph Lameter]

On Tue, 1 Jul 2014, Andrew Morton wrote:

> I can copy that text over and add the reported-by etc (ho hum) but I
> have a tiny feeling that this patch hasn't been rigorously tested?

The testing so far was to verify that  a kernel successfully builds with
the patch and then booted upo in a kvm instance.

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

Re: mm: slub: invalid memory access in setup_object

[Wei Yang]

On Tue, Jul 01, 2014 at 02:49:47PM -0700, Andrew Morton wrote:
>On Tue, 1 Jul 2014 09:58:52 -0500 (CDT) Christoph Lameter <cl@gentwo.org> wrote:
>
>> On Mon, 30 Jun 2014, David Rientjes wrote:
>> 
>> > It's not at all clear to me that that patch is correct.  Wei?
>> 
>> Looks ok to me. But I do not like the convoluted code in new_slab() which
>> Wei's patch does not make easier to read. Makes it difficult for the
>> reader to see whats going on.
>> 
>> Lets drop the use of the variable named "last".
>> 
>> 
>> Subject: slub: Only call setup_object once for each object
>> 
>> Modify the logic for object initialization to be less convoluted
>> and initialize an object only once.
>> 
>
>Well, um.  Wei's changelog was much better:
>
>: When a kmem_cache is created with ctor, each object in the kmem_cache will
>: be initialized before use.  In the slub implementation, the first object
>: will be initialized twice.
>: 
>: This patch avoids the duplication of initialization of the first object.
>: 
>: Fixes commit 7656c72b5a63: ("SLUB: add macros for scanning objects in a
>: slab").
>
>I can copy that text over and add the reported-by etc (ho hum) but I
>have a tiny feeling that this patch hasn't been rigorously tested? 
>Perhaps someone (Wei?) can do that?

The result of the simple test is the same. And my laptop works a whole day
with this patch.

Thanks

>
>And we still don't know why Sasha's kernel went oops.

-- 
Richard Yang
Help you, Help me

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

Re: mm: slub: invalid memory access in setup_object

[Wei Yang]

On Tue, Jul 01, 2014 at 09:58:52AM -0500, Christoph Lameter wrote:
>On Mon, 30 Jun 2014, David Rientjes wrote:
>
>> It's not at all clear to me that that patch is correct.  Wei?
>
>Looks ok to me. But I do not like the convoluted code in new_slab() which
>Wei's patch does not make easier to read. Makes it difficult for the
>reader to see whats going on.

My patch is somewhat convoluted since I wanted to preserve the original logic
and make minimal change. And yes, it looks not that nice to audience.

I feel a little hurt by this patch. What I found and worked is gone with this
patch.

>
>Lets drop the use of the variable named "last".
>
>
>Subject: slub: Only call setup_object once for each object
>
>Modify the logic for object initialization to be less convoluted
>and initialize an object only once.
>
>Signed-off-by: Christoph Lameter <cl@linux.com>
>
>Index: linux/mm/slub.c
>===================================================================
>--- linux.orig/mm/slub.c	2014-07-01 09:50:02.486846653 -0500
>+++ linux/mm/slub.c	2014-07-01 09:52:07.918802585 -0500
>@@ -1409,7 +1409,6 @@ static struct page *new_slab(struct kmem
> {
> 	struct page *page;
> 	void *start;
>-	void *last;
> 	void *p;
> 	int order;
>
>@@ -1432,15 +1431,11 @@ static struct page *new_slab(struct kmem
> 	if (unlikely(s->flags & SLAB_POISON))
> 		memset(start, POISON_INUSE, PAGE_SIZE << order);
>
>-	last = start;
> 	for_each_object(p, s, start, page->objects) {
>-		setup_object(s, page, last);
>-		set_freepointer(s, last, p);
>-		last = p;
>+		setup_object(s, page, p);
>+		set_freepointer(s, p, p + s->size);
> 	}
>-	setup_object(s, page, last);
>-	set_freepointer(s, last, NULL);
>-
>+	set_freepointer(s, start + (page->objects - 1) * s->size, NULL);
> 	page->freelist = start;
> 	page->inuse = page->objects;
> 	page->frozen = 1;

-- 
Richard Yang
Help you, Help me

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

Re: mm: slub: invalid memory access in setup_object

[Christoph Lameter]

On Wed, 2 Jul 2014, Wei Yang wrote:

> My patch is somewhat convoluted since I wanted to preserve the original logic
> and make minimal change. And yes, it looks not that nice to audience.

Well I was the author of the initial "convoluted" logic.

> I feel a little hurt by this patch. What I found and worked is gone with this
> patch.

Ok how about giving this one additional revision. Maybe you can make the
function even easier to read? F.e. the setting of the NULL pointer at the
end of the loop is ugly.

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

Re: mm: slub: invalid memory access in setup_object

[Wei Yang]

On Wed, Jul 02, 2014 at 09:20:20AM -0500, Christoph Lameter wrote:
>On Wed, 2 Jul 2014, Wei Yang wrote:
>
>> My patch is somewhat convoluted since I wanted to preserve the original logic
>> and make minimal change. And yes, it looks not that nice to audience.
>
>Well I was the author of the initial "convoluted" logic.
>
>> I feel a little hurt by this patch. What I found and worked is gone with this
>> patch.
>
>Ok how about giving this one additional revision. Maybe you can make the
>function even easier to read? F.e. the setting of the NULL pointer at the
>end of the loop is ugly.

Hi, Christoph

Here is my refined version, hope this is more friendly to the audience.

Re: mm: slub: invalid memory access in setup_object

[Christoph Lameter]

On Thu, 3 Jul 2014, Wei Yang wrote:

> Here is my refined version, hope this is more friendly to the audience.

Acked-by: Christoph Lameter <cl@linux.com>

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

Re: mm: slub: invalid memory access in setup_object

[Wei Yang]

On Mon, Jul 07, 2014 at 08:51:08AM -0500, Christoph Lameter wrote:
>On Thu, 3 Jul 2014, Wei Yang wrote:
>
>> Here is my refined version, hope this is more friendly to the audience.
>
>Acked-by: Christoph Lameter <cl@linux.com>

Thanks. I am glad to work with you.

-- 
Richard Yang
Help you, Help me

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