[PATCH 01/11] ext4: Don't clear SGID when inheriting ACLs

[PATCH 01/11] ext4: Don't clear SGID when inheriting ACLs

[Jan Kara]

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 moving posix_acl_update_mode() out of
__ext4_set_acl() into ext4_set_acl(). That way the function will not be
called when inheriting ACLs which is what we want as it prevents SGID
bit clearing and the mode has been properly set by posix_acl_create()
anyway.

Fixes: 073931017b49d9458aa351605b43a7e34598caef
CC: stable@vger.kernel.org
CC: linux-ext4@vger.kernel.org
CC: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/acl.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index 3ec0e46de95f..37242af33ff2 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -193,13 +193,6 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
-		if (acl) {
-			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
-			if (error)
-				return error;
-			inode->i_ctime = current_time(inode);
-			ext4_mark_inode_dirty(handle, inode);
-		}
 		break;
 
 	case ACL_TYPE_DEFAULT:
@@ -242,7 +235,16 @@ ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	if (IS_ERR(handle))
 		return PTR_ERR(handle);
 
+	if (type == ACL_TYPE_ACCESS && acl) {
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+		if (error)
+			goto out_stop;
+		inode->i_ctime = current_time(inode);
+		ext4_mark_inode_dirty(handle, inode);
+	}
+
 	error = __ext4_set_acl(handle, inode, type, acl);
+out_stop:
 	ext4_journal_stop(handle);
 	if (error == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
 		goto retry;
-- 
2.12.3

Re: [PATCH 01/11] ext4: Don't clear SGID when inheriting ACLs

[Andreas Gruenbacher]

On Thu, Jun 22, 2017 at 3:31 PM, Jan Kara <jack@suse.cz> 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 moving posix_acl_update_mode() out of
> __ext4_set_acl() into ext4_set_acl(). That way the function will not be
> called when inheriting ACLs which is what we want as it prevents SGID
> bit clearing and the mode has been properly set by posix_acl_create()
> anyway.
>
> Fixes: 073931017b49d9458aa351605b43a7e34598caef
> CC: stable@vger.kernel.org
> CC: linux-ext4@vger.kernel.org
> CC: "Theodore Ts'o" <tytso@mit.edu>
> Signed-off-by: Jan Kara <jack@suse.cz>

This is looking good, thanks.

Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>

> ---
>  fs/ext4/acl.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
> index 3ec0e46de95f..37242af33ff2 100644
> --- a/fs/ext4/acl.c
> +++ b/fs/ext4/acl.c
> @@ -193,13 +193,6 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
>         switch (type) {
>         case ACL_TYPE_ACCESS:
>                 name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
> -               if (acl) {
> -                       error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> -                       if (error)
> -                               return error;
> -                       inode->i_ctime = current_time(inode);
> -                       ext4_mark_inode_dirty(handle, inode);
> -               }
>                 break;
>
>         case ACL_TYPE_DEFAULT:
> @@ -242,7 +235,16 @@ ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>         if (IS_ERR(handle))
>                 return PTR_ERR(handle);
>
> +       if (type == ACL_TYPE_ACCESS && acl) {
> +               error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> +               if (error)
> +                       goto out_stop;
> +               inode->i_ctime = current_time(inode);
> +               ext4_mark_inode_dirty(handle, inode);
> +       }
> +
>         error = __ext4_set_acl(handle, inode, type, acl);
> +out_stop:
>         ext4_journal_stop(handle);
>         if (error == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>                 goto retry;
> --
> 2.12.3
>

[PATCH 02/11] ext2: Don't clear SGID when inheriting ACLs

[Jan Kara]

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 __ext2_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: linux-ext4@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/acl.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
index 79dafa71effd..069c0dceda01 100644
--- a/fs/ext2/acl.c
+++ b/fs/ext2/acl.c
@@ -175,11 +175,8 @@ ext2_get_acl(struct inode *inode, int type)
 	return acl;
 }
 
-/*
- * inode->i_mutex: down
- */
-int
-ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
+static int
+__ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int name_index;
 	void *value = NULL;
@@ -189,13 +186,6 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	switch(type) {
 		case ACL_TYPE_ACCESS:
 			name_index = EXT2_XATTR_INDEX_POSIX_ACL_ACCESS;
-			if (acl) {
-				error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
-				if (error)
-					return error;
-				inode->i_ctime = current_time(inode);
-				mark_inode_dirty(inode);
-			}
 			break;
 
 		case ACL_TYPE_DEFAULT:
@@ -222,6 +212,24 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 }
 
 /*
+ * inode->i_mutex: down
+ */
+int
+ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
+{
+	int error;
+
+	if (type == ACL_TYPE_ACCESS && acl) {
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+		if (error)
+			return error;
+		inode->i_ctime = current_time(inode);
+		mark_inode_dirty(inode);
+	}
+	return __ext2_set_acl(inode, acl, type);
+}
+
+/*
  * Initialize the ACLs of a new inode. Called from ext2_new_inode.
  *
  * dir->i_mutex: down
@@ -238,12 +246,12 @@ ext2_init_acl(struct inode *inode, struct inode *dir)
 		return error;
 
 	if (default_acl) {
-		error = ext2_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
+		error = __ext2_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
 		posix_acl_release(default_acl);
 	}
 	if (acl) {
 		if (!error)
-			error = ext2_set_acl(inode, acl, ACL_TYPE_ACCESS);
+			error = __ext2_set_acl(inode, acl, ACL_TYPE_ACCESS);
 		posix_acl_release(acl);
 	}
 	return error;
-- 
2.12.3

[PATCH 03/11] btrfs: Don't clear SGID when inheriting ACLs

[Jan Kara]

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 moving posix_acl_update_mode() out of
__btrfs_set_acl() into btrfs_set_acl(). That way the function will not be
called when inheriting ACLs which is what we want as it prevents SGID
bit clearing and the mode has been properly set by posix_acl_create()
anyway.

Fixes: 073931017b49d9458aa351605b43a7e34598caef
CC: stable@vger.kernel.org
CC: linux-btrfs@vger.kernel.org
CC: David Sterba <dsterba@suse.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/btrfs/acl.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 247b8dfaf6e5..8d8370ddb6b2 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -78,12 +78,6 @@ static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
-		if (acl) {
-			ret = posix_acl_update_mode(inode, &inode->i_mode, &acl);
-			if (ret)
-				return ret;
-		}
-		ret = 0;
 		break;
 	case ACL_TYPE_DEFAULT:
 		if (!S_ISDIR(inode->i_mode))
@@ -119,6 +113,13 @@ static int __btrfs_set_acl(struct btrfs_trans_handle *trans,
 
 int btrfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
+	int ret;
+
+	if (type == ACL_TYPE_ACCESS && acl) {
+		ret = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+		if (ret)
+			return ret;
+	}
 	return __btrfs_set_acl(NULL, inode, acl, type);
 }
 
-- 
2.12.3

Re: [PATCH 03/11] btrfs: Don't clear SGID when inheriting ACLs

[David Sterba]

On Thu, Jun 22, 2017 at 03:31:07PM +0200, 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 moving posix_acl_update_mode() out of
> __btrfs_set_acl() into btrfs_set_acl(). That way the function will not be
> called when inheriting ACLs which is what we want as it prevents SGID
> bit clearing and the mode has been properly set by posix_acl_create()
> anyway.
> 
> Fixes: 073931017b49d9458aa351605b43a7e34598caef
> CC: stable@vger.kernel.org
> CC: linux-btrfs@vger.kernel.org
> CC: David Sterba <dsterba@suse.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Added to btrfs patch queue, thanks.

[PATCH 04/11] gfs2: Don't clear SGID when inheriting ACLs

[Jan Kara]

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 moving posix_acl_update_mode() out of
__gfs2_set_acl() into gfs2_set_acl(). That way the function will not be
called when inheriting ACLs which is what we want as it prevents SGID
bit clearing and the mode has been properly set by posix_acl_create()
anyway.

Fixes: 073931017b49d9458aa351605b43a7e34598caef
CC: stable@vger.kernel.org
CC: cluster-devel@redhat.com
CC: Bob Peterson <rpeterso@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/gfs2/acl.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
index 2524807ee070..a6d962323790 100644
--- a/fs/gfs2/acl.c
+++ b/fs/gfs2/acl.c
@@ -86,19 +86,6 @@ int __gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	char *data;
 	const char *name = gfs2_acl_name(type);
 
-	if (acl && acl->a_count > GFS2_ACL_MAX_ENTRIES(GFS2_SB(inode)))
-		return -E2BIG;
-
-	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode = inode->i_mode;
-
-		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
-		if (error)
-			return error;
-		if (mode != inode->i_mode)
-			mark_inode_dirty(inode);
-	}
-
 	if (acl) {
 		len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
 		if (len == 0)
@@ -130,6 +117,9 @@ int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	bool need_unlock = false;
 	int ret;
 
+	if (acl && acl->a_count > GFS2_ACL_MAX_ENTRIES(GFS2_SB(inode)))
+		return -E2BIG;
+
 	ret = gfs2_rsqa_alloc(ip);
 	if (ret)
 		return ret;
@@ -140,7 +130,18 @@ int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			return ret;
 		need_unlock = true;
 	}
+	if (type == ACL_TYPE_ACCESS && acl) {
+		umode_t mode = inode->i_mode;
+
+		ret = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+		if (ret)
+			goto unlock;
+		if (mode != inode->i_mode)
+			mark_inode_dirty(inode);
+	}
+
 	ret = __gfs2_set_acl(inode, acl, type);
+unlock:
 	if (need_unlock)
 		gfs2_glock_dq_uninit(&gh);
 	return ret;
-- 
2.12.3

Re: [PATCH 04/11] gfs2: Don't clear SGID when inheriting ACLs

[Jan Kara]

On Thu 22-06-17 15:31:08, 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 moving posix_acl_update_mode() out of
> __gfs2_set_acl() into gfs2_set_acl(). That way the function will not be
> called when inheriting ACLs which is what we want as it prevents SGID
> bit clearing and the mode has been properly set by posix_acl_create()
> anyway.
> 
> Fixes: 073931017b49d9458aa351605b43a7e34598caef
> CC: stable@vger.kernel.org
> CC: cluster-devel@redhat.com
> CC: Bob Peterson <rpeterso@redhat.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Bob, can you please pick up this fix? Thanks!

								Honza

> ---
>  fs/gfs2/acl.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index 2524807ee070..a6d962323790 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -86,19 +86,6 @@ int __gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  	char *data;
>  	const char *name = gfs2_acl_name(type);
>  
> -	if (acl && acl->a_count > GFS2_ACL_MAX_ENTRIES(GFS2_SB(inode)))
> -		return -E2BIG;
> -
> -	if (type == ACL_TYPE_ACCESS) {
> -		umode_t mode = inode->i_mode;
> -
> -		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> -		if (error)
> -			return error;
> -		if (mode != inode->i_mode)
> -			mark_inode_dirty(inode);
> -	}
> -
>  	if (acl) {
>  		len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
>  		if (len == 0)
> @@ -130,6 +117,9 @@ int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  	bool need_unlock = false;
>  	int ret;
>  
> +	if (acl && acl->a_count > GFS2_ACL_MAX_ENTRIES(GFS2_SB(inode)))
> +		return -E2BIG;
> +
>  	ret = gfs2_rsqa_alloc(ip);
>  	if (ret)
>  		return ret;
> @@ -140,7 +130,18 @@ int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  			return ret;
>  		need_unlock = true;
>  	}
> +	if (type == ACL_TYPE_ACCESS && acl) {
> +		umode_t mode = inode->i_mode;
> +
> +		ret = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> +		if (ret)
> +			goto unlock;
> +		if (mode != inode->i_mode)
> +			mark_inode_dirty(inode);
> +	}
> +
>  	ret = __gfs2_set_acl(inode, acl, type);
> +unlock:
>  	if (need_unlock)
>  		gfs2_glock_dq_uninit(&gh);
>  	return ret;
> -- 
> 2.12.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 04/11] gfs2: Don't clear SGID when inheriting ACLs

[Bob Peterson]

----- Original Message -----
| On Thu 22-06-17 15:31:08, 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 moving posix_acl_update_mode() out of
| > __gfs2_set_acl() into gfs2_set_acl(). That way the function will not be
| > called when inheriting ACLs which is what we want as it prevents SGID
| > bit clearing and the mode has been properly set by posix_acl_create()
| > anyway.
| > 
| > Fixes: 073931017b49d9458aa351605b43a7e34598caef
| > CC: stable@vger.kernel.org
| > CC: cluster-devel@redhat.com
| > CC: Bob Peterson <rpeterso@redhat.com>
| > Signed-off-by: Jan Kara <jack@suse.cz>
| 
| Bob, can you please pick up this fix? Thanks!

Hi Honza,

Sorry this slipped my attention for so long.
This is now applied to the for-next branch of the linux-gfs2 tree:
https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?h=for-next&id=914cea93dd89f00b41c1d8ff93f17be47356a36a

Regards,

Bob Peterson
Red Hat File Systems

[PATCH 05/11] hfsplus: Don't clear SGID when inheriting ACLs

[Jan Kara]

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 __hfsplus_set_posix_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
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/hfsplus/posix_acl.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c
index 9b92058a1240..6bb5d7c42888 100644
--- a/fs/hfsplus/posix_acl.c
+++ b/fs/hfsplus/posix_acl.c
@@ -51,8 +51,8 @@ struct posix_acl *hfsplus_get_posix_acl(struct inode *inode, int type)
 	return acl;
 }
 
-int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl,
-		int type)
+static int __hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl,
+				   int type)
 {
 	int err;
 	char *xattr_name;
@@ -64,12 +64,6 @@ int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl,
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		xattr_name = XATTR_NAME_POSIX_ACL_ACCESS;
-		if (acl) {
-			err = posix_acl_update_mode(inode, &inode->i_mode, &acl);
-			if (err)
-				return err;
-		}
-		err = 0;
 		break;
 
 	case ACL_TYPE_DEFAULT:
@@ -105,6 +99,18 @@ int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl,
 	return err;
 }
 
+int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl, int type)
+{
+	int err;
+
+	if (type == ACL_TYPE_ACCESS && acl) {
+		err = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+		if (err)
+			return err;
+	}
+	return __hfsplus_set_posix_acl(inode, acl, type);
+}
+
 int hfsplus_init_posix_acl(struct inode *inode, struct inode *dir)
 {
 	int err = 0;
@@ -122,15 +128,15 @@ int hfsplus_init_posix_acl(struct inode *inode, struct inode *dir)
 		return err;
 
 	if (default_acl) {
-		err = hfsplus_set_posix_acl(inode, default_acl,
-					    ACL_TYPE_DEFAULT);
+		err = __hfsplus_set_posix_acl(inode, default_acl,
+					      ACL_TYPE_DEFAULT);
 		posix_acl_release(default_acl);
 	}
 
 	if (acl) {
 		if (!err)
-			err = hfsplus_set_posix_acl(inode, acl,
-						    ACL_TYPE_ACCESS);
+			err = __hfsplus_set_posix_acl(inode, acl,
+						      ACL_TYPE_ACCESS);
 		posix_acl_release(acl);
 	}
 	return err;
-- 
2.12.3

[PATCH 06/11] jfs: Don't clear SGID when inheriting ACLs

[Jan Kara]

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 moving posix_acl_update_mode() out of
__jfs_set_acl() into jfs_set_acl(). That way the function will not be
called when inheriting ACLs which is what we want as it prevents SGID
bit clearing and the mode has been properly set by posix_acl_create()
anyway.

Fixes: 073931017b49d9458aa351605b43a7e34598caef
CC: stable@vger.kernel.org
CC: Dave Kleikamp <shaggy@kernel.org>
CC: jfs-discussion@lists.sourceforge.net
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jfs/acl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
index 7bc186f4ed4d..1be45c8d460d 100644
--- a/fs/jfs/acl.c
+++ b/fs/jfs/acl.c
@@ -77,13 +77,6 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		ea_name = XATTR_NAME_POSIX_ACL_ACCESS;
-		if (acl) {
-			rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
-			if (rc)
-				return rc;
-			inode->i_ctime = current_time(inode);
-			mark_inode_dirty(inode);
-		}
 		break;
 	case ACL_TYPE_DEFAULT:
 		ea_name = XATTR_NAME_POSIX_ACL_DEFAULT;
@@ -118,9 +111,17 @@ int jfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 
 	tid = txBegin(inode->i_sb, 0);
 	mutex_lock(&JFS_IP(inode)->commit_mutex);
+	if (type == ACL_TYPE_ACCESS && acl) {
+		rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+		if (rc)
+			goto end_tx;
+		inode->i_ctime = current_time(inode);
+		mark_inode_dirty(inode);
+	}
 	rc = __jfs_set_acl(tid, inode, type, acl);
 	if (!rc)
 		rc = txCommit(tid, 1, &inode, 0);
+end_tx:
 	txEnd(tid);
 	mutex_unlock(&JFS_IP(inode)->commit_mutex);
 	return rc;
-- 
2.12.3

Re: [PATCH 06/11] jfs: Don't clear SGID when inheriting ACLs

[Jan Kara]

On Thu 22-06-17 15:31:10, 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 moving posix_acl_update_mode() out of
> __jfs_set_acl() into jfs_set_acl(). That way the function will not be
> called when inheriting ACLs which is what we want as it prevents SGID
> bit clearing and the mode has been properly set by posix_acl_create()
> anyway.
> 
> Fixes: 073931017b49d9458aa351605b43a7e34598caef
> CC: stable@vger.kernel.org
> CC: Dave Kleikamp <shaggy@kernel.org>
> CC: jfs-discussion@lists.sourceforge.net
> Signed-off-by: Jan Kara <jack@suse.cz>

Dave, can you please pick up this fix? Thanks!

								Honza

> ---
>  fs/jfs/acl.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
> index 7bc186f4ed4d..1be45c8d460d 100644
> --- a/fs/jfs/acl.c
> +++ b/fs/jfs/acl.c
> @@ -77,13 +77,6 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
>  	switch (type) {
>  	case ACL_TYPE_ACCESS:
>  		ea_name = XATTR_NAME_POSIX_ACL_ACCESS;
> -		if (acl) {
> -			rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> -			if (rc)
> -				return rc;
> -			inode->i_ctime = current_time(inode);
> -			mark_inode_dirty(inode);
> -		}
>  		break;
>  	case ACL_TYPE_DEFAULT:
>  		ea_name = XATTR_NAME_POSIX_ACL_DEFAULT;
> @@ -118,9 +111,17 @@ int jfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  
>  	tid = txBegin(inode->i_sb, 0);
>  	mutex_lock(&JFS_IP(inode)->commit_mutex);
> +	if (type == ACL_TYPE_ACCESS && acl) {
> +		rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> +		if (rc)
> +			goto end_tx;
> +		inode->i_ctime = current_time(inode);
> +		mark_inode_dirty(inode);
> +	}
>  	rc = __jfs_set_acl(tid, inode, type, acl);
>  	if (!rc)
>  		rc = txCommit(tid, 1, &inode, 0);
> +end_tx:
>  	txEnd(tid);
>  	mutex_unlock(&JFS_IP(inode)->commit_mutex);
>  	return rc;
> -- 
> 2.12.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 06/11] jfs: Don't clear SGID when inheriting ACLs

[Dave Kleikamp]

On 07/18/2017 11:19 AM, Jan Kara wrote:
> On Thu 22-06-17 15:31:10, 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 moving posix_acl_update_mode() out of
>> __jfs_set_acl() into jfs_set_acl(). That way the function will not be
>> called when inheriting ACLs which is what we want as it prevents SGID
>> bit clearing and the mode has been properly set by posix_acl_create()
>> anyway.
>>
>> Fixes: 073931017b49d9458aa351605b43a7e34598caef
>> CC: stable@vger.kernel.org
>> CC: Dave Kleikamp <shaggy@kernel.org>
>> CC: jfs-discussion@lists.sourceforge.net
>> Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Dave, can you please pick up this fix? Thanks!

Yeah. I'll take care if it.

Thanks,
Shaggy

> 
> 								Honza
> 
>> ---
>>  fs/jfs/acl.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
>> index 7bc186f4ed4d..1be45c8d460d 100644
>> --- a/fs/jfs/acl.c
>> +++ b/fs/jfs/acl.c
>> @@ -77,13 +77,6 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
>>  	switch (type) {
>>  	case ACL_TYPE_ACCESS:
>>  		ea_name = XATTR_NAME_POSIX_ACL_ACCESS;
>> -		if (acl) {
>> -			rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
>> -			if (rc)
>> -				return rc;
>> -			inode->i_ctime = current_time(inode);
>> -			mark_inode_dirty(inode);
>> -		}
>>  		break;
>>  	case ACL_TYPE_DEFAULT:
>>  		ea_name = XATTR_NAME_POSIX_ACL_DEFAULT;
>> @@ -118,9 +111,17 @@ int jfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>>  
>>  	tid = txBegin(inode->i_sb, 0);
>>  	mutex_lock(&JFS_IP(inode)->commit_mutex);
>> +	if (type == ACL_TYPE_ACCESS && acl) {
>> +		rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
>> +		if (rc)
>> +			goto end_tx;
>> +		inode->i_ctime = current_time(inode);
>> +		mark_inode_dirty(inode);
>> +	}
>>  	rc = __jfs_set_acl(tid, inode, type, acl);
>>  	if (!rc)
>>  		rc = txCommit(tid, 1, &inode, 0);
>> +end_tx:
>>  	txEnd(tid);
>>  	mutex_unlock(&JFS_IP(inode)->commit_mutex);
>>  	return rc;
>> -- 
>> 2.12.3
>>

[PATCH 08/11] ocfs2: Don't clear SGID when inheriting ACLs

[Jan Kara]

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 moving posix_acl_update_mode() out of ocfs2_set_acl()
into ocfs2_iop_set_acl(). That way the function will not be called when
inheriting ACLs which is what we want as it prevents SGID bit clearing
and the mode has been properly set by posix_acl_create() anyway. Also
posix_acl_chmod() that is calling ocfs2_set_acl() takes care of updating
mode itself.

Fixes: 073931017b49d9458aa351605b43a7e34598caef
CC: stable@vger.kernel.org
CC: Joel Becker <jlbec@evilplan.org>
CC: ocfs2-devel@oss.oracle.com
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/acl.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c
index 3b9c3638a381..40b5cc97f7b0 100644
--- a/fs/ocfs2/acl.c
+++ b/fs/ocfs2/acl.c
@@ -240,18 +240,6 @@ static int ocfs2_set_acl(handle_t *handle,
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		name_index = OCFS2_XATTR_INDEX_POSIX_ACL_ACCESS;
-		if (acl) {
-			umode_t mode;
-
-			ret = posix_acl_update_mode(inode, &mode, &acl);
-			if (ret)
-				return ret;
-
-			ret = ocfs2_acl_set_mode(inode, di_bh,
-						 handle, mode);
-			if (ret)
-				return ret;
-		}
 		break;
 	case ACL_TYPE_DEFAULT:
 		name_index = OCFS2_XATTR_INDEX_POSIX_ACL_DEFAULT;
@@ -289,7 +277,19 @@ int ocfs2_iop_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	had_lock = ocfs2_inode_lock_tracker(inode, &bh, 1, &oh);
 	if (had_lock < 0)
 		return had_lock;
+	if (type == ACL_TYPE_ACCESS && acl) {
+		umode_t mode;
+
+		status = posix_acl_update_mode(inode, &mode, &acl);
+		if (status)
+			goto unlock;
+
+		status = ocfs2_acl_set_mode(inode, bh, NULL, mode);
+		if (status)
+			goto unlock;
+	}
 	status = ocfs2_set_acl(NULL, inode, bh, type, acl, NULL, NULL);
+unlock:
 	ocfs2_inode_unlock_tracker(inode, 1, &oh, had_lock);
 	brelse(bh);
 	return status;
-- 
2.12.3

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

[Jan Kara]

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

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

[Jan Kara]

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

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

[Mike Marshall]

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

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

[Jan Kara]

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

[PATCH 10/11] reiserfs: Don't clear SGID when inheriting ACLs

[Jan Kara]

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 moving posix_acl_update_mode() out of
__reiserfs_set_acl() into reiserfs_set_acl(). That way the function will
not be called when inheriting ACLs which is what we want as it prevents
SGID bit clearing and the mode has been properly set by
posix_acl_create() anyway.

Fixes: 073931017b49d9458aa351605b43a7e34598caef
CC: stable@vger.kernel.org
CC: reiserfs-devel@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/reiserfs/xattr_acl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
index 3d2256a425ee..d92a1dc6ee70 100644
--- a/fs/reiserfs/xattr_acl.c
+++ b/fs/reiserfs/xattr_acl.c
@@ -37,7 +37,14 @@ reiserfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	error = journal_begin(&th, inode->i_sb, jcreate_blocks);
 	reiserfs_write_unlock(inode->i_sb);
 	if (error == 0) {
+		if (type == ACL_TYPE_ACCESS && acl) {
+			error = posix_acl_update_mode(inode, &inode->i_mode,
+						      &acl);
+			if (error)
+				goto unlock;
+		}
 		error = __reiserfs_set_acl(&th, inode, type, acl);
+unlock:
 		reiserfs_write_lock(inode->i_sb);
 		error2 = journal_end(&th);
 		reiserfs_write_unlock(inode->i_sb);
@@ -241,11 +248,6 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
-		if (acl) {
-			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
-			if (error)
-				return error;
-		}
 		break;
 	case ACL_TYPE_DEFAULT:
 		name = XATTR_NAME_POSIX_ACL_DEFAULT;
-- 
2.12.3

[PATCH 11/11] xfs: Don't clear SGID when inheriting ACLs

[Jan Kara]

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 calling __xfs_set_acl() instead of xfs_set_acl() when
setting up inode in xfs_generic_create(). That prevents SGID bit
clearing and mode is properly set by posix_acl_create() anyway. We also
reorder arguments of __xfs_set_acl() to match the ordering of
xfs_set_acl() to make things consistent.

Fixes: 073931017b49d9458aa351605b43a7e34598caef
CC: stable@vger.kernel.org
CC: Darrick J. Wong <darrick.wong@oracle.com>
CC: linux-xfs@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_acl.c  | 6 +++---
 fs/xfs/xfs_acl.h  | 1 +
 fs/xfs/xfs_iops.c | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index b468e041f207..7034e17535de 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -170,8 +170,8 @@ xfs_get_acl(struct inode *inode, int type)
 	return acl;
 }
 
-STATIC int
-__xfs_set_acl(struct inode *inode, int type, struct posix_acl *acl)
+int
+__xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	struct xfs_inode *ip = XFS_I(inode);
 	unsigned char *ea_name;
@@ -268,5 +268,5 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	}
 
  set_acl:
-	return __xfs_set_acl(inode, type, acl);
+	return __xfs_set_acl(inode, acl, type);
 }
diff --git a/fs/xfs/xfs_acl.h b/fs/xfs/xfs_acl.h
index 286fa89217f5..04327318ef67 100644
--- a/fs/xfs/xfs_acl.h
+++ b/fs/xfs/xfs_acl.h
@@ -24,6 +24,7 @@ struct posix_acl;
 #ifdef CONFIG_XFS_POSIX_ACL
 extern struct posix_acl *xfs_get_acl(struct inode *inode, int type);
 extern int xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
+extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
 #else
 static inline struct posix_acl *xfs_get_acl(struct inode *inode, int type)
 {
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index ebfc13350f9a..077e2b2ac773 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -190,12 +190,12 @@ xfs_generic_create(
 
 #ifdef CONFIG_XFS_POSIX_ACL
 	if (default_acl) {
-		error = xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
+		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
 		if (error)
 			goto out_cleanup_inode;
 	}
 	if (acl) {
-		error = xfs_set_acl(inode, acl, ACL_TYPE_ACCESS);
+		error = __xfs_set_acl(inode, acl, ACL_TYPE_ACCESS);
 		if (error)
 			goto out_cleanup_inode;
 	}
-- 
2.12.3

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

[Darrick J. Wong]

On Thu, Jun 22, 2017 at 03:31:15PM +0200, 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 calling __xfs_set_acl() instead of xfs_set_acl() when
> setting up inode in xfs_generic_create(). That prevents SGID bit
> clearing and mode is properly set by posix_acl_create() anyway. We also
> reorder arguments of __xfs_set_acl() to match the ordering of
> xfs_set_acl() to make things consistent.
> 
> Fixes: 073931017b49d9458aa351605b43a7e34598caef
> CC: stable@vger.kernel.org
> CC: Darrick J. Wong <darrick.wong@oracle.com>
> CC: linux-xfs@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks ok, will pull into xfs tree...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_acl.c  | 6 +++---
>  fs/xfs/xfs_acl.h  | 1 +
>  fs/xfs/xfs_iops.c | 4 ++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index b468e041f207..7034e17535de 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -170,8 +170,8 @@ xfs_get_acl(struct inode *inode, int type)
>  	return acl;
>  }
>  
> -STATIC int
> -__xfs_set_acl(struct inode *inode, int type, struct posix_acl *acl)
> +int
> +__xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  {
>  	struct xfs_inode *ip = XFS_I(inode);
>  	unsigned char *ea_name;
> @@ -268,5 +268,5 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  	}
>  
>   set_acl:
> -	return __xfs_set_acl(inode, type, acl);
> +	return __xfs_set_acl(inode, acl, type);
>  }
> diff --git a/fs/xfs/xfs_acl.h b/fs/xfs/xfs_acl.h
> index 286fa89217f5..04327318ef67 100644
> --- a/fs/xfs/xfs_acl.h
> +++ b/fs/xfs/xfs_acl.h
> @@ -24,6 +24,7 @@ struct posix_acl;
>  #ifdef CONFIG_XFS_POSIX_ACL
>  extern struct posix_acl *xfs_get_acl(struct inode *inode, int type);
>  extern int xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> +extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
>  #else
>  static inline struct posix_acl *xfs_get_acl(struct inode *inode, int type)
>  {
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index ebfc13350f9a..077e2b2ac773 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -190,12 +190,12 @@ xfs_generic_create(
>  
>  #ifdef CONFIG_XFS_POSIX_ACL
>  	if (default_acl) {
> -		error = xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> +		error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>  		if (error)
>  			goto out_cleanup_inode;
>  	}
>  	if (acl) {
> -		error = xfs_set_acl(inode, acl, ACL_TYPE_ACCESS);
> +		error = __xfs_set_acl(inode, acl, ACL_TYPE_ACCESS);
>  		if (error)
>  			goto out_cleanup_inode;
>  	}
> -- 
> 2.12.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html