Re: [PATCH v13 01/39] selftests/dyndbg: Add kselftest script to verify dynamic-debug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/dyndbg: stale record check in audit_golden_records() never fails
- [Medium] selftests/dyndbg: broken LACK_DD_BUILTIN fallback and missing root check
--

commit d24db61e852431796c45358b620c1c921638c8b3
Author: Jim Cromie <jim.cromie@gmail.com>

selftests/dyndbg: Add kselftest script to verify dynamic-debug

This commit introduces a kselftest script and a syslog/control-file hash
validation helper library to test dynamic_debug grammar and query execution
against a database of golden MD5 fingerprints.

> diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
> new file mode 100755
> index 0000000000000..c1475c7555429
> --- /dev/null
> +++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
[ ... ]
> +[ -e /proc/dynamic_debug/control ] || {
> +    echo -e "${RED}: this test requires CONFIG_DYNAMIC_DEBUG=y ${NC}"
> +    exit $ksft_skip # nothing to test here, no good reason to fail.
> +}
> +
> +lsmod >/dev/null 2>&1 || {
> +    echo -e "${RED}: lsmod requires /proc/modules ${NC}"
> +    # exit $ksft_skip # maybe later we can do more
> +}
> +
> +# need info to avoid failures due to untestable configs
> +
> +[ -f "$KCONFIG_CONFIG" ] || KCONFIG_CONFIG=".config"
> +if [ -f "$KCONFIG_CONFIG" ]; then
> +    v_echo "# consulting KCONFIG_CONFIG: $KCONFIG_CONFIG"
> +    grep -q "CONFIG_DYNAMIC_DEBUG=y" $KCONFIG_CONFIG ; LACK_DD_BUILTIN=$?
> +    grep -q "CONFIG_TEST_DYNAMIC_DEBUG=m" $KCONFIG_CONFIG ; LACK_TMOD=$?
> +else
> +    # if no config, try runtime probes
> +    modprobe -n test_dynamic_debug 2>/dev/null ; LACK_TMOD=$?
> +    # assume builtin dyndbg if control exists (checked above)
> +    LACK_DD_BUILTIN=0
> +fi

[Severity: Medium]
When kselftest runs dyndbg_selftest.sh via runner.sh, it first changes the
working directory into tools/testing/selftests/dynamic_debug/ (or the
installed selftest directory), where .config is not present unless
KCONFIG_CONFIG is set to an absolute path.

In that case, the fallback branch unconditionally sets LACK_DD_BUILTIN=0.
However, on a kernel built with CONFIG_DYNAMIC_DEBUG_CORE=y and
CONFIG_DYNAMIC_DEBUG=n, /proc/dynamic_debug/control still exists, while
built-in files such as kernel/params.c have no dynamic debug descriptors.
Will FT_basic_queries() then fail to skip and report false DRIFTED failures
on empty diffs? Could the runtime fallback instead probe
/proc/dynamic_debug/control for ^kernel/params.c: to set LACK_DD_BUILTIN?

Also, /proc/dynamic_debug/control is created with mode 0644, so the
[ -e /proc/dynamic_debug/control ] check succeeds for unprivileged users,
causing the script to fail with $ksft_fail on the first write in ddcmd()
or /dev/kmsg instead of skipping. Should this check for write permission
(-w /proc/dynamic_debug/control) or root privileges before running the
tests?

