WARNING: CPU: 1 PID: 495 at mm/slab_common.c:69 kmem_cache_create+0x1a9/0x330()

I guess someone working over the summertime. :)

------------[ cut here ]------------
WARNING: CPU: 1 PID: 495 at mm/slab_common.c:69 kmem_cache_create+0x1a9/0x330()
Modules linked in: virtio_net virtio_scsi(+) drm virtio_pci i2c_core virtio_ring ata_generic pata_acpi virtio sunrpc dm_crypt dm_round_robin linear raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor xor async_tx raid6_pq raid1 raid0 iscsi_ibft iscsi_boot_sysfs floppy iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi squashfs cramfs edd dm_multipath
CPU: 1 PID: 495 Comm: systemd-udevd Not tainted 3.16.0-0.rc5.git0.1.fc21.x86_64 #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
  0000000000000000 00000000bfd3b05a ffff88007f55fa40 ffffffff8171b4e3
  0000000000000000 ffff88007f55fa78 ffffffff8108f30d ffff880079175d40
  ffff88007f55ffd8 ffffffff81c6de08 ffff88007f55ffd8 ffff88007f55ffd8
Call Trace:
  [<ffffffff8171b4e3>] dump_stack+0x45/0x56
  [<ffffffff8108f30d>] warn_slowpath_common+0x7d/0xa0
  [<ffffffff8108f43a>] warn_slowpath_null+0x1a/0x20
  [<ffffffff811a7c59>] kmem_cache_create+0x1a9/0x330
  [<ffffffff814a4070>] scsi_setup_command_freelist+0x120/0x270
  [<ffffffff814a4d90>] scsi_add_host_with_dma+0x60/0x2b0
  [<ffffffffa017b9c9>] virtscsi_probe+0x269/0x2af [virtio_scsi]
  [<ffffffffa01877c0>] ? vp_reset+0x90/0x90 [virtio_pci]
  [<ffffffffa010e1d9>] virtio_dev_probe+0xe9/0x160 [virtio]
  [<ffffffff81483d83>] driver_probe_device+0xa3/0x400
  [<ffffffff814841ab>] __driver_attach+0x8b/0x90
  [<ffffffff81484120>] ? __device_attach+0x40/0x40
  [<ffffffff81481b23>] bus_for_each_dev+0x73/0xc0
  [<ffffffff8148381e>] driver_attach+0x1e/0x20
  [<ffffffff814833f0>] bus_add_driver+0x180/0x250
  [<ffffffffa0180000>] ? 0xffffffffa017ffff
  [<ffffffff81484974>] driver_register+0x64/0xf0
  [<ffffffffa010e550>] register_virtio_driver+0x20/0x40 [virtio]
  [<ffffffffa0180085>] init+0x85/0x1000 [virtio_scsi]
  [<ffffffff81002148>] do_one_initcall+0xd8/0x210
  [<ffffffff811c3952>] ? __vunmap+0xa2/0x100
  [<ffffffff8110d951>] load_module+0x2061/0x2600
  [<ffffffff811092f0>] ? store_uevent+0x70/0x70
  [<ffffffff8110dfbd>] SyS_init_module+0xcd/0x120
  [<ffffffff817223a9>] system_call_fastpath+0x16/0x1b
---[ end trace 64d7cf025fd3bf4a ]---

https://bugzilla.redhat.com/show_bug.cgi?id=1121092

poma

Slab warns, because the name of the cache being created contains spaces. The "bad" cache is created by scsi_get_host_cmd_pool. Its name (pool->cmd_name) is initialized by scsi_alloc_host_cmd_pool as follows:

	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->name);

So, if hostt->name contains spaces, the cache name will also contain spaces and we'll get the warning. And hostt->name can contain spaces, e.g. virtscsi_host_template_single.name="Virtio SCSI HBA".

I guess someone working over the summertime. :)

Quoted 6 lines · Vladimir Davydov

Slab warns, because the name of the cache being created contains spaces. The "bad" cache is created by scsi_get_host_cmd_pool. Its name (pool->cmd_name) is initialized by scsi_alloc_host_cmd_pool as follows:

	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->name);

Quoted 3 lines

So, if hostt->name contains spaces, the cache name will also contain spaces and we'll get the warning. And hostt->name can contain spaces, e.g. virtscsi_host_template_single.name="Virtio SCSI HBA".

Or might not even be present. I'll send a patch to replace it with ->proc_name, which must not contain spaces and is generally shorter as well.

Quoted 1 line · poma

I guess someone working over the summertime. :)

Cache names should not contain blanks. I guess the

WARN_ON(strchr(name, ' ')); /* It confuses parsers */

was triggered?

Quoted 11 lines · Christoph Hellwig

Quoted 9 lines · Vladimir Davydov

Slab warns, because the name of the cache being created contains spaces. The "bad" cache is created by scsi_get_host_cmd_pool. Its name (pool->cmd_name) is initialized by scsi_alloc_host_cmd_pool as follows:

	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->name);

