* Re: [PATCH v2] nfs: use vfs setgid helper
2023-03-14 11:51 [PATCH v2] nfs: use vfs setgid helper Christian Brauner
@ 2023-03-14 12:37 ` Matthew Wilcox
2023-03-14 13:08 ` Christian Brauner
2023-03-28 8:30 ` Christian Brauner
` (4 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2023-03-14 12:37 UTC (permalink / raw)
To: Christian Brauner
Cc: Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
Christoph Hellwig, linux-fsdevel, linux-nfs
On Tue, Mar 14, 2023 at 12:51:10PM +0100, Christian Brauner wrote:
> +++ b/include/linux/fs.h
> @@ -2675,6 +2675,8 @@ extern struct inode *new_inode(struct super_block *sb);
> extern void free_inode_nonrcu(struct inode *inode);
> extern int setattr_should_drop_suidgid(struct mnt_idmap *, struct inode *);
> extern int file_remove_privs(struct file *);
> +int setattr_should_drop_sgid(struct mnt_idmap *idmap,
> + const struct inode *inode);
Is there a reason that setattr_should_drop_sgid() doesn't take a
const struct mnt_idmap pointer?
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] nfs: use vfs setgid helper
2023-03-14 12:37 ` Matthew Wilcox
@ 2023-03-14 13:08 ` Christian Brauner
0 siblings, 0 replies; 9+ messages in thread
From: Christian Brauner @ 2023-03-14 13:08 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
Christoph Hellwig, linux-fsdevel, linux-nfs
On Tue, Mar 14, 2023 at 12:37:03PM +0000, Matthew Wilcox wrote:
> On Tue, Mar 14, 2023 at 12:51:10PM +0100, Christian Brauner wrote:
> > +++ b/include/linux/fs.h
> > @@ -2675,6 +2675,8 @@ extern struct inode *new_inode(struct super_block *sb);
> > extern void free_inode_nonrcu(struct inode *inode);
> > extern int setattr_should_drop_suidgid(struct mnt_idmap *, struct inode *);
> > extern int file_remove_privs(struct file *);
> > +int setattr_should_drop_sgid(struct mnt_idmap *idmap,
> > + const struct inode *inode);
>
> Is there a reason that setattr_should_drop_sgid() doesn't take a
> const struct mnt_idmap pointer?
Yeah, unfortunately a bunch of helpers that we're passing it down to
from setattr_should_drop_sgid() currently take a non-const pointer to
mnt_idmap. I have a ToDo that to go through all helpers and make them
const wherever we can.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] nfs: use vfs setgid helper
2023-03-14 11:51 [PATCH v2] nfs: use vfs setgid helper Christian Brauner
2023-03-14 12:37 ` Matthew Wilcox
@ 2023-03-28 8:30 ` Christian Brauner
2023-03-30 5:45 ` Christoph Hellwig
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Christian Brauner @ 2023-03-28 8:30 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton
Cc: Christoph Hellwig, linux-fsdevel, linux-nfs
On Tue, Mar 14, 2023 at 12:51:10PM +0100, Christian Brauner 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
> ---
If someone has a few cycles to give this a review it would be
appreciated. I'm happy to carry this patch or the NFS tree can.
I'm not fussed.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] nfs: use vfs setgid helper
2023-03-14 11:51 [PATCH v2] nfs: use vfs setgid helper Christian Brauner
2023-03-14 12:37 ` Matthew Wilcox
2023-03-28 8:30 ` Christian Brauner
@ 2023-03-30 5:45 ` Christoph Hellwig
2023-03-30 6:57 ` Christian Brauner
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2023-03-30 5:45 UTC (permalink / raw)
To: Christian Brauner
Cc: Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
Christoph Hellwig, linux-fsdevel, linux-nfs
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] nfs: use vfs setgid helper
2023-03-14 11:51 [PATCH v2] nfs: use vfs setgid helper Christian Brauner
` (2 preceding siblings ...)
2023-03-30 5:45 ` Christoph Hellwig
@ 2023-03-30 6:57 ` Christian Brauner
2023-04-11 16:54 ` Anna Schumaker
2023-04-11 17:10 ` Jeff Layton
5 siblings, 0 replies; 9+ messages in thread
From: Christian Brauner @ 2023-03-30 6:57 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
Christian Brauner
Cc: Christoph Hellwig, linux-fsdevel, linux-nfs
On Tue, 14 Mar 2023 12:51:10 +0100, Christian Brauner 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.
>
> [...]
So I've picked this up now,
tree: git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git
branch: fs.misc
[1/1] nfs: use vfs setgid helper
commit: 4f704d9a8352f5c0a8fcdb6213b934630342bd44
Thanks!
Christian
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] nfs: use vfs setgid helper
2023-03-14 11:51 [PATCH v2] nfs: use vfs setgid helper Christian Brauner
` (3 preceding siblings ...)
2023-03-30 6:57 ` Christian Brauner
@ 2023-04-11 16:54 ` Anna Schumaker
2023-04-12 8:58 ` Christian Brauner
2023-04-11 17:10 ` Jeff Layton
5 siblings, 1 reply; 9+ messages in thread
From: Anna Schumaker @ 2023-04-11 16:54 UTC (permalink / raw)
To: Christian Brauner
Cc: Trond Myklebust, Chuck Lever, Jeff Layton, Christoph Hellwig,
linux-fsdevel, linux-nfs
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>
for the NFS bits.
Thanks,
Anna
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c85916e9f7db..af95b64fc810 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2675,6 +2675,8 @@ extern struct inode *new_inode(struct super_block *sb);
> extern void free_inode_nonrcu(struct inode *inode);
> extern int setattr_should_drop_suidgid(struct mnt_idmap *, struct inode *);
> extern int file_remove_privs(struct file *);
> +int setattr_should_drop_sgid(struct mnt_idmap *idmap,
> + const struct inode *inode);
>
> /*
> * This must be used for allocating filesystems specific inodes to set
>
> ---
> base-commit: eeac8ede17557680855031c6f305ece2378af326
> change-id: 20230313-fs-nfs-setgid-659410a10b25
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] nfs: use vfs setgid helper
2023-04-11 16:54 ` Anna Schumaker
@ 2023-04-12 8:58 ` Christian Brauner
0 siblings, 0 replies; 9+ messages in thread
From: Christian Brauner @ 2023-04-12 8:58 UTC (permalink / raw)
To: Anna Schumaker
Cc: Trond Myklebust, Chuck Lever, Jeff Layton, Christoph Hellwig,
linux-fsdevel, linux-nfs
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] nfs: use vfs setgid helper
2023-03-14 11:51 [PATCH v2] nfs: use vfs setgid helper Christian Brauner
` (4 preceding siblings ...)
2023-04-11 16:54 ` Anna Schumaker
@ 2023-04-11 17:10 ` Jeff Layton
5 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2023-04-11 17:10 UTC (permalink / raw)
To: Christian Brauner, Trond Myklebust, Anna Schumaker, Chuck Lever
Cc: Christoph Hellwig, linux-fsdevel, linux-nfs
On Tue, 2023-03-14 at 12:51 +0100, Christian Brauner 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;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c85916e9f7db..af95b64fc810 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2675,6 +2675,8 @@ extern struct inode *new_inode(struct super_block *sb);
> extern void free_inode_nonrcu(struct inode *inode);
> extern int setattr_should_drop_suidgid(struct mnt_idmap *, struct inode *);
> extern int file_remove_privs(struct file *);
> +int setattr_should_drop_sgid(struct mnt_idmap *idmap,
> + const struct inode *inode);
>
> /*
> * This must be used for allocating filesystems specific inodes to set
>
> ---
> base-commit: eeac8ede17557680855031c6f305ece2378af326
> change-id: 20230313-fs-nfs-setgid-659410a10b25
>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread