[LTP] [PATCH 1/1] mknod02.c: Convert to new LTP API

[Avinesh Kumar]

Hi Cyril,

On Wednesday, March 15, 2023 8:09:34 PM IST you wrote:
> Cyril's review of this patch which was send to another thread - https://lore.kernel.org/ltp/ZAEBOhm3nAExVfT2@yuki/#t
> 
> ========
> Re: [LTP] [PATCH v2 1/1] mknod01: Rewrite the test using new LTP API
> 
> From:	Cyril Hrubis <chrubis@suse.cz>
> To:	Avinesh Kumar <akumar@suse.de>
> CC:	ltp@lists.linux.it
> Date:	3/3/23 1:34 AM
> Hi!
> > --- a/testcases/kernel/syscalls/mknod/mknod02.c
> > +++ b/testcases/kernel/syscalls/mknod/mknod02.c
> > @@ -1,301 +1,77 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > /*
> > - *
> > *   Copyright (c) International Business Machines  Corp., 2001
> > - *
> > - *   This program is free software;  you can redistribute it and/or modify
> > - *   it under the terms of the GNU General Public License as published by
> > - *   the Free Software Foundation; either version 2 of the License, or
> > - *   (at your option) any later version.
> > - *
> > - *   This program is distributed in the hope that it will be useful,
> > - *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
> > - *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> > - *   the GNU General Public License for more details.
> > - *
> > - *   You should have received a copy of the GNU General Public License
> > - *   along with this program;  if not, write to the Free Software
> > - *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> > + *     07/2001 Ported by Wayne Boyer
> > + *   Copyright (c) 2023 SUSE LLC Avinesh Kumar <avinesh.kumar@suse.com>
> > */
> >
> > -/*
> > - * Test Name: mknod02
> > +/*\
> > + * [Description]
> > *
> > - * Test Description:
> > *  Verify that mknod(2) succeeds when used to create a filesystem
> > *  node with set group-ID bit set on a directory without set group-ID bit set.
> > *  The node created should have set group-ID bit set and its gid should be
> > *  equal to that of its parent directory.
> 
> I've read this a few times and I stil do not understand what exactly is
> this test supposed to test or at least the code does not correspond to
> the description.
> 
> I did look at the kernel sources for a while and as far as I can tell
> the whole S_ISGID handling is done in the inode_init_owner() function:
> 
> void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>                       const struct inode *dir, umode_t mode)
> {
>         inode_fsuid_set(inode, mnt_userns);
>         if (dir && dir->i_mode & S_ISGID) {
>                 inode->i_gid = dir->i_gid;
> 
>                 /* Directories are special, and always inherit S_ISGID */
>                 if (S_ISDIR(mode))
>                         mode |= S_ISGID;
>         } else
>                 inode_fsgid_set(inode, mnt_userns);
>         inode->i_mode = mode;
> }
> EXPORT_SYMBOL(inode_init_owner);
> 
> Which looks pretty straightforward.
> 
> - If the parent direcotry has S_ISGID we set the inode GID to the
>   parent directory gid
> - If not we call the inode_fsgid_set(), which does namepsace mappings,
>   but eventually we end up whaterver the current running process GID
>   maps to
> 
> So the test is awfully complex for what it's supposed to do, which is
> without S_ISGID on parent directory the FIFO gid is set to the running
> process GID. As far as I can tell all that needs to be done here is:
> 
> With .require_root:
> 
> 1. Create a directory and set the directory GID to nobody
> 2. Create a FIFO inside the directory and check that the
>    FIFO GID matches the root gid
Yes this seems simple and enough, implementing this in the v2
of this patch.

> 
> And that should be it. As far as I can tell the S_ISGID on FIFO is
> pointless in this case since for files the S_ISGID means that the GID is
> changed when they are executed, which we do not attempt here and I'm not
> even sure if we can execute FIFO.
Yes, agree.
`man 2 mknod` doesn't say anything about the S_ISGID of the new node being
created, so in which cases new node should have S_ISGID bit set? 
IIUC, in the above kernel function inode_init_owner(), it is being set only if
new node is a directory?

(looking at mknod03|04|05|08, these tests somehow trying to  verify the 
S_ISGID bit of the newly created node)

> 
> And the same should be done by the mknod03 with the difference that th
> S_ISGID should be set on the directory and we should expect the GID to
> be inherited.
> 
> 

Thank you,
Avinesh