Use a separate strtok_r saveptr for each listen-address, internal-portal
validation and portal-match operation. Preserve delimiters, limits and
buffer ownership. No tokenizer race was reproduced; this change makes
the parser state explicit and local to each operation.
Cppcheck reports strtokCalled at these tokenization sites:
Non reentrant function 'strtok' called. For threadsafe applications
it is recommended to use the reentrant replacement function 'strtok_r'.
Replace SCN flag, iSNS header, list and CRC macros with typed inline
helpers that evaluate their inputs once. Preserve SCN bit reversal and
htonl(), header ntohs(), list mutation order and CRC arithmetic. The
big-endian SCN helper returns its input.
Decode only the header fields each handler consumes; retain full
decoding for diagnostics and keep TLV bounds checks before reading SCN
attributes. This also removes unused decoded fields and their unused
attributes.
Native Checkpatch reports MACRO_ARG_REUSE for SCN flags, iSNS header
extraction, list initialization/removal and CRC byte updates:
Macro argument reuse 'hdr' - possible side-effects?
Macro argument reuse 'crc' - possible side-effects?
It also reports MACRO_ARG_UNUSED for the big-endian SCN no-op macro.
Checkpatch reports PREFER_DEFINED_ATTRIBUTE_MACRO:
__always_unused or __maybe_unused is preferred over
__attribute__((__unused__))
Replace parameter selection and range-check macros with typed helpers.
Capture SET_PARAM input once and retain assignment through each field's
actual type. All callers use it as a statement, so drop the unused
changed result. Remove GET_PARAM's unused info operand at every call.
Retain partial updates, bounds and digest checks. Manual review found
that the too-small warning printed the already-clamped value; print the
original signed s32 input before clamping. No ioctl layout changes.
Use min_t for the daemon's integer queue-depth limits and remove the
unnecessary cast on the fileio integer constant.
Native Checkpatch reports MACRO_ARG_REUSE for info, iparams, min, max
and params, and MACRO_ARG_UNUSED for GET_PARAM's info argument:
Macro argument reuse 'iparams' - possible side-effects?
Argument 'info' is not used in function-like macro
Checkpatch reports MINMAX, TYPECAST_INT_CONSTANT. One example:
min() should probably be min_t(int, target_keys[key_queued_cmnds].max,
iscsi_init_params.max_queued_cmds)
Checkpatch reports:
ASSIGN_IN_IF: do not use assignment in if condition
DEEP_INDENTATION: Too many leading tabs - consider code refactoring
USE_NEGATIVE_ERRNO: return of an errno should typically be negative
(ie: return -EAGAIN)
Separate status assignments from simple if conditions and preserve
short-circuit dispatch bounds. Leave the parameter-dispatch else-if
chains and their lookup precedence intact.
Flatten incoming-connection cleanup with early continues. Keep the CLOSE
case, kernel-owned destruction, session-count rollback and empty-session
release in their original order; no new ownership transfer is
introduced.
Return -EAGAIN for an empty nonblocking event queue. The drain loop
checks zero versus nonzero, while main-loop callers discard this result.
Retain EINTR retry and event draining behavior and document the return
convention.
Retain the 50 ms and 100 ms polling intervals and the existing
signal-interruption behavior while replacing the obsolete usleep
interface.
Cppcheck reports usleepCalled:
Obsolescent function 'usleep' called. It is recommended to use
'nanosleep' or 'setitimer' instead.
Checkpatch reports DEEP_INDENTATION in sg_cmp_elem():
Too many leading tabs - consider code refactoring
Extract the first-mismatch search into scst_miscompare_offset(). Retain
the aligned-word fast path and byte fallback, while keeping scatterlist
cursor updates and reverse unmapping in the caller. Manual inspection
also shows that comparison mappings can be read-only; use const pointers
and return a byte pointer from the mapping helper for offset arithmetic.
Adapt the private KUnit stub signature and add an aligned buffer whose
length is not word aligned to the existing comparison cases. The earlier
mapping-order fix and its regression coverage remain separate.
Use kmap_local_page()/kunmap_local() for scatterlist copy/comparison,
WRITE SAME replication and nullio clearing. These short sections only
copy, compare or clear memory and unmap in reverse order; they do not
need implicit pagefault or preemption disabling.
For kernels before 5.11, provide the same non-sleeping subset with atomic
mappings, also when a vendor has backported the newer API. No other SCST
consumer uses these local-mapping names before this conversion.
Rename the private scatterlist redirects and matching KUnit declarations
and stub registrations together. Keep the mapping-order fixture and its
assertions intact; the ordering defect was fixed separately.
Checkpatch reports DEPRECATED_API for the old kmap_atomic()/kunmap_atomic()
calls and for the WRITE SAME/nullio kmap() calls:
Deprecated use of 'kmap', prefer 'kmap_local_page' instead
Checkpatch reports:
MACRO_ARG_UNUSED: Argument 'log_flag' is not used in function-like macro
LOGGING_CONTINUATION: Avoid logging continuation uses where feasible
Pass PRINT_LOG_FLAG's severity to debug_print_with_prefix() instead of
forcing KERN_INFO. Correct the adjacent PRINT/PRINTN comment about which
component supplies the level.
Replace six iSCSI pr_cont() sites with complete 16-byte hex/ASCII
records assembled in a bounded local buffer. Preserve byte
classification, four-byte separators and padding for short rows.
Unrelated output can still appear between rows but cannot split their
individual byte writes.
Emit explicit KERN_DEBUG headings and rows under TRACE_D_DUMP_PDU. An
enabled dump must not depend on the heading's pr_debug() callsite
setting. Use %zu for the BHS sizeof. No filtering or analyzer exception
is changed.
Cppcheck reports at the affected calls:
invalidPrintfArgType_sint: %d requires 'int' but the argument type
is 'unsigned int'.
invalidPrintfArgType_sint: %zd requires 'ssize_t' but the argument
type is 'size_t {aka unsigned long}'.
invalidPrintfArgType_uint: %llx requires 'unsigned long long' but
the argument type is 'unsigned long'.
invalidPrintfArgType_uint: %u requires 'unsigned int' but the
argument type is 'signed int'.
invalidScanfArgType_int: %lld requires 'long long *' but the
argument type is 'unsigned long long *'.
Use unsigned formats for command states, event IDs, handles and sizes.
Cast the u64 disk ID for %llx on both 32-bit and 64-bit builds. Preserve
unsigned export-link naming through an explicit counter conversion. Use
%llu for the unsigned virtual-disk size destination; retain the byte/MiB
shifts and overflow check.
At five DLM lock-name calls, explicitly convert node IDs to int for the
existing %d formats. Switching them to %u would change high-bit node
IDs' lock identities and break matching with older peers. Preserve that
signed spelling and leave membership and node-ID storage unchanged.
DEBUG_TM_IGNORE_ALL workers share a freeze flag without synchronization.
Manual review of the volatile warning confirmed that volatile does not
protect these concurrent accesses. Store and snapshot the flag under the
existing per-device mutex, then unlock before the intentional infinite
sleep. The mutex is initialized before worker creation and destroyed
after joins.
Checkpatch reports VOLATILE:
Use of volatile is usually wrong: see
Documentation/process/volatile-considered-harmful.rst
Replace the volatile black-hole-mode member with explicit READ_ONCE and
WRITE_ONCE accesses. Keep scst_mutex serialization of stores and
session-cache updates; readers still take one independent mode snapshot.
The int layout stays unchanged, and KUnit fixture initialization remains
before publication.
Checkpatch reports VOLATILE:
Use of volatile is usually wrong: see
Documentation/process/volatile-considered-harmful.rst
Coverity CID 565114 reports FORWARD_NULL in dev_user_get_ext_cdb():
Passing null pointer "cmd" to "dev_user_ext_cdb", which dereferences it.
The lookup helper initializes its core-command output to NULL on failure,
but the ioctl tests the separately returned user-command pointer with
IS_ERR(). The normal success path already rejects absent core commands
and acquires both references. The reported use therefore depends on the
relationship between two separate helper results.
Return the validated core command or ERR_PTR directly, and publish the
paired user-command reference through the output parameter only after
successful validation and acquisition. The ioctl now checks the same
pointer it passes to dev_user_ext_cdb(). Capture the core pointer once
for validation, reference acquisition and return.
Keep existing state checks, errno values, reference ownership and the
unlock-before-ucmd_put rejection path. Update the private declaration and
the eight KUnit reference cases for the new contract. Successful cases
also use the returned command to obtain the external CDB tail; rejected
cases check that the user-command output is NULL and neither reference
was retained.
Checkpatch reports STRLCPY at the daemon copy sites:
Prefer strscpy over strlcpy
The private strlcpy() implementation calls strlen(src) even when the
caller only needs a short prefix. Manual inspection of that warning
shows an unbounded source scan, including for a zero-capacity
destination.
Replace it with iscsi_strscpy(), which examines at most size source
bytes, returns -E2BIG on truncation and terminates every nonempty
destination. Use a private name instead of defining a replacement libc
strlcpy symbol. All thirty callers discard the previous
full-source-length result; their bounded, NUL-terminated output remains
the same for valid string inputs.
Add isolated Ginkgo coverage of the production userspace helper through
the existing C-probe mechanism. Guard both buffers with inaccessible
pages and cover zero capacity, empty strings, fitting strings, exact
fits, one-byte destinations and nonterminated sources at the copy limit.
No kernel code is compiled into this userspace probe.
Manual review found this defect while investigating scatterlist
analysis; there is no saved Sparse, Smatch or Cppcheck diagnostic for
the ordering.
sg_copy_elem() and sg_cmp_elem() map the source page before the
destination page, but unmap the source first. HIGHMEM atomic mappings
require last-in, first-out unmapping. Both the normal comparison exit
and the early miscompare exit violate that contract.
Unmap the destination before the source on all three paths. Retain the
existing kmap_atomic()/kunmap_atomic() API so the ordering fix can be
reviewed independently of the mapping API conversion.
Add private, task-scoped KUnit redirects around the atomic mapping
operations. Extend scst-LIB inside scst.ko with a mapping-stack fixture
that checks copying, equal comparisons and early mismatches for full
pages and offset buffers. Cover mismatches in the first byte, an interior
word and the last byte. The fixture checks balanced mapping counts and
LIFO order without needing a HIGHMEM kernel or creating real mappings.
In-tree CI builds report compatibility errors after whitespace cleanup:
Linux 7.3:
conflicting types for 'wr'; have 'struct ib_rdma_wr'
'sgv_shrinker' is a pointer; did you mean to use '->'?
Linux 6.18.57, dev_handlers/scst_vdisk_main.c:330:
too many arguments to function 'blkdev_issue_flush'
Two assumptions in specialize-patch change branch selection when only
whitespace changes in otherwise equivalent preprocessor expressions.
The dispatch expression accepts only spaces before '#'. A tab-indented
opening directive is copied as ordinary text, while its closing directive
still changes the conditional nesting state. Accept all horizontal
whitespace as the expression evaluator already does, including when an
indented #elif becomes #if after discarding its original opening branch.
The evaluator also substitutes an empty string for absent RHEL_MAJOR,
RHEL_RELEASE_CODE and MOFED version macros. Thus 'RHEL_MAJOR - 0' becomes
'- 0', which its signed-number patterns cannot reduce correctly. This
leaves the two-argument blkdev_issue_flush() call in a modern upstream
patch. Substitute zero for these absent numeric macros, matching C
preprocessor semantics and the existing RHEL_MINOR handling. Keep their
defined() results false and preserve supplied vendor version values.
Add isolated regressions for spaces and tabs around '#', nested unknown
guards, promoted #elif branches and absent vendor versions. Cover all
three flush signatures at the upstream 5.8/5.12 and RHEL 8.4/8.6 boundaries,
plus EL7, RHEL 9, upstream 6.18.57 and 7.3, with both '-0' and '- 0'. Apply
each resulting patch in a temporary directory in removal and blank-
preserving modes, checking selected branches, surviving guards and hunk
lengths without compiling or loading SCST.
Install patch in the CI build stage used by these generator tests. Without
this dependency, all 98 specialization subcases abort before applying the
patches, with the following error in the minimal Fedora build image:
FileNotFoundError: [Errno 2] No such file or directory: 'patch'
Found by manual code inspection, not by an analyzer or a runtime test.
When scst_suspend_activity() times out with commands still active,
__scst_resume_activity() revives the command and management counters.
The old wrapper calls percpu_ref_reinit(), whose zero-count requirement
is enforced by this kernel assertion:
WARN_ON_ONCE(!percpu_ref_is_zero(ref));
Upstream 3.18-4.19 and the retained EL7/UEK4/UEK5 implementations preserve
the per-CPU allocation and use long counters, but reinit still rejects
this legitimate nonzero resurrection. Restore the initial reference
additively, retain outstanding references, and restore per-CPU operation
with the kernel's bias, counter clearing and release-store ordering.
Keep explicitly forced atomic mode unchanged.
SCST serializes kill and resurrection with scst_suspend_mutex and keeps
both counters alive even when their release callbacks run. Require the
same external serialization for mode changes and exit. Concurrent gets
and puts remain allowed.
Kill confirmation can wake the caller before confirm_switch is cleared.
Drain a pending switch callback with rcu_barrier_sched() before reusing
the counters. The kernel switch waitqueue is private, and older kernels
do not export percpu_ref_switch_to_percpu(). This conditional barrier
can also wait for unrelated RCU callbacks; it is skipped once the switch
has completed. Native resurrection on 4.20+ and EL8+ remains unchanged.
Add scst-LIB-percpu-ref to scst.ko with private, real kernel counters.
Keep the backport helper available to KUnit on modern kernels, using
their counter layout and unified RCU API. Cover immediate resurrection
with active refs, resurrection after draining, forced atomic mode, and
repeated suspend cancellations with commands completing while dead.
Also fix the fake Kbuild cleaner used by the Python tooling tests.
CI reported test_cleanup_without_packaged_tooling failing with
"find: cannot delete .../isert-scst.ko: No such file or directory".
iSCSI cleans kernel/ and kernel/isert-scst/ concurrently, so one fixture
find can remove an artifact after the other has enumerated it. Use
-ignore_readdir_race to tolerate only vanished files, matching Kbuild's
idempotent rm -f behavior. Keep other find errors and the injected
FAIL_CLEAN failure fatal; retain parallel cleanup and artifact checks.
Tests, builds and VM validation were not run, as requested.
The minimum upstream kernel is now 3.18, while the retained EL7 kernels
already provide the newer percpu_ref layout and API.
Remove the 32-bit percpu counters, the two-argument init adapter, the
pre-3.17 reinit and exit helpers, and the stand-alone atomic fallback.
Include the native percpu-refcount header for every supported kernel.
Also remove upstream-only PTR_RET, ktime_before, kvfree, __ATTR_RW and
32Gb FC speed fallbacks, plus the obsolete 3.16 strscpy version test.
These paths were already excluded for EL7. Retain old-version branches
that EL7 still uses, including shrinker, RCU-head and SCSI adapters.
This removes unreachable compatibility code; it does not claim an
analyzer-warning fix. The supported-kernel resurrection wrapper still
uses reinit and is corrected separately.
Upstream Linux 3.11-3.17 uses older percpu_ref implementations whose
memory lifetime and reinitialization rules do not support the revival
needed when SCST cancels a suspend with outstanding commands. Maintaining
those implementations requires a separate compatibility path.
Require upstream Linux 3.18 or later and remove the eight 3.10-3.17
entries from the nightly matrix and the 3.10 selector from regression CI.
Keep every distribution entry, including EL7 7.5-7.9 and UEK4/UEK5. EL7
has the newer percpu_ref implementation despite its 3.10 base version.
Enforce the minimum and vendor exception in the shared build header.
This is a support-policy change, not an analyzer-warning fix. It does not
fix the remaining reinit-versus-resurrect mismatch on supported kernels.
The basic-or-smoke PR filter selects 67 specs. Recent successful PR
runs spent more than 41 minutes executing their VM tests, in addition
to image preparation. Broad functional labels keep growing this set
whenever a new basic or smoke regression is added.
Select explicit pr-labelled specs instead: 11 local and 8 iSCSI cases.
Keep representative device, target, LUN and group management, repeated
configuration application, failed fileio creation, discovery and login,
MD5/SHA256 CHAP, portal validation, READ CAPACITY, read-only rejection,
and verified fileio/blockio I/O. Preserve each test's workload, assertions
and cleanup. Exclude manual, race and stress scenarios from the PR set.
The master and manual selections remain 103 and 106 specs respectively.
Limit the complete PR job to 30 minutes, its image build to 10 minutes
and VM execution to 18 minutes. Preserve the longer push/manual limits.
The intended end-to-end PR duration is 15-20 minutes; confirm it on CI
rather than treating dry-run counts as runtime evidence.
Validate selection in the Go runner and Python VM supervisor tests.
Framework checks, all 59 CI tests, actionlint and selection dry runs pass.
The saved Cppcheck report flags driver_major:
iscsi-scst/usr/misc.c:44: warning: sscanf() without field width limits
can crash with huge input data. [invalidscanf]
The preceding fgets currently bounds input to a 256-byte buffer, so
this report does not demonstrate an overflow in the present path.
Still, state the 255-character limit in the %s conversion to bind it
directly to devname's capacity, including the terminating NUL.
The saved Cppcheck report shows that config_isns_load cannot fail:
iscsi-scst/usr/config.c:1162: style: Condition 'err==0' is always true
[knownConditionTrueFalse]
Manual review of that path found an unchecked strdup. An allocation
failure discards the existing server name and silently accepts the
configuration without its iSNS server. The diagnostic above describes
the constant result, not the unchecked allocation itself.
Duplicate the replacement before freeing the old name and return
-ENOMEM on failure. Propagate that error through config_load before
initializing iSNS or applying the remaining configuration.
The saved Cppcheck report flags the zero-length final send:
iscsi-scst/usr/af_alg.c:76: error: Null pointer dereference
[nullPointer]
Linux accepts a NULL buffer for this zero-length AF_ALG operation; this
report does not establish a runtime dereference. Pass an empty string
instead so the buffer contract is explicit without a model or waiver.
The length remains zero and MSG_MORE remains clear, preserving hash
finalization behavior.
The saved Cppcheck report warns in isert_send_completion_handler:
iscsi-scst/kernel/isert-scst/iser_rdma.c:429: warning:
Either the condition 'iscsi_req_pdu' is redundant or there is overflow
in pointer subtraction. [nullPointerArithmeticRedundantCheck]
The parent request is optional, but container_of subtracts the member
offset before the NULL check. Resolve the parent container only after
confirming that the request exists and has a buffer. Preserve completion
handling for responses without a parent and for valid remote tags.
Manual review found alignment-dependent loads in scst_unpack_lun;
no saved analyzer diagnostic directly reports this defect. The helper
accepts a byte pointer without an alignment contract, then casts it to
16-, 32- or 64-bit pointers. In particular, a six-byte LUN reads a 32-bit
tail at offset two even when the start is aligned.
Use unaligned big-endian loads for each supported length while retaining
flat-address decoding and rejection of nonzero tail bytes. This preserves
the public interface and avoids faults on strict-alignment architectures.
Add KUnit coverage for all eight alignments and supported lengths,
nonzero bytes outside the supplied length, and every rejected tail byte.
The saved Cppcheck report warns about the formatter output:
scst/src/scst_main.c:637: warning: Uninitialized variable: buf
[uninitvar]
scst_dump_cdb does not write anything when cdb_len is zero, but
scst_trace_cmds prints its stack buffer as a string. Unknown opcode
groups can produce an empty CDB, so termination must not depend on
valid command contents.
Initialize nonempty output buffers and leave zero-capacity buffers
untouched. Add KUnit coverage for empty, ordinary, truncated, one-byte
and zero-size output. Include the new private test header in generated
kernel patches.
Manual review found a use-after-free in internal_portal validation; no
saved analyzer diagnostic directly reports this aliasing case. portal
points inside the allocation owned by portals, but the error branch
frees portals before passing portal to the logging function.
Keep the allocation alive until the invalid address has been logged.
Preserve the existing EINVAL response and cleanup path.
Add a Ginkgo management regression through scstadmin. Reject a list
starting with an invalid token, check that the daemon log contains the
intact token, and verify daemon liveness and the unchanged setting.
Manual review found division-by-zero and truncation bugs in REQUEST
SENSE during FORMAT UNIT. No saved analyzer diagnostic directly reports
these failures. do_div truncates the shifted 64-bit total to 32 bits:
small totals and totals such as 1ULL << 48 produce a zero divisor.
Completion can also wrap the 16-bit progress field back to zero.
Use bounded 64-bit scaling and saturate completion at 0xffff. Publish
initial counters with format_active and read/update them under
flags_lock, avoiding inconsistent or torn 64-bit counter snapshots.
Include the progress descriptor in the returned sense length.
Add twelve KUnit cases for both sense formats, unknown/small totals,
truncated divisors, large counters and completion. Exercise the actual
REQUEST SENSE handler and validate the returned descriptor length.
The saved Cppcheck report identifies the lost directory-read result:
scst/src/scst_dlm.c:784: style: Variable 'ret' is reassigned a value
before the old one has been used. [redundantAssignment]
scst_dlm_update_nodeids overwrites the result of iterate_dir or
vfs_readdir with -ENOMEM. An empty or partial listing after a read error
can therefore replace the existing membership and report success.
Check the initial allocation and return directory errors before changing
node IDs, participant count or the local node ID. Isolate directory
access behind a task-scoped KUnit stub while retaining both kernel APIs.
Cover EIO, EACCES and ENOMEM with an existing membership and verify its
array ownership and contents survive. Include the private test header
in generated kernel patches as well as out-of-tree builds.
Manual review found that isert_poll_cq forms &cq->wc[err] when
ib_poll_cq returns a negative errno. This pointer formation already
indexes outside the completion array, even if the following loop does
not dereference it. No saved analyzer diagnostic directly reports it.
Stop on nonpositive results before forming the end pointer and preserve
the result for the caller. Keep the loop exit next to the poll result.
Add the scst-ISER-cq KUnit suite to isert-scst.ko. Call the production
polling function with a private ib_device poll callback and task-local
stubs for the success and error completion handlers. Cover initial
-EIO and -EINVAL results, an empty queue, full and short batches, and
an error after completions. Check poll termination, completion order
and dispatch to the correct handler. Negative-result cases also exercise
kernel UBSAN bounds checking when CONFIG_UBSAN_BOUNDS is enabled.
Include iSER in the existing KUnit module inventory and build it after
iscsi-scst with the RDMA compatibility probes. Propagate the common
KUnit flags through ordinary iSER builds as well, so analyzer builds
include the suite. Extend tooling checks for module selection, dependency
order, cleanup, private headers and in-tree source export.
Manual review found missing bounds checks in print_scn_pdu and
qry_rsp_handle; no saved analyzer diagnostic directly reports these
bugs. Both handlers read a TLV header before proving it is present.
Clamping an oversized value can underflow the remaining length or
accept a truncated attribute. Query handling also reads a missing
status word and can discard the old ACL before noticing bad input.
Validate every TLV header, value length and alignment before processing
any attributes. Check and decode the response status in host byte order.
Reject malformed tails before modifying names or replacing the ACL.
Add an isolated Ginkgo probe of both production handlers with payloads
ending at a guard page. Cover short headers/status, oversized and
unaligned values, malformed tails, delimiters and valid ACL replacement.
Manual review found that chap_rand ignores read errors and short reads,
then returns a partially or wholly uninitialized integer. No saved
analyzer diagnostic directly reports this defect. A negative signed
value can also reduce the intended 25-to-49-byte challenge length.
Read a complete unsigned value, retry EINTR and return failures to the
caller. Reject challenge creation on entropy failure and free a partial
challenge before returning, without publishing CHAP_C.
Add an isolated Ginkgo suite using the production CHAP implementation
with deterministic entropy I/O. Cover length bounds, interrupted and
short reads, open/read failures, EOF and partial-challenge cleanup.
Prepare interface headers privately so probes need no preceding build.
Enable Cppcheck alongside the other analyzers on master pushes and
manual dispatch, using the existing container and KUnit-enabled runner.
Keep failures for source findings and incomplete analysis, and list the
enabled job in the tooling guide. Use only analyzer names for job titles;
KUnit source coverage remains mandatory.
The Ginkgo VM job reports this runner warning:
Node.js 20 is deprecated. The following actions target Node.js 20
but are being forced to run on Node.js 24: actions/checkout@v4,
actions/upload-artifact@v4.
KUnit uses the same action versions. Update checkout and upload-artifact
to v7 in both VM workflows so they explicitly select Node.js 24, matching
the analyzer workflow. Retain checkout and artifact collection settings.
Manual review found that the explicit scatterlist argument is checked
but not used to initialize traversal. The saved pre-fix Cppcheck log
also contains this style diagnostic for the helper:
scst/include/scst.h:5012: style:
Parameter 'sg' can be declared as pointer to const
[constParameterPointer]
That message describes the argument's limited use; it does not diagnose
the wrong-list traversal or NULL dereference fixed here.
scst_get_sg_buf_first() checks its sg argument and uses sg_cnt as the
iteration bound, but initializes the cursor from cmd->sg. Supplying a
different list therefore reads the wrong buffers with an unrelated
length limit. A valid explicit list with cmd->sg == NULL also reaches a
NULL scatterlist in __scst_get_buf().
Initialize the cursor from sg, keeping the list and its element count
paired. The public helper's signature and command-buffer helpers remain
unchanged. The in-tree scst_zero_write_rest() caller selects write_sg,
which can be out_sg for bidirectional commands, although its own call
site is currently disabled by #if 0. This fixes the helper contract; it
does not establish a currently reachable in-tree I/O corruption path.
Add LIB KUnit coverage for a two-entry explicit list when cmd->sg points
to another list or is NULL, including end-of-list and DMA-sync state.
Also cover NULL and zero-length selections. This source-level defect
was confirmed while reviewing the historical analyzer fixes; no new
analyzer model or suppression is required.
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.
The saved pre-fix Cppcheck log reports:
iscsi-scst/kernel/nthread.c:326: style:
Clarify calculation precedence for '+' and '?'. [clarifyCalculation]
close_conn calculates its shutdown deadline with this expression:
shut_start_waiting + conn->deleting ? CONN_DEL_SHUT_TIMEOUT :
CONN_REG_SHUT_TIMEOUT
Addition binds more tightly than the conditional operator. The expression
therefore tests shut_start_waiting + conn->deleting and selects an
absolute 10- or 125-second jiffies value, rather than adding an interval
to the start timestamp. On a host with sufficient uptime, the transport
can be closed immediately after shutdown starts.
Select the interval first, add it to shut_start_waiting, and retain the
wrap-safe time_after comparison. Move the check into a private helper so
its timing behavior can be covered without a live connection.
Add KUnit coverage for both deletion and normal shutdown, high uptime,
the ticks before/at/after expiry and jiffies wraparound. The tests retain
the original strict-after deadline convention.
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.
The saved pre-fix Cppcheck log reports:
scst/src/dev_handlers/scst_user_main.c:1951: style:
Same expression on both sides of '=='. [duplicateExpression]
The extended-CDB ioctl contains an always-true assertion:
EXTRACHECKS_BUG_ON(cmd->cdb_buf == cmd->cdb_buf);
Any otherwise valid command longer than the inline CDB buffer reaches
that assertion. With CONFIG_SCST_EXTRACHECKS enabled it triggers BUG
instead of copying the extension to userspace. The intended invariant
is that a long command uses external CDB storage.
Compare cmd->cdb with cmd->cdb_buf and share tail selection through a
private helper. Keep the existing inline-length handling, copy length
and ioctl error behavior.
Add a USER-cdb KUnit suite covering every inline length and external
lengths through 32 bytes. The external-buffer cases exercise the path
that previously asserted, without user-memory access or a live handler.
Include the user ABI header needed by the private command definitions.
When every valid fallback tag is occupied, find_first_zero_bit returns
tag_num. The old loop calls test_and_set_bit before checking that
sentinel. If the bit was clear, it even exits the loop without checking
the range and uses tag_num as an index into tgt_cmd_map.
For word-aligned pool sizes this touches a word beyond the bitmap. For
other sizes it can claim padding in the last word. Both cases can return
an out-of-range command-map entry and let memset overwrite memory beyond
the map. Manual review found this bounds bug while auditing the fallback
allocation flagged by Coccinelle null/kmerr.cocci. No separate analyzer
diagnostic reported the invalid bitmap or command-map access.
Check the sentinel before test_and_set_bit and return -ENOENT when no tag
is available. Preserve the retry when another allocator wins the bit.
Expose the helper to the QLA KUnit suite in qla2x00tgt.ko.
Add KUnit cases for exhausted pools at word and non-word boundaries, an
empty pool, unchanged guard words and reuse of the final valid tag. A
spare guard word makes the old invalid access observable without needing
an actual out-of-bounds access in the test fixture.
The saved Coccinelle null/kmerr.cocci report flags the unchecked fallback
allocation in sqa_init_scst_tgt before the resource-helper refactoring:
qla2x00t-32gbit/qla2x00-target/scst_qla2xxx.c:1354:1-22:
alloc with no test, possible model on line 1368
PTR_ERR_OR_ZERO(NULL) returns success, so allocation failure is treated
as an initialized tag pool. Later command allocation dereferences NULL.
Check the allocation result explicitly and return -ENOMEM instead.
Manual review also found that BITS_TO_LONGS(tag_num) is passed as a byte
count; the diagnostic above does not report this size error. For 2048
tags on a 64-bit kernel this allocates 32 bytes instead of 256,
allowing bitmap operations to access beyond the allocation. Use kcalloc
with sizeof(unsigned long) to allocate and zero the complete bitmap.
Keep the fallback allocation helper available to KUnit on modern kernels.
Cover one tag, word boundaries, the default 2048-tag pool, zeroed storage
and injected allocation failure. Check ksize before touching the last bit
so an undersized allocation is detected even without KASAN.
Manual review of the allocation path flagged by Coccinelle's
null/kmerr.cocci rule found two resource leaks. No separate analyzer
diagnostic is recorded for these leaks. Failed SCST target registration
frees only sqa_tgt, and normal target release also leaves its command
map and tag pool behind. Repeated registration failures or target
recreation retain those allocations indefinitely.
Move allocation and cleanup into SCST-owned resource helpers. Unwind the
command map if tag initialization fails, and destroy both resources if
registration fails or the target is released. Preserve the bitmap,
percpu_ida and sbitmap backends selected by the existing kernel guards.
Target stop phase 1 uses bounded session waits. Before dropping lport
callbacks and releasing command storage, wait for the SCST session list
to drain. SCST removes each session after its commands and unregistration
callback have completed, so late command releases can still return tags.
Keep the helpers in scst_qla2xxx.c and link the QLA KUnit suite into
qla2x00tgt.ko, following the existing SCST module layout. Expose private
helpers only in test builds and use task-scoped registration and
allocation stubs. Cover map allocation failure, tag initialization
failure, registration failure and normal resource destruction.
Build and record the matching qla2xxx_scst.ko dependency for the runner,
and clean both module builds together. Extend the build and cleanup
fixtures to check this dependency and failure propagation.
The dependency has no tests of its own. Requiring every supplied module
to have a KUnit manifest made the runner abort before loading any module:
Module has no KUnit label manifest; rebuild with kunit-build
Accept modules without suites only when they are dependencies of supplied
test owners. Keep rejecting missing or malformed test manifests and
unrelated modules. Exercise native metadata parsing, label selection,
dependency load order and reverse cleanup in isolated runner fixtures.
The fixtures need no real target, but loading their owner still
initializes the QLogic drivers. Run them in a disposable VM without an
HBA; hardware session teardown requires separate integration validation.
The analyzer image and runner already support Coccinelle, but its matrix
entry is commented out. Pushes to master and manual workflow runs
therefore skip semantic-patch analysis entirely.
Enable coccicheck as a parallel matrix job, using the existing runner to
invoke make coccicheck with KUnit sources included. Remaining source
findings fail the job, and parser/tool failures retain the separate
incomplete-analysis status. Existing diagnostics remain visible without
adding exclusions or changing analyzer models.
Update the CI guide to list the enabled target. Cppcheck remains disabled,
and workflow triggers remain push to master and manual dispatch.
Coccinelle null/badzero.cocci reports:
iscsi-scst/usr/config.c:868:15-16: WARNING comparing pointer to 0
iscsi-scst/usr/config.c:1035:16-17: WARNING comparing pointer to 0
Both expressions compare struct target pointers to the integer zero.
Use !target for the failed lookup in config_params_set and for the
discovery-account flag passed to __config_account_add in
config_parse_main.
Preserve lookup failure handling and discovery-account selection. This
is a pointer-test cleanup, with no change to the represented NULL value.
Coccinelle misc/semicolon.cocci reports:
iscsi-scst/usr/iscsi_scstd.c:958:2-3: Unneeded semicolon
iscsi-scst/usr/iscsid.c:367:4-5: Unneeded semicolon
scst/src/scst_event.c:772:2-3: Unneeded semicolon
usr/stpgd/stpgd_main.c:269:3-4: Unneeded semicolon
Remove the trailing semicolons after the iSCSI netlink-open check, login
parameter check, event-size read check and STPG child-cleanup loop. Each
semicolon is
an empty statement after a completed block, so removing it preserves
control flow and requires no analyzer exception.
Coccinelle misc/cond_no_effect.cocci reports:
usr/include/debug.c:121:1-3:
WARNING: possible condition with no effect (if == else)
Buffer tracing prints the same final line whether or not a full row has
already been printed. Remove that conditional and its unused j state.
Also remove the identical immediate/non-immediate branches in fileio
cache synchronization. Both branches already call exec_fsync, so retain
that call and the TODO for future background execution. This second
cleanup was found while reviewing the same pattern; it is not a separate
finding in the current report.
The trace output and synchronous cache-flush behavior are unchanged.
Coccinelle misc/array_size.cocci reports:
iscsi-scst/usr/iscsi_adm.c:595:33-34: WARNING: Use ARRAY_SIZE
Move the existing iSNS ARRAY_SIZE helper to misc.h and use it for the
user-command dispatch table in iscsi-scst-adm. Both users already include
that header, so they can share the definition.
Preserve the size_t comparison, including rejection of negative operation
values, and the unsupported-operation path. The table index is used
only after the bounds check succeeds.
Coccinelle reports "parse error" while parsing SHARED_OPCODE_DESCRIPTORS
and VDISK_OPCODE_DESCRIPTORS in scst_vdisk_main.c. For the shared list,
the raw log marks its final element:
BAD:!!!!! &scst_op_descr_verify16,
The trailing comma leaves an incomplete expression when the macro is
parsed on its own. Remove it from each private macro and add the comma
at every initializer use instead.
Token comparisons confirm that vdisk_opcode_descriptors,
vdisk_opcode_descriptors_type2 and vcdrom_opcode_descriptors expand to
the same C tokens. The public SCST_OPCODE_DESCRIPTORS convention is
unchanged. This fixes these parser failures without a macro model.
Use the existing kzalloc_obj and kzalloc_objs helpers for PR abort state,
copy-manager list IDs, WRITE SAME state, latency statistics and UNMAP
fixtures. Drop the redundant default GFP_KERNEL argument in the ordering
fixture.
For single objects, the helper derives the allocation size from the
object expression. For arrays, keep the element count and checked
multiplication provided by the shared compatibility helper. Preserve
zeroing and explicit allocation flags, including the GFP_ATOMIC path.
These are allocation cleanups carried over from the historical analyzer
series; they do not fix an observed report-mode diagnostic in the
current Coccinelle run. No allocator model or exclusion is introduced.
Coccinelle free/ifnullfree.cocci reports:
scst/src/scst_lib.c:15441:2-7:
WARNING: NULL check before some freeing functions is not needed.
Remove the redundant guard around vfree(buf) in scst_copy_file and the
same guard around vfree(mem_verify) in vdev_verify. Both pointers may be
NULL on cleanup paths, which vfree accepts throughout the supported
kernel range.
Leave the NULL guards in backport.h intact: they protect older teardown
APIs whose NULL handling differs, rather than these vfree calls.
Coccinelle api/err_cast.cocci reports:
scst/src/scst_lib.c:8710:8-15:
WARNING: ERR_CAST can be used with bw
blk_copy_kern_sg returns an error pointer on failure. Its caller,
blk_map_kern_sg, converts that value to an integer with PTR_ERR and
immediately back to a pointer with ERR_PTR.
Assign ERR_CAST(bw) to rq instead. It expresses propagation of the same
error pointer to the request-pointer return type without the redundant
pointer-to-integer round trip.
Coccinelle reports "parse error" at declarations using compl, including
this fragment of struct scst_lksb in scst/include/scst.h:
BAD:!!!!! struct completion compl;
The parser treats compl as the C++ complement operator even in these C
files. Rename the DLM and SRPT fields and the local target variable to
completion. Update their init, wait and signal sites, as well as the
SRPT field documentation.
This removes the conflicting identifier without adding a parser model.
Types, structure layout and completion lifetimes remain unchanged.
Other unsupported macros can still cause independent parser failures.
Gleb Chesnokov