Re: [PATCH 15/34] check: run tests in a private pid/mount namespace

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Feb 06, 2025 at 08:15:03AM +1100, Dave Chinner wrote:
> On Wed, Feb 05, 2025 at 10:19:59AM -0800, Darrick J. Wong wrote:
> > On Wed, Feb 05, 2025 at 10:00:48AM -0800, Darrick J. Wong wrote:
> > > On Wed, Feb 05, 2025 at 11:37:00AM +1100, Dave Chinner wrote:
> > > > On Tue, Feb 04, 2025 at 01:26:13PM -0800, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > > 
> > > > > As mentioned in the previous patch, trying to isolate processes from
> > > > > separate test instances through the use of distinct Unix process
> > > > > sessions is annoying due to the many complications with signal handling.
> > > > > 
> > > > > Instead, we could just use nsexec to run the test program with a private
> > > > > pid namespace so that each test instance can only see its own processes;
> > > > > and private mount namespace so that tests writing to /tmp cannot clobber
> > > > > other tests or the stuff running on the main system.
> > > > > 
> > > > > However, it's not guaranteed that a particular kernel has pid and mount
> > > > > namespaces enabled.  Mount (2.4.19) and pid (2.6.24) namespaces have
> > > > > been around for a long time, but there's no hard requirement for the
> > > > > latter to be enabled in the kernel.  Therefore, this bugfix slips
> > > > > namespace support in alongside the session id thing.
> > > > > 
> > > > > Declaring CONFIG_PID_NS=n a deprecated configuration and removing
> > > > > support should be a separate conversation, not something that I have to
> > > > > do in a bug fix to get mainline QA back up.
> > > > > 
> > > > > Cc: <fstests@vger.kernel.org> # v2024.12.08
> > > > > Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
> > > > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > > > ---
> > > > >  check               |   34 +++++++++++++++++++++++-----------
> > > > >  common/rc           |   12 ++++++++++--
> > > > >  src/nsexec.c        |   18 +++++++++++++++---
> > > > >  tests/generic/504   |   15 +++++++++++++--
> > > > >  tools/run_seq_pidns |   28 ++++++++++++++++++++++++++++
> > > > >  5 files changed, 89 insertions(+), 18 deletions(-)
> > > > >  create mode 100755 tools/run_seq_pidns
> > > > 
> > > > Same question as for session ids - is this all really necessary (or
> > > > desired) if check-parallel executes check in it's own private PID
> > > > namespace?
> > 
> > Forgot to respond to this question --
> > 
> > Because check-parallel runs (will run?) each child ./check instance in a
> > private namepsace, each of those instances will be isolated from the
> > others.  So no, it's probably not absolutely necessary.
> > 
> > However, there are a couple of reasons to let it happen: (a) the private
> > ns that ./check creates in _run_seq() isolates the actual test code from
> > its parent ./check process; and (b) the process started by nsexec is
> > considered to be the "init" process for that namespace, so when it dies,
> > the kernel will kill -9 all other processes in that namespace, so we
> > won't have any stray fsstress processes that bleed into the next test.
> 
> Ok - that might be worth adding to the commit message so that anyone
> looking at it in a few months time doesn't need to remember this
> detail.

It /does/ say that in _run_seq() but yeah I'll add it to the commit
message too:

"Instead, we could just use nsexec to run the test program with a
private pid namespace so that each test instance can only see its own
processes; and private mount namespace so that tests writing to /tmp
cannot clobber other tests or the stuff running on the main system.
Further, the process created by the clone(CLONE_NEWPID) call is
considered the init process of that pid namespace, so all processes will
be SIGKILL'd when the init process terminates, so we no longer need
systemd scopes for externally enforced cleanup."

--D

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>