[Cluster-devel] [PATCH 1/2] gfs2: don't return ENODATA in __gfs2_xattr_set unless replacing

[Cluster-devel] [PATCH 1/2] gfs2: don't return ENODATA in __gfs2_xattr_set unless replacing

[Ernesto A. Fernández]

The function __gfs2_xattr_set() will return -ENODATA when called to
remove a xattr that does not exist. The result is that setfacl will
show an exit status of 1 when called to set only a file's mode bits
(on a file with no ACLs), despite succeeding. A "No data available"
error will be printed as well.

To fix this return 0 instead, except when the XATTR_REPLACE flag is
set, in which case -ENODATA is appropriate. This is consistent with
how most other xattr setting functions work, in other filesystems.

Signed-off-by: Ernesto A. Fern?ndez <ernesto.mnd.fernandez@gmail.com>
---
This patch is needed for the next one to work properly. After the next
patch the mode will no longer be changed if there is an error when setting
the xattr, so it's important that the error code means actual failure,
while this ENODATA was just informative.

 fs/gfs2/xattr.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
index 5417955..8ca56a7 100644
--- a/fs/gfs2/xattr.c
+++ b/fs/gfs2/xattr.c
@@ -1209,8 +1209,12 @@ int __gfs2_xattr_set(struct inode *inode, const char *name,
 	if (namel > GFS2_EA_MAX_NAME_LEN)
 		return -ERANGE;
 
-	if (value == NULL)
-		return gfs2_xattr_remove(ip, type, name);
+	if (value == NULL) {
+		error = gfs2_xattr_remove(ip, type, name);
+		if (error == -ENODATA && !(flags & XATTR_REPLACE))
+			error = 0;
+		return error;
+	}
 
 	if (ea_check_size(sdp, namel, size))
 		return -ERANGE;
-- 
2.1.4

[Cluster-devel] [PATCH 2/2] gfs2: preserve i_mode if __gfs2_set_acl() fails

[Ernesto A. Fernández]

When changing a file's acl mask, __gfs2_set_acl() will first set the
group bits of i_mode to the value of the mask, and only then set the
actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the
file had no acl attribute to begin with, the system will from now on
assume that the mask permission bits are actual group permission bits,
potentially granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by: Ernesto A. Fern?ndez <ernesto.mnd.fernandez@gmail.com>
---
This issue affects several filesystems, some of them have already applied
patches. See for example:
  - 397e434 ext4: preserve i_mode if __ext4_set_acl() fails
  - f070e5a jfs: preserve i_mode if __jfs_set_acl() fails
  - fcea8ae reiserfs: preserve i_mode if __reiserfs_set_acl() fails
  - fe26569 ext2: preserve i_mode if ext2_set_acl() fails

 fs/gfs2/acl.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
index 2524807..06bbb6b 100644
--- a/fs/gfs2/acl.c
+++ b/fs/gfs2/acl.c
@@ -85,18 +85,15 @@ int __gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	int len;
 	char *data;
 	const char *name = gfs2_acl_name(type);
+	umode_t mode = inode->i_mode;
 
 	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);
+		error = posix_acl_update_mode(inode, &mode, &acl);
 		if (error)
 			return error;
-		if (mode != inode->i_mode)
-			mark_inode_dirty(inode);
 	}
 
 	if (acl) {
@@ -118,6 +115,10 @@ int __gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	if (error)
 		goto out;
 	set_cached_acl(inode, type, acl);
+	if (mode != inode->i_mode) {
+		inode->i_mode = mode;
+		mark_inode_dirty(inode);
+	}
 out:
 	kfree(data);
 	return error;
-- 
2.1.4

[Cluster-devel] [PATCH 2/2] gfs2: preserve i_mode if __gfs2_set_acl() fails

[Bob Peterson]

----- Original Message -----
| When changing a file's acl mask, __gfs2_set_acl() will first set the
| group bits of i_mode to the value of the mask, and only then set the
| actual extended attribute representing the new acl.
| 
| If the second part fails (due to lack of space, for example) and the
| file had no acl attribute to begin with, the system will from now on
| assume that the mask permission bits are actual group permission bits,
| potentially granting access to the wrong users.
| 
| Prevent this by only changing the inode mode after the acl has been set.
| 
| Signed-off-by: Ernesto A. Fern?ndez <ernesto.mnd.fernandez@gmail.com>
| ---

Hi Ernesto,

This patch seems to be for Linus's tree, not the active "for-next"
upstream development branch we use for GFS2.

Since the last merge window, function __gfs2_set_acl was heavily modified
by a patch from Jan Kara called "gfs2: Don't clear SGID when inheriting
ACLs" which deleted the section of code you modified, in favor of another.
So your patch does not apply to for-next. See:

https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?h=for-next&id=914cea93dd89f00b41c1d8ff93f17be47356a36a

Can you rework this patch so it applies to the for-next branch of
the linux-gfs2 git tree? Thanks.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH v2 2/2] gfs2: preserve i_mode if __gfs2_set_acl() fails

