Re: [LTP] [PATCH v2] mknod02.c: Simplify and convert to new LTP API

[Petr Vorel <pvorel@suse.cz>]

Hi Avinesh,

> Simply test when parent directory does not have set-group-ID bit set,
> new node gets GID from effective GID of the process and does not inherit
> the group ownership from its parent directory.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Few comments below.

>  testcases/kernel/syscalls/mknod/mknod02.c | 316 +++-------------------
>  1 file changed, 36 insertions(+), 280 deletions(-)

...
> +/*\
> + * [Description]
>   *
> + * Verify that if mknod(2) creates a filesystem node in a directory which
> + * does not have the set-group-ID bit set, new node will not inherit the
> + * group ownership from its parent directory and its group ID will be the
> + * effective group ID of the process.

@Cyril I wonder if it'd be good to test this on all_filesystems. Are we trying
to use use all_filesystems = 1 when subject of testing is using VFS or the
opposite? (kernel docs mentions "VFS system calls open(2), stat(2), read(2),
write(2), chmod(2)". It also mentions locking [2]).

BTW looking what has mknod in vfs, it's just nfsd and 9p (none of them are used
in all_filesystems):

$ git grep  mknod $(git ls-files fs/|grep -i vfs)
fs/9p/vfs_inode.c: * for mknod(2).
fs/9p/vfs_inode.c: * v9fs_vfs_mknod - create a special file
fs/9p/vfs_inode.c:v9fs_vfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
fs/9p/vfs_inode.c:      .mknod = v9fs_vfs_mknod,
fs/9p/vfs_inode.c:      .mknod = v9fs_vfs_mknod,
fs/9p/vfs_inode_dotl.c:v9fs_vfs_mknod_dotl(struct mnt_idmap *idmap, struct inode *dir,
fs/9p/vfs_inode_dotl.c: return v9fs_vfs_mknod_dotl(idmap, dir, dentry, omode, 0);
fs/9p/vfs_inode_dotl.c: * v9fs_vfs_mknod_dotl - create a special file
fs/9p/vfs_inode_dotl.c:v9fs_vfs_mknod_dotl(struct mnt_idmap *idmap, struct inode *dir,
fs/9p/vfs_inode_dotl.c:         p9_debug(P9_DEBUG_VFS, "Failed to get acl values in mknod %d\n",
fs/9p/vfs_inode_dotl.c: err = p9_client_mknod_dotl(dfid, name, mode, rdev, gid, &qid);
fs/9p/vfs_inode_dotl.c: .mknod = v9fs_vfs_mknod_dotl,
fs/nfsd/vfs.c:          host_err = vfs_mknod(&nop_mnt_idmap, dirp, dchild,

>   */

...
> +static void run(void)
>  {
...
> +	SAFE_CHDIR(TEMP_DIR);
> +	TST_EXP_PASS(mknod(TEMP_NODE, MODE1, 0), "mknod(%s, %o, 0)", TEMP_NODE, MODE1);

IMHO this simple form will print the same info, I suggest to use it.
	TST_EXP_PASS(mknod(TEMP_NODE, MODE1, 0));

> -	/*
> -	 * Create a test directory under temporary directory with the
> -	 * specified mode permissions, with uid/gid set to that of guest
> -	 * user and the test process.
> -	 */
> -	SAFE_MKDIR(cleanup, DIR_TEMP, MODE_RWX);
> -	SAFE_CHOWN(cleanup, DIR_TEMP, user1_uid, group2_gid);
> +	SAFE_STAT(TEMP_NODE, &buf);
> +	TST_EXP_EQ_LI(buf.st_gid, 0);

...

Diff for using all_filesystems (I'm not sure myself).

Kind regards,
Petr

[1] https://www.kernel.org/doc/html/next/filesystems/vfs.html
[2] https://www.kernel.org/doc/html/next/filesystems/locking.html

+++ testcases/kernel/syscalls/mknod/mknod02.c
@@ -24,6 +24,8 @@
 #define TEMP_DIR "testdir"
 #define TEMP_NODE "testnode"
 
+#define MNTPOINT	"mntpoint"
+
 static struct stat buf;
 static struct passwd *user_nobody;
 static gid_t gid_nobody;
@@ -40,18 +42,28 @@ static void setup(void)
 static void run(void)
 {
 	SAFE_CHDIR(TEMP_DIR);
-	TST_EXP_PASS(mknod(TEMP_NODE, MODE1, 0), "mknod(%s, %o, 0)", TEMP_NODE, MODE1);
+	TST_EXP_PASS(mknod(TEMP_NODE, MODE1, 0));
 
 	SAFE_STAT(TEMP_NODE, &buf);
 	TST_EXP_EQ_LI(buf.st_gid, 0);
 
 	SAFE_UNLINK(TEMP_NODE);
 	SAFE_CHDIR("..");
+
+}
+
+static void cleanup(void)
+{
+	SAFE_RMDIR(TEMP_DIR);
 }
 
 static struct tst_test test = {
 	.setup = setup,
+	.cleanup = cleanup,
 	.test_all = run,
 	.needs_root = 1,
-	.needs_tmpdir = 1
+	.needs_tmpdir = 1,
+	.mount_device = 1,
+	.mntpoint = MNTPOINT,
+	.all_filesystems = 1,
 };

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