[PATCH 0/5] xfsprogs: miscellaneous cleanups

[PATCH 0/5] xfsprogs: miscellaneous cleanups

[Darrick J. Wong]

Hi all,

Here's some miscellaneous fixes for problems I found while fuzzing
xfsprogs.

--Darrick

[PATCH 1/5] xfs_db: sanitize geometry on load

[Darrick J. Wong]

xfs_db doesn't check the filesystem geometry when it's mounting, which
means that garbage agcount values can cause OOMs when we try to allocate
all the per-AG incore metadata.  If we see geometry that looks
suspicious, try to derive the actual AG geometry to avoid crashing the
system.  This should help with xfs/1301 fuzzing.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
variables to avoid screwing up the rest of the metadata.
v3: Suggest a possible agcount value, but always restrict to 1 AG.
v4: Remove the suggested agcount value and make sure we can't exit
the program with an exitcode of zero.
---
 db/init.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 10 deletions(-)


diff --git a/db/init.c b/db/init.c
index ec1e274..e60ac66 100644
--- a/db/init.c
+++ b/db/init.c
@@ -41,6 +41,7 @@ struct xfs_mount	*mp;
 struct xlog		xlog;
 libxfs_init_t		x;
 xfs_agnumber_t		cur_agno = NULLAGNUMBER;
+static bool		insane_agcount;
 
 static void
 usage(void)
@@ -51,13 +52,80 @@ usage(void)
 	exit(1);
 }
 
+/* Try to load a superblock for the given agno, no verifiers. */
+static bool
+load_sb(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	struct xfs_sb		*sbp)
+{
+	struct xfs_buf		*bp;
+
+	bp = libxfs_readbuf(mp->m_ddev_targp,
+			    XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
+			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
+
+	if (!bp || bp->b_error)
+		return false;
+
+	/* copy SB from buffer to in-core, converting architecture as we go */
+	libxfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
+	libxfs_putbuf(bp);
+	libxfs_purgebuf(bp);
+
+	return true;
+}
+
+/*
+ * If the agcount doesn't look sane, suggest a real agcount to the user,
+ * and pretend agcount = 1 to avoid OOMing libxfs_initialize_perag.
+ */
+static void
+sanitize_geometry(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
+{
+	unsigned int		blocklog;
+	unsigned int		blocksize;
+	unsigned long long	dblocks;
+
+	/* If the geometry looks ok, we're done. */
+	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
+	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
+	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
+	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
+	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
+	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
+		return;
+
+	/* Check blocklog and blocksize */
+	blocklog = sbp->sb_blocklog;
+	blocksize = sbp->sb_blocksize;
+	if (blocklog < XFS_MIN_BLOCKSIZE_LOG ||
+	    blocklog > XFS_MAX_BLOCKSIZE_LOG)
+		blocklog = libxfs_log2_roundup(blocksize);
+	if (blocksize != (1 << blocklog))
+		blocksize = (1 << blocksize);
+
+	/* Clamp dblocks to the size of the device. */
+	dblocks = sbp->sb_dblocks;
+	if (dblocks > x.dsize * x.dbsize / blocksize)
+		dblocks = x.dsize * x.dbsize / blocksize;
+
+	/* Assume 1 AG to avoid OOM. */
+	fprintf(stderr,
+_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
+		progname, fsdevice);
+	sbp->sb_agcount = 1;
+	insane_agcount = true;
+}
+
 void
 init(
 	int		argc,
 	char		**argv)
 {
 	struct xfs_sb	*sbp;
-	struct xfs_buf	*bp;
 	int		c;
 
 	setlocale(LC_ALL, "");
@@ -124,20 +192,12 @@ init(
 	 */
 	memset(&xmount, 0, sizeof(struct xfs_mount));
 	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
-	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
-			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
-
-	if (!bp || bp->b_error) {
+	if (!load_sb(&xmount, 0, &xmount.m_sb)) {
 		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
 			"bytes)\n"), progname, fsdevice);
 		exit(1);
 	}
 
-	/* copy SB from buffer to in-core, converting architecture as we go */
-	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
-	libxfs_putbuf(bp);
-	libxfs_purgebuf(bp);
-
 	sbp = &xmount.m_sb;
 	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
 		fprintf(stderr, _("%s: %s is not a valid XFS filesystem (unexpected SB magic number 0x%08x)\n"),
@@ -148,6 +208,8 @@ init(
 		}
 	}
 
+	sanitize_geometry(&xmount, sbp);
+
 	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
 			  LIBXFS_MOUNT_DEBUGGER);
 	if (!mp) {
@@ -230,5 +292,7 @@ main(
 		libxfs_device_close(x.logdev);
 	if (x.rtdev)
 		libxfs_device_close(x.rtdev);
+	if (insane_agcount && exitcode == 0)
+		exitcode = 1;
 	return exitcode;
 }

Re: [PATCH 1/5] xfs_db: sanitize geometry on load

[Eric Sandeen]

On 1/20/17 2:25 PM, Darrick J. Wong wrote:
> xfs_db doesn't check the filesystem geometry when it's mounting, which
> means that garbage agcount values can cause OOMs when we try to allocate
> all the per-AG incore metadata.  If we see geometry that looks
> suspicious, try to derive the actual AG geometry to avoid crashing the
> system.  This should help with xfs/1301 fuzzing.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
> variables to avoid screwing up the rest of the metadata.
> v3: Suggest a possible agcount value, but always restrict to 1 AG.
> v4: Remove the suggested agcount value and make sure we can't exit
> the program with an exitcode of zero.

ok, but insane_agcount doesn't need to be global... 

> ---
>  db/init.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 74 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/db/init.c b/db/init.c
> index ec1e274..e60ac66 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -41,6 +41,7 @@ struct xfs_mount	*mp;
>  struct xlog		xlog;
>  libxfs_init_t		x;
>  xfs_agnumber_t		cur_agno = NULLAGNUMBER;
> +static bool		insane_agcount;

Not a fan of the global.  Can you make it local, and then
do something like this:

if (sanitize_agount(&xmount, sbp))
	insane_agount = true;

>  static void
>  usage(void)
> @@ -51,13 +52,80 @@ usage(void)
>  	exit(1);
>  }
>  
> +/* Try to load a superblock for the given agno, no verifiers. */
> +static bool
> +load_sb(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		agno,
> +	struct xfs_sb		*sbp)

Now this is always called with agno == 0, so is there any point
to the arg?  Eh, I guess it's fine.

I can fix up the insane_agcount thing on the way in if you like.

-Eric

> +{
> +	struct xfs_buf		*bp;
> +
> +	bp = libxfs_readbuf(mp->m_ddev_targp,
> +			    XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
> +			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> +
> +	if (!bp || bp->b_error)
> +		return false;
> +
> +	/* copy SB from buffer to in-core, converting architecture as we go */
> +	libxfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
> +	libxfs_putbuf(bp);
> +	libxfs_purgebuf(bp);
> +
> +	return true;
> +}
> +
> +/*
> + * If the agcount doesn't look sane, suggest a real agcount to the user,
> + * and pretend agcount = 1 to avoid OOMing libxfs_initialize_perag.
> + */
> +static void
> +sanitize_geometry(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	unsigned int		blocklog;
> +	unsigned int		blocksize;
> +	unsigned long long	dblocks;
> +
> +	/* If the geometry looks ok, we're done. */
> +	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
> +	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
> +	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
> +	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
> +		return;
> +
> +	/* Check blocklog and blocksize */
> +	blocklog = sbp->sb_blocklog;
> +	blocksize = sbp->sb_blocksize;
> +	if (blocklog < XFS_MIN_BLOCKSIZE_LOG ||
> +	    blocklog > XFS_MAX_BLOCKSIZE_LOG)
> +		blocklog = libxfs_log2_roundup(blocksize);
> +	if (blocksize != (1 << blocklog))
> +		blocksize = (1 << blocksize);
> +
> +	/* Clamp dblocks to the size of the device. */
> +	dblocks = sbp->sb_dblocks;
> +	if (dblocks > x.dsize * x.dbsize / blocksize)
> +		dblocks = x.dsize * x.dbsize / blocksize;
> +
> +	/* Assume 1 AG to avoid OOM. */
> +	fprintf(stderr,
> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> +		progname, fsdevice);
> +	sbp->sb_agcount = 1;
> +	insane_agcount = true;



> +}
> +
>  void
>  init(
>  	int		argc,
>  	char		**argv)
>  {
>  	struct xfs_sb	*sbp;
> -	struct xfs_buf	*bp;
>  	int		c;
>  
>  	setlocale(LC_ALL, "");
> @@ -124,20 +192,12 @@ init(
>  	 */
>  	memset(&xmount, 0, sizeof(struct xfs_mount));
>  	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
> -	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
> -			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> -
> -	if (!bp || bp->b_error) {
> +	if (!load_sb(&xmount, 0, &xmount.m_sb)) {
>  		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
>  			"bytes)\n"), progname, fsdevice);
>  		exit(1);
>  	}
>  
> -	/* copy SB from buffer to in-core, converting architecture as we go */
> -	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
> -	libxfs_putbuf(bp);
> -	libxfs_purgebuf(bp);
> -
>  	sbp = &xmount.m_sb;
>  	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
>  		fprintf(stderr, _("%s: %s is not a valid XFS filesystem (unexpected SB magic number 0x%08x)\n"),
> @@ -148,6 +208,8 @@ init(
>  		}
>  	}
>  
> +	sanitize_geometry(&xmount, sbp);
> +
>  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
>  			  LIBXFS_MOUNT_DEBUGGER);
>  	if (!mp) {
> @@ -230,5 +292,7 @@ main(
>  		libxfs_device_close(x.logdev);
>  	if (x.rtdev)
>  		libxfs_device_close(x.rtdev);
> +	if (insane_agcount && exitcode == 0)
> +		exitcode = 1;
>  	return exitcode;
>  }
> 

[PATCH v5 1/5] xfs_db: sanitize geometry on load

[Darrick J. Wong]

xfs_db doesn't check the filesystem geometry when it's mounting, which
means that garbage agcount values can cause OOMs when we try to allocate
all the per-AG incore metadata.  If we see geometry that looks
suspicious, try to derive the actual AG geometry to avoid crashing the
system.  This should help with xfs/1301 fuzzing.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
variables to avoid screwing up the rest of the metadata.
v3: Suggest a possible agcount value, but always restrict to 1 AG.
v4: Remove the suggested agcount value and make sure we can't exit
the program with an exitcode of zero.
v5: remove global variables and unnecessary parameters
---
 db/init.c |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 77 insertions(+), 12 deletions(-)

diff --git a/db/init.c b/db/init.c
index ec1e274..0c684a5 100644
--- a/db/init.c
+++ b/db/init.c
@@ -51,13 +51,81 @@ usage(void)
 	exit(1);
 }
 
+/* Try to load a superblock for the given agno, no verifiers. */
+static bool
+load_sb(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
+{
+	struct xfs_buf		*bp;
+
+	bp = libxfs_readbuf(mp->m_ddev_targp, XFS_SB_DADDR,
+			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
+
+	if (!bp || bp->b_error)
+		return false;
+
+	/* copy SB from buffer to in-core, converting architecture as we go */
+	libxfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
+	libxfs_putbuf(bp);
+	libxfs_purgebuf(bp);
+
+	return true;
+}
+
+/*
+ * If the agcount doesn't look sane, suggest a real agcount to the user,
+ * and pretend agcount = 1 to avoid OOMing libxfs_initialize_perag.
+ * Returns true if the geometry was sane.
+ */
+static bool
+sanitize_geometry(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
+{
+	unsigned int		blocklog;
+	unsigned int		blocksize;
+	unsigned long long	dblocks;
+
+	/* If the geometry looks ok, we're done. */
+	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
+	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
+	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
+	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
+	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
+	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
+		return true;
+
+	/* Check blocklog and blocksize */
+	blocklog = sbp->sb_blocklog;
+	blocksize = sbp->sb_blocksize;
+	if (blocklog < XFS_MIN_BLOCKSIZE_LOG ||
+	    blocklog > XFS_MAX_BLOCKSIZE_LOG)
+		blocklog = libxfs_log2_roundup(blocksize);
+	if (blocksize != (1 << blocklog))
+		blocksize = (1 << blocksize);
+
+	/* Clamp dblocks to the size of the device. */
+	dblocks = sbp->sb_dblocks;
+	if (dblocks > x.dsize * x.dbsize / blocksize)
+		dblocks = x.dsize * x.dbsize / blocksize;
+
+	/* Assume 1 AG to avoid OOM. */
+	fprintf(stderr,
+_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
+		progname, fsdevice);
+	sbp->sb_agcount = 1;
+
+	return false;
+}
+
 void
 init(
 	int		argc,
-	char		**argv)
+	char		**argv,
+	bool		*insane_agcount)
 {
 	struct xfs_sb	*sbp;
-	struct xfs_buf	*bp;
 	int		c;
 
 	setlocale(LC_ALL, "");
@@ -124,20 +192,12 @@ init(
 	 */
 	memset(&xmount, 0, sizeof(struct xfs_mount));
 	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
-	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
-			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
-
-	if (!bp || bp->b_error) {
+	if (!load_sb(&xmount, &xmount.m_sb)) {
 		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
 			"bytes)\n"), progname, fsdevice);
 		exit(1);
 	}
 
-	/* copy SB from buffer to in-core, converting architecture as we go */
-	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
-	libxfs_putbuf(bp);
-	libxfs_purgebuf(bp);
-
 	sbp = &xmount.m_sb;
 	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
 		fprintf(stderr, _("%s: %s is not a valid XFS filesystem (unexpected SB magic number 0x%08x)\n"),
@@ -148,6 +208,8 @@ init(
 		}
 	}
 
+	*insane_agcount = !sanitize_geometry(&xmount, sbp);
+
 	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
 			  LIBXFS_MOUNT_DEBUGGER);
 	if (!mp) {
@@ -190,10 +252,11 @@ main(
 	int	c, i, done = 0;
 	char	*input;
 	char	**v;
+	bool	insane_agcount;
 	int	start_iocur_sp;
 
 	pushfile(stdin);
-	init(argc, argv);
+	init(argc, argv, &insane_agcount);
 	start_iocur_sp = iocur_sp;
 
 	for (i = 0; !done && i < ncmdline; i++) {
@@ -230,5 +293,7 @@ main(
 		libxfs_device_close(x.logdev);
 	if (x.rtdev)
 		libxfs_device_close(x.rtdev);
+	if (insane_agcount && exitcode == 0)
+		exitcode = 1;
 	return exitcode;
 }

Re: [PATCH v5 1/5] xfs_db: sanitize geometry on load

[Eric Sandeen]

On 1/20/17 6:15 PM, Darrick J. Wong wrote:
> xfs_db doesn't check the filesystem geometry when it's mounting, which
> means that garbage agcount values can cause OOMs when we try to allocate
> all the per-AG incore metadata.  If we see geometry that looks
> suspicious, try to derive the actual AG geometry to avoid crashing the
> system.  This should help with xfs/1301 fuzzing.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

If it's ok with you I'll modify a comment (no suggestion of proper
agcount is made) and rename sanitize_geometry to sanitize_agcount
or something like that - otherwise,

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
> v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
> variables to avoid screwing up the rest of the metadata.
> v3: Suggest a possible agcount value, but always restrict to 1 AG.
> v4: Remove the suggested agcount value and make sure we can't exit
> the program with an exitcode of zero.
> v5: remove global variables and unnecessary parameters
> ---
>  db/init.c |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 77 insertions(+), 12 deletions(-)
> 
> diff --git a/db/init.c b/db/init.c
> index ec1e274..0c684a5 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -51,13 +51,81 @@ usage(void)
>  	exit(1);
>  }
>  
> +/* Try to load a superblock for the given agno, no verifiers. */
> +static bool
> +load_sb(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	struct xfs_buf		*bp;
> +
> +	bp = libxfs_readbuf(mp->m_ddev_targp, XFS_SB_DADDR,
> +			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> +
> +	if (!bp || bp->b_error)
> +		return false;
> +
> +	/* copy SB from buffer to in-core, converting architecture as we go */
> +	libxfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
> +	libxfs_putbuf(bp);
> +	libxfs_purgebuf(bp);
> +
> +	return true;
> +}
> +
> +/*
> + * If the agcount doesn't look sane, suggest a real agcount to the user,
> + * and pretend agcount = 1 to avoid OOMing libxfs_initialize_perag.
> + * Returns true if the geometry was sane.
> + */
> +static bool
> +sanitize_geometry(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	unsigned int		blocklog;
> +	unsigned int		blocksize;
> +	unsigned long long	dblocks;
> +
> +	/* If the geometry looks ok, we're done. */
> +	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
> +	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
> +	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
> +	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
> +		return true;
> +
> +	/* Check blocklog and blocksize */
> +	blocklog = sbp->sb_blocklog;
> +	blocksize = sbp->sb_blocksize;
> +	if (blocklog < XFS_MIN_BLOCKSIZE_LOG ||
> +	    blocklog > XFS_MAX_BLOCKSIZE_LOG)
> +		blocklog = libxfs_log2_roundup(blocksize);
> +	if (blocksize != (1 << blocklog))
> +		blocksize = (1 << blocksize);
> +
> +	/* Clamp dblocks to the size of the device. */
> +	dblocks = sbp->sb_dblocks;
> +	if (dblocks > x.dsize * x.dbsize / blocksize)
> +		dblocks = x.dsize * x.dbsize / blocksize;
> +
> +	/* Assume 1 AG to avoid OOM. */
> +	fprintf(stderr,
> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> +		progname, fsdevice);
> +	sbp->sb_agcount = 1;
> +
> +	return false;
> +}
> +
>  void
>  init(
>  	int		argc,
> -	char		**argv)
> +	char		**argv,
> +	bool		*insane_agcount)
>  {
>  	struct xfs_sb	*sbp;
> -	struct xfs_buf	*bp;
>  	int		c;
>  
>  	setlocale(LC_ALL, "");
> @@ -124,20 +192,12 @@ init(
>  	 */
>  	memset(&xmount, 0, sizeof(struct xfs_mount));
>  	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
> -	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
> -			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> -
> -	if (!bp || bp->b_error) {
> +	if (!load_sb(&xmount, &xmount.m_sb)) {
>  		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
>  			"bytes)\n"), progname, fsdevice);
>  		exit(1);
>  	}
>  
> -	/* copy SB from buffer to in-core, converting architecture as we go */
> -	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
> -	libxfs_putbuf(bp);
> -	libxfs_purgebuf(bp);
> -
>  	sbp = &xmount.m_sb;
>  	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
>  		fprintf(stderr, _("%s: %s is not a valid XFS filesystem (unexpected SB magic number 0x%08x)\n"),
> @@ -148,6 +208,8 @@ init(
>  		}
>  	}
>  
> +	*insane_agcount = !sanitize_geometry(&xmount, sbp);
> +
>  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
>  			  LIBXFS_MOUNT_DEBUGGER);
>  	if (!mp) {
> @@ -190,10 +252,11 @@ main(
>  	int	c, i, done = 0;
>  	char	*input;
>  	char	**v;
> +	bool	insane_agcount;
>  	int	start_iocur_sp;
>  
>  	pushfile(stdin);
> -	init(argc, argv);
> +	init(argc, argv, &insane_agcount);
>  	start_iocur_sp = iocur_sp;
>  
>  	for (i = 0; !done && i < ncmdline; i++) {
> @@ -230,5 +293,7 @@ main(
>  		libxfs_device_close(x.logdev);
>  	if (x.rtdev)
>  		libxfs_device_close(x.rtdev);
> +	if (insane_agcount && exitcode == 0)
> +		exitcode = 1;
>  	return exitcode;
>  }
> --
> 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 v5 1/5] xfs_db: sanitize geometry on load

[Darrick J. Wong]

On Mon, Jan 23, 2017 at 02:02:07PM -0600, Eric Sandeen wrote:
> On 1/20/17 6:15 PM, Darrick J. Wong wrote:
> > xfs_db doesn't check the filesystem geometry when it's mounting, which
> > means that garbage agcount values can cause OOMs when we try to allocate
> > all the per-AG incore metadata.  If we see geometry that looks
> > suspicious, try to derive the actual AG geometry to avoid crashing the
> > system.  This should help with xfs/1301 fuzzing.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If it's ok with you I'll modify a comment (no suggestion of proper
> agcount is made) and rename sanitize_geometry to sanitize_agcount
> or something like that - otherwise,

Sure.

--D

> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > ---
> > v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
> > variables to avoid screwing up the rest of the metadata.
> > v3: Suggest a possible agcount value, but always restrict to 1 AG.
> > v4: Remove the suggested agcount value and make sure we can't exit
> > the program with an exitcode of zero.
> > v5: remove global variables and unnecessary parameters
> > ---
> >  db/init.c |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 77 insertions(+), 12 deletions(-)
> > 
> > diff --git a/db/init.c b/db/init.c
> > index ec1e274..0c684a5 100644
> > --- a/db/init.c
> > +++ b/db/init.c
> > @@ -51,13 +51,81 @@ usage(void)
> >  	exit(1);
> >  }
> >  
> > +/* Try to load a superblock for the given agno, no verifiers. */
> > +static bool
> > +load_sb(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_sb		*sbp)
> > +{
> > +	struct xfs_buf		*bp;
> > +
> > +	bp = libxfs_readbuf(mp->m_ddev_targp, XFS_SB_DADDR,
> > +			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> > +
> > +	if (!bp || bp->b_error)
> > +		return false;
> > +
> > +	/* copy SB from buffer to in-core, converting architecture as we go */
> > +	libxfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
> > +	libxfs_putbuf(bp);
> > +	libxfs_purgebuf(bp);
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * If the agcount doesn't look sane, suggest a real agcount to the user,
> > + * and pretend agcount = 1 to avoid OOMing libxfs_initialize_perag.
> > + * Returns true if the geometry was sane.
> > + */
> > +static bool
> > +sanitize_geometry(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_sb		*sbp)
> > +{
> > +	unsigned int		blocklog;
> > +	unsigned int		blocksize;
> > +	unsigned long long	dblocks;
> > +
> > +	/* If the geometry looks ok, we're done. */
> > +	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
> > +	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
> > +	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
> > +	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
> > +	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
> > +	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
> > +		return true;
> > +
> > +	/* Check blocklog and blocksize */
> > +	blocklog = sbp->sb_blocklog;
> > +	blocksize = sbp->sb_blocksize;
> > +	if (blocklog < XFS_MIN_BLOCKSIZE_LOG ||
> > +	    blocklog > XFS_MAX_BLOCKSIZE_LOG)
> > +		blocklog = libxfs_log2_roundup(blocksize);
> > +	if (blocksize != (1 << blocklog))
> > +		blocksize = (1 << blocksize);
> > +
> > +	/* Clamp dblocks to the size of the device. */
> > +	dblocks = sbp->sb_dblocks;
> > +	if (dblocks > x.dsize * x.dbsize / blocksize)
> > +		dblocks = x.dsize * x.dbsize / blocksize;
> > +
> > +	/* Assume 1 AG to avoid OOM. */
> > +	fprintf(stderr,
> > +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> > +		progname, fsdevice);
> > +	sbp->sb_agcount = 1;
> > +
> > +	return false;
> > +}
> > +
> >  void
> >  init(
> >  	int		argc,
> > -	char		**argv)
> > +	char		**argv,
> > +	bool		*insane_agcount)
> >  {
> >  	struct xfs_sb	*sbp;
> > -	struct xfs_buf	*bp;
> >  	int		c;
> >  
> >  	setlocale(LC_ALL, "");
> > @@ -124,20 +192,12 @@ init(
> >  	 */
> >  	memset(&xmount, 0, sizeof(struct xfs_mount));
> >  	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
> > -	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
> > -			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> > -
> > -	if (!bp || bp->b_error) {
> > +	if (!load_sb(&xmount, &xmount.m_sb)) {
> >  		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
> >  			"bytes)\n"), progname, fsdevice);
> >  		exit(1);
> >  	}
> >  
> > -	/* copy SB from buffer to in-core, converting architecture as we go */
> > -	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
> > -	libxfs_putbuf(bp);
> > -	libxfs_purgebuf(bp);
> > -
> >  	sbp = &xmount.m_sb;
> >  	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
> >  		fprintf(stderr, _("%s: %s is not a valid XFS filesystem (unexpected SB magic number 0x%08x)\n"),
> > @@ -148,6 +208,8 @@ init(
> >  		}
> >  	}
> >  
> > +	*insane_agcount = !sanitize_geometry(&xmount, sbp);
> > +
> >  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
> >  			  LIBXFS_MOUNT_DEBUGGER);
> >  	if (!mp) {
> > @@ -190,10 +252,11 @@ main(
> >  	int	c, i, done = 0;
> >  	char	*input;
> >  	char	**v;
> > +	bool	insane_agcount;
> >  	int	start_iocur_sp;
> >  
> >  	pushfile(stdin);
> > -	init(argc, argv);
> > +	init(argc, argv, &insane_agcount);
> >  	start_iocur_sp = iocur_sp;
> >  
> >  	for (i = 0; !done && i < ncmdline; i++) {
> > @@ -230,5 +293,7 @@ main(
> >  		libxfs_device_close(x.logdev);
> >  	if (x.rtdev)
> >  		libxfs_device_close(x.rtdev);
> > +	if (insane_agcount && exitcode == 0)
> > +		exitcode = 1;
> >  	return exitcode;
> >  }
> > --
> > 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
> > 
> --
> 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 v5 1/5] xfs_db: sanitize geometry on load

[Darrick J. Wong]

On Fri, Jan 20, 2017 at 04:15:05PM -0800, Darrick J. Wong wrote:
> xfs_db doesn't check the filesystem geometry when it's mounting, which
> means that garbage agcount values can cause OOMs when we try to allocate
> all the per-AG incore metadata.  If we see geometry that looks
> suspicious, try to derive the actual AG geometry to avoid crashing the
> system.  This should help with xfs/1301 fuzzing.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
> variables to avoid screwing up the rest of the metadata.
> v3: Suggest a possible agcount value, but always restrict to 1 AG.
> v4: Remove the suggested agcount value and make sure we can't exit
> the program with an exitcode of zero.
> v5: remove global variables and unnecessary parameters
> ---
>  db/init.c |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 77 insertions(+), 12 deletions(-)
> 
> diff --git a/db/init.c b/db/init.c
> index ec1e274..0c684a5 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -51,13 +51,81 @@ usage(void)
>  	exit(1);
>  }
>  
> +/* Try to load a superblock for the given agno, no verifiers. */
> +static bool
> +load_sb(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	struct xfs_buf		*bp;
> +
> +	bp = libxfs_readbuf(mp->m_ddev_targp, XFS_SB_DADDR,
> +			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> +
> +	if (!bp || bp->b_error)
> +		return false;
> +
> +	/* copy SB from buffer to in-core, converting architecture as we go */
> +	libxfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
> +	libxfs_putbuf(bp);
> +	libxfs_purgebuf(bp);
> +
> +	return true;
> +}
> +
> +/*
> + * If the agcount doesn't look sane, suggest a real agcount to the user,
> + * and pretend agcount = 1 to avoid OOMing libxfs_initialize_perag.
> + * Returns true if the geometry was sane.
> + */
> +static bool
> +sanitize_geometry(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	unsigned int		blocklog;
> +	unsigned int		blocksize;
> +	unsigned long long	dblocks;
> +
> +	/* If the geometry looks ok, we're done. */
> +	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
> +	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
> +	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
> +	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
> +		return true;
> +
> +	/* Check blocklog and blocksize */
> +	blocklog = sbp->sb_blocklog;
> +	blocksize = sbp->sb_blocksize;
> +	if (blocklog < XFS_MIN_BLOCKSIZE_LOG ||
> +	    blocklog > XFS_MAX_BLOCKSIZE_LOG)
> +		blocklog = libxfs_log2_roundup(blocksize);
> +	if (blocksize != (1 << blocklog))
> +		blocksize = (1 << blocksize);
> +
> +	/* Clamp dblocks to the size of the device. */
> +	dblocks = sbp->sb_dblocks;
> +	if (dblocks > x.dsize * x.dbsize / blocksize)
> +		dblocks = x.dsize * x.dbsize / blocksize;

NAK, this is dead code and the comments are wrong.  Sending v6.

--D

> +
> +	/* Assume 1 AG to avoid OOM. */
> +	fprintf(stderr,
> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> +		progname, fsdevice);
> +	sbp->sb_agcount = 1;
> +
> +	return false;
> +}
> +
>  void
>  init(
>  	int		argc,
> -	char		**argv)
> +	char		**argv,
> +	bool		*insane_agcount)
>  {
>  	struct xfs_sb	*sbp;
> -	struct xfs_buf	*bp;
>  	int		c;
>  
>  	setlocale(LC_ALL, "");
> @@ -124,20 +192,12 @@ init(
>  	 */
>  	memset(&xmount, 0, sizeof(struct xfs_mount));
>  	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
> -	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
> -			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> -
> -	if (!bp || bp->b_error) {
> +	if (!load_sb(&xmount, &xmount.m_sb)) {
>  		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
>  			"bytes)\n"), progname, fsdevice);
>  		exit(1);
>  	}
>  
> -	/* copy SB from buffer to in-core, converting architecture as we go */
> -	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
> -	libxfs_putbuf(bp);
> -	libxfs_purgebuf(bp);
> -
>  	sbp = &xmount.m_sb;
>  	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
>  		fprintf(stderr, _("%s: %s is not a valid XFS filesystem (unexpected SB magic number 0x%08x)\n"),
> @@ -148,6 +208,8 @@ init(
>  		}
>  	}
>  
> +	*insane_agcount = !sanitize_geometry(&xmount, sbp);
> +
>  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
>  			  LIBXFS_MOUNT_DEBUGGER);
>  	if (!mp) {
> @@ -190,10 +252,11 @@ main(
>  	int	c, i, done = 0;
>  	char	*input;
>  	char	**v;
> +	bool	insane_agcount;
>  	int	start_iocur_sp;
>  
>  	pushfile(stdin);
> -	init(argc, argv);
> +	init(argc, argv, &insane_agcount);
>  	start_iocur_sp = iocur_sp;
>  
>  	for (i = 0; !done && i < ncmdline; i++) {
> @@ -230,5 +293,7 @@ main(
>  		libxfs_device_close(x.logdev);
>  	if (x.rtdev)
>  		libxfs_device_close(x.rtdev);
> +	if (insane_agcount && exitcode == 0)
> +		exitcode = 1;
>  	return exitcode;
>  }
> --
> 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 v6 1/5] xfs_db: sanitize geometry on load

[Darrick J. Wong]

xfs_db doesn't check the filesystem geometry when it's mounting, which
means that garbage agcount values can cause OOMs when we try to allocate
all the per-AG incore metadata.  If we see geometry that looks
suspicious, limit agcount to 1 so that we avoid OOM.  This should help
with xfs/1301 fuzzing.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
variables to avoid screwing up the rest of the metadata.
v3: Suggest a possible agcount value, but always restrict to 1 AG.
v4: Remove the suggested agcount value and make sure we can't exit
the program with an exitcode of zero.
v5: remove global variables and unnecessary parameters
v6: remove dead code, fix comments
---
 db/init.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 59 insertions(+), 12 deletions(-)

diff --git a/db/init.c b/db/init.c
index ec1e274..6ebe6b7 100644
--- a/db/init.c
+++ b/db/init.c
@@ -51,13 +51,63 @@ usage(void)
 	exit(1);
 }
 
+/* Try to load a superblock for the given agno, no verifiers. */
+static bool
+load_sb(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
+{
+	struct xfs_buf		*bp;
+
+	bp = libxfs_readbuf(mp->m_ddev_targp, XFS_SB_DADDR,
+			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
+
+	if (!bp || bp->b_error)
+		return false;
+
+	/* copy SB from buffer to in-core, converting architecture as we go */
+	libxfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
+	libxfs_putbuf(bp);
+	libxfs_purgebuf(bp);
+
+	return true;
+}
+
+/*
+ * If the agcount doesn't look sane, pretend agcount = 1 to avoid
+ * OOMing libxfs_initialize_perag.  Returns true if the geometry looked
+ * sane.
+ */
+static bool
+sanitize_agcount(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
+{
+	/* If the geometry looks ok, we're done. */
+	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
+	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
+	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
+	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
+	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
+	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
+		return true;
+
+	/* Assume 1 AG to avoid OOM. */
+	fprintf(stderr,
+_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
+		progname, fsdevice);
+	sbp->sb_agcount = 1;
+
+	return false;
+}
+
 void
 init(
 	int		argc,
-	char		**argv)
+	char		**argv,
+	bool		*insane_agcount)
 {
 	struct xfs_sb	*sbp;
-	struct xfs_buf	*bp;
 	int		c;
 
 	setlocale(LC_ALL, "");
@@ -124,20 +174,12 @@ init(
 	 */
 	memset(&xmount, 0, sizeof(struct xfs_mount));
 	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
-	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
-			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
-
-	if (!bp || bp->b_error) {
+	if (!load_sb(&xmount, &xmount.m_sb)) {
 		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
 			"bytes)\n"), progname, fsdevice);
 		exit(1);
 	}
 
-	/* copy SB from buffer to in-core, converting architecture as we go */
-	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
-	libxfs_putbuf(bp);
-	libxfs_purgebuf(bp);
-
 	sbp = &xmount.m_sb;
 	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
 		fprintf(stderr, _("%s: %s is not a valid XFS filesystem (unexpected SB magic number 0x%08x)\n"),
@@ -148,6 +190,8 @@ init(
 		}
 	}
 
+	*insane_agcount = !sanitize_agcount(&xmount, sbp);
+
 	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
 			  LIBXFS_MOUNT_DEBUGGER);
 	if (!mp) {
@@ -190,10 +234,11 @@ main(
 	int	c, i, done = 0;
 	char	*input;
 	char	**v;
+	bool	insane_agcount;
 	int	start_iocur_sp;
 
 	pushfile(stdin);
-	init(argc, argv);
+	init(argc, argv, &insane_agcount);
 	start_iocur_sp = iocur_sp;
 
 	for (i = 0; !done && i < ncmdline; i++) {
@@ -230,5 +275,7 @@ main(
 		libxfs_device_close(x.logdev);
 	if (x.rtdev)
 		libxfs_device_close(x.rtdev);
+	if (insane_agcount && exitcode == 0)
+		exitcode = 1;
 	return exitcode;
 }

Re: [PATCH v6 1/5] xfs_db: sanitize geometry on load

[Eric Sandeen]

On 1/23/17 3:31 PM, Darrick J. Wong wrote:
> xfs_db doesn't check the filesystem geometry when it's mounting, which
> means that garbage agcount values can cause OOMs when we try to allocate
> all the per-AG incore metadata.  If we see geometry that looks
> suspicious, limit agcount to 1 so that we avoid OOM.  This should help
> with xfs/1301 fuzzing.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
> variables to avoid screwing up the rest of the metadata.
> v3: Suggest a possible agcount value, but always restrict to 1 AG.
> v4: Remove the suggested agcount value and make sure we can't exit
> the program with an exitcode of zero.
> v5: remove global variables and unnecessary parameters
> v6: remove dead code, fix comments
> ---
>  db/init.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 59 insertions(+), 12 deletions(-)
> 
> diff --git a/db/init.c b/db/init.c
> index ec1e274..6ebe6b7 100644
> --- a/db/init.c
> +++ b/db/init.c

...

> +/*
> + * If the agcount doesn't look sane, pretend agcount = 1 to avoid
> + * OOMing libxfs_initialize_perag.  Returns true if the geometry looked
> + * sane.
> + */
> +static bool
> +sanitize_agcount(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	/* If the geometry looks ok, we're done. */
> +	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
> +	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
> +	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
> +	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
> +		return true;
> +
> +	/* Assume 1 AG to avoid OOM. */
> +	fprintf(stderr,
> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> +		progname, fsdevice);
> +	sbp->sb_agcount = 1;
> +
> +	return false;

So now this claims insane AGs for a bad blocklog, etc.  :(

Let me send a patch which I think will work better.

-Eric

[PATCH v7 1/5] xfs_db: sanitize agcount on load

[Eric Sandeen]

Before we get into libxfs_initialize_perag and try to blindly
allocate a perag struct for every (possibly corrupted number of)
AGs, see if we can read the last one.  If not, assume it's corrupt,
and load only the first AG.

Do this only for an arbitrarily high-ish agcount, so that normal-ish
geometry on a possibly truncated file or device will still do
its best to make all readable AGs available.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/libxfs/init.c b/libxfs/init.c
index a08575a..ca5101e 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -817,6 +817,28 @@ libxfs_mount(
 			return NULL;
 	}
 
+	/*
+	 * libxfs_initialize_perag will allocate a perag structure for each AG.
+	 * If agcount is corrupted and insanely high, this will OOM the box.
+	 * If the agount seems (arbitrarily) high, try to read what would be
+	 * the last AG, and if that fails, just read the first one and let
+	 * the user know what happened.
+	 */
+	if (sbp->sb_agcount > 10000) {
+		error = xfs_read_agf(mp, NULL, sbp->sb_agcount - 1, 0, &bp);
+		if (error) {
+			fprintf(stderr, _("%s: read of AG %d failed\n"),
+						progname, sbp->sb_agcount);
+			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
+				return NULL;
+			fprintf(stderr, _("%s: limiting reads to AG 0\n"),
+								progname);
+			sbp->sb_agcount = 1;
+		}
+		if (bp)
+			libxfs_putbuf(bp);
+	}
+
 	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
 	if (error) {
 		fprintf(stderr, _("%s: perag init failed\n"),

Re: [PATCH v7 1/5] xfs_db: sanitize agcount on load

[Darrick J. Wong]

On Tue, Jan 24, 2017 at 04:52:59PM -0600, Eric Sandeen wrote:
> Before we get into libxfs_initialize_perag and try to blindly
> allocate a perag struct for every (possibly corrupted number of)
> AGs, see if we can read the last one.  If not, assume it's corrupt,
> and load only the first AG.
> 
> Do this only for an arbitrarily high-ish agcount, so that normal-ish
> geometry on a possibly truncated file or device will still do
> its best to make all readable AGs available.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index a08575a..ca5101e 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -817,6 +817,28 @@ libxfs_mount(
>  			return NULL;
>  	}
>  
> +	/*
> +	 * libxfs_initialize_perag will allocate a perag structure for each AG.
> +	 * If agcount is corrupted and insanely high, this will OOM the box.
> +	 * If the agount seems (arbitrarily) high, try to read what would be
> +	 * the last AG, and if that fails, just read the first one and let
> +	 * the user know what happened.
> +	 */
> +	if (sbp->sb_agcount > 10000) {

10,000 isn't all that high -- that's only 960K worth of perag structs.
Also,

<create 200gb /dev/mapper/moo>

# mkfs.xfs -f -b size=4096 -d agsize=4096b /dev/mapper/moo
meta-data=/dev/mapper/moo        isize=512    agcount=12800, agsize=4096 blks

Ok, admittedly I'm trolling here.  Maybe a better limit would be
1,000,000 AGs?  That's at least 2TB with the minimum AG size, and 100MB
of RAM.

(Really I'd say 10 million but I've been brainwashed by the people
fscking 16TB filesystems on embedded arm boxen with 256M of RAM...)

> +		error = xfs_read_agf(mp, NULL, sbp->sb_agcount - 1, 0, &bp);
> +		if (error) {

__read_buf sends back -EIO for any zero-byte pread, including reads past
the end of the device, which makes a media error looks the same as a
too-small device.  Also, if the AGF is present but garbage then we'll
get -EFSCORRUPTED here, right?

I think I like the idea of computing the AGF location and comparing to
the device size to guess that our geometry is crazy.

--D

> +			fprintf(stderr, _("%s: read of AG %d failed\n"),
> +						progname, sbp->sb_agcount);
> +			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
> +				return NULL;
> +			fprintf(stderr, _("%s: limiting reads to AG 0\n"),
> +								progname);
> +			sbp->sb_agcount = 1;
> +		}
> +		if (bp)
> +			libxfs_putbuf(bp);
> +	}
> +
>  	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
>  	if (error) {
>  		fprintf(stderr, _("%s: perag init failed\n"),
> 
> 
> --
> 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 v7 1/5] xfs_db: sanitize agcount on load

[Eric Sandeen]

On 1/24/17 6:21 PM, Darrick J. Wong wrote:
> On Tue, Jan 24, 2017 at 04:52:59PM -0600, Eric Sandeen wrote:
>> Before we get into libxfs_initialize_perag and try to blindly
>> allocate a perag struct for every (possibly corrupted number of)
>> AGs, see if we can read the last one.  If not, assume it's corrupt,
>> and load only the first AG.
>>
>> Do this only for an arbitrarily high-ish agcount, so that normal-ish
>> geometry on a possibly truncated file or device will still do
>> its best to make all readable AGs available.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/libxfs/init.c b/libxfs/init.c
>> index a08575a..ca5101e 100644
>> --- a/libxfs/init.c
>> +++ b/libxfs/init.c
>> @@ -817,6 +817,28 @@ libxfs_mount(
>>  			return NULL;
>>  	}
>>  
>> +	/*
>> +	 * libxfs_initialize_perag will allocate a perag structure for each AG.
>> +	 * If agcount is corrupted and insanely high, this will OOM the box.
>> +	 * If the agount seems (arbitrarily) high, try to read what would be
>> +	 * the last AG, and if that fails, just read the first one and let
>> +	 * the user know what happened.
>> +	 */
>> +	if (sbp->sb_agcount > 10000) {
> 
> 10,000 isn't all that high -- that's only 960K worth of perag structs.
> Also,

It's not a lot of memory but it's a lot of AGs.  *shrug* doesn't really
matter what the number is, I just wanted most common-case xfs_db
invocations to work even if for some reason we can't read the last AG,
due to a truncated image or whatever.  10,000 would be unusual.
If you want a million, fine by me.

> <create 200gb /dev/mapper/moo>
> 
> # mkfs.xfs -f -b size=4096 -d agsize=4096b /dev/mapper/moo
> meta-data=/dev/mapper/moo        isize=512    agcount=12800, agsize=4096 blks
> 
> Ok, admittedly I'm trolling here.  Maybe a better limit would be
> 1,000,000 AGs?  That's at least 2TB with the minimum AG size, and 100MB
> of RAM.
> 
> (Really I'd say 10 million but I've been brainwashed by the people
> fscking 16TB filesystems on embedded arm boxen with 256M of RAM...)

ok, one million it is.

>> +		error = xfs_read_agf(mp, NULL, sbp->sb_agcount - 1, 0, &bp);
>> +		if (error) {
> 
> __read_buf sends back -EIO for any zero-byte pread, including reads past
> the end of the device, which makes a media error looks the same as a
> too-small device.  Also, if the AGF is present but garbage then we'll
> get -EFSCORRUPTED here, right?

Oh, I guess so.  Could compare to -EIO?  This was the reason for only
taking this "media error" risk for a crazily large number of AGs, which
is /probably/ wrong in the first place.  Chances of /really/ having a
million AGs and then happening upon a media error on the millionth AG
seems pretty small.

> I think I like the idea of computing the AGF location and comparing to
> the device size to guess that our geometry is crazy.

Meh, ok, but maybe with some slop.  If agcount >= 1 million, /and/ last
AG > 2x device size, bail.  Howzat?

-Eric

> --D
> 
>> +			fprintf(stderr, _("%s: read of AG %d failed\n"),
>> +						progname, sbp->sb_agcount);
>> +			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
>> +				return NULL;
>> +			fprintf(stderr, _("%s: limiting reads to AG 0\n"),
>> +								progname);
>> +			sbp->sb_agcount = 1;
>> +		}
>> +		if (bp)
>> +			libxfs_putbuf(bp);
>> +	}
>> +
>>  	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
>>  	if (error) {
>>  		fprintf(stderr, _("%s: perag init failed\n"),
>>
>>
>> --
>> 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 v8 1/5] xfs_db: sanitize agcount on load

[Eric Sandeen]

Before we get into libxfs_initialize_perag and try to blindly
allocate a perag struct for every (possibly corrupted number of)
AGs, see if we can read the last one.  If not, assume it's corrupt,
and load only the first AG.

Do this only for an arbitrarily high-ish agcount, so that normal-ish
geometry on a possibly truncated file or device will still do
its best to make all readable AGs available.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

v7: blow it all up
v8: use bare libxfs_readbuf so verifiers don't matter,
    "ours goes to 1 million!"

diff --git a/libxfs/init.c b/libxfs/init.c
index a08575a..a14aa17 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -817,6 +817,29 @@ libxfs_mount(
 			return NULL;
 	}
 
+	/*
+	 * libxfs_initialize_perag will allocate a perag structure for each ag.
+	 * If agcount is corrupted and insanely high, this will OOM the box.
+	 * If the agount seems (arbitrarily) high, try to read what would be
+	 * the last AG, and if that fails for a relatively high agcount, just
+	 * read the first one and let the user know to check the geometry.
+	 */
+	if (sbp->sb_agcount > 1000000) {
+		bp = libxfs_readbuf(mp->m_dev,
+				XFS_AG_DADDR(mp, sbp->sb_agcount - 1, 0), 1,
+				!(flags & LIBXFS_MOUNT_DEBUGGER), NULL);
+		if (bp->b_error) {
+			fprintf(stderr, _("%s: read of AG %d failed\n"),
+						progname, sbp->sb_agcount);
+			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
+				return NULL;
+			fprintf(stderr, _("%s: limiting reads to AG 0\n"),
+								progname);
+			sbp->sb_agcount = 1;
+		}
+		libxfs_putbuf(bp);
+	}
+
 	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
 	if (error) {
 		fprintf(stderr, _("%s: perag init failed\n"),

Re: [PATCH v8 1/5] xfs_db: sanitize agcount on load

[Darrick J. Wong]

On Tue, Jan 24, 2017 at 09:09:01PM -0600, Eric Sandeen wrote:
> Before we get into libxfs_initialize_perag and try to blindly
> allocate a perag struct for every (possibly corrupted number of)
> AGs, see if we can read the last one.  If not, assume it's corrupt,
> and load only the first AG.
> 
> Do this only for an arbitrarily high-ish agcount, so that normal-ish
> geometry on a possibly truncated file or device will still do
> its best to make all readable AGs available.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Huh, what happened to v1-6? :)

> v7: blow it all up
> v8: use bare libxfs_readbuf so verifiers don't matter,
>     "ours goes to 1 million!"
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index a08575a..a14aa17 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -817,6 +817,29 @@ libxfs_mount(
>  			return NULL;
>  	}
>  
> +	/*
> +	 * libxfs_initialize_perag will allocate a perag structure for each ag.
> +	 * If agcount is corrupted and insanely high, this will OOM the box.
> +	 * If the agount seems (arbitrarily) high, try to read what would be
> +	 * the last AG, and if that fails for a relatively high agcount, just
> +	 * read the first one and let the user know to check the geometry.
> +	 */
> +	if (sbp->sb_agcount > 1000000) {
> +		bp = libxfs_readbuf(mp->m_dev,
> +				XFS_AG_DADDR(mp, sbp->sb_agcount - 1, 0), 1,
> +				!(flags & LIBXFS_MOUNT_DEBUGGER), NULL);
> +		if (bp->b_error) {
> +			fprintf(stderr, _("%s: read of AG %d failed\n"),
> +						progname, sbp->sb_agcount);

AG counts are unsigned.

# xfs_db -x -c 'sb 0' -c 'fuzz -d agcount random' /dev/sdf
Allowing fuzz of corrupted data with good CRC
agcount = 3298133433
# xfs_db /dev/sdf
xfs_db: error - read only 0 of 512 bytes
xfs_db: read of AG -996833863 failed
xfs_db: limiting reads to AG 0

Assuming it passes testing, fix the two things, slap on a:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

and let's call it done.

--D

> +			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
> +				return NULL;
> +			fprintf(stderr, _("%s: limiting reads to AG 0\n"),
> +								progname);
> +			sbp->sb_agcount = 1;
> +		}
> +		libxfs_putbuf(bp);
> +	}
> +
>  	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
>  	if (error) {
>  		fprintf(stderr, _("%s: perag init failed\n"),
> 
> --
> 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 v9 1/5] xfs_db: sanitize agcount on load

[Eric Sandeen]

Before we get into libxfs_initialize_perag and try to blindly
allocate a perag struct for every (possibly corrupted number of)
AGs, see if we can read the last one.  If not, assume it's corrupt,
and load only the first AG.

Do this only for an arbitrarily high-ish agcount, so that normal-ish
geometry on a possibly truncated file or device will still do
its best to make all readable AGs available.

Teach metadump to detect this and exit appropriately if truncated.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

v1->v6: Tale of woe.
v7: blow it all up
v8: use bare libxfs_readbuf so verifiers don't matter,
    "ours goes to 1 million!"
v9: Fix printf format, exit metadump with error if things look wonky.

diff --git a/db/metadump.c b/db/metadump.c
index 1ba6b38..d307771 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2760,6 +2760,16 @@ metadump_f(
 		return 0;
 	}
 
+	/*
+	 * on load, we sanity-checked agcount and possibly set to 1
+	 * if it was corrupted and large.
+	 */
+	if (mp->m_sb.sb_agcount == 1 &&
+	    XFS_MAX_DBLOCKS(&mp->m_sb) < mp->m_sb.sb_dblocks) {
+		print_warning("truncated agcount, giving up");
+		return 0;
+	}
+
 	while ((c = getopt(argc, argv, "aegm:ow")) != EOF) {
 		switch (c) {
 			case 'a':
diff --git a/libxfs/init.c b/libxfs/init.c
index a08575a..85e0d15 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -817,6 +817,29 @@ libxfs_mount(
 			return NULL;
 	}
 
+	/*
+	 * libxfs_initialize_perag will allocate a perag structure for each ag.
+	 * If agcount is corrupted and insanely high, this will OOM the box.
+	 * If the agount seems (arbitrarily) high, try to read what would be
+	 * the last AG, and if that fails for a relatively high agcount, just
+	 * read the first one and let the user know to check the geometry.
+	 */
+	if (sbp->sb_agcount > 1000000) {
+		bp = libxfs_readbuf(mp->m_dev,
+				XFS_AG_DADDR(mp, sbp->sb_agcount - 1, 0), 1,
+				!(flags & LIBXFS_MOUNT_DEBUGGER), NULL);
+		if (bp->b_error) {
+			fprintf(stderr, _("%s: read of AG %u failed\n"),
+						progname, sbp->sb_agcount);
+			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
+				return NULL;
+			fprintf(stderr, _("%s: limiting reads to AG 0\n"),
+								progname);
+			sbp->sb_agcount = 1;
+		}
+		libxfs_putbuf(bp);
+	}
+
 	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
 	if (error) {
 		fprintf(stderr, _("%s: perag init failed\n"),

[PATCH v10 1/5] xfs_db: sanitize agcount on load

[Eric Sandeen]

Before we get into libxfs_initialize_perag and try to blindly
allocate a perag struct for every (possibly corrupted number of)
AGs, see if we can read the last one.  If not, assume it's corrupt,
and load only the first AG.

Do this only for an arbitrarily high-ish agcount, so that normal-ish
geometry on a possibly truncated file or device will still do
its best to make all readable AGs available.

Set xfs_db's exitcode to 1 if this happens.

Also teach metadump to detect this and exit appropriately if
truncated, as it resets exitcode to 0 for its own purposes internally.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

v1->v6: Tale of woe.
v7: blow it all up
v8: use bare libxfs_readbuf so verifiers don't matter,
    "ours goes to 1 million!"
v9: Fix printf format, exit metadump with error if things look wonky.
v10: set exitcode to 1 in init().

diff --git a/db/init.c b/db/init.c
index ec1e274..59fc3e0 100644
--- a/db/init.c
+++ b/db/init.c
@@ -58,6 +58,7 @@ init(
 {
 	struct xfs_sb	*sbp;
 	struct xfs_buf	*bp;
+	unsigned int	agcount;
 	int		c;
 
 	setlocale(LC_ALL, "");
@@ -148,6 +149,7 @@ init(
 		}
 	}
 
+	agcount = sbp->sb_agcount;
 	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
 			  LIBXFS_MOUNT_DEBUGGER);
 	if (!mp) {
@@ -159,6 +161,10 @@ init(
 	mp->m_log = &xlog;
 	blkbb = 1 << mp->m_blkbb_log;
 
+	/* Did we limit a broken agcount in libxfs_mount? */
+	if (sbp->sb_agcount != agcount)
+		exitcode = 1;
+
 	/*
 	 * xfs_check needs corrected incore superblock values
 	 */
diff --git a/db/metadump.c b/db/metadump.c
index 1ba6b38..38519f1 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2760,6 +2760,16 @@ metadump_f(
 		return 0;
 	}
 
+	/*
+	 * on load, we sanity-checked agcount and possibly set to 1
+	 * if it was corrupted and large.
+	 */
+	if (mp->m_sb.sb_agcount == 1 &&
+	    XFS_MAX_DBLOCKS(&mp->m_sb) < mp->m_sb.sb_dblocks) {
+		print_warning("truncated agcount, giving up");
+		return 0;
+	}
+
 	while ((c = getopt(argc, argv, "aegm:ow")) != EOF) {
 		switch (c) {
 			case 'a':
diff --git a/libxfs/init.c b/libxfs/init.c
index a08575a..85e0d15 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -817,6 +817,29 @@ libxfs_mount(
 			return NULL;
 	}
 
+	/*
+	 * libxfs_initialize_perag will allocate a perag structure for each ag.
+	 * If agcount is corrupted and insanely high, this will OOM the box.
+	 * If the agount seems (arbitrarily) high, try to read what would be
+	 * the last AG, and if that fails for a relatively high agcount, just
+	 * read the first one and let the user know to check the geometry.
+	 */
+	if (sbp->sb_agcount > 1000000) {
+		bp = libxfs_readbuf(mp->m_dev,
+				XFS_AG_DADDR(mp, sbp->sb_agcount - 1, 0), 1,
+				!(flags & LIBXFS_MOUNT_DEBUGGER), NULL);
+		if (bp->b_error) {
+			fprintf(stderr, _("%s: read of AG %u failed\n"),
+						progname, sbp->sb_agcount);
+			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
+				return NULL;
+			fprintf(stderr, _("%s: limiting reads to AG 0\n"),
+								progname);
+			sbp->sb_agcount = 1;
+		}
+		libxfs_putbuf(bp);
+	}
+
 	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
 	if (error) {
 		fprintf(stderr, _("%s: perag init failed\n"),

Re: [PATCH v10 1/5] xfs_db: sanitize agcount on load

[Darrick J. Wong]

On Wed, Jan 25, 2017 at 07:17:56PM -0600, Eric Sandeen wrote:
> Before we get into libxfs_initialize_perag and try to blindly
> allocate a perag struct for every (possibly corrupted number of)
> AGs, see if we can read the last one.  If not, assume it's corrupt,
> and load only the first AG.
> 
> Do this only for an arbitrarily high-ish agcount, so that normal-ish
> geometry on a possibly truncated file or device will still do
> its best to make all readable AGs available.
> 
> Set xfs_db's exitcode to 1 if this happens.
> 
> Also teach metadump to detect this and exit appropriately if
> truncated, as it resets exitcode to 0 for its own purposes internally.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
> 
> v1->v6: Tale of woe.
> v7: blow it all up
> v8: use bare libxfs_readbuf so verifiers don't matter,
>     "ours goes to 1 million!"
> v9: Fix printf format, exit metadump with error if things look wonky.
> v10: set exitcode to 1 in init().
> 
> diff --git a/db/init.c b/db/init.c
> index ec1e274..59fc3e0 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -58,6 +58,7 @@ init(
>  {
>  	struct xfs_sb	*sbp;
>  	struct xfs_buf	*bp;
> +	unsigned int	agcount;
>  	int		c;
>  
>  	setlocale(LC_ALL, "");
> @@ -148,6 +149,7 @@ init(
>  		}
>  	}
>  
> +	agcount = sbp->sb_agcount;
>  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
>  			  LIBXFS_MOUNT_DEBUGGER);
>  	if (!mp) {
> @@ -159,6 +161,10 @@ init(
>  	mp->m_log = &xlog;
>  	blkbb = 1 << mp->m_blkbb_log;
>  
> +	/* Did we limit a broken agcount in libxfs_mount? */
> +	if (sbp->sb_agcount != agcount)
> +		exitcode = 1;
> +
>  	/*
>  	 * xfs_check needs corrected incore superblock values
>  	 */
> diff --git a/db/metadump.c b/db/metadump.c
> index 1ba6b38..38519f1 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -2760,6 +2760,16 @@ metadump_f(
>  		return 0;
>  	}
>  
> +	/*
> +	 * on load, we sanity-checked agcount and possibly set to 1
> +	 * if it was corrupted and large.
> +	 */
> +	if (mp->m_sb.sb_agcount == 1 &&
> +	    XFS_MAX_DBLOCKS(&mp->m_sb) < mp->m_sb.sb_dblocks) {
> +		print_warning("truncated agcount, giving up");
> +		return 0;
> +	}
> +
>  	while ((c = getopt(argc, argv, "aegm:ow")) != EOF) {
>  		switch (c) {
>  			case 'a':
> diff --git a/libxfs/init.c b/libxfs/init.c
> index a08575a..85e0d15 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -817,6 +817,29 @@ libxfs_mount(
>  			return NULL;
>  	}
>  
> +	/*
> +	 * libxfs_initialize_perag will allocate a perag structure for each ag.
> +	 * If agcount is corrupted and insanely high, this will OOM the box.
> +	 * If the agount seems (arbitrarily) high, try to read what would be
> +	 * the last AG, and if that fails for a relatively high agcount, just
> +	 * read the first one and let the user know to check the geometry.
> +	 */
> +	if (sbp->sb_agcount > 1000000) {
> +		bp = libxfs_readbuf(mp->m_dev,
> +				XFS_AG_DADDR(mp, sbp->sb_agcount - 1, 0), 1,
> +				!(flags & LIBXFS_MOUNT_DEBUGGER), NULL);
> +		if (bp->b_error) {
> +			fprintf(stderr, _("%s: read of AG %u failed\n"),
> +						progname, sbp->sb_agcount);
> +			if (!(flags & LIBXFS_MOUNT_DEBUGGER))
> +				return NULL;
> +			fprintf(stderr, _("%s: limiting reads to AG 0\n"),
> +								progname);
> +			sbp->sb_agcount = 1;
> +		}
> +		libxfs_putbuf(bp);
> +	}
> +
>  	error = libxfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
>  	if (error) {
>  		fprintf(stderr, _("%s: perag init failed\n"),
> 
> --
> 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 2/5] xfs_db: fix the 'source' command when passed as a -c option

[Darrick J. Wong]

The 'source' command is supposed to read commands out of a file and
execute them.  This works great when done from an interactive command
line, but it doesn't work at all when invoked from the command line
because we never actually do anything with the opened file.

So don't load stdin into the input stack when we're only executing
command line options, and use that to decide if source_f is executing
from the command line so that we can actually run the input loop.  We'll
use this for the per-field fuzzing xfstests.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/init.c  |    2 +-
 db/input.c |   29 +++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 3 deletions(-)


diff --git a/db/init.c b/db/init.c
index e60ac66..46af024 100644
--- a/db/init.c
+++ b/db/init.c
@@ -254,7 +254,6 @@ main(
 	char	**v;
 	int	start_iocur_sp;
 
-	pushfile(stdin);
 	init(argc, argv);
 	start_iocur_sp = iocur_sp;
 
@@ -269,6 +268,7 @@ main(
 		goto close_devices;
 	}
 
+	pushfile(stdin);
 	while (!done) {
 		if ((input = fetchline()) == NULL)
 			break;
diff --git a/db/input.c b/db/input.c
index 8f65190..7b12c97 100644
--- a/db/input.c
+++ b/db/input.c
@@ -145,6 +145,12 @@ get_prompt(void)
 	return prompt;
 }
 
+static bool
+interactive_input(void)
+{
+	return inputstacksize == 1 && inputstack[0] == stdin;
+}
+
 static char *
 fetchline_internal(void)
 {
@@ -156,7 +162,9 @@ fetchline_internal(void)
 
 	rval = NULL;
 	for (rlen = iscont = 0; ; ) {
-		if (inputstacksize == 1) {
+		if (curinput == NULL)
+			return NULL;
+		if (interactive_input()) {
 			if (iscont)
 				dbprintf("... ");
 			else
@@ -183,7 +191,8 @@ fetchline_internal(void)
 		    (len = strlen(buf)) == 0) {
 			popfile();
 			if (curinput == NULL) {
-				dbprintf("\n");
+				if (interactive_input())
+					dbprintf("\n");
 				return NULL;
 			}
 			iscont = 0;
@@ -314,12 +323,28 @@ source_f(
 	char	**argv)
 {
 	FILE	*f;
+	int	c, done = 0;
+	char	*input;
+	char	**v;
 
 	f = fopen(argv[1], "r");
 	if (f == NULL)
 		dbprintf(_("can't open %s\n"), argv[0]);
 	else
 		pushfile(f);
+
+	/* If this isn't an interactive shell, run the commands now. */
+	if (inputstacksize == 0 || inputstack[0] == stdin)
+		return 0;
+	while (!done) {
+		if ((input = fetchline_internal()) == NULL)
+			break;
+		v = breakline(input, &c);
+		if (c)
+			done = command(c, v);
+		doneline(input, v);
+	}
+
 	return 0;
 }
 

Re: [PATCH 2/5] xfs_db: fix the 'source' command when passed as a -c option

[Eric Sandeen]

On 1/20/17 2:25 PM, Darrick J. Wong wrote:
> The 'source' command is supposed to read commands out of a file and
> execute them.  This works great when done from an interactive command
> line, but it doesn't work at all when invoked from the command line
> because we never actually do anything with the opened file.
> 
> So don't load stdin into the input stack when we're only executing
> command line options, and use that to decide if source_f is executing
> from the command line so that we can actually run the input loop.  We'll
> use this for the per-field fuzzing xfstests.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  db/init.c  |    2 +-
>  db/input.c |   29 +++++++++++++++++++++++++++--
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/db/init.c b/db/init.c
> index e60ac66..46af024 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -254,7 +254,6 @@ main(
>  	char	**v;
>  	int	start_iocur_sp;
>  
> -	pushfile(stdin);
>  	init(argc, argv);
>  	start_iocur_sp = iocur_sp;
>  
> @@ -269,6 +268,7 @@ main(
>  		goto close_devices;
>  	}

Ok, so anything above this is commandline invocations.

Everything below is interactive...

> +	pushfile(stdin);
>  	while (!done) {
>  		if ((input = fetchline()) == NULL)
>  			break;
> diff --git a/db/input.c b/db/input.c
> index 8f65190..7b12c97 100644
> --- a/db/input.c
> +++ b/db/input.c
> @@ -145,6 +145,12 @@ get_prompt(void)
>  	return prompt;
>  }
>  
> +static bool
> +interactive_input(void)
> +{
> +	return inputstacksize == 1 && inputstack[0] == stdin;
> +}

how about just:

	return curinput == stdin?

> +
>  static char *
>  fetchline_internal(void)
>  {
> @@ -156,7 +162,9 @@ fetchline_internal(void)
>  
>  	rval = NULL;
>  	for (rlen = iscont = 0; ; ) {
> -		if (inputstacksize == 1) {
> +		if (curinput == NULL)
> +			return NULL;
> +		if (interactive_input()) {
>  			if (iscont)
>  				dbprintf("... ");
>  			else
> @@ -183,7 +191,8 @@ fetchline_internal(void)
>  		    (len = strlen(buf)) == 0) {
>  			popfile();
>  			if (curinput == NULL) {
> -				dbprintf("\n");
> +				if (interactive_input())
> +					dbprintf("\n");

I give up.  I see from testing that it is, but why is \n special here?

>  				return NULL;
>  			}
>  			iscont = 0;
> @@ -314,12 +323,28 @@ source_f(
>  	char	**argv)
>  {
>  	FILE	*f;
> +	int	c, done = 0;
> +	char	*input;
> +	char	**v;
>  
>  	f = fopen(argv[1], "r");
>  	if (f == NULL)
>  		dbprintf(_("can't open %s\n"), argv[0]);
>  	else
>  		pushfile(f);
> +
> +	/* If this isn't an interactive shell, run the commands now. */
> +	if (inputstacksize == 0 || inputstack[0] == stdin)

This confuses me a bit, partly from the comment.  We could be in
an interactive shell, but we've just issued a source command ...

If inputstacksize == 0, that means fopen failed and we didn't
do a pushfile, right?  Maybe best to just return in the failure
case above, and skip that test here?

And how can we ever be here w/ inputstack[0] == stdin?

I'm wondering if this isn't cleaner and still correct:

        f = fopen(argv[1], "r");
        if (f == NULL) {
                dbprintf(_("can't open %s\n"), argv[0]);
                return 0;
        }

        /* Run the sourced commands now */
        pushfile(f);
        while (!done) {
                if ((input = fetchline_internal()) == NULL)

> +		return 0;
> +	while (!done) {
> +		if ((input = fetchline_internal()) == NULL)
> +			break;
> +		v = breakline(input, &c);
> +		if (c)
> +			done = command(c, v);
> +		doneline(input, v);
> +	}
> +
>  	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  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/5] xfs_db: fix the 'source' command when passed as a -c option

[Darrick J. Wong]

On Mon, Jan 23, 2017 at 04:29:00PM -0600, Eric Sandeen wrote:
> On 1/20/17 2:25 PM, Darrick J. Wong wrote:
> > The 'source' command is supposed to read commands out of a file and
> > execute them.  This works great when done from an interactive command
> > line, but it doesn't work at all when invoked from the command line
> > because we never actually do anything with the opened file.
> > 
> > So don't load stdin into the input stack when we're only executing
> > command line options, and use that to decide if source_f is executing
> > from the command line so that we can actually run the input loop.  We'll
> > use this for the per-field fuzzing xfstests.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  db/init.c  |    2 +-
> >  db/input.c |   29 +++++++++++++++++++++++++++--
> >  2 files changed, 28 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/db/init.c b/db/init.c
> > index e60ac66..46af024 100644
> > --- a/db/init.c
> > +++ b/db/init.c
> > @@ -254,7 +254,6 @@ main(
> >  	char	**v;
> >  	int	start_iocur_sp;
> >  
> > -	pushfile(stdin);
> >  	init(argc, argv);
> >  	start_iocur_sp = iocur_sp;
> >  
> > @@ -269,6 +268,7 @@ main(
> >  		goto close_devices;
> >  	}
> 
> Ok, so anything above this is commandline invocations.
> 
> Everything below is interactive...
> 
> > +	pushfile(stdin);
> >  	while (!done) {
> >  		if ((input = fetchline()) == NULL)
> >  			break;
> > diff --git a/db/input.c b/db/input.c
> > index 8f65190..7b12c97 100644
> > --- a/db/input.c
> > +++ b/db/input.c
> > @@ -145,6 +145,12 @@ get_prompt(void)
> >  	return prompt;
> >  }
> >  
> > +static bool
> > +interactive_input(void)
> > +{
> > +	return inputstacksize == 1 && inputstack[0] == stdin;
> > +}
> 
> how about just:
> 
> 	return curinput == stdin?

Sure.

> > +
> >  static char *
> >  fetchline_internal(void)
> >  {
> > @@ -156,7 +162,9 @@ fetchline_internal(void)
> >  
> >  	rval = NULL;
> >  	for (rlen = iscont = 0; ; ) {
> > -		if (inputstacksize == 1) {
> > +		if (curinput == NULL)
> > +			return NULL;
> > +		if (interactive_input()) {
> >  			if (iscont)
> >  				dbprintf("... ");
> >  			else
> > @@ -183,7 +191,8 @@ fetchline_internal(void)
> >  		    (len = strlen(buf)) == 0) {
> >  			popfile();
> >  			if (curinput == NULL) {
> > -				dbprintf("\n");
> > +				if (interactive_input())
> > +					dbprintf("\n");
> 
> I give up.  I see from testing that it is, but why is \n special here?

When we have an interactive session, no libreadline, and the user hits
^D, this makes it so that we print a newline before the prompt returns
so that we don't end up with:

xfs_db> bash-2.05$

However, now that we've moved pushfile(stdin), it's possible that a
non-interactive session (which is what you get with "-c 'source moo'")
will pop all the fds off the stack, causing the newline to print.

A better fix for all this is to:

if (curinput == stdin)
	dbprintf("\n");
popfile();
iscont = 0...

Another odd quirk I noticed is that when an invocation of
fetchline_internal reaches feof/ferror on a file descriptor, it'll pop
the input stack and try to continue with the next lower level.  Since
each source_f starts its own interpreter loop we could just jump out
when we run out of input data and let the /caller/ continue with the fd
that it pushed onto the stack.

> 
> >  				return NULL;
> >  			}
> >  			iscont = 0;
> > @@ -314,12 +323,28 @@ source_f(
> >  	char	**argv)
> >  {
> >  	FILE	*f;
> > +	int	c, done = 0;
> > +	char	*input;
> > +	char	**v;
> >  
> >  	f = fopen(argv[1], "r");
> >  	if (f == NULL)
> >  		dbprintf(_("can't open %s\n"), argv[0]);
> >  	else
> >  		pushfile(f);
> > +
> > +	/* If this isn't an interactive shell, run the commands now. */
> > +	if (inputstacksize == 0 || inputstack[0] == stdin)
> 
> This confuses me a bit, partly from the comment.  We could be in
> an interactive shell, but we've just issued a source command ...
> 
> If inputstacksize == 0, that means fopen failed and we didn't
> do a pushfile, right?  Maybe best to just return in the failure
> case above, and skip that test here?
> 
> And how can we ever be here w/ inputstack[0] == stdin?
> 
> I'm wondering if this isn't cleaner and still correct:
> 
>         f = fopen(argv[1], "r");
>         if (f == NULL) {
>                 dbprintf(_("can't open %s\n"), argv[0]);
>                 return 0;
>         }
> 
>         /* Run the sourced commands now */
>         pushfile(f);
>         while (!done) {
>                 if ((input = fetchline_internal()) == NULL)

Yes, that does seem cleaner.

--D

> 
> > +		return 0;
> > +	while (!done) {
> > +		if ((input = fetchline_internal()) == NULL)
> > +			break;
> > +		v = breakline(input, &c);
> > +		if (c)
> > +			done = command(c, v);
> > +		doneline(input, v);
> > +	}
> > +
> >  	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  http://vger.kernel.org/majordomo-info.html
> > 

[PATCH v2 2/5] xfs_db: fix the 'source' command when passed as a -c option

[Darrick J. Wong]

The 'source' command is supposed to read commands out of a file and
execute them.  This works great when done from an interactive command
line, but it doesn't work at all when invoked from the command line
because we never actually do anything with the opened file.

So don't load stdin into the input stack when we're only executing
command line options, and use that to decide if source_f is executing
from the command line so that we can actually run the input loop.  We'll
use this for the per-field fuzzing xfstests.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: clean up some warts, handle recursive source'ing properly
---
 db/init.c  |    2 +-
 db/input.c |   43 +++++++++++++++++++++++++++++++++----------
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/db/init.c b/db/init.c
index 6ebe6b7..ee0d16e 100644
--- a/db/init.c
+++ b/db/init.c
@@ -237,7 +237,6 @@ main(
 	bool	insane_agcount;
 	int	start_iocur_sp;
 
-	pushfile(stdin);
 	init(argc, argv, &insane_agcount);
 	start_iocur_sp = iocur_sp;
 
@@ -252,6 +251,7 @@ main(
 		goto close_devices;
 	}
 
+	pushfile(stdin);
 	while (!done) {
 		if ((input = fetchline()) == NULL)
 			break;
diff --git a/db/input.c b/db/input.c
index 8f65190..0cb77bb 100644
--- a/db/input.c
+++ b/db/input.c
@@ -156,7 +156,7 @@ fetchline_internal(void)
 
 	rval = NULL;
 	for (rlen = iscont = 0; ; ) {
-		if (inputstacksize == 1) {
+		if (curinput == stdin) {
 			if (iscont)
 				dbprintf("... ");
 			else
@@ -181,18 +181,24 @@ fetchline_internal(void)
 		}
 		if (ferror(curinput) || feof(curinput) ||
 		    (len = strlen(buf)) == 0) {
-			popfile();
-			if (curinput == NULL) {
+			/*
+			 * No more input at this inputstack level; pop
+			 * our fd off and return so that a lower
+			 * level fetchline can handle us.  If this was
+			 * an interactive session, print a newline
+			 * because ^D doesn't emit one.
+			 */
+			if (curinput == stdin)
 				dbprintf("\n");
-				return NULL;
-			}
+
+			popfile();
 			iscont = 0;
 			rlen = 0;
 			if (rval) {
 				xfree(rval);
 				rval = NULL;
 			}
-			continue;
+			return NULL;
 		}
 		if (inputstacksize == 1)
 			logprintf("%s", buf);
@@ -225,7 +231,9 @@ fetchline(void)
 
 	if (inputstacksize == 1) {
 		line = readline(get_prompt());
-		if (line && *line) {
+		if (!line)
+			dbprintf("\n");
+		else if (line && *line) {
 			add_history(line);
 			logprintf("%s", line);
 		}
@@ -314,12 +322,27 @@ source_f(
 	char	**argv)
 {
 	FILE	*f;
+	int	c, done = 0;
+	char	*input;
+	char	**v;
 
 	f = fopen(argv[1], "r");
-	if (f == NULL)
+	if (f == NULL) {
 		dbprintf(_("can't open %s\n"), argv[0]);
-	else
-		pushfile(f);
+		return 0;
+	}
+
+	/* Run the sourced commands now. */
+	pushfile(f);
+	while (!done) {
+		if ((input = fetchline_internal()) == NULL)
+			break;
+		v = breakline(input, &c);
+		if (c)
+			done = command(c, v);
+		doneline(input, v);
+	}
+
 	return 0;
 }
 

[PATCH 3/5] xfs_repair: strengthen geometry checks

[Darrick J. Wong]

In xfs_repair, the inodelog, sectlog, and dirblklog values are read
directly into the xfs_mount structure without any sanity checking by the
verifier.  This results in xfs_repair segfaulting when those fields have
ridiculously high values because the pointer arithmetic runs us off the
end of the metadata buffers.  Therefore, reject the superblock if these
values are garbage and try to find one of the other ones.

Also clean up the dblocks checking to use the relevant macros and remove
a bogus ASSERT that crashes repair when sunit is set but swidth isn't.

The superblock field fuzzer (xfs/1301) triggers all these segfaults.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: make the inode geometry more consistent with the kernel sb verifier,
and prevent an ASSERT if the fs is !dalign but sunit != 0.
---
 repair/sb.c         |   24 +++++++++++++++---------
 repair/xfs_repair.c |    4 ++++
 2 files changed, 19 insertions(+), 9 deletions(-)


diff --git a/repair/sb.c b/repair/sb.c
index 004702c..06b034d 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -395,20 +395,22 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
 	/* sanity check ag count, size fields against data size field */
 
 	if (sb->sb_dblocks == 0 ||
-		sb->sb_dblocks >
-			((__uint64_t)sb->sb_agcount * sb->sb_agblocks) ||
-		sb->sb_dblocks <
-			((__uint64_t)(sb->sb_agcount - 1) * sb->sb_agblocks
-			+ XFS_MIN_AG_BLOCKS))
+		sb->sb_dblocks > XFS_MAX_DBLOCKS(sb) ||
+		sb->sb_dblocks < XFS_MIN_DBLOCKS(sb))
 		return(XR_BAD_FS_SIZE_DATA);
 
 	if (sb->sb_agblklog != (__uint8_t)libxfs_log2_roundup(sb->sb_agblocks))
 		return(XR_BAD_FS_SIZE_DATA);
 
-	if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE ||
-		sb->sb_inodesize > XFS_DINODE_MAX_SIZE ||
-		sb->sb_inopblock != howmany(sb->sb_blocksize,sb->sb_inodesize))
-		return(XR_BAD_INO_SIZE_DATA);
+	if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE                     ||
+	    sb->sb_inodesize > XFS_DINODE_MAX_SIZE                     ||
+	    sb->sb_inodelog < XFS_DINODE_MIN_LOG                       ||
+	    sb->sb_inodelog > XFS_DINODE_MAX_LOG                       ||
+	    sb->sb_inodesize != (1 << sb->sb_inodelog)                 ||
+	    sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE                    ||
+	    sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) ||
+	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog))
+		return XR_BAD_INO_SIZE_DATA;
 
 	if (xfs_sb_version_hassector(sb))  {
 
@@ -494,6 +496,10 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
 			return(XR_BAD_SB_WIDTH);
 	}
 
+	/* Directory block log */
+	if (sb->sb_dirblklog > XFS_MAX_BLOCKSIZE_LOG)
+		return XR_BAD_FS_SIZE_DATA;
+
 	return(XR_OK);
 }
 
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 5c79fd9..622d569 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -614,6 +614,10 @@ is_multidisk_filesystem(
 	if (sbp->sb_agcount >= XFS_MULTIDISK_AGCOUNT)
 		return true;
 
+	/* sunit/swidth are only useful if fs supports dalign. */
+	if (!xfs_sb_version_hasdalign(sbp))
+		return false;
+
 	/*
 	 * If it doesn't have a sunit/swidth, mkfs didn't consider it a
 	 * multi-disk array, so we don't either.

Re: [PATCH 3/5] xfs_repair: strengthen geometry checks

[Eric Sandeen]

On 1/20/17 2:25 PM, Darrick J. Wong wrote:
> In xfs_repair, the inodelog, sectlog, and dirblklog values are read
> directly into the xfs_mount structure without any sanity checking by the
> verifier.  This results in xfs_repair segfaulting when those fields have
> ridiculously high values because the pointer arithmetic runs us off the
> end of the metadata buffers.  Therefore, reject the superblock if these
> values are garbage and try to find one of the other ones.
> 
> Also clean up the dblocks checking to use the relevant macros and remove
> a bogus ASSERT that crashes repair when sunit is set but swidth isn't.

( ... no ASSERT is removed in this patch ...)

> The superblock field fuzzer (xfs/1301) triggers all these segfaults.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: make the inode geometry more consistent with the kernel sb verifier,
> and prevent an ASSERT if the fs is !dalign but sunit != 0.
> ---
>  repair/sb.c         |   24 +++++++++++++++---------
>  repair/xfs_repair.c |    4 ++++
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/repair/sb.c b/repair/sb.c
> index 004702c..06b034d 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -395,20 +395,22 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
>  	/* sanity check ag count, size fields against data size field */
>  
>  	if (sb->sb_dblocks == 0 ||
> -		sb->sb_dblocks >
> -			((__uint64_t)sb->sb_agcount * sb->sb_agblocks) ||
> -		sb->sb_dblocks <
> -			((__uint64_t)(sb->sb_agcount - 1) * sb->sb_agblocks
> -			+ XFS_MIN_AG_BLOCKS))
> +		sb->sb_dblocks > XFS_MAX_DBLOCKS(sb) ||
> +		sb->sb_dblocks < XFS_MIN_DBLOCKS(sb))

Ok, again this just uses macros in place of open coding, cool.

>  		return(XR_BAD_FS_SIZE_DATA);
>  
>  	if (sb->sb_agblklog != (__uint8_t)libxfs_log2_roundup(sb->sb_agblocks))
>  		return(XR_BAD_FS_SIZE_DATA);
>  
> -	if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE ||
> -		sb->sb_inodesize > XFS_DINODE_MAX_SIZE ||
> -		sb->sb_inopblock != howmany(sb->sb_blocksize,sb->sb_inodesize))
> -		return(XR_BAD_INO_SIZE_DATA);
> +	if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE                     ||
> +	    sb->sb_inodesize > XFS_DINODE_MAX_SIZE                     ||
> +	    sb->sb_inodelog < XFS_DINODE_MIN_LOG                       ||
> +	    sb->sb_inodelog > XFS_DINODE_MAX_LOG                       ||
> +	    sb->sb_inodesize != (1 << sb->sb_inodelog)                 ||
> +	    sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE                    ||
> +	    sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) ||
> +	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog))
> +		return XR_BAD_INO_SIZE_DATA;

This adds more checks: inodelog, logsunit, and blocklog tests, also cool.
  
>  	if (xfs_sb_version_hassector(sb))  {
>  
> @@ -494,6 +496,10 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
>  			return(XR_BAD_SB_WIDTH);
>  	}
>  
> +	/* Directory block log */
> +	if (sb->sb_dirblklog > XFS_MAX_BLOCKSIZE_LOG)
> +		return XR_BAD_FS_SIZE_DATA;
> +

Sorry for not catching this before, but -

Hm, FS_SIZE_DATA ("inconsistent filesystem geometry information") doesn't
seem quite right for this case, it's used for AG problems elsewhere.

I ... guess a new XR_BAD_DIR_SIZE_DATA or something might be a good idea.
It's only informative, but may as well be correctly informative.

>  	return(XR_OK);
>  }
>  
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 5c79fd9..622d569 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -614,6 +614,10 @@ is_multidisk_filesystem(
>  	if (sbp->sb_agcount >= XFS_MULTIDISK_AGCOUNT)
>  		return true;

And now for something completely different :)

> +	/* sunit/swidth are only useful if fs supports dalign. */
> +	if (!xfs_sb_version_hasdalign(sbp))
> +		return false;

I'm still bothered that this is a bit of a special snowflake here.
Why isn't this part of superblock sanity checking?  Thinking
it through...

Outcomes for various correct/incorrect sets of values:

has_dalign	sb_unit	set	sb_width set
0		0		0		OK
0		0		1		invalid [1]
0		1		0		ASSERT (earlier)
0		1		1		invalid [1]
1		0		0		OK (huh?)
1		0		1		invalid [2]
1		1		0		invalid [2]
1		1		1		OK


[1] invalid in secondary_sb_whack (!?), fixes by zeroing both
[2] invalid in verify_sb, fixes by using secondaries

Ok, so this ASSERT is going off because verify_sb didn't
catch it or care, even though a later secondary_sb_whack()
does catch it and fixes it, but we have this ASSERT in between
the two.

At the end of the day, what is_multidisk_filesystem() returns
doesn't impact correctness at all, just performance of xfs_repair.

The ASSERT was essentially coding superblock consistency checks
into this performance-tweaking function.  :/

So I guess the question is: If the superblock stripe geometry
is inconsistent by the time we get to is_multidisk_filesystem(),
what should we do?

1) catch it earlier in verify_sb, and copy from backup?
2) keep that as it is, but only return true here if all 3 are set?
3) return true for some subset of fields set, even if inconsistent,
   as this does? :)

-Eric

Re: [PATCH 3/5] xfs_repair: strengthen geometry checks

[Darrick J. Wong]

On Mon, Jan 23, 2017 at 05:47:20PM -0600, Eric Sandeen wrote:
> On 1/20/17 2:25 PM, Darrick J. Wong wrote:
> > In xfs_repair, the inodelog, sectlog, and dirblklog values are read
> > directly into the xfs_mount structure without any sanity checking by the
> > verifier.  This results in xfs_repair segfaulting when those fields have
> > ridiculously high values because the pointer arithmetic runs us off the
> > end of the metadata buffers.  Therefore, reject the superblock if these
> > values are garbage and try to find one of the other ones.
> > 
> > Also clean up the dblocks checking to use the relevant macros and remove
> > a bogus ASSERT that crashes repair when sunit is set but swidth isn't.
> 
> ( ... no ASSERT is removed in this patch ...)
> 
> > The superblock field fuzzer (xfs/1301) triggers all these segfaults.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: make the inode geometry more consistent with the kernel sb verifier,
> > and prevent an ASSERT if the fs is !dalign but sunit != 0.
> > ---
> >  repair/sb.c         |   24 +++++++++++++++---------
> >  repair/xfs_repair.c |    4 ++++
> >  2 files changed, 19 insertions(+), 9 deletions(-)
> > 
> > 
> > diff --git a/repair/sb.c b/repair/sb.c
> > index 004702c..06b034d 100644
> > --- a/repair/sb.c
> > +++ b/repair/sb.c
> > @@ -395,20 +395,22 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
> >  	/* sanity check ag count, size fields against data size field */
> >  
> >  	if (sb->sb_dblocks == 0 ||
> > -		sb->sb_dblocks >
> > -			((__uint64_t)sb->sb_agcount * sb->sb_agblocks) ||
> > -		sb->sb_dblocks <
> > -			((__uint64_t)(sb->sb_agcount - 1) * sb->sb_agblocks
> > -			+ XFS_MIN_AG_BLOCKS))
> > +		sb->sb_dblocks > XFS_MAX_DBLOCKS(sb) ||
> > +		sb->sb_dblocks < XFS_MIN_DBLOCKS(sb))
> 
> Ok, again this just uses macros in place of open coding, cool.
> 
> >  		return(XR_BAD_FS_SIZE_DATA);
> >  
> >  	if (sb->sb_agblklog != (__uint8_t)libxfs_log2_roundup(sb->sb_agblocks))
> >  		return(XR_BAD_FS_SIZE_DATA);
> >  
> > -	if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE ||
> > -		sb->sb_inodesize > XFS_DINODE_MAX_SIZE ||
> > -		sb->sb_inopblock != howmany(sb->sb_blocksize,sb->sb_inodesize))
> > -		return(XR_BAD_INO_SIZE_DATA);
> > +	if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE                     ||
> > +	    sb->sb_inodesize > XFS_DINODE_MAX_SIZE                     ||
> > +	    sb->sb_inodelog < XFS_DINODE_MIN_LOG                       ||
> > +	    sb->sb_inodelog > XFS_DINODE_MAX_LOG                       ||
> > +	    sb->sb_inodesize != (1 << sb->sb_inodelog)                 ||
> > +	    sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE                    ||
> > +	    sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) ||
> > +	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog))
> > +		return XR_BAD_INO_SIZE_DATA;
> 
> This adds more checks: inodelog, logsunit, and blocklog tests, also cool.
>   
> >  	if (xfs_sb_version_hassector(sb))  {
> >  
> > @@ -494,6 +496,10 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
> >  			return(XR_BAD_SB_WIDTH);
> >  	}
> >  
> > +	/* Directory block log */
> > +	if (sb->sb_dirblklog > XFS_MAX_BLOCKSIZE_LOG)
> > +		return XR_BAD_FS_SIZE_DATA;
> > +
> 
> Sorry for not catching this before, but -
> 
> Hm, FS_SIZE_DATA ("inconsistent filesystem geometry information") doesn't
> seem quite right for this case, it's used for AG problems elsewhere.
> 
> I ... guess a new XR_BAD_DIR_SIZE_DATA or something might be a good idea.
> It's only informative, but may as well be correctly informative.

Ok.

> >  	return(XR_OK);
> >  }
> >  
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 5c79fd9..622d569 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -614,6 +614,10 @@ is_multidisk_filesystem(
> >  	if (sbp->sb_agcount >= XFS_MULTIDISK_AGCOUNT)
> >  		return true;
> 
> And now for something completely different :)
> 
> > +	/* sunit/swidth are only useful if fs supports dalign. */
> > +	if (!xfs_sb_version_hasdalign(sbp))
> > +		return false;
> 
> I'm still bothered that this is a bit of a special snowflake here.
> Why isn't this part of superblock sanity checking?  Thinking
> it through...
> 
> Outcomes for various correct/incorrect sets of values:
> 
> has_dalign	sb_unit	set	sb_width set
> 0		0		0		OK
> 0		0		1		invalid [1]
> 0		1		0		ASSERT (earlier)
> 0		1		1		invalid [1]
> 1		0		0		OK (huh?)
> 1		0		1		invalid [2]
> 1		1		0		invalid [2]
> 1		1		1		OK
> 
> 
> [1] invalid in secondary_sb_whack (!?), fixes by zeroing both
> [2] invalid in verify_sb, fixes by using secondaries
> 
> Ok, so this ASSERT is going off because verify_sb didn't
> catch it or care, even though a later secondary_sb_whack()
> does catch it and fixes it, but we have this ASSERT in between
> the two.
> 
> At the end of the day, what is_multidisk_filesystem() returns
> doesn't impact correctness at all, just performance of xfs_repair.
> 
> The ASSERT was essentially coding superblock consistency checks
> into this performance-tweaking function.  :/
> 
> So I guess the question is: If the superblock stripe geometry
> is inconsistent by the time we get to is_multidisk_filesystem(),
> what should we do?
> 
> 1) catch it earlier in verify_sb, and copy from backup?
> 2) keep that as it is, but only return true here if all 3 are set?

I'm ok with running in slow mode if the stripe data seems garbage.
TBH I'd prefer to have as few asserts in the repair program as possible,
particularly since this is a performance optimization.

return xfs_sb_version_hasdalign(sbp) && sbp->sb_unit && sbp->s_width;

--D

> 3) return true for some subset of fields set, even if inconsistent,
>    as this does? :)
> 
> -Eric
> --
> 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/5] xfs_repair: strengthen geometry checks

[Eric Sandeen]

On 1/23/17 6:13 PM, Darrick J. Wong wrote:
>> So I guess the question is: If the superblock stripe geometry
>> is inconsistent by the time we get to is_multidisk_filesystem(),
>> what should we do?
>>
>> 1) catch it earlier in verify_sb, and copy from backup?
>> 2) keep that as it is, but only return true here if all 3 are set?
> I'm ok with running in slow mode if the stripe data seems garbage.
> TBH I'd prefer to have as few asserts in the repair program as possible,
> particularly since this is a performance optimization.

yeah.

> return xfs_sb_version_hasdalign(sbp) && sbp->sb_unit && sbp->s_width;

Backing up, I guess it's really not all that important in this
function, as long as we don't ASSERT.  I was actually going to suggest
returning true if all 3 were set, yes.

It just seems that this all should have been consistent by the time
we get here, having already gone through superblock verification
routines.

Might make sense to add it, but I guess that may as well be another
patch.

-Eric

[PATCH v2 3/5] xfs_repair: strengthen geometry checks

[Darrick J. Wong]

In xfs_repair, the inodelog, sectlog, and dirblklog values are read
directly into the xfs_mount structure without any sanity checking by the
verifier.  This results in xfs_repair segfaulting when those fields have
ridiculously high values because the pointer arithmetic runs us off the
end of the metadata buffers.  Therefore, reject the superblock if these
values are garbage and try to find one of the other ones.  Clean up the
dblocks checking to use the relevant macros.

The superblock field fuzzer (xfs/1301) triggers all these segfaults.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: make the inode geometry more consistent with the kernel sb verifier,
and prevent an ASSERT if the fs is !dalign but sunit != 0.
v3: add a new failure message for directory geometry inconsistency and
complain if sunit/swidth are set and dalign is not.
---
 repair/globals.h    |    3 ++-
 repair/sb.c         |   27 +++++++++++++++++----------
 repair/xfs_repair.c |    2 ++
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/repair/globals.h b/repair/globals.h
index efd1d03..4085ccc 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -50,7 +50,8 @@
 #define XR_BAD_SB_WIDTH		18	/* bad stripe width */
 #define XR_BAD_SVN		19	/* bad shared version number */
 #define XR_BAD_CRC		20	/* Bad CRC */
-#define XR_BAD_ERR_CODE		21	/* Bad error code */
+#define XR_BAD_DIR_SIZE_DATA	21	/* Bad directory geometry */
+#define XR_BAD_ERR_CODE		22	/* Bad error code */
 
 /* XFS filesystem (il)legal values */
 
diff --git a/repair/sb.c b/repair/sb.c
index 004702c..77e5154 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -395,20 +395,22 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
 	/* sanity check ag count, size fields against data size field */
 
 	if (sb->sb_dblocks == 0 ||
-		sb->sb_dblocks >
-			((__uint64_t)sb->sb_agcount * sb->sb_agblocks) ||
-		sb->sb_dblocks <
-			((__uint64_t)(sb->sb_agcount - 1) * sb->sb_agblocks
-			+ XFS_MIN_AG_BLOCKS))
+		sb->sb_dblocks > XFS_MAX_DBLOCKS(sb) ||
+		sb->sb_dblocks < XFS_MIN_DBLOCKS(sb))
 		return(XR_BAD_FS_SIZE_DATA);
 
 	if (sb->sb_agblklog != (__uint8_t)libxfs_log2_roundup(sb->sb_agblocks))
 		return(XR_BAD_FS_SIZE_DATA);
 
-	if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE ||
-		sb->sb_inodesize > XFS_DINODE_MAX_SIZE ||
-		sb->sb_inopblock != howmany(sb->sb_blocksize,sb->sb_inodesize))
-		return(XR_BAD_INO_SIZE_DATA);
+	if (sb->sb_inodesize < XFS_DINODE_MIN_SIZE                     ||
+	    sb->sb_inodesize > XFS_DINODE_MAX_SIZE                     ||
+	    sb->sb_inodelog < XFS_DINODE_MIN_LOG                       ||
+	    sb->sb_inodelog > XFS_DINODE_MAX_LOG                       ||
+	    sb->sb_inodesize != (1 << sb->sb_inodelog)                 ||
+	    sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE                    ||
+	    sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) ||
+	    (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog))
+		return XR_BAD_INO_SIZE_DATA;
 
 	if (xfs_sb_version_hassector(sb))  {
 
@@ -492,7 +494,12 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
 		if ((sb->sb_unit && !sb->sb_width) ||
 		    (sb->sb_width && sb->sb_unit && sb->sb_width % sb->sb_unit))
 			return(XR_BAD_SB_WIDTH);
-	}
+	} else if (sb->sb_unit || sb->sb_width)
+		return XR_BAD_SB_WIDTH;
+
+	/* Directory block log */
+	if (sb->sb_blocklog + sb->sb_dirblklog > XFS_MAX_BLOCKSIZE_LOG)
+		return XR_BAD_DIR_SIZE_DATA;
 
 	return(XR_OK);
 }
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 5c79fd9..b07567b 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -143,6 +143,8 @@ err_string(int err_code)
 			_("bad shared version number in superblock");
 		err_message[XR_BAD_CRC] =
 			_("bad CRC in superblock");
+		err_message[XR_BAD_DIR_SIZE_DATA] =
+			_("inconsistent directory geometry information");
 		done = 1;
 	}
 

[PATCH 4/5] xfs_repair: zero shared_vn

[Darrick J. Wong]

Since shared_vn always has to be zero, zero it at the end of repair.

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


diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 622d569..ecfa6b4 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -1050,6 +1050,9 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
 			be32_to_cpu(dsb->sb_unit), be32_to_cpu(dsb->sb_width));
 	}
 
+	/* shared_vn is always zero. */
+	dsb->sb_shared_vn = 0;
+
 	libxfs_writebuf(sbp, 0);
 
 	/*

Re: [PATCH 4/5] xfs_repair: zero shared_vn

[Eric Sandeen]

On 1/20/17 2:25 PM, Darrick J. Wong wrote:
> Since shared_vn always has to be zero, zero it at the end of repair.

If it's not, shouldn't it be properly warned, fixed, exit-code
set, etc?

-Eric
 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/xfs_repair.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> 
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 622d569..ecfa6b4 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -1050,6 +1050,9 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
>  			be32_to_cpu(dsb->sb_unit), be32_to_cpu(dsb->sb_width));
>  	}
>  
> +	/* shared_vn is always zero. */
> +	dsb->sb_shared_vn = 0;
> +
>  	libxfs_writebuf(sbp, 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  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/5] xfs_repair: zero shared_vn

[Darrick J. Wong]

On Fri, Jan 20, 2017 at 04:20:27PM -0600, Eric Sandeen wrote:
> On 1/20/17 2:25 PM, Darrick J. Wong wrote:
> > Since shared_vn always has to be zero, zero it at the end of repair.
> 
> If it's not, shouldn't it be properly warned, fixed, exit-code
> set, etc?

Yeah, you're right, that's what we have phase 1 for.

--D

> 
> -Eric
>  
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  repair/xfs_repair.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > 
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 622d569..ecfa6b4 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -1050,6 +1050,9 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
> >  			be32_to_cpu(dsb->sb_unit), be32_to_cpu(dsb->sb_width));
> >  	}
> >  
> > +	/* shared_vn is always zero. */
> > +	dsb->sb_shared_vn = 0;
> > +
> >  	libxfs_writebuf(sbp, 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  http://vger.kernel.org/majordomo-info.html
> > 
> --
> 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/5] xfs_repair: zero shared_vn

[Darrick J. Wong]

Since shared_vn always has to be zero, zero it at the end of repair.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: reset shared_vn in phase 1 and tell the user about it, per sandeen
---
 repair/phase1.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/repair/phase1.c b/repair/phase1.c
index 126d0b3..ee5d3d7 100644
--- a/repair/phase1.c
+++ b/repair/phase1.c
@@ -138,6 +138,17 @@ _("Cannot disable lazy-counters on V5 fs\n"));
 		}
 	}
 
+	/* shared_vn should be zero */
+	if (sb->sb_shared_vn) {
+		if (!no_modify)  {
+			do_warn(_("resetting shared_vn to zero\n"));
+			sb->sb_shared_vn = 0;
+			primary_sb_modified = 1;
+		} else  {
+			do_warn(_("would reset shared_vn to zero\n"));
+		}
+	}
+
 	if (primary_sb_modified)  {
 		if (!no_modify)  {
 			do_warn(_("writing modified primary superblock\n"));

Re: [PATCH v2 4/5] xfs_repair: zero shared_vn

[Eric Sandeen]

On 1/20/17 4:52 PM, Darrick J. Wong wrote:
> Since shared_vn always has to be zero, zero it at the end of repair.
                                                 ^^^^^^^^^^^^^^^^^^^^

Or at the beginning ;)

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: reset shared_vn in phase 1 and tell the user about it, per sandeen
> ---
>  repair/phase1.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/repair/phase1.c b/repair/phase1.c
> index 126d0b3..ee5d3d7 100644
> --- a/repair/phase1.c
> +++ b/repair/phase1.c
> @@ -138,6 +138,17 @@ _("Cannot disable lazy-counters on V5 fs\n"));
>  		}
>  	}
>  
> +	/* shared_vn should be zero */
> +	if (sb->sb_shared_vn) {
> +		if (!no_modify)  {
> +			do_warn(_("resetting shared_vn to zero\n"));
> +			sb->sb_shared_vn = 0;
> +			primary_sb_modified = 1;
> +		} else  {
> +			do_warn(_("would reset shared_vn to zero\n"));
> +		}
> +	}
> +

No other test has the if/else on no modify, they simply say what the problem
is, "correcting" etc, but at the /end/ it says whether or not it's written out:

        if (primary_sb_modified)  {
                if (!no_modify)  {
                        do_warn(_("writing modified primary superblock\n"));
                        write_primary_sb(sb, sb->sb_sectsize);
                } else  {
                        do_warn(_("would write modified primary superblock\n"));
                }
        }

So I think just something like:

+	if (sb->sb_shared_vn) {
+		do_warn(_("superblock has invalid shared_vn, resetting\n"));
+		sb->sb_shared_vn = 0;
+		primary_sb_modified = 1;
+	}

would suffice.

-Eric

>  	if (primary_sb_modified)  {
>  		if (!no_modify)  {
>  			do_warn(_("writing modified primary superblock\n"));
> --
> 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 v2 4/5] xfs_repair: zero shared_vn

[Darrick J. Wong]

On Fri, Jan 20, 2017 at 05:08:37PM -0600, Eric Sandeen wrote:
> On 1/20/17 4:52 PM, Darrick J. Wong wrote:
> > Since shared_vn always has to be zero, zero it at the end of repair.
>                                                  ^^^^^^^^^^^^^^^^^^^^
> 
> Or at the beginning ;)

"...at the start of repair."

> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: reset shared_vn in phase 1 and tell the user about it, per sandeen
> > ---
> >  repair/phase1.c |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/repair/phase1.c b/repair/phase1.c
> > index 126d0b3..ee5d3d7 100644
> > --- a/repair/phase1.c
> > +++ b/repair/phase1.c
> > @@ -138,6 +138,17 @@ _("Cannot disable lazy-counters on V5 fs\n"));
> >  		}
> >  	}
> >  
> > +	/* shared_vn should be zero */
> > +	if (sb->sb_shared_vn) {
> > +		if (!no_modify)  {
> > +			do_warn(_("resetting shared_vn to zero\n"));
> > +			sb->sb_shared_vn = 0;
> > +			primary_sb_modified = 1;
> > +		} else  {
> > +			do_warn(_("would reset shared_vn to zero\n"));
> > +		}
> > +	}
> > +
> 
> No other test has the if/else on no modify, they simply say what the problem
> is, "correcting" etc, but at the /end/ it says whether or not it's written out:
> 
>         if (primary_sb_modified)  {
>                 if (!no_modify)  {
>                         do_warn(_("writing modified primary superblock\n"));
>                         write_primary_sb(sb, sb->sb_sectsize);
>                 } else  {
>                         do_warn(_("would write modified primary superblock\n"));
>                 }
>         }
> 

Many of the repairs in later phases /do/ use this paradigm, though none
in this function do that.  I don't really care one way or the other, so
I'll change it to:

> So I think just something like:
> 
> +	if (sb->sb_shared_vn) {
> +		do_warn(_("superblock has invalid shared_vn, resetting\n"));
> +		sb->sb_shared_vn = 0;
> +		primary_sb_modified = 1;
> +	}
> 
> would suffice.

--D

> 
> -Eric
> 
> >  	if (primary_sb_modified)  {
> >  		if (!no_modify)  {
> >  			do_warn(_("writing modified primary superblock\n"));
> > --
> > 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
> > 
> --
> 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 v3 4/5] xfs_repair: zero shared_vn

[Darrick J. Wong]

Since shared_vn always has to be zero, zero it at the start of repair.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: reset shared_vn in phase 1 and tell the user about it, per sandeen
v3: more suggestions by sandeen to make the new code more consistent
---
 repair/phase1.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/repair/phase1.c b/repair/phase1.c
index 126d0b3..9799883 100644
--- a/repair/phase1.c
+++ b/repair/phase1.c
@@ -138,6 +138,13 @@ _("Cannot disable lazy-counters on V5 fs\n"));
 		}
 	}
 
+	/* shared_vn should be zero */
+	if (sb->sb_shared_vn) {
+		do_warn(_("resetting shared_vn to zero\n"));
+		sb->sb_shared_vn = 0;
+		primary_sb_modified = 1;
+	}
+
 	if (primary_sb_modified)  {
 		if (!no_modify)  {
 			do_warn(_("writing modified primary superblock\n"));

Re: [PATCH v3 4/5] xfs_repair: zero shared_vn

[Eric Sandeen]

On 1/20/17 6:09 PM, Darrick J. Wong wrote:
> Since shared_vn always has to be zero, zero it at the start of repair.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
> v2: reset shared_vn in phase 1 and tell the user about it, per sandeen
> v3: more suggestions by sandeen to make the new code more consistent
> ---
>  repair/phase1.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/repair/phase1.c b/repair/phase1.c
> index 126d0b3..9799883 100644
> --- a/repair/phase1.c
> +++ b/repair/phase1.c
> @@ -138,6 +138,13 @@ _("Cannot disable lazy-counters on V5 fs\n"));
>  		}
>  	}
>  
> +	/* shared_vn should be zero */
> +	if (sb->sb_shared_vn) {
> +		do_warn(_("resetting shared_vn to zero\n"));
> +		sb->sb_shared_vn = 0;
> +		primary_sb_modified = 1;
> +	}
> +
>  	if (primary_sb_modified)  {
>  		if (!no_modify)  {
>  			do_warn(_("writing modified primary superblock\n"));
> --
> 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 5/5] xfs_repair: trash dirattr btrees that cycle to the root

[Darrick J. Wong]

If xfs_repair detects a dir/attr btree that cycles back to the root, the
tree should be cleared and/or rebuilt instead of simply aborting the
repair program.

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


diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index b855a10..37a821b 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -763,7 +763,12 @@ process_leaf_attr_level(xfs_mount_t	*mp,
 		 * 0 is the root block and no block
 		 * pointer can point to the root block of the btree
 		 */
-		ASSERT(da_bno != 0);
+		if (da_bno == 0) {
+			do_warn(
+	_("btree cycle detected in attribute fork for inode %" PRIu64 "\n"),
+				ino);
+			goto error_out;
+		}
 
 		if (dev_bno == NULLFSBLOCK) {
 			do_warn(

Re: [PATCH 5/5] xfs_repair: trash dirattr btrees that cycle to the root

[Eric Sandeen]

On 1/20/17 2:25 PM, Darrick J. Wong wrote:
> If xfs_repair detects a dir/attr btree that cycles back to the root, the
> tree should be cleared and/or rebuilt instead of simply aborting the
> repair program.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

So, while this seems ok, so:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

you can take this or leave it:

It'd be a bit nicer to just test da_bno outside the loop, I think;
the whole loop is

	do {
....
        } while (da_bno != 0);

so the only way da_bno can be zero here is if we entered the loop this way
after:

        da_bno = da_cursor->level[0].bno;

prior to entering it the first time:

        ino = da_cursor->ino;
        prev_bno = 0;

        do {
 
-Eric

> ---
>  repair/attr_repair.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> index b855a10..37a821b 100644
> --- a/repair/attr_repair.c
> +++ b/repair/attr_repair.c
> @@ -763,7 +763,12 @@ process_leaf_attr_level(xfs_mount_t	*mp,
>  		 * 0 is the root block and no block
>  		 * pointer can point to the root block of the btree
>  		 */
> -		ASSERT(da_bno != 0);
> +		if (da_bno == 0) {
> +			do_warn(
> +	_("btree cycle detected in attribute fork for inode %" PRIu64 "\n"),
> +				ino);
> +			goto error_out;
> +		}
>  
>  		if (dev_bno == NULLFSBLOCK) {
>  			do_warn(
> 
> --
> 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
>