[PATCH] nfs: use vfs setgid helper

Christian Brauner posted 1 patch 2 years, 11 months ago
fs/nfs/inode.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
[PATCH] nfs: use vfs setgid helper
Posted by Christian Brauner 2 years, 11 months ago
We've aligned setgid behavior over multiple kernel releases. The details
can be found in the following two merge messages:
cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2')
426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0')
Consistent setgid stripping behavior is now encapsulated in the
setattr_should_drop_sgid() helper which is used by all filesystems that
strip setgid bits outside of vfs proper. Switch nfs to rely on this
helper as well. Without this patch the setgid stripping tests in
xfstests will fail.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 fs/nfs/inode.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 222a28320e1c..5001086500b3 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -42,6 +42,7 @@
 #include <linux/uaccess.h>
 #include <linux/iversion.h>
 
+#include "../internal.h"
 #include "nfs4_fs.h"
 #include "callback.h"
 #include "delegation.h"
@@ -717,9 +718,7 @@ void nfs_setattr_update_inode(struct inode *inode, struct iattr *attr,
 		if ((attr->ia_valid & ATTR_KILL_SUID) != 0 &&
 		    inode->i_mode & S_ISUID)
 			inode->i_mode &= ~S_ISUID;
-		if ((attr->ia_valid & ATTR_KILL_SGID) != 0 &&
-		    (inode->i_mode & (S_ISGID | S_IXGRP)) ==
-		     (S_ISGID | S_IXGRP))
+		if (setattr_should_drop_sgid(&nop_mnt_idmap, inode))
 			inode->i_mode &= ~S_ISGID;
 		if ((attr->ia_valid & ATTR_MODE) != 0) {
 			int mode = attr->ia_mode & S_IALLUGO;

---
base-commit: eeac8ede17557680855031c6f305ece2378af326
change-id: 20230313-fs-nfs-setgid-659410a10b25
Re: [PATCH] nfs: use vfs setgid helper
Posted by kernel test robot 2 years, 11 months ago
Hi Christian,

I love your patch! Yet something to improve:

[auto build test ERROR on eeac8ede17557680855031c6f305ece2378af326]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Brauner/nfs-use-vfs-setgid-helper/20230313-212725
base:   eeac8ede17557680855031c6f305ece2378af326
patch link:    https://lore.kernel.org/r/20230313-fs-nfs-setgid-v1-1-5b1fa599f186%40kernel.org
patch subject: [PATCH] nfs: use vfs setgid helper
config: parisc64-defconfig (https://download.01.org/0day-ci/archive/20230314/202303140652.dN7XrtM4-lkp@intel.com/config)
compiler: hppa64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/503d040be490a519b2e483672702dcca530443ce
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christian-Brauner/nfs-use-vfs-setgid-helper/20230313-212725
        git checkout 503d040be490a519b2e483672702dcca530443ce
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303140652.dN7XrtM4-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfs/nfs.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Re: [PATCH] nfs: use vfs setgid helper
Posted by Christoph Hellwig 2 years, 11 months ago
On Mon, Mar 13, 2023 at 02:25:34PM +0100, Christian Brauner wrote:
> +#include "../internal.h"

> +		if (setattr_should_drop_sgid(&nop_mnt_idmap, inode))

It setattr_should_drop_sgid is used by file systems, it should not be in
internal.h.
Re: [PATCH] nfs: use vfs setgid helper
Posted by Christian Brauner 2 years, 11 months ago
On Mon, Mar 13, 2023 at 08:23:49AM -0700, Christoph Hellwig wrote:
> On Mon, Mar 13, 2023 at 02:25:34PM +0100, Christian Brauner wrote:
> > +#include "../internal.h"
> 
> > +		if (setattr_should_drop_sgid(&nop_mnt_idmap, inode))
> 
> It setattr_should_drop_sgid is used by file systems, it should not be in
> internal.h.

Good catch. I accidently didn't move it into include/linux/fs.h
with setattr_should_drop_suidgid(). Let me resend. Thanks for
catching this...