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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Honza...

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...

-Mike

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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]