|
| 1 | +null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues' |
| 2 | + |
| 3 | +jira LE-1907 |
| 4 | +Rebuild_History Non-Buildable kernel-4.18.0-553.16.1.el8_10 |
| 5 | +commit-author Yu Kuai < [email protected]> |
| 6 | +commit a2db328b0839312c169eb42746ec46fc1ab53ed2 |
| 7 | +Empty-Commit: Cherry-Pick Conflicts during history rebuild. |
| 8 | +Will be included in final tarball splat. Ref for failed cherry-pick at: |
| 9 | +ciq/ciq_backports/kernel-4.18.0-553.16.1.el8_10/a2db328b.failed |
| 10 | + |
| 11 | +Writing 'power' and 'submit_queues' concurrently will trigger kernel |
| 12 | +panic: |
| 13 | + |
| 14 | +Test script: |
| 15 | + |
| 16 | +modprobe null_blk nr_devices=0 |
| 17 | +mkdir -p /sys/kernel/config/nullb/nullb0 |
| 18 | +while true; do echo 1 > submit_queues; echo 4 > submit_queues; done & |
| 19 | +while true; do echo 1 > power; echo 0 > power; done |
| 20 | + |
| 21 | +Test result: |
| 22 | + |
| 23 | +BUG: kernel NULL pointer dereference, address: 0000000000000148 |
| 24 | +Oops: 0000 [#1] PREEMPT SMP |
| 25 | +RIP: 0010:__lock_acquire+0x41d/0x28f0 |
| 26 | +Call Trace: |
| 27 | + <TASK> |
| 28 | + lock_acquire+0x121/0x450 |
| 29 | + down_write+0x5f/0x1d0 |
| 30 | + simple_recursive_removal+0x12f/0x5c0 |
| 31 | + blk_mq_debugfs_unregister_hctxs+0x7c/0x100 |
| 32 | + blk_mq_update_nr_hw_queues+0x4a3/0x720 |
| 33 | + nullb_update_nr_hw_queues+0x71/0xf0 [null_blk] |
| 34 | + nullb_device_submit_queues_store+0x79/0xf0 [null_blk] |
| 35 | + configfs_write_iter+0x119/0x1e0 |
| 36 | + vfs_write+0x326/0x730 |
| 37 | + ksys_write+0x74/0x150 |
| 38 | + |
| 39 | +This is because del_gendisk() can concurrent with |
| 40 | +blk_mq_update_nr_hw_queues(): |
| 41 | + |
| 42 | +nullb_device_power_store nullb_apply_submit_queues |
| 43 | + null_del_dev |
| 44 | + del_gendisk |
| 45 | + nullb_update_nr_hw_queues |
| 46 | + if (!dev->nullb) |
| 47 | + // still set while gendisk is deleted |
| 48 | + return 0 |
| 49 | + blk_mq_update_nr_hw_queues |
| 50 | + dev->nullb = NULL |
| 51 | + |
| 52 | +Fix this problem by resuing the global mutex to protect |
| 53 | +nullb_device_power_store() and nullb_update_nr_hw_queues() from configfs. |
| 54 | + |
| 55 | +Fixes: 45919fbfe1c4 ("null_blk: Enable modifying 'submit_queues' after an instance has been configured") |
| 56 | +Reported-and-tested-by: Yi Zhang < [email protected]> |
| 57 | +Closes: https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStuwZA@mail.gmail.com/ |
| 58 | + Signed-off-by: Yu Kuai < [email protected]> |
| 59 | + Reviewed-by: Zhu Yanjun < [email protected]> |
| 60 | +Link: https://lore.kernel.org/r/ [email protected] |
| 61 | + Signed-off-by: Jens Axboe < [email protected]> |
| 62 | +(cherry picked from commit a2db328b0839312c169eb42746ec46fc1ab53ed2) |
| 63 | + Signed-off-by: Jonathan Maple < [email protected]> |
| 64 | + |
| 65 | +# Conflicts: |
| 66 | +# drivers/block/null_blk_main.c |
| 67 | +diff --cc drivers/block/null_blk_main.c |
| 68 | +index 737b4e4d4b8a,eb023d267369..000000000000 |
| 69 | +--- a/drivers/block/null_blk_main.c |
| 70 | ++++ b/drivers/block/null_blk_main.c |
| 71 | +@@@ -2029,33 -1946,45 +2044,37 @@@ static int null_add_dev(struct nullb_de |
| 72 | + |
| 73 | + nullb->q->queuedata = nullb; |
| 74 | + blk_queue_flag_set(QUEUE_FLAG_NONROT, nullb->q); |
| 75 | + + blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, nullb->q); |
| 76 | + |
| 77 | +++<<<<<<< HEAD:drivers/block/null_blk_main.c |
| 78 | + + mutex_lock(&lock); |
| 79 | + + rv = ida_simple_get(&nullb_indexes, 0, 0, GFP_KERNEL); |
| 80 | + + if (rv < 0) { |
| 81 | + + mutex_unlock(&lock); |
| 82 | + + goto out_cleanup_zone; |
| 83 | + + } |
| 84 | +++======= |
| 85 | ++ rv = ida_alloc(&nullb_indexes, GFP_KERNEL); |
| 86 | ++ if (rv < 0) |
| 87 | ++ goto out_cleanup_disk; |
| 88 | ++ |
| 89 | +++>>>>>>> a2db328b0839 (null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'):drivers/block/null_blk/main.c |
| 90 | + nullb->index = rv; |
| 91 | + dev->index = rv; |
| 92 | +- mutex_unlock(&lock); |
| 93 | + |
| 94 | + - if (config_item_name(&dev->group.cg_item)) { |
| 95 | + - /* Use configfs dir name as the device name */ |
| 96 | + - snprintf(nullb->disk_name, sizeof(nullb->disk_name), |
| 97 | + - "%s", config_item_name(&dev->group.cg_item)); |
| 98 | + - } else { |
| 99 | + - sprintf(nullb->disk_name, "nullb%d", nullb->index); |
| 100 | + - } |
| 101 | + + blk_queue_logical_block_size(nullb->q, dev->blocksize); |
| 102 | + + blk_queue_physical_block_size(nullb->q, dev->blocksize); |
| 103 | + |
| 104 | + - set_capacity(nullb->disk, |
| 105 | + - ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT); |
| 106 | + - nullb->disk->major = null_major; |
| 107 | + - nullb->disk->first_minor = nullb->index; |
| 108 | + - nullb->disk->minors = 1; |
| 109 | + - nullb->disk->fops = &null_ops; |
| 110 | + - nullb->disk->private_data = nullb; |
| 111 | + - strscpy_pad(nullb->disk->disk_name, nullb->disk_name, DISK_NAME_LEN); |
| 112 | + + null_config_discard(nullb); |
| 113 | + |
| 114 | + - if (nullb->dev->zoned) { |
| 115 | + - rv = null_register_zoned_dev(nullb); |
| 116 | + - if (rv) |
| 117 | + - goto out_ida_free; |
| 118 | + - } |
| 119 | + + sprintf(nullb->disk_name, "nullb%d", nullb->index); |
| 120 | + |
| 121 | + - rv = add_disk(nullb->disk); |
| 122 | + + rv = null_gendisk_register(nullb); |
| 123 | + if (rv) |
| 124 | + goto out_ida_free; |
| 125 | + |
| 126 | +- mutex_lock(&lock); |
| 127 | + list_add_tail(&nullb->list, &nullb_list); |
| 128 | +- mutex_unlock(&lock); |
| 129 | + |
| 130 | + - pr_info("disk %s created\n", nullb->disk_name); |
| 131 | + - |
| 132 | + return 0; |
| 133 | + |
| 134 | + out_ida_free: |
| 135 | +@@@ -2076,6 -2005,51 +2095,54 @@@ out |
| 136 | + return rv; |
| 137 | + } |
| 138 | + |
| 139 | +++<<<<<<< HEAD:drivers/block/null_blk_main.c |
| 140 | +++======= |
| 141 | ++ static struct nullb *null_find_dev_by_name(const char *name) |
| 142 | ++ { |
| 143 | ++ struct nullb *nullb = NULL, *nb; |
| 144 | ++ |
| 145 | ++ mutex_lock(&lock); |
| 146 | ++ list_for_each_entry(nb, &nullb_list, list) { |
| 147 | ++ if (strcmp(nb->disk_name, name) == 0) { |
| 148 | ++ nullb = nb; |
| 149 | ++ break; |
| 150 | ++ } |
| 151 | ++ } |
| 152 | ++ mutex_unlock(&lock); |
| 153 | ++ |
| 154 | ++ return nullb; |
| 155 | ++ } |
| 156 | ++ |
| 157 | ++ static int null_create_dev(void) |
| 158 | ++ { |
| 159 | ++ struct nullb_device *dev; |
| 160 | ++ int ret; |
| 161 | ++ |
| 162 | ++ dev = null_alloc_dev(); |
| 163 | ++ if (!dev) |
| 164 | ++ return -ENOMEM; |
| 165 | ++ |
| 166 | ++ mutex_lock(&lock); |
| 167 | ++ ret = null_add_dev(dev); |
| 168 | ++ mutex_unlock(&lock); |
| 169 | ++ if (ret) { |
| 170 | ++ null_free_dev(dev); |
| 171 | ++ return ret; |
| 172 | ++ } |
| 173 | ++ |
| 174 | ++ return 0; |
| 175 | ++ } |
| 176 | ++ |
| 177 | ++ static void null_destroy_dev(struct nullb *nullb) |
| 178 | ++ { |
| 179 | ++ struct nullb_device *dev = nullb->dev; |
| 180 | ++ |
| 181 | ++ null_del_dev(nullb); |
| 182 | ++ null_free_device_storage(dev, false); |
| 183 | ++ null_free_dev(dev); |
| 184 | ++ } |
| 185 | ++ |
| 186 | +++>>>>>>> a2db328b0839 (null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'):drivers/block/null_blk/main.c |
| 187 | + static int __init null_init(void) |
| 188 | + { |
| 189 | + int ret = 0; |
| 190 | +* Unmerged path drivers/block/null_blk_main.c |
0 commit comments