Re: slab_common: fix the check for duplicate slab names

Re: slab_common: fix the check for duplicate slab names

[Mike Snitzer]

[Sorry for top-post but...]

This patch still isn't upstream.  Who should be shepherding it to Linus?

Without it we're seeing crashes on Fedora when running regression tests
that use dm-raid (dm target that wraps MD raid), e.g.:

6,484484,682379136295,-;md: mdX: recovery done.
6,484485,682379145308,-;bio: create slab <bio-0> at 0
6,484486,682379147581,-;md/raid:mdX: device dm-17 operational as raid disk 4
6,484487,682379149216,-;md/raid:mdX: device dm-11 operational as raid disk 2
6,484488,682379150829,-;md/raid:mdX: device dm-20 operational as raid disk 1
6,484489,682379152369,-;md/raid:mdX: device dm-9 operational as raid disk 0
3,484490,682379153954,-;kmem_cache_sanity_check (raid6-ffff880014e8b010): Cache name already exists.
4,484491,682379155824,-;CPU: 0 PID: 11228 Comm: lvm Not tainted 3.14.0-0.rc6.git0.1.fc21.x86_64 #1
4,484492,682379157704,-;Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2007
4,484493,682379159298,-; 0000000000000000 00000000a0bb80c5 ffff88003428d9d8 ffffffff816edd6b
4,484494,682379161238,-; ffff880076b51700 ffff88003428da50 ffffffff811982c3 0000000000000000
4,484495,682379163199,-; 0000000000000000 0000000000000790 0000000000000000 0000000000000000
4,484496,682379165206,-;Call Trace:
4,484497,682379166523,-; [<ffffffff816edd6b>] dump_stack+0x45/0x56
4,484498,682379168098,-; [<ffffffff811982c3>] kmem_cache_create_memcg+0x143/0x3e0
4,484499,682379169854,-; [<ffffffff8119858b>] kmem_cache_create+0x2b/0x30
4,484500,682379171541,-; [<ffffffffa020fc6c>] setup_conf+0x5cc/0x810 [raid456]
4,484501,682379173264,-; [<ffffffff811771ad>] ? mempool_create_node+0xdd/0x140
4,484502,682379174988,-; [<ffffffff81176dd0>] ? mempool_alloc_slab+0x20/0x20
4,484503,682379176728,-; [<ffffffffa0210a38>] run+0x868/0xa60 [raid456]
4,484504,682379178380,-; [<ffffffff81220a3e>] ? bioset_create+0x21e/0x2e0
4,484505,682379180038,-; [<ffffffff81563d3a>] md_run+0x3fa/0x980
4,484506,682379181631,-; [<ffffffff81221778>] ? bio_put+0x78/0x90
4,484507,682379183339,-; [<ffffffff8155badd>] ? sync_page_io+0x8d/0x110
4,484508,682379185000,-; [<ffffffffa0227570>] raid_ctr+0xf30/0x1389 [dm_raid]
4,484509,682379186771,-; [<ffffffff8156f857>] dm_table_add_target+0x177/0x460
4,484510,682379188538,-; [<ffffffff81572d57>] table_load+0x157/0x380
4,484511,682379190198,-; [<ffffffff81572c00>] ? retrieve_status+0x1c0/0x1c0
4,484512,682379191925,-; [<ffffffff815739c5>] ctl_ioctl+0x255/0x500
4,484513,682379193589,-; [<ffffffff811e8b00>] ? do_sync_write+0x50/0xa0
4,484514,682379195256,-; [<ffffffff81573c83>] dm_ctl_ioctl+0x13/0x20
4,484515,682379196900,-; [<ffffffff811fc790>] do_vfs_ioctl+0x2e0/0x4a0
4,484516,682379198600,-; [<ffffffff811eb931>] ? __sb_end_write+0x31/0x60
4,484517,682379200286,-; [<ffffffff811e9392>] ? vfs_write+0x172/0x1e0
4,484518,682379201957,-; [<ffffffff811fc9f1>] SyS_ioctl+0xa1/0xc0
4,484519,682379203651,-; [<ffffffff816fe129>] system_call_fastpath+0x16/0x1b
3,484520,682379205496,-;md/raid:mdX: couldn't allocate 0kB for buffers

