* [PATCH STABLE v4.14.y 1/2] xfs: validate sb_logsunit is a multiple of the fs blocksize
2020-04-24 23:05 [PATCH STABLE v4.14.y 0/2] xfs: Backport two fixes Suraj Jitindar Singh
@ 2020-04-24 23:05 ` Suraj Jitindar Singh
2020-04-24 23:05 ` [PATCH STABLE v4.14.y 2/2] xfs: Fix deadlock between AGI and AGF with RENAME_WHITEOUT Suraj Jitindar Singh
2020-04-28 9:02 ` [PATCH STABLE v4.14.y 0/2] xfs: Backport two fixes Greg KH
2 siblings, 0 replies; 5+ messages in thread
From: Suraj Jitindar Singh @ 2020-04-24 23:05 UTC (permalink / raw)
To: stable; +Cc: sjitindarsingh, linux-xfs, Darrick J. Wong, Suraj Jitindar Singh
From: "Darrick J. Wong" <darrick.wong@oracle.com>
commit 9c92ee208b1faa0ef2cc899b85fd0607b6fac7fe upstream.
Make sure the log stripe unit is sane before proceeding with mounting.
AFAICT this means that logsunit has to be 0, 1, or a multiple of the fs
block size. Found this by setting the LSB of logsunit in xfs/350 and
watching the system crash as soon as we try to write to the log.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
---
fs/xfs/xfs_log.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 4e768e606998..360e32220f93 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -608,6 +608,7 @@ xfs_log_mount(
xfs_daddr_t blk_offset,
int num_bblks)
{
+ bool fatal = xfs_sb_version_hascrc(&mp->m_sb);
int error = 0;
int min_logfsbs;
@@ -659,9 +660,20 @@ xfs_log_mount(
XFS_FSB_TO_B(mp, mp->m_sb.sb_logblocks),
XFS_MAX_LOG_BYTES);
error = -EINVAL;
+ } else if (mp->m_sb.sb_logsunit > 1 &&
+ mp->m_sb.sb_logsunit % mp->m_sb.sb_blocksize) {
+ xfs_warn(mp,
+ "log stripe unit %u bytes must be a multiple of block size",
+ mp->m_sb.sb_logsunit);
+ error = -EINVAL;
+ fatal = true;
}
if (error) {
- if (xfs_sb_version_hascrc(&mp->m_sb)) {
+ /*
+ * Log check errors are always fatal on v5; or whenever bad
+ * metadata leads to a crash.
+ */
+ if (fatal) {
xfs_crit(mp, "AAIEEE! Log failed size checks. Abort!");
ASSERT(0);
goto out_free_log;
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH STABLE v4.14.y 2/2] xfs: Fix deadlock between AGI and AGF with RENAME_WHITEOUT
2020-04-24 23:05 [PATCH STABLE v4.14.y 0/2] xfs: Backport two fixes Suraj Jitindar Singh
2020-04-24 23:05 ` [PATCH STABLE v4.14.y 1/2] xfs: validate sb_logsunit is a multiple of the fs blocksize Suraj Jitindar Singh
@ 2020-04-24 23:05 ` Suraj Jitindar Singh
2020-04-25 1:49 ` Sasha Levin
2020-04-28 9:02 ` [PATCH STABLE v4.14.y 0/2] xfs: Backport two fixes Greg KH
2 siblings, 1 reply; 5+ messages in thread
From: Suraj Jitindar Singh @ 2020-04-24 23:05 UTC (permalink / raw)
To: stable
Cc: sjitindarsingh, linux-xfs, kaixuxia, kaixuxia, Darrick J . Wong,
Suraj Jitindar Singh
From: kaixuxia <xiakaixu1987@gmail.com>
commit bc56ad8c74b8588685c2875de0df8ab6974828ef upstream.
When performing rename operation with RENAME_WHITEOUT flag, we will
hold AGF lock to allocate or free extents in manipulating the dirents
firstly, and then doing the xfs_iunlink_remove() call last to hold
AGI lock to modify the tmpfile info, so we the lock order AGI->AGF.
The big problem here is that we have an ordering constraint on AGF
and AGI locking - inode allocation locks the AGI, then can allocate
a new extent for new inodes, locking the AGF after the AGI. Hence
the ordering that is imposed by other parts of the code is AGI before
AGF. So we get an ABBA deadlock between the AGI and AGF here.
Process A:
Call trace:
? __schedule+0x2bd/0x620
schedule+0x33/0x90
schedule_timeout+0x17d/0x290
__down_common+0xef/0x125
? xfs_buf_find+0x215/0x6c0 [xfs]
down+0x3b/0x50
xfs_buf_lock+0x34/0xf0 [xfs]
xfs_buf_find+0x215/0x6c0 [xfs]
xfs_buf_get_map+0x37/0x230 [xfs]
xfs_buf_read_map+0x29/0x190 [xfs]
xfs_trans_read_buf_map+0x13d/0x520 [xfs]
xfs_read_agf+0xa6/0x180 [xfs]
? schedule_timeout+0x17d/0x290
xfs_alloc_read_agf+0x52/0x1f0 [xfs]
xfs_alloc_fix_freelist+0x432/0x590 [xfs]
? down+0x3b/0x50
? xfs_buf_lock+0x34/0xf0 [xfs]
? xfs_buf_find+0x215/0x6c0 [xfs]
xfs_alloc_vextent+0x301/0x6c0 [xfs]
xfs_ialloc_ag_alloc+0x182/0x700 [xfs]
? _xfs_trans_bjoin+0x72/0xf0 [xfs]
xfs_dialloc+0x116/0x290 [xfs]
xfs_ialloc+0x6d/0x5e0 [xfs]
? xfs_log_reserve+0x165/0x280 [xfs]
xfs_dir_ialloc+0x8c/0x240 [xfs]
xfs_create+0x35a/0x610 [xfs]
xfs_generic_create+0x1f1/0x2f0 [xfs]
...
Process B:
Call trace:
? __schedule+0x2bd/0x620
? xfs_bmapi_allocate+0x245/0x380 [xfs]
schedule+0x33/0x90
schedule_timeout+0x17d/0x290
? xfs_buf_find+0x1fd/0x6c0 [xfs]
__down_common+0xef/0x125
? xfs_buf_get_map+0x37/0x230 [xfs]
? xfs_buf_find+0x215/0x6c0 [xfs]
down+0x3b/0x50
xfs_buf_lock+0x34/0xf0 [xfs]
xfs_buf_find+0x215/0x6c0 [xfs]
xfs_buf_get_map+0x37/0x230 [xfs]
xfs_buf_read_map+0x29/0x190 [xfs]
xfs_trans_read_buf_map+0x13d/0x520 [xfs]
xfs_read_agi+0xa8/0x160 [xfs]
xfs_iunlink_remove+0x6f/0x2a0 [xfs]
? current_time+0x46/0x80
? xfs_trans_ichgtime+0x39/0xb0 [xfs]
xfs_rename+0x57a/0xae0 [xfs]
xfs_vn_rename+0xe4/0x150 [xfs]
...
In this patch we move the xfs_iunlink_remove() call to
before acquiring the AGF lock to preserve correct AGI/AGF locking
order.
Signed-off-by: kaixuxia <kaixuxia@tencent.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Minor massage required to backport to apply due to removal of
out_bmap_cancel: error path label upstream as a result of code
rework. Only change was to the last code block removed by the
patch. Functionally equivalent to upstream.
Signed-off-by: Suraj Jitindar Singh <surajjs@amazon.com>
---
fs/xfs/xfs_inode.c | 85 +++++++++++++++++++++++-----------------------
1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index cb4833d06467..7cfbe2b0f886 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3035,7 +3035,8 @@ xfs_rename(
&dfops, &first_block, spaceres);
/*
- * Set up the target.
+ * Check for expected errors before we dirty the transaction
+ * so we can return an error without a transaction abort.
*/
if (target_ip == NULL) {
/*
@@ -3047,6 +3048,46 @@ xfs_rename(
if (error)
goto out_trans_cancel;
}
+ } else {
+ /*
+ * If target exists and it's a directory, check that whether
+ * it can be destroyed.
+ */
+ if (S_ISDIR(VFS_I(target_ip)->i_mode) &&
+ (!xfs_dir_isempty(target_ip) ||
+ (VFS_I(target_ip)->i_nlink > 2))) {
+ error = -EEXIST;
+ goto out_trans_cancel;
+ }
+ }
+
+ /*
+ * Directory entry creation below may acquire the AGF. Remove
+ * the whiteout from the unlinked list first to preserve correct
+ * AGI/AGF locking order. This dirties the transaction so failures
+ * after this point will abort and log recovery will clean up the
+ * mess.
+ *
+ * For whiteouts, we need to bump the link count on the whiteout
+ * inode. After this point, we have a real link, clear the tmpfile
+ * state flag from the inode so it doesn't accidentally get misused
+ * in future.
+ */
+ if (wip) {
+ ASSERT(VFS_I(wip)->i_nlink == 0);
+ error = xfs_iunlink_remove(tp, wip);
+ if (error)
+ goto out_trans_cancel;
+
+ xfs_bumplink(tp, wip);
+ xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
+ VFS_I(wip)->i_state &= ~I_LINKABLE;
+ }
+
+ /*
+ * Set up the target.
+ */
+ if (target_ip == NULL) {
/*
* If target does not exist and the rename crosses
* directories, adjust the target directory link count
@@ -3067,22 +3108,6 @@ xfs_rename(
goto out_bmap_cancel;
}
} else { /* target_ip != NULL */
- /*
- * If target exists and it's a directory, check that both
- * target and source are directories and that target can be
- * destroyed, or that neither is a directory.
- */
- if (S_ISDIR(VFS_I(target_ip)->i_mode)) {
- /*
- * Make sure target dir is empty.
- */
- if (!(xfs_dir_isempty(target_ip)) ||
- (VFS_I(target_ip)->i_nlink > 2)) {
- error = -EEXIST;
- goto out_trans_cancel;
- }
- }
-
/*
* Link the source inode under the target name.
* If the source inode is a directory and we are moving
@@ -3175,32 +3200,6 @@ xfs_rename(
if (error)
goto out_bmap_cancel;
- /*
- * For whiteouts, we need to bump the link count on the whiteout inode.
- * This means that failures all the way up to this point leave the inode
- * on the unlinked list and so cleanup is a simple matter of dropping
- * the remaining reference to it. If we fail here after bumping the link
- * count, we're shutting down the filesystem so we'll never see the
- * intermediate state on disk.
- */
- if (wip) {
- ASSERT(VFS_I(wip)->i_nlink == 0);
- error = xfs_bumplink(tp, wip);
- if (error)
- goto out_bmap_cancel;
- error = xfs_iunlink_remove(tp, wip);
- if (error)
- goto out_bmap_cancel;
- xfs_trans_log_inode(tp, wip, XFS_ILOG_CORE);
-
- /*
- * Now we have a real link, clear the "I'm a tmpfile" state
- * flag from the inode so it doesn't accidentally get misused in
- * future.
- */
- VFS_I(wip)->i_state &= ~I_LINKABLE;
- }
-
xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
if (new_parent)
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH STABLE v4.14.y 0/2] xfs: Backport two fixes
2020-04-24 23:05 [PATCH STABLE v4.14.y 0/2] xfs: Backport two fixes Suraj Jitindar Singh
2020-04-24 23:05 ` [PATCH STABLE v4.14.y 1/2] xfs: validate sb_logsunit is a multiple of the fs blocksize Suraj Jitindar Singh
2020-04-24 23:05 ` [PATCH STABLE v4.14.y 2/2] xfs: Fix deadlock between AGI and AGF with RENAME_WHITEOUT Suraj Jitindar Singh
@ 2020-04-28 9:02 ` Greg KH
2 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2020-04-28 9:02 UTC (permalink / raw)
To: Suraj Jitindar Singh; +Cc: stable, sjitindarsingh, linux-xfs
On Fri, Apr 24, 2020 at 04:05:30PM -0700, Suraj Jitindar Singh wrote:
> This series backports two patches which fix known bugs in the xfs
> filesystem code to the v4.14.y stable tree.
>
> They are each verified by the xfs tests xfs/439 and generic/585
> respectively.
>
> The first patch applies cleanly.
>
> The second patch required slight massage due to the last code block
> being removed having changed slightly upstream due to rework. I think
> the backport is functionally equivalent.
> Only thing is I request comment that it is correct to use the following
> error path:
>
> ASSERT(VFS_I(wip)->i_nlink == 0);
> error = xfs_iunlink_remove(tp, wip);
> if (error)
> > goto out_trans_cancel;
>
> The old error patch out_bmap_cancel still exists here. However as
> nothing can have modified the deferred ops struct at this point I
> believe it is sufficient to go to the "out_trans_cancel" error label.
>
> Darrick J. Wong (1):
> xfs: validate sb_logsunit is a multiple of the fs blocksize
>
> kaixuxia (1):
> xfs: Fix deadlock between AGI and AGF with RENAME_WHITEOUT
>
> fs/xfs/xfs_inode.c | 85 +++++++++++++++++++++++-----------------------
> fs/xfs/xfs_log.c | 14 +++++++-
> 2 files changed, 55 insertions(+), 44 deletions(-)
All (including the 4.19 patch), now queued up, thanks.
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread