Re: [Devel] [PATCH VZ10 1/3] selftests/ve: Add namespace management helpers

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

Sashiko review + replies.

On 9/7/26 21:14, Vladimir Riabchun wrote:
> When mount namespace is created in a child, it is destroyed
> when the child dies. We may need to keep it for a while to check
> how VE handles resources.
> 
> Instead of keeping a thread which lives in a new ve+mnt namespaces,
> let's store open namespace file descriptors to enter them later
> without keeping children alive.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-144504
> 
> Feature: ve selftests
> Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> ---
>   tools/testing/selftests/ve/ve_selftest.h | 118 +++++++++++++++++++++++++++++++
>   1 file changed, 118 insertions(+)
> 
> diff --git a/tools/testing/selftests/ve/ve_selftest.h b/tools/testing/selftests/ve/ve_selftest.h
> index c53bf7900d20..b3c5bf63f34d 100644
> --- a/tools/testing/selftests/ve/ve_selftest.h
> +++ b/tools/testing/selftests/ve/ve_selftest.h
> @@ -31,6 +31,124 @@
>   #define VE_CONTROLLERS \
>   	"+cpuset +cpu +cpuacct +io +memory +hugetlb +pids +rdma +misc +ve"
>   
> +struct namespace_ctx {
> +	int ns_save_mask;	/* Mask of stored CLONE_NEW* namespaces. */
> +	/* Namespace file descriptors. Name should match /proc/PID/ns/NAME */
> +	int ve_fd;
> +	int mnt_fd;
> +};
> +
> +static inline void close_namespace_fds(struct namespace_ctx *ctx)
> +{
> +	if (ctx->ns_save_mask & CLONE_NEWVE)
> +		close(ctx->ve_fd);
> +	if (ctx->ns_save_mask & CLONE_NEWNS)
> +		close(ctx->mnt_fd);
> +
> +	ctx->ns_save_mask = 0;
> +}
> +
> +static inline int save_namespace_fds(struct namespace_ctx *ctx, int flags)
> +{
> +#define OPEN_NS(name, flag)                                                \
> +do {                                                                       \
> +	if (flags & flag) {                                                \
> +		ctx->name##_fd = open("/proc/self/ns/" #name, O_RDONLY);   \
> +		if (ctx->name##_fd < 0) {                                  \
> +			fprintf(stderr, "%s: could not open fd %s: %s\n",  \
> +				__func__, #name, strerror(errno));         \
> +			goto err;                                          \
> +		}                                                          \
> +		ctx->ns_save_mask |= flag;                                 \
> +	}                                                                  \
> +} while (0)
> +
> +	ctx->ns_save_mask = 0;

Sashiko says:
  If save_namespace_fds() is called twice on the same namespace_ctx
  without calling close_namespace_fds() in between, does this line reset
  the mask and leak the previously opened file descriptors?

User is responsible for freeing resources correctly. The idea of this design
was to avoid additional initialization of ctx before calling it. With current
approach we can pass ctx with any stack garbage and it will work.

> +
> +	OPEN_NS(ve, CLONE_NEWVE);
> +	OPEN_NS(mnt, CLONE_NEWNS);
> +
> +	return 0;
> +
> +err:
> +	close_namespace_fds(ctx);
> +	return -1;
> +}
> +
> +static inline int enter_namespaces(struct namespace_ctx *ctx, int flags)
> +{
> +#define JOIN_NS(name, flag)                                                 \
> +do {                                                                        \
> +	if (flags & flag) {                                                 \
> +		if (!(ctx->ns_save_mask & flag)) {                          \
> +			fprintf(stderr, "%s: could not switch ns %s:"       \
> +				" no fd in context.\n", __func__, #name);   \
> +			return -1;                                          \
> +		}                                                           \
> +		if (setns(ctx->name##_fd, flag)) {                          \
> +			fprintf(stderr, "%s: could not switch ns %s: %s\n", \
> +				__func__, #name, strerror(errno));          \
> +			return -1;                                          \
> +		}                                                           \
> +	}                                                                   \
> +} while (0)
> +
> +	JOIN_NS(mnt, CLONE_NEWNS);
> +	JOIN_NS(ve, CLONE_NEWVE);
> +	return 0;
> +}

Sashiko says:
  In enter_namespaces(), the namespaces are restored in a fixed order.
  Could this cause issues if there is a dependency between the VE and
  MNT namespaces that requires a specific restoration order based on
  the flags provided?

At least we should keep the same order as in save_namespace_fds. Will fix
in v2.

> +
> +static inline int enter_cgroup(int cgv2_fd, int ctid);
> +
> +/*
> + * Create new namespaces and store them in ctx.
> + * CLONE_NEWVE is treated specially - before unshare corresponding
> + * CT cgroup is entered, after it root cgroup is entered(not original one).
> + */
> +static inline int prepare_namespaces(struct namespace_ctx *ctx, int flags,
> +	int cgv2_fd, int ctid)
> +{
> +	struct namespace_ctx tmp_ctx;
> +
> +	if (save_namespace_fds(&tmp_ctx, flags) < 0)
> +		return -1;
> +
> +	if (flags & CLONE_NEWVE) {
> +		if (enter_cgroup(cgv2_fd, ctid) < 0) {
> +			fprintf(stderr, "%s: failed to enter CT cgroup\n", __func__);
> +			return -1;
> +		}
> +	}
> +
> +	if (unshare(flags) < 0) {
> +		fprintf(stderr, "%s: failed to unshare, flags %x: %s\n",
> +			__func__, flags, strerror(errno));
> +		return -1;
> +	}
> +
> +	if (save_namespace_fds(ctx, flags)) {
> +		fprintf(stderr, "%s: failed to save new namespaces, flags %x\n",
> +			__func__, flags);
> +		return -1;
> +	}

Sashiko says:
  In the error paths above, such as when enter_cgroup(), unshare(), or
  save_namespace_fds(ctx, flags) fails, are the file descriptors held
  in tmp_ctx leaked?

True, but honestly I don't think it matters a lot - failing here is a
critical error and the test should be aborted. But I'll fix in v2.

> +
> +	if (flags & CLONE_NEWVE) {
> +		if (enter_cgroup(cgv2_fd, 0) < 0) {
> +			fprintf(stderr, "%s: failed to restore root cgroup\n", __func__);
> +			return -1;
> +		}
> +	}
> +
> +	if (enter_namespaces(&tmp_ctx, flags) < 0) {
> +		fprintf(stderr, "%s: failed to restore old namespaces, flags %x\n",
> +			__func__, flags);
> +		return -1;
> +	}
> +
> +	close_namespace_fds(&tmp_ctx);
> +	return 0;
> +}

Sashiko says:
  If enter_namespaces(&tmp_ctx, flags) fails, does the process remain
  in an inconsistent namespace state for subsequent tests?
  Additionally, does this failure path leak the file descriptors
  stored in tmp_ctx?

Fair, will fix in v2.

> +
>   static inline int write_file_at(int dirfd, const char *path, const char *val)
>   {
>   	int fd, ret;
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel