From c22adc0b0cbe3768619eedc240bf58d88a1d6ed7 Mon Sep 17 00:00:00 2001 From: Xiubo Li Date: Thu, 14 Sep 2017 09:30:05 +0800 Subject: tcmu: fix crash when removing the tcmu device Before the nl REMOVE msg has been sent to the userspace, the ring's and other resources have been released, but the userspace maybe still using them. And then we can see the crash messages like: ring broken, not handling completions BUG: unable to handle kernel paging request at ffffffffffffffd0 IP: tcmu_handle_completions+0x134/0x2f0 [target_core_user] PGD 11bdc0c067 P4D 11bdc0c067 PUD 11bdc0e067 PMD 0 Oops: 0000 [#1] SMP cmd_id not found, ring is broken RIP: 0010:tcmu_handle_completions+0x134/0x2f0 [target_core_user] RSP: 0018:ffffb8a2d8983d88 EFLAGS: 00010296 RAX: 0000000000000000 RBX: ffffb8a2aaa4e000 RCX: 00000000ffffffff RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000220 R10: 0000000076c71401 R11: ffff8d2e76c713f0 R12: ffffb8a2aad56bc0 R13: 000000000000001c R14: ffff8d2e32c90000 R15: ffff8d2e76c713f0 FS: 00007f411ffff700(0000) GS:ffff8d1e7fdc0000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffffffffffffffd0 CR3: 0000001027070000 CR4: 00000000001406e0 Call Trace: ? tcmu_irqcontrol+0x2a/0x40 [target_core_user] ? uio_write+0x7b/0xc0 [uio] ? __vfs_write+0x37/0x150 ? __getnstimeofday64+0x3b/0xd0 ? vfs_write+0xb2/0x1b0 ? syscall_trace_enter+0x1d0/0x2b0 ? SyS_write+0x55/0xc0 ? do_syscall_64+0x67/0x150 ? entry_SYSCALL64_slow_path+0x25/0x25 Code: 41 5d 41 5e 41 5f 5d c3 83 f8 01 0f 85 cf 01 00 00 48 8b 7d d0 e8 dd 5c 1d f3 41 0f b7 74 24 04 48 8b 7d c8 31 d2 e8 5c c7 1b f3 <48> 8b 7d d0 49 89 c7 c6 07 00 0f 1f 40 00 4d 85 ff 0f 84 82 01 RIP: tcmu_handle_completions+0x134/0x2f0 [target_core_user] RSP: ffffb8a2d8983d88 CR2: ffffffffffffffd0 And the crash also could happen in tcmu_page_fault and other places. Signed-off-by: Zhang Zhuoyu Signed-off-by: Xiubo Li Reviewed-by: Mike Christie Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_user.c | 92 ++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 45 deletions(-) (limited to 'drivers/target/target_core_user.c') diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index 942d094269fb..8b9ec0fc91c2 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -1112,6 +1112,8 @@ static struct se_device *tcmu_alloc_device(struct se_hba *hba, const char *name) init_waitqueue_head(&udev->nl_cmd_wq); spin_lock_init(&udev->nl_cmd_lock); + INIT_RADIX_TREE(&udev->data_blocks, GFP_KERNEL); + return &udev->se_dev; } @@ -1280,10 +1282,54 @@ static void tcmu_dev_call_rcu(struct rcu_head *p) kfree(udev); } +static int tcmu_check_and_free_pending_cmd(struct tcmu_cmd *cmd) +{ + if (test_bit(TCMU_CMD_BIT_EXPIRED, &cmd->flags)) { + kmem_cache_free(tcmu_cmd_cache, cmd); + return 0; + } + return -EINVAL; +} + +static void tcmu_blocks_release(struct tcmu_dev *udev) +{ + int i; + struct page *page; + + /* Try to release all block pages */ + mutex_lock(&udev->cmdr_lock); + for (i = 0; i <= udev->dbi_max; i++) { + page = radix_tree_delete(&udev->data_blocks, i); + if (page) { + __free_page(page); + atomic_dec(&global_db_count); + } + } + mutex_unlock(&udev->cmdr_lock); +} + static void tcmu_dev_kref_release(struct kref *kref) { struct tcmu_dev *udev = container_of(kref, struct tcmu_dev, kref); struct se_device *dev = &udev->se_dev; + struct tcmu_cmd *cmd; + bool all_expired = true; + int i; + + vfree(udev->mb_addr); + udev->mb_addr = NULL; + + /* Upper layer should drain all requests before calling this */ + spin_lock_irq(&udev->commands_lock); + idr_for_each_entry(&udev->commands, cmd, i) { + if (tcmu_check_and_free_pending_cmd(cmd) != 0) + all_expired = false; + } + idr_destroy(&udev->commands); + spin_unlock_irq(&udev->commands_lock); + WARN_ON(!all_expired); + + tcmu_blocks_release(udev); call_rcu(&dev->rcu_head, tcmu_dev_call_rcu); } @@ -1476,8 +1522,6 @@ static int tcmu_configure_device(struct se_device *dev) WARN_ON(udev->data_size % PAGE_SIZE); WARN_ON(udev->data_size % DATA_BLOCK_SIZE); - INIT_RADIX_TREE(&udev->data_blocks, GFP_KERNEL); - info->version = __stringify(TCMU_MAILBOX_VERSION); info->mem[0].name = "tcm-user command & data buffer"; @@ -1527,6 +1571,7 @@ err_netlink: uio_unregister_device(&udev->uio_info); err_register: vfree(udev->mb_addr); + udev->mb_addr = NULL; err_vzalloc: kfree(info->name); info->name = NULL; @@ -1534,37 +1579,11 @@ err_vzalloc: return ret; } -static int tcmu_check_and_free_pending_cmd(struct tcmu_cmd *cmd) -{ - if (test_bit(TCMU_CMD_BIT_EXPIRED, &cmd->flags)) { - kmem_cache_free(tcmu_cmd_cache, cmd); - return 0; - } - return -EINVAL; -} - static bool tcmu_dev_configured(struct tcmu_dev *udev) { return udev->uio_info.uio_dev ? true : false; } -static void tcmu_blocks_release(struct tcmu_dev *udev) -{ - int i; - struct page *page; - - /* Try to release all block pages */ - mutex_lock(&udev->cmdr_lock); - for (i = 0; i <= udev->dbi_max; i++) { - page = radix_tree_delete(&udev->data_blocks, i); - if (page) { - __free_page(page); - atomic_dec(&global_db_count); - } - } - mutex_unlock(&udev->cmdr_lock); -} - static void tcmu_free_device(struct se_device *dev) { struct tcmu_dev *udev = TCMU_DEV(dev); @@ -1576,9 +1595,6 @@ static void tcmu_free_device(struct se_device *dev) static void tcmu_destroy_device(struct se_device *dev) { struct tcmu_dev *udev = TCMU_DEV(dev); - struct tcmu_cmd *cmd; - bool all_expired = true; - int i; del_timer_sync(&udev->timeout); @@ -1586,20 +1602,6 @@ static void tcmu_destroy_device(struct se_device *dev) list_del(&udev->node); mutex_unlock(&root_udev_mutex); - vfree(udev->mb_addr); - - /* Upper layer should drain all requests before calling this */ - spin_lock_irq(&udev->commands_lock); - idr_for_each_entry(&udev->commands, cmd, i) { - if (tcmu_check_and_free_pending_cmd(cmd) != 0) - all_expired = false; - } - idr_destroy(&udev->commands); - spin_unlock_irq(&udev->commands_lock); - WARN_ON(!all_expired); - - tcmu_blocks_release(udev); - tcmu_netlink_event(udev, TCMU_CMD_REMOVED_DEVICE, 0, NULL); uio_unregister_device(&udev->uio_info); -- cgit v1.2.3-59-g8ed1b From b5ab697c62724b3d31556d91c6f9b76d2e264e4b Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Fri, 15 Sep 2017 14:44:55 +0900 Subject: target/tcmu: Use macro to call container_of in tcmu_cmd_time_out_show This patch makes a tiny change that using TCMU_DEV in tcmu_cmd_time_out_show so it is consistent with other functions. Signed-off-by: Kenjiro Nakayama Reviewed-by: Mike Christie Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_user.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/target/target_core_user.c') diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index 8b9ec0fc91c2..2dd329773249 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -1736,8 +1736,7 @@ static ssize_t tcmu_cmd_time_out_show(struct config_item *item, char *page) { struct se_dev_attrib *da = container_of(to_config_group(item), struct se_dev_attrib, da_group); - struct tcmu_dev *udev = container_of(da->da_dev, - struct tcmu_dev, se_dev); + struct tcmu_dev *udev = TCMU_DEV(da->da_dev); return snprintf(page, PAGE_SIZE, "%lu\n", udev->cmd_time_out / MSEC_PER_SEC); } -- cgit v1.2.3-59-g8ed1b From b849b4567549d5a54ab34ffacfd48fca05e8b34e Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Wed, 13 Sep 2017 14:01:22 +0900 Subject: target: Add netlink command reply supported option for each device Currently netlink command reply support option (TCMU_ATTR_SUPP_KERN_CMD_REPLY) can be enabled only on module scope. Because of that, once an application enables the netlink command reply support, all applications using target_core_user.ko would be expected to support the netlink reply. To make matters worse, users will not be able to add a device via configfs manually. To fix these issues, this patch adds an option to make netlink command reply disabled on each device through configfs. Original TCMU_ATTR_SUPP_KERN_CMD_REPLY is still enabled on module scope to keep backward-compatibility and used by default, however once users set nl_reply_supported= via configfs for a particular device, the device disables the netlink command reply support. Signed-off-by: Kenjiro Nakayama Reviewed-by: Mike Christie Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_user.c | 59 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) (limited to 'drivers/target/target_core_user.c') diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index 2dd329773249..3189bf0bc7f1 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -150,6 +150,8 @@ struct tcmu_dev { wait_queue_head_t nl_cmd_wq; char dev_config[TCMU_CONFIG_LEN]; + + int nl_reply_supported; }; #define TCMU_DEV(_se_dev) container_of(_se_dev, struct tcmu_dev, se_dev) @@ -1352,6 +1354,10 @@ static void tcmu_init_genl_cmd_reply(struct tcmu_dev *udev, int cmd) if (!tcmu_kern_cmd_reply_supported) return; + + if (udev->nl_reply_supported <= 0) + return; + relock: spin_lock(&udev->nl_cmd_lock); @@ -1378,6 +1384,9 @@ static int tcmu_wait_genl_cmd_reply(struct tcmu_dev *udev) if (!tcmu_kern_cmd_reply_supported) return 0; + if (udev->nl_reply_supported <= 0) + return 0; + pr_debug("sleeping for nl reply\n"); wait_for_completion(&nl_cmd->complete); @@ -1550,6 +1559,12 @@ static int tcmu_configure_device(struct se_device *dev) dev->dev_attrib.emulate_write_cache = 0; dev->dev_attrib.hw_queue_depth = 128; + /* If user didn't explicitly disable netlink reply support, use + * module scope setting. + */ + if (udev->nl_reply_supported >= 0) + udev->nl_reply_supported = tcmu_kern_cmd_reply_supported; + /* * Get a ref incase userspace does a close on the uio device before * LIO has initiated tcmu_free_device. @@ -1612,7 +1627,7 @@ static void tcmu_destroy_device(struct se_device *dev) enum { Opt_dev_config, Opt_dev_size, Opt_hw_block_size, Opt_hw_max_sectors, - Opt_err, + Opt_nl_reply_supported, Opt_err, }; static match_table_t tokens = { @@ -1620,6 +1635,7 @@ static match_table_t tokens = { {Opt_dev_size, "dev_size=%u"}, {Opt_hw_block_size, "hw_block_size=%u"}, {Opt_hw_max_sectors, "hw_max_sectors=%u"}, + {Opt_nl_reply_supported, "nl_reply_supported=%d"}, {Opt_err, NULL} }; @@ -1694,6 +1710,18 @@ static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev, ret = tcmu_set_dev_attrib(&args[0], &(dev->dev_attrib.hw_max_sectors)); break; + case Opt_nl_reply_supported: + arg_p = match_strdup(&args[0]); + if (!arg_p) { + ret = -ENOMEM; + break; + } + ret = kstrtol(arg_p, 0, + (long int *) &udev->nl_reply_supported); + kfree(arg_p); + if (ret < 0) + pr_err("kstrtoul() failed for nl_reply_supported=\n"); + break; default: break; } @@ -1843,6 +1871,34 @@ static ssize_t tcmu_dev_size_store(struct config_item *item, const char *page, } CONFIGFS_ATTR(tcmu_, dev_size); +static ssize_t tcmu_nl_reply_supported_show(struct config_item *item, + char *page) +{ + struct se_dev_attrib *da = container_of(to_config_group(item), + struct se_dev_attrib, da_group); + struct tcmu_dev *udev = TCMU_DEV(da->da_dev); + + return snprintf(page, PAGE_SIZE, "%d\n", udev->nl_reply_supported); +} + +static ssize_t tcmu_nl_reply_supported_store(struct config_item *item, + const char *page, size_t count) +{ + struct se_dev_attrib *da = container_of(to_config_group(item), + struct se_dev_attrib, da_group); + struct tcmu_dev *udev = TCMU_DEV(da->da_dev); + s8 val; + int ret; + + ret = kstrtos8(page, 0, &val); + if (ret < 0) + return ret; + + udev->nl_reply_supported = val; + return count; +} +CONFIGFS_ATTR(tcmu_, nl_reply_supported); + static ssize_t tcmu_emulate_write_cache_show(struct config_item *item, char *page) { @@ -1885,6 +1941,7 @@ static struct configfs_attribute *tcmu_attrib_attrs[] = { &tcmu_attr_dev_config, &tcmu_attr_dev_size, &tcmu_attr_emulate_write_cache, + &tcmu_attr_nl_reply_supported, NULL, }; -- cgit v1.2.3-59-g8ed1b From 0d44374c1aaec7c81b470d3b5f955bc270711f9c Mon Sep 17 00:00:00 2001 From: Mike Christie Date: Wed, 25 Oct 2017 11:47:15 -0500 Subject: tcmu: fix double se_cmd completion If cmd_time_out != 0, then tcmu_queue_cmd_ring could end up sleeping waiting for ring space, timing out and then returning failure to lio, and tcmu_check_expired_cmd could also detect the timeout and call target_complete_cmd on the cmd. This patch just delays setting up the deadline value and adding the cmd to the udev->commands idr until we have allocated ring space and are about to send the cmd to userspace. Signed-off-by: Mike Christie Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_user.c | 54 ++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 23 deletions(-) (limited to 'drivers/target/target_core_user.c') diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index 3189bf0bc7f1..9ddf0909d33e 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -432,7 +432,6 @@ static struct tcmu_cmd *tcmu_alloc_cmd(struct se_cmd *se_cmd) struct se_device *se_dev = se_cmd->se_dev; struct tcmu_dev *udev = TCMU_DEV(se_dev); struct tcmu_cmd *tcmu_cmd; - int cmd_id; tcmu_cmd = kmem_cache_zalloc(tcmu_cmd_cache, GFP_KERNEL); if (!tcmu_cmd) @@ -440,9 +439,6 @@ static struct tcmu_cmd *tcmu_alloc_cmd(struct se_cmd *se_cmd) tcmu_cmd->se_cmd = se_cmd; tcmu_cmd->tcmu_dev = udev; - if (udev->cmd_time_out) - tcmu_cmd->deadline = jiffies + - msecs_to_jiffies(udev->cmd_time_out); tcmu_cmd_reset_dbi_cur(tcmu_cmd); tcmu_cmd->dbi_cnt = tcmu_cmd_get_block_cnt(tcmu_cmd); @@ -453,19 +449,6 @@ static struct tcmu_cmd *tcmu_alloc_cmd(struct se_cmd *se_cmd) return NULL; } - idr_preload(GFP_KERNEL); - spin_lock_irq(&udev->commands_lock); - cmd_id = idr_alloc(&udev->commands, tcmu_cmd, 0, - USHRT_MAX, GFP_NOWAIT); - spin_unlock_irq(&udev->commands_lock); - idr_preload_end(); - - if (cmd_id < 0) { - tcmu_free_cmd(tcmu_cmd); - return NULL; - } - tcmu_cmd->cmd_id = cmd_id; - return tcmu_cmd; } @@ -748,6 +731,30 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd, return command_size; } +static int tcmu_setup_cmd_timer(struct tcmu_cmd *tcmu_cmd) +{ + struct tcmu_dev *udev = tcmu_cmd->tcmu_dev; + unsigned long tmo = udev->cmd_time_out; + int cmd_id; + + if (tcmu_cmd->cmd_id) + return 0; + + cmd_id = idr_alloc(&udev->commands, tcmu_cmd, 1, USHRT_MAX, GFP_NOWAIT); + if (cmd_id < 0) { + pr_err("tcmu: Could not allocate cmd id.\n"); + return cmd_id; + } + tcmu_cmd->cmd_id = cmd_id; + + if (!tmo) + return 0; + + tcmu_cmd->deadline = round_jiffies_up(jiffies + msecs_to_jiffies(tmo)); + mod_timer(&udev->timeout, tcmu_cmd->deadline); + return 0; +} + static sense_reason_t tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd) { @@ -841,7 +848,6 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd) entry = (void *) mb + CMDR_OFF + cmd_head; memset(entry, 0, command_size); tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD); - entry->hdr.cmd_id = tcmu_cmd->cmd_id; /* Handle allocating space from the data area */ tcmu_cmd_reset_dbi_cur(tcmu_cmd); @@ -879,6 +885,13 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd) } entry->req.iov_bidi_cnt = iov_cnt; + ret = tcmu_setup_cmd_timer(tcmu_cmd); + if (ret) { + tcmu_cmd_free_data(tcmu_cmd, tcmu_cmd->dbi_cnt); + return TCM_OUT_OF_RESOURCES; + } + entry->hdr.cmd_id = tcmu_cmd->cmd_id; + /* * Recalaulate the command's base size and size according * to the actual needs @@ -912,8 +925,6 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd) static sense_reason_t tcmu_queue_cmd(struct se_cmd *se_cmd) { - struct se_device *se_dev = se_cmd->se_dev; - struct tcmu_dev *udev = TCMU_DEV(se_dev); struct tcmu_cmd *tcmu_cmd; sense_reason_t ret; @@ -924,9 +935,6 @@ tcmu_queue_cmd(struct se_cmd *se_cmd) ret = tcmu_queue_cmd_ring(tcmu_cmd); if (ret != TCM_NO_SENSE) { pr_err("TCMU: Could not queue command\n"); - spin_lock_irq(&udev->commands_lock); - idr_remove(&udev->commands, tcmu_cmd->cmd_id); - spin_unlock_irq(&udev->commands_lock); tcmu_free_cmd(tcmu_cmd); } -- cgit v1.2.3-59-g8ed1b From 16b932770417b1bc304d87c48aa0bb8a3c1164e1 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 8 Nov 2017 11:43:44 +0300 Subject: tcmu: Fix some memory corruption "udev->nl_reply_supported" is an int but on 64 bit arches we are writing 8 bytes of data to it so it corrupts four bytes beyond the end of the struct. Fixes: b849b4567549 ("target: Add netlink command reply supported option for each device") Signed-off-by: Dan Carpenter Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_user.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/target/target_core_user.c') diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index 9ddf0909d33e..d2b8d5ccb446 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -1724,11 +1724,10 @@ static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev, ret = -ENOMEM; break; } - ret = kstrtol(arg_p, 0, - (long int *) &udev->nl_reply_supported); + ret = kstrtoint(arg_p, 0, &udev->nl_reply_supported); kfree(arg_p); if (ret < 0) - pr_err("kstrtoul() failed for nl_reply_supported=\n"); + pr_err("kstrtoint() failed for nl_reply_supported=\n"); break; default: break; -- cgit v1.2.3-59-g8ed1b From 97488c73190bb785cba818bf31e7361a27aded41 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 8 Nov 2017 11:44:15 +0300 Subject: tcmu: Add a missing unlock on an error path We added a new error path here but we forgot to drop the lock first before returning. Fixes: 0d44374c1aae ("tcmu: fix double se_cmd completion") Signed-off-by: Dan Carpenter Signed-off-by: Nicholas Bellinger --- drivers/target/target_core_user.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/target/target_core_user.c') diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index d2b8d5ccb446..bf4fd40dde2b 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -888,6 +888,7 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd) ret = tcmu_setup_cmd_timer(tcmu_cmd); if (ret) { tcmu_cmd_free_data(tcmu_cmd, tcmu_cmd->dbi_cnt); + mutex_unlock(&udev->cmdr_lock); return TCM_OUT_OF_RESOURCES; } entry->hdr.cmd_id = tcmu_cmd->cmd_id; -- cgit v1.2.3-59-g8ed1b