Re: [LTP] ftrace: Add common library for C implementation

[linuxtestproject.agent@gmail.com]

Hi Praveen,

On Sun, 07 Jun 2026 01:28:14 +0530, Praveen K Pandey wrote:
> ftrace: Add common library for C implementation

--- [PATCH 1/5] ---

> +	/* Mount debugfs if not already mounted */
> +	if (!found) {
> +		char *tmpdir = tst_tmpdir_path();

tst_tmpdir_path() calls tst_brkm(TBROK) with the message
".needs_tmpdir must be set!" when the tmpdir has not been created by
the framework. None of the tests in this series set .needs_tmpdir = 1,
so if debugfs is not already mounted the test aborts with a confusing
TBROK instead of a clear error. Either set .needs_tmpdir = 1 in each
test, or use a separate hard-coded path (e.g. via mkdtemp()) for the
debugfs mount point.

--- [PATCH 2/5] ---

> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "1dbd195"},
> +		{"CVE", "2015-8550"},
> +		{}
> +	}

CVE-2015-8550 is a Xen cross-VM information-leak vulnerability. It is
unrelated to the kernel ftrace preempt-count leak fixed by commit
1dbd195. Is this the right CVE?

> +	/* Wait for trace events to be written */
> +	TST_RETRY_FUNC(ftrace_read_file("trace"), TST_RETVAL_NOTNULL);

The return value of TST_RETRY_FUNC is the last return value of
ftrace_read_file(), which is an allocated buffer. Discarding it leaks
that memory. The result should be assigned to a pointer and freed, even
if the content is not needed here.

> + * [Algorithm]
> + *
> + * 1. Enable stack tracer
> + * 2. Enable userstacktrace option
> + * 3. Enable page fault events (either exceptions:page_fault_kernel or
> + *    kmem:mm_kernel_pagefault depending on kernel version)
> + * 4. Repeat multiple times to trigger the issue
> + * 5. System should not panic

The [Algorithm] section must use a bulleted list with '-' markers, not
a numbered list. See documentation.md rule 4.

> +static struct tst_test test = {
> +	.test_all = run_test,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,

Ground Rule 4 requires that the reason root is needed be documented in
the test's doc comment. The /*\ ... */ block does not mention it. A
line such as "Requires root to access and configure the ftrace
subsystem via debugfs." is sufficient.

--- [PATCH 3/5] ---

> + * [Algorithm]
> + *
> + * 1. Enable signal:signal_generate event
> + * 2. Enable tracing
> + * 3. Generate some signals by running commands
> + * 4. Check trace output for 'grp=' and 'res=' fields
> + * 5. Test passes if both fields are present

Same as above: [Algorithm] must use '-' bullets, not a numbered list.

> +	.needs_root = 1,

Same as above: the doc comment must state why root is required.

--- [PATCH 4/5] ---

> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> +
> +# C test binaries with shared library dependencies
> +ftrace_regression01:	ftrace_lib.o
> +ftrace_regression02:	ftrace_lib.o
> +ftrace_stress_test:	ftrace_lib.o
> +
> +# Only ftrace_stress_test needs pthread
> +ftrace_stress_test: LDLIBS += -lpthread

Per-target LDLIBS assignments must appear between the two framework
includes, not after generic_leaf_target.mk. Object-dependency rules
(the ftrace_lib.o lines) must be after, but the LDLIBS line must move
up, before the generic_leaf_target.mk include. See build-system.md
section "Per-target flags".

> +ftrace_stress_test:	ftrace_lib.o

There is no .gitignore in this directory. The three new binaries
(ftrace_regression01, ftrace_regression02, ftrace_stress_test) must be
listed there so they are not reported as untracked files. See
c-tests.md rule 6.

> + * [Description]
> + *
> + * Stress test for ftrace subsystem.

The [Description] header is deprecated. Remove it and keep only the
description text. See c-tests.md rule 18.

> +	.needs_root = 1,

Same as above: doc comment must state why root is required.

The series updates runtest/tracing to reference the new C binaries but
does not remove the old shell files that are now orphaned:

  testcases/kernel/tracing/ftrace_test/ftrace_regression01.sh
  testcases/kernel/tracing/ftrace_test/ftrace_regression02.sh
  testcases/kernel/tracing/ftrace_test/ftrace_stress_test.sh
  testcases/kernel/tracing/ftrace_test/ftrace_lib.sh

These files are no longer installed (the INSTALL_TARGETS line was
removed) and are no longer referenced from runtest, but they remain in
the tree. They should be deleted, either in this patch or in a
follow-up.

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