Bug 818536

Summary: Setgid not preserved in GFS2 with ACL
Product: [Fedora] Fedora Reporter: Nicolas Ecarnot <nicolas.ecarnot>
Component: GFSAssignee: Steve Whitehouse <swhiteho>
Status: CLOSED UPSTREAM QA Contact:
Severity: high Docs Contact:
Priority: medium    
Version: rawhideCC: adas, anprice, bmarzins, nicolas.ecarnot, rpeterso, swhiteho
Target Milestone: ---   
Target Release: ---   
Hardware: x86_64   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-05-23 17:23:10 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Test script none

Description Nicolas Ecarnot 2012-05-03 10:05:42 UTC
Description of problem:

The setgid bit gets lost when a user creates a subdir.

This has been discovered on an Ubuntu oneiric server 11.10 with a 3.0.0-12-server kernel, but the issue is not distro specific.

Version-Release number of selected component (if applicable):

GFS2 is 3.0.12-2ubuntu5.2

How reproducible:

Always

Steps to Reproduce:

1. set up a directory with ACLs and setgid flag like this :
root@server:/foo/bar# getfacl .
# file: .
# owner: root
# group: adminsGroup
# flags: -s-
user::rwx
group::rwx
group:domainUsers:rwx
mask::rwx
other::---
default:user::rwx
default:group::rwx
default:group:domainUsers:rwx
default:mask::rwx
default:other::--- 

2. When the user root runs 'mkdir rootDir', this directory correctly gets the adequate rights, and it gets the setgid bit (allowing deeper inheritance to keep working).

3. When a non-root user belonging to the adminsGroup group runs 'mkdir privDir', the directory also gain the same feature as above.

4. When a basic non-root user belonging to the domainUsers group runs 'mkdir basicDir', it gets created (the ACL allows it) but the setgid bit is *NOT* preserved. 
  
Actual results:

The sgid bit is lost. (flags: ---)

Expected results:

The sgid bit is preserved. (flags: -s-)

Additional info:

I have tried to add the suiddir flag when mounting the GFS2 partition, but this does not improve anything.

Further tests have been done using the exact same conditions (same users, same servers, same context, same rights, same test procedures), but on ext3, ext4 and ocfs2 partitions, and I confirm that the sgid bit is preserved.

Comment 1 Nicolas Ecarnot 2012-05-03 11:59:44 UTC
Just to be helpful and efficient : for the time being, I still have this GFS2 partition available in a cluster I am still setting up, beside another OCFS2 partition.
I don't doubt your soft and hardware testing means are greater than mine, but just to ask : Would it be helpful if I keep this partition available until you send a patch that I could test.
I ask that because one day or another, I will need this disk space and partition equipped with a adequate FS, and I don't doubt GFS2 will be this choice.

Comment 2 Steve Whitehouse 2012-05-04 09:34:12 UTC
Created attachment 582066 [details]
Test script

Attached is a shell script which I used to try and recreate the bug. I've used different group names since I wanted to use something that was already on my system. Note that user steve is in group steve and not in the users group.

I got the following results:

[root@chywoon mnt]# ./test.sh 
Test directory:
# file: test
# owner: root
# group: steve
# flags: -s-
user::rwx
group::rwx
group:users:rwx
mask::rwx
other::---
default:user::rwx
default:group::rwx
default:group:users:rwx
default:mask::rwx
default:other::---

Subdir created by root:
# file: test/foo
# owner: root
# group: steve
# flags: -s-
user::rwx
group::rwx
group:users:rwx
mask::rwx
other::---
default:user::rwx
default:group::rwx
default:group:users:rwx
default:mask::rwx
default:other::---

Subdir created by user steve:
# file: test/bar
# owner: steve
# group: steve
# flags: -s-
user::rwx
group::rwx
group:users:rwx
mask::rwx
other::---
default:user::rwx
default:group::rwx
default:group:users:rwx
default:mask::rwx
default:other::---

So in this case, it looks like the sgid bit is preserved. I wonder if I'm doing something wrong here? Can you reproduce with my script, or have I misinterpreted what you were doing?

Comment 3 Nicolas Ecarnot 2012-05-04 10:14:49 UTC
I'm sincere : I read your script and your test results, but I did not exec'ed it on my servers for this simple reason : the case I found is when the user that does the mkdir does not belong to the owner group.

Translate to your test case, that would be :

- use a third user, bob, that belong to the group 'users'
- He has the rights to mkdir, thanks to the ACLs
- According to me, its new dir won't get th sgid bit

Please, may you try this precise case ?

Comment 4 Steve Whitehouse 2012-05-04 10:32:28 UTC
Ok, got that now and I can reproduce this:

Subdir created by user bob:
# file: test/bar
# owner: bob
# group: steve
user::rwx
group::rwx
group:users:rwx
mask::rwx
other::---
default:user::rwx
default:group::rwx
default:group:users:rwx
default:mask::rwx
default:other::---

Comment 5 Steve Whitehouse 2012-05-04 11:26:43 UTC
I can see where things are going haywire... we are using setattr_copy() to update the attributes based on what posix_acl_create say we ought to be doing, but setattr_copy() has this code:


       if (ia_valid & ATTR_MODE) {
                umode_t mode = attr->ia_mode;

                if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
                        mode &= ~S_ISGID;
                inode->i_mode = mode;
        }

which is why it doesn't work for "out of group" creators which are allowed to create subdirs according to the acl.

Comment 6 Nicolas Ecarnot 2012-05-04 11:52:52 UTC
(YEAH !)
and
(HOORAY!)

Steve,

You're the one who did the job and found the faulty code, but I can't help enjoying being helpful in improving such a nice opensource product as GFS2!
Thank you for the time you took for that.
That had to be said.

Comment 7 Steve Whitehouse 2012-05-04 14:08:17 UTC
Patch now posted to cluster-devel which should make it into the next merge window upstream.

Comment 8 Steve Whitehouse 2012-05-23 17:23:10 UTC
Patch has been merged upstream at the merge window