Re: [PATCH v5] selftests/mm: add process_madvise() tests

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]

On Tue, Jul 15, 2025 at 06:58:05PM +0800, wang lian wrote:
> > On Mon, Jul 14, 2025 at 08:25:33PM +0800, wang lian wrote:

> > > +	/* Cleanup */
> > > +	kill(self->child_pid, SIGKILL);
> > > +	waitpid(self->child_pid, NULL, 0);
> > > +	if (pidfd >= 0)
> > > +		close(pidfd);

> > The cleanup here won't get run if we skip or assert, skipping will
> > return immediately (you could replace the return with a 'goto cleanup')
> > and the asserts will exit the test immediately.  This will mean we leak

> Fortunately, this situation is handled by FIXTURE_TEARDOWN_PARENT, 
> which reliably takes care of cleanup when the test exits early via ASSERT_* or SKIP(). 

> During earlier testing (in v3), I did run into an issue where a missing cleanup 
> led to run_vmtests hanging,which prompted me to make sure that subsequent versions 
> correctly rely on the fixture teardown mechanism for child process termination.

> So while your concern definitely makes sense, this specific case should be 
> well-covered by the existing teardown logic.

Are you sure the parent cleanup sees variables set in the child and
actually takes effect?  We fork() the child so it should be a new VM
which means that values stored there won't be seen by the parent, it'll
see whatever values it had before the fork().

It does also seem like bad practice to have duplicated cleanup code in
both the test and the fixture cleanups, the fixture cleanup always runs
so we should just use that all the time if it's in use (that's the whole
idea really).  Indeed as things stand since the cleanup in the test
doesn't reset self->child_pid so if the cleanup at the fixture level
does anything there's a minor risk that we might race with the PID being
reused and kill some new task 

The fixture teardown handler also doesn't do the close(pidfd), either
that's redundant for the in test cleanup or should be in the fixture
(given that it's a child process it should be redundant as any open file
descriptors are closed when the process exits).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]