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: Use kref for user command ownership

Browse Source 
Coccinelle api/atomic_as_refcounter.cocci reports:

  scst/src/dev_handlers/scst_user_main.c:175:5-24:
  WARNING: atomic_dec_and_test variation before object free at line 176.

ucmd_ref controls the lifetime of a user command. Replace its generic
atomic operations with kref and a last-reference release callback.
Update initialization, diagnostic reads and KUnit fixtures together.

A command can remain in the hash after its last put while the destructor
waits for cmd_list_lock. The old acquisition increments that zero count
and then undoes the increment. Use kref_get_unless_zero under the same
lock to reject a dying command without modifying its zero count.

Preserve the full ordering of successful acquisition that the old
atomic_inc_return supplied. Add KUnit cases for acquiring a live command
and rejecting the zero-count state before hash removal.

The kref API supports the old kernels in the build matrix without a new
refcount_t compatibility layer. No analyzer exclusion is needed.
This commit is contained in:
Gleb Chesnokov
2026-10-20 18:02:29 +03:00
parent 9b14a97cd4
commit 337a387bbd
4 changed files with 72 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
scst/src/dev_handlers/kunit/user/suites/scst_user_cleanup_test.c
+44 -3
View File
@@ -73,7 +73,9 @@ static int user_cleanup_init(struct kunit *test)
u->dev = dev;
u->state = UCMD_STATE_ATTACH_SESS;
u->cmpl = &f->reply;
atomic_set(&u->ucmd_ref, 2); /* Fixture reference plus outstanding reply. */
/* Fixture reference plus outstanding reply. */
kref_init(&u->ucmd_ref);
kref_get(&u->ucmd_ref);
INIT_LIST_HEAD(&u->hash_list_entry);
INIT_LIST_HEAD(&u->ready_cmd_list_entry);
}
@@ -144,7 +146,7 @@ static void user_cleanup_shared_test(struct kunit *test)
KUNIT_ASSERT_EQ(test, dev_user_process_cleanup(&f->devices[1]), 1);
KUNIT_EXPECT_TRUE(test, completion_done(&f->reply));
KUNIT_EXPECT_EQ(test, f->pending[1].result, -EFAULT);
KUNIT_EXPECT_EQ(test, atomic_read(&f->pending[1].ucmd_ref), 1);
KUNIT_EXPECT_EQ(test, kref_read(&f->pending[1].ucmd_ref), 1);
KUNIT_EXPECT_EQ(test, dev_user_process_cleanup(&f->devices[0]), 0);
KUNIT_EXPECT_EQ(test, dev_user_process_cleanup(&f->devices[1]), 0);
for (i = 0; i < 2; i++)
@@ -203,16 +205,55 @@ static void user_reply_test(struct kunit *test)
spin_unlock_irq(lock);
KUNIT_EXPECT_EQ(test, completion_done(&f->reply), !p->no_completion);
KUNIT_EXPECT_EQ(test, u->result, p->detach ? 0 : -EFAULT);
KUNIT_EXPECT_EQ(test, atomic_read(&u->ucmd_ref), 1);
KUNIT_EXPECT_EQ(test, kref_read(&u->ucmd_ref), 1);
KUNIT_EXPECT_TRUE(test, !!u->jammed);
KUNIT_EXPECT_TRUE(test, !!u->this_state_unjammed);
KUNIT_EXPECT_FALSE(test, !!u->sent_to_user);
}
static void user_defer_release(struct kref *ref)
{
/* Model the destructor waiting for cmd_list_lock before hash removal. */
}
static void user_get_live_command(struct kunit *test)
{
struct user_cleanup_fixture *f = test->priv;
struct scst_user_cmd *u = f->pending;
spinlock_t *lock = &f->devices[0].udev_cmd_threads.cmd_list_lock;
unsigned long flags;
bool skip;
spin_lock_irqsave(lock, flags);
skip = ucmd_get_check(u);
spin_unlock_irqrestore(lock, flags);
KUNIT_EXPECT_FALSE(test, skip);
KUNIT_EXPECT_EQ(test, kref_read(&u->ucmd_ref), 3U);
}
static void user_get_destroying_command(struct kunit *test)
{
struct user_cleanup_fixture *f = test->priv;
struct scst_user_cmd *u = f->pending;
spinlock_t *lock = &f->devices[0].udev_cmd_threads.cmd_list_lock;
unsigned long flags;
bool skip;
kref_put(&u->ucmd_ref, user_defer_release);
kref_put(&u->ucmd_ref, user_defer_release);
spin_lock_irqsave(lock, flags);
skip = ucmd_get_check(u);
spin_unlock_irqrestore(lock, flags);
KUNIT_EXPECT_TRUE(test, skip);
KUNIT_EXPECT_EQ(test, kref_read(&u->ucmd_ref), 0U);
}
static struct kunit_case user_cleanup_cases_kunit[] = {
KUNIT_CASE_PARAM(user_cleanup_test, user_cleanup_gen_params),
KUNIT_CASE(user_cleanup_shared_test),
KUNIT_CASE_PARAM(user_reply_test, user_reply_gen_params),
KUNIT_CASE(user_get_live_command),
KUNIT_CASE(user_get_destroying_command),
{}
};
scst/src/dev_handlers/scst_user_main.c
+26 -31
View File
@@ -133,47 +133,42 @@ static struct task_struct *cleanup_thread;
* Skip this command if result is not 0. Must be called under
* udev_cmd_threads.cmd_list_lock and IRQ off.
*/
static inline bool ucmd_get_check(struct scst_user_cmd *ucmd)
SCST_KUNIT_VISIBLE_INLINE
bool ucmd_get_check(struct scst_user_cmd *ucmd)
{
int r = atomic_inc_return(&ucmd->ucmd_ref);
int res;
if (unlikely(r == 1)) {
/* Keep successful gets fully ordered, as atomic_inc_return() was. */
smp_mb();
if (unlikely(!kref_get_unless_zero(&ucmd->ucmd_ref))) {
TRACE_DBG("ucmd %p is being destroyed", ucmd);
atomic_dec(&ucmd->ucmd_ref);
res = true;
/*
* Necessary code is serialized by cmd_list_lock in
* cmd_remove_hash()
*/
} else {
TRACE_DBG("ucmd %p, new ref_cnt %d", ucmd,
atomic_read(&ucmd->ucmd_ref));
res = false;
return true;
}
return res;
smp_mb__after_atomic_inc();
TRACE_DBG("ucmd %p, new ref_cnt %u", ucmd, kref_read(&ucmd->ucmd_ref));
return false;
}
static inline void ucmd_get(struct scst_user_cmd *ucmd)
{
TRACE_DBG("ucmd %p, ucmd_ref %d", ucmd, atomic_read(&ucmd->ucmd_ref));
atomic_inc(&ucmd->ucmd_ref);
/*
* For the same reason as in kref_get(). Let's be safe and
* always do it.
*/
TRACE_DBG("ucmd %p, ucmd_ref %u", ucmd, kref_read(&ucmd->ucmd_ref));
kref_get(&ucmd->ucmd_ref);
/* Preserve the ordering of SCST's command-reference acquisition. */
smp_mb__after_atomic_inc();
}
static void ucmd_release(struct kref *ref)
{
dev_user_free_ucmd(container_of(ref, struct scst_user_cmd, ucmd_ref));
}
/* Must not be called under cmd_list_lock!! */
static inline void ucmd_put(struct scst_user_cmd *ucmd)
{
TRACE_DBG("ucmd %p, ucmd_ref %d", ucmd, atomic_read(&ucmd->ucmd_ref));
TRACE_DBG("ucmd %p, ucmd_ref %u", ucmd, kref_read(&ucmd->ucmd_ref));
EXTRACHECKS_BUG_ON(atomic_read(&ucmd->ucmd_ref) == 0);
EXTRACHECKS_BUG_ON(kref_read(&ucmd->ucmd_ref) == 0);
if (atomic_dec_and_test(&ucmd->ucmd_ref))
dev_user_free_ucmd(ucmd);
kref_put(&ucmd->ucmd_ref, ucmd_release);
}
static inline int calc_num_pg(unsigned long buf, int len)
@@ -598,7 +593,7 @@ static struct scst_user_cmd *dev_user_alloc_ucmd(struct scst_user_dev *dev, gfp_
goto out;
}
ucmd->dev = dev;
atomic_set(&ucmd->ucmd_ref, 1);
kref_init(&ucmd->ucmd_ref);
cmd_insert_hash(ucmd);
@@ -3796,8 +3791,8 @@ static void dev_user_check_lost_ucmds(struct scst_user_dev *dev)
struct scst_user_cmd *ucmd2, *tmp;
list_for_each_entry_safe(ucmd2, tmp, head, hash_list_entry) {
PRINT_ERROR("Lost ucmd %p (state %x, ref %d)",
ucmd2, ucmd2->state, atomic_read(&ucmd2->ucmd_ref));
PRINT_ERROR("Lost ucmd %p (state %x, ref %u)",
ucmd2, ucmd2->state, kref_read(&ucmd2->ucmd_ref));
ucmd_put(ucmd2);
}
}
@@ -3880,9 +3875,9 @@ static ssize_t dev_user_sysfs_commands_show(struct kobject *kobj, struct kobj_at
list_for_each_entry(ucmd, head, hash_list_entry) {
pos = ret;
ret += sysfs_emit_at(buf, ret,
"ucmd %p (state %x, ref %d), sent_to_user %d, seen_by_user %d, aborted %d, jammed %d, scst_cmd %p\n",
"ucmd %p (state %x, ref %u), sent_to_user %d, seen_by_user %d, aborted %d, jammed %d, scst_cmd %p\n",
ucmd, ucmd->state,
atomic_read(&ucmd->ucmd_ref),
kref_read(&ucmd->ucmd_ref),
ucmd->sent_to_user, ucmd->seen_by_user,
ucmd->aborted, ucmd->jammed, ucmd->cmd);
if (ret >= SCST_SYSFS_BLOCK_SIZE - 1) {
scst/src/dev_handlers/scst_user_priv.h
+1 -1
View File
@@ -78,7 +78,7 @@ struct scst_user_cmd {
struct scst_user_cmd *buf_ucmd;
atomic_t ucmd_ref;
struct kref ucmd_ref;
int cur_data_page;
int num_data_pages;
scst/src/dev_handlers/scst_user_test.h
+1
View File
@@ -6,6 +6,7 @@
#ifdef SCST_KUNIT_ENABLED
bool ucmd_get_check(struct scst_user_cmd *ucmd);
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);