Re: [PATCH 5/5] selftests/filesystems: create setup_userns() helper

[John Hubbard <jhubbard@nvidia.com>]

On 5/7/25 1:43 PM, Amir Goldstein wrote:
> Add helper to utils and use it in statmount userns tests.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>   .../filesystems/statmount/statmount_test_ns.c | 60 +----------------
>   tools/testing/selftests/filesystems/utils.c   | 65 +++++++++++++++++++
>   tools/testing/selftests/filesystems/utils.h   |  1 +
>   3 files changed, 68 insertions(+), 58 deletions(-)
> 
> diff --git a/tools/testing/selftests/filesystems/statmount/statmount_test_ns.c b/tools/testing/selftests/filesystems/statmount/statmount_test_ns.c
> index 375a52101d08..3c5bc2e33821 100644
> --- a/tools/testing/selftests/filesystems/statmount/statmount_test_ns.c
> +++ b/tools/testing/selftests/filesystems/statmount/statmount_test_ns.c
> @@ -79,66 +79,10 @@ static int get_mnt_ns_id(const char *mnt_ns, uint64_t *mnt_ns_id)
>   	return NSID_PASS;
>   }
>   
> -static int write_file(const char *path, const char *val)
> -{
> -	int fd = open(path, O_WRONLY);
> -	size_t len = strlen(val);
> -	int ret;
> -
> -	if (fd == -1) {
> -		ksft_print_msg("opening %s for write: %s\n", path, strerror(errno));
> -		return NSID_ERROR;
> -	}
> -
> -	ret = write(fd, val, len);
> -	if (ret == -1) {
> -		ksft_print_msg("writing to %s: %s\n", path, strerror(errno));
> -		return NSID_ERROR;
> -	}
> -	if (ret != len) {
> -		ksft_print_msg("short write to %s\n", path);
> -		return NSID_ERROR;
> -	}
> -
> -	ret = close(fd);
> -	if (ret == -1) {
> -		ksft_print_msg("closing %s\n", path);
> -		return NSID_ERROR;
> -	}
> -
> -	return NSID_PASS;
> -}
> -
>   static int setup_namespace(void)
>   {
> -	int ret;
> -	char buf[32];
> -	uid_t uid = getuid();
> -	gid_t gid = getgid();
> -
> -	ret = unshare(CLONE_NEWNS|CLONE_NEWUSER|CLONE_NEWPID);
> -	if (ret == -1)
> -		ksft_exit_fail_msg("unsharing mountns and userns: %s\n",
> -				   strerror(errno));
> -
> -	sprintf(buf, "0 %d 1", uid);
> -	ret = write_file("/proc/self/uid_map", buf);
> -	if (ret != NSID_PASS)
> -		return ret;
> -	ret = write_file("/proc/self/setgroups", "deny");
> -	if (ret != NSID_PASS)
> -		return ret;
> -	sprintf(buf, "0 %d 1", gid);
> -	ret = write_file("/proc/self/gid_map", buf);
> -	if (ret != NSID_PASS)
> -		return ret;
> -
> -	ret = mount("", "/", NULL, MS_REC|MS_PRIVATE, NULL);
> -	if (ret == -1) {
> -		ksft_print_msg("making mount tree private: %s\n",
> -			       strerror(errno));
> +	if (setup_userns() != 0)
>   		return NSID_ERROR;
> -	}
>   
>   	return NSID_PASS;
>   }
> @@ -200,7 +144,7 @@ static void test_statmount_mnt_ns_id(void)
>   		return;
>   	}
>   
> -	ret = setup_namespace();
> +	ret = setup_userns();
>   	if (ret != NSID_PASS)
>   		exit(ret);
>   	ret = _test_statmount_mnt_ns_id();
> diff --git a/tools/testing/selftests/filesystems/utils.c b/tools/testing/selftests/filesystems/utils.c
> index 9b5419e6f28d..9dab197ddd9c 100644
> --- a/tools/testing/selftests/filesystems/utils.c
> +++ b/tools/testing/selftests/filesystems/utils.c
> @@ -18,6 +18,7 @@
>   #include <sys/types.h>
>   #include <sys/wait.h>
>   #include <sys/xattr.h>
> +#include <sys/mount.h>
>   
>   #include "utils.h"
>   
> @@ -447,6 +448,70 @@ static int create_userns_hierarchy(struct userns_hierarchy *h)
>   	return fret;
>   }
>   
> +static int write_file(const char *path, const char *val)
> +{
> +	int fd = open(path, O_WRONLY);
> +	size_t len = strlen(val);
> +	int ret;
> +
> +	if (fd == -1) {
> +		syserror("opening %s for write: %s\n", path, strerror(errno));

While I have no opinion about ksft_print_msg() vs. syserror(), I do
think it's worth a mention in the commit log: there is some reason
that you changed to syserror() throughout. Could you write down
what that was?

In any case, it looks correct, so with an update commit message, please
feel free to add:


Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard


> +		return -1;
> +	}
> +
> +	ret = write(fd, val, len);
> +	if (ret == -1) {
> +		syserror("writing to %s: %s\n", path, strerror(errno));
> +		return -1;
> +	}
> +	if (ret != len) {
> +		syserror("short write to %s\n", path);
> +		return -1;
> +	}
> +
> +	ret = close(fd);
> +	if (ret == -1) {
> +		syserror("closing %s\n", path);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +int setup_userns(void)
> +{
> +	int ret;
> +	char buf[32];
> +	uid_t uid = getuid();
> +	gid_t gid = getgid();
> +
> +	ret = unshare(CLONE_NEWNS|CLONE_NEWUSER|CLONE_NEWPID);
> +	if (ret) {
> +		syserror("unsharing mountns and userns: %s\n", strerror(errno));
> +		return ret;
> +	}
> +
> +	sprintf(buf, "0 %d 1", uid);
> +	ret = write_file("/proc/self/uid_map", buf);
> +	if (ret)
> +		return ret;
> +	ret = write_file("/proc/self/setgroups", "deny");
> +	if (ret)
> +		return ret;
> +	sprintf(buf, "0 %d 1", gid);
> +	ret = write_file("/proc/self/gid_map", buf);
> +	if (ret)
> +		return ret;
> +
> +	ret = mount("", "/", NULL, MS_REC|MS_PRIVATE, NULL);
> +	if (ret) {
> +		syserror("making mount tree private: %s\n", strerror(errno));
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   /* caps_down - lower all effective caps */
>   int caps_down(void)
>   {
> diff --git a/tools/testing/selftests/filesystems/utils.h b/tools/testing/selftests/filesystems/utils.h
> index d9cf145b321a..70f7ccc607f4 100644
> --- a/tools/testing/selftests/filesystems/utils.h
> +++ b/tools/testing/selftests/filesystems/utils.h
> @@ -27,6 +27,7 @@ extern int caps_down(void);
>   extern int cap_down(cap_value_t down);
>   
>   extern bool switch_ids(uid_t uid, gid_t gid);
> +extern int setup_userns(void);
>   
>   static inline bool switch_userns(int fd, uid_t uid, gid_t gid, bool drop_caps)
>   {