[Devel] [PATCH vz10] fs: let the protection flags of a mount be locked from userspace

[no-reply at sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] fs: do_remount() returns -EPERM after reconfigure_super() already changed the sb
- [Low] fs: MOUNT_ATTR_VE_LOCK lock state is write-only, not reported by mnt_to_attr_flags()
- [Low] perf beauty: MOUNT_ATTR_VE_LOCK grows fsmount_attr_flags to 65 entries, UB on 32-bit
--

commit 8c0f169fd03f2f47c6755d18498f04737a8f7f21
Author: Mirian Shilakadze <mirian.shilakadze at virtuozzo.com>

fs: let the protection flags of a mount be locked from userspace

This patch adds MOUNT_ATTR_VE_LOCK to mount_setattr(). With it, a host task
with CAP_SYS_ADMIN can lock or unlock the ro, nosuid, nodev and noexec
protections of a mount. It also re-tests can_change_locked_flags() under the
mount hash in do_reconfigure_mnt() and do_remount().

Link: https://virtuozzo.atlassian.net/browse/VSTOR-141092

> diff --git a/fs/namespace.c b/fs/namespace.c
> --- a/fs/namespace.c
> +++ b/fs/namespace.c

[ ... ]

> @@ -3483,7 +3520,15 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags,
>  			err = reconfigure_super(fc);
>  			if (!err) {
>  				lock_mount_hash();
> -				set_mount_attributes(mnt, mnt_flags);
> +				/*
> +				 * A lock may have landed since the first test.
> +				 * The superblock change above stands, the mount
> +				 * flags do not change.
> +				 */
> +				if (can_change_locked_flags(mnt, mnt_flags))
> +					set_mount_attributes(mnt, mnt_flags);
> +				else
> +					err = -EPERM;
>  				unlock_mount_hash();
>  			}
>  		}

[Severity: Medium]
Can do_remount() return -EPERM after the superblock has already been
changed?

The first can_change_locked_flags() test is made with no lock held. Then
reconfigure_super(fc) runs under s_umount. It rewrites sb->s_flags (for
example it clears SB_RDONLY) and calls fc->ops->reconfigure(). Neither can
be rolled back.

mount_setattr() takes only the mount hash, not s_umount. So nothing
serializes a host MOUNT_ATTR_VE_LOCK against this window. If the lock lands
between the first test and the re-test, the re-test fails and err is set to
-EPERM.

The caller then sees a failure, but the shared superblock may have gone rw
or taken new options. That affects every other mount of the same sb. The
comment in the patch says "The superblock change above stands".

Would it be possible to run the re-test before reconfigure_super(), or to
exclude the lock path from remount, for example by taking s_umount shared in
the lock path?

[ ... ]

> @@ -5230,8 +5278,33 @@ static int build_mount_kattr(const struct mount_attr *attr, size_t usize,
>  	if ((attr->attr_set | attr->attr_clr) & ~MOUNT_SETATTR_VALID_FLAGS)
>  		return -EINVAL;
>  
> +	kattr->ve_lock = !!(attr->attr_set & MOUNT_ATTR_VE_LOCK);

[Severity: Low]
This isn't a bug, but the lock state set here looks write-only. Can
userspace read it back?

This is the only place the new flag is consumed. mnt_to_attr_flags(), which
builds the attribute word that statmount() returns as sm.mnt_attr, ignores
MNT_LOCK_READONLY, MNT_LOCK_NOSUID, MNT_LOCK_NODEV and MNT_LOCK_NOEXEC. It
never returns MOUNT_ATTR_VE_LOCK.

The commit message says libvzctl locks bindmounts at container start, on a
live add and again after restore. How is it expected to learn whether a
mount is already locked? The only probe appears to be a remount that is
expected to fail with EPERM.

[ ... ]

> diff --git a/tools/perf/trace/beauty/include/uapi/linux/mount.h b/tools/perf/trace/beauty/include/uapi/linux/mount.h
> --- a/tools/perf/trace/beauty/include/uapi/linux/mount.h
> +++ b/tools/perf/trace/beauty/include/uapi/linux/mount.h

[ ... ]

> +#define MOUNT_ATTR_VE_LOCK	0x8000000000000000ULL

[Severity: Low]
Can this define break the perf beauty flag table on 32-bit builds?

tools/perf/trace/beauty/fsmount.sh matches the define with its regex. The
ULL suffix is absorbed by the trailing ".*". It then emits this entry:

	[ilog2(0x8000000000000000) + 1] = "VE_LOCK",

That is index 64, so fsmount_attr_flags grows to 65 entries.
strarray__scnprintf_flags() then loops over those entries:

tools/perf/trace/beauty/pkey_alloc.c:strarray__scnprintf_flags() {
	for (i = 1; i < sa->nr_entries; ++i) {
		unsigned long bit = 1UL << (i - 1);
		...
}

On a 32-bit host, unsigned long is 32 bits wide. Every iteration with
i >= 34 shifts by 33 or more, which is undefined behaviour. The shift
can alias low flag bits, so perf trace may print wrong flag names for
fsmount() attributes.

Before this patch the highest index was around 22. Also, fsmount() takes
only 32-bit attributes, so the VE_LOCK entry would never be used there.

Could this define be kept out of the copy that the beauty scripts parse, or
could fsmount.sh skip entries that do not fit in 32 bits?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012102058.46896-1-mirian.shilakadze@virtuozzo.com?part=1