On Tue, Mar 04 2014 at  5:13pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> The patch 3e374919b314f20e2a04f641ebc1093d758f66a4 is supposed to fix the
> problem where kmem_cache_create incorrectly reports duplicate cache name
> and fails. The problem is described in the header of that patch.
> 
> However, the patch doesn't really fix the problem because of these
> reasons:
> 
> * the logic to test for debugging is reversed. It was intended to perform
>   the check only if slub debugging is enabled (which implies that caches
>   with the same parameters are not merged). Therefore, there should be
>   #if !defined(CONFIG_SLUB) || defined(CONFIG_SLUB_DEBUG_ON)
>   The current code has the condition reversed and performs the test if
>   debugging is disabled.
> 
> * slub debugging may be enabled or disabled based on kernel command line,
>   CONFIG_SLUB_DEBUG_ON is just the default settings. Therefore the test
>   based on definition of CONFIG_SLUB_DEBUG_ON is unreliable.
> 
> This patch fixes the problem by removing the test
> "!defined(CONFIG_SLUB_DEBUG_ON)". Therefore, duplicate names are never
> checked if the SLUB allocator is used.
> 
> Note to stable kernel maintainers: when backporint this patch, please
> backport also the patch 3e374919b314f20e2a04f641ebc1093d758f66a4.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org	# 3.6+
> 
> ---
>  mm/slab_common.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-3.14-rc5/mm/slab_common.c
> ===================================================================
> --- linux-3.14-rc5.orig/mm/slab_common.c	2014-03-04 22:47:02.000000000 +0100
> +++ linux-3.14-rc5/mm/slab_common.c	2014-03-04 22:47:08.000000000 +0100
> @@ -56,7 +56,7 @@ static int kmem_cache_sanity_check(struc
>  			continue;
>  		}
>  
> -#if !defined(CONFIG_SLUB) || !defined(CONFIG_SLUB_DEBUG_ON)
> +#if !defined(CONFIG_SLUB)
>  		/*
>  		 * For simplicity, we won't check this in the list of memcg
>  		 * caches. We have control over memcg naming, and if there
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: slab_common: fix the check for duplicate slab names

[Christoph Lameter]

On Tue, 25 Mar 2014, Mike Snitzer wrote:

> This patch still isn't upstream.  Who should be shepherding it to Linus?

Pekka usually does that.

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

Re: slab_common: fix the check for duplicate slab names

[Mike Snitzer]

On Tue, Mar 25 2014 at  2:07pm -0400,
Christoph Lameter <cl@linux.com> wrote:

> On Tue, 25 Mar 2014, Mike Snitzer wrote:
> 
> > This patch still isn't upstream.  Who should be shepherding it to Linus?
> 
> Pekka usually does that.
> 
> Acked-by: Christoph Lameter <cl@linux.com>

This still hasn't gotten upstream.

Pekka, any chance you can pick it up?  Here it is in dm-devel's
kernel.org patchwork: https://patchwork.kernel.org/patch/3768901/

(Though it looks like it needs to be rebased due to the recent commit
794b1248, should Mikulas rebase and re-send?)

Mike

Re: slab_common: fix the check for duplicate slab names

[Pekka Enberg]

On 05/23/2014 11:16 PM, Mike Snitzer wrote:
> On Tue, Mar 25 2014 at  2:07pm -0400,
> Christoph Lameter <cl@linux.com> wrote:
>
>> On Tue, 25 Mar 2014, Mike Snitzer wrote:
>>
>>> This patch still isn't upstream.  Who should be shepherding it to Linus?
>> Pekka usually does that.
>>
>> Acked-by: Christoph Lameter <cl@linux.com>
> This still hasn't gotten upstream.
>
> Pekka, any chance you can pick it up?  Here it is in dm-devel's
> kernel.org patchwork: https://patchwork.kernel.org/patch/3768901/
>
> (Though it looks like it needs to be rebased due to the recent commit
> 794b1248, should Mikulas rebase and re-send?)

I applied it and fixed the conflict by hand.

Please double-check commit 694617474e33b8603fc76e090ed7d09376514b1a in 
my tree:

https://git.kernel.org/cgit/linux/kernel/git/penberg/linux.git/

- Pekka

Re: slab_common: fix the check for duplicate slab names

[Mike Snitzer]

On Fri, May 23 2014 at  5:28pm -0400,
Pekka Enberg <penberg@iki.fi> wrote:

> On 05/23/2014 11:16 PM, Mike Snitzer wrote:
> >On Tue, Mar 25 2014 at  2:07pm -0400,
> >Christoph Lameter <cl@linux.com> wrote:
> >
> >>On Tue, 25 Mar 2014, Mike Snitzer wrote:
> >>
> >>>This patch still isn't upstream.  Who should be shepherding it to Linus?
> >>Pekka usually does that.
> >>
> >>Acked-by: Christoph Lameter <cl@linux.com>
> >This still hasn't gotten upstream.
> >
> >Pekka, any chance you can pick it up?  Here it is in dm-devel's
> >kernel.org patchwork: https://patchwork.kernel.org/patch/3768901/
> >
> >(Though it looks like it needs to be rebased due to the recent commit
> >794b1248, should Mikulas rebase and re-send?)
> 
> I applied it and fixed the conflict by hand.
> 
> Please double-check commit 694617474e33b8603fc76e090ed7d09376514b1a
> in my tree:
> 
> https://git.kernel.org/cgit/linux/kernel/git/penberg/linux.git/

Looks good, thanks.

Re: slab_common: fix the check for duplicate slab names

[Mike Snitzer]

On Fri, May 23 2014 at  5:28pm -0400,
Pekka Enberg <penberg@iki.fi> wrote:

> On 05/23/2014 11:16 PM, Mike Snitzer wrote:
> >On Tue, Mar 25 2014 at  2:07pm -0400,
> >Christoph Lameter <cl@linux.com> wrote:
> >
> >>On Tue, 25 Mar 2014, Mike Snitzer wrote:
> >>
> >>>This patch still isn't upstream.  Who should be shepherding it to Linus?
> >>Pekka usually does that.
> >>
> >>Acked-by: Christoph Lameter <cl@linux.com>
> >This still hasn't gotten upstream.
> >
> >Pekka, any chance you can pick it up?  Here it is in dm-devel's
> >kernel.org patchwork: https://patchwork.kernel.org/patch/3768901/
> >
> >(Though it looks like it needs to be rebased due to the recent commit
> >794b1248, should Mikulas rebase and re-send?)
> 
> I applied it and fixed the conflict by hand.
> 
> Please double-check commit 694617474e33b8603fc76e090ed7d09376514b1a
> in my tree:
> 
> https://git.kernel.org/cgit/linux/kernel/git/penberg/linux.git/

Pekka, this clearly still hasn't landed for 3.16.  Can you please get
it upstream ASAP?  It is a lingering issue that keeps rearing its ugly
head, latest report on Fedora rawhide:

3,2059,887335968,-;kmem_cache_sanity_check (raid5-ffff880074a56010): Cache name already exists.
4,2060,887339337,-;CPU: 1 PID: 12874 Comm: lvm Not tainted 3.16.0-0.rc4.git1.1.fc21.x86_64 #1
4,2061,887342267,-;Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2007
4,2062,887344959,-; 0000000000000000 00000000808b098a ffff880074d23a00 ffffffff81807d5c
4,2063,887347591,-; ffff88007557ab48 ffff880074d23a78 ffffffff811e9451 ffff88007557a800
4,2064,887350230,-; ffff880074d23a78 0000000000000000 0000000000000000 0000000000000690
4,2065,887352882,-;Call Trace:
4,2066,887355210,-; [<ffffffff81807d5c>] dump_stack+0x4d/0x66
4,2067,887357157,-; [<ffffffff811e9451>] kmem_cache_create+0x2c1/0x330
4,2068,887360103,-; [<ffffffffa0272527>] setup_conf+0x677/0x8c0 [raid456]
4,2069,887362901,-; [<ffffffff810e1d78>] ? sched_clock_cpu+0x98/0xc0
4,2070,887364749,-; [<ffffffffa0273350>] run+0x8c0/0xab0 [raid456]
4,2071,887366545,-; [<ffffffff8162f562>] md_run+0x562/0x980
4,2072,887368240,-; [<ffffffffa028756c>] ? raid_ctr+0xf3c/0x13f8 [dm_raid]
4,2073,887370163,-; [<ffffffff810fbc44>] ? static_obj+0x34/0x50
4,2074,887371973,-; [<ffffffff810fc4ac>] ? lockdep_init_map+0x6c/0x570
4,2075,887373889,-; [<ffffffffa0287578>] raid_ctr+0xf48/0x13f8 [dm_raid]
4,2076,887375849,-; [<ffffffff8163c5b0>] dm_table_add_target+0x160/0x3b0
4,2077,887377816,-; [<ffffffff8163fb14>] table_load+0x144/0x360
4,2078,887379645,-; [<ffffffff8163f9d0>] ? retrieve_status+0x1c0/0x1c0
4,2079,887381536,-; [<ffffffff816407db>] ctl_ioctl+0x25b/0x550
4,2080,887383318,-; [<ffffffff81640ae3>] dm_ctl_ioctl+0x13/0x20
4,2081,887385130,-; [<ffffffff812628e0>] do_vfs_ioctl+0x2f0/0x520
4,2082,887387025,-; [<ffffffff8126f2bd>] ? __fget_light+0x13d/0x160
4,2083,887388895,-; [<ffffffff81262b91>] SyS_ioctl+0x81/0xa0
4,2084,887390694,-; [<ffffffff8115fbac>] ? __audit_syscall_entry+0x9c/0xf0
4,2085,887392656,-; [<ffffffff81811969>] system_call_fastpath+0x16/0x1b
4,2086,887394585,-;kmem_cache_create(raid5-ffff880074a56010) failed with error -22
4,2087,887395592,-;CPU: 1 PID: 12874 Comm: lvm Not tainted 3.16.0-0.rc4.git1.1.fc21.x86_64 #1

[patch] slab_common: fix the check for duplicate slab names

[David Rientjes]

From: Mikulas Patocka <mpatocka@redhat.com>

The patch 3e374919b314f20e2a04f641ebc1093d758f66a4 is supposed to fix the
problem where kmem_cache_create incorrectly reports duplicate cache name
and fails. The problem is described in the header of that patch.

However, the patch doesn't really fix the problem because of these
reasons:

* the logic to test for debugging is reversed. It was intended to perform
  the check only if slub debugging is enabled (which implies that caches
  with the same parameters are not merged). Therefore, there should be
  #if !defined(CONFIG_SLUB) || defined(CONFIG_SLUB_DEBUG_ON)
  The current code has the condition reversed and performs the test if
  debugging is disabled.

* slub debugging may be enabled or disabled based on kernel command line,
  CONFIG_SLUB_DEBUG_ON is just the default settings. Therefore the test
  based on definition of CONFIG_SLUB_DEBUG_ON is unreliable.

This patch fixes the problem by removing the test
"!defined(CONFIG_SLUB_DEBUG_ON)". Therefore, duplicate names are never
checked if the SLUB allocator is used.

Note to stable kernel maintainers: when backporint this patch, please
backport also the patch 3e374919b314f20e2a04f641ebc1093d758f66a4.

Cc: stable@vger.kernel.org	# 3.6+
Acked-by: Christoph Lameter <cl@linux.com>
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 Andrew, this is in Pekka's tree but has not managed to be pushed for 
 3.16, mind picking it up?

 I'll check to see if there's anything else in Pekka's tree that you need.

 mm/slab_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -55,7 +55,7 @@ static int kmem_cache_sanity_check(const char *name, size_t size)
 			continue;
 		}
 
-#if !defined(CONFIG_SLUB) || !defined(CONFIG_SLUB_DEBUG_ON)
+#if !defined(CONFIG_SLUB)
 		if (!strcmp(s->name, name)) {
 			pr_err("%s (%s): Cache name already exists.\n",
 			       __func__, name);

[git pull] stable mm/slab_common.c fix for 3.16-rc7

[Mike Snitzer]

Hi Linus,

Not sure you'll be OK with what I've done or not but I pulled in a
1-liner "slab/urgent" fix that Pekka staged a couple months ago.  I've
made it available through a signed tag in the linux-dm.git tree.  My
reasoning on why this is OK is it regularly impacts DM and Pekka already
Signed-off on it (as did other mm developers).  I'm pretty sure Pekka
just forgot to follow through preparing a pull request for 3.15.

If you'd rather take this direct from Pekka (or wait for Andrew to pick
up the same patch which David just sent him) that is fine by me, I just
want the issue fixed.

The following changes since commit 048e5a07f282c57815b3901d4a68a77fa131ce0a:

  dm cache metadata: do not allow the data block size to change (2014-07-15 14:07:50 -0400)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git tags/urgent-slab-fix

for you to fetch changes up to 45ccaf4764278f6544db412d38a1bae056ee3acc:

  Merge branch 'slab/urgent' of git://git.kernel.org/pub/scm/linux/kernel/git/penberg/linux into for-3.16-rcX (2014-07-22 18:38:27 -0400)

Please pull, thanks.
Mike

----------------------------------------------------------------

This fixes the broken duplicate slab name check in
kmem_cache_sanity_check() that has been repeatedly reported (as recently
as today against Fedora rawhide).  Pekka seemed to have it staged for a
late 3.15-rc in his 'slab/urgent' branch but never sent a pull request,
see: https://lkml.org/lkml/2014/5/23/648

----------------------------------------------------------------
Mike Snitzer (1):
      Merge branch 'slab/urgent' of git://git.kernel.org/.../penberg/linux into for-3.16-rcX

Mikulas Patocka (1):
      slab_common: fix the check for duplicate slab names

 mm/slab_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)