Move response headers, residuals, DMA state, FC/SRP target views and the
QLA bitmap cursor into their consuming branches or loops. Remove the
single-use QLA host alias and the redundant session-completion block;
keep its stack completion alive through the existing wait.
Preserve allocation and release order, transport ownership and callback
signatures. QLogic edits remain within the SCST target subtree.
Capture the SRPT management status immediately before response
conversion, after request-limit accounting. Preserve its single
evaluation while shortening the nested conversion expression.
Keep this expression refactoring separate from the remaining LONG_LINE
formatting changes.
Cppcheck reports variableScope:
The scope of the variable 'out_resid' can be reduced.
Move reset results, DLM indices, ALUA lookup state, per-iteration buffer
and sense temporaries, iSER completion bounds and KUnit fixture counters
into the branches or loops that consume them. Each is initialized before
use in its new scope.
Keep counters carrying state across iterations outside their loops.
Preserve lock pairs, error handling, debug guards and fixture
assertions.
Cppcheck reports variableScope in scst_copy_to_dlm(),
scst_target_reset(), scst_lun_reset() and scst_check_alua_invariant():
The scope of the variable 'i' can be reduced.
The scope of the variable 'rc' can be reduced.
The scope of the variable 'expected_state' can be reduced.
Move per-address socket variables, parameter-format cursors, netmask
fragments, iSNS read results and Base64 words into the loops or branches
that use them. Preserve operation order and values carried between
iterations. Compute the full initiator name immediately before the kernel
access check instead of nesting the helper call inside that check. Keep
the name buffer, access-check inputs and call order unchanged.
The initiator-name extraction is a readability change found during
source review of the nested call, separate from the variableScope report.
Cppcheck reports variableScope:
The scope of the variable 'sock' can be reduced.
Name anonymous function and negotiation-callback parameters and match
declaration names to their definitions. This includes the misleading
length name of the CDB LBA argument. Types and calling conventions
remain unchanged. Use pool_type for the two private sysfs parameter names
so their declarations fit; structure fields and diagnostics keep their
existing names.
Drop redundant extern on function prototypes, retain it on objects, and
align continued arguments with the resulting opening parentheses.
Cppcheck reports funcArgNamesDifferent, funcArgNamesDifferentUnnamed.
One example:
Function 'sqa_enable_tgt' argument 1 names different: declaration
'tgt' definition 'scst_tgt'.
Checkpatch reports FUNCTION_ARGUMENTS:
function definition argument 'const struct iscsi_key *' should also
have an identifier name
Checkpatch reports AVOID_EXTERNS:
extern prototypes should be avoided in .h files
Checkpatch reports PARENTHESIS_ALIGNMENT:
Alignment should match open parenthesis
Use distinct names for parser arguments, local descriptors, trace flags
and iSNS send/receive transaction IDs. Rename the process-wide SRPT RDMA
listener so connection-specific IDs do not shadow it.
Remove initial values replaced unconditionally before use and the
impossible fgets NULL branch. Retain configuration-dependent
initializers, listener lifetime and every connection ID passed to RDMA
operations.
Cppcheck reports shadowArgument, redundantInitialization. One example:
Local variable 'pp' shadows outer argument
Cppcheck reports shadowVariable, unreadVariable. One example:
Argument 'trace_flag' shadows outer variable
Use u8 pointers for byte-addressed buffers and cast protocol buffers
before adding byte offsets. This removes GNU void-pointer arithmetic
without changing offsets, ownership or public interfaces.
Cppcheck reports arithOperationsOnVoidPointer:
'to' is of type 'void *'. When using void pointers in calculations,
the behaviour is undefined.
Parenthesize expression operands in digest rounds, CDB groups and
statistics bounds while preserving declaration/name fragments. Group the
ARRAY_SIZE operand before indexing its first element.
Replace the reservation lock assertion macro with an inline helper so
its device argument is evaluated once. Retain the CONFIG_LOCKDEP guard
and the no-op behavior without lockdep.
Checkpatch reports MACRO_ARG_PRECEDENCE, COMPLEX_MACRO. One example:
Macro argument 'dir' may be better as '(dir)' to avoid precedence
issues
Checkpatch reports ARRAY_SIZE:
Prefer ARRAY_SIZE(x)
Checkpatch reports MACRO_ARG_REUSE:
Macro argument reuse 'dev' - possible side-effects?
Manual source review found no callers of scst_acg_sess_is_empty(),
scst_devt_cleanup(), sgv_pool_sg() or scst_store_sense(). Remove those
private definitions and the matching sense-helper declaration. The last
sense-helper caller was removed by ae72c4193 after an infinite loop.
Also remove the unused local isblank fallback. Preserve
scst_zero_write_rest() and its disabled caller for separate debug-code
review. These removals are source-review findings, not reported runtime
failures.
For TTT allocation, remove the unused connection argument from the
helper and its sole caller instead of const-qualifying an input it never
reads.
Cppcheck reports constParameterPointer:
Parameter 'conn' can be declared as pointer to const
Compute the version-dependent buffer validity before WARN() instead of
splitting its argument list with preprocessor branches. Preserve the old
SLUB exception, NULL short-circuiting and offset bounds. Use __func__
for the unchanged diagnostic function names.
Checkpatch reports EMBEDDED_FUNCTION_NAME:
Prefer using '"%s...", __func__' to using 'sysfs_emit_at', this
function's name, in a string
Manual source review found that isert_task_abort() and
isert_data_in_sent() only return zero, and every caller discards that
result. Remove the helpers, declarations and calls, including the
completion branch used solely to reach the empty helper. Keep PDU
completion and DMA unmapping in their existing order.
Make netlink registration and the optional mode-page path check return
void and remove callers' unreachable error checks. Neither helper can
fail its caller. Preserve success ACKs, the disabled mode-page fallback
and all reachable error paths.
Return explicit success after the checked portal-address ioctl. Source
review also found that scst_cm_ext_copy_exec() never changes its result
variable; return SCST_EXEC_COMPLETED directly while preserving command
completion callbacks and error paths.
Cppcheck reports knownConditionTrueFalse:
Condition 'err' is always false.
Cppcheck reports identicalConditionAfterEarlyExit:
Identical condition and return expression 'ret', return value is
always 0
Capture cache slots, legacy timer pointers and memtostr
source/destination pointers once. Publish newly allocated caches before
tracing, and retain object-size checks on the original memtostr
expressions. Preserve timer version predicates, callback conversions and
flag handling.
Parenthesize the seconds-conversion expression and the from_timer owner
before dereferencing it in typeof. Keep conversion order and use %zu for
cache sizeof diagnostics. Current callers use ordinary slots, timer
addresses or fixed byte arrays; no side-effecting call-site failure was
reproduced.
Native Checkpatch reports MACRO_ARG_REUSE:
Macro argument reuse 'p' - possible side-effects?
Macro argument reuse '_timer' - possible side-effects?
Macro argument reuse 'dest' - possible side-effects?
Macro argument reuse 'src' - possible side-effects?
Checkpatch reports COMPLEX_MACRO:
Macros with complex values should be enclosed in parentheses
Checkpatch reports MACRO_ARG_PRECEDENCE:
Macro argument 'var' may be better as '(var)' to avoid precedence
issues
Copy iSCSI text fragments as bytes: their lengths are bounded by the
remaining strlen, while framing emits separators and terminators
separately. Keep virtual-device name rejection and use the existing
strscpy compatibility helper for accepted bounded names.
Reuse checked redirect and directory-entry lengths, including the NUL,
and copy the fixed CHAP prefix by its literal size. Replace command-line
strncpy/termination pairs with snprintf while retaining existing
truncation and zero-filled padding. Copy terminated fixed-width event
fields and the constant core issuer by their exact extents.
Checkpatch reports STRNCPY:
Prefer strscpy, strscpy_pad, or __nonstring over strncpy
Checkpatch reports STRCPY:
Prefer strscpy over strcpy
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.
Gleb Chesnokov