[PATCH 0/4] xfs-4.18: fix rtdev programming errors

[PATCH 0/4] xfs-4.18: fix rtdev programming errors

[Darrick J. Wong]

Hi all,

This is a fairly short series to fix some problems I found while playing
with the realtime device on a SMR drive with one rt extent per zone.
The first three patches fix various programming mistakes, and the fourth
one deals with unit analysis confusion in the rtalloc range query
functions, which only pop up if rextsize != 1.

Comments and questions are, as always, welcome.

--D

[PATCH 1/4] xfs: xfs_rtword_t should be unsigned, not signed

[Darrick J. Wong]

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

xfs_rtword_t is used for bit manipulations in the realtime bitmap file.
Since we're performing bit shifts with this type, we don't want sign
extension and we don't want to be left shifting negative quantities
because that's undefined behavior.

This also shuts up these UBSAN warnings:
UBSAN: Undefined behaviour in fs/xfs/libxfs/xfs_rtbitmap.c:833:48
signed integer overflow:
-2147483648 - 1 cannot be represented in type 'int'

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


diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index 3c560695c546..ea18449bd732 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -30,7 +30,7 @@ typedef int64_t		xfs_fsize_t;	/* bytes in a file */
 typedef uint64_t	xfs_ufsize_t;	/* unsigned bytes in a file */
 
 typedef int32_t		xfs_suminfo_t;	/* type of bitmap summary info */
-typedef int32_t		xfs_rtword_t;	/* word type for bitmap manipulations */
+typedef uint32_t	xfs_rtword_t;	/* word type for bitmap manipulations */
 
 typedef int64_t		xfs_lsn_t;	/* log sequence number */
 typedef int32_t		xfs_tid_t;	/* transaction identifier */

[PATCH 2/4] xfs: xfs_rtbuf_get should check the bmapi_read results

[Darrick J. Wong]

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

The xfs_rtbuf_get function should check the block mapping it gets back
from bmapi_read.  If there are no mappings or the mapping isn't a real
extent, we should return -EFSCORRUPTED rather than trying to read a
garbage value.  We also require realtime bitmap blocks to be real,
written allocations.

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


diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 106be2d0bb88..7712f282d172 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -90,6 +90,9 @@ xfs_rtbuf_get(
 	if (error)
 		return error;
 
+	if (nmap == 0 || !xfs_bmap_is_real_extent(&map))
+		return -EFSCORRUPTED;
+
 	ASSERT(map.br_startblock != NULLFSBLOCK);
 	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
 				   XFS_FSB_TO_DADDR(mp, map.br_startblock),

[PATCH 3/4] xfs: strengthen rtalloc query range checks

[Darrick J. Wong]

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

Strengthen the rtalloc range query checks to make sure that the keys do
not run off the end of the realtime device inappropriately.  Note that
the query range functions require units of rt extents, not blocks,
despite the type name.

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


diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 7712f282d172..1855182c11ec 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -1038,8 +1038,11 @@ xfs_rtalloc_query_range(
 
 	if (low_rec->ar_startblock > high_rec->ar_startblock)
 		return -EINVAL;
-	else if (low_rec->ar_startblock == high_rec->ar_startblock)
+	if (low_rec->ar_startblock >= mp->m_sb.sb_rextents ||
+	    low_rec->ar_startblock == high_rec->ar_startblock)
 		return 0;
+	if (high_rec->ar_startblock >= mp->m_sb.sb_rextents)
+		high_rec->ar_startblock = mp->m_sb.sb_rextents - 1;
 
 	/* Iterate the bitmap, looking for discrepancies. */
 	rtstart = low_rec->ar_startblock;
@@ -1083,7 +1086,7 @@ xfs_rtalloc_query_all(
 	struct xfs_rtalloc_rec		keys[2];
 
 	keys[0].ar_startblock = 0;
-	keys[1].ar_startblock = tp->t_mountp->m_sb.sb_rblocks;
+	keys[1].ar_startblock = tp->t_mountp->m_sb.sb_rextents - 1;
 	keys[0].ar_blockcount = keys[1].ar_blockcount = 0;
 
 	return xfs_rtalloc_query_range(tp, &keys[0], &keys[1], fn, priv);

[PATCH 4/4] xfs: fix xfs_rtalloc_rec units

[Darrick J. Wong]

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

All the realtime allocation functions deal with space on the rtdev in
units of realtime extents.  However, struct xfs_rtalloc_rec confusingly
uses the word 'block' in the name, even though they're really extents.

Fix the naming problem and fix all the unit handling problems in the two
existing users.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_rtbitmap.c |   26 +++++++++++++-------------
 fs/xfs/scrub/rtbitmap.c      |   12 ++++++++----
 fs/xfs/xfs_fsmap.c           |   13 +++++++------
 fs/xfs/xfs_rtalloc.h         |    9 +++++++--
 4 files changed, 35 insertions(+), 25 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 1855182c11ec..369eeb7a52ec 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -1036,17 +1036,17 @@ xfs_rtalloc_query_range(
 	int				is_free;
 	int				error = 0;
 
-	if (low_rec->ar_startblock > high_rec->ar_startblock)
+	if (low_rec->ar_startext > high_rec->ar_startext)
 		return -EINVAL;
-	if (low_rec->ar_startblock >= mp->m_sb.sb_rextents ||
-	    low_rec->ar_startblock == high_rec->ar_startblock)
+	if (low_rec->ar_startext >= mp->m_sb.sb_rextents ||
+	    low_rec->ar_startext == high_rec->ar_startext)
 		return 0;
-	if (high_rec->ar_startblock >= mp->m_sb.sb_rextents)
-		high_rec->ar_startblock = 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 - 1;
 
 	/* Iterate the bitmap, looking for discrepancies. */
-	rtstart = low_rec->ar_startblock;
-	rem = high_rec->ar_startblock - rtstart;
+	rtstart = low_rec->ar_startext;
+	rem = high_rec->ar_startext - rtstart;
 	while (rem) {
 		/* Is the first block free? */
 		error = xfs_rtcheck_range(mp, tp, rtstart, 1, 1, &rtend,
@@ -1056,13 +1056,13 @@ xfs_rtalloc_query_range(
 
 		/* How long does the extent go for? */
 		error = xfs_rtfind_forw(mp, tp, rtstart,
-				high_rec->ar_startblock - 1, &rtend);
+				high_rec->ar_startext - 1, &rtend);
 		if (error)
 			break;
 
 		if (is_free) {
-			rec.ar_startblock = rtstart;
-			rec.ar_blockcount = rtend - rtstart + 1;
+			rec.ar_startext = rtstart;
+			rec.ar_extcount = rtend - rtstart + 1;
 
 			error = fn(tp, &rec, priv);
 			if (error)
@@ -1085,9 +1085,9 @@ xfs_rtalloc_query_all(
 {
 	struct xfs_rtalloc_rec		keys[2];
 
-	keys[0].ar_startblock = 0;
-	keys[1].ar_startblock = tp->t_mountp->m_sb.sb_rextents - 1;
-	keys[0].ar_blockcount = keys[1].ar_blockcount = 0;
+	keys[0].ar_startext = 0;
+	keys[1].ar_startext = tp->t_mountp->m_sb.sb_rextents - 1;
+	keys[0].ar_extcount = keys[1].ar_extcount = 0;
 
 	return xfs_rtalloc_query_range(tp, &keys[0], &keys[1], fn, priv);
 }
diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
index 0fa3ef5c83b8..177d77878b0d 100644
--- a/fs/xfs/scrub/rtbitmap.c
+++ b/fs/xfs/scrub/rtbitmap.c
@@ -66,11 +66,15 @@ xfs_scrub_rtbitmap_rec(
 	void				*priv)
 {
 	struct xfs_scrub_context	*sc = priv;
+	xfs_rtblock_t			startblock;
+	xfs_rtblock_t			blockcount;
 
-	if (rec->ar_startblock + rec->ar_blockcount <= rec->ar_startblock ||
-	    !xfs_verify_rtbno(sc->mp, rec->ar_startblock) ||
-	    !xfs_verify_rtbno(sc->mp, rec->ar_startblock +
-			rec->ar_blockcount - 1))
+	startblock = rec->ar_startext * tp->t_mountp->m_sb.sb_rextsize;
+	blockcount = rec->ar_extcount * tp->t_mountp->m_sb.sb_rextsize;
+
+	if (startblock + blockcount <= startblock ||
+	    !xfs_verify_rtbno(sc->mp, startblock) ||
+	    !xfs_verify_rtbno(sc->mp, startblock + blockcount - 1))
 		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
 	return 0;
 }
diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 43cfc07996a4..92ce0e94085a 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -465,10 +465,9 @@ xfs_getfsmap_rtdev_rtbitmap_helper(
 	struct xfs_rmap_irec		irec;
 	xfs_daddr_t			rec_daddr;
 
-	rec_daddr = XFS_FSB_TO_BB(mp, rec->ar_startblock);
-
-	irec.rm_startblock = rec->ar_startblock;
-	irec.rm_blockcount = rec->ar_blockcount;
+	irec.rm_startblock = rec->ar_startext * mp->m_sb.sb_rextsize;
+	rec_daddr = XFS_FSB_TO_BB(mp, irec.rm_startblock);
+	irec.rm_blockcount = rec->ar_extcount * mp->m_sb.sb_rextsize;
 	irec.rm_owner = XFS_RMAP_OWN_NULL;	/* "free" */
 	irec.rm_offset = 0;
 	irec.rm_flags = 0;
@@ -534,8 +533,10 @@ xfs_getfsmap_rtdev_rtbitmap_query(
 
 	xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
 
-	alow.ar_startblock = info->low.rm_startblock;
-	ahigh.ar_startblock = info->high.rm_startblock;
+	alow.ar_startext = info->low.rm_startblock;
+	ahigh.ar_startext = info->high.rm_startblock;
+	do_div(alow.ar_startext, tp->t_mountp->m_sb.sb_rextsize);
+	do_div(ahigh.ar_startext, tp->t_mountp->m_sb.sb_rextsize);
 	error = xfs_rtalloc_query_range(tp, &alow, &ahigh,
 			xfs_getfsmap_rtdev_rtbitmap_helper, info);
 	if (error)
diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
index dfee3c991155..52632ab727f7 100644
--- a/fs/xfs/xfs_rtalloc.h
+++ b/fs/xfs/xfs_rtalloc.h
@@ -23,9 +23,14 @@
 struct xfs_mount;
 struct xfs_trans;
 
+/*
+ * XXX: Most of the realtime allocation functions deal in units of realtime
+ * extents, not realtime blocks.  This looks funny when paired with the type
+ * name and screams for a larger cleanup.
+ */
 struct xfs_rtalloc_rec {
-	xfs_rtblock_t		ar_startblock;
-	xfs_rtblock_t		ar_blockcount;
+	xfs_rtblock_t		ar_startext;
+	xfs_rtblock_t		ar_extcount;
 };
 
 typedef int (*xfs_rtalloc_query_range_fn)(

Re: [PATCH 1/4] xfs: xfs_rtword_t should be unsigned, not signed

[Allison Henderson]

On 05/31/2018 09:33 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> xfs_rtword_t is used for bit manipulations in the realtime bitmap file.
> Since we're performing bit shifts with this type, we don't want sign
> extension and we don't want to be left shifting negative quantities
> because that's undefined behavior.
> 
> This also shuts up these UBSAN warnings:
> UBSAN: Undefined behaviour in fs/xfs/libxfs/xfs_rtbitmap.c:833:48
> signed integer overflow:
> -2147483648 - 1 cannot be represented in type 'int'
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_types.h |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> index 3c560695c546..ea18449bd732 100644
> --- a/fs/xfs/libxfs/xfs_types.h
> +++ b/fs/xfs/libxfs/xfs_types.h
> @@ -30,7 +30,7 @@ typedef int64_t		xfs_fsize_t;	/* bytes in a file */
>   typedef uint64_t	xfs_ufsize_t;	/* unsigned bytes in a file */
>   
>   typedef int32_t		xfs_suminfo_t;	/* type of bitmap summary info */
> -typedef int32_t		xfs_rtword_t;	/* word type for bitmap manipulations */
> +typedef uint32_t	xfs_rtword_t;	/* word type for bitmap manipulations */

Looks good.  The spacing turned out a little offset though?  You can add 
my review.

Reviewed by: Allison Henderson <allison.henderson@oracle.com>
>   
>   typedef int64_t		xfs_lsn_t;	/* log sequence number */
>   typedef int32_t		xfs_tid_t;	/* transaction identifier */
> 
> --
> 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=n7ydpJVQ4YnI_crj8hHWumC8ltUYOP1uX00s9_PTdcA&s=4PrUlxl9KFk-MjkyiZuib9VkeSUbfIv2VV_E4jyPzOI&e=
> 

Re: [PATCH 2/4] xfs: xfs_rtbuf_get should check the bmapi_read results

[Allison Henderson]

On 05/31/2018 09:33 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The xfs_rtbuf_get function should check the block mapping it gets back
> from bmapi_read.  If there are no mappings or the mapping isn't a real
> extent, we should return -EFSCORRUPTED rather than trying to read a
> garbage value.  We also require realtime bitmap blocks to be real,
> written allocations.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_rtbitmap.c |    3 +++
>   1 file changed, 3 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 106be2d0bb88..7712f282d172 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -90,6 +90,9 @@ xfs_rtbuf_get(
>   	if (error)
>   		return error;
>   
> +	if (nmap == 0 || !xfs_bmap_is_real_extent(&map))
> +		return -EFSCORRUPTED;
> +
>   	ASSERT(map.br_startblock != NULLFSBLOCK);
>   	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
>   				   XFS_FSB_TO_DADDR(mp, map.br_startblock),
> 
Looks ok:
Reviewed by: Allison Henderson <allison.henderson@oracle.com>
> --
> 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=Nfk9xyeEu8tolNh3C6XthiC4BSuJcRRP0QGYJ_iBdDc&s=0bnHDf3AvXAlQ_rj_227qYXSraL5XP-OFUC8Mw3yI0E&e=
> 

Re: [PATCH 3/4] xfs: strengthen rtalloc query range checks

[Allison Henderson]

On 05/31/2018 09:34 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Strengthen the rtalloc range query checks to make sure that the keys do
> not run off the end of the realtime device inappropriately.  Note that
> the query range functions require units of rt extents, not blocks,
> despite the type name.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_rtbitmap.c |    7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 7712f282d172..1855182c11ec 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1038,8 +1038,11 @@ xfs_rtalloc_query_range(
>   
>   	if (low_rec->ar_startblock > high_rec->ar_startblock)
>   		return -EINVAL;
> -	else if (low_rec->ar_startblock == high_rec->ar_startblock)
> +	if (low_rec->ar_startblock >= mp->m_sb.sb_rextents ||
> +	    low_rec->ar_startblock == high_rec->ar_startblock)
>   		return 0;
> +	if (high_rec->ar_startblock >= mp->m_sb.sb_rextents)
> +		high_rec->ar_startblock = mp->m_sb.sb_rextents - 1;
>   
>   	/* Iterate the bitmap, looking for discrepancies. */
>   	rtstart = low_rec->ar_startblock;
> @@ -1083,7 +1086,7 @@ xfs_rtalloc_query_all(
>   	struct xfs_rtalloc_rec		keys[2];
>   
>   	keys[0].ar_startblock = 0;
> -	keys[1].ar_startblock = tp->t_mountp->m_sb.sb_rblocks;
> +	keys[1].ar_startblock = tp->t_mountp->m_sb.sb_rextents - 1;
>   	keys[0].ar_blockcount = keys[1].ar_blockcount = 0;
>   
>   	return xfs_rtalloc_query_range(tp, &keys[0], &keys[1], fn, priv);
> 
Looks ok.
Reviewed by: Allison Henderson <allison.henderson@oracle.com>
> --
> 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=MFVzrJTKXLIlUFVLz6r3OL2vnmcjObSpReWRk7GYGuo&s=hxgtdqlBCiIpubwA7m9BUItlt1b_6qY1TueIDaRqA5o&e=
> 

Re: [PATCH 4/4] xfs: fix xfs_rtalloc_rec units

[Allison Henderson]

On 05/31/2018 09:34 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> All the realtime allocation functions deal with space on the rtdev in
> units of realtime extents.  However, struct xfs_rtalloc_rec confusingly
> uses the word 'block' in the name, even though they're really extents.
> 
> Fix the naming problem and fix all the unit handling problems in the two
> existing users.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   fs/xfs/libxfs/xfs_rtbitmap.c |   26 +++++++++++++-------------
>   fs/xfs/scrub/rtbitmap.c      |   12 ++++++++----
>   fs/xfs/xfs_fsmap.c           |   13 +++++++------
>   fs/xfs/xfs_rtalloc.h         |    9 +++++++--
>   4 files changed, 35 insertions(+), 25 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 1855182c11ec..369eeb7a52ec 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1036,17 +1036,17 @@ xfs_rtalloc_query_range(
>   	int				is_free;
>   	int				error = 0;
>   
> -	if (low_rec->ar_startblock > high_rec->ar_startblock)
> +	if (low_rec->ar_startext > high_rec->ar_startext)
>   		return -EINVAL;
> -	if (low_rec->ar_startblock >= mp->m_sb.sb_rextents ||
> -	    low_rec->ar_startblock == high_rec->ar_startblock)
> +	if (low_rec->ar_startext >= mp->m_sb.sb_rextents ||
> +	    low_rec->ar_startext == high_rec->ar_startext)
>   		return 0;
> -	if (high_rec->ar_startblock >= mp->m_sb.sb_rextents)
> -		high_rec->ar_startblock = 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 - 1;
>   
>   	/* Iterate the bitmap, looking for discrepancies. */
> -	rtstart = low_rec->ar_startblock;
> -	rem = high_rec->ar_startblock - rtstart;
> +	rtstart = low_rec->ar_startext;
> +	rem = high_rec->ar_startext - rtstart;
>   	while (rem) {
>   		/* Is the first block free? */
>   		error = xfs_rtcheck_range(mp, tp, rtstart, 1, 1, &rtend,
> @@ -1056,13 +1056,13 @@ xfs_rtalloc_query_range(
>   
>   		/* How long does the extent go for? */
>   		error = xfs_rtfind_forw(mp, tp, rtstart,
> -				high_rec->ar_startblock - 1, &rtend);
> +				high_rec->ar_startext - 1, &rtend);
>   		if (error)
>   			break;
>   
>   		if (is_free) {
> -			rec.ar_startblock = rtstart;
> -			rec.ar_blockcount = rtend - rtstart + 1;
> +			rec.ar_startext = rtstart;
> +			rec.ar_extcount = rtend - rtstart + 1;
>   
>   			error = fn(tp, &rec, priv);
>   			if (error)
> @@ -1085,9 +1085,9 @@ xfs_rtalloc_query_all(
>   {
>   	struct xfs_rtalloc_rec		keys[2];
>   
> -	keys[0].ar_startblock = 0;
> -	keys[1].ar_startblock = tp->t_mountp->m_sb.sb_rextents - 1;
> -	keys[0].ar_blockcount = keys[1].ar_blockcount = 0;
> +	keys[0].ar_startext = 0;
> +	keys[1].ar_startext = tp->t_mountp->m_sb.sb_rextents - 1;
> +	keys[0].ar_extcount = keys[1].ar_extcount = 0;
>   
>   	return xfs_rtalloc_query_range(tp, &keys[0], &keys[1], fn, priv);
>   }
> diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> index 0fa3ef5c83b8..177d77878b0d 100644
> --- a/fs/xfs/scrub/rtbitmap.c
> +++ b/fs/xfs/scrub/rtbitmap.c
> @@ -66,11 +66,15 @@ xfs_scrub_rtbitmap_rec(
>   	void				*priv)
>   {
>   	struct xfs_scrub_context	*sc = priv;
> +	xfs_rtblock_t			startblock;
> +	xfs_rtblock_t			blockcount;
>   
> -	if (rec->ar_startblock + rec->ar_blockcount <= rec->ar_startblock ||
> -	    !xfs_verify_rtbno(sc->mp, rec->ar_startblock) ||
> -	    !xfs_verify_rtbno(sc->mp, rec->ar_startblock +
> -			rec->ar_blockcount - 1))
> +	startblock = rec->ar_startext * tp->t_mountp->m_sb.sb_rextsize;
> +	blockcount = rec->ar_extcount * tp->t_mountp->m_sb.sb_rextsize;
> +
> +	if (startblock + blockcount <= startblock ||
> +	    !xfs_verify_rtbno(sc->mp, startblock) ||
> +	    !xfs_verify_rtbno(sc->mp, startblock + blockcount - 1))
>   		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
>   	return 0;
>   }
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 43cfc07996a4..92ce0e94085a 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -465,10 +465,9 @@ xfs_getfsmap_rtdev_rtbitmap_helper(
>   	struct xfs_rmap_irec		irec;
>   	xfs_daddr_t			rec_daddr;
>   
> -	rec_daddr = XFS_FSB_TO_BB(mp, rec->ar_startblock);
> -
> -	irec.rm_startblock = rec->ar_startblock;
> -	irec.rm_blockcount = rec->ar_blockcount;
> +	irec.rm_startblock = rec->ar_startext * mp->m_sb.sb_rextsize;
> +	rec_daddr = XFS_FSB_TO_BB(mp, irec.rm_startblock);
> +	irec.rm_blockcount = rec->ar_extcount * mp->m_sb.sb_rextsize;
>   	irec.rm_owner = XFS_RMAP_OWN_NULL;	/* "free" */
>   	irec.rm_offset = 0;
>   	irec.rm_flags = 0;
> @@ -534,8 +533,10 @@ xfs_getfsmap_rtdev_rtbitmap_query(
>   
>   	xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
>   
> -	alow.ar_startblock = info->low.rm_startblock;
> -	ahigh.ar_startblock = info->high.rm_startblock;
> +	alow.ar_startext = info->low.rm_startblock;
> +	ahigh.ar_startext = info->high.rm_startblock;
> +	do_div(alow.ar_startext, tp->t_mountp->m_sb.sb_rextsize);
> +	do_div(ahigh.ar_startext, tp->t_mountp->m_sb.sb_rextsize);
>   	error = xfs_rtalloc_query_range(tp, &alow, &ahigh,
>   			xfs_getfsmap_rtdev_rtbitmap_helper, info);
>   	if (error)
> diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
> index dfee3c991155..52632ab727f7 100644
> --- a/fs/xfs/xfs_rtalloc.h
> +++ b/fs/xfs/xfs_rtalloc.h
> @@ -23,9 +23,14 @@
>   struct xfs_mount;
>   struct xfs_trans;
>   
> +/*
> + * XXX: Most of the realtime allocation functions deal in units of realtime
> + * extents, not realtime blocks.  This looks funny when paired with the type
> + * name and screams for a larger cleanup.
> + */
>   struct xfs_rtalloc_rec {
> -	xfs_rtblock_t		ar_startblock;
> -	xfs_rtblock_t		ar_blockcount;
> +	xfs_rtblock_t		ar_startext;
> +	xfs_rtblock_t		ar_extcount;
>   };
>   
>   typedef int (*xfs_rtalloc_query_range_fn)(
> 
Ok, I think that looks better than it was.  Thx!
Reviewed by: Allison Henderson <allison.henderson@oracle.com>
> --
> 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=qCtkjUk7DtakiO_0mbtpZoMENgwkzaIEiA4aAX5QCmM&s=YXBJILtyvqDQuErf-gZPvJ461gczA0x4C47nvpL4Su0&e=
> 

Re: [PATCH 1/4] xfs: xfs_rtword_t should be unsigned, not signed

[Bill O'Donnell]

On Thu, May 31, 2018 at 09:33:50AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> xfs_rtword_t is used for bit manipulations in the realtime bitmap file.
> Since we're performing bit shifts with this type, we don't want sign
> extension and we don't want to be left shifting negative quantities
> because that's undefined behavior.
> 
> This also shuts up these UBSAN warnings:
> UBSAN: Undefined behaviour in fs/xfs/libxfs/xfs_rtbitmap.c:833:48
> signed integer overflow:
> -2147483648 - 1 cannot be represented in type 'int'
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

looks fine.
Reviewed-by: Bill O'Donnell <billodo@redhat.com>


> ---
>  fs/xfs/libxfs/xfs_types.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> index 3c560695c546..ea18449bd732 100644
> --- a/fs/xfs/libxfs/xfs_types.h
> +++ b/fs/xfs/libxfs/xfs_types.h
> @@ -30,7 +30,7 @@ typedef int64_t		xfs_fsize_t;	/* bytes in a file */
>  typedef uint64_t	xfs_ufsize_t;	/* unsigned bytes in a file */
>  
>  typedef int32_t		xfs_suminfo_t;	/* type of bitmap summary info */
> -typedef int32_t		xfs_rtword_t;	/* word type for bitmap manipulations */
> +typedef uint32_t	xfs_rtword_t;	/* word type for bitmap manipulations */
>  
>  typedef int64_t		xfs_lsn_t;	/* log sequence number */
>  typedef int32_t		xfs_tid_t;	/* transaction identifier */
> 
> --
> 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 2/4] xfs: xfs_rtbuf_get should check the bmapi_read results

[Bill O'Donnell]

On Thu, May 31, 2018 at 09:33:57AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The xfs_rtbuf_get function should check the block mapping it gets back
> from bmapi_read.  If there are no mappings or the mapping isn't a real
> extent, we should return -EFSCORRUPTED rather than trying to read a
> garbage value.  We also require realtime bitmap blocks to be real,
> written allocations.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
looks fine...
Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 106be2d0bb88..7712f282d172 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -90,6 +90,9 @@ xfs_rtbuf_get(
>  	if (error)
>  		return error;
>  
> +	if (nmap == 0 || !xfs_bmap_is_real_extent(&map))
> +		return -EFSCORRUPTED;
> +
>  	ASSERT(map.br_startblock != NULLFSBLOCK);
>  	error = xfs_trans_read_buf(mp, tp, mp->m_ddev_targp,
>  				   XFS_FSB_TO_DADDR(mp, map.br_startblock),
> 
> --
> 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 3/4] xfs: strengthen rtalloc query range checks

[Bill O'Donnell]

On Thu, May 31, 2018 at 09:34:03AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Strengthen the rtalloc range query checks to make sure that the keys do
> not run off the end of the realtime device inappropriately.  Note that
> the query range functions require units of rt extents, not blocks,
> despite the type name.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

looks fine.
Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 7712f282d172..1855182c11ec 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1038,8 +1038,11 @@ xfs_rtalloc_query_range(
>  
>  	if (low_rec->ar_startblock > high_rec->ar_startblock)
>  		return -EINVAL;
> -	else if (low_rec->ar_startblock == high_rec->ar_startblock)
> +	if (low_rec->ar_startblock >= mp->m_sb.sb_rextents ||
> +	    low_rec->ar_startblock == high_rec->ar_startblock)
>  		return 0;
> +	if (high_rec->ar_startblock >= mp->m_sb.sb_rextents)
> +		high_rec->ar_startblock = mp->m_sb.sb_rextents - 1;
>  
>  	/* Iterate the bitmap, looking for discrepancies. */
>  	rtstart = low_rec->ar_startblock;
> @@ -1083,7 +1086,7 @@ xfs_rtalloc_query_all(
>  	struct xfs_rtalloc_rec		keys[2];
>  
>  	keys[0].ar_startblock = 0;
> -	keys[1].ar_startblock = tp->t_mountp->m_sb.sb_rblocks;
> +	keys[1].ar_startblock = tp->t_mountp->m_sb.sb_rextents - 1;
>  	keys[0].ar_blockcount = keys[1].ar_blockcount = 0;
>  
>  	return xfs_rtalloc_query_range(tp, &keys[0], &keys[1], fn, priv);
> 
> --
> 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 4/4] xfs: fix xfs_rtalloc_rec units

[Bill O'Donnell]

On Thu, May 31, 2018 at 09:34:09AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> All the realtime allocation functions deal with space on the rtdev in
> units of realtime extents.  However, struct xfs_rtalloc_rec confusingly
> uses the word 'block' in the name, even though they're really extents.
> 
> Fix the naming problem and fix all the unit handling problems in the two
> existing users.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

looks good.
Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c |   26 +++++++++++++-------------
>  fs/xfs/scrub/rtbitmap.c      |   12 ++++++++----
>  fs/xfs/xfs_fsmap.c           |   13 +++++++------
>  fs/xfs/xfs_rtalloc.h         |    9 +++++++--
>  4 files changed, 35 insertions(+), 25 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 1855182c11ec..369eeb7a52ec 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1036,17 +1036,17 @@ xfs_rtalloc_query_range(
>  	int				is_free;
>  	int				error = 0;
>  
> -	if (low_rec->ar_startblock > high_rec->ar_startblock)
> +	if (low_rec->ar_startext > high_rec->ar_startext)
>  		return -EINVAL;
> -	if (low_rec->ar_startblock >= mp->m_sb.sb_rextents ||
> -	    low_rec->ar_startblock == high_rec->ar_startblock)
> +	if (low_rec->ar_startext >= mp->m_sb.sb_rextents ||
> +	    low_rec->ar_startext == high_rec->ar_startext)
>  		return 0;
> -	if (high_rec->ar_startblock >= mp->m_sb.sb_rextents)
> -		high_rec->ar_startblock = 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 - 1;
>  
>  	/* Iterate the bitmap, looking for discrepancies. */
> -	rtstart = low_rec->ar_startblock;
> -	rem = high_rec->ar_startblock - rtstart;
> +	rtstart = low_rec->ar_startext;
> +	rem = high_rec->ar_startext - rtstart;
>  	while (rem) {
>  		/* Is the first block free? */
>  		error = xfs_rtcheck_range(mp, tp, rtstart, 1, 1, &rtend,
> @@ -1056,13 +1056,13 @@ xfs_rtalloc_query_range(
>  
>  		/* How long does the extent go for? */
>  		error = xfs_rtfind_forw(mp, tp, rtstart,
> -				high_rec->ar_startblock - 1, &rtend);
> +				high_rec->ar_startext - 1, &rtend);
>  		if (error)
>  			break;
>  
>  		if (is_free) {
> -			rec.ar_startblock = rtstart;
> -			rec.ar_blockcount = rtend - rtstart + 1;
> +			rec.ar_startext = rtstart;
> +			rec.ar_extcount = rtend - rtstart + 1;
>  
>  			error = fn(tp, &rec, priv);
>  			if (error)
> @@ -1085,9 +1085,9 @@ xfs_rtalloc_query_all(
>  {
>  	struct xfs_rtalloc_rec		keys[2];
>  
> -	keys[0].ar_startblock = 0;
> -	keys[1].ar_startblock = tp->t_mountp->m_sb.sb_rextents - 1;
> -	keys[0].ar_blockcount = keys[1].ar_blockcount = 0;
> +	keys[0].ar_startext = 0;
> +	keys[1].ar_startext = tp->t_mountp->m_sb.sb_rextents - 1;
> +	keys[0].ar_extcount = keys[1].ar_extcount = 0;
>  
>  	return xfs_rtalloc_query_range(tp, &keys[0], &keys[1], fn, priv);
>  }
> diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> index 0fa3ef5c83b8..177d77878b0d 100644
> --- a/fs/xfs/scrub/rtbitmap.c
> +++ b/fs/xfs/scrub/rtbitmap.c
> @@ -66,11 +66,15 @@ xfs_scrub_rtbitmap_rec(
>  	void				*priv)
>  {
>  	struct xfs_scrub_context	*sc = priv;
> +	xfs_rtblock_t			startblock;
> +	xfs_rtblock_t			blockcount;
>  
> -	if (rec->ar_startblock + rec->ar_blockcount <= rec->ar_startblock ||
> -	    !xfs_verify_rtbno(sc->mp, rec->ar_startblock) ||
> -	    !xfs_verify_rtbno(sc->mp, rec->ar_startblock +
> -			rec->ar_blockcount - 1))
> +	startblock = rec->ar_startext * tp->t_mountp->m_sb.sb_rextsize;
> +	blockcount = rec->ar_extcount * tp->t_mountp->m_sb.sb_rextsize;
> +
> +	if (startblock + blockcount <= startblock ||
> +	    !xfs_verify_rtbno(sc->mp, startblock) ||
> +	    !xfs_verify_rtbno(sc->mp, startblock + blockcount - 1))
>  		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
>  	return 0;
>  }
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 43cfc07996a4..92ce0e94085a 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -465,10 +465,9 @@ xfs_getfsmap_rtdev_rtbitmap_helper(
>  	struct xfs_rmap_irec		irec;
>  	xfs_daddr_t			rec_daddr;
>  
> -	rec_daddr = XFS_FSB_TO_BB(mp, rec->ar_startblock);
> -
> -	irec.rm_startblock = rec->ar_startblock;
> -	irec.rm_blockcount = rec->ar_blockcount;
> +	irec.rm_startblock = rec->ar_startext * mp->m_sb.sb_rextsize;
> +	rec_daddr = XFS_FSB_TO_BB(mp, irec.rm_startblock);
> +	irec.rm_blockcount = rec->ar_extcount * mp->m_sb.sb_rextsize;
>  	irec.rm_owner = XFS_RMAP_OWN_NULL;	/* "free" */
>  	irec.rm_offset = 0;
>  	irec.rm_flags = 0;
> @@ -534,8 +533,10 @@ xfs_getfsmap_rtdev_rtbitmap_query(
>  
>  	xfs_ilock(tp->t_mountp->m_rbmip, XFS_ILOCK_SHARED);
>  
> -	alow.ar_startblock = info->low.rm_startblock;
> -	ahigh.ar_startblock = info->high.rm_startblock;
> +	alow.ar_startext = info->low.rm_startblock;
> +	ahigh.ar_startext = info->high.rm_startblock;
> +	do_div(alow.ar_startext, tp->t_mountp->m_sb.sb_rextsize);
> +	do_div(ahigh.ar_startext, tp->t_mountp->m_sb.sb_rextsize);
>  	error = xfs_rtalloc_query_range(tp, &alow, &ahigh,
>  			xfs_getfsmap_rtdev_rtbitmap_helper, info);
>  	if (error)
> diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
> index dfee3c991155..52632ab727f7 100644
> --- a/fs/xfs/xfs_rtalloc.h
> +++ b/fs/xfs/xfs_rtalloc.h
> @@ -23,9 +23,14 @@
>  struct xfs_mount;
>  struct xfs_trans;
>  
> +/*
> + * XXX: Most of the realtime allocation functions deal in units of realtime
> + * extents, not realtime blocks.  This looks funny when paired with the type
> + * name and screams for a larger cleanup.
> + */
>  struct xfs_rtalloc_rec {
> -	xfs_rtblock_t		ar_startblock;
> -	xfs_rtblock_t		ar_blockcount;
> +	xfs_rtblock_t		ar_startext;
> +	xfs_rtblock_t		ar_extcount;
>  };
>  
>  typedef int (*xfs_rtalloc_query_range_fn)(
> 
> --
> 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