mirrors/scst
1
0
Fork 0
mirror of https://github.com/SCST-project/scst.git synced 2026-10-22 06:02:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

scst_user: Release rejected extended-CDB lookup references

Browse Source 
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.
This commit is contained in:
Gleb Chesnokov
2026-10-21 09:23:02 +03:00
parent 907cc680b1
commit baca511baf
3 changed files with 138 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files
scst/src/dev_handlers/kunit/user/suites/scst_user_cdb_test.c
+88
View File
@@ -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),
{}
};
scst/src/dev_handlers/scst_user_main.c
+48 -36
View File
@@ -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)
scst/src/dev_handlers/scst_user_test.h
+2
View File
@@ -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);