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@xxxxxxx> 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@xxxxxxxxxxxxxxx
> >> CC: Mike Marshall <hubcap@xxxxxxxxxxxx>
> >> CC: pvfs2-developers@xxxxxxxxxxxxxxxxxxxxxxx
> >> Signed-off-by: Jan Kara <jack@xxxxxxx>
> >
> > 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@xxxxxxxx>
> > SUSE Labs, CR
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR