public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCHSET 0/3] xfs: fix realtime geometry integer overflows
@ 2023-12-03 19:00 Darrick J. Wong
  2023-12-03 19:05 ` [PATCH 1/3] xfs: make rextslog computation consistent with mkfs Darrick J. Wong
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:00 UTC (permalink / raw)
  To: djwong, chandanbabu, hch; +Cc: linux-xfs

Hi all,

While reading through the realtime geometry support code in xfsprogs, I
noticed a discrepancy between the sb_rextslog computation used when
writing out the superblock during mkfs and the validation code used in
xfs_repair.  This discrepancy would lead to system failure for a runt rt
volume having more than 1 rt block but zero rt extents in length.  Most
people aren't going to configure a 1M extent size for their 360k rt
floppy disk volume, but I did!

In the process of studying that code, it occurred to me that there is a
second bug in the computation -- the use of highbit32 for a 64-bit
value means that the upper 32 bits are not considered in the search for
a high bit.  This causes the creation of a realtime summary file that is
the wrong length.  If rextents is a multiple of U32_MAX then this will
appear to work fine because highbit32 returns -1 for an input of 0; but
for all other cases the rt summary is undersized, leading to failures.

Fix the first problem by standardizing the computation with a helper in
libxfs; and the second problem by correcting the computation.  This will
cause any existing rt volumes larger than 2^32 blocks to fail validation
but they probably were already crashing anyway.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=fix-rtmount-overflows-6.7
---
 fs/xfs/libxfs/xfs_rtbitmap.c |   12 ++++++++++++
 fs/xfs/libxfs/xfs_rtbitmap.h |   14 ++++++++++++++
 fs/xfs/libxfs/xfs_sb.c       |    6 ++++--
 fs/xfs/xfs_rtalloc.c         |    6 ++++--
 4 files changed, 34 insertions(+), 4 deletions(-)


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] xfs: make rextslog computation consistent with mkfs
  2023-12-03 19:00 [PATCHSET 0/3] xfs: fix realtime geometry integer overflows Darrick J. Wong
@ 2023-12-03 19:05 ` Darrick J. Wong
  2023-12-04  4:55   ` Christoph Hellwig
  2023-12-03 19:05 ` [PATCH 2/3] xfs: fix 32-bit truncation in xfs_compute_rextslog Darrick J. Wong
  2023-12-03 19:05 ` [PATCH 3/3] xfs: don't allow overly small or large realtime volumes Darrick J. Wong
  2 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:05 UTC (permalink / raw)
  To: djwong, chandanbabu, hch; +Cc: linux-xfs

From: Darrick J. Wong <djwong@kernel.org>

There's a weird discrepancy in xfsprogs dating back to the creation of
the Linux port -- if there are zero rt extents, mkfs will set
sb_rextents and sb_rextslog both to zero:

	sbp->sb_rextslog =
		(uint8_t)(rtextents ?
			libxfs_highbit32((unsigned int)rtextents) : 0);

However, that's not the check that xfs_repair uses for nonzero rtblocks:

	if (sb->sb_rextslog !=
			libxfs_highbit32((unsigned int)sb->sb_rextents))

The difference here is that xfs_highbit32 returns -1 if its argument is
zero.  Unfortunately, this means that in the weird corner case of a
realtime volume shorter than 1 rt extent, xfs_repair will immediately
flag a freshly formatted filesystem as corrupt.  Because mkfs has been
writing ondisk artifacts like this for decades, we have to accept that
as "correct".  TBH, zero rextslog for zero rtextents makes more sense to
me anyway.

Regrettably, the superblock verifier checks created in commit copied
xfs_repair even though mkfs has been writing out such filesystems for
ages.  Fix the superblock verifier to accept what mkfs spits out; the
userspace version of this patch will have to fix xfs_repair as well.

Note that the new helper leaves the zeroday bug where the upper 32 bits
of sb_rextents is ripped off and fed to highbit32.  This leads to a
seriously undersized rt summary file, which immediately breaks mkfs:

$ hugedisk.sh foo /dev/sdc $(( 0x100000080 * 4096))B
$ /sbin/mkfs.xfs -f /dev/sda -m rmapbt=0,reflink=0 -r rtdev=/dev/mapper/foo
meta-data=/dev/sda               isize=512    agcount=4, agsize=1298176 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=0    bigtime=1 inobtcount=1 nrext64=1
data     =                       bsize=4096   blocks=5192704, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =/dev/mapper/foo        extsz=4096   blocks=4294967424, rtextents=4294967424
Discarding blocks...Done.
mkfs.xfs: Error initializing the realtime space [117 - Structure needs cleaning]

Fixes: f8e566c0f5e1f ("xfs: validate the realtime geometry in xfs_validate_sb_common")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_rtbitmap.c |   12 ++++++++++++
 fs/xfs/libxfs/xfs_rtbitmap.h |    2 ++
 fs/xfs/libxfs/xfs_sb.c       |    3 ++-
 fs/xfs/xfs_rtalloc.c         |    4 ++--
 4 files changed, 18 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index c269d704314d..1c9fed76a356 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -1130,6 +1130,18 @@ xfs_rtbitmap_blockcount(
 	return howmany_64(rtextents, NBBY * mp->m_sb.sb_blocksize);
 }
 
+/*
+ * Compute the maximum level number of the realtime summary file, as defined by
+ * mkfs.  The use of highbit32 on a 64-bit quantity is a historic artifact that
+ * prohibits correct use of rt volumes with more than 2^32 extents.
+ */
+uint8_t
+xfs_compute_rextslog(
+	xfs_rtbxlen_t		rtextents)
+{
+	return rtextents ? xfs_highbit32(rtextents) : 0;
+}
+
 /*
  * Compute the number of rtbitmap words needed to populate every block of a
  * bitmap that is large enough to track the given number of rt extents.
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
index c0637057d69c..1610d0e4a04c 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.h
+++ b/fs/xfs/libxfs/xfs_rtbitmap.h
@@ -351,6 +351,8 @@ xfs_rtfree_extent(
 int xfs_rtfree_blocks(struct xfs_trans *tp, xfs_fsblock_t rtbno,
 		xfs_filblks_t rtlen);
 
+uint8_t xfs_compute_rextslog(xfs_rtbxlen_t rtextents);
+
 xfs_filblks_t xfs_rtbitmap_blockcount(struct xfs_mount *mp, xfs_rtbxlen_t
 		rtextents);
 unsigned long long xfs_rtbitmap_wordcount(struct xfs_mount *mp,
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 1f74d0cd1618..df12bf82ed18 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -25,6 +25,7 @@
 #include "xfs_da_format.h"
 #include "xfs_health.h"
 #include "xfs_ag.h"
+#include "xfs_rtbitmap.h"
 
 /*
  * Physical superblock buffer manipulations. Shared with libxfs in userspace.
@@ -509,7 +510,7 @@ xfs_validate_sb_common(
 				       NBBY * sbp->sb_blocksize);
 
 		if (sbp->sb_rextents != rexts ||
-		    sbp->sb_rextslog != xfs_highbit32(sbp->sb_rextents) ||
+		    sbp->sb_rextslog != xfs_compute_rextslog(rexts) ||
 		    sbp->sb_rbmblocks != rbmblocks) {
 			xfs_notice(mp,
 				"realtime geometry sanity check failed");
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 88c48de5c9c8..7c5a50163d2d 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -964,7 +964,7 @@ xfs_growfs_rt(
 	nrextents = nrblocks;
 	do_div(nrextents, in->extsize);
 	nrbmblocks = xfs_rtbitmap_blockcount(mp, nrextents);
-	nrextslog = xfs_highbit32(nrextents);
+	nrextslog = xfs_compute_rextslog(nrextents);
 	nrsumlevels = nrextslog + 1;
 	nrsumblocks = xfs_rtsummary_blockcount(mp, nrsumlevels, nrbmblocks);
 	nrsumsize = XFS_FSB_TO_B(mp, nrsumblocks);
@@ -1031,7 +1031,7 @@ xfs_growfs_rt(
 		nsbp->sb_rblocks = min(nrblocks, nrblocks_step);
 		nsbp->sb_rextents = xfs_rtb_to_rtx(nmp, nsbp->sb_rblocks);
 		ASSERT(nsbp->sb_rextents != 0);
-		nsbp->sb_rextslog = xfs_highbit32(nsbp->sb_rextents);
+		nsbp->sb_rextslog = xfs_compute_rextslog(nsbp->sb_rextents);
 		nrsumlevels = nmp->m_rsumlevels = nsbp->sb_rextslog + 1;
 		nrsumblocks = xfs_rtsummary_blockcount(mp, nrsumlevels,
 				nsbp->sb_rbmblocks);


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/3] xfs: fix 32-bit truncation in xfs_compute_rextslog
  2023-12-03 19:00 [PATCHSET 0/3] xfs: fix realtime geometry integer overflows Darrick J. Wong
  2023-12-03 19:05 ` [PATCH 1/3] xfs: make rextslog computation consistent with mkfs Darrick J. Wong
