[PATCH 0/8] xfs-4.18: various fixes

[PATCH 0/8] xfs-4.18: various fixes

[Darrick J. Wong]

Hi all,

Here's a series of patches to fix some problems I noticed in 4.18.

The first patch fixes a problem where we WARN_ON if someone tries to
create an empty transaction while the fs is frozen.  Since these
transactions can't modify anything, it seems unnecessary to warn about
them.

The second patch fixes per-ag rmapbt reservation accounting problems.
The on-disk fdblocks counter is not decremented for rmapbt blocks, so
the incore reservation should always decrement the incore fdblocks by
the entire reservation regardless of rmapbt use.  This fixes the problem
where b_avail as reported by statfs changes across an unmount/mount
cycle if the rmapbt had grown prior to the unmount, even though the
number of blocks in use has not changed at all.

The third patch fixes the userspace-configurable default reserved
blocks calculation when there are fewer fdblocks than what we set aside
to avoid deadlocks.

The fourth patch fixes a problem Zorro Lang found where on a 512b
block filesystem we can cause an extent at s_maxbytes to be right
shifted to the point that it block offset exceeds the field size of 54
bytes.  generic/485 is the regression test.

The fifth patch fixes a hard to trigger TOCTOU bug in generic/166 where
we take ILOCK_SHARED in preparation for a directio write but by the time
we get the lock a second thread has set the reflink flag on the file and
shared the extent we're trying to write but we proceed into the COW
allocation paths without the required ILOCK_EXCL.

The sixth patch fixes an uninitialized field in the rtbitmap fsmap code.

The seventh pach fixes an off-by-one error in the rtbitmap range query
function (which fortunately only affects the rtbitmap fsmap results).

The eighth patch fixes a bug reported by fsx in generic/127 wherein if
we write a file to a non-page-aligned length, mmap read the EOF page,
truncate the file to a smaller non-page-aligned length, run zero_range
out to EOF, and then mmap read the EOF page a second time, we briefly
see non-zero bytes after EOF.

Comments and questions are, as always, welcome.

--D

[PATCH 1/8] xfs: allow empty transactions while frozen

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In commit e89c041338ed6ef ("xfs: implement the GETFSMAP ioctl") we
created the ability to obtain empty transactions.  These transactions
have no log or block reservations and therefore can't modify anything.
Since they're also NO_WRITECOUNT they can run while the fs is frozen,
so we don't need to WARN_ON about that usage.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_trans.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index e040af120b69..524f543c5b82 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -258,7 +258,12 @@ xfs_trans_alloc(
 	if (!(flags & XFS_TRANS_NO_WRITECOUNT))
 		sb_start_intwrite(mp->m_super);
 
-	WARN_ON(mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE);
+	/*
+	 * Zero-reservation ("empty") transactions can't modify anything, so
+	 * they're allowed to run while we're frozen.
+	 */
+	WARN_ON(resp->tr_logres > 0 &&
+		mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE);
 	atomic_inc(&mp->m_active_trans);
 
 	tp = kmem_zone_zalloc(xfs_trans_zone,

Re: [PATCH 1/8] xfs: allow empty transactions while frozen

[Allison Henderson]

Looks fine
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

On 06/21/2018 11:31 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit e89c041338ed6ef ("xfs: implement the GETFSMAP ioctl") we
> created the ability to obtain empty transactions.  These transactions
> have no log or block reservations and therefore can't modify anything.
> Since they're also NO_WRITECOUNT they can run while the fs is frozen,
> so we don't need to WARN_ON about that usage.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_trans.c |    7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index e040af120b69..524f543c5b82 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -258,7 +258,12 @@ xfs_trans_alloc(
>   	if (!(flags & XFS_TRANS_NO_WRITECOUNT))
>   		sb_start_intwrite(mp->m_super);
>   
> -	WARN_ON(mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE);
> +	/*
> +	 * Zero-reservation ("empty") transactions can't modify anything, so
> +	 * they're allowed to run while we're frozen.
> +	 */
> +	WARN_ON(resp->tr_logres > 0 &&
> +		mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE);
>   	atomic_inc(&mp->m_active_trans);
>   
>   	tp = kmem_zone_zalloc(xfs_trans_zone,
> 
> --
> 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=jfRT1oYtPVNVBt-lMSR8ZrPgEPjfv7foQ0meuw0oGxM&s=GsacuLUdUkUoZWMAAmlRTyyR7FByN9OA43H6dYDIK6Q&e=
> 

Re: [PATCH 1/8] xfs: allow empty transactions while frozen

[Christoph Hellwig]

On Thu, Jun 21, 2018 at 11:31:10AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit e89c041338ed6ef ("xfs: implement the GETFSMAP ioctl") we
> created the ability to obtain empty transactions.

That was sneaked in nicely, normally it should be a separate
commit..

> These transactions
> have no log or block reservations and therefore can't modify anything.
> Since they're also NO_WRITECOUNT they can run while the fs is frozen,
> so we don't need to WARN_ON about that usage.

This looks correct, but it really makes me hate these empty
transactions.  We're going to find more and more issues like this
and will end up with a huge bag of special cases.

Reluctantly:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/8] xfs: fix fdblocks accounting w/ RMAPBT per-AG reservation

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In __xfs_ag_resv_init we incorrectly calculate the amount by which to
decrease fdblocks when reserving blocks for the rmapbt.  Because rmapbt
allocations do not decrease fdblocks, we must decrease fdblocks by the
entire size of the requested reservation in order to achieve our goal of
always having enough free blocks to satisfy an rmapbt expansion.

This is in contrast to the refcountbt/finobt, which /do/ subtract from
fdblocks whenever they allocate a block.  For this allocation type we
preserve the existing behavior where we decrease fdblocks only by the
requested reservation minus the size of the existing tree.

This fixes the problem where the available block counts reported by
statfs change across a remount if there had been an rmapbt size change
since mount time.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_ag_resv.c |   31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index 84db76e0e3e3..fecd187fcf2c 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -157,6 +157,7 @@ __xfs_ag_resv_free(
 	error = xfs_mod_fdblocks(pag->pag_mount, oldresv, true);
 	resv->ar_reserved = 0;
 	resv->ar_asked = 0;
+	resv->ar_orig_reserved = 0;
 
 	if (error)
 		trace_xfs_ag_resv_free_error(pag->pag_mount, pag->pag_agno,
@@ -189,13 +190,34 @@ __xfs_ag_resv_init(
 	struct xfs_mount		*mp = pag->pag_mount;
 	struct xfs_ag_resv		*resv;
 	int				error;
-	xfs_extlen_t			reserved;
+	xfs_extlen_t			hidden_space;
 
 	if (used > ask)
 		ask = used;
-	reserved = ask - used;
 
-	error = xfs_mod_fdblocks(mp, -(int64_t)reserved, true);
+	switch (type) {
+	case XFS_AG_RESV_RMAPBT:
+		/*
+		 * Space taken by the rmapbt is not subtracted from fdblocks
+		 * because the rmapbt lives in the free space.  Here we must
+		 * subtract the entire reservation from fdblocks so that we
+		 * always have blocks available for rmapbt expansion.
+		 */
+		hidden_space = ask;
+		break;
+	case XFS_AG_RESV_METADATA:
+		/*
+		 * Space taken by all other metadata btrees are accounted
+		 * on-disk as used space.  We therefore only hide the space
+		 * that is reserved but not used by the trees.
+		 */
+		hidden_space = ask - used;
+		break;
+	default:
+		ASSERT(0);
+		return -EINVAL;
+	}
+	error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
 	if (error) {
 		trace_xfs_ag_resv_init_error(pag->pag_mount, pag->pag_agno,
 				error, _RET_IP_);
@@ -216,7 +238,8 @@ __xfs_ag_resv_init(
 
 	resv = xfs_perag_resv(pag, type);
 	resv->ar_asked = ask;
-	resv->ar_reserved = resv->ar_orig_reserved = reserved;
+	resv->ar_orig_reserved = hidden_space;
+	resv->ar_reserved = ask - used;
 
 	trace_xfs_ag_resv_init(pag, type, ask);
 	return 0;

Re: [PATCH 2/8] xfs: fix fdblocks accounting w/ RMAPBT per-AG reservation

[Allison Henderson]

Alrighty, thx for the comments
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

On 06/21/2018 11:31 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In __xfs_ag_resv_init we incorrectly calculate the amount by which to
> decrease fdblocks when reserving blocks for the rmapbt.  Because rmapbt
> allocations do not decrease fdblocks, we must decrease fdblocks by the
> entire size of the requested reservation in order to achieve our goal of
> always having enough free blocks to satisfy an rmapbt expansion.
> 
> This is in contrast to the refcountbt/finobt, which /do/ subtract from
> fdblocks whenever they allocate a block.  For this allocation type we
> preserve the existing behavior where we decrease fdblocks only by the
> requested reservation minus the size of the existing tree.
> 
> This fixes the problem where the available block counts reported by
> statfs change across a remount if there had been an rmapbt size change
> since mount time.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_ag_resv.c |   31 +++++++++++++++++++++++++++----
>   1 file changed, 27 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> index 84db76e0e3e3..fecd187fcf2c 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.c
> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> @@ -157,6 +157,7 @@ __xfs_ag_resv_free(
>   	error = xfs_mod_fdblocks(pag->pag_mount, oldresv, true);
>   	resv->ar_reserved = 0;
>   	resv->ar_asked = 0;
> +	resv->ar_orig_reserved = 0;
>   
>   	if (error)
>   		trace_xfs_ag_resv_free_error(pag->pag_mount, pag->pag_agno,
> @@ -189,13 +190,34 @@ __xfs_ag_resv_init(
>   	struct xfs_mount		*mp = pag->pag_mount;
>   	struct xfs_ag_resv		*resv;
>   	int				error;
> -	xfs_extlen_t			reserved;
> +	xfs_extlen_t			hidden_space;
>   
>   	if (used > ask)
>   		ask = used;
> -	reserved = ask - used;
>   
> -	error = xfs_mod_fdblocks(mp, -(int64_t)reserved, true);
> +	switch (type) {
> +	case XFS_AG_RESV_RMAPBT:
> +		/*
> +		 * Space taken by the rmapbt is not subtracted from fdblocks
> +		 * because the rmapbt lives in the free space.  Here we must
> +		 * subtract the entire reservation from fdblocks so that we
> +		 * always have blocks available for rmapbt expansion.
> +		 */
> +		hidden_space = ask;
> +		break;
> +	case XFS_AG_RESV_METADATA:
> +		/*
> +		 * Space taken by all other metadata btrees are accounted
> +		 * on-disk as used space.  We therefore only hide the space
> +		 * that is reserved but not used by the trees.
> +		 */
> +		hidden_space = ask - used;
> +		break;
> +	default:
> +		ASSERT(0);
> +		return -EINVAL;
> +	}
> +	error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
>   	if (error) {
>   		trace_xfs_ag_resv_init_error(pag->pag_mount, pag->pag_agno,
>   				error, _RET_IP_);
> @@ -216,7 +238,8 @@ __xfs_ag_resv_init(
>   
>   	resv = xfs_perag_resv(pag, type);
>   	resv->ar_asked = ask;
> -	resv->ar_reserved = resv->ar_orig_reserved = reserved;
> +	resv->ar_orig_reserved = hidden_space;
> +	resv->ar_reserved = ask - used;
>   
>   	trace_xfs_ag_resv_init(pag, type, ask);
>   	return 0;
> 
> --
> 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=57YhFRxyRqjdPfAPfs4wnqA9n455Z75z7qYPx5N5g5A&s=xlMBoLgUrvEbqMKbDc72qnV-gJsBNzbR1VFqvVF9XKg&e=
> 

[PATCH 3/8] xfs: don't trip over negative free space in xfs_reserve_blocks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If we somehow end up with a filesystem that has fewer free blocks than
the blocks set aside to avoid ENOSPC deadlocks, it's possible that the
free space calculation in xfs_reserve_blocks will spit out a negative
number (because percpu_counter_sum returns s64).  We fail to notice
this negative number and set fdblks_delta to it.  Now we increment
fdblocks(!) and the unsigned type of m_resblks means that we end up
setting a ridiculously huge m_resblks reservation.

Avoid this comedy of errors by detecting the negative free space and
returning -ENOSPC.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_fsops.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index a7afcad6b711..3f2bd6032cf8 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -387,7 +387,7 @@ xfs_reserve_blocks(
 	do {
 		free = percpu_counter_sum(&mp->m_fdblocks) -
 						mp->m_alloc_set_aside;
-		if (!free)
+		if (free <= 0)
 			break;
 
 		delta = request - mp->m_resblks;

Re: [PATCH 3/8] xfs: don't trip over negative free space in xfs_reserve_blocks

[Christoph Hellwig]

On Thu, Jun 21, 2018 at 11:31:22AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If we somehow end up with a filesystem that has fewer free blocks than
> the blocks set aside to avoid ENOSPC deadlocks, it's possible that the
> free space calculation in xfs_reserve_blocks will spit out a negative
> number (because percpu_counter_sum returns s64).  We fail to notice
> this negative number and set fdblks_delta to it.  Now we increment
> fdblocks(!) and the unsigned type of m_resblks means that we end up
> setting a ridiculously huge m_resblks reservation.
> 
> Avoid this comedy of errors by detecting the negative free space and
> returning -ENOSPC.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/8] xfs: don't trip over negative free space in xfs_reserve_blocks

[Allison Henderson]

Looks ok, thx!
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

On 06/21/2018 11:31 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If we somehow end up with a filesystem that has fewer free blocks than
> the blocks set aside to avoid ENOSPC deadlocks, it's possible that the
> free space calculation in xfs_reserve_blocks will spit out a negative
> number (because percpu_counter_sum returns s64).  We fail to notice
> this negative number and set fdblks_delta to it.  Now we increment
> fdblocks(!) and the unsigned type of m_resblks means that we end up
> setting a ridiculously huge m_resblks reservation.
> 
> Avoid this comedy of errors by detecting the negative free space and
> returning -ENOSPC.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_fsops.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index a7afcad6b711..3f2bd6032cf8 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -387,7 +387,7 @@ xfs_reserve_blocks(
>   	do {
>   		free = percpu_counter_sum(&mp->m_fdblocks) -
>   						mp->m_alloc_set_aside;
> -		if (!free)
> +		if (free <= 0)
>   			break;
>   
>   		delta = request - mp->m_resblks;
> 
> --
> 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=N6x3Cwn-CGLBE6LOOAbcetDshZ_SB_Mnv0Jn2FdPq38&s=CizYCC09hHj0NL-62QgWA-Tla03lpYgbx5ytFsHSFx0&e=
> 

[PATCH 4/8] xfs: don't allow insert-range to shift extents past the maximum offset

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Zorro Lang reports that generic/485 blows an assert on a filesystem with
512 byte blocks.  The test tries to fallocate a post-eof extent at the
maximum file size and calls insert range to shift the extents right by
two blocks.  On a 512b block filesystem this causes startoff to overflow
the 54-bit startoff field, leading to the assert.

Therefore, always check the rightmost extent to see if it would overflow
prior to invoking the insert range machinery.

Reported-by: zlang@redhat.com
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200137
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c |   40 ++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_bmap.h |    2 ++
 fs/xfs/xfs_bmap_util.c   |    4 ++++
 3 files changed, 46 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 01628f0c9a0c..b7f094e19bab 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5780,6 +5780,46 @@ xfs_bmap_collapse_extents(
 	return error;
 }
 
+#define BMBT_STARTOFF_MASK	((1ULL << BMBT_STARTOFF_BITLEN) - 1)
+/* Make sure we won't be right-shifting an extent past the maximum bound. */
+int
+xfs_bmap_can_insert_extents(
+	struct xfs_inode	*ip,
+	xfs_fileoff_t		off,
+	xfs_fileoff_t		shift)
+{
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	struct xfs_bmbt_irec	got;
+	struct xfs_iext_cursor	icur;
+	xfs_fileoff_t		new_off;
+	int			error = 0;
+
+	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
+		return -EIO;
+
+	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+
+	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
+		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
+		if (error)
+			goto out;
+	}
+
+	xfs_iext_last(ifp, &icur);
+	if (!xfs_iext_get_extent(ifp, &icur, &got) || off > got.br_startoff)
+		goto out;
+
+	/* Make sure we can't overflow the 54-bit offset field. */
+	new_off = (got.br_startoff + shift) & BMBT_STARTOFF_MASK;
+	if (new_off < got.br_startoff)
+		error = -EINVAL;
+
+out:
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	return error;
+}
+
 int
 xfs_bmap_insert_extents(
 	struct xfs_trans	*tp,
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 99dddbd0fcc6..9b49ddf99c41 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -227,6 +227,8 @@ int	xfs_bmap_collapse_extents(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
 		bool *done, xfs_fsblock_t *firstblock,
 		struct xfs_defer_ops *dfops);
+int	xfs_bmap_can_insert_extents(struct xfs_inode *ip, xfs_fileoff_t off,
+		xfs_fileoff_t shift);
 int	xfs_bmap_insert_extents(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
 		bool *done, xfs_fileoff_t stop_fsb, xfs_fsblock_t *firstblock,
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index c35009a86699..abc37b0899c0 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1404,6 +1404,10 @@ xfs_insert_file_space(
 
 	trace_xfs_insert_file_space(ip);
 
+	error = xfs_bmap_can_insert_extents(ip, stop_fsb, shift_fsb);
+	if (error)
+		return error;
+
 	error = xfs_prepare_shift(ip, offset);
 	if (error)
 		return error;

Re: [PATCH 4/8] xfs: don't allow insert-range to shift extents past the maximum offset

[Allison Henderson]

Ok, you can add my review
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

On 06/21/2018 11:31 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Zorro Lang reports that generic/485 blows an assert on a filesystem with
> 512 byte blocks.  The test tries to fallocate a post-eof extent at the
> maximum file size and calls insert range to shift the extents right by
> two blocks.  On a 512b block filesystem this causes startoff to overflow
> the 54-bit startoff field, leading to the assert.
> 
> Therefore, always check the rightmost extent to see if it would overflow
> prior to invoking the insert range machinery.
> 
> Reported-by: zlang@redhat.com
> Bugzilla: https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.kernel.org_show-5Fbug.cgi-3Fid-3D200137&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=YCaDC-F1dLxMWur8rwN6LkUZEkB0mCLT1kaX18afe3Y&s=21OAUtb7E_eZcNMI0iepr7J9fg90RHPo0hUJBgJxDrI&e=
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_bmap.c |   40 ++++++++++++++++++++++++++++++++++++++++
>   fs/xfs/libxfs/xfs_bmap.h |    2 ++
>   fs/xfs/xfs_bmap_util.c   |    4 ++++
>   3 files changed, 46 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 01628f0c9a0c..b7f094e19bab 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5780,6 +5780,46 @@ xfs_bmap_collapse_extents(
>   	return error;
>   }
>   
> +#define BMBT_STARTOFF_MASK	((1ULL << BMBT_STARTOFF_BITLEN) - 1)
> +/* Make sure we won't be right-shifting an extent past the maximum bound. */
> +int
> +xfs_bmap_can_insert_extents(
> +	struct xfs_inode	*ip,
> +	xfs_fileoff_t		off,
> +	xfs_fileoff_t		shift)
> +{
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +	struct xfs_bmbt_irec	got;
> +	struct xfs_iext_cursor	icur;
> +	xfs_fileoff_t		new_off;
> +	int			error = 0;
> +
> +	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> +		return -EIO;
> +
> +	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +
> +	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> +		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
> +		if (error)
> +			goto out;
> +	}
> +
> +	xfs_iext_last(ifp, &icur);
> +	if (!xfs_iext_get_extent(ifp, &icur, &got) || off > got.br_startoff)
> +		goto out;
> +
> +	/* Make sure we can't overflow the 54-bit offset field. */
> +	new_off = (got.br_startoff + shift) & BMBT_STARTOFF_MASK;
> +	if (new_off < got.br_startoff)
> +		error = -EINVAL;
> +
> +out:
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	return error;
> +}
> +
>   int
>   xfs_bmap_insert_extents(
>   	struct xfs_trans	*tp,
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 99dddbd0fcc6..9b49ddf99c41 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -227,6 +227,8 @@ int	xfs_bmap_collapse_extents(struct xfs_trans *tp, struct xfs_inode *ip,
>   		xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
>   		bool *done, xfs_fsblock_t *firstblock,
>   		struct xfs_defer_ops *dfops);
> +int	xfs_bmap_can_insert_extents(struct xfs_inode *ip, xfs_fileoff_t off,
> +		xfs_fileoff_t shift);
>   int	xfs_bmap_insert_extents(struct xfs_trans *tp, struct xfs_inode *ip,
>   		xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
>   		bool *done, xfs_fileoff_t stop_fsb, xfs_fsblock_t *firstblock,
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index c35009a86699..abc37b0899c0 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1404,6 +1404,10 @@ xfs_insert_file_space(
>   
>   	trace_xfs_insert_file_space(ip);
>   
> +	error = xfs_bmap_can_insert_extents(ip, stop_fsb, shift_fsb);
> +	if (error)
> +		return error;
> +
>   	error = xfs_prepare_shift(ip, offset);
>   	if (error)
>   		return error;
> 
> --
> 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=YCaDC-F1dLxMWur8rwN6LkUZEkB0mCLT1kaX18afe3Y&s=RhleurH5wYku58VywlbdBFmjaXSEGIfX7SCThJq1clE&e=
> 

Re: [PATCH 4/8] xfs: don't allow insert-range to shift extents past the maximum offset

[Christoph Hellwig]

> +#define BMBT_STARTOFF_MASK	((1ULL << BMBT_STARTOFF_BITLEN) - 1)

Please move this next to BMBT_STARTOFF_BITLEN.

For brownie points also use this new defined instead of
XFS_IEXT_STARTOFF_MASK in the extent tree code.


> +/* Make sure we won't be right-shifting an extent past the maximum bound. */
> +int
> +xfs_bmap_can_insert_extents(
> +	struct xfs_inode	*ip,
> +	xfs_fileoff_t		off,
> +	xfs_fileoff_t		shift)
> +{
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +	struct xfs_bmbt_irec	got;
> +	struct xfs_iext_cursor	icur;
> +	xfs_fileoff_t		new_off;
> +	int			error = 0;
> +
> +	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> +		return -EIO;
> +
> +	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +
> +	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> +		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
> +		if (error)
> +			goto out;
> +	}
> +
> +	xfs_iext_last(ifp, &icur);
> +	if (!xfs_iext_get_extent(ifp, &icur, &got) || off > got.br_startoff)
> +		goto out;

This largely duplicates xfs_bmap_last_extent.  The function body could
probably be written as:

{
	struct xfs_bmbt_irec	got;
	int			error = 0;

	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));

	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
		return -EIO;

	xfs_ilock(ip, XFS_ILOCK_EXCL);
	error = xfs_bmap_last_extent(NULL, ip, XFS_DATA_FORK, &got,
			&is_empty)
	if (!error && !is_empty && got.br_startoff >= off &&
	    got.br_startoff + shift) & BMBT_STARTOFF_MASK < got.br_startoff)
		error = -EINVAL;
	xfs_iunlock(ip, XFS_ILOCK_EXCL);

	return error;

Re: [PATCH 4/8] xfs: don't allow insert-range to shift extents past the maximum offset

[Darrick J. Wong]

On Thu, Jun 21, 2018 at 11:39:34PM -0700, Christoph Hellwig wrote:
> > +#define BMBT_STARTOFF_MASK	((1ULL << BMBT_STARTOFF_BITLEN) - 1)
> 
> Please move this next to BMBT_STARTOFF_BITLEN.
> 
> For brownie points also use this new defined instead of
> XFS_IEXT_STARTOFF_MASK in the extent tree code.

Separate cleanup patch, but ok.

> 
> > +/* Make sure we won't be right-shifting an extent past the maximum bound. */
> > +int
> > +xfs_bmap_can_insert_extents(
> > +	struct xfs_inode	*ip,
> > +	xfs_fileoff_t		off,
> > +	xfs_fileoff_t		shift)
> > +{
> > +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> > +	struct xfs_bmbt_irec	got;
> > +	struct xfs_iext_cursor	icur;
> > +	xfs_fileoff_t		new_off;
> > +	int			error = 0;
> > +
> > +	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> > +		return -EIO;
> > +
> > +	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> > +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > +
> > +	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> > +		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
> > +		if (error)
> > +			goto out;
> > +	}
> > +
> > +	xfs_iext_last(ifp, &icur);
> > +	if (!xfs_iext_get_extent(ifp, &icur, &got) || off > got.br_startoff)
> > +		goto out;
> 
> This largely duplicates xfs_bmap_last_extent.  The function body could
> probably be written as:
> 
> {
> 	struct xfs_bmbt_irec	got;
> 	int			error = 0;
> 
> 	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> 
> 	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
> 		return -EIO;
> 
> 	xfs_ilock(ip, XFS_ILOCK_EXCL);
> 	error = xfs_bmap_last_extent(NULL, ip, XFS_DATA_FORK, &got,
> 			&is_empty)
> 	if (!error && !is_empty && got.br_startoff >= off &&
> 	    got.br_startoff + shift) & BMBT_STARTOFF_MASK < got.br_startoff)
> 		error = -EINVAL;
> 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> 
> 	return error;

I'll give this a test run over night.

--D

> --
> 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

[PATCH v2 4/8] xfs: don't allow insert-range to shift extents past the maximum offset

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Zorro Lang reports that generic/485 blows an assert on a filesystem with
512 byte blocks.  The test tries to fallocate a post-eof extent at the
maximum file size and calls insert range to shift the extents right by
two blocks.  On a 512b block filesystem this causes startoff to overflow
the 54-bit startoff field, leading to the assert.

Therefore, always check the rightmost extent to see if it would overflow
prior to invoking the insert range machinery.

Reported-by: zlang@redhat.com
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200137
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
---
v2: use xfs_bmap_last_extent, move macro defs to headers
---
 fs/xfs/libxfs/xfs_bmap.c   |   26 ++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_bmap.h   |    2 ++
 fs/xfs/libxfs/xfs_format.h |    2 ++
 fs/xfs/xfs_bmap_util.c     |    4 ++++
 4 files changed, 34 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 01628f0c9a0c..7205268b30bc 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5780,6 +5780,32 @@ xfs_bmap_collapse_extents(
 	return error;
 }
 
+/* Make sure we won't be right-shifting an extent past the maximum bound. */
+int
+xfs_bmap_can_insert_extents(
+	struct xfs_inode	*ip,
+	xfs_fileoff_t		off,
+	xfs_fileoff_t		shift)
+{
+	struct xfs_bmbt_irec	got;
+	int			is_empty;
+	int			error = 0;
+
+	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+
+	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
+		return -EIO;
+
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	error = xfs_bmap_last_extent(NULL, ip, XFS_DATA_FORK, &got, &is_empty);
+	if (!error && !is_empty && got.br_startoff >= off &&
+	    ((got.br_startoff + shift) & BMBT_STARTOFF_MASK) < got.br_startoff)
+		error = -EINVAL;
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+
+	return error;
+}
+
 int
 xfs_bmap_insert_extents(
 	struct xfs_trans	*tp,
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 99dddbd0fcc6..9b49ddf99c41 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -227,6 +227,8 @@ int	xfs_bmap_collapse_extents(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
 		bool *done, xfs_fsblock_t *firstblock,
 		struct xfs_defer_ops *dfops);
+int	xfs_bmap_can_insert_extents(struct xfs_inode *ip, xfs_fileoff_t off,
+		xfs_fileoff_t shift);
 int	xfs_bmap_insert_extents(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
 		bool *done, xfs_fileoff_t stop_fsb, xfs_fsblock_t *firstblock,
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 7b4a43deb83e..059bc44c27e8 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -1529,6 +1529,8 @@ typedef struct xfs_bmdr_block {
 #define BMBT_STARTBLOCK_BITLEN	52
 #define BMBT_BLOCKCOUNT_BITLEN	21
 
+#define BMBT_STARTOFF_MASK	((1ULL << BMBT_STARTOFF_BITLEN) - 1)
+
 typedef struct xfs_bmbt_rec {
 	__be64			l0, l1;
 } xfs_bmbt_rec_t;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index c195b9d857af..bb417156e3bf 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1383,6 +1383,10 @@ xfs_insert_file_space(
 
 	trace_xfs_insert_file_space(ip);
 
+	error = xfs_bmap_can_insert_extents(ip, stop_fsb, shift_fsb);
+	if (error)
+		return error;
+
 	error = xfs_prepare_shift(ip, offset);
 	if (error)
 		return error;

Re: [PATCH v2 4/8] xfs: don't allow insert-range to shift extents past the maximum offset

[Christoph Hellwig]

On Fri, Jun 22, 2018 at 02:21:04PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Zorro Lang reports that generic/485 blows an assert on a filesystem with
> 512 byte blocks.  The test tries to fallocate a post-eof extent at the
> maximum file size and calls insert range to shift the extents right by
> two blocks.  On a 512b block filesystem this causes startoff to overflow
> the 54-bit startoff field, leading to the assert.
> 
> Therefore, always check the rightmost extent to see if it would overflow
> prior to invoking the insert range machinery.
> 
> Reported-by: zlang@redhat.com
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200137
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 5/8] xfs: recheck reflink state after grabbing ILOCK_SHARED for a write

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The reflink iflag could have changed since the earlier unlocked check,
so if we got ILOCK_SHARED for a write and but we're now a reflink inode
we have to switch to ILOCK_EXCL and relock.

This helps us avoid blowing lock assertions in things like generic/166:

XFS: Assertion failed: xfs_isilocked(ip, XFS_ILOCK_EXCL), file: fs/xfs/xfs_reflink.c, line: 383
WARNING: CPU: 1 PID: 24707 at fs/xfs/xfs_message.c:104 assfail+0x25/0x30 [xfs]
Modules linked in: deadline_iosched dm_snapshot dm_bufio ext4 mbcache jbd2 dm_flakey xfs libcrc32c dax_pmem device_dax nd_pmem sch_fq_codel af_packet [last unloaded: scsi_debug]
CPU: 1 PID: 24707 Comm: xfs_io Not tainted 4.18.0-rc1-djw #1
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1 04/01/2014
RIP: 0010:assfail+0x25/0x30 [xfs]
Code: ff 0f 0b c3 90 66 66 66 66 90 48 89 f1 41 89 d0 48 c7 c6 e8 ef 1b a0 48 89 fa 31 ff e8 54 f9 ff ff 80 3d fd ba 0f 00 00 75 03 <0f> 0b c3 0f 0b 66 0f 1f 44 00 00 66 66 66 66 90 48 63 f6 49 89 f9
RSP: 0018:ffffc90006423ad8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff880030b65e80 RCX: 0000000000000000
RDX: 00000000ffffffc0 RSI: 000000000000000a RDI: ffffffffa01b0447
RBP: ffffc90006423c10 R08: 0000000000000000 R09: 0000000000000000
R10: ffff88003d43fc30 R11: f000000000000000 R12: ffff880077cda000
R13: 0000000000000000 R14: ffffc90006423c30 R15: ffffc90006423bf9
FS:  00007feba8986800(0000) GS:ffff88003ec00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000138ab58 CR3: 000000003d40a000 CR4: 00000000000006a0
Call Trace:
 xfs_reflink_allocate_cow+0x24c/0x3d0 [xfs]
 xfs_file_iomap_begin+0x6d2/0xeb0 [xfs]
 ? iomap_to_fiemap+0x80/0x80
 iomap_apply+0x5e/0x130
 iomap_dio_rw+0x2e0/0x400
 ? iomap_to_fiemap+0x80/0x80
 ? xfs_file_dio_aio_write+0x133/0x4a0 [xfs]
 xfs_file_dio_aio_write+0x133/0x4a0 [xfs]
 xfs_file_write_iter+0x7b/0xb0 [xfs]
 __vfs_write+0x16f/0x1f0
 vfs_write+0xc8/0x1c0
 ksys_pwrite64+0x74/0x90
 do_syscall_64+0x56/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_iomap.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 49f5492eed3b..55876dd02f0c 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -963,12 +963,13 @@ xfs_ilock_for_iomap(
 	unsigned		*lockmode)
 {
 	unsigned		mode = XFS_ILOCK_SHARED;
+	bool			is_write = flags & (IOMAP_WRITE | IOMAP_ZERO);
 
 	/*
 	 * COW writes may allocate delalloc space or convert unwritten COW
 	 * extents, so we need to make sure to take the lock exclusively here.
 	 */
-	if (xfs_is_reflink_inode(ip) && (flags & (IOMAP_WRITE | IOMAP_ZERO))) {
+	if (xfs_is_reflink_inode(ip) && is_write) {
 		/*
 		 * FIXME: It could still overwrite on unshared extents and not
 		 * need allocation.
@@ -989,6 +990,7 @@ xfs_ilock_for_iomap(
 		mode = XFS_ILOCK_EXCL;
 	}
 
+relock:
 	if (flags & IOMAP_NOWAIT) {
 		if (!xfs_ilock_nowait(ip, mode))
 			return -EAGAIN;
@@ -996,6 +998,17 @@ xfs_ilock_for_iomap(
 		xfs_ilock(ip, mode);
 	}
 
+	/*
+	 * The reflink iflag could have changed since the earlier unlocked
+	 * check, so if we got ILOCK_SHARED for a write and but we're now a
+	 * reflink inode we have to switch to ILOCK_EXCL and relock.
+	 */
+	if (mode == XFS_ILOCK_SHARED && is_write && xfs_is_reflink_inode(ip)) {
+		xfs_iunlock(ip, mode);
+		mode = XFS_ILOCK_EXCL;
+		goto relock;
+	}
+
 	*lockmode = mode;
 	return 0;
 }

Re: [PATCH 5/8] xfs: recheck reflink state after grabbing ILOCK_SHARED for a write

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 6/8] xfs: fix uninitialized field in rtbitmap fsmap backend

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Initialize the extent count field of the high key so that when we use
the high key to synthesize an 'unknown owner' record (i.e. used space
record) at the end of the queried range we have a field with which to
compute rm_blockcount.  This is not strictly necessary because the
synthesizer never uses the rm_blockcount field, but we can shut up the
static code analysis anyway.

Coverity-id: 1437358
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_fsmap.c |    1 +
 1 file changed, 1 insertion(+)


diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index c34fa9c342f2..2be795b01ce0 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -531,6 +531,7 @@ xfs_getfsmap_rtdev_rtbitmap_query(
 
 	/* Report any gaps at the end of the rtbitmap */
 	info->last = true;
+	ahigh.ar_extcount = 0;
 	error = xfs_getfsmap_rtdev_rtbitmap_helper(tp, &ahigh, info);
 	if (error)
 		goto err;

Re: [PATCH 6/8] xfs: fix uninitialized field in rtbitmap fsmap backend

[Allison Henderson]

Looks ok:
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

On 06/21/2018 11:31 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Initialize the extent count field of the high key so that when we use
> the high key to synthesize an 'unknown owner' record (i.e. used space
> record) at the end of the queried range we have a field with which to
> compute rm_blockcount.  This is not strictly necessary because the
> synthesizer never uses the rm_blockcount field, but we can shut up the
> static code analysis anyway.
> 
> Coverity-id: 1437358
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_fsmap.c |    1 +
>   1 file changed, 1 insertion(+)
> 
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index c34fa9c342f2..2be795b01ce0 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -531,6 +531,7 @@ xfs_getfsmap_rtdev_rtbitmap_query(
>   
>   	/* Report any gaps at the end of the rtbitmap */
>   	info->last = true;
> +	ahigh.ar_extcount = 0;
>   	error = xfs_getfsmap_rtdev_rtbitmap_helper(tp, &ahigh, info);
>   	if (error)
>   		goto err;
> 
> --
> 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=kG0q-nvJIi8c2zctskm0vTRxZIBqEK8mud41-TvKgAI&s=c2QBy7jBkpo_q56thW4Lrp4PTZl40Ulep8xufwE5NVc&e=
> 

Re: [PATCH 6/8] xfs: fix uninitialized field in rtbitmap fsmap backend

[Christoph Hellwig]

On Thu, Jun 21, 2018 at 11:31:41AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Initialize the extent count field of the high key so that when we use
> the high key to synthesize an 'unknown owner' record (i.e. used space
> record) at the end of the queried range we have a field with which to
> compute rm_blockcount.  This is not strictly necessary because the
> synthesizer never uses the rm_blockcount field, but we can shut up the
> static code analysis anyway.
> 
> Coverity-id: 1437358
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I'd rather ensure the structs are entirely zeroed to start with, e.g.:

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 0299febece9c..21bbdee451a6 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -527,8 +527,8 @@ xfs_getfsmap_rtdev_rtbitmap_query(
 	struct xfs_trans		*tp,
 	struct xfs_getfsmap_info	*info)
 {
-	struct xfs_rtalloc_rec		alow;
-	struct xfs_rtalloc_rec		ahigh;
+	struct xfs_rtalloc_rec		alow = { 0, };
+	struct xfs_rtalloc_rec		ahigh = { 0, };
 	int				error;
 
 	xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);

Re: [PATCH 6/8] xfs: fix uninitialized field in rtbitmap fsmap backend

[Darrick J. Wong]

On Thu, Jun 21, 2018 at 11:41:18PM -0700, Christoph Hellwig wrote:
> On Thu, Jun 21, 2018 at 11:31:41AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Initialize the extent count field of the high key so that when we use
> > the high key to synthesize an 'unknown owner' record (i.e. used space
> > record) at the end of the queried range we have a field with which to
> > compute rm_blockcount.  This is not strictly necessary because the
> > synthesizer never uses the rm_blockcount field, but we can shut up the
> > static code analysis anyway.
> > 
> > Coverity-id: 1437358
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I'd rather ensure the structs are entirely zeroed to start with, e.g.:
> 
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 0299febece9c..21bbdee451a6 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -527,8 +527,8 @@ xfs_getfsmap_rtdev_rtbitmap_query(
>  	struct xfs_trans		*tp,
>  	struct xfs_getfsmap_info	*info)
>  {
> -	struct xfs_rtalloc_rec		alow;
> -	struct xfs_rtalloc_rec		ahigh;
> +	struct xfs_rtalloc_rec		alow = { 0, };
> +	struct xfs_rtalloc_rec		ahigh = { 0, };

Looks reasonable, I'll give it a spin overnight.

--D

>  	int				error;
>  
>  	xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
> --
> 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

[PATCH v2 6/8] xfs: fix uninitialized field in rtbitmap fsmap backend

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Initialize the extent count field of the high key so that when we use
the high key to synthesize an 'unknown owner' record (i.e. used space
record) at the end of the queried range we have a field with which to
compute rm_blockcount.  This is not strictly necessary because the
synthesizer never uses the rm_blockcount field, but we can shut up the
static code analysis anyway.

Coverity-id: 1437358
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
---
v2: init both structs to zero
---
 fs/xfs/xfs_fsmap.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index c34fa9c342f2..c7157bc48bd1 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -513,8 +513,8 @@ xfs_getfsmap_rtdev_rtbitmap_query(
 	struct xfs_trans		*tp,
 	struct xfs_getfsmap_info	*info)
 {
-	struct xfs_rtalloc_rec		alow;
-	struct xfs_rtalloc_rec		ahigh;
+	struct xfs_rtalloc_rec		alow = { 0 };
+	struct xfs_rtalloc_rec		ahigh = { 0 };
 	int				error;
 
 	xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);

Re: [PATCH v2 6/8] xfs: fix uninitialized field in rtbitmap fsmap backend

[Christoph Hellwig]

On Fri, Jun 22, 2018 at 02:22:22PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Initialize the extent count field of the high key so that when we use
> the high key to synthesize an 'unknown owner' record (i.e. used space
> record) at the end of the queried range we have a field with which to
> compute rm_blockcount.  This is not strictly necessary because the
> synthesizer never uses the rm_blockcount field, but we can shut up the
> static code analysis anyway.
> 
> Coverity-id: 1437358
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 7/8] xfs: fix off-by-one error in xfs_rtalloc_query_range

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In commit 8ad560d2565e6 ("xfs: strengthen rtalloc query range checks")
we strengthened the input parameter checks in the rtbitmap range query
function, but introduced an off-by-one error in the process.  The call
to xfs_rtfind_forw deals with the high key being rextents, but we clamp
the high key to rextents - 1.  This causes the returned results to stop
one block short of the end of the rtdev, which is incorrect.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_rtbitmap.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 65fc4ed2e9a1..b228c821bae6 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -1029,8 +1029,8 @@ xfs_rtalloc_query_range(
 	if (low_rec->ar_startext >= mp->m_sb.sb_rextents ||
 	    low_rec->ar_startext == high_rec->ar_startext)
 		return 0;
-	if (high_rec->ar_startext >= mp->m_sb.sb_rextents)
-		high_rec->ar_startext = mp->m_sb.sb_rextents - 1;
+	if (high_rec->ar_startext > mp->m_sb.sb_rextents)
+		high_rec->ar_startext = mp->m_sb.sb_rextents;
 
 	/* Iterate the bitmap, looking for discrepancies. */
 	rtstart = low_rec->ar_startext;

Re: [PATCH 7/8] xfs: fix off-by-one error in xfs_rtalloc_query_range

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 7/8] xfs: fix off-by-one error in xfs_rtalloc_query_range

[Allison Henderson]

Looks good:
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

On 06/21/2018 11:31 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit 8ad560d2565e6 ("xfs: strengthen rtalloc query range checks")
> we strengthened the input parameter checks in the rtbitmap range query
> function, but introduced an off-by-one error in the process.  The call
> to xfs_rtfind_forw deals with the high key being rextents, but we clamp
> the high key to rextents - 1.  This causes the returned results to stop
> one block short of the end of the rtdev, which is incorrect.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_rtbitmap.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 65fc4ed2e9a1..b228c821bae6 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1029,8 +1029,8 @@ xfs_rtalloc_query_range(
>   	if (low_rec->ar_startext >= mp->m_sb.sb_rextents ||
>   	    low_rec->ar_startext == high_rec->ar_startext)
>   		return 0;
> -	if (high_rec->ar_startext >= mp->m_sb.sb_rextents)
> -		high_rec->ar_startext = mp->m_sb.sb_rextents - 1;
> +	if (high_rec->ar_startext > mp->m_sb.sb_rextents)
> +		high_rec->ar_startext = mp->m_sb.sb_rextents;
>   
>   	/* Iterate the bitmap, looking for discrepancies. */
>   	rtstart = low_rec->ar_startext;
> 
> --
> 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=zsOQBNyhInhyExIQUb5j_oNmILj9vGQF3A7q32gZLuQ&s=JMHXMs2LK6ulhFTcRhasFM3G1BC2jgwHvrD9CU3INrk&e=
> 

[PATCH 8/8] xfs: ensure post-EOF zeroing happens after zeroing part of a file

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If a user asks us to zero_range part of a file, the end of the range is
EOF, and not aligned to a page boundary, invoke writeback of the EOF
page to ensure that the post-EOF part of the page is zeroed.  This
ensures that we don't expose stale memory contents via mmap, if in a
clumsy manner.

Found by running generic/127 when it runs zero_range and mapread at EOF
one after the other.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_bmap_util.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index abc37b0899c0..c94d376e4152 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1208,7 +1208,20 @@ xfs_free_file_space(
 		return 0;
 	if (offset + len > XFS_ISIZE(ip))
 		len = XFS_ISIZE(ip) - offset;
-	return iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
+	error = iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
+	if (error)
+		return error;
+
+	/*
+	 * If we zeroed right up to EOF and EOF straddles a page boundary we
+	 * must make sure that the post-EOF area is also zeroed because the
+	 * page could be mmap'd and iomap_zero_range doesn't do that for us.
+	 * Writeback of the eof page will do this, albeit clumsily.
+	 */
+	if (offset + len < XFS_ISIZE(ip) || ((offset + len) & PAGE_MASK) == 0)
+		return 0;
+	return filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
+			(offset + len) & ~PAGE_MASK, LLONG_MAX);
 }
 
 /*

Re: [PATCH 8/8] xfs: ensure post-EOF zeroing happens after zeroing part of a file

[Christoph Hellwig]

On Thu, Jun 21, 2018 at 11:31:53AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If a user asks us to zero_range part of a file, the end of the range is
> EOF, and not aligned to a page boundary, invoke writeback of the EOF
> page to ensure that the post-EOF part of the page is zeroed.  This
> ensures that we don't expose stale memory contents via mmap, if in a
> clumsy manner.
> 
> Found by running generic/127 when it runs zero_range and mapread at EOF
> one after the other.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_bmap_util.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index abc37b0899c0..c94d376e4152 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1208,7 +1208,20 @@ xfs_free_file_space(
>  		return 0;
>  	if (offset + len > XFS_ISIZE(ip))
>  		len = XFS_ISIZE(ip) - offset;
> -	return iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
> +	error = iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * If we zeroed right up to EOF and EOF straddles a page boundary we
> +	 * must make sure that the post-EOF area is also zeroed because the
> +	 * page could be mmap'd and iomap_zero_range doesn't do that for us.
> +	 * Writeback of the eof page will do this, albeit clumsily.
> +	 */
> +	if (offset + len < XFS_ISIZE(ip) || ((offset + len) & PAGE_MASK) == 0)
> +		return 0;
> +	return filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
> +			(offset + len) & ~PAGE_MASK, LLONG_MAX);

Total nitpick, but between the kernel logic flow and comment I'd invert
the check:

	if (offset + len >= XFS_ISIZE(ip) && (offset + len) & PAGE_MASK) {
		error = filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
			(offset + len) & ~PAGE_MASK, LLONG_MAX);
	}

	return error;

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 8/8] xfs: ensure post-EOF zeroing happens after zeroing part of a file

[Darrick J. Wong]

On Thu, Jun 21, 2018 at 11:29:07PM -0700, Christoph Hellwig wrote:
> On Thu, Jun 21, 2018 at 11:31:53AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > If a user asks us to zero_range part of a file, the end of the range is
> > EOF, and not aligned to a page boundary, invoke writeback of the EOF
> > page to ensure that the post-EOF part of the page is zeroed.  This
> > ensures that we don't expose stale memory contents via mmap, if in a
> > clumsy manner.
> > 
> > Found by running generic/127 when it runs zero_range and mapread at EOF
> > one after the other.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_bmap_util.c |   15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index abc37b0899c0..c94d376e4152 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1208,7 +1208,20 @@ xfs_free_file_space(
> >  		return 0;
> >  	if (offset + len > XFS_ISIZE(ip))
> >  		len = XFS_ISIZE(ip) - offset;
> > -	return iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
> > +	error = iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
> > +	if (error)
> > +		return error;
> > +
> > +	/*
> > +	 * If we zeroed right up to EOF and EOF straddles a page boundary we
> > +	 * must make sure that the post-EOF area is also zeroed because the
> > +	 * page could be mmap'd and iomap_zero_range doesn't do that for us.
> > +	 * Writeback of the eof page will do this, albeit clumsily.
> > +	 */
> > +	if (offset + len < XFS_ISIZE(ip) || ((offset + len) & PAGE_MASK) == 0)
> > +		return 0;
> > +	return filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
> > +			(offset + len) & ~PAGE_MASK, LLONG_MAX);
> 
> Total nitpick, but between the kernel logic flow and comment I'd invert
> the check:
> 
> 	if (offset + len >= XFS_ISIZE(ip) && (offset + len) & PAGE_MASK) {
> 		error = filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
> 			(offset + len) & ~PAGE_MASK, LLONG_MAX);
> 	}
> 
> 	return error;
> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Ok, will update & test overnight.

--D

> --
> 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

Re: [PATCH 8/8] xfs: ensure post-EOF zeroing happens after zeroing part of a file

[Allison Henderson]

Looks ok to me. Thx!

Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

On 06/21/2018 11:31 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If a user asks us to zero_range part of a file, the end of the range is
> EOF, and not aligned to a page boundary, invoke writeback of the EOF
> page to ensure that the post-EOF part of the page is zeroed.  This
> ensures that we don't expose stale memory contents via mmap, if in a
> clumsy manner.
> 
> Found by running generic/127 when it runs zero_range and mapread at EOF
> one after the other.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/xfs_bmap_util.c |   15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index abc37b0899c0..c94d376e4152 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1208,7 +1208,20 @@ xfs_free_file_space(
>   		return 0;
>   	if (offset + len > XFS_ISIZE(ip))
>   		len = XFS_ISIZE(ip) - offset;
> -	return iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
> +	error = iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * If we zeroed right up to EOF and EOF straddles a page boundary we
> +	 * must make sure that the post-EOF area is also zeroed because the
> +	 * page could be mmap'd and iomap_zero_range doesn't do that for us.
> +	 * Writeback of the eof page will do this, albeit clumsily.
> +	 */
> +	if (offset + len < XFS_ISIZE(ip) || ((offset + len) & PAGE_MASK) == 0)
> +		return 0;
> +	return filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
> +			(offset + len) & ~PAGE_MASK, LLONG_MAX);
>   }
>   
>   /*
> 
> --
> 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  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=wue-_aW-3CpAS-pXEQXCk8gKGGwIqQ1nh1DhJ-3JtBU&s=q874s9Hzp4N6x29WZ6DqXxY7xh1oCZSwFHNb6ZqnTs4&e=
> 

[PATCH 9/8] xfs: check leaf attribute block freemap in verifier

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Check the leaf attribute freemap when we we're verifying the block.
Found via generic/070.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 76e90046731c..b3c19339e1b5 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -244,6 +244,8 @@ xfs_attr3_leaf_verify(
 	struct xfs_attr_leafblock	*leaf = bp->b_addr;
 	struct xfs_perag		*pag = bp->b_pag;
 	struct xfs_attr_leaf_entry	*entries;
+	uint16_t			end;
+	int				i;
 
 	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
 
@@ -289,6 +291,22 @@ xfs_attr3_leaf_verify(
 	/* XXX: need to range check rest of attr header values */
 	/* XXX: hash order check? */
 
+	for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
+		if (ichdr.freemap[i].base > mp->m_attr_geo->blksize)
+			return __this_address;
+		if (ichdr.freemap[i].base & 0x3)
+			return __this_address;
+		if (ichdr.freemap[i].size > mp->m_attr_geo->blksize)
+			return __this_address;
+		if (ichdr.freemap[i].size & 0x3)
+			return __this_address;
+		end = ichdr.freemap[i].base + ichdr.freemap[i].size;
+		if (end < ichdr.freemap[i].base)
+			return __this_address;
+		if (end > mp->m_attr_geo->blksize)
+			return __this_address;
+	}
+
 	return NULL;
 }
 

[PATCH 0/4] fstests: fixes and new tests

[Darrick J. Wong]

Hi all,

The first patch fixes xfs/365 to fuzz the AG1 inobt since as of 4.18 XFS
requires an intact AG0 inobt even to mount.

The second patch provides a regression test for "xfs: fix fdblocks
accounting w/ RMAPBT per-AG reservation".

The third patch provides a regression test for a condition where mmap
read of a file's EOF page shows nonzero bytes after EOF if the
previous operation was a zero_range all the way to EOF.  This bug only
seems to manifest on XFS, but it uses generic APIs and so is a generic
test.  The patch concerned here is "xfs: ensure post-EOF zeroing happens
after zeroing part of a file".

The fourth patch adds a variant to the xfs fuzz tests where we fuzz the
metadata fields and immediately try to write to the filesystem.

--D

[PATCH 2/4] xfs: regression test for rmapbt fdblocks accounting problems

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In "xfs: fix fdblocks accounting w/ RMAPBT per-AG reservation", we fixed
the per-ag reservation code so that we always decrease fdblocks by the
reserved size because rmapbt blocks are counted as free space.

The primary symptom of this bug is that if the rmapbt has expanded since
mount time, the disk block counters reported via statfs will change
across a remount.  Therefore, we exercise this as a regression test.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/xfs/712     |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/712.out |    1 +
 tests/xfs/group   |    1 +
 3 files changed, 64 insertions(+)
 create mode 100755 tests/xfs/712
 create mode 100644 tests/xfs/712.out


diff --git a/tests/xfs/712 b/tests/xfs/712
new file mode 100755
index 00000000..f90c7ec9
--- /dev/null
+++ b/tests/xfs/712
@@ -0,0 +1,62 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Oracle, Inc.  All rights reserved.
+#
+# FS QA Test No. 712
+#
+# Make sure that the statfs b_avail counter doesn't change across remount
+# after the rmapbt has grown in size.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous \$seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_os Linux
+_supported_fs xfs
+_require_test_program "punch-alternating"
+_require_xfs_scratch_rmapbt
+
+_scratch_mkfs >> $seqres.full
+_scratch_mount
+
+# Make sure we allocate a big enough file such that punching out every other
+# block will cause the rmapbt to expand by at least one block.
+blksz=$(_get_file_block_size $SCRATCH_MNT)
+nr_rmap_per_rmapbt=$(( (blksz - 56) / 24))
+blks=$(( (nr_rmap_per_rmapbt + 2) * 2))
+$XFS_IO_PROG -f -c "falloc 0 $((blks * blksz))" $SCRATCH_MNT/a >> $seqres.full
+./src/punch-alternating $SCRATCH_MNT/a
+
+# Do the stat counters change across the remount?
+before=$(stat -f -c '%b %f' $SCRATCH_MNT)
+_scratch_cycle_mount
+after=$(stat -f -c '%b %f' $SCRATCH_MNT)
+
+if [ "${before}" != "${after}" ]; then
+	echo "counters changed: ${before} ${after}"
+	echo "before: ${before}" >> $seqres.full
+	echo "after: ${after}" >> $seqres.full
+fi
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/712.out b/tests/xfs/712.out
new file mode 100644
index 00000000..4e900419
--- /dev/null
+++ b/tests/xfs/712.out
@@ -0,0 +1 @@
+QA output created by 712
diff --git a/tests/xfs/group b/tests/xfs/group
index 932ab909..2f1c8467 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -447,3 +447,4 @@
 447 auto mount
 448 auto quick fuzzers
 449 auto quick
+712 auto quick rmap

Re: [PATCH 5/8] xfs: recheck reflink state after grabbing ILOCK_SHARED for a write

[Allison Henderson]

On 06/21/2018 11:31 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In "xfs: fix fdblocks accounting w/ RMAPBT per-AG reservation", we fixed
> the per-ag reservation code so that we always decrease fdblocks by the
> reserved size because rmapbt blocks are counted as free space.
> 
> The primary symptom of this bug is that if the rmapbt has expanded since
> mount time, the disk block counters reported via statfs will change
> across a remount.  Therefore, we exercise this as a regression test.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   tests/xfs/712     |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   tests/xfs/712.out |    1 +
>   tests/xfs/group   |    1 +
>   3 files changed, 64 insertions(+)
>   create mode 100755 tests/xfs/712
>   create mode 100644 tests/xfs/712.out

Humm, was there supposed to be more to this maybe?  Shouldn't we see a 
712.out somewhere?
> 
> 
> diff --git a/tests/xfs/712 b/tests/xfs/712
> new file mode 100755
> index 00000000..f90c7ec9
> --- /dev/null
> +++ b/tests/xfs/712
> @@ -0,0 +1,62 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2018 Oracle, Inc.  All rights reserved.
> +#
> +# FS QA Test No. 712
> +#
> +# Make sure that the statfs b_avail counter doesn't change across remount
> +# after the rmapbt has grown in size.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remo
> 

Re: [PATCH 5/8] xfs: recheck reflink state after grabbing ILOCK_SHARED for a write

[Darrick J. Wong]

On Thu, Jun 21, 2018 at 11:35:38PM -0700, Allison Henderson wrote:
> On 06/21/2018 11:31 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In "xfs: fix fdblocks accounting w/ RMAPBT per-AG reservation", we fixed
> > the per-ag reservation code so that we always decrease fdblocks by the
> > reserved size because rmapbt blocks are counted as free space.
> > 
> > The primary symptom of this bug is that if the rmapbt has expanded since
> > mount time, the disk block counters reported via statfs will change
> > across a remount.  Therefore, we exercise this as a regression test.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >   tests/xfs/712     |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   tests/xfs/712.out |    1 +
> >   tests/xfs/group   |    1 +
> >   3 files changed, 64 insertions(+)
> >   create mode 100755 tests/xfs/712
> >   create mode 100644 tests/xfs/712.out
> 
> Humm, was there supposed to be more to this maybe?  Shouldn't we see a
> 712.out somewhere?
> > 
> > 
> > diff --git a/tests/xfs/712 b/tests/xfs/712
> > new file mode 100755
> > index 00000000..f90c7ec9
> > --- /dev/null
> > +++ b/tests/xfs/712
> > @@ -0,0 +1,62 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2018 Oracle, Inc.  All rights reserved.
> > +#
> > +# FS QA Test No. 712
> > +#
> > +# Make sure that the statfs b_avail counter doesn't change across remount
> > +# after the rmapbt has grown in size.
> > +#
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1	# failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remo
> > 

Hmm, well, you did actually catch that I omitted the usual "Silence is
golden" but I do wonder about this cutting off right there....

Will send v2.

--D

> --
> 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