Re: [PATCH 09/11] orangefs: Don't clear SGID when inheriting ACLs

[Jan Kara <jack@suse.cz>]

Hi Mike,

On Thu 20-07-17 12:05:17, Mike Marshall wrote:
> I saw this when you put it out. My review of your patch caused me to
> find something broken in general about how ACLs work in
> Orangefs. I continue to work on that bug, and plan on picking
> up your patch as soon as I fix the other bug... your patch looks
> fine, I could pick it up without being able to test it if my delay is
> holding up something you are doing...

No, it's not holding up anything. It is enough for me to know you have the
patch somewhere queued and I can forget about it ;). Thanks for getting
back to me.

								Honza

> On Tue, Jul 18, 2017 at 12:18 PM, Jan Kara <jack@suse.cz> wrote:
> > On Thu 22-06-17 15:31:13, Jan Kara wrote:
> >> When new directory 'DIR1' is created in a directory 'DIR0' with SGID bit
> >> set, DIR1 is expected to have SGID bit set (and owning group equal to
> >> the owning group of 'DIR0'). However when 'DIR0' also has some default
> >> ACLs that 'DIR1' inherits, setting these ACLs will result in SGID bit on
> >> 'DIR1' to get cleared if user is not member of the owning group.
> >>
> >> Fix the problem by creating __orangefs_set_acl() function that does not
> >> call posix_acl_update_mode() and use it when inheriting ACLs. That
> >> prevents SGID bit clearing and the mode has been properly set by
> >> posix_acl_create() anyway.
> >>
> >> Fixes: 073931017b49d9458aa351605b43a7e34598caef
> >> CC: stable@vger.kernel.org
> >> CC: Mike Marshall <hubcap@omnibond.com>
> >> CC: pvfs2-developers@beowulf-underground.org
> >> Signed-off-by: Jan Kara <jack@suse.cz>
> >
> > Mike, can you please pick up this fix? Thanks!
> >
> >                                                                 Honza
> >
> >> ---
> >>  fs/orangefs/acl.c | 48 ++++++++++++++++++++++++++++--------------------
> >>  1 file changed, 28 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/fs/orangefs/acl.c b/fs/orangefs/acl.c
> >> index 7a3754488312..9409aac232f7 100644
> >> --- a/fs/orangefs/acl.c
> >> +++ b/fs/orangefs/acl.c
> >> @@ -61,9 +61,9 @@ struct posix_acl *orangefs_get_acl(struct inode *inode, int type)
> >>       return acl;
> >>  }
> >>
> >> -int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >> +static int __orangefs_set_acl(struct inode *inode, struct posix_acl *acl,
> >> +                           int type)
> >>  {
> >> -     struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
> >>       int error = 0;
> >>       void *value = NULL;
> >>       size_t size = 0;
> >> @@ -72,22 +72,6 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >>       switch (type) {
> >>       case ACL_TYPE_ACCESS:
> >>               name = XATTR_NAME_POSIX_ACL_ACCESS;
> >> -             if (acl) {
> >> -                     umode_t mode;
> >> -
> >> -                     error = posix_acl_update_mode(inode, &mode, &acl);
> >> -                     if (error) {
> >> -                             gossip_err("%s: posix_acl_update_mode err: %d\n",
> >> -                                        __func__,
> >> -                                        error);
> >> -                             return error;
> >> -                     }
> >> -
> >> -                     if (inode->i_mode != mode)
> >> -                             SetModeFlag(orangefs_inode);
> >> -                     inode->i_mode = mode;
> >> -                     mark_inode_dirty_sync(inode);
> >> -             }
> >>               break;
> >>       case ACL_TYPE_DEFAULT:
> >>               name = XATTR_NAME_POSIX_ACL_DEFAULT;
> >> @@ -132,6 +116,29 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >>       return error;
> >>  }
> >>
> >> +int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> >> +{
> >> +     int error;
> >> +
> >> +     if (type == ACL_TYPE_ACCESS && acl) {
> >> +             umode_t mode;
> >> +
> >> +             error = posix_acl_update_mode(inode, &mode, &acl);
> >> +             if (error) {
> >> +                     gossip_err("%s: posix_acl_update_mode err: %d\n",
> >> +                                __func__,
> >> +                                error);
> >> +                     return error;
> >> +             }
> >> +
> >> +             if (inode->i_mode != mode)
> >> +                     SetModeFlag(ORANGEFS_I(inode));
> >> +             inode->i_mode = mode;
> >> +             mark_inode_dirty_sync(inode);
> >> +     }
> >> +     return __orangefs_set_acl(inode, acl, type);
> >> +}
> >> +
> >>  int orangefs_init_acl(struct inode *inode, struct inode *dir)
> >>  {
> >>       struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
> >> @@ -146,13 +153,14 @@ int orangefs_init_acl(struct inode *inode, struct inode *dir)
> >>               return error;
> >>
> >>       if (default_acl) {
> >> -             error = orangefs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> >> +             error = __orangefs_set_acl(inode, default_acl,
> >> +                                        ACL_TYPE_DEFAULT);
> >>               posix_acl_release(default_acl);
> >>       }
> >>
> >>       if (acl) {
> >>               if (!error)
> >> -                     error = orangefs_set_acl(inode, acl, ACL_TYPE_ACCESS);
> >> +                     error = __orangefs_set_acl(inode, acl, ACL_TYPE_ACCESS);
> >>               posix_acl_release(acl);
> >>       }
> >>
> >> --
> >> 2.12.3
> >>
> > --
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR