Re: [LTP] thp04: Simplify to focus on CVE-2017-1000405 race test only

[linuxtestproject.agent@gmail.com]

Hi Jan,

On Thu, 16 Jul 2026, Jan Polensky wrote:
> thp04: Simplify to focus on CVE-2017-1000405 race test only

--- [PATCH 1/3] ---

> +	/* write_ptr points to last int in write_thp page */
> +	state->write_ptr = (int *)(state->write_thp + state->thp_size - sizeof(int));
> +	/* read_ptr points to last int in read_thp page */
> +	state->read_ptr = (int *)(state->read_thp + state->thp_size - sizeof(int));

These two comments restate what the pointer arithmetic already
expresses literally. The LTP coding style prohibits comments that
are plain English translations of the code. Should both be dropped?

--- [PATCH 2/3] ---

> +/*\
> + * ...
> + * Test behavior:
> + * - If write succeeds: TCONF (feature not active)

RST requires a blank line before a bulleted list. Without it the
list items render as continuation of the preceding paragraph. Should
a blank "* " line be inserted between "Test behavior:" and the list?

> +static void setup(void)
> +{
> +	/* Allocate a page, initialize it, then make it read-only */
> +	test_ptr = SAFE_MMAP(NULL, sizeof(int), PROT_READ | PROT_WRITE,
> +			     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +	*test_ptr = 0;
> +
> +	/* Make the page read-only to test permission override blocking */
> +	SAFE_MPROTECT((void *)test_ptr, sizeof(int), PROT_READ);
> +
> +	/* Open /proc/self/mem for writing */
> +	memfd = SAFE_OPEN("/proc/self/mem", O_RDWR);

The first comment is immediately superseded by the more specific one
that follows it. The third ("Open /proc/self/mem for writing") and the
inline comments in run() ("Seek to our test memory location",
"Attempt to write to our own memory via /proc/self/mem") all describe
what the API call already says. Are these needed?

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

> +/*\
> + * ...
> + * This test validates the ptrace-based memory write mechanism that
> + * becomes mandatory when CONFIG_PROC_MEM_FORCE_PTRACE=y is active.
> + *
> + * Test flow:
> + * 1. Parent forks a child process

Same RST issue as in patch 2: no blank line between "Test flow:" and
the ordered list. Should a blank "* " line be inserted there?

> +	for (i = 0; i < TEST_ITERATIONS; i++) {
> +		/* Verify parent's write from previous iteration */
> +		if (*shared->test_ptr != shared->expected_val) {

The comment says "previous iteration" but the parent writes
expected_val and *test_ptr, then resumes the tracee. When the
tracee executes iteration i it is verifying what the parent wrote
in iteration i -- not i-1. Is the comment wrong?

> +struct shared_state {
> +	int *test_ptr;
> +	int expected_val;
> +	int result;

The result field is assigned in tracee_main() (0 on success, 1 on
failure) but is never read by the parent. The parent uses
WEXITSTATUS(status) to detect tracee failure. Is the field intended
to be used somewhere, or is it dead code that can be removed?

> +static void tracee_main(void)
> +{
> +	int i;
> +
> +	/* Allocate test memory in tracee's address space */
> +	shared->test_ptr = SAFE_MMAP(NULL, sizeof(int), ...);
> +	*shared->test_ptr = 0;
> +
> +	/* Make the page read-only - parent will need FOLL_FORCE to write */
> +	SAFE_MPROTECT((void *)shared->test_ptr, sizeof(int), PROT_READ);
> +
> +	/* Signal parent that we're ready */
> +	TST_CHECKPOINT_WAKE(0);
> +
> +	/* Wait for parent to complete PTRACE_SEIZE */
> +	TST_CHECKPOINT_WAIT(1);
> +
> +	/* Initial stop - parent will write first value */
> +	raise(SIGSTOP);

And inside run():

> +	/* Fork tracee */
> +	tracee_pid = SAFE_FORK();
> +	...
> +	/* Wait for tracee to be ready */
> +	TST_CHECKPOINT_WAIT(0);
> +	/* Attach to tracee with PTRACE_SEIZE */
> +	SAFE_PTRACE(PTRACE_SEIZE, tracee_pid, NULL, NULL);
> +	/* Signal tracee that attachment is complete */
> +	TST_CHECKPOINT_WAKE(1);
> +	/* Wait for tracee's first self-stop */
> +	SAFE_WAITPID(tracee_pid, &status, 0);

And in the loop:

> +		/* Update expected value for tracee to verify */
> +		shared->expected_val = write_val;
> +		/* Write to tracee's memory while it's stopped */
> +		SAFE_LSEEK(memfd, (off_t)shared->test_ptr, SEEK_SET);
> +		SAFE_WRITE(SAFE_WRITE_ALL, memfd, &write_val, sizeof(write_val));
> +		/* Continue tracee to verify the write and stop again */
> +		SAFE_PTRACE(PTRACE_CONT, tracee_pid, NULL, NULL);
> +		/* Wait for tracee to stop itself after verification */
> +		SAFE_WAITPID(tracee_pid, &status, 0);

These comments ("Allocate test memory in tracee's address space",
"Signal parent that we're ready", "Wait for parent to complete
PTRACE_SEIZE", "Initial stop - parent will write first value",
"Fork tracee", "Wait for tracee to be ready", "Attach to tracee with
PTRACE_SEIZE", "Signal tracee that attachment is complete", "Wait for
tracee's first self-stop", "Update expected value for tracee to verify",
"Write to tracee's memory while it's stopped", "Continue tracee to
verify the write and stop again", "Wait for tracee to stop itself after
verification") each restate what the API call or macro already
expresses. The coding style rule says to explain why, not how. Should
these be removed, retaining only the comments that explain intent
(such as "parent will need FOLL_FORCE to write")?

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