xfs: preserve i_mode if __xfs_set_acl() fails

PATCH In review Ernesto A. Fernández 4 participants 18 messages Aug 14, 2017 – Dec 7, 2017 XFS filesystem

Share & tools

Ernesto A. Fernández

author

2017-08-14 08:18 UTC #0 lore raw

PATCH 1/1 xfs: preserve i_mode if __xfs_set_acl() fails

When changing a file's ACL mask, xfs_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.

To prevent this, perform the mode update and the ACL update as part of the same transaction, and restore the original mode to the vfs inode in case of failure. This requires a change in how extended attributes are
deleted: commit the transaction even if there was no attribute to remove, to log the standard inode fields.

Signed-off-by Ernesto A. Fernández <[email protected]>

---
This issue is covered by generic/449 in xfstests. Several other filesystems are affected by this, some of them have already applied patches:
- 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
The test won't exactly pass after this patch is applied. It will fail differently, this time claiming that the filesystem is inconsistent. This probably has something to do with setting too many extended attributes, as it goes away if you change the attribute size in the test from 1k to 64k.

 fs/xfs/libxfs/xfs_attr.c | 13 +++++++++++--
 fs/xfs/xfs_acl.c         | 39 ++++++++++++++-------------------------
 2 files changed, 25 insertions(+), 27 deletions(-)

fs/xfs/libxfs/xfs_attr.c +11 -2

index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;
 
 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);

