[Devel] [PATCH vz10 0/3] ve/fs: make mount ownership follow the mount namespace

[Mirian Shilakadze]

mnt->ve_owner is meant to say which VE a mount belongs to. It is read by
ve_check_trusted_file(), which stops ve0 executing content a container
could have written, and by the per-VE mount accounting behind ve.mnt_nr.

It is wrong in two different ways, and each one alone defeats the trusted
exec check. Fixing the second exposed a third bug, a host panic, which is
patch 1 and the reason this is one series rather than two.

Patch 1 fixes the panic. alloc_mnt_ns() links every namespace onto
all_mntns_list and takes a reference on its owning VE, and both are undone
only in free_mnt_ns(). copy_mnt_ns()'s copy_tree() failure path does not
call it, so it frees a namespace that is still linked and leaks the VE
reference, and the next namespace creation runs list_add_tail() through
the dangling entry. A container at its own sysctl_ve_mount_nr limit
calling unshare(CLONE_NEWNS) reaches that path deterministically and
panics the host. This is VSTOR-141545.

Patch 2 fixes ownership not being updated when a mount moves. ve_owner is
assigned once in ve_mount_nr_inc() from alloc_vfsmnt() and never changes,
so a mount the host hands to a container keeps ve_owner == ve0 while
living in the container's mount namespace, and a host tmpfs bindmounted
into a container is trusted even though the container can write to it.
This is VSTOR-141322.

Patch 3 fixes ownership being wrong at creation. A new mount takes its
owner from get_exec_env(), the VE of the task, rather than the VE of the
mount namespace being worked in. Those differ for a ve0 task that entered
a container's mount namespace with nsenter -m. copy_mnt_ns() and
open_detached_copy() both build mounts that way and neither reaches
commit_tree(), so a plain unshare(CLONE_NEWNS) or an
open_tree(OPEN_TREE_CLONE) hands back ve0 owned copies of container
content. This is VSTOR-141429.

Patches 2 and 3 apply the same rule, the one already used when a namespace
is copied at container creation: ownership comes from the mount namespace,
never from the calling task.

Testing
=======

Built on 6.12.0-211.39.1.16.4.vz10, debug flavour, with KASAN, lockdep,
PROVE_LOCKING, DEBUG_ATOMIC_SLEEP, DEBUG_LIST, DEBUG_VM and gcov on
fs/namespace.o. Also compiled with CONFIG_VE=n. Each commit builds
standalone, so bisect is safe.

Before and after on the same host, same commands. Stock
6.12.0-211.30.1.14.4.vz10 on the left, the series on the right:

  container at its mount limit, unshare(CLONE_NEWNS)  PANIC    -> survives
  CT tmpfs, plain nsenter (control)                   refused  -> refused
  nsenter + unshare -m                                EXECUTED -> refused
  nsenter + open_tree + execveat                      EXECUTED -> refused
  host tmpfs lent via --bindmount_add                 EXECUTED -> refused

The panic was captured on stock over netconsole: "list_add corruption ...
kernel BUG at lib/list_debug.c:32", Comm: unshare, ve: 900, trace
alloc_mnt_ns <- copy_mnt_ns <- unshare. Against the series the identical
sequence, 150 container unshares at the limit followed by 300 host
namespace creations, completes with no corruption and no dump. The
control case behaving the same on both kernels shows the difference is
the change and not the environment.

No KASAN, lockdep, atomic sleep, refcount or list corruption reports
across any of it. DEBUG_ATOMIC_SLEEP staying quiet covers the one thing
worth asking about in patch 2, that get_ve()/put_ve() are called under
lock_mount_hash().

ve.mnt_nr drifted by 0 over 15 bindmount add and remove cycles, and
nr_dying_descendants moved by 1 across 150 opportunities to leak a VE
reference, so patch 1 releases the reference rather than merely not
crashing.

Coverage from gcov, so the new code is known to have run rather than just
linked: ve_mount_reown() called 54300 times with the ownership transfer
branch taken 958 times, and both commit_tree() call sites exercised,
20024 for the moved tree and 24081 for the propagation loop.

selftests: mount 2/2, mount_setattr 21/21, ve_perms 14/14, ve_ns_owner
2/2. The last one matters most, it asserts ve.mnt_nr behaviour around
CLONE_NEWVE, which is the line patch 3 changes.

vzctl functional suite: 538 of 603 passed. About 25 of the failures are
vzctl returning exit 21 where the suite asserts 20 for an unrecognized
option, which never reaches the kernel. The other ten were re-run one at
a time: two passed, and the rest fail in container creation or disk setup
on a test filesystem out of space, not on the mount operations they
exercise.

KCSAN, on a separate build of the same series with KCSAN enabled at
runtime: 2520 reports over a 15 minute run, drained continuously so that
is every report rather than what happened to survive in the ring buffer,
while KCSAN's own counter went from 5174 to 10179 data races. None of
them names ve_owner, commit_tree(), ve_check_trusted_file(),
is_sb_ve_accessible() or mnt_ns_unlink(). The code carrying the new store
was hot and instrumented throughout: attach_recursive_mnt(), which calls
commit_tree(), appears in 201 stack traces, and 193 of the reports are
races in propagate_one() and propagate_mnt() beside it. Those are
pre-existing upstream races on inode and mount fields, not on ve_owner.

Not addressed here
==================

Patch 2 lets a container be pushed above sysctl_ve_mount_nr by mounts the
host gives it, and while over it the container's own mounts are refused
until the count drops. The default limit is 4096 so this takes an unusual
number of lent mounts, and the container can unmount them, but it is the
host's action that spends the container's budget. Enforcing the limit at
handover is possible, attach_recursive_mnt() already does the equivalent
for sysctl_mount_max in count_mounts(), but ve_mount_allowed() tests the
creating task's VE while the counter follows the owner, so the limit
currently has two meanings and picking one is a separate decision.

Mirian Shilakadze (3):
  ve/fs: unlink the mount namespace on the copy_mnt_ns() error path
  ve/fs: transfer mount ownership when a mount enters another VE
  ve/fs: take the owner of copied mounts from the namespace, not the task

 fs/mount.h     |  2 +-
 fs/namespace.c | 73 ++++++++++++++++++++++++++++++++++++++++++++------
 kernel/ve/ve.c |  6 ++++-
 3 files changed, 71 insertions(+), 10 deletions(-)

-- 
2.43.0