Skip to content

Commit 147b27e

Browse files
sagigrimbergChristoph Hellwig
sagigrimberg
authored and
Christoph Hellwig
committed
nvme-pci: allocate device queues storage space at probe
It may cause race by setting 'nvmeq' in nvme_init_request() because .init_request is called inside switching io scheduler, which may happen when the NVMe device is being resetted and its nvme queues are being freed and created. We don't have any sync between the two pathes. This patch changes the nvmeq allocation to occur at probe time so there is no way we can dereference it at init_request. [ 93.268391] kernel BUG at drivers/nvme/host/pci.c:408! [ 93.274146] invalid opcode: 0000 [#1] SMP [ 93.278618] Modules linked in: nfsv3 nfs_acl rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache sunrpc ipmi_ssif vfat fat intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel iTCO_wdt intel_cstate ipmi_si iTCO_vendor_support intel_uncore mxm_wmi mei_me ipmi_devintf intel_rapl_perf pcspkr sg ipmi_msghandler lpc_ich dcdbas mei shpchp acpi_power_meter wmi dm_multipath ip_tables xfs libcrc32c sd_mod mgag200 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm ahci libahci nvme libata crc32c_intel nvme_core tg3 megaraid_sas ptp i2c_core pps_core dm_mirror dm_region_hash dm_log dm_mod [ 93.349071] CPU: 5 PID: 1842 Comm: sh Not tainted 4.15.0-rc2.ming+ #4 [ 93.356256] Hardware name: Dell Inc. PowerEdge R730xd/072T6D, BIOS 2.5.5 08/16/2017 [ 93.364801] task: 00000000fb8abf2a task.stack: 0000000028bd82d1 [ 93.371408] RIP: 0010:nvme_init_request+0x36/0x40 [nvme] [ 93.377333] RSP: 0018:ffffc90002537ca8 EFLAGS: 00010246 [ 93.383161] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000008 [ 93.391122] RDX: 0000000000000000 RSI: ffff880276ae0000 RDI: ffff88047bae9008 [ 93.399084] RBP: ffff88047bae9008 R08: ffff88047bae9008 R09: 0000000009dabc00 [ 93.407045] R10: 0000000000000004 R11: 000000000000299c R12: ffff880186bc1f00 [ 93.415007] R13: ffff880276ae0000 R14: 0000000000000000 R15: 0000000000000071 [ 93.422969] FS: 00007f33cf288740(0000) GS:ffff88047ba80000(0000) knlGS:0000000000000000 [ 93.431996] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 93.438407] CR2: 00007f33cf28e000 CR3: 000000047e5bb006 CR4: 00000000001606e0 [ 93.446368] Call Trace: [ 93.449103] blk_mq_alloc_rqs+0x231/0x2a0 [ 93.453579] blk_mq_sched_alloc_tags.isra.8+0x42/0x80 [ 93.459214] blk_mq_init_sched+0x7e/0x140 [ 93.463687] elevator_switch+0x5a/0x1f0 [ 93.467966] ? elevator_get.isra.17+0x52/0xc0 [ 93.472826] elv_iosched_store+0xde/0x150 [ 93.477299] queue_attr_store+0x4e/0x90 [ 93.481580] kernfs_fop_write+0xfa/0x180 [ 93.485958] __vfs_write+0x33/0x170 [ 93.489851] ? __inode_security_revalidate+0x4c/0x60 [ 93.495390] ? selinux_file_permission+0xda/0x130 [ 93.500641] ? _cond_resched+0x15/0x30 [ 93.504815] vfs_write+0xad/0x1a0 [ 93.508512] SyS_write+0x52/0xc0 [ 93.512113] do_syscall_64+0x61/0x1a0 [ 93.516199] entry_SYSCALL64_slow_path+0x25/0x25 [ 93.521351] RIP: 0033:0x7f33ce96aab0 [ 93.525337] RSP: 002b:00007ffe57570238 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 [ 93.533785] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f33ce96aab0 [ 93.541746] RDX: 0000000000000006 RSI: 00007f33cf28e000 RDI: 0000000000000001 [ 93.549707] RBP: 00007f33cf28e000 R08: 000000000000000a R09: 00007f33cf288740 [ 93.557669] R10: 00007f33cf288740 R11: 0000000000000246 R12: 00007f33cec42400 [ 93.565630] R13: 0000000000000006 R14: 0000000000000001 R15: 0000000000000000 [ 93.573592] Code: 4c 8d 40 08 4c 39 c7 74 16 48 8b 00 48 8b 04 08 48 85 c0 74 16 48 89 86 78 01 00 00 31 c0 c3 8d 4a 01 48 63 c9 48 c1 e1 03 eb de <0f> 0b 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 85 f6 53 48 89 [ 93.594676] RIP: nvme_init_request+0x36/0x40 [nvme] RSP: ffffc90002537ca8 [ 93.602273] ---[ end trace 810dde3993e5f14e ]--- Reported-by: Yi Zhang <[email protected]> Signed-off-by: Sagi Grimberg <[email protected]> Signed-off-by: Christoph Hellwig <[email protected]>
1 parent 79c48cc commit 147b27e

