Re: [PATCH net-next v3 14/14] selftests: dpll: add reference leak tests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 4

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The snapshot taken right after the probe can record the probe
  device's own dead ref_tracker entries.
- [Low] The leak check treats any non-empty dpll_device@*/dpll_pin@*
  entry missing from the snapshot as a leak of this run.
- [Low] The two new final leak checks handle a leftover device
  differently.
- [Low] dpll_ref_tracker_probe() returns 1 whenever the deployed probe
  device produces no non-empty new entry.
- [Low] dpll_debugfs_available() mounts debugfs when /sys/kernel/debug is
  not a mountpoint and sets DPLL_MOUNTED_DEBUGFS=1.

--

selftests: dpll: add reference leak tests

This adds helpers to dpll_lib.sh that snapshot the ref_tracker debugfs
entries before a test creates dpllsim devices. A test then fails when an
entry that is not in the snapshot still holds references afterwards. A
throwaway probe device checks that the tracker works; the probe runs first
in dpll_stress.sh, which also gains a final leak check, and in the new
dpll_reftrack.sh.

> diff --git a/tools/testing/selftests/drivers/dpll/dpll_lib.sh b/tools/testing/selftests/drivers/dpll/dpll_lib.sh
> index 7a0bb144af150..87686bb53d355 100644
> --- a/tools/testing/selftests/drivers/dpll/dpll_lib.sh
> +++ b/tools/testing/selftests/drivers/dpll/dpll_lib.sh

[ ... ]

> @@ -641,3 +658,215 @@ dpllsim_setup_standard_device()

[ ... ]

> +dpll_debugfs_available()
> +{
> +	if ! mountpoint -q "$DEBUGFS_DIR"; then
> +		mount -t debugfs none "$DEBUGFS_DIR" &> /dev/null &&
> +			DPLL_MOUNTED_DEBUGFS=1
> +	fi
> +	mountpoint -q "$DEBUGFS_DIR"
> +}

[Severity: Low]
The only sign that this script owns the mount is that debugfs was not
mounted when it checked. If no dpllsim device is left behind,
dpll_lib_cleanup() unmounts it at exit:

	[ "$DPLL_MOUNTED_DEBUGFS" -eq 1 ] && mounted+=("$DEBUGFS_DIR")
	...
	for dir in "${mounted[@]}"; do
		__dpll_lib_umount "$dir"
	done

During a long dpll_stress.sh run, another process (another selftest, a
tracing tool, a user) can find debugfs already mounted and start using it
without mounting it itself.

Would the umount at exit pull debugfs out from under that process? A plain
umount succeeds unless a file or cwd is held open at that moment, and the
other process would not remount it.

The configfs handling already works this way. This patch applies the same
pattern to debugfs, which is shared far more widely.

[ ... ]

> +__dpll_ref_tracker_new_entries()
> +{
> +	local file=$1
> +	local entry name content
> +
> +	for entry in "$DPLL_REF_TRACKER_DIR"/dpll_device@* \
> +		     "$DPLL_REF_TRACKER_DIR"/dpll_pin@*; do
> +		[ -f "$entry" ] || continue
> +		name=$(basename "$entry")
> +		grep -q "^$name " "$file" && continue
> +		content=$(cat "$entry" 2>/dev/null)
> +		[ -n "$content" ] || continue
> +		echo "Unreleased references in $name:"
> +		echo "$content"
> +	done
> +}

[Severity: Low]
The only attribution here is snapshot membership plus non-empty content.
What happens when another DPLL driver (ice, zl3073x, ptp_ocp, ...)
registers a new dpll_device or dpll_pin after the snapshot? This could come
from a deferred or async probe, a driver bind, a module reload, or a driver
re-allocating its pins during the long dpll_stress.sh run.

A live object of that driver always holds tracked references, so its entry
is non-empty and not in the snapshot.

Would test_basic_reftrack(), test_final_leak_check() and test_ref_leaks()
then wait BUSYWAIT_TIMEOUT and fail, blaming dpllsim for references it does
not own?

dpll_lib_lock() is an flock on /run/dpll-selftest/lock, so it only excludes
other dpll selftest scripts. The library comment only covers objects of
other drivers that already exist when the snapshot is taken.

[ ... ]

> +dpll_ref_tracker_probe()
> +{

[ ... ]

> +	if dpllsim_add_pin "$name" ext0 ext 10000000 input &&
> +	   dpllsim_deploy "$name"; then
> +		ret=1
> +		[ -n "$(__dpll_ref_tracker_new_entries "$file")" ] && ret=0
> +	fi

[Severity: Low]
Can this tell a kernel without CONFIG_DPLL_REFCNT_TRACKER apart from one
where the tracker is enabled but does not record anything?

dpll_device_alloc() and dpll_pin_alloc() call ref_tracker_dir_init()
unconditionally:

	ref_tracker_dir_init(&dpll->refcnt_tracker, 128, "dpll_device");

So the dpll_device@ and dpll_pin@ files exist whenever CONFIG_REF_TRACKER
and CONFIG_DEBUG_FS are set. Only dpll_device_tracker_alloc/free and
dpll_pin_tracker_alloc/free are under #ifdef CONFIG_DPLL_REFCNT_TRACKER, and
an empty dir->list prints nothing.

Suppose a regression makes the core stop calling ref_tracker_alloc(), or
pass a NULL tracker so that only no_tracker is bumped. The probe would then
return 1, and both scripts log:

	log_test_skip "DPLL ref_tracker probe" \
		      "CONFIG_DPLL_REFCNT_TRACKER not enabled"

This happens even though the selftest config sets
CONFIG_DPLL_REFCNT_TRACKER=y. Would such a regression show up as a SKIP
with a misleading reason instead of a FAIL?

> +
> +	if dpllsim_delete_device "$name"; then
> +		DPLLSIM_PROBE_DEVICE=""
> +	else
> +		log_info "cannot delete device $name"
> +		return 4
> +	fi
> +	if [ "$ret" -eq 0 ] &&
> +	   ! busywait "$BUSYWAIT_TIMEOUT" __dpll_ref_tracker_settled "$file" \
> +		      > /dev/null; then
> +		ret=3
> +	fi
> +	return "$ret"
> +}

[Severity: Medium]
Can the snapshot taken right after the probe record the probe device's own
dead ref_tracker entries?

The commit message says:

  The teardown of a device releases its references before the write to
  live returns, so the entries are expected to be gone at the first look

In lib/ref_tracker.c only the references go away synchronously.
ref_tracker_dir_exit() calls ref_tracker_debugfs_mark(), which only marks
the dir dead and schedules the reaper:

	__xa_set_mark(&debugfs_dentries, (unsigned long)dir, REF_TRACKER_DIR_DEAD);
	...
	schedule_work(&debugfs_reap_worker);

The dentry is removed later, in debugfs_reap_work(). Until then the file
still exists, and reading it returns -ENODATA from
ref_tracker_debugfs_show().

That allows this sequence:

dpllsim_delete_device(probe)
  dpll_device_put() -> ref_tracker_dir_exit(), kfree(dpll)
busywait __dpll_ref_tracker_settled
  cat of the dead entry fails, content is empty, entry is skipped
test_ref_tracker_probe()
  dpll_ref_tracker_snapshot "$REF_SNAPSHOT"
    [ -f "$entry" ] is still true, so the probe's names are recorded

The next test (test_basic_reftrack() or test_rapid_create_delete()) then
allocates a dpll_device right away. It can land on the probe's freed slab
slot, and the %p hash then gives the same entry name.

If the reaper has run by then, the new object's entry is in the snapshot,
and __dpll_ref_tracker_new_entries() skips it:

	grep -q "^$name " "$file" && continue

If the reaper still has not run, ref_tracker_dir_debugfs() returns early:

	/* No-op if already created */
	dentry = xa_load(&debugfs_dentries, (unsigned long)dir);
	if (dentry && !xa_is_err(dentry))
		return;

The reaper later removes the old dentry, so the new object never gets a
debugfs file at all.

In either case, would a leak in the first device created after the probe go
unreported?

Would it help to skip unreadable entries in dpll_ref_tracker_snapshot(), or
to wait after deleting the probe until its entries are gone from the
ref_tracker dir before taking the snapshot again?

[ ... ]

> diff --git a/tools/testing/selftests/drivers/dpll/dpll_stress.sh b/tools/testing/selftests/drivers/dpll/dpll_stress.sh
> index 47b8f4bf55b26..198a50ed67873 100755
> --- a/tools/testing/selftests/drivers/dpll/dpll_stress.sh
> +++ b/tools/testing/selftests/drivers/dpll/dpll_stress.sh

[ ... ]

> @@ -463,16 +524,36 @@ test_deploy_cycle()
>  	log_test "DPLL deploy/undeploy cycling"
>  }
>  
> +test_ref_leaks()
> +{
> +	RET=0
> +
> +	# The probe test found the tracker inactive or could not run.
> +	if [ "$REF_TRACKER" -eq 0 ]; then
> +		log_test_skip "DPLL stress ref_tracker check" \
> +			      "DPLL reference tracker not available"
> +		return
> +	fi
> +
> +	dpll_ref_tracker_check_leaks "$REF_SNAPSHOT"
> +	check_err $? "leaked DPLL references after stress"

[Severity: Low]
Should this skip when CREATED_DEVICES is not empty, the way
test_final_leak_check() in dpll_reftrack.sh does?

	# A device an earlier test could not remove still holds its
	# references, the failure to report is that one.
	if [ -n "$CREATED_DEVICE" ]; then
		log_test_skip "DPLL final ref_tracker check" \
			      "device from an earlier test still present"

dpllsim_delete_device() can fail in test_pin_state_flooding(),
test_max_devices() or test_deploy_cycle() because the undeploy failed. The
device then stays live and stays in CREATED_DEVICES.

It still holds its dpll_device_get(..., &sim->tracker) and
dpll_pin_get(..., &pin->tracker) references, so its entries are non-empty
and not in the snapshot.

cleanup() only removes those devices at exit. Wouldn't test_ref_leaks()
spin for BUSYWAIT_TIMEOUT and then report "leaked DPLL references after
stress" for references the device legitimately holds?

> +
> +	log_test "DPLL stress ref_tracker check"
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109093523.931201-1-poros%40redhat.com