[ ... ]
> +audit_golden_records
> +
> +# Output consolidated blocks of unregistered and drifted fingerprints
> +failed=0
> +
> +if [ -s "$UNREG_HASHES_FILE" ]; then
> +    if [ "$K" -ne 2 ]; then
> +        echo -e "${YELLOW}\n# --- Unregistered Baselines ---"
> +        cat "$UNREG_HASHES_FILE"
> +        echo -e "# ------------------------------${NC}"
> +    fi
> +    : > "$UNREG_HASHES_FILE"
> +    failed=1
> +fi
> +
> +if [ -s "$DRIFT_HASHES_FILE" ]; then
> +    if [ "$K" -ne 2 ]; then
> +        echo -e "${RED}\n# --- Drifted Baselines ---"
> +        cat "$DRIFT_HASHES_FILE"
> +        echo -e "# -------------------------${NC}"
> +    fi
> +    : > "$DRIFT_HASHES_FILE"
> +    failed=1
> +fi
> +
> +if [ $failed -eq 1 ]; then
> +    [ "$K" -eq 1 ] && echo "fake success" && exit $ksft_pass
> +    [ "$K" -eq 2 ] && exit $ksft_pass
> +    exit $ksft_fail
> +fi
> +
> +exit $ksft_pass
> +
> diff --git a/tools/testing/selftests/dynamic_debug/syslog_hash_validation.sh b/tools/testing/selftests/dynamic_debug/syslog_hash_validation.sh
> new file mode 100644
> index 0000000000000..8c6e91f5d9c27
> --- /dev/null
> +++ b/tools/testing/selftests/dynamic_debug/syslog_hash_validation.sh
[ ... ]
> +    # Strictly verify that the computed fingerprint matches any
> +    # expected hash for this label
> +    if [ -n "$expected_hash_field" ] && [ $matched -eq 1 ]; then
> +        local short_hash="${fingerprint:0:12}"
> +        [ "$V" -ge 1 ] && echo -e "${GREEN}✔ Verified '${label}' " \
> +            "(${short_hash}) [via: '${DDCMD_LOG}']${NC}"
> +
> +        if [ "$V" -ge 2 ]; then
> +            echo -e "${CYAN}--- Captured Invariant ${capture_desc} Output ($label) ---"
> +	    printf "#K= %-32s %-24s\n" "${fingerprint}" "${label}"
> +            echo "$raw_capture"
> +            echo -e "-----------------------------------${NC}"
> +        fi
> +        echo "$fingerprint" >> "$SEEN_HASHES_FILE"
> +    else
[ ... ]
> +function audit_golden_records {
> +    local seen_file="$SEEN_HASHES_FILE"
> +
> +    if [ ! -f "$seen_file" ]; then
> +        return
> +    fi
> +
> +    # Require GOLDEN_RECORDS to be defined in the caller script
> +    if ! declare -f GOLDEN_RECORDS >/dev/null; then
> +        return
> +    fi
> +
> +    [ "${V:-0}" -ge 1 ] && echo -e "${YELLOW}# --- GOLDEN_RECORDS Audit ---${NC}"
> +    local stale_found=0
> +    local total_records=$(GOLDEN_RECORDS | grep -c "^#K=")
> +
> +    # Read each active record line from GOLDEN_RECORDS
> +    while read -r line; do
> +        # Extract the hash/hashes (second word) from the #K= line
> +        local hash_field=$(echo "$line" | awk '{print $2}')
> +
> +        # Check if at least one of the comma-separated hashes was seen
> +        local hash_seen=0
> +        local h
> +        local OLD_IFS="$IFS"
> +        IFS=","
> +        for h in $hash_field; do
> +            if grep -q "$h" "$seen_file" 2>/dev/null; then
> +                hash_seen=1
> +                break
> +            fi
> +        done
> +        IFS="$OLD_IFS"
> +
> +        # Check if this hash field was seen during the run
> +        if [ $hash_seen -eq 0 ]; then
> +            if [ "${K:-0}" -ne 2 ]; then
> +                if [ $stale_found -eq 0 ]; then
> +                    # On first failure, print header if not already printed
> +                    [ "${V:-0}" -eq 0 ] && \
> +                        echo -e "${YELLOW}# --- GOLDEN_RECORDS Audit ---${NC}"
> +                    echo -e "${YELLOW}# The following GOLDEN_RECORDS entries " \
> +                        "were never hit and may be stale:${NC}"
> +                fi
> +                echo -e "${YELLOW}#K_STALE= $line${NC}"
> +            fi
> +            stale_found=1
> +        fi
> +    done < <(GOLDEN_RECORDS | grep "^#K=" | grep -v "<md5_hash>")
> +
> +    if [ $stale_found -eq 0 ] && [ "${V:-0}" -ge 1 ]; then
> +        echo -e "${GREEN}# All $total_records GOLDEN_RECORDS entries " \
> +            "were successfully hit!${NC}"
> +    fi
> +
> +    # Detect duplicate labels in the database
> +    local dupes=$(GOLDEN_RECORDS | grep "^#K=" | awk '{print $3}' | sort | uniq -d)
> +    if [ -n "$dupes" ]; then
> +        echo -e "\n${RED}# WARNING: Duplicate labels detected in GOLDEN_RECORDS():${NC}"
> +        echo "$dupes" | sed 's/^/#   /'
> +    fi
> +
> +    # Clean up
> +    rm -f "$seen_file"
> +}

[Severity: Medium]
The header comment in dyndbg_selftest.sh documents that K=0 fails with exit 1
on checksum drift or stale records, and K=1 exits 0 with "fake success".

In audit_golden_records(), stale_found and dupes are local variables and the
function ends with rm -f "$seen_file", returning 0 without propagating the
failure to the caller in dyndbg_selftest.sh (which only sets failed=1 when
UNREG_HASHES_FILE or DRIFT_HASHES_FILE is non-empty). Doesn't this mean stale
records or duplicate labels in GOLDEN_RECORDS() will never cause K=0 to exit
with $ksft_fail or K=1 to report "fake success"?

In addition, verify_fingerprint() writes only "$fingerprint" to
SEEN_HASHES_FILE without the test label, and audit_golden_records() checks
only whether the hash appears in "$seen_file". When multiple test cases in
GOLDEN_RECORDS() share the same MD5 hash (for example, FT_grammar_errs.21 and
FT_grammar_errs.41 both have 94cdf3b32afa4f12a00a704208aa2e53, and
FT_grammar_errs.25 and FT_grammar_errs.45 both have
644b81753949ac1285e38ac995eeba46), won't running just one of those tests mark
the hash as seen for all of them and hide unexecuted labels? Should
SEEN_HASHES_FILE record both the fingerprint and label (and be truncated with
: > "$seen_file" instead of removed with rm -f)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-dd-cmap-part2-clean-v13-0-9267923be612@gmail.com?part=1