File tree

1 file changed

+26
-37
lines changed

1 file changed

+26
-37
lines changed

‎drivers/nvme/host/pci.c

Lines changed: 26 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
7575
* Represents an NVM Express device. Each nvme_dev is a PCI function.
7676
*/
7777
struct nvme_dev {
78-
struct nvme_queue **queues;
78+
struct nvme_queue *queues;
7979
struct blk_mq_tag_set tagset;
8080
struct blk_mq_tag_set admin_tagset;
8181
u32 __iomem *dbs;
@@ -365,7 +365,7 @@ static int nvme_admin_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
365365
unsigned int hctx_idx)
366366
{
367367
struct nvme_dev *dev = data;
368-
struct nvme_queue *nvmeq = dev->queues[0];
368+
struct nvme_queue *nvmeq = &dev->queues[0];
369369

370370
WARN_ON(hctx_idx != 0);
371371
WARN_ON(dev->admin_tagset.tags[0] != hctx->tags);
@@ -387,7 +387,7 @@ static int nvme_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
387387
unsigned int hctx_idx)
388388
{
389389
struct nvme_dev *dev = data;
390-
struct nvme_queue *nvmeq = dev->queues[hctx_idx + 1];
390+
struct nvme_queue *nvmeq = &dev->queues[hctx_idx + 1];
391391

392392
if (!nvmeq->tags)
393393
nvmeq->tags = &dev->tagset.tags[hctx_idx];
@@ -403,7 +403,7 @@ static int nvme_init_request(struct blk_mq_tag_set *set, struct request *req,
403403
struct nvme_dev *dev = set->driver_data;
404404
struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
405405
int queue_idx = (set == &dev->tagset) ? hctx_idx + 1 : 0;
406-
struct nvme_queue *nvmeq = dev->queues[queue_idx];
406+
struct nvme_queue *nvmeq = &dev->queues[queue_idx];
407407

408408
BUG_ON(!nvmeq);
409409
iod->nvmeq = nvmeq;
@@ -1046,7 +1046,7 @@ static int nvme_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
10461046
static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl)
10471047
{
10481048
struct nvme_dev *dev = to_nvme_dev(ctrl);
1049-
struct nvme_queue *nvmeq = dev->queues[0];
1049+
struct nvme_queue *nvmeq = &dev->queues[0];
10501050
struct nvme_command c;
10511051

10521052
memset(&c, 0, sizeof(c));
@@ -1282,18 +1282,15 @@ static void nvme_free_queue(struct nvme_queue *nvmeq)
12821282
if (nvmeq->sq_cmds)
12831283
dma_free_coherent(nvmeq->q_dmadev, SQ_SIZE(nvmeq->q_depth),
12841284
nvmeq->sq_cmds, nvmeq->sq_dma_addr);
1285-
kfree(nvmeq);
12861285
}
12871286

12881287
static void nvme_free_queues(struct nvme_dev *dev, int lowest)
12891288
{
12901289
int i;
12911290

12921291
for (i = dev->ctrl.queue_count - 1; i >= lowest; i--) {
1293-
struct nvme_queue *nvmeq = dev->queues[i];
12941292
dev->ctrl.queue_count--;
1295-
dev->queues[i] = NULL;
1296-
nvme_free_queue(nvmeq);
1293+
nvme_free_queue(&dev->queues[i]);
12971294
}
12981295
}
12991296

@@ -1325,10 +1322,8 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
13251322

13261323
static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
13271324
{
1328-
struct nvme_queue *nvmeq = dev->queues[0];
1325+
struct nvme_queue *nvmeq = &dev->queues[0];
13291326

1330-
if (!nvmeq)
1331-
return;
13321327
if (nvme_suspend_queue(nvmeq))
13331328
return;
13341329

@@ -1384,13 +1379,10 @@ static int nvme_alloc_sq_cmds(struct nvme_dev *dev, struct nvme_queue *nvmeq,
13841379
return 0;
13851380
}
13861381

1387-
static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
1388-
int depth, int node)
1382+
static int nvme_alloc_queue(struct nvme_dev *dev, int qid,
1383+
int depth, int node)
13891384
{
1390-
struct nvme_queue *nvmeq = kzalloc_node(sizeof(*nvmeq), GFP_KERNEL,
1391-
node);
1392-
if (!nvmeq)
1393-
return NULL;
1385+
struct nvme_queue *nvmeq = &dev->queues[qid];
13941386

13951387
nvmeq->cqes = dma_zalloc_coherent(dev->dev, CQ_SIZE(depth),
13961388
&nvmeq->cq_dma_addr, GFP_KERNEL);
@@ -1409,17 +1401,15 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
14091401
nvmeq->q_depth = depth;
14101402
nvmeq->qid = qid;
14111403
nvmeq->cq_vector = -1;
1412-
dev->queues[qid] = nvmeq;
14131404
dev->ctrl.queue_count++;
14141405

1415-
return nvmeq;
1406+
return 0;
14161407

14171408
free_cqdma:
14181409
dma_free_coherent(dev->dev, CQ_SIZE(depth), (void *)nvmeq->cqes,
14191410
nvmeq->cq_dma_addr);
14201411
free_nvmeq:
1421-
kfree(nvmeq);
1422-
return NULL;
1412+
return -ENOMEM;
14231413
}
14241414

14251415
static int queue_request_irq(struct nvme_queue *nvmeq)
@@ -1592,14 +1582,12 @@ static int nvme_pci_configure_admin_queue(struct nvme_dev *dev)
15921582
if (result < 0)
15931583
return result;
15941584

1595-
nvmeq = dev->queues[0];
1596-
if (!nvmeq) {
1597-
nvmeq = nvme_alloc_queue(dev, 0, NVME_AQ_DEPTH,
1598-
dev_to_node(dev->dev));
1599-
if (!nvmeq)
1600-
return -ENOMEM;
1601-
}
1585+
result = nvme_alloc_queue(dev, 0, NVME_AQ_DEPTH,
1586+
dev_to_node(dev->dev));
1587+
if (result)
1588+
return result;
16021589

1590+
nvmeq = &dev->queues[0];
16031591
aqa = nvmeq->q_depth - 1;
16041592
aqa |= aqa << 16;
16051593

@@ -1629,7 +1617,7 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
16291617

16301618
for (i = dev->ctrl.queue_count; i <= dev->max_qid; i++) {
16311619
/* vector == qid - 1, match nvme_create_queue */
1632-
if (!nvme_alloc_queue(dev, i, dev->q_depth,
1620+
if (nvme_alloc_queue(dev, i, dev->q_depth,
16331621
pci_irq_get_node(to_pci_dev(dev->dev), i - 1))) {
16341622
ret = -ENOMEM;
16351623
break;
@@ -1638,7 +1626,7 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
16381626

16391627
max = min(dev->max_qid, dev->ctrl.queue_count - 1);
16401628
for (i = dev->online_queues; i <= max; i++) {
1641-
ret = nvme_create_queue(dev->queues[i], i);
1629+
ret = nvme_create_queue(&dev->queues[i], i);
16421630
if (ret)
16431631
break;
16441632
}
@@ -1894,7 +1882,7 @@ static int nvme_setup_host_mem(struct nvme_dev *dev)
18941882

18951883
static int nvme_setup_io_queues(struct nvme_dev *dev)
18961884
{
1897-
struct nvme_queue *adminq = dev->queues[0];
1885+
struct nvme_queue *adminq = &dev->queues[0];
18981886
struct pci_dev *pdev = to_pci_dev(dev->dev);
18991887
int result, nr_io_queues;
19001888
unsigned long size;
@@ -2020,7 +2008,7 @@ static void nvme_disable_io_queues(struct nvme_dev *dev, int queues)
20202008
retry:
20212009
timeout = ADMIN_TIMEOUT;
20222010
for (; i > 0; i--, sent++)
2023-
if (nvme_delete_queue(dev->queues[i], opcode))
2011+
if (nvme_delete_queue(&dev->queues[i], opcode))
20242012
break;
20252013

20262014
while (sent--) {
@@ -2212,15 +2200,15 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
22122200

22132201
queues = dev->online_queues - 1;
22142202
for (i = dev->ctrl.queue_count - 1; i > 0; i--)
2215-
nvme_suspend_queue(dev->queues[i]);
2203+
nvme_suspend_queue(&dev->queues[i]);
22162204

22172205
if (dead) {
22182206
/* A device might become IO incapable very soon during
22192207
* probe, before the admin queue is configured. Thus,
22202208
* queue_count can be 0 here.
22212209
*/
22222210
if (dev->ctrl.queue_count)
2223-
nvme_suspend_queue(dev->queues[0]);
2211+
nvme_suspend_queue(&dev->queues[0]);
22242212
} else {
22252213
nvme_disable_io_queues(dev, queues);
22262214
nvme_disable_admin_queue(dev, shutdown);
@@ -2482,8 +2470,9 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
24822470
dev = kzalloc_node(sizeof(*dev), GFP_KERNEL, node);
24832471
if (!dev)
24842472
return -ENOMEM;
2485-
dev->queues = kzalloc_node((num_possible_cpus() + 1) * sizeof(void *),
2486-
GFP_KERNEL, node);
2473+
2474+
dev->queues = kcalloc_node(num_possible_cpus() + 1,
2475+
sizeof(struct nvme_queue), GFP_KERNEL, node);
24872476
if (!dev->queues)
24882477
goto free;
24892478

0 commit comments

Comments
 (0)