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 <[email protected]> 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/[email protected]?part=1