So, if hostt->name contains spaces, the cache name will also contain spaces and we'll get the warning. And hostt->name can contain spaces, e.g. virtscsi_host_template_single.name="Virtio SCSI HBA".

Quoted 4 lines

Or might not even be present. I'll send a patch to replace it with ->proc_name, which must not contain spaces and is generally shorter as well.

Is this what you thought?

Quoted 12 lines · poma

Quoted 11 lines · Christoph Hellwig

Quoted 9 lines · Vladimir Davydov

Slab warns, because the name of the cache being created contains spaces. The "bad" cache is created by scsi_get_host_cmd_pool. Its name (pool->cmd_name) is initialized by scsi_alloc_host_cmd_pool as follows:

	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->name);

So, if hostt->name contains spaces, the cache name will also contain spaces and we'll get the warning. And hostt->name can contain spaces, e.g. virtscsi_host_template_single.name="Virtio SCSI HBA".

Quoted 6 lines

Quoted 4 lines

Or might not even be present. I'll send a patch to replace it with ->proc_name, which must not contain spaces and is generally shorter as well.

Is this what you thought?

No, he means this, if you want to try it.

James

---

drivers/scsi/scsi.c +0 -0

index 88d46fe..eb07a9b 100644

Quoted 33 lines · James Bottomley

Quoted 12 lines · poma

Quoted 11 lines · Christoph Hellwig

Quoted 9 lines · Vladimir Davydov

Slab warns, because the name of the cache being created contains spaces. The "bad" cache is created by scsi_get_host_cmd_pool. Its name (pool->cmd_name) is initialized by scsi_alloc_host_cmd_pool as follows:

	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->name);

So, if hostt->name contains spaces, the cache name will also contain spaces and we'll get the warning. And hostt->name can contain spaces, e.g. virtscsi_host_template_single.name="Virtio SCSI HBA".

Quoted 6 lines

Quoted 4 lines

Or might not even be present. I'll send a patch to replace it with ->proc_name, which must not contain spaces and is generally shorter as well.

Is this what you thought?

No, he means this, if you want to try it.

James

---

drivers/scsi/scsi.c +0 -0

index 88d46fe..eb07a9b 100644

drivers/scsi/scsi.c scsi_alloc_host_cmd_pool(struct Scsi_Host *shost) · line 375

Quoted 10 lines

drivers/scsi/scsi.c +2 -2

@@ -368,8 +368,8 @@ scsi_alloc_host_cmd_pool(struct Scsi_Host *shost)
   	if (!pool)
   		return NULL;
 
-	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->name);
-	pool->sense_name = kasprintf(GFP_KERNEL, "%s_sense", hostt->name);
+	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->proc_name);
+	pool->sense_name = kasprintf(GFP_KERNEL, "%s_sense", hostt->proc_name);
   	if (!pool->cmd_name || !pool->sense_name) {
   		scsi_free_host_cmd_pool(pool);
   		return NULL;

Man, I just now read it correctly - "So, if hostt->name contains spaces". Thanks.

I'll be back.

Quoted 4 lines · Christoph Lameter

Quoted 1 line · poma

I guess someone working over the summertime. :)

Quoted 6 lines

Cache names should not contain blanks. I guess the

WARN_ON(strchr(name, ' ')); /* It confuses parsers */

was triggered?

I can only guess also. ;)

Quoted 1 line

Is this what you thought?

No, he means this, if you want to try it.

Quoted 3 lines · James Bottomley

Quoted 1 line

Is this what you thought?

No, he means this, if you want to try it.

Yes, that's what I mean.

Thanks for the tip.
Is there a patch somewhere?

Quoted 2 lines · poma

Thanks for the tip.
Is there a patch somewhere?

James sent the patch earlier and you replied to it.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Christoph" == Christoph Hellwig <[email protected]> writes:

Here's a formal one.

James, can I get your signoff for it?

Vladimir, can I get a reviewed-by from you (or anyone else)?

---
From 73b1034ab1418e2dea75ccf642bc85c728b57313 Mon Sep 17 00:00:00 2001
From: James Bottomley <[email protected]>
Date: Sat, 26 Jul 2014 12:21:26 -0400
Subject: scsi: use short driver name for per-driver cmd slab caches

hostt->name might contain space, so use the ->proc_name short name instead when creating per-driver command slabs.

Reported-by Vladimir Davydov <[email protected]>

Tested-by Vladimir Davydov <[email protected]>

---
 drivers/scsi/scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/scsi.c +2 -2

index 33318f5..df33060 100644
@@ -365,8 +365,8 @@ scsi_alloc_host_cmd_pool(struct Scsi_Host *shost)
 	if (!pool)
 		return NULL;
 