@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);
 
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
 
 	/*

@@ -468,6 +474,9 @@ xfs_attr_remove(
 	error = xfs_trans_commit(args.trans);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
+	if (!error && noattr)
+		error = -ENOATTR;
+
 	return error;
 
 out:

fs/xfs/xfs_acl.c +14 ~~-25~~

index 7034e17..1fe4363 100644
@@ -226,28 +226,13 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	return error;
 }
 
-static int
-xfs_set_mode(struct inode *inode, umode_t mode)
-{
-	int error = 0;
-
-	if (mode != inode->i_mode) {
-		struct iattr iattr;
-
-		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
-		iattr.ia_mode = mode;
-		iattr.ia_ctime = current_time(inode);
-
-		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
-	}
-
-	return error;
-}
-
 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int error = 0;
+	int mode_changed = 0;
+	umode_t old_mode = inode->i_mode;
+	struct timespec old_ctime = inode->i_ctime;
 
 	if (!acl)
 		goto set_acl;

@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);
-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}
 
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}
+	return error;
 }

Signature · 1 line

2.1.4

Darrick J. Wong

2017-08-14 22:28 UTC #1 lore raw

Quoted 16 lines · Ernesto A. Fernández

When changing a file's ACL mask, xfs_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.
To prevent this, perform the mode update and the ACL update as part of the same transaction, and restore the original mode to the vfs inode in case of failure. This requires a change in how extended attributes are
deleted: commit the transaction even if there was no attribute to remove, to log the standard inode fields.
Signed-off-by Ernesto A. Fernández <[email protected]>

Quoted 9 lines

---
This issue is covered by generic/449 in xfstests. Several other filesystems are affected by this, some of them have already applied patches:
- 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
The test won't exactly pass after this patch is applied. It will fail differently, this time claiming that the filesystem is inconsistent. This

I disagree that corrupting the filesystem is an acceptable strategy for dealing with insufficient space to handle setfacl.

Quoted 2 lines

probably has something to do with setting too many extended attributes, as it goes away if you change the attribute size in the test from 1k to 64k.

Hadn't you better look into why it does that and handle it?

--D

Quoted 121 lines

 fs/xfs/libxfs/xfs_attr.c | 13 +++++++++++--
 fs/xfs/xfs_acl.c         | 39 ++++++++++++++-------------------------
 2 files changed, 25 insertions(+), 27 deletions(-)

fs/xfs/libxfs/xfs_attr.c +11 -2

index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;
 
 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);

@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);
 
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
 
 	/*

@@ -468,6 +474,9 @@ xfs_attr_remove(
 	error = xfs_trans_commit(args.trans);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
+	if (!error && noattr)
+		error = -ENOATTR;
+
 	return error;
 
 out:

fs/xfs/xfs_acl.c +14 ~~-25~~

index 7034e17..1fe4363 100644
@@ -226,28 +226,13 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	return error;
 }
 
-static int
-xfs_set_mode(struct inode *inode, umode_t mode)
-{
-	int error = 0;
-
-	if (mode != inode->i_mode) {
-		struct iattr iattr;
-
-		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
-		iattr.ia_mode = mode;
-		iattr.ia_ctime = current_time(inode);
-
-		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
-	}
-
-	return error;
-}
-
 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int error = 0;
+	int mode_changed = 0;
+	umode_t old_mode = inode->i_mode;
+	struct timespec old_ctime = inode->i_ctime;
 
 	if (!acl)
 		goto set_acl;

@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);
-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}
 
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}
+	return error;
 }

Signature · 6 lines

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ernesto A. Fernández

author

2017-08-15 00:58 UTC #2 lore raw

Quoted 11 lines · Darrick J. Wong

Quoted 2 lines · Ernesto A. Fernández
The test won't exactly pass after this patch is applied. It will fail differently, this time claiming that the filesystem is inconsistent. This
I disagree that corrupting the filesystem is an acceptable strategy for dealing with insufficient space to handle setfacl.
Quoted 2 lines
probably has something to do with setting too many extended attributes, as it goes away if you change the attribute size in the test from 1k to 64k.
Hadn't you better look into why it does that and handle it?

Hi Darrick, thank for your quick reply.

I think I failed to explain myself properly. The inconsistency issue is NOT being caused by my patch. It was already there, and my patch just makes it visible. I'll try to be more clear. The test generic/449 essentially goes like this:

1) It sets as many extended attributes as possible so the filesystem will run out of space for the ACL.
2) It tries to set the ACL, which will fail because of (1).
3) It checks that the mode was not altered by the failed setfacl.
4) It checks that the filesystem was not corrupted.

The filesystem becomes corrupted after step (1). Before my patch, the test was failing in step (3), so step (4) was never run. Once the ACL issue is fixed, step (3) will pass and the test will get to (4). So now the test announces the inconsistency, while before it was silent about it.

If the test is run without applying my patch, but changing it slightly so it passes step (3), the end result will be the same: inconsistent filesystem. A simple way to try this is to comment out the whole if block that deals with the call to setfacl.

So whatever this issue is, it seems to be a general problem with extended attributes in xfs. It has nothing to do with the bug my patch intends to fix, other than the fact that generic/449 accidentally checks for both problems.

My suggestion to adjust the attribute size in the test was so that we could focus on the issue at hand, not so we could ignore the other one. Of course it needs to be fixed as well. I will try to figure out what is going on and hopefully send another patch. But both issues are independent, so please take a look at my patch on its own merits. It does fix the ACL bug even if the test does not pass.

Thank you for your attention,
Ernest

Dave Chinner

2017-08-15 02:00 UTC #3 lore raw

In reply to Ernesto A. Fernández

fs/xfs/libxfs/xfs_attr.c xfs_attr_remove( · line 398

Quoted 9 lines · Ernesto A. Fernández

fs/xfs/libxfs/xfs_attr.c +1 -0

index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;

bool.

And make it "bool has_attr = true" to avoid the nasty double negative of "!noattr" later on.

Quoted 18 lines

fs/xfs/libxfs/xfs_attr.c +6 -1

 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);

fs/xfs/libxfs/xfs_attr.c xfs_attr_remove( · line 468

Quoted 6 lines

fs/xfs/libxfs/xfs_attr.c +1 -1

@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);
 
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);

Why?

Quoted 27 lines

fs/xfs/libxfs/xfs_attr.c +3 -0

/*

@@ -468,6 +474,9 @@ xfs_attr_remove(
 	error = xfs_trans_commit(args.trans);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
+	if (!error && noattr)
+		error = -ENOATTR;
+
 	return error;
 
 out:

fs/xfs/xfs_acl.c +0 -7

index 7034e17..1fe4363 100644
@@ -226,28 +226,13 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	return error;
 }
 
-static int
-xfs_set_mode(struct inode *inode, umode_t mode)
-{
-	int error = 0;
-
-	if (mode != inode->i_mode) {
-		struct iattr iattr;

fs/xfs/xfs_acl.c __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 245

Quoted 10 lines

fs/xfs/xfs_acl.c +0 ~~-10~~

-
-		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
-		iattr.ia_mode = mode;
-		iattr.ia_ctime = current_time(inode);
-
-		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
-	}
-
-	return error;
-}

Ok, so that does an atomic, transactional change to the inode mode....

Quoted 17 lines

fs/xfs/xfs_acl.c +3 -3

 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int error = 0;
+	int mode_changed = 0;
+	umode_t old_mode = inode->i_mode;
+	struct timespec old_ctime = inode->i_ctime;
 
 	if (!acl)
 		goto set_acl;

@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);

fs/xfs/xfs_acl.c xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 251

Quoted 10 lines

fs/xfs/xfs_acl.c +4 -3

-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}

And this doesn't.

So this breaks all our rules for inode metadata updates, both for runtime access serialisation and for crash consistency.

fs/xfs/xfs_acl.c xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 259

Quoted 8 lines

fs/xfs/xfs_acl.c +6 -1

  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}

Same here.

IOWs, you can't just remove a transactional modification of inode state and replace it with in-memory VFS inode level changes. You need to do the changes in transaction context and with the correct locks held.

IOWs, if you need to undo the mode changes xfs_set_mode() did, you need to call xfs_set_mode() again. And because modifications have been made, the new ctime should remain in place, too.

Of course, this still isn't atomic from a runtime access POV, but that's substantially harder to provide than just undoing the mode change on ACL attribute save error.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Ernesto A. Fernández

author

2017-08-15 06:19 UTC #4 lore raw

Hi Dave, thanks for the review!

Quoted 13 lines · Dave Chinner

fs/xfs/libxfs/xfs_attr.c xfs_attr_remove( · line 398
Quoted 9 lines · Ernesto A. Fernández
fs/xfs/libxfs/xfs_attr.c +1 -0
index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;
bool.

Quoted 2 lines

And make it "bool has_attr = true" to avoid the nasty double negative of "!noattr" later on.

I called it noattr because the error was -ENOATTR, but you are right, has_attr is better. I assume you would prefer mode_changed to be a bool as well?

Quoted 22 lines

fs/xfs/libxfs/xfs_attr.c +6 -1

 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);

@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);

Quoted 5 lines

fs/xfs/libxfs/xfs_attr.c xfs_attr_remove( · line 468
Quoted 3 lines
fs/xfs/libxfs/xfs_attr.c +1 -1
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
Why?

Because we don't want the ctime to change when you try to delete an attribute that didn't exist in the first place. At least that's how it went before, so I tried to keep it the same.

Quoted 72 lines

Quoted 28 lines

fs/xfs/libxfs/xfs_attr.c +3 -0

/*

@@ -468,6 +474,9 @@ xfs_attr_remove(
 	error = xfs_trans_commit(args.trans);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
+	if (!error && noattr)
+		error = -ENOATTR;
+
 	return error;
 
 out:

fs/xfs/xfs_acl.c +0 -7

index 7034e17..1fe4363 100644
@@ -226,28 +226,13 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	return error;
 }
 
-static int
-xfs_set_mode(struct inode *inode, umode_t mode)
-{
-	int error = 0;
-
-	if (mode != inode->i_mode) {
-		struct iattr iattr;

fs/xfs/xfs_acl.c __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 245

Quoted 10 lines

fs/xfs/xfs_acl.c +0 ~~-10~~

-
-		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
-		iattr.ia_mode = mode;
-		iattr.ia_ctime = current_time(inode);
-
-		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
-	}
-
-	return error;
-}

Ok, so that does an atomic, transactional change to the inode mode....

Quoted 17 lines

fs/xfs/xfs_acl.c +3 -3

 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int error = 0;
+	int mode_changed = 0;
+	umode_t old_mode = inode->i_mode;
+	struct timespec old_ctime = inode->i_ctime;
 
 	if (!acl)
 		goto set_acl;

@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);

fs/xfs/xfs_acl.c xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 251

Quoted 10 lines

fs/xfs/xfs_acl.c +4 -3

-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}

And this doesn't.

Quoted 3 lines

So this breaks all our rules for inode metadata updates, both for runtime access serialisation and for crash consistency.

I'm only making changes to the vfs inode, and xfs_set_acl() should always be called with i_rwsem held for writing. I think that should be enough to serialize access to the vfs inode fields.

I'm sorry, I'm not sure I follow the crash consistency part. This is not writing anything to disk. My expectation was that the changes I made to the vfs inode would be attached to the transaction later, when calling xfs_trans_log_inode() in xfs_attr_set() or xfs_attr_remove(). Would that not work as intended? I was mostly imitating how btrfs sets an ACL, so I could be misunderstanding how xfs does journaling.

Quoted 10 lines

fs/xfs/xfs_acl.c xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 259
Quoted 8 lines
fs/xfs/xfs_acl.c +6 -1
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}
Same here.

i_rwsem would still be held here. My reasoning is that, since there was an error, no changes were actually committed; so there would be no need to commit the restoration either, just restore the old in-memory vfs inode fields. Once again, this is inspired by btrfs, so I suppose it may fail here in some way I can't see.

(I should probably clarify I did run tests before submitting)

Quoted 7 lines

IOWs, you can't just remove a transactional modification of inode state and replace it with in-memory VFS inode level changes. You need to do the changes in transaction context and with the correct locks held.
IOWs, if you need to undo the mode changes xfs_set_mode() did, you need to call xfs_set_mode() again. And because modifications have

But that could fail as well, and how would we handle that? If this bug is to be fixed the whole thing needs to be a single transaction. That's why I removed the xfs_set_mode() function in the first place.

Quoted 12 lines

been made, the new ctime should remain in place, too.
Of course, this still isn't atomic from a runtime access POV, but that's substantially harder to provide than just undoing the mode change on ACL attribute save error.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner
[email protected]

Thanks,
Ernest

Dave Chinner

2017-08-15 08:44 UTC #5 lore raw

Quoted 51 lines · Ernesto A. Fernández

Hi Dave, thanks for the review!
Quoted 13 lines · Dave Chinner
fs/xfs/libxfs/xfs_attr.c xfs_attr_remove( · line 398
Quoted 9 lines · Ernesto A. Fernández
fs/xfs/libxfs/xfs_attr.c +1 -0
index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;
bool.
Quoted 2 lines
And make it "bool has_attr = true" to avoid the nasty double negative of "!noattr" later on.
I called it noattr because the error was -ENOATTR, but you are right, has_attr is better. I assume you would prefer mode_changed to be a bool as well?
Quoted 22 lines
Quoted 22 lines
fs/xfs/libxfs/xfs_attr.c +6 -1
 	XFS_STATS_INC(mp, xs_attr_remove);
 
@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);
@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);
 
Quoted 5 lines
fs/xfs/libxfs/xfs_attr.c xfs_attr_remove( · line 468
Quoted 3 lines
fs/xfs/libxfs/xfs_attr.c +1 -1
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
Why?

Quoted 3 lines

Because we don't want the ctime to change when you try to delete an attribute that didn't exist in the first place. At least that's how it went before, so I tried to keep it the same.

Which means the logic is convoluted and no longer obvious as to what it does. i.e. the noattr case is running a transaction that dirties an inode that no modifications were made to. Alarm bells right there.

Quoted 20 lines

Quoted 7 lines

fs/xfs/libxfs/xfs_attr.c +0 -3

@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);

fs/xfs/libxfs/xfs_attr.c xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 251

Quoted 10 lines

fs/xfs/libxfs/xfs_attr.c +4 -3

-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}

And this doesn't.

Quoted 6 lines

Quoted 2 lines
So this breaks all our rules for inode metadata updates, both for runtime access serialisation and for crash consistency.
I'm only making changes to the vfs inode, and xfs_set_acl() should always be called with i_rwsem held for writing. I think that should be enough to serialize access to the vfs inode fields.

It's not. The i_rwsem does not protect XFS inode metadata, which is what needs changing here. i.e. we change both the VFS inode and the corresponding XFS inode metadata at the same time, under the same XFS_ILOCK_EXCL lock, only inside a transaction context.

That's the rules, no exceptions.

Quoted 2 lines

I'm sorry, I'm not sure I follow the crash consistency part. This is not writing anything to disk.

xfs_set_mode() runs a transaction that is journalled to change the inode mode. You break in-memory consistency by modifying the VFS inode state directly without holding the correct locks (see xfs_setattr_mode()), and you break the transactional change model (i.e. on-disk journal consistency and on-disk metadata change ordering) by making metadata changes this outside transaction contexts and without holding the correct locks.

Quoted 4 lines

My expectation was that the changes I made to
the vfs inode would be attached to the transaction later, when calling xfs_trans_log_inode() in xfs_attr_set() or xfs_attr_remove(). Would that not work as intended?

No, it wouldn't. You have to follow the filesystem specific update rules to change inode metadata, not make up your own....

Quoted 2 lines

I was mostly imitating how btrfs sets an ACL, so I could be misunderstanding how xfs does journaling.

... and btrfs has a completely different transaction model and implementation to XFS. IOWs, trying to do what works for btrfs in XFS code will break XFS. And vice versa.

Quoted 11 lines

Quoted 10 lines
fs/xfs/libxfs/xfs_attr.c xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 258
Quoted 8 lines
fs/xfs/libxfs/xfs_attr.c +6 -1
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}
Same here.

Quoted 7 lines

i_rwsem would still be held here. My reasoning is that, since there was an error, no changes were actually committed; so there would be no need to commit the restoration either, just restore the old in-memory vfs inode fields. Once again, this is inspired by btrfs, so I suppose it may fail here in some way I can't see.
(I should probably clarify I did run tests before submitting)

When you treat the code as a black box for testing, then end result might appear to be what you want. That doesn't mean the implementation is correct or that hidden transient states within the block box are incorrect or invalid.

Quoted 9 lines

Quoted 7 lines
IOWs, you can't just remove a transactional modification of inode state and replace it with in-memory VFS inode level changes. You need to do the changes in transaction context and with the correct locks held.
IOWs, if you need to undo the mode changes xfs_set_mode() did, you need to call xfs_set_mode() again. And because modifications have
But that could fail as well, and how would we handle that?

If the xattr change was not a fatal error (which will shut down the filesystem), then calling xfs_set_mode() should work just fine as it doesn't require any new space to be allocated (XFS != BTRFS - we can run overwriting transactions even at ENOSPC).

And you need to run the mode undo as a separate transaction because....

Quoted 2 lines

If this bug
is to be fixed the whole thing needs to be a single transaction.

.... this is simply not possible. Why? Because __xfs_set_acl() can run up three separate transactions internally in adding the new xattr. That means your unlogged vfs indoe change has already been logged to the journal and so we *require* a transaction to overwrite the new mode that has already been committed to the journal.

But I have to ask - why do we even need to modify the mode first? Why not change the ACL first, then modify the mode+timestamp? If setting the ACL fails, then we don't have anything to undo and all is good....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Ernesto A. Fernández

author

2017-08-15 19:29 UTC #6 lore raw

Quoted 163 lines · Dave Chinner

Quoted 51 lines · Ernesto A. Fernández
Hi Dave, thanks for the review!
Quoted 13 lines · Dave Chinner
fs/xfs/libxfs/xfs_attr.c xfs_attr_remove( · line 398
Quoted 9 lines · Ernesto A. Fernández
fs/xfs/libxfs/xfs_attr.c +1 -0
index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;
bool.
Quoted 2 lines
And make it "bool has_attr = true" to avoid the nasty double negative of "!noattr" later on.
I called it noattr because the error was -ENOATTR, but you are right, has_attr is better. I assume you would prefer mode_changed to be a bool as well?
Quoted 22 lines
Quoted 22 lines
fs/xfs/libxfs/xfs_attr.c +6 -1
 	XFS_STATS_INC(mp, xs_attr_remove);
 
@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);
@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);
 
Quoted 5 lines
fs/xfs/libxfs/xfs_attr.c xfs_attr_remove( · line 468
Quoted 3 lines
fs/xfs/libxfs/xfs_attr.c +1 -1
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
Why?
Quoted 3 lines
Because we don't want the ctime to change when you try to delete an attribute that didn't exist in the first place. At least that's how it went before, so I tried to keep it the same.
Which means the logic is convoluted and no longer obvious as to what it does. i.e. the noattr case is running a transaction that dirties an inode that no modifications were made to. Alarm bells right there.
Quoted 20 lines
Quoted 20 lines
Quoted 7 lines
fs/xfs/libxfs/xfs_attr.c +0 -3
@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);
fs/xfs/libxfs/xfs_attr.c xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 251
Quoted 10 lines
fs/xfs/libxfs/xfs_attr.c +4 -3
-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}
And this doesn't.
Quoted 6 lines
Quoted 2 lines
So this breaks all our rules for inode metadata updates, both for runtime access serialisation and for crash consistency.
I'm only making changes to the vfs inode, and xfs_set_acl() should always be called with i_rwsem held for writing. I think that should be enough to serialize access to the vfs inode fields.
It's not. The i_rwsem does not protect XFS inode metadata, which is what needs changing here. i.e. we change both the VFS inode and the corresponding XFS inode metadata at the same time, under the same XFS_ILOCK_EXCL lock, only inside a transaction context.
That's the rules, no exceptions.
Quoted 2 lines
I'm sorry, I'm not sure I follow the crash consistency part. This is not writing anything to disk.
xfs_set_mode() runs a transaction that is journalled to change the inode mode. You break in-memory consistency by modifying the VFS inode state directly without holding the correct locks (see xfs_setattr_mode()), and you break the transactional change model (i.e. on-disk journal consistency and on-disk metadata change ordering) by making metadata changes this outside transaction contexts and without holding the correct locks.
Quoted 4 lines
My expectation was that the changes I made to
the vfs inode would be attached to the transaction later, when calling xfs_trans_log_inode() in xfs_attr_set() or xfs_attr_remove(). Would that not work as intended?
No, it wouldn't. You have to follow the filesystem specific update rules to change inode metadata, not make up your own....
Quoted 2 lines
I was mostly imitating how btrfs sets an ACL, so I could be misunderstanding how xfs does journaling.
... and btrfs has a completely different transaction model and implementation to XFS. IOWs, trying to do what works for btrfs in XFS code will break XFS. And vice versa.
Quoted 11 lines
Quoted 10 lines
fs/xfs/libxfs/xfs_attr.c xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) · line 258
Quoted 8 lines
fs/xfs/libxfs/xfs_attr.c +6 -1
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}
Same here.
Quoted 7 lines
i_rwsem would still be held here. My reasoning is that, since there was an error, no changes were actually committed; so there would be no need to commit the restoration either, just restore the old in-memory vfs inode fields. Once again, this is inspired by btrfs, so I suppose it may fail here in some way I can't see.
(I should probably clarify I did run tests before submitting)
When you treat the code as a black box for testing, then end result might appear to be what you want. That doesn't mean the implementation is correct or that hidden transient states within the block box are incorrect or invalid.
Quoted 9 lines
Quoted 7 lines
IOWs, you can't just remove a transactional modification of inode state and replace it with in-memory VFS inode level changes. You need to do the changes in transaction context and with the correct locks held.
IOWs, if you need to undo the mode changes xfs_set_mode() did, you need to call xfs_set_mode() again. And because modifications have
But that could fail as well, and how would we handle that?
If the xattr change was not a fatal error (which will shut down the filesystem), then calling xfs_set_mode() should work just fine as it doesn't require any new space to be allocated (XFS != BTRFS - we can run overwriting transactions even at ENOSPC).
And you need to run the mode undo as a separate transaction because....

Quoted 8 lines

Quoted 2 lines
If this bug
is to be fixed the whole thing needs to be a single transaction.
.... this is simply not possible. Why? Because __xfs_set_acl() can run up three separate transactions internally in adding the new xattr. That means your unlogged vfs indoe change has already been logged to the journal and so we *require* a transaction to overwrite the new mode that has already been committed to the journal.

It's clear that I don't understand xfs, and I'm causing more trouble than I'm helping. I should leave this issue to you guys.

Quoted 4 lines

But I have to ask - why do we even need to modify the mode first? Why not change the ACL first, then modify the mode+timestamp? If setting the ACL fails, then we don't have anything to undo and all is good....

I intended for the mode to be committed as part of the same transaction that sets or removes the ACL. In my mind making the changes later, as part of a separate transaction, would have meant that a crash between the two left the filesystem in an inconsistent state, with a new ACL but without the corresponding mode bits.

Of course that was based on a poor understanding of xfs journaling, so now I don't know.

Quoted 6 lines

Cheers,
Dave.
Signature · 2 lines
Dave Chinner
[email protected]

Thanks,
Ernest

Dave Chinner

2017-08-16 00:25 UTC #7 lore raw

Quoted 11 lines · Ernesto A. Fernández

Quoted 5 lines · Dave Chinner
On Tue, Aug 15, 2017 at 03:18:58AM -0300, Ernesto A. Fernández wrote:
But I have to ask - why do we even need to modify the mode first? Why not change the ACL first, then modify the mode+timestamp? If setting the ACL fails, then we don't have anything to undo and all is good....
I intended for the mode to be committed as part of the same transaction that sets or removes the ACL. In my mind making the changes later, as part of a separate transaction, would have meant that a crash between the two left the filesystem in an inconsistent state,

No, it will not leave the fileystem in an inconsistent state. It will leave the inode permissions in an /unwanted/ state, but there is no filesystem metadata inconsistency.

Quoted 2 lines

with a new ACL but without
the corresponding mode bits.

Yup, but that's no different from right now, where a crash after setting the mode bits could be applied but the ACL update is missing.

Either way is even rarely than "crash at the wrong time" implies, because we've also got to have a complete journal checkpoint occur between the two operations and then crash between the checkpoint and the second operation. Yes, it's possible, but in the entire time I've been working on XFS (almost 15 years now) I can count on one hand the number of times such a problem has occurred and been reported...

So, it's a rare problem, and one that will get solved in time because there's much more to solving the problem than just this case. e.g. I worte this in 2008:

http://xfs.org/index.php/Improving_Metadata_Performance_By_Reducing_Journal_Overhead#Atomic_Multi-Transaction_Operations

And we've really only got the infrastructure we could use to implement this in a widespread manner with the rmap/reflink functionality. But implementing it will require a large amount of re-organisation of filesystem operations, so it's something that will take time to roll out.

With that in mind, here's waht I suggested above: set the mode after the xattr. I haven't tested it - can you check it solves the problem case you are testing?

Cheers,

Dave.

Signature · 11 lines

Dave Chinner
[email protected]


xfs: don't change inode mode if ACL update fails

XXX: untested

---
 fs/xfs/xfs_acl.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

fs/xfs/xfs_acl.c +16 -6

index 7034e17535de..3354140de07e 100644
@@ -247,6 +247,8 @@ xfs_set_mode(struct inode *inode, umode_t mode)
 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
+	umode_t mode;
+	bool set_mode = false;
 	int error = 0;
 
 	if (!acl)

@@ -257,16 +259,24 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
 		error = posix_acl_update_mode(inode, &mode, &acl);
 		if (error)
 			return error;
-		error = xfs_set_mode(inode, mode);
-		if (error)
-			return error;
+		set_mode = true;
 	}
 
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error =  __xfs_set_acl(inode, acl, type);
+	if (error)
+		return error;
+
+	/*
+	 * We set the mode after successfully updating the ACL xattr because the
+	 * xattr update can fail at ENOSPC and we don't want to change the mode
+	 * if the ACL update hasn't been applied.
+	 */
+	if (set_mode)
+		error = xfs_set_mode(inode, mode);
+
+	return error;
 }

