Re: [PATCH v2] nfs: use vfs setgid helper

[Christian Brauner <brauner@kernel.org>]

On Tue, Apr 11, 2023 at 12:54:25PM -0400, Anna Schumaker wrote:
> Hi Christian,
> 
> On Tue, Mar 14, 2023 at 7:51 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > 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>
> > ---
> > Changes in v2:
> > - Christoph Hellwig <hch@lst.de>:
> >   * Export setattr_should_sgid() so it actually can be used by filesystems
> > - Link to v1: https://lore.kernel.org/r/20230313-fs-nfs-setgid-v1-1-5b1fa599f186@kernel.org
> > ---
> >  fs/attr.c          | 1 +
> >  fs/internal.h      | 2 --
> >  fs/nfs/inode.c     | 4 +---
> >  include/linux/fs.h | 2 ++
> >  4 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/attr.c b/fs/attr.c
> > index aca9ff7aed33..d60dc1edb526 100644
> > --- a/fs/attr.c
> > +++ b/fs/attr.c
> > @@ -47,6 +47,7 @@ int setattr_should_drop_sgid(struct mnt_idmap *idmap,
> >                 return ATTR_KILL_SGID;
> >         return 0;
> >  }
> > +EXPORT_SYMBOL(setattr_should_drop_sgid);
> >
> >  /**
> >   * setattr_should_drop_suidgid - determine whether the set{g,u}id bit needs to
> > diff --git a/fs/internal.h b/fs/internal.h
> > index dc4eb91a577a..ab36ed8fa41c 100644
> > --- a/fs/internal.h
> > +++ b/fs/internal.h
> > @@ -259,8 +259,6 @@ ssize_t __kernel_write_iter(struct file *file, struct iov_iter *from, loff_t *po
> >  /*
> >   * fs/attr.c
> >   */
> > -int setattr_should_drop_sgid(struct mnt_idmap *idmap,
> > -                            const struct inode *inode);
> >  struct mnt_idmap *alloc_mnt_idmap(struct user_namespace *mnt_userns);
> >  struct mnt_idmap *mnt_idmap_get(struct mnt_idmap *idmap);
> >  void mnt_idmap_put(struct mnt_idmap *idmap);
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index 222a28320e1c..97a76706fd54 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -717,9 +717,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;
> 
> Will this be going through your tree (due to the VFS leve changes)?
> If so, you can add:
> 
> Acked-by: Anna Schumaker <anna.schumaker@netapp.com>

Yes, I have it applied to fs.misc. There should be a corresponding
"applied message" I've sent last week. Thanks for taking a look!

Christian