-	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->name);
-	pool->sense_name = kasprintf(GFP_KERNEL, "%s_sense", hostt->name);
+	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->proc_name);
+	pool->sense_name = kasprintf(GFP_KERNEL, "%s_sense", hostt->proc_name);
 	if (!pool->cmd_name || !pool->sense_name) {
 		scsi_free_host_cmd_pool(pool);
 		return NULL;

Signature · 1 line

1.9.1

Here's a formal one.

James, can I get your signoff for it?

Vladimir, can I get a reviewed-by from you (or anyone else)?

---

Quoted 1 line

From 73b1034ab1418e2dea75ccf642bc85c728b57313 Mon Sep 17 00:00:00 2001

From: James Bottomley <[email protected]>

Date: Sat, 26 Jul 2014 12:21:26 -0400
Subject: scsi: use short driver name for per-driver cmd slab caches

hostt->name might contain space, so use the ->proc_name short name instead when creating per-driver command slabs.

Reported-by Vladimir Davydov <[email protected]>

Tested-by Vladimir Davydov <[email protected]>

---
 drivers/scsi/scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/scsi.c +2 -2

index 33318f5..df33060 100644
@@ -365,8 +365,8 @@ scsi_alloc_host_cmd_pool(struct Scsi_Host *shost)
 	if (!pool)
 		return NULL;
 
-	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->name);
-	pool->sense_name = kasprintf(GFP_KERNEL, "%s_sense", hostt->name);
+	pool->cmd_name = kasprintf(GFP_KERNEL, "%s_cmd", hostt->proc_name);
+	pool->sense_name = kasprintf(GFP_KERNEL, "%s_sense", hostt->proc_name);
 	if (!pool->cmd_name || !pool->sense_name) {
 		scsi_free_host_cmd_pool(pool);
 		return NULL;

That needs to be

From: James Bottomley <[email protected]>

As well. I do list handling on hansenpartnership.com to minimise exchange wreckage on mailinglists, but I should acknowledge Parallels as supporting the work I do.

Quoted 7 lines · James Bottomley

That needs to be

From: James Bottomley <[email protected]>

As well. I do list handling on hansenpartnership.com to minimise exchange wreckage on mailinglists, but I should acknowledge Parallels as supporting the work I do.

Thanks, I've update the author, added a Cc to ћtable and pushed it out to the core-for-3.17 branch.

Quoted 7 lines · James Bottomley

That needs to be

From: James Bottomley <[email protected]>

As well. I do list handling on hansenpartnership.com to minimise exchange wreckage on mailinglists, but I should acknowledge Parallels as supporting the work I do.

Thanks, I've update the author, added a Cc to ћtable and pushed it out to the core-for-3.17 branch.

Quoted 2 lines

Thanks, I've update the author, added a Cc to ћtable and pushed it out to the core-for-3.17 branch.

This fixes a bug in the 3.16 kernel. Why wouldn't it be sent to Linus for inclusion in the final release there?

Quoted 5 lines · Josh Boyer

Quoted 2 lines

Thanks, I've update the author, added a Cc to ћtable and pushed it out to the core-for-3.17 branch.

This fixes a bug in the 3.16 kernel. Why wouldn't it be sent to Linus for inclusion in the final release there?

I'm only collecting patches for scsi, but James remains formal maintainer, so I don't send anything to Linus. Given that delays between me committing things, them getting picked up by James and finally making it to Linux-next I don't feel like another for-3.16 branch at this point is easily workable.

If James wants to cherry pick it and send it on at this time that might still work fine, but he seems fairly busy.

It's been remarkably current, I believe ... it's already up to date.

However, we only have 2 -next builds between now and the anticipated 3.16 release (unless Linus does another -rc) so there's not enough time for a patch that isn't already in (although this one is).

Quoted 2 lines

If James wants to cherry pick it and send it on at this time that might still work fine, but he seems fairly busy.

I'll pull it out and refactor the tree.

Here's a formal one.

James, can I get your signoff for it?

Vladimir, can I get a reviewed-by from you (or anyone else)?

---
From 73b1034ab1418e2dea75ccf642bc85c728b57313 Mon Sep 17 00:00:00 2001
From: James Bottomley <[email protected]>
Date: Sat, 26 Jul 2014 12:21:26 -0400
Subject: scsi: use short driver name for per-driver cmd slab caches

hostt->name might contain space, so use the ->proc_name short name instead when creating per-driver command slabs.

Reported-by Vladimir Davydov <[email protected]>

Tested-by Vladimir Davydov <[email protected]>

---
 drivers/scsi/scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/scsi.c +0 -0

index 33318f5..df33060 100644

Some drivers like qla2xxx do not set proc_name. I think if 2 drivers like that are loaded then you will hit some other warns/bugs in the kmem cache setup code right?

Quoted 3 lines · Mike Christie

Some drivers like qla2xxx do not set proc_name. I think if 2 drivers like that are loaded then you will hit some other warns/bugs in the kmem cache setup code right?

Drivers have to opt into using their own caches by setting .cmd_size in the host template. We could enforce they set ->proc_name for them, but I'd rather keep the amount of sanity checks low unless there's a real need.