@ 2023-12-03 19:05 ` Darrick J. Wong
  2023-12-04  4:55   ` Christoph Hellwig
  2023-12-03 19:05 ` [PATCH 3/3] xfs: don't allow overly small or large realtime volumes Darrick J. Wong
  2 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:05 UTC (permalink / raw)
  To: djwong, chandanbabu, hch; +Cc: linux-xfs

From: Darrick J. Wong <djwong@kernel.org>

It's quite reasonable that some customer somewhere will want to
configure a realtime volume with more than 2^32 extents.  If they try to
do this, the highbit32() call will truncate the upper bits of the
xfs_rtbxlen_t and produce the wrong value for rextslog.  This in turn
causes the rsumlevels to be wrong, which results in a realtime summary
file that is the wrong length.  Fix that.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_rtbitmap.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index 1c9fed76a356..0626909a2481 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -1132,14 +1132,14 @@ xfs_rtbitmap_blockcount(
 
 /*
  * Compute the maximum level number of the realtime summary file, as defined by
- * mkfs.  The use of highbit32 on a 64-bit quantity is a historic artifact that
- * prohibits correct use of rt volumes with more than 2^32 extents.
+ * mkfs.  The historic use of highbit32 on a 64-bit quantity prohibited correct
+ * use of rt volumes with more than 2^32 extents.
  */
 uint8_t
 xfs_compute_rextslog(
 	xfs_rtbxlen_t		rtextents)
 {
-	return rtextents ? xfs_highbit32(rtextents) : 0;
+	return rtextents ? xfs_highbit64(rtextents) : 0;
 }
 
 /*


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/3] xfs: don't allow overly small or large realtime volumes
  2023-12-03 19:00 [PATCHSET 0/3] xfs: fix realtime geometry integer overflows Darrick J. Wong
  2023-12-03 19:05 ` [PATCH 1/3] xfs: make rextslog computation consistent with mkfs Darrick J. Wong
  2023-12-03 19:05 ` [PATCH 2/3] xfs: fix 32-bit truncation in xfs_compute_rextslog Darrick J. Wong
@ 2023-12-03 19:05 ` Darrick J. Wong
  2023-12-03 19:09   ` Darrick J. Wong
  2023-12-04  4:56   ` Christoph Hellwig
  2 siblings, 2 replies; 10+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:05 UTC (permalink / raw)
  To: djwong, chandanbabu, hch; +Cc: linux-xfs

From: Darrick J. Wong <djwong@kernel.org>

Don't allow realtime volumes that are less than one rt extent long.
This has been broken across 4 LTS kernels with nobody noticing, so let's
just disable it.

Per the previous patch, I also observed integer overflows in calculating
rextslog (the number of rt summary levels) when the rtextent count
exceeds 2^32.  If you're lucky, this means that mkfs will fail to format
the filesystem; if not, then the fs will go down due to corruption
errors.  Prohibit those too; larger volume support will return with
rtgroups.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_rtbitmap.h |   12 ++++++++++++
 fs/xfs/libxfs/xfs_sb.c       |    3 ++-
 fs/xfs/xfs_rtalloc.c         |    2 ++
 3 files changed, 16 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
index 1610d0e4a04c..411de3b889ae 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.h
+++ b/fs/xfs/libxfs/xfs_rtbitmap.h
@@ -353,6 +353,18 @@ int xfs_rtfree_blocks(struct xfs_trans *tp, xfs_fsblock_t rtbno,
 
 uint8_t xfs_compute_rextslog(xfs_rtbxlen_t rtextents);
 
+/* Do we support an rt volume having this number of rtextents? */
+static inline bool
+xfs_validate_rtextents(
+	xfs_rtbxlen_t		rtextents)
+{
+	/* No runt rt volumes */
+	if (rtextents == 0)
+		return false;
+
+	return true;
+}
+
 xfs_filblks_t xfs_rtbitmap_blockcount(struct xfs_mount *mp, xfs_rtbxlen_t
 		rtextents);
 unsigned long long xfs_rtbitmap_wordcount(struct xfs_mount *mp,
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index df12bf82ed18..4a9e8588f4c9 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -509,7 +509,8 @@ xfs_validate_sb_common(
 		rbmblocks = howmany_64(sbp->sb_rextents,
 				       NBBY * sbp->sb_blocksize);
 
-		if (sbp->sb_rextents != rexts ||
+		if (!xfs_validate_rtextents(rexts) ||
+		    sbp->sb_rextents != rexts ||
 		    sbp->sb_rextslog != xfs_compute_rextslog(rexts) ||
 		    sbp->sb_rbmblocks != rbmblocks) {
 			xfs_notice(mp,
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 7c5a50163d2d..8feb58c6241c 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -963,6 +963,8 @@ xfs_growfs_rt(
 	 */
 	nrextents = nrblocks;
 	do_div(nrextents, in->extsize);
+	if (!xfs_validate_rtextents(nrextents))
+		return -EINVAL;
 	nrbmblocks = xfs_rtbitmap_blockcount(mp, nrextents);
 	nrextslog = xfs_compute_rextslog(nrextents);
 	nrsumlevels = nrextslog + 1;


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] xfs: don't allow overly small or large realtime volumes
  2023-12-03 19:05 ` [PATCH 3/3] xfs: don't allow overly small or large realtime volumes Darrick J. Wong
@ 2023-12-03 19:09   ` Darrick J. Wong
  2023-12-04  4:56   ` Christoph Hellwig
  1 sibling, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2023-12-03 19:09 UTC (permalink / raw)
  To: chandanbabu, hch; +Cc: linux-xfs

On Sun, Dec 03, 2023 at 11:05:50AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Don't allow realtime volumes that are less than one rt extent long.
> This has been broken across 4 LTS kernels with nobody noticing, so let's
> just disable it.
> 
> Per the previous patch, I also observed integer overflows in calculating
> rextslog (the number of rt summary levels) when the rtextent count
> exceeds 2^32.  If you're lucky, this means that mkfs will fail to format
> the filesystem; if not, then the fs will go down due to corruption
> errors.  Prohibit those too; larger volume support will return with
> rtgroups.

....aaand I forgot to update the commit message on this one, sorry.
Strike the previous paragraph, please.

--D

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_rtbitmap.h |   12 ++++++++++++
>  fs/xfs/libxfs/xfs_sb.c       |    3 ++-
>  fs/xfs/xfs_rtalloc.c         |    2 ++
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
> index 1610d0e4a04c..411de3b889ae 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.h
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.h
> @@ -353,6 +353,18 @@ int xfs_rtfree_blocks(struct xfs_trans *tp, xfs_fsblock_t rtbno,
>  
>  uint8_t xfs_compute_rextslog(xfs_rtbxlen_t rtextents);
>  
> +/* Do we support an rt volume having this number of rtextents? */
> +static inline bool
> +xfs_validate_rtextents(
> +	xfs_rtbxlen_t		rtextents)
> +{
> +	/* No runt rt volumes */
> +	if (rtextents == 0)
> +		return false;
> +
> +	return true;
> +}
> +
>  xfs_filblks_t xfs_rtbitmap_blockcount(struct xfs_mount *mp, xfs_rtbxlen_t
>  		rtextents);
>  unsigned long long xfs_rtbitmap_wordcount(struct xfs_mount *mp,
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index df12bf82ed18..4a9e8588f4c9 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -509,7 +509,8 @@ xfs_validate_sb_common(
>  		rbmblocks = howmany_64(sbp->sb_rextents,
>  				       NBBY * sbp->sb_blocksize);
>  
> -		if (sbp->sb_rextents != rexts ||
> +		if (!xfs_validate_rtextents(rexts) ||
> +		    sbp->sb_rextents != rexts ||
>  		    sbp->sb_rextslog != xfs_compute_rextslog(rexts) ||
>  		    sbp->sb_rbmblocks != rbmblocks) {
>  			xfs_notice(mp,
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 7c5a50163d2d..8feb58c6241c 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -963,6 +963,8 @@ xfs_growfs_rt(
>  	 */
>  	nrextents = nrblocks;
>  	do_div(nrextents, in->extsize);
> +	if (!xfs_validate_rtextents(nrextents))
> +		return -EINVAL;
>  	nrbmblocks = xfs_rtbitmap_blockcount(mp, nrextents);
>  	nrextslog = xfs_compute_rextslog(nrextents);
>  	nrsumlevels = nrextslog + 1;
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] xfs: make rextslog computation consistent with mkfs
  2023-12-03 19:05 ` [PATCH 1/3] xfs: make rextslog computation consistent with mkfs Darrick J. Wong
@ 2023-12-04  4:55   ` Christoph Hellwig
  2023-12-04 18:52     ` Darrick J. Wong
  0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2023-12-04  4:55 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: chandanbabu, hch, linux-xfs

> + */
> +uint8_t
> +xfs_compute_rextslog(
> +	xfs_rtbxlen_t		rtextents)
> +{
> +	return rtextents ? xfs_highbit32(rtextents) : 0;

It might just be a personal pet peeve, but I find a good old if much
more readable for this:

	if (!rtextents)
		return 0;
	return xfs_highbit32(rtextents);

Otherwise looks good:

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] xfs: fix 32-bit truncation in xfs_compute_rextslog
  2023-12-03 19:05 ` [PATCH 2/3] xfs: fix 32-bit truncation in xfs_compute_rextslog Darrick J. Wong
@ 2023-12-04  4:55   ` Christoph Hellwig
  0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2023-12-04  4:55 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: chandanbabu, hch, linux-xfs

Looks good:

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] xfs: don't allow overly small or large realtime volumes
  2023-12-03 19:05 ` [PATCH 3/3] xfs: don't allow overly small or large realtime volumes Darrick J. Wong
  2023-12-03 19:09   ` Darrick J. Wong
@ 2023-12-04  4:56   ` Christoph Hellwig
  1 sibling, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2023-12-04  4:56 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: chandanbabu, hch, linux-xfs

With the updated commit message:

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] xfs: make rextslog computation consistent with mkfs
  2023-12-04  4:55   ` Christoph Hellwig
@ 2023-12-04 18:52     ` Darrick J. Wong
  0 siblings, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2023-12-04 18:52 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: chandanbabu, linux-xfs

On Mon, Dec 04, 2023 at 05:55:26AM +0100, Christoph Hellwig wrote:
> > + */
> > +uint8_t
> > +xfs_compute_rextslog(
> > +	xfs_rtbxlen_t		rtextents)
> > +{
> > +	return rtextents ? xfs_highbit32(rtextents) : 0;
> 
> It might just be a personal pet peeve, but I find a good old if much
> more readable for this:
> 
> 	if (!rtextents)
> 		return 0;
> 	return xfs_highbit32(rtextents);
> 
> Otherwise looks good:

Same here.  I'll adjust it in the next patch, since this one is the
copypastahappy hoist.

--D

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] xfs: make rextslog computation consistent with mkfs
  2023-12-07  2:23 [PATCHSET v2 0/3] xfs: fix realtime geometry integer overflows Darrick J. Wong
@ 2023-12-07  2:27 ` Darrick J. Wong
  0 siblings, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2023-12-07  2:27 UTC (permalink / raw)
  To: chandanbabu, hch, djwong; +Cc: linux-xfs

From: Darrick J. Wong <djwong@kernel.org>

There's a weird discrepancy in xfsprogs dating back to the creation of
the Linux port -- if there are zero rt extents, mkfs will set
sb_rextents and sb_rextslog both to zero:

	sbp->sb_rextslog =
		(uint8_t)(rtextents ?
			libxfs_highbit32((unsigned int)rtextents) : 0);

However, that's not the check that xfs_repair uses for nonzero rtblocks:

	if (sb->sb_rextslog !=
			libxfs_highbit32((unsigned int)sb->sb_rextents))

The difference here is that xfs_highbit32 returns -1 if its argument is
zero.  Unfortunately, this means that in the weird corner case of a
realtime volume shorter than 1 rt extent, xfs_repair will immediately
flag a freshly formatted filesystem as corrupt.  Because mkfs has been
writing ondisk artifacts like this for decades, we have to accept that
as "correct".  TBH, zero rextslog for zero rtextents makes more sense to
me anyway.

Regrettably, the superblock verifier checks created in commit copied
xfs_repair even though mkfs has been writing out such filesystems for
ages.  Fix the superblock verifier to accept what mkfs spits out; the
userspace version of this patch will have to fix xfs_repair as well.

Note that the new helper leaves the zeroday bug where the upper 32 bits
of sb_rextents is ripped off and fed to highbit32.  This leads to a
seriously undersized rt summary file, which immediately breaks mkfs:

$ hugedisk.sh foo /dev/sdc $(( 0x100000080 * 4096))B
$ /sbin/mkfs.xfs -f /dev/sda -m rmapbt=0,reflink=0 -r rtdev=/dev/mapper/foo
meta-data=/dev/sda               isize=512    agcount=4, agsize=1298176 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=0    bigtime=1 inobtcount=1 nrext64=1
data     =                       bsize=4096   blocks=5192704, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=16384, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =/dev/mapper/foo        extsz=4096   blocks=4294967424, rtextents=4294967424
Discarding blocks...Done.
mkfs.xfs: Error initializing the realtime space [117 - Structure needs cleaning]

The next patch will drop support for rt volumes with fewer than 1 or
more than 2^32-1 rt extents, since they've clearly been broken forever.

Fixes: f8e566c0f5e1f ("xfs: validate the realtime geometry in xfs_validate_sb_common")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_rtbitmap.c |   12 ++++++++++++
 fs/xfs/libxfs/xfs_rtbitmap.h |    3 +++
 fs/xfs/libxfs/xfs_sb.c       |    3 ++-
 fs/xfs/xfs_rtalloc.c         |    4 ++--
 4 files changed, 19 insertions(+), 3 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index c269d704314d..1c9fed76a356 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -1130,6 +1130,18 @@ xfs_rtbitmap_blockcount(
 	return howmany_64(rtextents, NBBY * mp->m_sb.sb_blocksize);
 }
 
+/*
+ * Compute the maximum level number of the realtime summary file, as defined by
+ * mkfs.  The use of highbit32 on a 64-bit quantity is a historic artifact that
+ * prohibits correct use of rt volumes with more than 2^32 extents.
+ */
+uint8_t
+xfs_compute_rextslog(
+	xfs_rtbxlen_t		rtextents)
+{
+	return rtextents ? xfs_highbit32(rtextents) : 0;
+}
+
 /*
  * Compute the number of rtbitmap words needed to populate every block of a
  * bitmap that is large enough to track the given number of rt extents.
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
index c0637057d69c..6e5bae324cc3 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.h
+++ b/fs/xfs/libxfs/xfs_rtbitmap.h
@@ -351,6 +351,8 @@ xfs_rtfree_extent(
 int xfs_rtfree_blocks(struct xfs_trans *tp, xfs_fsblock_t rtbno,
 		xfs_filblks_t rtlen);
 
+uint8_t xfs_compute_rextslog(xfs_rtbxlen_t rtextents);
+
 xfs_filblks_t xfs_rtbitmap_blockcount(struct xfs_mount *mp, xfs_rtbxlen_t
 		rtextents);
 unsigned long long xfs_rtbitmap_wordcount(struct xfs_mount *mp,
@@ -369,6 +371,7 @@ unsigned long long xfs_rtsummary_wordcount(struct xfs_mount *mp,
 # define xfs_rtsummary_read_buf(a,b)			(-ENOSYS)
 # define xfs_rtbuf_cache_relse(a)			(0)
 # define xfs_rtalloc_extent_is_free(m,t,s,l,i)		(-ENOSYS)
+# define xfs_compute_rextslog(rtx)			(0)
 static inline xfs_filblks_t
 xfs_rtbitmap_blockcount(struct xfs_mount *mp, xfs_rtbxlen_t rtextents)
 {
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 1f74d0cd1618..df12bf82ed18 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -25,6 +25,7 @@
 #include "xfs_da_format.h"
 #include "xfs_health.h"
 #include "xfs_ag.h"
+#include "xfs_rtbitmap.h"
 
 /*
  * Physical superblock buffer manipulations. Shared with libxfs in userspace.
@@ -509,7 +510,7 @@ xfs_validate_sb_common(
 				       NBBY * sbp->sb_blocksize);
 
 		if (sbp->sb_rextents != rexts ||
-		    sbp->sb_rextslog != xfs_highbit32(sbp->sb_rextents) ||
+		    sbp->sb_rextslog != xfs_compute_rextslog(rexts) ||
 		    sbp->sb_rbmblocks != rbmblocks) {
 			xfs_notice(mp,
 				"realtime geometry sanity check failed");
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 88c48de5c9c8..7c5a50163d2d 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -964,7 +964,7 @@ xfs_growfs_rt(
 	nrextents = nrblocks;
 	do_div(nrextents, in->extsize);
 	nrbmblocks = xfs_rtbitmap_blockcount(mp, nrextents);
-	nrextslog = xfs_highbit32(nrextents);
+	nrextslog = xfs_compute_rextslog(nrextents);
 	nrsumlevels = nrextslog + 1;
 	nrsumblocks = xfs_rtsummary_blockcount(mp, nrsumlevels, nrbmblocks);
 	nrsumsize = XFS_FSB_TO_B(mp, nrsumblocks);
@@ -1031,7 +1031,7 @@ xfs_growfs_rt(
 		nsbp->sb_rblocks = min(nrblocks, nrblocks_step);
 		nsbp->sb_rextents = xfs_rtb_to_rtx(nmp, nsbp->sb_rblocks);
 		ASSERT(nsbp->sb_rextents != 0);
-		nsbp->sb_rextslog = xfs_highbit32(nsbp->sb_rextents);
+		nsbp->sb_rextslog = xfs_compute_rextslog(nsbp->sb_rextents);
 		nrsumlevels = nmp->m_rsumlevels = nsbp->sb_rextslog + 1;
 		nrsumblocks = xfs_rtsummary_blockcount(mp, nrsumlevels,
 				nsbp->sb_rbmblocks);


^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-12-07  2:27 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-03 19:00 [PATCHSET 0/3] xfs: fix realtime geometry integer overflows Darrick J. Wong
2023-12-03 19:05 ` [PATCH 1/3] xfs: make rextslog computation consistent with mkfs Darrick J. Wong
2023-12-04  4:55   ` Christoph Hellwig
2023-12-04 18:52     ` Darrick J. Wong
2023-12-03 19:05 ` [PATCH 2/3] xfs: fix 32-bit truncation in xfs_compute_rextslog Darrick J. Wong
2023-12-04  4:55   ` Christoph Hellwig
2023-12-03 19:05 ` [PATCH 3/3] xfs: don't allow overly small or large realtime volumes Darrick J. Wong
2023-12-03 19:09   ` Darrick J. Wong
2023-12-04  4:56   ` Christoph Hellwig
  -- strict thread matches above, loose matches on Subject: below --
2023-12-07  2:23 [PATCHSET v2 0/3] xfs: fix realtime geometry integer overflows Darrick J. Wong
2023-12-07  2:27 ` [PATCH 1/3] xfs: make rextslog computation consistent with mkfs Darrick J. Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox