linux-kernel - Re: [PATCH v5] selftests/mm: add process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250715105808.3634-1-lianux.mm@gmail.com>
Date: Tue, 15 Jul 2025 18:58:05 +0800
From: wang lian <lianux.mm@...il.com>
To: broonie@...nel.org
Cc: Liam.Howlett@...cle.com,
	akpm@...ux-foundation.org,
	brauner@...nel.org,
	david@...hat.com,
	gkwang@...x-info.com,
	jannh@...gle.com,
	lianux.mm@...il.com,
	linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org,
	linux-mm@...ck.org,
	lorenzo.stoakes@...cle.com,
	p1ucky0923@...il.com,
	ryncsn@...il.com,
	shuah@...nel.org,
	sj@...nel.org,
	vbabka@...e.cz,
	zijing.zhang@...ton.me,
	ziy@...dia.com
Subject: Re: [PATCH v5] selftests/mm: add process_madvise() tests

> On Mon, Jul 14, 2025 at 08:25:33PM +0800, wang lian wrote:
> > +TEST_F(process_madvise, remote_collapse)
> > +{
> > +	self->child_pid = fork();
> > +	ASSERT_NE(self->child_pid, -1);
> > +
> > +	ret = read(pipe_info[0], &info, sizeof(info));
> > +	if (ret <= 0) {
> > +		waitpid(self->child_pid, NULL, 0);
> > +		SKIP(return, "Failed to read child info from pipe.\n");
> > +	}
> > +	ASSERT_EQ(ret, sizeof(info));
> > +
> > +cleanup:
> > +	/* 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
> the child.  This isn't an issue for things that are memory mapped or
> tracked with file descriptors, the harness will for a new child for each
> test so anything that's cleaned up with the process will be handled, but
> that doesn't apply to child processes.

> I think doing the child setup in a fixture should DTRT but I haven't
> gone through in full detail to verify that this is the case.

Thanks a lot for pointing this out — it's a very reasonable concern.

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.

Thanks again for the careful review!

Best regards,
wang lian