From ab1ef1886693511f0b1d729cf71b13235d906fed Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Tue, 20 Oct 2026 18:13:57 +0300 Subject: [PATCH] scst_lib: Keep relative target port IDs within the valid range The saved pre-fix Cppcheck log reports: scst/src/scst_lib.c:4170: style: Redundant initialization for 'res'. The initialized value is overwritten before it is read. [redundantInitialization] The diagnostic points to the lost -EOVERFLOW result. Manual review of the same function additionally found the cursor wraparound failures below; those are not separate analyzer diagnostics. gen_relative_target_port_id has two boundary failures. Returning ID 65535 increments the static cursor to 65536 without wrapping it. The next uniqueness check truncates that value to u16, allowing reserved ID zero to be returned. If the range is occupied, the invalid saved cursor can also prevent the search from completing a full cycle. The initial -EOVERFLOW value is separately overwritten by successful mutex_lock_interruptible. Exhausting a valid range then returns success without assigning an ID, allowing target enable to continue incorrectly. Advance and wrap the cursor before every successful return. Search each valid candidate once and return -EOVERFLOW if none is available, leaving the caller's output and cursor unchanged on failure. Keep serialization under scst_mutex in the existing entry point. Add KUnit cases for the first candidate, occupied candidates, allocation of the last ID, wrap to ID 1 and complete exhaustion. Stub uniqueness checks per test task so the tests do not modify the global target list. --- scst/src/kunit/lib/suites/scst_kunit_lib.c | 61 ++++++++++++++++++++++ scst/src/scst_lib.c | 49 ++++++++++------- scst/src/scst_lib_test.h | 2 + 3 files changed, 94 insertions(+), 18 deletions(-) diff --git a/scst/src/kunit/lib/suites/scst_kunit_lib.c b/scst/src/kunit/lib/suites/scst_kunit_lib.c index fe33abcaf..2dbb67da1 100644 --- a/scst/src/kunit/lib/suites/scst_kunit_lib.c +++ b/scst/src/kunit/lib/suites/scst_kunit_lib.c @@ -7,6 +7,66 @@ #include "scst.h" #endif +#include "../../../scst_lib_test.h" + +struct scst_port_id_fixture { + unsigned int free_id; + unsigned int probes; + bool invalid_id; +}; + +static bool scst_port_id_available(u16 id, const struct scst_tgt *t) +{ + struct kunit *test = kunit_get_current_test(); + struct scst_port_id_fixture *fixture = test->priv; + + fixture->probes++; + fixture->invalid_id |= id < SCST_MIN_REL_TGT_ID; + return id == fixture->free_id; +} + +static void scst_port_id_boundaries(struct kunit *test) +{ + static const struct { + unsigned int start; + unsigned int free_id; + unsigned int next; + unsigned int probes; + int status; + } cases[] = { + { 1, 1, 2, 1, 0 }, + { 1, 3, 4, 3, 0 }, + { SCST_MAX_REL_TGT_ID, SCST_MAX_REL_TGT_ID, 1, 1, 0 }, + { SCST_MAX_REL_TGT_ID, 1, 2, 2, 0 }, + { 42, 0, 42, SCST_MAX_REL_TGT_ID, -EOVERFLOW }, + }; + struct scst_port_id_fixture fixture; + unsigned int i; + + test->priv = &fixture; + kunit_activate_static_stub(test, __scst_is_relative_target_port_id_unique, + scst_port_id_available); + for (i = 0; i < ARRAY_SIZE(cases); i++) { + unsigned int next = cases[i].start; + u16 id = 123; + u16 expected_id = cases[i].free_id; + + if (cases[i].status) + expected_id = id; + + memset(&fixture, 0, sizeof(fixture)); + fixture.free_id = cases[i].free_id; + KUNIT_EXPECT_EQ(test, scst_alloc_relative_target_port_id(&id, &next), + cases[i].status); + KUNIT_EXPECT_EQ(test, next, cases[i].next); + KUNIT_EXPECT_EQ(test, id, expected_id); + KUNIT_EXPECT_EQ(test, fixture.probes, cases[i].probes); + KUNIT_EXPECT_FALSE(test, fixture.invalid_id); + } + kunit_deactivate_static_stub(test, __scst_is_relative_target_port_id_unique); + test->priv = NULL; +} + static void scst_lun_flat_encoding(struct kunit *test) { /* Flat addressing: 01 in the top bits, followed by the 14-bit LUN. */ @@ -20,6 +80,7 @@ static void scst_lun_flat_encoding(struct kunit *test) } static struct kunit_case scst_lib_cases[] = { + KUNIT_CASE(scst_port_id_boundaries), KUNIT_CASE(scst_lun_flat_encoding), {} }; diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 08cdf7452..682ee69d9 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -4115,12 +4115,14 @@ restart: TRACE_EXIT(); } -static bool __scst_is_relative_target_port_id_unique(uint16_t id, - const struct scst_tgt *t) +SCST_KUNIT_VISIBLE +bool __scst_is_relative_target_port_id_unique(u16 id, const struct scst_tgt *t) { bool res = true; struct scst_tgt_template *tgtt; + SCST_KUNIT_REDIRECT(__scst_is_relative_target_port_id_unique, id, t); + TRACE_ENTRY(); list_for_each_entry(tgtt, &scst_template_list, @@ -4160,10 +4162,32 @@ bool scst_is_relative_target_port_id_unique(uint16_t id, return res; } +/* Caller serializes the cursor and target list with scst_mutex. */ +SCST_KUNIT_VISIBLE +int scst_alloc_relative_target_port_id(u16 *id, unsigned int *next) +{ + unsigned int candidate = *next; + + do { + bool available = __scst_is_relative_target_port_id_unique(candidate, NULL); + unsigned int selected = candidate; + + if (++candidate > SCST_MAX_REL_TGT_ID) + candidate = SCST_MIN_REL_TGT_ID; + if (available) { + *id = selected; + *next = candidate; + return 0; + } + } while (candidate != *next); + + return -EOVERFLOW; +} + int gen_relative_target_port_id(uint16_t *id) { - int res = -EOVERFLOW; - static unsigned long rti = SCST_MIN_REL_TGT_ID, rti_prev; + static unsigned int next = SCST_MIN_REL_TGT_ID; + int res; TRACE_ENTRY(); @@ -4171,21 +4195,10 @@ int gen_relative_target_port_id(uint16_t *id) if (res != 0) goto out; - rti_prev = rti; - do { - if (__scst_is_relative_target_port_id_unique(rti, NULL)) { - *id = (uint16_t)rti++; - res = 0; - goto out_unlock; - } - rti++; - if (rti > SCST_MAX_REL_TGT_ID) - rti = SCST_MIN_REL_TGT_ID; - } while (rti != rti_prev); + res = scst_alloc_relative_target_port_id(id, &next); + if (res) + PRINT_ERROR("%s", "Unable to create unique relative target port id"); - PRINT_ERROR("%s", "Unable to create unique relative target port id"); - -out_unlock: mutex_unlock(&scst_mutex); out: diff --git a/scst/src/scst_lib_test.h b/scst/src/scst_lib_test.h index 8769db781..dd82a382c 100644 --- a/scst/src/scst_lib_test.h +++ b/scst/src/scst_lib_test.h @@ -8,6 +8,8 @@ struct scst_write_same_priv; +bool __scst_is_relative_target_port_id_unique(u16 id, const struct scst_tgt *t); +int scst_alloc_relative_target_port_id(u16 *id, unsigned int *next); int scst_ws_build_write_cdb(const struct scst_write_same_priv *wsp, u8 *write_cdb, s64 lba, int blocks); void scst_del_free_acg_dev(struct scst_acg_dev *acg_dev, bool del_sysfs);