Re: [LTP] [PATCH v2 1/1] mknod01: Rewrite the test using new LTP API

[Cyril Hrubis <chrubis@suse.cz>]

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

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.

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.

-- 
Cyril Hrubis
chrubis@suse.cz

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