linux-kernel - [PATCH 3.12 002/142] xfs: pass total block res. as total xfs_bmapi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <2d2888080cc0f811cd7c544a51e17f5214944baf.1491838390.git.jslaby@suse.cz>
Date:   Mon, 10 Apr 2017 17:31:23 +0200
From:   Jiri Slaby <jslaby@...e.cz>
To:     stable@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, Brian Foster <bfoster@...hat.com>,
        Dave Chinner <david@...morbit.com>,
        Nikolay Borisov <nborisov@...e.com>,
        Jiri Slaby <jslaby@...e.cz>
Subject: [PATCH 3.12 002/142] xfs: pass total block res. as total xfs_bmapi_write() parameter

From: Brian Foster <bfoster@...hat.com>

3.12-stable review patch.  If anyone has any objections, please let me know.

===============

commit dbd5c8c9a28899c6ca719eb21afc0afba9dd5574 upstream.

The total field from struct xfs_alloc_arg is a bit of an unknown
commodity. It is documented as the total block requirement for the
transaction and is used in this manner from most call sites by virtue of
passing the total block reservation of the transaction associated with
an allocation. Several xfs_bmapi_write() callers pass hardcoded values
of 0 or 1 for the total block requirement, which is a historical oddity
without any clear reasoning.

The xfs_iomap_write_direct() caller, for example, passes 0 for the total
block requirement. This has been determined to cause problems in the
form of ABBA deadlocks of AGF buffers due to incorrect AG selection in
the block allocator. Specifically, the xfs_alloc_space_available()
function incorrectly selects an AG that doesn't actually have sufficient
space for the allocation. This occurs because the args.total field is 0
and thus the remaining free space check on the AG doesn't actually
consider the size of the allocation request. This locks the AGF buffer,
the allocation attempt proceeds and ultimately fails (in
xfs_alloc_fix_minleft()), and xfs_alloc_vexent() moves on to the next
AG. In turn, this can lead to incorrect AG locking order (if the
allocator wraps around, attempting to lock AG 0 after acquiring AG N)
and thus deadlock if racing with another operation. This problem has
been reproduced via generic/299 on smallish (1GB) ramdisk test devices.

To avoid this problem, replace the undocumented hardcoded total
parameters from the iomap and utility callers to pass the block
reservation used for the associated transaction. This is consistent with
other xfs_bmapi_write() callers throughout XFS. The assumption is that
the total field allows the selection of an AG that can handle the entire
operation rather than simply the allocation/range being requested (e.g.,
resulting btree splits, etc.). This addresses the aforementioned
generic/299 hang by ensuring AG selection only occurs when the
allocation can be satisfied by the AG.

[nb] backport to 3.12

Reported-by: Ross Zwisler <ross.zwisler@...ux.intel.com>
Signed-off-by: Brian Foster <bfoster@...hat.com>
Reviewed-by: Dave Chinner <dchinner@...hat.com>
Signed-off-by: Dave Chinner <david@...morbit.com>
Signed-off-by: Nikolay Borisov <nborisov@...e.com>
Acked-by: Brian Foster <bfoster@...hat.com>
Signed-off-by: Jiri Slaby <jslaby@...e.cz>
---
 fs/xfs/xfs_bmap_util.c | 2 +-
 fs/xfs/xfs_iomap.c     | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 97f952caea74..42cb2f3ea51f 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1100,7 +1100,7 @@ xfs_alloc_file_space(
 		xfs_bmap_init(&free_list, &firstfsb);
 		error = xfs_bmapi_write(tp, ip, startoffset_fsb,
 					allocatesize_fsb, alloc_type, &firstfsb,
-					0, imapp, &nimaps, &free_list);
+					resblks, imapp, &nimaps, &free_list);
 		if (error) {
 			goto error0;
 		}
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 8d4d49b6fbf3..1d48f7a9b63e 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -217,7 +217,7 @@ xfs_iomap_write_direct(
 	xfs_bmap_init(&free_list, &firstfsb);
 	nimaps = 1;
 	error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb, bmapi_flag,
-				&firstfsb, 0, imap, &nimaps, &free_list);
+				&firstfsb, resblks, imap, &nimaps, &free_list);
 	if (error)
 		goto out_bmap_cancel;

@@ -762,7 +762,7 @@ xfs_iomap_write_allocate(
 			error = xfs_bmapi_write(tp, ip, map_start_fsb,
 						count_fsb,
 						XFS_BMAPI_STACK_SWITCH,
-						&first_block, 1,
+						&first_block, nres,
 						imap, &nimaps, &free_list);
 			if (error)
 				goto trans_cancel;
@@ -877,8 +877,8 @@ xfs_iomap_write_unwritten(
 		xfs_bmap_init(&free_list, &firstfsb);
 		nimaps = 1;
 		error = xfs_bmapi_write(tp, ip, offset_fsb, count_fsb,
-				  XFS_BMAPI_CONVERT, &firstfsb,
-				  1, &imap, &nimaps, &free_list);
+					XFS_BMAPI_CONVERT, &firstfsb, resblks,
+					&imap, &nimaps, &free_list);
 		if (error)
 			goto error_on_bmapi_transaction;

-- 
2.12.2