Ernesto A. Fernández

author

2017-08-16 07:16 UTC #8 lore raw

Quoted 36 lines · Dave Chinner

Quoted 11 lines · Ernesto A. Fernández
Quoted 5 lines · Dave Chinner
On Tue, Aug 15, 2017 at 03:18:58AM -0300, Ernesto A. Fernández wrote:
But I have to ask - why do we even need to modify the mode first? Why not change the ACL first, then modify the mode+timestamp? If setting the ACL fails, then we don't have anything to undo and all is good....
I intended for the mode to be committed as part of the same transaction that sets or removes the ACL. In my mind making the changes later, as part of a separate transaction, would have meant that a crash between the two left the filesystem in an inconsistent state,
No, it will not leave the fileystem in an inconsistent state. It will leave the inode permissions in an /unwanted/ state, but there is no filesystem metadata inconsistency.
Quoted 2 lines
with a new ACL but without
the corresponding mode bits.
Yup, but that's no different from right now, where a crash after setting the mode bits could be applied but the ACL update is missing.
Either way is even rarely than "crash at the wrong time" implies, because we've also got to have a complete journal checkpoint occur between the two operations and then crash between the checkpoint and the second operation. Yes, it's possible, but in the entire time I've been working on XFS (almost 15 years now) I can count on one hand the number of times such a problem has occurred and been reported...
So, it's a rare problem, and one that will get solved in time because there's much more to solving the problem than just this case. e.g. I worte this in 2008:

