gfs2: don't return ENODATA in __gfs2_xattr_set unless replacing

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.

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

index 5417955..8ca56a7 100644
@@ -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

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.

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

index 2524807..06bbb6b 100644
@@ -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

----- 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 <[email protected]>
| ---

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

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.

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

index a6d9623..9d5eecb 100644
@@ -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

----- 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 <[email protected]>
| ---

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

----- 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 <[email protected]>
| ---

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