Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
From: "Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>
To: linux-xfs@vger.kernel.org
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>,
	"Zorro Lang" <zlang@redhat.com>,
	"Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>
Subject: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
Date: Mon, 14 Aug 2017 05:18:10 -0300
Message-ID: <20170814081806.GA3924@debian.home> (raw)

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 <ernesto.mnd.fernandez@gmail.com>
---
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(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index de7b9bd..c3193e1 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -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:
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 7034e17..1fe4363 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -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;
 }
-- 
2.1.4


             reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-14  8:18 Ernesto A. Fernández [this message]
2017-08-14 22:28 ` Darrick J. Wong
2017-08-15  0:58   ` Ernesto A. Fernández
2017-08-15  2:00 ` Dave Chinner
2017-08-15  6:18   ` Ernesto A. Fernández
2017-08-15  8:44     ` Dave Chinner
2017-08-15 19:29       ` Ernesto A. Fernández
2017-08-16  0:25         ` Dave Chinner
2017-08-16  7:16           ` Ernesto A. Fernández
2017-08-16 13:35             ` Dave Chinner
2017-08-16 19:31               ` Ernesto A. Fernández
2017-08-17  0:00                 ` Dave Chinner
2017-08-17  5:34                   ` Ernesto A. Fernández
2017-08-17  6:34                     ` Dave Chinner
2017-12-07 17:31 ` Bill O'Donnell
2017-12-07 17:38   ` Bill O'Donnell
2017-12-07 17:43   ` Darrick J. Wong
2017-12-07 17:51     ` Bill O'Donnell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170814081806.GA3924@debian.home \
    --to=ernesto.mnd.fernandez@gmail.com \
    --cc=darrick.wong@oracle.com \
    --cc=linux-xfs@vger.kernel.org \
    --cc=zlang@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git