From 7dc98f5b297a967c741b9e13cc25bd413ddbe8bc Mon Sep 17 00:00:00 2001 From: Gleb Chesnokov Date: Fri, 16 Oct 2026 12:40:01 +0300 Subject: [PATCH] scst: Cache the session black-hole mode for task management Task-management initialization reads sess->acg before acquiring activity protection. A concurrent reassignment can clear this pointer and later drop the old ACG reference, so the read can dereference NULL or freed storage even while ordinary command processing is suspended. Keep a session-owned copy of the black-hole mode and read it with READ_ONCE() in the TM path. Initialize it before admitting session work, retain the old mode while replacing LUNs, and publish the new mode when installing the new ACG. Update existing sessions on sysfs writes as well. All writers hold scst_mutex and use WRITE_ONCE(); the TM command already owns a session reference, so no additional locking or ACG reference is needed for this scalar policy read. Read the previous sysfs value under the same mutex so concurrent stores cannot mistake a real change for a no-op and leave session copies stale. Extend the private TM tests with sysfs mode transitions, rejected writes, and old/new drop-policy checks inside and after real reassignment. Remove the dedicated Ginkgo opt-in and Skip path now that the fix is in place, so both teardown scenarios run whenever their labels are selected, including full CI runs. Link: https://github.com/SCST-project/scst/issues/367 --- doc/KUNIT.md | 10 ++-- scripts/generate-kernel-patch | 3 +- scst/include/scst.h | 6 +++ scst/src/kunit/tm/suites/scst_kunit_tm.c | 57 +++++++++++++++++--- scst/src/scst_lib.c | 1 + scst/src/scst_sysfs.c | 8 +-- scst/src/scst_sysfs_test.h | 14 +++++ scst/src/scst_targ.c | 4 +- tests/ginkgo/config.iscsi.example.json | 1 - tests/ginkgo/internal/testcfg/config.go | 9 ++-- tests/ginkgo/internal/testcfg/config_test.go | 8 +-- tests/ginkgo/iscsi/acg_reassign_test.go | 3 -- 12 files changed, 93 insertions(+), 31 deletions(-) create mode 100644 scst/src/scst_sysfs_test.h diff --git a/doc/KUNIT.md b/doc/KUNIT.md index f1b93ee00..0de40c2c8 100644 --- a/doc/KUNIT.md +++ b/doc/KUNIT.md @@ -156,13 +156,15 @@ releases runner-owned KUnit; incomplete execution can retain modules. ## Issue #367: task management and ACG reassignment `KUNIT_LABEL=TM` selects `scst-TM-acg`. It checks five `black_hole` modes -for session teardown commands, mode changes across ACG reassignment, and -unchanged-group handling. These checks should pass before a production fix. +for session teardown commands, all mode transitions through the sysfs store, +rejected and repeated writes, and unchanged-group handling. Reassignment +checks require the old mode during the NULL-ACG window and the new mode after +the transition; later writes to the old group must not affect the session. `tm_during_reassign_test` also invokes the TM reader from an allocation stub while the real reassignment is in progress, for all five modes. It checks command state, drop policy and event delivery during and after the transition. -This case is enabled without a SKIP: before the production fix, its reader -can crash the test VM. Use a disposable VM and retain its console log outside +The reassignment regressions run without a SKIP: with the original ACG reader, +they can crash the test VM. Use a disposable VM and retain its console log outside the guest. The fixture controls one interleaving; it does not prove the absence of other races. diff --git a/scripts/generate-kernel-patch b/scripts/generate-kernel-patch index 812df1879..120cb2bd2 100755 --- a/scripts/generate-kernel-patch +++ b/scripts/generate-kernel-patch @@ -319,7 +319,8 @@ scst/src/scst_targ_test.h" scst_06_lib="scst/src/scst_lib.c \ scst/src/scst_lib_test.h" scst_07_pres="scst/src/scst_pres.h scst/src/scst_pres.c" -scst_08_sysfs="scst/src/scst_sysfs.c" +scst_08_sysfs="scst/src/scst_sysfs.c \ +scst/src/scst_sysfs_test.h" scst_09_debug="scst/include/scst_debug.h scst/src/scst_debug.c" scst_10_sgv="scst/include/scst_sgv.h scst/src/scst_mem.h scst/src/scst_mem.c doc/scst_pg.sgml" scst_user="scst/include/scst_user.h scst/src/dev_handlers/scst_user_main.c \ diff --git a/scst/include/scst.h b/scst/include/scst.h index 2eaaa53de..e096708f2 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -1856,6 +1856,12 @@ struct scst_session { /* Access control for this session and list entry there */ struct scst_acg *acg; + /* + * Cached ACG black-hole mode for TM, which can run during reassignment. + * Writers hold scst_mutex; the TM reader uses READ_ONCE(). + */ + int black_hole_type; + /* Initiator port transport id */ uint8_t *transport_id; diff --git a/scst/src/kunit/tm/suites/scst_kunit_tm.c b/scst/src/kunit/tm/suites/scst_kunit_tm.c index 6dfbe394d..821f7d7bb 100644 --- a/scst/src/kunit/tm/suites/scst_kunit_tm.c +++ b/scst/src/kunit/tm/suites/scst_kunit_tm.c @@ -8,6 +8,7 @@ #endif #include "../../../scst_priv.h" #include "../../../scst_lib_test.h" +#include "../../../scst_sysfs_test.h" #include "../../../scst_targ_test.h" struct tm_fixture { @@ -61,8 +62,10 @@ static int tm_alloc_tgt_dev_stub(struct scst_session *sess, KUNIT_EXPECT_PTR_EQ(test, acg_dev, &f->acg_dev); f->allocations++; /* Exercise the reader before the real reassignment has completed. */ - if (f->check_tm_during_reassign) + if (f->check_tm_during_reassign) { + KUNIT_EXPECT_PTR_EQ(test, sess->acg, NULL); tm_check_teardown_commands(test, f->expected_tm_dropped); + } *out = NULL; return -EPERM; } @@ -168,12 +171,20 @@ static void tm_check_teardown_commands(struct kunit *test, bool dropped) KUNIT_EXPECT_EQ(test, f->events - events, (unsigned int)ARRAY_SIZE(functions)); } +static void tm_set_black_hole(struct kunit *test, struct scst_acg *acg, int mode) +{ + char buf[] = { '0' + mode, '\0' }; + + KUNIT_EXPECT_EQ(test, __scst_acg_black_hole_store(acg, buf, 1), (ssize_t)0); + KUNIT_EXPECT_EQ(test, acg->acg_black_hole_type, mode); +} + static void tm_black_hole_test(struct kunit *test) { const struct tm_mode_case *p = test->param_value; struct tm_fixture *f = test->priv; - f->old_acg.acg_black_hole_type = p->mode; + tm_set_black_hole(test, &f->old_acg, p->mode); tm_check_teardown_commands(test, p->dropped); } @@ -193,11 +204,15 @@ static void tm_reassign_mode_test(struct kunit *test) struct tm_fixture *f = test->priv; /* Cover both entering and leaving the modes which drop TM commands. */ - f->old_acg.acg_black_hole_type = p->dropped ? - SCST_ACG_BLACK_HOLE_NONE : SCST_ACG_BLACK_HOLE_ALL; + tm_set_black_hole(test, &f->old_acg, p->dropped ? + SCST_ACG_BLACK_HOLE_NONE : SCST_ACG_BLACK_HOLE_ALL); f->new_acg.acg_black_hole_type = p->mode; tm_check_teardown_commands(test, !p->dropped); + /* Keep the old policy until the new ACG is installed. */ + f->expected_tm_dropped = !p->dropped; + f->check_tm_during_reassign = true; tm_reassign(test); + f->check_tm_during_reassign = false; KUNIT_ASSERT_PTR_EQ(test, f->sess.acg, &f->new_acg); KUNIT_EXPECT_EQ(test, f->allocations, 1U); KUNIT_EXPECT_EQ(test, f->links, 1U); @@ -206,6 +221,11 @@ static void tm_reassign_mode_test(struct kunit *test) KUNIT_EXPECT_EQ(test, kref_read(&f->old_acg.acg_kref), 1U); KUNIT_EXPECT_EQ(test, kref_read(&f->new_acg.acg_kref), 2U); tm_check_teardown_commands(test, p->dropped); + /* Updating the former group must no longer affect this session. */ + tm_set_black_hole(test, &f->old_acg, p->mode); + tm_set_black_hole(test, &f->old_acg, p->dropped ? + SCST_ACG_BLACK_HOLE_NONE : SCST_ACG_BLACK_HOLE_ALL); + tm_check_teardown_commands(test, p->dropped); } static void tm_during_reassign_test(struct kunit *test) @@ -214,7 +234,7 @@ static void tm_during_reassign_test(struct kunit *test) struct tm_fixture *f = test->priv; /* The expected TM policy is unambiguous throughout this transition. */ - f->old_acg.acg_black_hole_type = p->mode; + tm_set_black_hole(test, &f->old_acg, p->mode); f->new_acg.acg_black_hole_type = p->mode; f->expected_tm_dropped = p->dropped; f->check_tm_during_reassign = true; @@ -231,17 +251,42 @@ static void tm_same_acg_test(struct kunit *test) struct tm_fixture *f = test->priv; f->tgt.default_acg = &f->old_acg; + tm_set_black_hole(test, &f->old_acg, SCST_ACG_BLACK_HOLE_ALL); tm_reassign(test); KUNIT_EXPECT_PTR_EQ(test, f->sess.acg, &f->old_acg); KUNIT_EXPECT_EQ(test, f->allocations, 0U); KUNIT_EXPECT_EQ(test, f->links, 0U); KUNIT_EXPECT_TRUE(test, list_is_singular(&f->old_acg.acg_sess_list)); KUNIT_EXPECT_TRUE(test, list_empty(&f->new_acg.acg_sess_list)); - tm_check_teardown_commands(test, false); + tm_check_teardown_commands(test, true); +} + +static void tm_sysfs_mode_test(struct kunit *test) +{ + const struct tm_mode_case *p = test->param_value; + struct tm_fixture *f = test->priv; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(tm_modes); i++) { + tm_set_black_hole(test, &f->old_acg, tm_modes[i].mode); + tm_check_teardown_commands(test, tm_modes[i].dropped); + tm_set_black_hole(test, &f->old_acg, p->mode); + tm_check_teardown_commands(test, p->dropped); + /* Repeating or rejecting a write must preserve the current mode. */ + tm_set_black_hole(test, &f->old_acg, p->mode); + KUNIT_EXPECT_EQ(test, __scst_acg_black_hole_store(&f->old_acg, "x", 1), + (ssize_t)-EINVAL); + KUNIT_EXPECT_EQ(test, __scst_acg_black_hole_store(&f->old_acg, "0", 0), + (ssize_t)0); + KUNIT_EXPECT_EQ(test, __scst_acg_black_hole_store(&f->old_acg, NULL, 1), + (ssize_t)0); + tm_check_teardown_commands(test, p->dropped); + } } static struct kunit_case tm_cases[] = { KUNIT_CASE_PARAM(tm_black_hole_test, tm_mode_gen_params), + KUNIT_CASE_PARAM(tm_sysfs_mode_test, tm_mode_gen_params), KUNIT_CASE_PARAM(tm_reassign_mode_test, tm_mode_gen_params), KUNIT_CASE(tm_same_acg_test), KUNIT_CASE_PARAM(tm_during_reassign_test, tm_mode_gen_params), diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 442b5e4d8..17c6b7ad7 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -3116,6 +3116,7 @@ next: } sess->acg = acg; + WRITE_ONCE(sess->black_hole_type, acg->acg_black_hole_type); TRACE_DBG("Moving sess %p from acg %s to acg %s", sess, old_acg->acg_name, acg->acg_name); diff --git a/scst/src/scst_sysfs.c b/scst/src/scst_sysfs.c index be3674f75..f83cc6a16 100644 --- a/scst/src/scst_sysfs.c +++ b/scst/src/scst_sysfs.c @@ -34,6 +34,7 @@ #include "scst_pres.h" #include "scst_mem.h" #include "scst_test.h" +#include "scst_sysfs_test.h" #undef DEFAULT_SYMBOL_NAMESPACE #define DEFAULT_SYMBOL_NAMESPACE SCST_NAMESPACE @@ -1758,20 +1759,20 @@ static ssize_t __scst_acg_black_hole_show(struct scst_acg *acg, char *buf) return sysfs_emit(buf, "%d\n", t); } -static ssize_t __scst_acg_black_hole_store(struct scst_acg *acg, const char *buf, size_t count) +SCST_KUNIT_VISIBLE ssize_t +__scst_acg_black_hole_store(struct scst_acg *acg, const char *buf, size_t count) { int res = 0; int prev, t; struct scst_session *sess; - prev = acg->acg_black_hole_type; - if (!buf || count == 0) { res = 0; goto out; } mutex_lock(&scst_mutex); + prev = acg->acg_black_hole_type; BUILD_BUG_ON((SCST_ACG_BLACK_HOLE_NONE != 0) || (SCST_ACG_BLACK_HOLE_CMD != 1) || @@ -1809,6 +1810,7 @@ static ssize_t __scst_acg_black_hole_store(struct scst_acg *acg, const char *buf list_for_each_entry(sess, &acg->acg_sess_list, acg_sess_list_entry) { int i; + WRITE_ONCE(sess->black_hole_type, t); rcu_read_lock(); for (i = 0; i < SESS_TGT_DEV_LIST_HASH_SIZE; i++) { struct list_head *head = &sess->sess_tgt_dev_list[i]; diff --git a/scst/src/scst_sysfs_test.h b/scst/src/scst_sysfs_test.h new file mode 100644 index 000000000..0c2ac7cb3 --- /dev/null +++ b/scst/src/scst_sysfs_test.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef SCST_SYSFS_TEST_H +#define SCST_SYSFS_TEST_H + +#include "scst_test.h" + +#ifdef SCST_KUNIT_ENABLED + +struct scst_acg; + +ssize_t __scst_acg_black_hole_store(struct scst_acg *acg, const char *buf, size_t count); + +#endif /* SCST_KUNIT_ENABLED */ +#endif /* SCST_SYSFS_TEST_H */ diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index 09da7337f..4d998597b 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -5648,7 +5648,8 @@ SCST_KUNIT_VISIBLE int scst_mgmt_cmd_init(struct scst_mgmt_cmd *mcmd) TRACE_ENTRY(); - t = mcmd->sess->acg->acg_black_hole_type; + /* sess->acg can be NULL, or its old ACG released, during reassignment. */ + t = READ_ONCE(mcmd->sess->black_hole_type); if (unlikely(t == SCST_ACG_BLACK_HOLE_ALL || t == SCST_ACG_BLACK_HOLE_DATA_MCMD)) { TRACE_MGMT_DBG("Dropping mcmd %p (fn %d, initiator %s)", mcmd, mcmd->fn, mcmd->sess->initiator_name); @@ -6996,6 +6997,7 @@ static int scst_init_session(struct scst_session *sess) mutex_lock(&scst_mutex); sess->acg = scst_find_acg(sess); + WRITE_ONCE(sess->black_hole_type, sess->acg->acg_black_hole_type); PRINT_INFO("Using security group \"%s\" for initiator \"%s\" (target %s)", sess->acg->acg_name, sess->initiator_name, sess->tgt->tgt_name); diff --git a/tests/ginkgo/config.iscsi.example.json b/tests/ginkgo/config.iscsi.example.json index ffb34647d..1503ba28c 100644 --- a/tests/ginkgo/config.iscsi.example.json +++ b/tests/ginkgo/config.iscsi.example.json @@ -8,7 +8,6 @@ "target_address": "192.0.2.10", "port": 3260, "login_timeout": "15s", - "acg_reassign_race": false, "initiator": { "host": "192.0.2.20", "user": "root", diff --git a/tests/ginkgo/internal/testcfg/config.go b/tests/ginkgo/internal/testcfg/config.go index e4cd34f92..a93871b32 100644 --- a/tests/ginkgo/internal/testcfg/config.go +++ b/tests/ginkgo/internal/testcfg/config.go @@ -47,11 +47,10 @@ type IO struct { // ISCSI selects a local TCP target and a separate SSH initiator. Credentials // for test CHAP accounts are generated by the suite, never supplied here. type ISCSI struct { - TargetAddress string `json:"target_address"` - Port int `json:"port"` - LoginTimeout string `json:"login_timeout"` - ACGReassignRace bool `json:"acg_reassign_race"` - Initiator Initiator `json:"initiator"` + TargetAddress string `json:"target_address"` + Port int `json:"port"` + LoginTimeout string `json:"login_timeout"` + Initiator Initiator `json:"initiator"` } type Initiator struct { diff --git a/tests/ginkgo/internal/testcfg/config_test.go b/tests/ginkgo/internal/testcfg/config_test.go index b20a126fb..18448b833 100644 --- a/tests/ginkgo/internal/testcfg/config_test.go +++ b/tests/ginkgo/internal/testcfg/config_test.go @@ -4,7 +4,6 @@ package testcfg import ( "os" "path/filepath" - "strings" "testing" ) @@ -89,14 +88,9 @@ func TestISCSIConfiguration(t *testing.T) { if err != nil { t.Fatal(err) } - if cfg.ISCSI.ACGReassignRace || cfg.ISCSI.Initiator.DisposableHost || cfg.ISCSI.Initiator.IdentityFile != filepath.Join(filepath.Dir(path), "id_test") { + if cfg.ISCSI.Initiator.DisposableHost || cfg.ISCSI.Initiator.IdentityFile != filepath.Join(filepath.Dir(path), "id_test") { t.Fatalf("unsafe defaults or bad path: %+v", cfg.ISCSI) } - optIn := strings.Replace(input, `"iscsi":{`, `"iscsi":{"acg_reassign_race":true,`, 1) - enabled, err := Load(configFile(t, optIn)) - if err != nil || !enabled.ISCSI.ACGReassignRace { - t.Fatalf("explicit race selection lost: %+v, %v", enabled.ISCSI, err) - } for name, change := range map[string]func(*ISCSI){ "wildcard target": func(i *ISCSI) { i.TargetAddress = "0.0.0.0" }, "localhost": func(i *ISCSI) { i.TargetAddress = "127.0.0.1" }, diff --git a/tests/ginkgo/iscsi/acg_reassign_test.go b/tests/ginkgo/iscsi/acg_reassign_test.go index 031c335a5..2d0d18d77 100644 --- a/tests/ginkgo/iscsi/acg_reassign_test.go +++ b/tests/ginkgo/iscsi/acg_reassign_test.go @@ -92,9 +92,6 @@ func (f *fixture) raceACG(ctx context.Context, kernel *testhost.KernelLog, from, var _ = DescribeTable("ACG reassignment during iSCSI teardown", Label("iscsi", "stress", "sessions", "issue-367"), func(ctx SpecContext, recoverTCP bool) { - if !cfg.ISCSI.ACGReassignRace { - Skip("set iscsi.acg_reassign_race=true on disposable VMs to attempt issue #367") - } path := filepath.Join(artifactDir, fmt.Sprintf("issue-367-%t-kernel.log", recoverTCP)) kernel := must(testhost.StartKernelLog(path)) AddReportEntry("kernel log", path)