Re: [LTP] lib: Add tst_sysfs_assert

[linuxtestproject.agent@gmail.com]

Hi Cyril,

On Tue, 25 Aug 2026, Cyril Hrubis wrote:
> lib: Add tst_sysfs_assert

This is a review of the 31-patch sysfs series. Comments are grouped per
patch; only patches with findings are quoted below.

--- [PATCH 1/31] lib: Add tst_sysfs_assert ---

> Adds helpers for a sysfs testcases.

The body only restates the subject. Could it say why the helper API is
needed (which upcoming tests use it, what problem it solves)? A ~1500 line
new public API is not self-explanatory from the subject alone.

> 	/* First pass: only to find the highest id so we can size the bitmap. */
> 	if (parse_list(file, lineno, path, NULL, NULL, &max_id))
> 		return NULL;
>
> 	bytes = max_id < 0 ? 1 : max_id / 8 + 1;
> 	map = SAFE_MALLOC(bytes);
> 	memset(map, 0, bytes);
>
> 	/* Second pass: fill the bitmap. */
> 	if (parse_list(file, lineno, path, map, NULL, NULL)) {

read_list_map() sizes the bitmap from a first parse and then fills it from a
second, independent parse of the same file. parse_list() writes without a
bounds check:

> 		if (map)
> 			map[i / 8] |= 1 << (i % 8);

The documented sources include mutable lists (cpu online/offline, node
online). If the list grows across an 8-id boundary between the two reads,
e.g. a CPU whose id exceeds the first pass's max is onlined by concurrent
hotplug, the second pass parses a higher id than "bytes" was sized for and
map[i / 8] writes past the allocation. Would it be safer to size the bitmap
from a fixed upper bound (kernel_max / nr possible), or to cap parse_list()
writes against the allocation size?

> unsigned long tst_sysfs_read_lx(const char *file, const int lineno,

> /**
>  * TST_SYSFS_READ_LX() - Reads a hexadecimal unsigned long from a file.
>  *
>  * Reads a long value from the file at the path built from fmt and returns it.

The description body says "Reads a long value" while the summary and the
implementation read a hexadecimal unsigned long (looks copy-pasted from
TST_SYSFS_READ_LI).

--- [PATCH 3/31] testcases: sysfs: Add sys_kernel01 ---

> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +include $(top_srcdir)/include/mk/generic_trunk_target.mk

This is a leaf test directory (it holds sys_kernel01.c and no
subdirectories) but includes the trunk target. generic_trunk_target.inc
errors out when SUBDIRS is empty:

    ifeq ($(strip $(SUBDIRS)),)
        $(error SUBDIRS empty -- did you want generic_leaf_target instead?)

so building this directory aborts, and sys_kernel01 is never built even
though runtest/sysfs and .gitignore reference it. Should it use
generic_leaf_target.mk like the sibling power/Makefile?

Note that patch 27 later adds kernel/mm/ under this directory, which makes
SUBDIRS non-empty and hides the hard error, but the trunk target still does
not compile sys_kernel01.c (it only recurses into mm/), so at the end of the
series sys_kernel01 is silently never built.

--- [PATCH 8/31] testcases: sysfs: Add sys_cpu_vulnerabilities01 ---

> +static const char *const known_prefixes[] = {
> +	"Not affected",
> +	"Vulnerable",
> +	"Mitigation:",
> +	"Unknown",
> +	"Processor vulnerable",
> +};

/sys/devices/system/cpu/vulnerabilities/itlb_multihit is emitted by
itlb_multihit_show_state() in arch/x86/kernel/cpu/bugs.c as one of:

    "KVM: Mitigation: VMX unsupported"
    "KVM: Mitigation: VMX disabled"
    "KVM: Mitigation: Split huge pages"
    "KVM: Vulnerable"

These start with "KVM: ", which matches none of the known prefixes, so
check_vuln() reports TFAIL on the common x86 Intel host that exposes
itlb_multihit. Should a "KVM:" prefix be added (or the "Mitigation:"/
"Vulnerable" match allowed after an optional "KVM: ")?

--- [PATCH 12/31] testcases: sysfs: Add sys_ata01 ---

> +static const char *const class_allowed[] = {
> +	"ata", "atapi", "pmp", "semb", "unknown", NULL
> +};

/sys/class/ata_device/<dev>/class is produced by get_ata_class_names() over
ata_class_names[] in drivers/ata/libata-transport.c, whose table also
contains:

    { ATA_DEV_ZAC,   "zac" },
    { ATA_DEV_NONE,  "none" }

A ZAC (host-managed SMR) ATA device reports class "zac", which is not in
class_allowed, so TST_SYSFS_ASSERT_ONEOF reports TFAIL for a valid value.
Should "zac" (and "none") be added?

--- [PATCH 23/31] testcases: sysfs: Add sys_net04 ---

> +	if (tun_fd >= 0)
> +		SAFE_CLOSE(tun_fd);
> +
> +	if (tap_fd >= 0)
> +		SAFE_CLOSE(tap_fd);

Both descriptors are initialized to -1; the convention is to test them with
fd != -1 rather than fd >= 0.

--- [PATCH 30/31] testcases: sysfs: Add sys_swap01 ---

> testcases: sysfs: Add sys_swap01

The test added is sys_mm_swap01 (sys_mm_swap01.c, runtest entry
"sys_mm_swap01 sys_mm_swap01", .gitignore /sys_mm_swap01). The subject names
sys_swap01, which does not match the actual test.

Verdict - Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp