* [PATCH 0/3] fuse: Fix clearing SGID when access ACL is set
@ 2021-03-19 19:55 Vivek Goyal
2021-03-19 19:55 ` [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared Vivek Goyal
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Vivek Goyal @ 2021-03-19 19:55 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel, virtio-fs, miklos
Cc: vgoyal, lhenriques, dgilbert, seth.forshee
Hi,
Luis reported that xfstests generic/375 fails with virtiofs. Little
debugging showed that when posix access acl is set that in some
cases SGID needs to be cleared and that does not happen with virtiofs.
Setting posix access acl can lead to mode change and it can also lead
to clear of SGID. fuse relies on file server taking care of all
the mode changes. But file server does not have enough information to
determine whether SGID should be cleared or not.
Hence this patch series add support to send a flag in SETXATTR message
to tell server to clear SGID.
I have staged corresponding virtiofsd patches here.
https://github.com/rhvgoyal/qemu/commits/acl-sgid-setxattr-flag
With these patches applied "./check -g acl" passes now on virtiofs.
Vivek Goyal (3):
posic_acl: Add a helper determine if SGID should be cleared
fuse: Add support for FUSE_SETXATTR_V2
fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID
fs/fuse/acl.c | 7 ++++++-
fs/fuse/fuse_i.h | 5 ++++-
fs/fuse/inode.c | 4 +++-
fs/fuse/xattr.c | 21 +++++++++++++++------
fs/posix_acl.c | 3 +--
include/linux/posix_acl.h | 11 +++++++++++
include/uapi/linux/fuse.h | 17 +++++++++++++++++
7 files changed, 57 insertions(+), 11 deletions(-)
--
2.25.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared 2021-03-19 19:55 [PATCH 0/3] fuse: Fix clearing SGID when access ACL is set Vivek Goyal @ 2021-03-19 19:55 ` Vivek Goyal 2021-03-19 22:42 ` Andreas Grünbacher 2021-03-19 19:55 ` [PATCH 2/3] fuse: Add support for FUSE_SETXATTR_V2 Vivek Goyal 2021-03-19 19:55 ` [PATCH 3/3] fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID Vivek Goyal 2 siblings, 1 reply; 9+ messages in thread From: Vivek Goyal @ 2021-03-19 19:55 UTC (permalink / raw) To: linux-fsdevel, linux-kernel, virtio-fs, miklos Cc: vgoyal, lhenriques, dgilbert, seth.forshee, Jan Kara, Andreas Gruenbacher, Alexander Viro posix_acl_update_mode() determines what's the equivalent mode and if SGID needs to be cleared or not. I need to make use of this code in fuse as well. Fuse will send this information to virtiofs file server and file server will take care of clearing SGID if it needs to be done. Hence move this code in a separate helper so that more than one place can call into it. Cc: Jan Kara <jack@suse.cz> Cc: Andreas Gruenbacher <agruenba@redhat.com> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/posix_acl.c | 3 +-- include/linux/posix_acl.h | 11 +++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/fs/posix_acl.c b/fs/posix_acl.c index f3309a7edb49..2d62494c4a5b 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -684,8 +684,7 @@ int posix_acl_update_mode(struct user_namespace *mnt_userns, return error; if (error == 0) *acl = NULL; - if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && - !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) + if (posix_acl_mode_clear_sgid(mnt_userns, inode)) mode &= ~S_ISGID; *mode_p = mode; return 0; diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h index 307094ebb88c..073c5e546de3 100644 --- a/include/linux/posix_acl.h +++ b/include/linux/posix_acl.h @@ -59,6 +59,17 @@ posix_acl_release(struct posix_acl *acl) } +static inline bool +posix_acl_mode_clear_sgid(struct user_namespace *mnt_userns, + struct inode *inode) +{ + if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && + !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) + return true; + + return false; +} + /* posix_acl.c */ extern void posix_acl_init(struct posix_acl *, int); -- 2.25.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared 2021-03-19 19:55 ` [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared Vivek Goyal @ 2021-03-19 22:42 ` Andreas Grünbacher 2021-03-20 10:03 ` Christian Brauner 0 siblings, 1 reply; 9+ messages in thread From: Andreas Grünbacher @ 2021-03-19 22:42 UTC (permalink / raw) To: Vivek Goyal Cc: Linux FS-devel Mailing List, Linux Kernel Mailing List, virtio-fs, Miklos Szeredi, lhenriques, dgilbert, Seth Forshee, Jan Kara, Andreas Gruenbacher, Alexander Viro Hi, Am Fr., 19. März 2021 um 20:58 Uhr schrieb Vivek Goyal <vgoyal@redhat.com>: > posix_acl_update_mode() determines what's the equivalent mode and if SGID > needs to be cleared or not. I need to make use of this code in fuse > as well. Fuse will send this information to virtiofs file server and > file server will take care of clearing SGID if it needs to be done. > > Hence move this code in a separate helper so that more than one place > can call into it. > > Cc: Jan Kara <jack@suse.cz> > Cc: Andreas Gruenbacher <agruenba@redhat.com> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > fs/posix_acl.c | 3 +-- > include/linux/posix_acl.h | 11 +++++++++++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index f3309a7edb49..2d62494c4a5b 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -684,8 +684,7 @@ int posix_acl_update_mode(struct user_namespace *mnt_userns, > return error; > if (error == 0) > *acl = NULL; > - if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > - !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > + if (posix_acl_mode_clear_sgid(mnt_userns, inode)) > mode &= ~S_ISGID; > *mode_p = mode; > return 0; > diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h > index 307094ebb88c..073c5e546de3 100644 > --- a/include/linux/posix_acl.h > +++ b/include/linux/posix_acl.h > @@ -59,6 +59,17 @@ posix_acl_release(struct posix_acl *acl) > } > > > +static inline bool > +posix_acl_mode_clear_sgid(struct user_namespace *mnt_userns, > + struct inode *inode) > +{ > + if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > + !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > + return true; > + > + return false; That's just return !in_group_p(i_gid_into_mnt(mnt_userns, inode)) && !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID); The same pattern we have in posix_acl_update_mode also exists in setattr_copy and inode_init_owner, and almost the same pattern exists in setattr_prepare, so can this be cleaned up as well? The function also isn't POSIX ACL specific, so the function name is misleading. > +} > + > /* posix_acl.c */ > > extern void posix_acl_init(struct posix_acl *, int); > -- > 2.25.4 Thanks, Andreas ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared 2021-03-19 22:42 ` Andreas Grünbacher @ 2021-03-20 10:03 ` Christian Brauner 2021-03-22 17:01 ` Vivek Goyal 0 siblings, 1 reply; 9+ messages in thread From: Christian Brauner @ 2021-03-20 10:03 UTC (permalink / raw) To: Andreas Grünbacher Cc: Vivek Goyal, Linux FS-devel Mailing List, Linux Kernel Mailing List, virtio-fs, Miklos Szeredi, lhenriques, dgilbert, Seth Forshee, Jan Kara, Andreas Gruenbacher, Alexander Viro On Fri, Mar 19, 2021 at 11:42:48PM +0100, Andreas Grünbacher wrote: > Hi, > > Am Fr., 19. März 2021 um 20:58 Uhr schrieb Vivek Goyal <vgoyal@redhat.com>: > > posix_acl_update_mode() determines what's the equivalent mode and if SGID > > needs to be cleared or not. I need to make use of this code in fuse > > as well. Fuse will send this information to virtiofs file server and > > file server will take care of clearing SGID if it needs to be done. > > > > Hence move this code in a separate helper so that more than one place > > can call into it. > > > > Cc: Jan Kara <jack@suse.cz> > > Cc: Andreas Gruenbacher <agruenba@redhat.com> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > --- > > fs/posix_acl.c | 3 +-- > > include/linux/posix_acl.h | 11 +++++++++++ > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > > index f3309a7edb49..2d62494c4a5b 100644 > > --- a/fs/posix_acl.c > > +++ b/fs/posix_acl.c > > @@ -684,8 +684,7 @@ int posix_acl_update_mode(struct user_namespace *mnt_userns, > > return error; > > if (error == 0) > > *acl = NULL; > > - if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > - !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > > + if (posix_acl_mode_clear_sgid(mnt_userns, inode)) > > mode &= ~S_ISGID; > > *mode_p = mode; > > return 0; > > diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h > > index 307094ebb88c..073c5e546de3 100644 > > --- a/include/linux/posix_acl.h > > +++ b/include/linux/posix_acl.h > > @@ -59,6 +59,17 @@ posix_acl_release(struct posix_acl *acl) > > } > > > > > > +static inline bool > > +posix_acl_mode_clear_sgid(struct user_namespace *mnt_userns, > > + struct inode *inode) > > +{ > > + if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > + !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > > + return true; > > + > > + return false; > > That's just > > return !in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID); > > The same pattern we have in posix_acl_update_mode also exists in > setattr_copy and inode_init_owner, and almost the same pattern exists > in setattr_prepare, so can this be cleaned up as well? The function > also isn't POSIX ACL specific, so the function name is misleading. Good idea but that should probably be spun into a separate patchset that only touches the vfs parts. Christian ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared 2021-03-20 10:03 ` Christian Brauner @ 2021-03-22 17:01 ` Vivek Goyal 2021-03-23 9:32 ` Christian Brauner 0 siblings, 1 reply; 9+ messages in thread From: Vivek Goyal @ 2021-03-22 17:01 UTC (permalink / raw) To: Christian Brauner Cc: Andreas Grünbacher, Linux FS-devel Mailing List, Linux Kernel Mailing List, virtio-fs, Miklos Szeredi, lhenriques, dgilbert, Seth Forshee, Jan Kara, Andreas Gruenbacher, Alexander Viro On Sat, Mar 20, 2021 at 11:03:22AM +0100, Christian Brauner wrote: > On Fri, Mar 19, 2021 at 11:42:48PM +0100, Andreas Grünbacher wrote: > > Hi, > > > > Am Fr., 19. März 2021 um 20:58 Uhr schrieb Vivek Goyal <vgoyal@redhat.com>: > > > posix_acl_update_mode() determines what's the equivalent mode and if SGID > > > needs to be cleared or not. I need to make use of this code in fuse > > > as well. Fuse will send this information to virtiofs file server and > > > file server will take care of clearing SGID if it needs to be done. > > > > > > Hence move this code in a separate helper so that more than one place > > > can call into it. > > > > > > Cc: Jan Kara <jack@suse.cz> > > > Cc: Andreas Gruenbacher <agruenba@redhat.com> > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > --- > > > fs/posix_acl.c | 3 +-- > > > include/linux/posix_acl.h | 11 +++++++++++ > > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > > > index f3309a7edb49..2d62494c4a5b 100644 > > > --- a/fs/posix_acl.c > > > +++ b/fs/posix_acl.c > > > @@ -684,8 +684,7 @@ int posix_acl_update_mode(struct user_namespace *mnt_userns, > > > return error; > > > if (error == 0) > > > *acl = NULL; > > > - if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > > - !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > > > + if (posix_acl_mode_clear_sgid(mnt_userns, inode)) > > > mode &= ~S_ISGID; > > > *mode_p = mode; > > > return 0; > > > diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h > > > index 307094ebb88c..073c5e546de3 100644 > > > --- a/include/linux/posix_acl.h > > > +++ b/include/linux/posix_acl.h > > > @@ -59,6 +59,17 @@ posix_acl_release(struct posix_acl *acl) > > > } > > > > > > > > > +static inline bool > > > +posix_acl_mode_clear_sgid(struct user_namespace *mnt_userns, > > > + struct inode *inode) > > > +{ > > > + if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > > + !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > > > + return true; > > > + > > > + return false; > > > > That's just > > > > return !in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID); > > > > The same pattern we have in posix_acl_update_mode also exists in > > setattr_copy and inode_init_owner, and almost the same pattern exists > > in setattr_prepare, so can this be cleaned up as well? The function > > also isn't POSIX ACL specific, so the function name is misleading. > > Good idea but that should probably be spun into a separate patchset that > only touches the vfs parts. IIUC, suggestion is that I should write a VFS helper (and not posix acl helper) and use that helper at other places too in the code. I will do that and post in a separate patch series. Thanks Vivek ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared 2021-03-22 17:01 ` Vivek Goyal @ 2021-03-23 9:32 ` Christian Brauner 2021-03-23 22:50 ` Vivek Goyal 0 siblings, 1 reply; 9+ messages in thread From: Christian Brauner @ 2021-03-23 9:32 UTC (permalink / raw) To: Vivek Goyal Cc: Christian Brauner, Andreas Grünbacher, Linux FS-devel Mailing List, Linux Kernel Mailing List, virtio-fs, Miklos Szeredi, lhenriques, dgilbert, Seth Forshee, Jan Kara, Andreas Gruenbacher, Alexander Viro On Mon, Mar 22, 2021 at 01:01:11PM -0400, Vivek Goyal wrote: > On Sat, Mar 20, 2021 at 11:03:22AM +0100, Christian Brauner wrote: > > On Fri, Mar 19, 2021 at 11:42:48PM +0100, Andreas Grünbacher wrote: > > > Hi, > > > > > > Am Fr., 19. März 2021 um 20:58 Uhr schrieb Vivek Goyal <vgoyal@redhat.com>: > > > > posix_acl_update_mode() determines what's the equivalent mode and if SGID > > > > needs to be cleared or not. I need to make use of this code in fuse > > > > as well. Fuse will send this information to virtiofs file server and > > > > file server will take care of clearing SGID if it needs to be done. > > > > > > > > Hence move this code in a separate helper so that more than one place > > > > can call into it. > > > > > > > > Cc: Jan Kara <jack@suse.cz> > > > > Cc: Andreas Gruenbacher <agruenba@redhat.com> > > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > > --- > > > > fs/posix_acl.c | 3 +-- > > > > include/linux/posix_acl.h | 11 +++++++++++ > > > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > > > > index f3309a7edb49..2d62494c4a5b 100644 > > > > --- a/fs/posix_acl.c > > > > +++ b/fs/posix_acl.c > > > > @@ -684,8 +684,7 @@ int posix_acl_update_mode(struct user_namespace *mnt_userns, > > > > return error; > > > > if (error == 0) > > > > *acl = NULL; > > > > - if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > > > - !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > > > > + if (posix_acl_mode_clear_sgid(mnt_userns, inode)) > > > > mode &= ~S_ISGID; > > > > *mode_p = mode; > > > > return 0; > > > > diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h > > > > index 307094ebb88c..073c5e546de3 100644 > > > > --- a/include/linux/posix_acl.h > > > > +++ b/include/linux/posix_acl.h > > > > @@ -59,6 +59,17 @@ posix_acl_release(struct posix_acl *acl) > > > > } > > > > > > > > > > > > +static inline bool > > > > +posix_acl_mode_clear_sgid(struct user_namespace *mnt_userns, > > > > + struct inode *inode) > > > > +{ > > > > + if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > > > + !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > > > > + return true; > > > > + > > > > + return false; > > > > > > That's just > > > > > > return !in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > > !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID); > > > > > > The same pattern we have in posix_acl_update_mode also exists in > > > setattr_copy and inode_init_owner, and almost the same pattern exists > > > in setattr_prepare, so can this be cleaned up as well? The function > > > also isn't POSIX ACL specific, so the function name is misleading. > > > > Good idea but that should probably be spun into a separate patchset that > > only touches the vfs parts. > > IIUC, suggestion is that I should write a VFS helper (and not posix > acl helper) and use that helper at other places too in the code. If there are other callers outside of acls (which should be iirc) then yes. > > I will do that and post in a separate patch series. Yeah, I think that makes more sense to have this be a separate change instead of putting it together with the fuse change if it touches more than one place. Thanks! Christian ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared 2021-03-23 9:32 ` Christian Brauner @ 2021-03-23 22:50 ` Vivek Goyal 0 siblings, 0 replies; 9+ messages in thread From: Vivek Goyal @ 2021-03-23 22:50 UTC (permalink / raw) To: Christian Brauner Cc: Christian Brauner, Andreas Grünbacher, Linux FS-devel Mailing List, Linux Kernel Mailing List, virtio-fs, Miklos Szeredi, lhenriques, dgilbert, Seth Forshee, Jan Kara, Andreas Gruenbacher, Alexander Viro On Tue, Mar 23, 2021 at 10:32:33AM +0100, Christian Brauner wrote: > On Mon, Mar 22, 2021 at 01:01:11PM -0400, Vivek Goyal wrote: > > On Sat, Mar 20, 2021 at 11:03:22AM +0100, Christian Brauner wrote: > > > On Fri, Mar 19, 2021 at 11:42:48PM +0100, Andreas Grünbacher wrote: > > > > Hi, > > > > > > > > Am Fr., 19. März 2021 um 20:58 Uhr schrieb Vivek Goyal <vgoyal@redhat.com>: > > > > > posix_acl_update_mode() determines what's the equivalent mode and if SGID > > > > > needs to be cleared or not. I need to make use of this code in fuse > > > > > as well. Fuse will send this information to virtiofs file server and > > > > > file server will take care of clearing SGID if it needs to be done. > > > > > > > > > > Hence move this code in a separate helper so that more than one place > > > > > can call into it. > > > > > > > > > > Cc: Jan Kara <jack@suse.cz> > > > > > Cc: Andreas Gruenbacher <agruenba@redhat.com> > > > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > > > --- > > > > > fs/posix_acl.c | 3 +-- > > > > > include/linux/posix_acl.h | 11 +++++++++++ > > > > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > > > > > index f3309a7edb49..2d62494c4a5b 100644 > > > > > --- a/fs/posix_acl.c > > > > > +++ b/fs/posix_acl.c > > > > > @@ -684,8 +684,7 @@ int posix_acl_update_mode(struct user_namespace *mnt_userns, > > > > > return error; > > > > > if (error == 0) > > > > > *acl = NULL; > > > > > - if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > > > > - !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > > > > > + if (posix_acl_mode_clear_sgid(mnt_userns, inode)) > > > > > mode &= ~S_ISGID; > > > > > *mode_p = mode; > > > > > return 0; > > > > > diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h > > > > > index 307094ebb88c..073c5e546de3 100644 > > > > > --- a/include/linux/posix_acl.h > > > > > +++ b/include/linux/posix_acl.h > > > > > @@ -59,6 +59,17 @@ posix_acl_release(struct posix_acl *acl) > > > > > } > > > > > > > > > > > > > > > +static inline bool > > > > > +posix_acl_mode_clear_sgid(struct user_namespace *mnt_userns, > > > > > + struct inode *inode) > > > > > +{ > > > > > + if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > > > > + !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) > > > > > + return true; > > > > > + > > > > > + return false; > > > > > > > > That's just > > > > > > > > return !in_group_p(i_gid_into_mnt(mnt_userns, inode)) && > > > > !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID); > > > > > > > > The same pattern we have in posix_acl_update_mode also exists in > > > > setattr_copy and inode_init_owner, and almost the same pattern exists > > > > in setattr_prepare, so can this be cleaned up as well? The function > > > > also isn't POSIX ACL specific, so the function name is misleading. > > > > > > Good idea but that should probably be spun into a separate patchset that > > > only touches the vfs parts. > > > > IIUC, suggestion is that I should write a VFS helper (and not posix > > acl helper) and use that helper at other places too in the code. > > If there are other callers outside of acls (which should be iirc) then > yes. > > > > > I will do that and post in a separate patch series. > > Yeah, I think that makes more sense to have this be a separate change > instead of putting it together with the fuse change if it touches more > than one place. I do see that there are few places where this pattern is used and atleast some of them should be straight forward conversion. I will follow this up in separate patch series. I suspect that this might take little bit of back and forth, so will follow with fuse changes in parallel and open code there. Once this series gets merged will send another patch for fuse. Thanks Vivek ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] fuse: Add support for FUSE_SETXATTR_V2 2021-03-19 19:55 [PATCH 0/3] fuse: Fix clearing SGID when access ACL is set Vivek Goyal 2021-03-19 19:55 ` [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared Vivek Goyal @ 2021-03-19 19:55 ` Vivek Goyal 2021-03-19 19:55 ` [PATCH 3/3] fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID Vivek Goyal 2 siblings, 0 replies; 9+ messages in thread From: Vivek Goyal @ 2021-03-19 19:55 UTC (permalink / raw) To: linux-fsdevel, linux-kernel, virtio-fs, miklos Cc: vgoyal, lhenriques, dgilbert, seth.forshee Fuse client needs to send additional information to file server when it calls SETXATTR(system.posix_acl_access). Right now there is no extra space in fuse_setxattr_in. So introduce a v2 of the structure which has more space in it and can be used to send extra flags. "struct fuse_setxattr_in_v2" is only used if file server opts-in for it using flag FUSE_SETXATTR_V2 during feature negotiations. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/fuse/acl.c | 2 +- fs/fuse/fuse_i.h | 5 ++++- fs/fuse/inode.c | 4 +++- fs/fuse/xattr.c | 21 +++++++++++++++------ include/uapi/linux/fuse.h | 10 ++++++++++ 5 files changed, 33 insertions(+), 9 deletions(-) diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c index e9c0f916349d..d31260a139d4 100644 --- a/fs/fuse/acl.c +++ b/fs/fuse/acl.c @@ -94,7 +94,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode, return ret; } - ret = fuse_setxattr(inode, name, value, size, 0); + ret = fuse_setxattr(inode, name, value, size, 0, 0); kfree(value); } else { ret = fuse_removexattr(inode, name); diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 63d97a15ffde..d00bf0b9a38c 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -668,6 +668,9 @@ struct fuse_conn { /** Is setxattr not implemented by fs? */ unsigned no_setxattr:1; + /** Does file server support setxattr_v2 */ + unsigned setxattr_v2:1; + /** Is getxattr not implemented by fs? */ unsigned no_getxattr:1; @@ -1170,7 +1173,7 @@ void fuse_unlock_inode(struct inode *inode, bool locked); bool fuse_lock_inode(struct inode *inode); int fuse_setxattr(struct inode *inode, const char *name, const void *value, - size_t size, int flags); + size_t size, int flags, unsigned extra_flags); ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value, size_t size); ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size); diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index b0e18b470e91..1c726df13f80 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -1052,6 +1052,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args, fc->handle_killpriv_v2 = 1; fm->sb->s_flags |= SB_NOSEC; } + if (arg->flags & FUSE_SETXATTR_V2) + fc->setxattr_v2 = 1; } else { ra_pages = fc->max_read / PAGE_SIZE; fc->no_lock = 1; @@ -1095,7 +1097,7 @@ void fuse_send_init(struct fuse_mount *fm) FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL | FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS | FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA | - FUSE_HANDLE_KILLPRIV_V2; + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_V2; #ifdef CONFIG_FUSE_DAX if (fm->fc->dax) ia->in.flags |= FUSE_MAP_ALIGNMENT; diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c index 1a7d7ace54e1..f2aae72653dc 100644 --- a/fs/fuse/xattr.c +++ b/fs/fuse/xattr.c @@ -12,24 +12,33 @@ #include <linux/posix_acl_xattr.h> int fuse_setxattr(struct inode *inode, const char *name, const void *value, - size_t size, int flags) + size_t size, int flags, unsigned extra_flags) { struct fuse_mount *fm = get_fuse_mount(inode); FUSE_ARGS(args); struct fuse_setxattr_in inarg; + struct fuse_setxattr_in_v2 inarg_v2; + bool setxattr_v2 = fm->fc->setxattr_v2; int err; if (fm->fc->no_setxattr) return -EOPNOTSUPP; memset(&inarg, 0, sizeof(inarg)); - inarg.size = size; - inarg.flags = flags; + memset(&inarg_v2, 0, sizeof(inarg_v2)); + if (setxattr_v2) { + inarg_v2.size = size; + inarg_v2.flags = flags; + inarg_v2.setxattr_flags = extra_flags; + } else { + inarg.size = size; + inarg.flags = flags; + } args.opcode = FUSE_SETXATTR; args.nodeid = get_node_id(inode); args.in_numargs = 3; - args.in_args[0].size = sizeof(inarg); - args.in_args[0].value = &inarg; + args.in_args[0].size = setxattr_v2 ? sizeof(inarg_v2) : sizeof(inarg); + args.in_args[0].value = setxattr_v2 ? &inarg_v2 : (void *)&inarg; args.in_args[1].size = strlen(name) + 1; args.in_args[1].value = name; args.in_args[2].size = size; @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler, if (!value) return fuse_removexattr(inode, name); - return fuse_setxattr(inode, name, value, size, flags); + return fuse_setxattr(inode, name, value, size, flags, 0); } static bool no_xattr_list(struct dentry *dentry) diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index 54442612c48b..1bb555c1c117 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -179,6 +179,7 @@ * 7.33 * - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID * - add FUSE_OPEN_KILL_SUIDGID + * - add FUSE_SETXATTR_V2 */ #ifndef _LINUX_FUSE_H @@ -330,6 +331,7 @@ struct fuse_file_lock { * does not have CAP_FSETID. Additionally upon * write/truncate sgid is killed only if file has group * execute permission. (Same as Linux VFS behavior). + * FUSE_SETXATTR_V2: Does file server support V2 of struct fuse_setxattr_in */ #define FUSE_ASYNC_READ (1 << 0) #define FUSE_POSIX_LOCKS (1 << 1) @@ -360,6 +362,7 @@ struct fuse_file_lock { #define FUSE_MAP_ALIGNMENT (1 << 26) #define FUSE_SUBMOUNTS (1 << 27) #define FUSE_HANDLE_KILLPRIV_V2 (1 << 28) +#define FUSE_SETXATTR_V2 (1 << 29) /** * CUSE INIT request/reply flags @@ -686,6 +689,13 @@ struct fuse_setxattr_in { uint32_t flags; }; +struct fuse_setxattr_in_v2 { + uint32_t size; + uint32_t flags; + uint32_t setxattr_flags; + uint32_t padding; +}; + struct fuse_getxattr_in { uint32_t size; uint32_t padding; -- 2.25.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID 2021-03-19 19:55 [PATCH 0/3] fuse: Fix clearing SGID when access ACL is set Vivek Goyal 2021-03-19 19:55 ` [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared Vivek Goyal 2021-03-19 19:55 ` [PATCH 2/3] fuse: Add support for FUSE_SETXATTR_V2 Vivek Goyal @ 2021-03-19 19:55 ` Vivek Goyal 2 siblings, 0 replies; 9+ messages in thread From: Vivek Goyal @ 2021-03-19 19:55 UTC (permalink / raw) To: linux-fsdevel, linux-kernel, virtio-fs, miklos Cc: vgoyal, lhenriques, dgilbert, seth.forshee When posix access ACL is set, it can have an effect on file mode and it can also need to clear SGID if. - None of caller's group/supplementary groups match file owner group. AND - Caller is not priviliged (No CAP_FSETID). As of now fuser server is responsible for changing the file mode as well. But it does not know whether to clear SGID or not. So add a flag FUSE_SETXATTR_ACL_KILL_SGID and send this info with SETXATTR to let file server know that sgid needs to be cleared as well. Reported-by: Luis Henriques <lhenriques@suse.de> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/fuse/acl.c | 7 ++++++- include/uapi/linux/fuse.h | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c index d31260a139d4..45358124181a 100644 --- a/fs/fuse/acl.c +++ b/fs/fuse/acl.c @@ -71,6 +71,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode, return -EINVAL; if (acl) { + unsigned extra_flags = 0; /* * Fuse userspace is responsible for updating access * permissions in the inode, if needed. fuse_setxattr @@ -94,7 +95,11 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode, return ret; } - ret = fuse_setxattr(inode, name, value, size, 0, 0); + if (fc->setxattr_v2 && + posix_acl_mode_clear_sgid(&init_user_ns, inode)) + extra_flags |= FUSE_SETXATTR_ACL_KILL_SGID; + + ret = fuse_setxattr(inode, name, value, size, 0, extra_flags); kfree(value); } else { ret = fuse_removexattr(inode, name); diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index 1bb555c1c117..08c11a7beaa7 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -180,6 +180,7 @@ * - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID * - add FUSE_OPEN_KILL_SUIDGID * - add FUSE_SETXATTR_V2 + * - add FUSE_SETXATTR_ACL_KILL_SGID */ #ifndef _LINUX_FUSE_H @@ -454,6 +455,12 @@ struct fuse_file_lock { */ #define FUSE_OPEN_KILL_SUIDGID (1 << 0) +/** + * setxattr flags + * FUSE_SETXATTR_ACL_KILL_SGID: Clear SGID when system.posix_acl_access is set + */ +#define FUSE_SETXATTR_ACL_KILL_SGID (1 << 0) + enum fuse_opcode { FUSE_LOOKUP = 1, FUSE_FORGET = 2, /* no reply */ -- 2.25.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-23 22:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-19 19:55 [PATCH 0/3] fuse: Fix clearing SGID when access ACL is set Vivek Goyal 2021-03-19 19:55 ` [PATCH 1/3] posic_acl: Add a helper determine if SGID should be cleared Vivek Goyal 2021-03-19 22:42 ` Andreas Grünbacher 2021-03-20 10:03 ` Christian Brauner 2021-03-22 17:01 ` Vivek Goyal 2021-03-23 9:32 ` Christian Brauner 2021-03-23 22:50 ` Vivek Goyal 2021-03-19 19:55 ` [PATCH 2/3] fuse: Add support for FUSE_SETXATTR_V2 Vivek Goyal 2021-03-19 19:55 ` [PATCH 3/3] fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID Vivek Goyal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).