[Ernesto A. Fernández]

When changing a file's acl mask, __gfs2_set_acl() will first set the
group bits of i_mode to the value of the mask, and only then set the
actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the
file had no acl attribute to begin with, the system will from now on
assume that the mask permission bits are actual group permission bits,
potentially granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by: Ernesto A. Fern?ndez <ernesto.mnd.fernandez@gmail.com>
---
Changes in v2:
  - Rebased on top of the for-next branch. The first part of the patchset
    does not need to be fixed.

 fs/gfs2/acl.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
index a6d9623..9d5eecb 100644
--- a/fs/gfs2/acl.c
+++ b/fs/gfs2/acl.c
@@ -116,6 +116,7 @@ int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	struct gfs2_holder gh;
 	bool need_unlock = false;
 	int ret;
+	umode_t mode;
 
 	if (acl && acl->a_count > GFS2_ACL_MAX_ENTRIES(GFS2_SB(inode)))
 		return -E2BIG;
@@ -130,17 +131,19 @@ 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);
+	mode = inode->i_mode;
+	if (type == ACL_TYPE_ACCESS && acl) {
+		ret = posix_acl_update_mode(inode, &mode, &acl);
 		if (ret)
 			goto unlock;
-		if (mode != inode->i_mode)
-			mark_inode_dirty(inode);
 	}
 
 	ret = __gfs2_set_acl(inode, acl, type);
+	if (!ret && mode != inode->i_mode) {
+		inode->i_mode = mode;
+		mark_inode_dirty(inode);
+	}
 unlock:
 	if (need_unlock)
 		gfs2_glock_dq_uninit(&gh);
-- 
2.1.4

[Cluster-devel] [PATCH v2 2/2] gfs2: preserve i_mode if __gfs2_set_acl() fails

[Bob Peterson]

----- Original Message -----
| When changing a file's acl mask, __gfs2_set_acl() will first set the
| group bits of i_mode to the value of the mask, and only then set the
| actual extended attribute representing the new acl.
| 
| If the second part fails (due to lack of space, for example) and the
| file had no acl attribute to begin with, the system will from now on
| assume that the mask permission bits are actual group permission bits,
| potentially granting access to the wrong users.
| 
| Prevent this by only changing the inode mode after the acl has been set.
| 
| Signed-off-by: Ernesto A. Fern?ndez <ernesto.mnd.fernandez@gmail.com>
| ---
Hi,

Thanks. This is now pushed 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=309e8cda596f6552a32dd14b969ce9b17f837f2f

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 1/2] gfs2: don't return ENODATA in __gfs2_xattr_set unless replacing

[Bob Peterson]

----- Original Message -----
| The function __gfs2_xattr_set() will return -ENODATA when called to
| remove a xattr that does not exist. The result is that setfacl will
| show an exit status of 1 when called to set only a file's mode bits
| (on a file with no ACLs), despite succeeding. A "No data available"
| error will be printed as well.
| 
| To fix this return 0 instead, except when the XATTR_REPLACE flag is
| set, in which case -ENODATA is appropriate. This is consistent with
| how most other xattr setting functions work, in other filesystems.
| 
| Signed-off-by: Ernesto A. Fern?ndez <ernesto.mnd.fernandez@gmail.com>
| ---
Hi,

Thanks. This is now pushed 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=54aae14beee6a6e9f72358f1873b3e497029c41d

Regards,

Bob Peterson
Red Hat File Systems