| Message ID | 20210319195547.427371-2-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Fix clearing SGID when access ACL is set | expand |
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
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
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
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
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
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);
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(-)