Quoted 7 lines

http://xfs.org/index.php/Improving_Metadata_Performance_By_Reducing_Journal_Overhead#Atomic_Multi-Transaction_Operations
And we've really only got the infrastructure we could use to implement this in a widespread manner with the rmap/reflink functionality. But implementing it will require a large amount of re-organisation of filesystem operations, so it's something that will take time to roll out.

Alright, thanks for the explanation.

Quoted 3 lines

With that in mind, here's waht I suggested above: set the mode after the xattr. I haven't tested it - can you check it solves the problem case you are testing?

It does. Of course the test still fails, as I said before, now claiming that the filesystem is inconsistent. But that's a separate issue.

Quoted 60 lines

Cheers,

Dave.

Signature · 11 lines

Dave Chinner
[email protected]


xfs: don't change inode mode if ACL update fails

XXX: untested

---
 fs/xfs/xfs_acl.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

fs/xfs/xfs_acl.c +16 -6

index 7034e17535de..3354140de07e 100644
@@ -247,6 +247,8 @@ xfs_set_mode(struct inode *inode, umode_t mode)
 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
+	umode_t mode;
+	bool set_mode = false;
 	int error = 0;
 
 	if (!acl)

@@ -257,16 +259,24 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
 		error = posix_acl_update_mode(inode, &mode, &acl);
 		if (error)
 			return error;
-		error = xfs_set_mode(inode, mode);
-		if (error)
-			return error;
+		set_mode = true;
 	}
 
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error =  __xfs_set_acl(inode, acl, type);
+	if (error)
+		return error;
+
+	/*
+	 * We set the mode after successfully updating the ACL xattr because the
+	 * xattr update can fail at ENOSPC and we don't want to change the mode
+	 * if the ACL update hasn't been applied.
+	 */
+	if (set_mode)
+		error = xfs_set_mode(inode, mode);
+
+	return error;
 }

Dave Chinner

2017-08-16 13:35 UTC #9 lore raw

Quoted 45 lines · Ernesto A. Fernández

Quoted 36 lines · Dave Chinner
Quoted 11 lines · Ernesto A. Fernández
Quoted 5 lines · Dave Chinner
On Tue, Aug 15, 2017 at 03:18:58AM -0300, Ernesto A. Fernández wrote:
But I have to ask - why do we even need to modify the mode first? Why not change the ACL first, then modify the mode+timestamp? If setting the ACL fails, then we don't have anything to undo and all is good....
I intended for the mode to be committed as part of the same transaction that sets or removes the ACL. In my mind making the changes later, as part of a separate transaction, would have meant that a crash between the two left the filesystem in an inconsistent state,
No, it will not leave the fileystem in an inconsistent state. It will leave the inode permissions in an /unwanted/ state, but there is no filesystem metadata inconsistency.
Quoted 2 lines
with a new ACL but without
the corresponding mode bits.
Yup, but that's no different from right now, where a crash after setting the mode bits could be applied but the ACL update is missing.
Either way is even rarely than "crash at the wrong time" implies, because we've also got to have a complete journal checkpoint occur between the two operations and then crash between the checkpoint and the second operation. Yes, it's possible, but in the entire time I've been working on XFS (almost 15 years now) I can count on one hand the number of times such a problem has occurred and been reported...
So, it's a rare problem, and one that will get solved in time because there's much more to solving the problem than just this case. e.g. I worte this in 2008:
Quoted 7 lines
http://xfs.org/index.php/Improving_Metadata_Performance_By_Reducing_Journal_Overhead#Atomic_Multi-Transaction_Operations
And we've really only got the infrastructure we could use to implement this in a widespread manner with the rmap/reflink functionality. But implementing it will require a large amount of re-organisation of filesystem operations, so it's something that will take time to roll out.

Quoted 8 lines

Alright, thanks for the explanation.
Quoted 3 lines
With that in mind, here's waht I suggested above: set the mode after the xattr. I haven't tested it - can you check it solves the problem case you are testing?
It does. Of course the test still fails, as I said before, now claiming that the filesystem is inconsistent. But that's a separate issue.

It shouldn't be - what's the error that is being reported?

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Ernesto A. Fernández

author

2017-08-16 19:31 UTC #10 lore raw

Quoted 10 lines · Dave Chinner

Quoted 7 lines · Ernesto A. Fernández
Quoted 3 lines · Dave Chinner
With that in mind, here's waht I suggested above: set the mode after the xattr. I haven't tested it - can you check it solves the problem case you are testing?
It does. Of course the test still fails, as I said before, now claiming that the filesystem is inconsistent. But that's a separate issue.
It shouldn't be - what's the error that is being reported?

xfs_repair, phase 3:
bad magic number febe in block 512 (14452) for directory inode 131 problem with attribute contents in inode 131
would clear attr fork
bad nblocks 11900 for inode 131, would reset to 11274 bad anextents 1 for inode 131, would reset to 0

This is not a result of your patch, it was already happening before. It seems to be caused by setting too many extended attributes to a file.

Quoted 7 lines

Cheers,
Dave.
Signature · 2 lines
Dave Chinner
[email protected]

Dave Chinner

2017-08-17 00:00 UTC #11 lore raw

Quoted 7 lines · Ernesto A. Fernández

Quoted 6 lines · Dave Chinner
Quoted 5 lines · Ernesto A. Fernández
Quoted 3 lines · Dave Chinner
With that in mind, here's waht I suggested above: set the mode after the xattr. I haven't tested it - can you check it solves the problem case you are testing?

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
It does. Of course the test still fails, as I said before, now claiming that the filesystem is inconsistent. But that's a separate issue.
It shouldn't be - what's the error that is being reported?
xfs_repair, phase 3:
bad magic number febe in block 512 (14452) for directory inode 131

Hmmmm - that's a magic number for a non-crc DA node. Kinda implies that it ENOSPC'd in the middle of a tree split.

Can you test on a CRC enabled filesystem and see if there are any other errors that are detected either at runtime or by repair? If it does fail, can youpost the full output of xfs_repair?

Quoted 2 lines

It seems to be caused by setting too many extended attributes to a file.

Yeah, > 500 blocks in the attribute tree implies at least several megabytes of xattrs on a file, which is something that pretty much never happens on production workloads. It's not likely to be a frequently exercised path...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Ernesto A. Fernández

author

2017-08-17 05:34 UTC #12 lore raw

Quoted 9 lines · Dave Chinner

Quoted 2 lines · Ernesto A. Fernández
xfs_repair, phase 3:
bad magic number febe in block 512 (14452) for directory inode 131
Hmmmm - that's a magic number for a non-crc DA node. Kinda implies that it ENOSPC'd in the middle of a tree split.
Can you test on a CRC enabled filesystem and see if there are any other errors that are detected either at runtime or by repair?

No other errors, no.

Quoted 1 line

If it does fail, can youpost the full output of xfs_repair?

Of course. I don't think it will be of much help though:

Phase 1 - find and verify superblock...
Phase 2 - using internal log
- scan filesystem freespace and inode maps...
- found root inode chunk
Phase 3 - for each AG...
- scan (but don't clear) agi unlinked lists...
- process known inodes and perform inode discovery...
- agno = 0
bad magic number 3ebe in block 506 (14446) for directory inode 67 problem with attribute contents in inode 67
would clear attr fork
bad nblocks 11894 for inode 67, would reset to 11268 bad anextents 1 for inode 67, would reset to 0
- agno = 1
- process newly discovered inodes...
Phase 4 - check for duplicate blocks...
- setting up duplicate extent list...
- check for inodes claiming duplicate blocks...
- agno = 0
- agno = 1
No modify flag set, skipping phase 5
Phase 6 - check inode connectivity...
- traversing filesystem ...
- traversal finished ...
- moving disconnected inodes to lost+found ...
Phase 7 - verify link counts...
No modify flag set, skipping filesystem flush and exiting.

Quoted 14 lines

Quoted 2 lines
It seems to be caused by setting too many extended attributes to a file.
Yeah, > 500 blocks in the attribute tree implies at least several megabytes of xattrs on a file, which is something that pretty much never happens on production workloads. It's not likely to be a frequently exercised path...
Cheers,
Dave.
Signature · 2 lines
Dave Chinner
[email protected]

Perhaps this can be of some use:

In my test I'm setting as many xattrs with 1k values as possible. If I change that to 64k values this bug is no longer seen.

The cutoff seems to be when the name+value of the xattr are above 3072+1 bytes. This does not depend on the amount of free space of the filesystem, so this is probably not just about the number of xattrs as I first thought.

If I change the block size to 2048 the cutoff is now 1537 bytes, that is, still 3/4 the block size plus one. For bsize=1024, the cutoff is when the value alone is longer than 756 bytes.

Also you don't actually need to max out the number of xattrs to see this bug. For example: if the block size is 1024, the value size is 757 and the name size is 18 bytes, you can trigger this bug by setting 2010 xattrs.

Let me know if you need more info.

Ernest

Dave Chinner

2017-08-17 06:34 UTC #13 lore raw

Quoted 18 lines · Ernesto A. Fernández

Quoted 9 lines · Dave Chinner
Quoted 2 lines · Ernesto A. Fernández
xfs_repair, phase 3:
bad magic number febe in block 512 (14452) for directory inode 131
Hmmmm - that's a magic number for a non-crc DA node. Kinda implies that it ENOSPC'd in the middle of a tree split.
Can you test on a CRC enabled filesystem and see if there are any other errors that are detected either at runtime or by repair?
No other errors, no.
Quoted 1 line
If it does fail, can youpost the full output of xfs_repair?
Of course. I don't think it will be of much help though:

Quoted 9 lines

Phase 1 - find and verify superblock...
Phase 2 - using internal log
- scan filesystem freespace and inode maps...
- found root inode chunk
Phase 3 - for each AG...
- scan (but don't clear) agi unlinked lists...
- process known inodes and perform inode discovery...
- agno = 0
bad magic number 3ebe in block 506 (14446) for directory inode 67

Ok, so it's a specific change in tree size that is causing the problem. It's probably a tree height change opertaion....

Quoted 4 lines

Perhaps this can be of some use:
In my test I'm setting as many xattrs with 1k values as possible. If I change that to 64k values this bug is no longer seen.

Of course. Large xattrs are stored remote to the attribute tree, so each attribute is only consuming ~30-40 bytes in the attribute tree rather than 1k.

Quoted 2 lines

The cutoff seems to be when the name+value of the xattr are above 3072+1 bytes.

Which is exactly 75% of the attribute tree block size. i.e. the size that attribute data is stored remotely instead of in the tree itself.

Quoted 3 lines

This does not depend on the amount of free space of the filesystem, so this is probably not just about the number of xattrs as I first thought.

Given it follows the block size, it smells of a btree level change going wrong. Haven't go ttime to look right now, but if you could look at the inode in xfs_db and print the state of the root attr fork and the root attr block header before running xfs_repair in the filesystem that would probably tell us..

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Bill O'Donnell

2017-12-07 17:31 UTC #14 lore raw

In reply to Ernesto A. Fernández

Quoted 15 lines · Ernesto A. Fernández

When changing a file's ACL mask, xfs_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.
To prevent this, perform the mode update and the ACL update as part of the same transaction, and restore the original mode to the vfs inode in case of failure. This requires a change in how extended attributes are
deleted: commit the transaction even if there was no attribute to remove, to log the standard inode fields.

Quoted 3 lines

Signed-off-by Ernesto A. Fernández <[email protected]>
---
This issue is covered by generic/449 in xfstests. Several other filesystems

Sorry, I'm coming into this late, and might be missing something... is there a particular xfs configuration that you've set up on which generic/449 fails? On 4.15-rc1, I see no failures of g/449.

Thanks-
Bill

Quoted 130 lines

are affected by this, some of them have already applied patches:
- 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
The test won't exactly pass after this patch is applied. It will fail differently, this time claiming that the filesystem is inconsistent. This probably has something to do with setting too many extended attributes, as it goes away if you change the attribute size in the test from 1k to 64k.

 fs/xfs/libxfs/xfs_attr.c | 13 +++++++++++--
 fs/xfs/xfs_acl.c         | 39 ++++++++++++++-------------------------
 2 files changed, 25 insertions(+), 27 deletions(-)

fs/xfs/libxfs/xfs_attr.c +11 -2

index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;
 
 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);

@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);
 
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
 
 	/*

@@ -468,6 +474,9 @@ xfs_attr_remove(
 	error = xfs_trans_commit(args.trans);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
+	if (!error && noattr)
+		error = -ENOATTR;
+
 	return error;
 
 out:

fs/xfs/xfs_acl.c +14 ~~-25~~

index 7034e17..1fe4363 100644
@@ -226,28 +226,13 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	return error;
 }
 
-static int
-xfs_set_mode(struct inode *inode, umode_t mode)
-{
-	int error = 0;
-
-	if (mode != inode->i_mode) {
-		struct iattr iattr;
-
-		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
-		iattr.ia_mode = mode;
-		iattr.ia_ctime = current_time(inode);
-
-		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
-	}
-
-	return error;
-}
-
 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int error = 0;
+	int mode_changed = 0;
+	umode_t old_mode = inode->i_mode;
+	struct timespec old_ctime = inode->i_ctime;
 
 	if (!acl)
 		goto set_acl;

@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);
-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}
 
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}
+	return error;
 }

Signature · 6 lines

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bill O'Donnell

2017-12-07 17:38 UTC #15 lore raw

Quoted 16 lines · Bill O'Donnell

Quoted 15 lines · Ernesto A. Fernández
When changing a file's ACL mask, xfs_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.
To prevent this, perform the mode update and the ACL update as part of the same transaction, and restore the original mode to the vfs inode in case of failure. This requires a change in how extended attributes are
deleted: commit the transaction even if there was no attribute to remove, to log the standard inode fields.

Quoted 7 lines

Quoted 3 lines
Signed-off-by Ernesto A. Fernández <[email protected]>
---
This issue is covered by generic/449 in xfstests. Several other filesystems
Sorry, I'm coming into this late, and might be missing something... is there a particular xfs configuration that you've set up on which generic/449 fails? On 4.15-rc1, I see no failures of g/449.

Oh, I see this was merged and fixes g/449:
commit 67f2ffe31d1a683170c2ba0ecc643e42a5fdd397

Author: Dave Chinner <[email protected]>
Date:   Mon Oct 9 11:37:23 2017 -0700

xfs: don't change inode mode if ACL update fails

Quoted 140 lines

Thanks-
Bill

Quoted 130 lines

 fs/xfs/libxfs/xfs_attr.c | 13 +++++++++++--
 fs/xfs/xfs_acl.c         | 39 ++++++++++++++-------------------------
 2 files changed, 25 insertions(+), 27 deletions(-)

fs/xfs/libxfs/xfs_attr.c +11 -2

index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;
 
 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);

@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);
 
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
 
 	/*

@@ -468,6 +474,9 @@ xfs_attr_remove(
 	error = xfs_trans_commit(args.trans);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
+	if (!error && noattr)
+		error = -ENOATTR;
+
 	return error;
 
 out:

fs/xfs/xfs_acl.c +14 ~~-25~~

index 7034e17..1fe4363 100644
@@ -226,28 +226,13 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	return error;
 }
 
-static int
-xfs_set_mode(struct inode *inode, umode_t mode)
-{
-	int error = 0;
-
-	if (mode != inode->i_mode) {
-		struct iattr iattr;
-
-		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
-		iattr.ia_mode = mode;
-		iattr.ia_ctime = current_time(inode);
-
-		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
-	}
-
-	return error;
-}
-
 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int error = 0;
+	int mode_changed = 0;
+	umode_t old_mode = inode->i_mode;
+	struct timespec old_ctime = inode->i_ctime;
 
 	if (!acl)
 		goto set_acl;

@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);
-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}
 
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}
+	return error;
 }

Signature · 6 lines

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Darrick J. Wong

2017-12-07 17:43 UTC #16 lore raw

In reply to Bill O'Donnell

Quoted 16 lines · Bill O'Donnell

Quoted 15 lines · Ernesto A. Fernández
When changing a file's ACL mask, xfs_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.
To prevent this, perform the mode update and the ACL update as part of the same transaction, and restore the original mode to the vfs inode in case of failure. This requires a change in how extended attributes are
deleted: commit the transaction even if there was no attribute to remove, to log the standard inode fields.

Quoted 7 lines

Quoted 3 lines
Signed-off-by Ernesto A. Fernández <[email protected]>
---
This issue is covered by generic/449 in xfstests. Several other filesystems
Sorry, I'm coming into this late, and might be missing something... is there a particular xfs configuration that you've set up on which generic/449 fails? On 4.15-rc1, I see no failures of g/449.

Fixed in:
67f2ffe31d1 ("xfs: don't change inode mode if ACL update fails")

(I think...)

--D

Quoted 139 lines

Thanks-
Bill

Quoted 130 lines

 fs/xfs/libxfs/xfs_attr.c | 13 +++++++++++--
 fs/xfs/xfs_acl.c         | 39 ++++++++++++++-------------------------
 2 files changed, 25 insertions(+), 27 deletions(-)

fs/xfs/libxfs/xfs_attr.c +11 -2

index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;
 
 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);

@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);
 
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
 
 	/*

@@ -468,6 +474,9 @@ xfs_attr_remove(
 	error = xfs_trans_commit(args.trans);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
+	if (!error && noattr)
+		error = -ENOATTR;
+
 	return error;
 
 out:

fs/xfs/xfs_acl.c +14 ~~-25~~

index 7034e17..1fe4363 100644
@@ -226,28 +226,13 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	return error;
 }
 
-static int
-xfs_set_mode(struct inode *inode, umode_t mode)
-{
-	int error = 0;
-
-	if (mode != inode->i_mode) {
-		struct iattr iattr;
-
-		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
-		iattr.ia_mode = mode;
-		iattr.ia_ctime = current_time(inode);
-
-		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
-	}
-
-	return error;
-}
-
 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int error = 0;
+	int mode_changed = 0;
+	umode_t old_mode = inode->i_mode;
+	struct timespec old_ctime = inode->i_ctime;
 
 	if (!acl)
 		goto set_acl;

@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);
-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}
 
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}
+	return error;
 }

Signature · 6 lines

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Bill O'Donnell

2017-12-07 17:51 UTC #17 lore raw

Quoted 21 lines · Darrick J. Wong

Quoted 20 lines · Bill O'Donnell
Quoted 18 lines · Ernesto A. Fernández
When changing a file's ACL mask, xfs_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.
To prevent this, perform the mode update and the ACL update as part of the same transaction, and restore the original mode to the vfs inode in case of failure. This requires a change in how extended attributes are
deleted: commit the transaction even if there was no attribute to remove, to log the standard inode fields.
Signed-off-by Ernesto A. Fernández <[email protected]>
---
This issue is covered by generic/449 in xfstests. Several other filesystems

Quoted 8 lines

Quoted 3 lines
Sorry, I'm coming into this late, and might be missing something... is there a particular xfs configuration that you've set up on which generic/449 fails? On 4.15-rc1, I see no failures of g/449.
Fixed in:
67f2ffe31d1 ("xfs: don't change inode mode if ACL update fails")
(I think...)

Correct.
Thanks-
Bill

Quoted 146 lines

--D

Quoted 131 lines

Thanks-
Bill

Quoted 126 lines

 fs/xfs/libxfs/xfs_attr.c | 13 +++++++++++--
 fs/xfs/xfs_acl.c         | 39 ++++++++++++++-------------------------
 2 files changed, 25 insertions(+), 27 deletions(-)

fs/xfs/libxfs/xfs_attr.c +11 -2

index de7b9bd..c3193e1 100644
@@ -395,6 +395,7 @@ xfs_attr_remove(
 	struct xfs_defer_ops	dfops;
 	xfs_fsblock_t		firstblock;
 	int			error;
+	int			noattr = 0;
 
 	XFS_STATS_INC(mp, xs_attr_remove);

@@ -438,7 +439,12 @@ xfs_attr_remove(
 	xfs_trans_ijoin(args.trans, dp, 0);
 
 	if (!xfs_inode_hasattr(dp)) {
-		error = -ENOATTR;
+		/*
+		 * Commit even if there is no attr to remove, because when
+		 * setting ACLs the caller may be changing the mode in the
+		 * same transaction.
+		 */
+		noattr = 1;
 	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
 		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
 		error = xfs_attr_shortform_remove(&args);

