* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-14 8:18 [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails Ernesto A. Fernández
@ 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-12-07 17:31 ` Bill O'Donnell
2 siblings, 1 reply; 18+ messages in thread
From: Darrick J. Wong @ 2017-08-14 22:28 UTC (permalink / raw)
To: Ernesto A. Fernández; +Cc: linux-xfs, Zorro Lang
On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> 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
I disagree that corrupting the filesystem is an acceptable strategy for
dealing with insufficient space to handle setfacl.
> 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
>
> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-14 22:28 ` Darrick J. Wong
@ 2017-08-15 0:58 ` Ernesto A. Fernández
0 siblings, 0 replies; 18+ messages in thread
From: Ernesto A. Fernández @ 2017-08-15 0:58 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Zorro Lang, Ernesto A. Fernández
On Mon, Aug 14, 2017 at 03:28:27PM -0700, Darrick J. Wong wrote:
> On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> > 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.
>
> > 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-14 8:18 [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails Ernesto A. Fernández
2017-08-14 22:28 ` Darrick J. Wong
@ 2017-08-15 2:00 ` Dave Chinner
2017-08-15 6:18 ` Ernesto A. Fernández
2017-12-07 17:31 ` Bill O'Donnell
2 siblings, 1 reply; 18+ messages in thread
From: Dave Chinner @ 2017-08-15 2:00 UTC (permalink / raw)
To: Ernesto A. Fernández; +Cc: linux-xfs, Darrick J. Wong, Zorro Lang
On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> 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;
bool.
And make it "bool has_attr = true" to avoid the nasty double
negative of "!noattr" later on.
>
> 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);
Why?
>
> /*
> @@ -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;
> -}
Ok, so that does an atomic, transactional change to the inode
mode....
> 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;
> }
And this doesn't.
So this breaks all our rules for inode metadata updates, both for
runtime access serialisation and for crash consistency.
> 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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-15 2:00 ` Dave Chinner
@ 2017-08-15 6:18 ` Ernesto A. Fernández
2017-08-15 8:44 ` Dave Chinner
0 siblings, 1 reply; 18+ messages in thread
From: Ernesto A. Fernández @ 2017-08-15 6:18 UTC (permalink / raw)
To: Dave Chinner
Cc: linux-xfs, Darrick J. Wong, Zorro Lang, Ernesto A. Fernández
Hi Dave, thanks for the review!
On Tue, Aug 15, 2017 at 12:00:18PM +1000, Dave Chinner wrote:
> On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> > 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;
>
> bool.
>
> 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?
>
> >
> > 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);
>
> 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.
>
> >
> > /*
> > @@ -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;
> > -}
>
> Ok, so that does an atomic, transactional change to the inode
> mode....
>
> > 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;
> > }
>
> And this doesn't.
>
> 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.
> > 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)
> 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.
> 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.
> --
> Dave Chinner
> david@fromorbit.com
Thanks,
Ernest
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
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
0 siblings, 1 reply; 18+ messages in thread
From: Dave Chinner @ 2017-08-15 8:44 UTC (permalink / raw)
To: Ernesto A. Fernández; +Cc: linux-xfs, Darrick J. Wong, Zorro Lang
On Tue, Aug 15, 2017 at 03:18:58AM -0300, Ernesto A. Fernández wrote:
> Hi Dave, thanks for the review!
>
> On Tue, Aug 15, 2017 at 12:00:18PM +1000, Dave Chinner wrote:
> > On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> > > 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;
> >
> > bool.
> >
> > 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?
>
> >
> > >
> > > 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);
> >
> > 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.
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.
> > > @@ -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;
> > > }
> >
> > And this doesn't.
> >
> > 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.
> 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.
> 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....
> 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.
> > > 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)
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.
> > 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....
> 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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-15 8:44 ` Dave Chinner
@ 2017-08-15 19:29 ` Ernesto A. Fernández
2017-08-16 0:25 ` Dave Chinner
0 siblings, 1 reply; 18+ messages in thread
From: Ernesto A. Fernández @ 2017-08-15 19:29 UTC (permalink / raw)
To: Dave Chinner
Cc: linux-xfs, Darrick J. Wong, Zorro Lang, Ernesto A. Fernández
On Tue, Aug 15, 2017 at 06:44:30PM +1000, Dave Chinner wrote:
> On Tue, Aug 15, 2017 at 03:18:58AM -0300, Ernesto A. Fernández wrote:
> > Hi Dave, thanks for the review!
> >
> > On Tue, Aug 15, 2017 at 12:00:18PM +1000, Dave Chinner wrote:
> > > On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> > > > 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;
> > >
> > > bool.
> > >
> > > 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?
> >
> > >
> > > >
> > > > 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);
> > >
> > > 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.
>
> 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.
>
> > > > @@ -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;
> > > > }
> > >
> > > And this doesn't.
> > >
> > > 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.
>
> > 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.
>
> > 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....
>
> > 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.
>
> > > > 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)
>
> 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.
>
> > > 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....
>
> > 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.
> 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.
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
Thanks,
Ernest
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
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
0 siblings, 1 reply; 18+ messages in thread
From: Dave Chinner @ 2017-08-16 0:25 UTC (permalink / raw)
To: Ernesto A. Fernández; +Cc: linux-xfs, Darrick J. Wong, Zorro Lang
On Tue, Aug 15, 2017 at 04:29:39PM -0300, Ernesto A. Fernández wrote:
> On Tue, Aug 15, 2017 at 06:44:30PM +1000, Dave Chinner wrote:
> > 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.
> 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.
--
Dave Chinner
david@fromorbit.com
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(-)
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 7034e17535de..3354140de07e 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -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;
}
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-16 0:25 ` Dave Chinner
@ 2017-08-16 7:16 ` Ernesto A. Fernández
2017-08-16 13:35 ` Dave Chinner
0 siblings, 1 reply; 18+ messages in thread
From: Ernesto A. Fernández @ 2017-08-16 7:16 UTC (permalink / raw)
To: Dave Chinner
Cc: linux-xfs, Darrick J. Wong, Zorro Lang, Ernesto A. Fernández
On Wed, Aug 16, 2017 at 10:25:11AM +1000, Dave Chinner wrote:
> On Tue, Aug 15, 2017 at 04:29:39PM -0300, Ernesto A. Fernández wrote:
> > On Tue, Aug 15, 2017 at 06:44:30PM +1000, Dave Chinner wrote:
> > > 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.
>
> > 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.
Alright, thanks for the explanation.
> 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.
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
>
> 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(-)
>
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 7034e17535de..3354140de07e 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -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;
> }
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
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
0 siblings, 1 reply; 18+ messages in thread
From: Dave Chinner @ 2017-08-16 13:35 UTC (permalink / raw)
To: Ernesto A. Fernández; +Cc: linux-xfs, Darrick J. Wong, Zorro Lang
On Wed, Aug 16, 2017 at 04:16:53AM -0300, Ernesto A. Fernández wrote:
> On Wed, Aug 16, 2017 at 10:25:11AM +1000, Dave Chinner wrote:
> > On Tue, Aug 15, 2017 at 04:29:39PM -0300, Ernesto A. Fernández wrote:
> > > On Tue, Aug 15, 2017 at 06:44:30PM +1000, Dave Chinner wrote:
> > > > 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.
> >
> > > 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.
>
> Alright, thanks for the explanation.
>
> > 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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-16 13:35 ` Dave Chinner
@ 2017-08-16 19:31 ` Ernesto A. Fernández
2017-08-17 0:00 ` Dave Chinner
0 siblings, 1 reply; 18+ messages in thread
From: Ernesto A. Fernández @ 2017-08-16 19:31 UTC (permalink / raw)
To: Dave Chinner
Cc: linux-xfs, Darrick J. Wong, Zorro Lang, Ernesto A. Fernández
On Wed, Aug 16, 2017 at 11:35:02PM +1000, Dave Chinner wrote:
> On Wed, Aug 16, 2017 at 04:16:53AM -0300, Ernesto A. Fernández wrote:
> > On Wed, Aug 16, 2017 at 10:25:11AM +1000, Dave Chinner wrote:
> > > 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.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
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
0 siblings, 1 reply; 18+ messages in thread
From: Dave Chinner @ 2017-08-17 0:00 UTC (permalink / raw)
To: Ernesto A. Fernández; +Cc: linux-xfs, Darrick J. Wong, Zorro Lang
On Wed, Aug 16, 2017 at 04:31:01PM -0300, Ernesto A. Fernández wrote:
> On Wed, Aug 16, 2017 at 11:35:02PM +1000, Dave Chinner wrote:
> > On Wed, Aug 16, 2017 at 04:16:53AM -0300, Ernesto A. Fernández wrote:
> > > On Wed, Aug 16, 2017 at 10:25:11AM +1000, Dave Chinner wrote:
> > > > 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
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?
> 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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-17 0:00 ` Dave Chinner
@ 2017-08-17 5:34 ` Ernesto A. Fernández
2017-08-17 6:34 ` Dave Chinner
0 siblings, 1 reply; 18+ messages in thread
From: Ernesto A. Fernández @ 2017-08-17 5:34 UTC (permalink / raw)
To: Dave Chinner
Cc: linux-xfs, Darrick J. Wong, Zorro Lang, Ernesto A. Fernández
On Thu, Aug 17, 2017 at 10:00:42AM +1000, Dave Chinner wrote:
> On Wed, Aug 16, 2017 at 04:31:01PM -0300, Ernesto A. Fernández wrote:
> > 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.
> 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.
> > 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.
> --
> Dave Chinner
> david@fromorbit.com
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-17 5:34 ` Ernesto A. Fernández
@ 2017-08-17 6:34 ` Dave Chinner
0 siblings, 0 replies; 18+ messages in thread
From: Dave Chinner @ 2017-08-17 6:34 UTC (permalink / raw)
To: Ernesto A. Fernández; +Cc: linux-xfs, Darrick J. Wong, Zorro Lang
On Thu, Aug 17, 2017 at 02:34:40AM -0300, Ernesto A. Fernández wrote:
> On Thu, Aug 17, 2017 at 10:00:42AM +1000, Dave Chinner wrote:
> > On Wed, Aug 16, 2017 at 04:31:01PM -0300, Ernesto A. Fernández wrote:
> > > 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.
>
> > 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
Ok, so it's a specific change in tree size that is causing the
problem. It's probably a tree height change opertaion....
> 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.
> 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.
> 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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-08-14 8:18 [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails Ernesto A. Fernández
2017-08-14 22:28 ` Darrick J. Wong
2017-08-15 2:00 ` 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
2 siblings, 2 replies; 18+ messages in thread
From: Bill O'Donnell @ 2017-12-07 17:31 UTC (permalink / raw)
To: Ernesto A. Fernández; +Cc: linux-xfs, Darrick J. Wong, Zorro Lang
On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> 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
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
> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-12-07 17:31 ` Bill O'Donnell
@ 2017-12-07 17:38 ` Bill O'Donnell
2017-12-07 17:43 ` Darrick J. Wong
1 sibling, 0 replies; 18+ messages in thread
From: Bill O'Donnell @ 2017-12-07 17:38 UTC (permalink / raw)
To: Ernesto A. Fernández, linux-xfs, Darrick J. Wong, Zorro Lang
On Thu, Dec 07, 2017 at 11:31:51AM -0600, Bill O'Donnell wrote:
> On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> > 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
>
> 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 <dchinner@redhat.com>
Date: Mon Oct 9 11:37:23 2017 -0700
xfs: don't change inode mode if ACL update fails
>
> Thanks-
> Bill
>
>
>
> > 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
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
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
1 sibling, 1 reply; 18+ messages in thread
From: Darrick J. Wong @ 2017-12-07 17:43 UTC (permalink / raw)
To: Bill O'Donnell; +Cc: Ernesto A. Fernández, linux-xfs, Zorro Lang
On Thu, Dec 07, 2017 at 11:31:51AM -0600, Bill O'Donnell wrote:
> On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> > 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
>
> 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
> Thanks-
> Bill
>
>
>
> > 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
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xfs: preserve i_mode if __xfs_set_acl() fails
2017-12-07 17:43 ` Darrick J. Wong
@ 2017-12-07 17:51 ` Bill O'Donnell
0 siblings, 0 replies; 18+ messages in thread
From: Bill O'Donnell @ 2017-12-07 17:51 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Ernesto A. Fernández, linux-xfs, Zorro Lang
On Thu, Dec 07, 2017 at 09:43:09AM -0800, Darrick J. Wong wrote:
> On Thu, Dec 07, 2017 at 11:31:51AM -0600, Bill O'Donnell wrote:
> > On Mon, Aug 14, 2017 at 05:18:10AM -0300, Ernesto A. Fernández wrote:
> > > 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
> >
> > 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
>
> --D
>
> > Thanks-
> > Bill
> >
> >
> >
> > > 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
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread