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
