Re: [LTP] [PATCH v3 2/2] syscalls/openat: Add new regression test when using open(O_TMPFILE) under umask

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
> + * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Check setgid strip logic whether works correctly when creating tmpfile under
> + * filesystem without posix acl supported(by using noacl mount option). Test it
                           ^
			   POSIX ACL

Both of these are acronyms and should be spelled with uppercase.

> + * with umask S_IXGRP and also check file mode whether has filtered S_IXGRP.
> + *
> + * Fixed in:
> + *
> + *  commit ac6800e279a22b28f4fc21439843025a0d5bf03e
> + *  Author: Yang Xu <xuyang2018.jy@fujitsu.com>
> + *  Date:   Thu July 14 14:11:26 2022 +0800
> + *
> + *  fs: Add missing umask strip in vfs_tmpfile
> + *
> + * The most code is pasted form creat09.c.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <pwd.h>
> +#include <sys/mount.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include "tst_test.h"
> +#include "tst_uid.h"
> +#include "tst_safe_file_at.h"
> +
> +#define MODE_RWX        0777
> +#define MODE_SGID       (S_ISGID|0777)
> +#define MNTPOINT	"mntpoint"
> +#define WORKDIR		MNTPOINT "/testdir"
> +#define OPEN_FILE	"open.tmp"
> +
> +static gid_t free_gid;
> +static int tmpfile_fd = -1, dir_fd = -1, mount_flag;
> +static struct passwd *ltpuser;
> +
> +static void do_mount(const char *source, const char *target,
> +	const char *filesystemtype, unsigned long mountflags,
> +	const void *data)
> +{
> +	TEST(mount(source, target, filesystemtype, mountflags, data));
> +
> +	if (TST_RET == -1 && TST_ERR == EINVAL)
> +		tst_brk(TCONF, "Kernel does not support noacl feature");
> +
> +	if (TST_RET == -1) {
> +		tst_brk(TBROK | TTERRNO, "mount(%s, %s, %s, %lu, %p) failed",
> +			source, target, filesystemtype, mountflags, data);
> +	} else if (TST_RET) {

There is no need for else if we do tst_brk() in the previous if ()

> +		tst_brk(TBROK, "Invalid return value %ld", TST_RET);
> +	}
> +
> +	mount_flag = 1;
> +}
> +
> +static void open_tmpfile_supported(int dirfd)
> +{
> +	TEST(openat(dirfd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID));
> +
> +	if (TST_RET == -1) {
> +		if (errno == ENOTSUP)
> +			tst_brk(TCONF, "fs doesn't support O_TMPFILE");
> +		else
> +			tst_brk(TBROK | TTERRNO, "open(%d, O_TMPFILE) failed", dirfd);
                                                      ^
						      openat
> +	} else if (TST_RET < 0) {

Here as well.

> +		tst_brk(TBROK, "Invalid return value %ld", TST_RET);
                                       ^
				       openat()
> +	}
> +
> +	SAFE_CLOSE(TST_RET);
> +}
> +
> +static void setup(void)
> +{
> +	struct stat buf;
> +
> +	ltpuser = SAFE_GETPWNAM("nobody");
> +
> +	do_mount(tst_device->dev, MNTPOINT, tst_device->fs_type, 0, "noacl");
> +
> +	tst_res(TINFO, "User nobody: uid = %d, gid = %d", (int)ltpuser->pw_uid,
> +		(int)ltpuser->pw_gid);
> +	free_gid = tst_get_free_gid(ltpuser->pw_gid);
> +
> +	/* Create directories and set permissions */
> +	SAFE_MKDIR(WORKDIR, MODE_RWX);
> +	dir_fd = SAFE_OPEN(WORKDIR, O_RDONLY, O_DIRECTORY);
> +	open_tmpfile_supported(dir_fd);
> +
> +	SAFE_CHOWN(WORKDIR, ltpuser->pw_uid, free_gid);
> +	SAFE_CHMOD(WORKDIR, MODE_SGID);
> +	SAFE_STAT(WORKDIR, &buf);
> +
> +	if (!(buf.st_mode & S_ISGID))
> +		tst_brk(TBROK, "%s: Setgid bit not set", WORKDIR);
> +
> +	if (buf.st_gid != free_gid) {
> +		tst_brk(TBROK, "%s: Incorrect group, %u != %u", WORKDIR,
> +			buf.st_gid, free_gid);
> +	}
> +
> +	/* Switch user */
> +	SAFE_SETGID(ltpuser->pw_gid);
> +	SAFE_SETREUID(-1, ltpuser->pw_uid);
> +}
> +
> +static void file_test(int dfd, const char *path, int flags)
> +{
> +	struct stat buf;
> +
> +	TST_EXP_PASS_SILENT(fstatat(dfd, path, &buf, flags));
> +	if (!TST_PASS) {
> +		tst_res(TFAIL, "fstat failed");
> +		return;
> +	}

If nothing else this part is really ugly, it's a misuse of the
TST_EXP_PASS_SILENT() macro and you even print the TFAIL message
manually for the second time.

This should really be replaced with SAFE_FSTATAT() after a patch that
adds SAFE_FSTATAT() to the test library.

> +	if (buf.st_gid != free_gid) {
> +		tst_res(TFAIL, "%s: Incorrect group, %u != %u", path,
> +			buf.st_gid, free_gid);
> +	} else {
> +		tst_res(TPASS, "%s: Owned by correct group", path);
> +	}

TST_EXP_EQ_LI(buf.st_gid, free_gid);

> +	if (buf.st_mode & S_ISGID)
> +		tst_res(TFAIL, "%s: Setgid bit is set", path);
> +	else
> +		tst_res(TPASS, "%s: Setgid bit not set", path);
> +
> +	if (buf.st_mode & S_IXGRP)
> +		tst_res(TFAIL, "%s: S_IXGRP bit is set", path);
> +	else
> +		tst_res(TPASS, "%s: S_IXGRP bit is not set", path);
> +}
> +
> +static void run(void)
> +{
> +	char path[PATH_MAX];
> +
> +	umask(S_IXGRP);
> +	tmpfile_fd = SAFE_OPENAT(dir_fd, ".", O_TMPFILE | O_RDWR, MODE_SGID);
> +	snprintf(path, PATH_MAX, "/proc/self/fd/%d", tmpfile_fd);
> +	SAFE_LINKAT(AT_FDCWD, path, dir_fd, OPEN_FILE, AT_SYMLINK_FOLLOW);
> +	file_test(dir_fd, OPEN_FILE, 0);
> +	SAFE_CLOSE(tmpfile_fd);
> +	/* Cleanup between loops */
> +	tst_purge_dir(WORKDIR);
> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_SETREUID(-1, 0);
> +
> +	if (tmpfile_fd >= 0)
> +		SAFE_CLOSE(tmpfile_fd);
> +	if (dir_fd >= 0)
> +		SAFE_CLOSE(dir_fd);
> +	if (mount_flag && tst_umount(MNTPOINT))
> +		tst_res(TWARN | TERRNO, "umount(%s)", MNTPOINT);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.all_filesystems = 1,
> +	.format_device = 1,
> +	.mntpoint = MNTPOINT,
> +	.skip_filesystems = (const char*[]) {
> +		"exfat",
> +		"ntfs",
> +		"vfat",
> +		NULL
> +	},
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "ac6800e279a2"},
> +		{}
> +	},
> +};
> -- 
> 2.23.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp