From baca511baf45d24124827aa83fae8a42c8c2ceba Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Tue, 20 Oct 2026 18:12:15 +0300 Subject: [PATCH] scst_user: Release rejected extended-CDB lookup references The saved pre-fix Cppcheck log reports an unreachable NULL check: scst/src/dev_handlers/scst_user_main.c:1943: style: Condition '!cmd' is always false [knownConditionTrueFalse] The extended-CDB ioctl acquires ucmd_ref before checking whether the command has core state, is still executable, and has been published to userspace or submitted for background execution. A rejected state jumps to the unlock-only path, leaking the reference on every invalid request. The retained command can then prevent device cleanup from completing. Keep hash lookup, state validation and paired user/core reference acquisition in a private helper. On rejection, drop the acquired user reference after releasing cmd_list_lock: its final put can take the same lock while removing the command from the hash. Return a core command only on success, eliminating the unreachable NULL-command cleanup path. The reference leak was found by manual review of the same ioctl while investigating its always-true assertion and error paths. The Cppcheck message above identifies the unreachable check removed here; it does not diagnose the leaked reference. Add eight KUnit cases for sent, background and parsing commands, absent core state, unpublished and completed commands, missing handles and zero-reference hash entries. Verify both counters and the returned core pointer so rejection cannot silently retain either reference. --- .../kunit/user/suites/scst_user_cdb_test.c | 88 +++++++++++++++++++ scst/src/dev_handlers/scst_user_main.c | 84 ++++++++++-------- scst/src/dev_handlers/scst_user_test.h | 2 + 3 files changed, 138 insertions(+), 36 deletions(-) diff --git a/scst/src/dev_handlers/kunit/user/suites/scst_user_cdb_test.c b/scst/src/dev_handlers/kunit/user/suites/scst_user_cdb_test.c index 8c1e05e49..ddced41d8 100644 --- a/scst/src/dev_handlers/kunit/user/suites/scst_user_cdb_test.c +++ b/scst/src/dev_handlers/kunit/user/suites/scst_user_cdb_test.c @@ -34,8 +34,96 @@ static void user_cdb_tail(struct kunit *test) } } +struct user_cdb_ref_case { + const char *name; + unsigned int state; + bool sent; + bool background; + bool no_cmd; + bool missing; + bool dead; + int error; +}; + +static const struct user_cdb_ref_case user_cdb_ref_cases[] = { + { .name = "sent", .state = UCMD_STATE_EXECING, .sent = true }, + { .name = "background", .state = UCMD_STATE_EXECING, .background = true }, + { .name = "parsing", .state = UCMD_STATE_PARSING, .sent = true }, + { .name = "no_core_command", .state = UCMD_STATE_EXECING, .sent = true, + .no_cmd = true, .error = -EINVAL }, + { .name = "not_published", .state = UCMD_STATE_EXECING, .error = -EINVAL }, + { .name = "finished", .state = UCMD_STATE_EXECING + 1, .sent = true, + .error = -EINVAL }, + { .name = "missing_handle", .missing = true, .error = -ESRCH }, + { .name = "dying_command", .dead = true, .error = -ESRCH }, +}; + +static void user_cdb_ref_desc(const struct user_cdb_ref_case *p, char *desc) +{ + snprintf(desc, KUNIT_PARAM_DESC_SIZE, "%s", p->name); +} + +KUNIT_ARRAY_PARAM(user_cdb_ref, user_cdb_ref_cases, user_cdb_ref_desc); + +static void user_cdb_unexpected_release(struct kref *ref) +{ + KUNIT_FAIL(kunit_get_current_test(), "dropped the fixture's retained reference"); +} + +static void user_cdb_references(struct kunit *test) +{ + const struct user_cdb_ref_case *p = test->param_value; + struct scst_user_dev *dev; + struct scst_user_cmd *ucmd, *result; + struct scst_cmd *cmd, *selected; + unsigned int i; + + dev = kunit_kzalloc(test, sizeof(*dev), GFP_KERNEL); + ucmd = kunit_kzalloc(test, sizeof(*ucmd), GFP_KERNEL); + cmd = kunit_kzalloc(test, sizeof(*cmd), GFP_KERNEL); + KUNIT_ASSERT_NOT_NULL(test, dev); + KUNIT_ASSERT_NOT_NULL(test, ucmd); + KUNIT_ASSERT_NOT_NULL(test, cmd); + spin_lock_init(&dev->udev_cmd_threads.cmd_list_lock); + for (i = 0; i < ARRAY_SIZE(dev->ucmd_hash); i++) + INIT_LIST_HEAD(&dev->ucmd_hash[i]); + ucmd->dev = dev; + ucmd->cmd = p->no_cmd ? NULL : cmd; + ucmd->h = 1; + ucmd->state = p->state; + ucmd->sent_to_user = p->sent; + ucmd->background_exec = p->background; + /* The fixture retains one reference; a dying hash entry has zero. */ + if (!p->dead) + kref_init(&ucmd->ucmd_ref); + kref_init(&cmd->cmd_ref); + list_add_tail(&ucmd->hash_list_entry, &dev->ucmd_hash[ucmd->h]); + + selected = cmd; + result = dev_user_ext_cdb_get(dev, p->missing ? 2 : ucmd->h, &selected); + KUNIT_EXPECT_EQ(test, kref_read(&ucmd->ucmd_ref), p->dead ? 0 : (p->error ? 1 : 2)); + KUNIT_EXPECT_EQ(test, kref_read(&cmd->cmd_ref), p->error ? 1 : 2); + if (p->error) { + if (IS_ERR(result)) + KUNIT_EXPECT_EQ(test, PTR_ERR(result), (long)p->error); + else + KUNIT_FAIL(test, "invalid CDB lookup unexpectedly succeeded"); + KUNIT_EXPECT_PTR_EQ(test, selected, NULL); + } else { + KUNIT_ASSERT_FALSE(test, IS_ERR(result)); + KUNIT_EXPECT_PTR_EQ(test, result, ucmd); + KUNIT_EXPECT_PTR_EQ(test, selected, cmd); + kref_put(&cmd->cmd_ref, user_cdb_unexpected_release); + kref_put(&ucmd->ucmd_ref, user_cdb_unexpected_release); + KUNIT_EXPECT_EQ(test, kref_read(&cmd->cmd_ref), 1); + KUNIT_EXPECT_EQ(test, kref_read(&ucmd->ucmd_ref), 1); + } + list_del_init(&ucmd->hash_list_entry); +} + static struct kunit_case user_cdb_cases[] = { KUNIT_CASE(user_cdb_tail), + KUNIT_CASE_PARAM(user_cdb_references, user_cdb_ref_gen_params), {} }; diff --git a/scst/src/dev_handlers/scst_user_main.c b/scst/src/dev_handlers/scst_user_main.c index c329bb3fc..f7db3048e 100644 --- a/scst/src/dev_handlers/scst_user_main.c +++ b/scst/src/dev_handlers/scst_user_main.c @@ -1937,12 +1937,55 @@ const u8 *dev_user_ext_cdb(const struct scst_cmd *cmd) return cmd->cdb + SCST_MAX_CDB_SIZE; } +/* Return paired user/core command references; rejected requests retain neither. */ +SCST_KUNIT_VISIBLE +struct scst_user_cmd *dev_user_ext_cdb_get(struct scst_user_dev *dev, unsigned int cmd_h, + struct scst_cmd **cmd) +{ + struct scst_user_cmd *ucmd; + int res; + + *cmd = NULL; + spin_lock_irq(&dev->udev_cmd_threads.cmd_list_lock); + + ucmd = __ucmd_find_hash(dev, cmd_h); + if (unlikely(!ucmd)) { + TRACE_MGMT_DBG("cmd_h %d not found", cmd_h); + res = -ESRCH; + goto out_unlock; + } + + if (unlikely(ucmd_get_check(ucmd))) { + TRACE_MGMT_DBG("Found being destroyed cmd_h %d", cmd_h); + res = -ESRCH; + goto out_unlock; + } + + if (!ucmd->cmd || ucmd->state > UCMD_STATE_EXECING || + !(ucmd->sent_to_user || ucmd->background_exec)) { + TRACE_MGMT_DBG("Invalid ucmd state %d for cmd_h %d", ucmd->state, cmd_h); + spin_unlock_irq(&dev->udev_cmd_threads.cmd_list_lock); + /* The final put can remove the command from the hash under this lock. */ + ucmd_put(ucmd); + return ERR_PTR(-EINVAL); + } + + *cmd = ucmd->cmd; + scst_cmd_get(*cmd); + spin_unlock_irq(&dev->udev_cmd_threads.cmd_list_lock); + return ucmd; + +out_unlock: + spin_unlock_irq(&dev->udev_cmd_threads.cmd_list_lock); + return ERR_PTR(res); +} + static int dev_user_get_ext_cdb(struct file *file, void __user *arg) { int res = 0, rc; struct scst_user_dev *dev; struct scst_user_cmd *ucmd; - struct scst_cmd *cmd = NULL; + struct scst_cmd *cmd; struct scst_user_get_ext_cdb get; const u8 *ext_cdb; @@ -1964,37 +2007,12 @@ static int dev_user_get_ext_cdb(struct file *file, void __user *arg) TRACE_BUFFER("Get ext cdb", &get, sizeof(get)); - spin_lock_irq(&dev->udev_cmd_threads.cmd_list_lock); - - ucmd = __ucmd_find_hash(dev, get.cmd_h); - if (unlikely(!ucmd)) { - TRACE_MGMT_DBG("cmd_h %d not found", get.cmd_h); - res = -ESRCH; - goto out_unlock; + ucmd = dev_user_ext_cdb_get(dev, get.cmd_h, &cmd); + if (IS_ERR(ucmd)) { + res = PTR_ERR(ucmd); + goto out; } - if (unlikely(ucmd_get_check(ucmd))) { - TRACE_MGMT_DBG("Found being destroyed cmd_h %d", get.cmd_h); - res = -ESRCH; - goto out_unlock; - } - - if (ucmd->cmd && ucmd->state <= UCMD_STATE_EXECING && - (ucmd->sent_to_user || ucmd->background_exec)) { - cmd = ucmd->cmd; - scst_cmd_get(cmd); - } else { - TRACE_MGMT_DBG("Invalid ucmd state %d for cmd_h %d", - ucmd->state, get.cmd_h); - res = -EINVAL; - goto out_unlock; - } - - spin_unlock_irq(&dev->udev_cmd_threads.cmd_list_lock); - - if (!cmd) - goto out_put; - ext_cdb = dev_user_ext_cdb(cmd); if (!ext_cdb) goto out_cmd_put; @@ -2012,17 +2030,11 @@ static int dev_user_get_ext_cdb(struct file *file, void __user *arg) out_cmd_put: scst_cmd_put(cmd); - -out_put: ucmd_put(ucmd); out: TRACE_EXIT_RES(res); return res; - -out_unlock: - spin_unlock_irq(&dev->udev_cmd_threads.cmd_list_lock); - goto out; } static int dev_user_process_scst_commands(struct scst_user_dev *dev) diff --git a/scst/src/dev_handlers/scst_user_test.h b/scst/src/dev_handlers/scst_user_test.h index c909562df..eff55ae34 100644 --- a/scst/src/dev_handlers/scst_user_test.h +++ b/scst/src/dev_handlers/scst_user_test.h @@ -8,6 +8,8 @@ bool ucmd_get_check(struct scst_user_cmd *ucmd); const u8 *dev_user_ext_cdb(const struct scst_cmd *cmd); +struct scst_user_cmd *dev_user_ext_cdb_get(struct scst_user_dev *dev, unsigned int cmd_h, + struct scst_cmd **cmd); void dev_user_unjam_cmd(struct scst_user_cmd *ucmd, int busy, unsigned long *flags); void dev_user_flush_pool(struct sgv_pool *pool); int dev_user_process_cleanup(struct scst_user_dev *dev);