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

[Avinesh Kumar <akumar@suse.de>]

Hi Petr,

On Tuesday, May 16, 2023 5:28:41 PM IST Petr Vorel wrote:
> 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

Thanks for reviewing, I agree with your proposed changes if we are going to 
enable if for all filesystems, test executes fine with all_filesystems=1 in my 
setup.

> 
> [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,
>  };

Regards,
Avinesh



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