@@ -458,7 +464,7 @@ xfs_attr_remove(
 	if (mp->m_flags & XFS_MOUNT_WSYNC)
 		xfs_trans_set_sync(args.trans);
 
-	if ((flags & ATTR_KERNOTIME) == 0)
+	if ((flags & ATTR_KERNOTIME) == 0 && !noattr)
 		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
 
 	/*

@@ -468,6 +474,9 @@ xfs_attr_remove(
 	error = xfs_trans_commit(args.trans);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
+	if (!error && noattr)
+		error = -ENOATTR;
+
 	return error;
 
 out:

fs/xfs/xfs_acl.c +14 ~~-25~~

index 7034e17..1fe4363 100644
@@ -226,28 +226,13 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	return error;
 }
 
-static int
-xfs_set_mode(struct inode *inode, umode_t mode)
-{
-	int error = 0;
-
-	if (mode != inode->i_mode) {
-		struct iattr iattr;
-
-		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
-		iattr.ia_mode = mode;
-		iattr.ia_ctime = current_time(inode);
-
-		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
-	}
-
-	return error;
-}
-
 int
 xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int error = 0;
+	int mode_changed = 0;
+	umode_t old_mode = inode->i_mode;
+	struct timespec old_ctime = inode->i_ctime;
 
 	if (!acl)
 		goto set_acl;

@@ -257,16 +242,20 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		return error;
 
 	if (type == ACL_TYPE_ACCESS) {
-		umode_t mode;
-
-		error = posix_acl_update_mode(inode, &mode, &acl);
-		if (error)
-			return error;
-		error = xfs_set_mode(inode, mode);
+		error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
 		if (error)
 			return error;
+		inode->i_ctime = current_time(inode);
+		XFS_STATS_INC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+		mode_changed = 1;
 	}
 
  set_acl:
-	return __xfs_set_acl(inode, acl, type);
+	error = __xfs_set_acl(inode, acl, type);
+	if (error && mode_changed) {
+		inode->i_mode = old_mode;
+		inode->i_ctime = old_ctime;
+		XFS_STATS_DEC(XFS_I(inode)->i_mount, xs_ig_attrchg);
+	}
+	return error;
 }

Signature · 1 line

2.1.4

Quoted 8 lines

Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html