diff mbox

slab_common: fix the check for duplicate slab names

Message ID alpine.LRH.2.02.1403041711300.29476@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Mikulas Patocka March 4, 2014, 10:13 p.m. UTC 
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(-)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

David Rientjes March 4, 2014, 10:48 p.m. UTC | #1 
On Tue, 4 Mar 2014, Mikulas Patocka 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>

Acked-by: David Rientjes <rientjes@google.com>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer March 25, 2014, 5:03 p.m. UTC | #2 
[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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Christoph Lameter (Ampere) March 25, 2014, 6:07 p.m. UTC | #3 
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>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer May 23, 2014, 8:16 p.m. UTC | #4 
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Pekka Enberg May 23, 2014, 9:28 p.m. UTC | #5 
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer May 24, 2014, 12:37 a.m. UTC | #6 
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.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Mike Snitzer July 22, 2014, 10:14 p.m. UTC | #7 
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox

Patch

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