[PATCH 0/5] xfsprogs: misc fixes

[PATCH 0/5] xfsprogs: misc fixes

[Darrick J. Wong]

Hi all,

Here's a rollup of all the xfsprogs bug fixes that I've accumulated
since the last patchbomb.

--D

[PATCH 1/5] xfs_io: fix the minimum arguments to the reflink command

[Darrick J. Wong]

The reflink command can reflink the entirety of two files if the
offsets and lengths are not specified... but we forgot to permit
that case.

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


diff --git a/io/reflink.c b/io/reflink.c
index a09e82d..d26cbdd 100644
--- a/io/reflink.c
+++ b/io/reflink.c
@@ -302,7 +302,7 @@ reflink_init(void)
 	reflink_cmd.name = "reflink";
 	reflink_cmd.altname = "rl";
 	reflink_cmd.cfunc = reflink_f;
-	reflink_cmd.argmin = 4;
+	reflink_cmd.argmin = 1;
 	reflink_cmd.argmax = -1;
 	reflink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
 	reflink_cmd.args =

Re: [PATCH 1/5] xfs_io: fix the minimum arguments to the reflink command

[Christoph Hellwig]

On Wed, Jan 11, 2017 at 07:06:34PM -0800, Darrick J. Wong wrote:
> The reflink command can reflink the entirety of two files if the
> offsets and lengths are not specified... but we forgot to permit
> that case.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 2/5] xfs_io: fix some documentation problems

[Darrick J. Wong]

Describe the numberless (i.e. "reflink the whole file") behavior
in the xfs_io help system and since the clone/dedupe ioctls were
promoted to the VFS before XFS reflink landed, mention those in
the manpage.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 io/reflink.c      |    4 ++--
 man/man8/xfs_io.8 |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)


diff --git a/io/reflink.c b/io/reflink.c
index d26cbdd..41cb884 100644
--- a/io/reflink.c
+++ b/io/reflink.c
@@ -306,9 +306,9 @@ reflink_init(void)
 	reflink_cmd.argmax = -1;
 	reflink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
 	reflink_cmd.args =
-_("infile src_off dst_off len");
+_("infile [src_off dst_off len]");
 	reflink_cmd.oneline =
-		_("reflinks a number of bytes at a specified offset");
+		_("reflinks an entire file, or a number of bytes at a specified offset");
 	reflink_cmd.help = reflink_help;
 
 	add_command(&reflink_cmd);
diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index 885df7f..f1da472 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -530,7 +530,7 @@ both data and holes are displayed together or performing a recusively display.
 .TP
 .BI "reflink  [ \-C ] [ \-q ] src_file [src_offset dst_offset length]"
 On filesystems that support the
-.B XFS_IOC_CLONE_RANGE
+.B FICLONERANGE
 or
 .B BTRFS_IOC_CLONE_RANGE
 ioctls, map
@@ -560,7 +560,7 @@ Do not print timing statistics at all.
 .TP
 .BI "dedupe  [ \-C ] [ \-q ] src_file src_offset dst_offset length"
 On filesystems that support the
-.B XFS_IOC_FILE_EXTENT_SAME
+.B FIDEDUPERANGE
 or
 .B BTRFS_IOC_FILE_EXTENT_SAME
 ioctls, map

Re: [PATCH 2/5] xfs_io: fix some documentation problems

[Christoph Hellwig]

On Wed, Jan 11, 2017 at 07:06:41PM -0800, Darrick J. Wong wrote:
> Describe the numberless (i.e. "reflink the whole file") behavior
> in the xfs_io help system and since the clone/dedupe ioctls were
> promoted to the VFS before XFS reflink landed, mention those in
> the manpage.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 3/5] xfs_io: prefix dedupe command error messages consistently

[Darrick J. Wong]

Prefix the perror output of the dedupe command consistently.  All the
other perror calls reference the ioctl name directly, so we might as
well do that for all the dedupe cases.

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


diff --git a/io/reflink.c b/io/reflink.c
index 41cb884..4c822b3 100644
--- a/io/reflink.c
+++ b/io/reflink.c
@@ -78,12 +78,12 @@ dedupe_ioctl(
 			goto done;
 		}
 		if (info->status < 0) {
-			fprintf(stderr, "dedupe: %s\n",
+			fprintf(stderr, "XFS_IOC_FILE_EXTENT_SAME: %s\n",
 					_(strerror(-info->status)));
 			goto done;
 		}
 		if (info->status == XFS_EXTENT_DATA_DIFFERS) {
-			fprintf(stderr, "dedupe: %s\n",
+			fprintf(stderr, "XFS_IOC_FILE_EXTENT_SAME: %s\n",
 					_("Extents did not match."));
 			goto done;
 		}

Re: [PATCH 3/5] xfs_io: prefix dedupe command error messages consistently

[Christoph Hellwig]

On Wed, Jan 11, 2017 at 07:06:47PM -0800, Darrick J. Wong wrote:
> Prefix the perror output of the dedupe command consistently.  All the
> other perror calls reference the ioctl name directly, so we might as
> well do that for all the dedupe cases.

Looks good,

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

[PATCH 4/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>
---
 db/init.c |   91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 81 insertions(+), 10 deletions(-)


diff --git a/db/init.c b/db/init.c
index ec1e274..a394728 100644
--- a/db/init.c
+++ b/db/init.c
@@ -51,13 +51,90 @@ usage(void)
 	exit(1);
 }
 
+/* Try to load an AG's superblock, 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 geometry doesn't look sane, try to figure out the real geometry. */
+static void
+sanitize_geometry(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
+{
+	struct xfs_sb		sb;
+	xfs_agblock_t		agblocks;
+
+	/* 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 */
+	if (sbp->sb_blocklog < XFS_MIN_BLOCKSIZE_LOG ||
+	    sbp->sb_blocklog > XFS_MAX_BLOCKSIZE_LOG)
+		sbp->sb_blocklog = libxfs_log2_roundup(sbp->sb_blocksize);
+	if (sbp->sb_blocksize != (1 << sbp->sb_blocklog))
+		sbp->sb_blocksize = (1 << sbp->sb_blocksize);
+
+	/* Clamp dblocks to the size of the device. */
+	if (sbp->sb_dblocks > x.dsize * x.dbsize / sbp->sb_blocksize)
+		sbp->sb_dblocks = x.dsize * x.dbsize / sbp->sb_blocksize;
+
+	/* See if agblocks helps us find a superblock. */
+	mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;
+	if (load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
+		sbp->sb_agcount = sbp->sb_dblocks / sbp->sb_agblocks;
+		goto out;
+	}
+
+	/* See if agcount helps us find a superblock. */
+	agblocks = sbp->sb_agblocks;
+	sbp->sb_agblocks = sbp->sb_dblocks / sbp->sb_agcount;
+	if (sbp->sb_agblocks != 0 &&
+	    load_sb(mp, 1, &sb) &&
+	    sb.sb_magicnum == XFS_SB_MAGIC) {
+		goto out;
+	}
+
+	/* Both are nuts, assume 1 AG. */
+	sbp->sb_agblocks = agblocks;
+	sbp->sb_agcount = 1;
+out:
+	fprintf(stderr,
+		_("%s: device %s AG count is insane.  Limiting reads to the first %u AGs.\n"),
+		progname, fsdevice, sbp->sb_agcount);
+}
+
 void
 init(
 	int		argc,
 	char		**argv)
 {
 	struct xfs_sb	*sbp;
-	struct xfs_buf	*bp;
 	int		c;
 
 	setlocale(LC_ALL, "");
@@ -124,20 +201,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 +217,8 @@ init(
 		}
 	}
 
+	sanitize_geometry(&xmount, sbp);
+
 	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
 			  LIBXFS_MOUNT_DEBUGGER);
 	if (!mp) {

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

[Eric Sandeen]

On 1/11/17 9:06 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>
> ---
>  db/init.c |   91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 81 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/db/init.c b/db/init.c
> index ec1e274..a394728 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -51,13 +51,90 @@ usage(void)
>  	exit(1);
>  }
>  
> +/* Try to load an AG's superblock, 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 geometry doesn't look sane, try to figure out the real geometry. */
> +static void
> +sanitize_geometry(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	struct xfs_sb		sb;
> +	xfs_agblock_t		agblocks;
> +
> +	/* 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 */
> +	if (sbp->sb_blocklog < XFS_MIN_BLOCKSIZE_LOG ||
> +	    sbp->sb_blocklog > XFS_MAX_BLOCKSIZE_LOG)
> +		sbp->sb_blocklog = libxfs_log2_roundup(sbp->sb_blocksize);
> +	if (sbp->sb_blocksize != (1 << sbp->sb_blocklog))
> +		sbp->sb_blocksize = (1 << sbp->sb_blocksize);

I'm really uneasy with having xfs_db ignore on-disk values and go
forward after deciding that it "knows better" and modifying what it
read from disk for fundamental geometry values.

For agcount, I get it - if we can't even /load/ the FS because we OOM,
then this debugging tool is of no use.  Partial initialization with a lower
agcount, if clearly stated to the user, seems reasonable.

But modifying the primary geometry in other ways, such as changing the
blocksize or blocklog or dblocks ... I'm just not comfortable with doing
that here in service to avoiding that OOM, which is related /only/ to
agcount.

Many other db functions use these values; modifying the behavior of
a low-level debugger by silently "knowing better" than what's on disk
based on educated guesses does not sit well with me.

I suppose other alternatives might be things like:

Add an option to read a backup super, instead of the primary
Add an option to limit the agcount regardless of what's on disk

I guess both of those have the downside of only knowing this should
be done /after/ you've OOMed the box on the first try...

I suppose the other option is to make an educated guess about insane
agcount, but without modifying any other superblock buffer values.

And hell at that point maybe just default to 1 ag, to give the admin
a chance to fix it, and restart xfs_db.  "Insane AG count.  Limiting
to 1 AG, please fix and restart xfs_db."

Last thought - how does this "fix it up" heuristic affect xfs_check?

-Eric

> +
> +	/* Clamp dblocks to the size of the device. */
> +	if (sbp->sb_dblocks > x.dsize * x.dbsize / sbp->sb_blocksize)
> +		sbp->sb_dblocks = x.dsize * x.dbsize / sbp->sb_blocksize;
> +
> +	/* See if agblocks helps us find a superblock. */
> +	mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;
> +	if (load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
> +		sbp->sb_agcount = sbp->sb_dblocks / sbp->sb_agblocks;
> +		goto out;
> +	}
> +
> +	/* See if agcount helps us find a superblock. */
> +	agblocks = sbp->sb_agblocks;
> +	sbp->sb_agblocks = sbp->sb_dblocks / sbp->sb_agcount;
> +	if (sbp->sb_agblocks != 0 &&
> +	    load_sb(mp, 1, &sb) &&
> +	    sb.sb_magicnum == XFS_SB_MAGIC) {
> +		goto out;
> +	}



> +
> +	/* Both are nuts, assume 1 AG. */
> +	sbp->sb_agblocks = agblocks;
> +	sbp->sb_agcount = 1;
> +out:
> +	fprintf(stderr,
> +		_("%s: device %s AG count is insane.  Limiting reads to the first %u AGs.\n"),
> +		progname, fsdevice, sbp->sb_agcount);
> +}
> +
>  void
>  init(
>  	int		argc,
>  	char		**argv)
>  {
>  	struct xfs_sb	*sbp;
> -	struct xfs_buf	*bp;
>  	int		c;
>  
>  	setlocale(LC_ALL, "");
> @@ -124,20 +201,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 +217,8 @@ init(
>  		}
>  	}
>  
> +	sanitize_geometry(&xmount, sbp);
> +
>  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
>  			  LIBXFS_MOUNT_DEBUGGER);
>  	if (!mp) {
> 
> --
> 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_db: sanitize geometry on load

[Brian Foster]

On Thu, Jan 12, 2017 at 08:34:50AM -0600, Eric Sandeen wrote:
> On 1/11/17 9:06 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>
> > ---
> >  db/init.c |   91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 81 insertions(+), 10 deletions(-)
> > 
> > 
> > diff --git a/db/init.c b/db/init.c
> > index ec1e274..a394728 100644
> > --- a/db/init.c
> > +++ b/db/init.c
> > @@ -51,13 +51,90 @@ usage(void)
> >  	exit(1);
> >  }
> >  
> > +/* Try to load an AG's superblock, 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 geometry doesn't look sane, try to figure out the real geometry. */
> > +static void
> > +sanitize_geometry(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_sb		*sbp)
> > +{
> > +	struct xfs_sb		sb;
> > +	xfs_agblock_t		agblocks;
> > +
> > +	/* 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 */
> > +	if (sbp->sb_blocklog < XFS_MIN_BLOCKSIZE_LOG ||
> > +	    sbp->sb_blocklog > XFS_MAX_BLOCKSIZE_LOG)
> > +		sbp->sb_blocklog = libxfs_log2_roundup(sbp->sb_blocksize);

What if blocksize is bogus?

> > +	if (sbp->sb_blocksize != (1 << sbp->sb_blocklog))
> > +		sbp->sb_blocksize = (1 << sbp->sb_blocksize);
> 

Do you mean (1 << sbp->sb_blocklog) here?

> I'm really uneasy with having xfs_db ignore on-disk values and go
> forward after deciding that it "knows better" and modifying what it
> read from disk for fundamental geometry values.
> 

I agree in principle. If I'm using xfs_db, I'd want it to navigate
primarily based on what's on disk. If what is on disk means the
application cannot sanely/safely initialize all of its data structures
and thus limits navigation ability, then so be it.

I guess I'm not clear on if/why we'd need xfs_db to stumble along in a
case where the superblock is hosed enough to cause this kind of problem.
Why wouldn't we just tell the user to run xfs_repair and exit, for
example?

> For agcount, I get it - if we can't even /load/ the FS because we OOM,
> then this debugging tool is of no use.  Partial initialization with a lower
> agcount, if clearly stated to the user, seems reasonable.
> 
> But modifying the primary geometry in other ways, such as changing the
> blocksize or blocklog or dblocks ... I'm just not comfortable with doing
> that here in service to avoiding that OOM, which is related /only/ to
> agcount.
> 
> Many other db functions use these values; modifying the behavior of
> a low-level debugger by silently "knowing better" than what's on disk
> based on educated guesses does not sit well with me.
> 
> I suppose other alternatives might be things like:
> 
> Add an option to read a backup super, instead of the primary
> Add an option to limit the agcount regardless of what's on disk
> 
> I guess both of those have the downside of only knowing this should
> be done /after/ you've OOMed the box on the first try...
> 

These seem like reasonable options if we can detect the off the rails
superblock and exit. Then the user can try more aggressive options as
appropriate. The first seems like a reasonable option. The second seems
like it requires a bit more detail about the supposed corruption and
might not be as generically useful.

Other options might be to scan for a valid superblock a la xfs_repair or
just not initialize format data structures such that we enter a crippled
mode where only raw block access is supported. Either of those might
still not be worth the extra effort beyond just exiting though..? I'm
guessing most of the code probably assumes/expects that things are
initialized one way or another, valid or otherwise..

Brian

> I suppose the other option is to make an educated guess about insane
> agcount, but without modifying any other superblock buffer values.
> 
> And hell at that point maybe just default to 1 ag, to give the admin
> a chance to fix it, and restart xfs_db.  "Insane AG count.  Limiting
> to 1 AG, please fix and restart xfs_db."
> 
> Last thought - how does this "fix it up" heuristic affect xfs_check?
> 
> -Eric
> 
> > +
> > +	/* Clamp dblocks to the size of the device. */
> > +	if (sbp->sb_dblocks > x.dsize * x.dbsize / sbp->sb_blocksize)
> > +		sbp->sb_dblocks = x.dsize * x.dbsize / sbp->sb_blocksize;
> > +
> > +	/* See if agblocks helps us find a superblock. */
> > +	mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;
> > +	if (load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
> > +		sbp->sb_agcount = sbp->sb_dblocks / sbp->sb_agblocks;
> > +		goto out;
> > +	}
> > +
> > +	/* See if agcount helps us find a superblock. */
> > +	agblocks = sbp->sb_agblocks;
> > +	sbp->sb_agblocks = sbp->sb_dblocks / sbp->sb_agcount;
> > +	if (sbp->sb_agblocks != 0 &&
> > +	    load_sb(mp, 1, &sb) &&
> > +	    sb.sb_magicnum == XFS_SB_MAGIC) {
> > +		goto out;
> > +	}
> 
> 
> 
> > +
> > +	/* Both are nuts, assume 1 AG. */
> > +	sbp->sb_agblocks = agblocks;
> > +	sbp->sb_agcount = 1;
> > +out:
> > +	fprintf(stderr,
> > +		_("%s: device %s AG count is insane.  Limiting reads to the first %u AGs.\n"),
> > +		progname, fsdevice, sbp->sb_agcount);
> > +}
> > +
> >  void
> >  init(
> >  	int		argc,
> >  	char		**argv)
> >  {
> >  	struct xfs_sb	*sbp;
> > -	struct xfs_buf	*bp;
> >  	int		c;
> >  
> >  	setlocale(LC_ALL, "");
> > @@ -124,20 +201,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 +217,8 @@ init(
> >  		}
> >  	}
> >  
> > +	sanitize_geometry(&xmount, sbp);
> > +
> >  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
> >  			  LIBXFS_MOUNT_DEBUGGER);
> >  	if (!mp) {
> > 
> > --
> > 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 4/5] xfs_db: sanitize geometry on load

[Darrick J. Wong]

On Thu, Jan 12, 2017 at 10:09:48AM -0500, Brian Foster wrote:
> On Thu, Jan 12, 2017 at 08:34:50AM -0600, Eric Sandeen wrote:
> > On 1/11/17 9:06 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>
> > > ---
> > >  db/init.c |   91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 81 insertions(+), 10 deletions(-)
> > > 
> > > 
> > > diff --git a/db/init.c b/db/init.c
> > > index ec1e274..a394728 100644
> > > --- a/db/init.c
> > > +++ b/db/init.c
> > > @@ -51,13 +51,90 @@ usage(void)
> > >  	exit(1);
> > >  }
> > >  
> > > +/* Try to load an AG's superblock, 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 geometry doesn't look sane, try to figure out the real geometry. */
> > > +static void
> > > +sanitize_geometry(
> > > +	struct xfs_mount	*mp,
> > > +	struct xfs_sb		*sbp)
> > > +{
> > > +	struct xfs_sb		sb;
> > > +	xfs_agblock_t		agblocks;
> > > +
> > > +	/* 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 */
> > > +	if (sbp->sb_blocklog < XFS_MIN_BLOCKSIZE_LOG ||
> > > +	    sbp->sb_blocklog > XFS_MAX_BLOCKSIZE_LOG)
> > > +		sbp->sb_blocklog = libxfs_log2_roundup(sbp->sb_blocksize);
> 
> What if blocksize is bogus?
> 
> > > +	if (sbp->sb_blocksize != (1 << sbp->sb_blocklog))
> > > +		sbp->sb_blocksize = (1 << sbp->sb_blocksize);
> > 
> 
> Do you mean (1 << sbp->sb_blocklog) here?
> 
> > I'm really uneasy with having xfs_db ignore on-disk values and go
> > forward after deciding that it "knows better" and modifying what it
> > read from disk for fundamental geometry values.
> > 
> 
> I agree in principle. If I'm using xfs_db, I'd want it to navigate
> primarily based on what's on disk. If what is on disk means the
> application cannot sanely/safely initialize all of its data structures
> and thus limits navigation ability, then so be it.
> 
> I guess I'm not clear on if/why we'd need xfs_db to stumble along in a
> case where the superblock is hosed enough to cause this kind of problem.
> Why wouldn't we just tell the user to run xfs_repair and exit, for
> example?
> 
> > For agcount, I get it - if we can't even /load/ the FS because we OOM,
> > then this debugging tool is of no use.  Partial initialization with a lower
> > agcount, if clearly stated to the user, seems reasonable.
> > 
> > But modifying the primary geometry in other ways, such as changing the
> > blocksize or blocklog or dblocks ... I'm just not comfortable with doing
> > that here in service to avoiding that OOM, which is related /only/ to
> > agcount.
> > 
> > Many other db functions use these values; modifying the behavior of
> > a low-level debugger by silently "knowing better" than what's on disk
> > based on educated guesses does not sit well with me.
> > 
> > I suppose other alternatives might be things like:
> > 
> > Add an option to read a backup super, instead of the primary
> > Add an option to limit the agcount regardless of what's on disk
> > 
> > I guess both of those have the downside of only knowing this should
> > be done /after/ you've OOMed the box on the first try...
> > 
> 
> These seem like reasonable options if we can detect the off the rails
> superblock and exit. Then the user can try more aggressive options as
> appropriate. The first seems like a reasonable option. The second seems
> like it requires a bit more detail about the supposed corruption and
> might not be as generically useful.
> 
> Other options might be to scan for a valid superblock a la xfs_repair or
> just not initialize format data structures such that we enter a crippled
> mode where only raw block access is supported. Either of those might
> still not be worth the extra effort beyond just exiting though..? I'm
> guessing most of the code probably assumes/expects that things are
> initialized one way or another, valid or otherwise..

A fair amount of it does, and crashes when we feed it junk values...

> Brian
> 
> > I suppose the other option is to make an educated guess about insane
> > agcount, but without modifying any other superblock buffer values.

I forgot to send out that patch last night... how about that instead?

> > And hell at that point maybe just default to 1 ag, to give the admin
> > a chance to fix it, and restart xfs_db.  "Insane AG count.  Limiting
> > to 1 AG, please fix and restart xfs_db."
> > 
> > Last thought - how does this "fix it up" heuristic affect xfs_check?

Seems to work fine after we reset agcount to something "reasonable",
in the sense that it complains about badness.

--D

> > 
> > -Eric
> > 
> > > +
> > > +	/* Clamp dblocks to the size of the device. */
> > > +	if (sbp->sb_dblocks > x.dsize * x.dbsize / sbp->sb_blocksize)
> > > +		sbp->sb_dblocks = x.dsize * x.dbsize / sbp->sb_blocksize;
> > > +
> > > +	/* See if agblocks helps us find a superblock. */
> > > +	mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;
> > > +	if (load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
> > > +		sbp->sb_agcount = sbp->sb_dblocks / sbp->sb_agblocks;
> > > +		goto out;
> > > +	}
> > > +
> > > +	/* See if agcount helps us find a superblock. */
> > > +	agblocks = sbp->sb_agblocks;
> > > +	sbp->sb_agblocks = sbp->sb_dblocks / sbp->sb_agcount;
> > > +	if (sbp->sb_agblocks != 0 &&
> > > +	    load_sb(mp, 1, &sb) &&
> > > +	    sb.sb_magicnum == XFS_SB_MAGIC) {
> > > +		goto out;
> > > +	}
> > 
> > 
> > 
> > > +
> > > +	/* Both are nuts, assume 1 AG. */
> > > +	sbp->sb_agblocks = agblocks;
> > > +	sbp->sb_agcount = 1;
> > > +out:
> > > +	fprintf(stderr,
> > > +		_("%s: device %s AG count is insane.  Limiting reads to the first %u AGs.\n"),
> > > +		progname, fsdevice, sbp->sb_agcount);
> > > +}
> > > +
> > >  void
> > >  init(
> > >  	int		argc,
> > >  	char		**argv)
> > >  {
> > >  	struct xfs_sb	*sbp;
> > > -	struct xfs_buf	*bp;
> > >  	int		c;
> > >  
> > >  	setlocale(LC_ALL, "");
> > > @@ -124,20 +201,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 +217,8 @@ init(
> > >  		}
> > >  	}
> > >  
> > > +	sanitize_geometry(&xmount, sbp);
> > > +
> > >  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
> > >  			  LIBXFS_MOUNT_DEBUGGER);
> > >  	if (!mp) {
> > > 
> > > --
> > > 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
> --
> 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_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.
---
 db/init.c |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 87 insertions(+), 10 deletions(-)

diff --git a/db/init.c b/db/init.c
index ec1e274..db133d7 100644
--- a/db/init.c
+++ b/db/init.c
@@ -51,13 +51,96 @@ usage(void)
 	exit(1);
 }
 
+/* Try to load an AG's superblock, 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 geometry doesn't look sane, try to figure out the real geometry. */
+static void
+sanitize_geometry(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
+{
+	struct xfs_sb		sb;
+	unsigned int		blocklog;
+	unsigned int		blocksize;
+	unsigned int		agblocks;
+	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;
+
+	/* See if agblocks helps us find a superblock. */
+	mp->m_blkbb_log = blocklog - BBSHIFT;
+	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
+	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
+		sbp->sb_agcount = dblocks / sbp->sb_agblocks;
+		goto out;
+	}
+
+	/* See if agcount helps us find a superblock. */
+	agblocks = sbp->sb_agblocks;
+	sbp->sb_agblocks = dblocks / sbp->sb_agcount;
+	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
+	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
+		goto out;
+	}
+
+	/* Both are nuts, assume 1 AG. */
+	sbp->sb_agblocks = agblocks;
+	sbp->sb_agcount = 1;
+out:
+	fprintf(stderr,
+		_("%s: device %s AG count is insane.  Limiting reads to the first %u AGs.\n"),
+		progname, fsdevice, sbp->sb_agcount);
+}
+
 void
 init(
 	int		argc,
 	char		**argv)
 {
 	struct xfs_sb	*sbp;
-	struct xfs_buf	*bp;
 	int		c;
 
 	setlocale(LC_ALL, "");
@@ -124,20 +207,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 +223,8 @@ init(
 		}
 	}
 
+	sanitize_geometry(&xmount, sbp);
+
 	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
 			  LIBXFS_MOUNT_DEBUGGER);
 	if (!mp) {

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

[Eric Sandeen]

On 1/12/17 2:41 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.

Ok, I like this a bit better.  More comments, though.  Sorry, will
try to do a better full review in future to avoid the iteration :(
Below ...

> ---
>  db/init.c |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 87 insertions(+), 10 deletions(-)
> 
> diff --git a/db/init.c b/db/init.c
> index ec1e274..db133d7 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -51,13 +51,96 @@ usage(void)
>  	exit(1);
>  }
>  
> +/* Try to load an AG's superblock, no verifiers. */

/* ... for the given agno ... */

> +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 geometry doesn't look sane, try to figure out the real geometry. */
> +static void
> +sanitize_geometry(
Probably now:

+/*
+ * If the agcount doesn't look sane, try to figure out the real agcount.
+ * A wildly too-large agcount may OOM in libxfs_initialize_perag
+ */
+static void
+sanitize_agcount(

> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	struct xfs_sb		sb;
> +	unsigned int		blocklog;
> +	unsigned int		blocksize;
> +	unsigned int		agblocks;
> +	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;

ok now in theory blocksize & dblocks are as good a guess as we can
get...

> +
> +	/* See if agblocks helps us find a superblock. */
> +	mp->m_blkbb_log = blocklog - BBSHIFT;
> +	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
> +	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {

load_sb translates the one we found into &sb....

> +		sbp->sb_agcount = dblocks / sbp->sb_agblocks;

wouldn't it make more sense to just assign from sb->sb_agcount?
But who's to say the 2nd one isn't corrupt in the same way?
Grump.

> +		goto out;
> +	}
> +
> +	/* See if agcount helps us find a superblock. */

Wait I thought agcount problems is why we're here in
the first place.  What's this for?

> +	agblocks = sbp->sb_agblocks;
> +	sbp->sb_agblocks = dblocks / sbp->sb_agcount;
> +	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
> +	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
> +		goto out;
> +	}
> +
> +	/* Both are nuts, assume 1 AG. */
> +	sbp->sb_agblocks = agblocks;
> +	sbp->sb_agcount = 1;

I'd almost rather just jump here and let the admin sort it out...

But let me play with this a little mmkay?

-Eric

> +out:
> +	fprintf(stderr,
> +		_("%s: device %s AG count is insane.  Limiting reads to the first %u AGs.\n"),
> +		progname, fsdevice, sbp->sb_agcount);
> +}
> +
>  void
>  init(
>  	int		argc,
>  	char		**argv)
>  {
>  	struct xfs_sb	*sbp;
> -	struct xfs_buf	*bp;
>  	int		c;
>  
>  	setlocale(LC_ALL, "");
> @@ -124,20 +207,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 +223,8 @@ init(
>  		}
>  	}
>  
> +	sanitize_geometry(&xmount, sbp);
> +
>  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
>  			  LIBXFS_MOUNT_DEBUGGER);
>  	if (!mp) {
> --
> 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_db: sanitize geometry on load

[Darrick J. Wong]

On Thu, Jan 12, 2017 at 05:20:26PM -0600, Eric Sandeen wrote:
> On 1/12/17 2:41 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.
> 
> Ok, I like this a bit better.  More comments, though.  Sorry, will
> try to do a better full review in future to avoid the iteration :(
> Below ...
> 
> > ---
> >  db/init.c |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 87 insertions(+), 10 deletions(-)
> > 
> > diff --git a/db/init.c b/db/init.c
> > index ec1e274..db133d7 100644
> > --- a/db/init.c
> > +++ b/db/init.c
> > @@ -51,13 +51,96 @@ usage(void)
> >  	exit(1);
> >  }
> >  
> > +/* Try to load an AG's superblock, no verifiers. */
> 
> /* ... for the given agno ... */
> 
> > +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 geometry doesn't look sane, try to figure out the real geometry. */
> > +static void
> > +sanitize_geometry(
> Probably now:
> 
> +/*
> + * If the agcount doesn't look sane, try to figure out the real agcount.
> + * A wildly too-large agcount may OOM in libxfs_initialize_perag
> + */
> +static void
> +sanitize_agcount(
> 
> > +	struct xfs_mount	*mp,
> > +	struct xfs_sb		*sbp)
> > +{
> > +	struct xfs_sb		sb;
> > +	unsigned int		blocklog;
> > +	unsigned int		blocksize;
> > +	unsigned int		agblocks;
> > +	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;
> 
> ok now in theory blocksize & dblocks are as good a guess as we can
> get...
> 
> > +
> > +	/* See if agblocks helps us find a superblock. */
> > +	mp->m_blkbb_log = blocklog - BBSHIFT;
> > +	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
> > +	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
> 
> load_sb translates the one we found into &sb....
> 
> > +		sbp->sb_agcount = dblocks / sbp->sb_agblocks;
> 
> wouldn't it make more sense to just assign from sb->sb_agcount?
> But who's to say the 2nd one isn't corrupt in the same way?
> Grump.

Eh.  Let's instead do the following: If agblocks helps us to find
something with the sb magic we'll suggest that agcount to the user.
In any case we'll set agcount = 1 and let the user sort it out.

> > +		goto out;
> > +	}
> > +
> > +	/* See if agcount helps us find a superblock. */
> 
> Wait I thought agcount problems is why we're here in
> the first place.  What's this for?

Leftover from when I was trying to fix all the geometry parameters.

> > +	agblocks = sbp->sb_agblocks;
> > +	sbp->sb_agblocks = dblocks / sbp->sb_agcount;
> > +	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
> > +	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
> > +		goto out;
> > +	}
> > +
> > +	/* Both are nuts, assume 1 AG. */
> > +	sbp->sb_agblocks = agblocks;
> > +	sbp->sb_agcount = 1;
> 
> I'd almost rather just jump here and let the admin sort it out...
> 
> But let me play with this a little mmkay?

<shrug> At this point I have a v3 ready so we might as well jump to that.

--D

> 
> -Eric
> 
> > +out:
> > +	fprintf(stderr,
> > +		_("%s: device %s AG count is insane.  Limiting reads to the first %u AGs.\n"),
> > +		progname, fsdevice, sbp->sb_agcount);
> > +}
> > +
> >  void
> >  init(
> >  	int		argc,
> >  	char		**argv)
> >  {
> >  	struct xfs_sb	*sbp;
> > -	struct xfs_buf	*bp;
> >  	int		c;
> >  
> >  	setlocale(LC_ALL, "");
> > @@ -124,20 +207,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 +223,8 @@ init(
> >  		}
> >  	}
> >  
> > +	sanitize_geometry(&xmount, sbp);
> > +
> >  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
> >  			  LIBXFS_MOUNT_DEBUGGER);
> >  	if (!mp) {
> > --
> > 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_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.
---
 db/init.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 10 deletions(-)

diff --git a/db/init.c b/db/init.c
index ec1e274..43a9409 100644
--- a/db/init.c
+++ b/db/init.c
@@ -51,13 +51,94 @@ 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)
+{
+	struct xfs_sb		sb;
+	unsigned int		blocklog;
+	unsigned int		blocksize;
+	unsigned int		agblocks;
+	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;
+
+	/*
+	 * See if agblocks helps us find a superblock.
+	 * blkbb_log is later (re)set by libxfs_mount.
+	 */
+	mp->m_blkbb_log = blocklog - BBSHIFT;
+	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
+	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
+		fprintf(stderr,
+_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
+			progname, fsdevice, dblocks / sbp->sb_agblocks);
+	} else {
+		fprintf(stderr,
+_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
+			progname, fsdevice);
+	}
+
+	/* Assume 1 AG to avoid OOM. */
+	sbp->sb_agcount = 1;
+}
+
 void
 init(
 	int		argc,
 	char		**argv)
 {
 	struct xfs_sb	*sbp;
-	struct xfs_buf	*bp;
 	int		c;
 
 	setlocale(LC_ALL, "");
@@ -124,20 +205,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 +221,8 @@ init(
 		}
 	}
 
+	sanitize_geometry(&xmount, sbp);
+
 	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
 			  LIBXFS_MOUNT_DEBUGGER);
 	if (!mp) {

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

[Brian Foster]

On Thu, Jan 12, 2017 at 04:32:01PM -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.
> ---
>  db/init.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 85 insertions(+), 10 deletions(-)
> 
> diff --git a/db/init.c b/db/init.c
> index ec1e274..43a9409 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -51,13 +51,94 @@ 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)
> +{
> +	struct xfs_sb		sb;
> +	unsigned int		blocklog;
> +	unsigned int		blocksize;
> +	unsigned int		agblocks;
> +	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);
> +

Same questions as before with regard to the above hunk.

> +	/* 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;
> +
> +	/*
> +	 * See if agblocks helps us find a superblock.
> +	 * blkbb_log is later (re)set by libxfs_mount.
> +	 */
> +	mp->m_blkbb_log = blocklog - BBSHIFT;
> +	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
> +	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {

If agblocks is bogus, this can effectively point anywhere in the fs,
correct? If so, I wonder how robust this magic number check is... for
example, what prevents us from pointing at a file data block with a
valid XFS superblock? IOW, perhaps we'd also need to validate sb_uuid.

> +		fprintf(stderr,
> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
> +	} else {
> +		fprintf(stderr,
> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> +			progname, fsdevice);
> +	}

For reasons like the above, I think xfs_db shouldn't be in the business
of repair like validation (xfs_check notwithstanding). That said,
dropping into a fixed single AG mode seems less risky than trying to
surmise a valid geometry. I'd get rid of the "this might be your
agcount" messaging entirely though and just replace it with something
that explicitly states the filesystem is corrupted, the runtime geometry
is invalid and that the user should probably run xfs_repair before doing
anything.

I still like the idea of the single AG mode thing as a command line flag
rather than default behavior because it requires user acknowledgement,
but this is a debug tool after all, so I'll defer to Eric on that. I do
think that if we create this kind of invalid runtime mode, this should
be split into two patches. First, a bugfix patch for the core OOM
problem (i.e., detect a wacky superblock and exit). Second, replace the
exit with the single AG runtime mode thing.

Brian

> +
> +	/* Assume 1 AG to avoid OOM. */
> +	sbp->sb_agcount = 1;
> +}
> +
>  void
>  init(
>  	int		argc,
>  	char		**argv)
>  {
>  	struct xfs_sb	*sbp;
> -	struct xfs_buf	*bp;
>  	int		c;
>  
>  	setlocale(LC_ALL, "");
> @@ -124,20 +205,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 +221,8 @@ init(
>  		}
>  	}
>  
> +	sanitize_geometry(&xmount, sbp);
> +
>  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
>  			  LIBXFS_MOUNT_DEBUGGER);
>  	if (!mp) {
> --
> 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 v3 4/5] xfs_db: sanitize geometry on load

[Eric Sandeen]

On 1/13/17 7:35 AM, Brian Foster wrote:
>> +		fprintf(stderr,
>> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
>> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
>> +	} else {
>> +		fprintf(stderr,
>> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
>> +			progname, fsdevice);
>> +	}
> For reasons like the above, I think xfs_db shouldn't be in the business
> of repair like validation (xfs_check notwithstanding). That said,
> dropping into a fixed single AG mode seems less risky than trying to
> surmise a valid geometry. I'd get rid of the "this might be your
> agcount" messaging entirely though and just replace it with something
> that explicitly states the filesystem is corrupted, the runtime geometry
> is invalid and that the user should probably run xfs_repair before doing
> anything.

So keep in mind that xfs_db is for people with super xfs powers. (*)

I wouldn't suggest repair, I'd start with 1 ag to avoid the OOM, state 
that clearly, and punt the problem to the admin with no other specific
suggestions.

> I still like the idea of the single AG mode thing as a command line flag
> rather than default behavior because it requires user acknowledgement,
> but this is a debug tool after all, so I'll defer to Eric on that. I do
> think that if we create this kind of invalid runtime mode, this should
> be split into two patches. First, a bugfix patch for the core OOM
> problem (i.e., detect a wacky superblock and exit). Second, replace the
> exit with the single AG runtime mode thing.

Well, the problem with a flag, I think, is that you might have already
unwittingly OOMed your box to find out that you need it.
Rebooting to try again with a flag sucks.

(*) unless you are invoking it via xfs_admin.sh, dammit.  We sure wouldn't
want xfs_admin to exit happily, having updated only one AG.  Dammit!

Perhaps it should set exitcode, and then xfs_admin could do something
like:

	xfs_db -c quit $DEV

first, and check that db is able to initialize sanely before using it again
to perform normal admin functions.

-Eric

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

[Brian Foster]

On Fri, Jan 13, 2017 at 08:25:47PM -0600, Eric Sandeen wrote:
> On 1/13/17 7:35 AM, Brian Foster wrote:
> >> +		fprintf(stderr,
> >> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
> >> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
> >> +	} else {
> >> +		fprintf(stderr,
> >> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> >> +			progname, fsdevice);
> >> +	}
> > For reasons like the above, I think xfs_db shouldn't be in the business
> > of repair like validation (xfs_check notwithstanding). That said,
> > dropping into a fixed single AG mode seems less risky than trying to
> > surmise a valid geometry. I'd get rid of the "this might be your
> > agcount" messaging entirely though and just replace it with something
> > that explicitly states the filesystem is corrupted, the runtime geometry
> > is invalid and that the user should probably run xfs_repair before doing
> > anything.
> 
> So keep in mind that xfs_db is for people with super xfs powers. (*)
> 
> I wouldn't suggest repair, I'd start with 1 ag to avoid the OOM, state 
> that clearly, and punt the problem to the admin with no other specific
> suggestions.
> 
> > I still like the idea of the single AG mode thing as a command line flag
> > rather than default behavior because it requires user acknowledgement,
> > but this is a debug tool after all, so I'll defer to Eric on that. I do
> > think that if we create this kind of invalid runtime mode, this should
> > be split into two patches. First, a bugfix patch for the core OOM
> > problem (i.e., detect a wacky superblock and exit). Second, replace the
> > exit with the single AG runtime mode thing.
> 
> Well, the problem with a flag, I think, is that you might have already
> unwittingly OOMed your box to find out that you need it.
> Rebooting to try again with a flag sucks.
> 

I don't see how that is relevant. I'm not suggesting a
--please-don't-oom-in-case-of-corruption flag. :) As mentioned
previously, I think the bug fix here is a simple patch to detect the
bogus superblock and exit gracefully rather than go off the rails and
end up OOM killed.

>From there the OOM is irrelevant and we can optionally enhance xfs_db to
try and allow it to run in such situations. To be honest, I'm perfectly
happy for xfs_db to exit gracefully in this situation and to leave it at
that. I think the majority of cases where this problem occurs, the next
logical step is to run xfs_repair. I suggested the flag approach more
because I think it's more appropriate to do things like fabricate fs
geometry behind a flag rather than by default. The larger point is that
if we want this kind of enhancement, it should probably be driven more
by a use case than an unfortunate (and probably rare) bug. I don't see
why we need to complicate the bug fix with the fancy enhancement.

Brian

> (*) unless you are invoking it via xfs_admin.sh, dammit.  We sure wouldn't
> want xfs_admin to exit happily, having updated only one AG.  Dammit!
> 
> Perhaps it should set exitcode, and then xfs_admin could do something
> like:
> 
> 	xfs_db -c quit $DEV
> 
> first, and check that db is able to initialize sanely before using it again
> to perform normal admin functions.
> 
> -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 v3 4/5] xfs_db: sanitize geometry on load

[Eric Sandeen]

On 1/13/17 9:44 PM, Brian Foster wrote:
> On Fri, Jan 13, 2017 at 08:25:47PM -0600, Eric Sandeen wrote:
>> On 1/13/17 7:35 AM, Brian Foster wrote:
>>>> +		fprintf(stderr,
>>>> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
>>>> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
>>>> +	} else {
>>>> +		fprintf(stderr,
>>>> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
>>>> +			progname, fsdevice);
>>>> +	}
>>> For reasons like the above, I think xfs_db shouldn't be in the business
>>> of repair like validation (xfs_check notwithstanding). That said,
>>> dropping into a fixed single AG mode seems less risky than trying to
>>> surmise a valid geometry. I'd get rid of the "this might be your
>>> agcount" messaging entirely though and just replace it with something
>>> that explicitly states the filesystem is corrupted, the runtime geometry
>>> is invalid and that the user should probably run xfs_repair before doing
>>> anything.
>>
>> So keep in mind that xfs_db is for people with super xfs powers. (*)
>>
>> I wouldn't suggest repair, I'd start with 1 ag to avoid the OOM, state 
>> that clearly, and punt the problem to the admin with no other specific
>> suggestions.
>>
>>> I still like the idea of the single AG mode thing as a command line flag
>>> rather than default behavior because it requires user acknowledgement,
>>> but this is a debug tool after all, so I'll defer to Eric on that. I do
>>> think that if we create this kind of invalid runtime mode, this should
>>> be split into two patches. First, a bugfix patch for the core OOM
>>> problem (i.e., detect a wacky superblock and exit). Second, replace the
>>> exit with the single AG runtime mode thing.
>>
>> Well, the problem with a flag, I think, is that you might have already
>> unwittingly OOMed your box to find out that you need it.
>> Rebooting to try again with a flag sucks.
>>
> 
> I don't see how that is relevant. I'm not suggesting a
> --please-don't-oom-in-case-of-corruption flag. :) As mentioned
> previously, I think the bug fix here is a simple patch to detect the
> bogus superblock and exit gracefully rather than go off the rails and
> end up OOM killed.

sorry, misunderstood the 
"idea of the single AG mode thing as a command line flag" idea, I guess.
 
> From there the OOM is irrelevant and we can optionally enhance xfs_db to
> try and allow it to run in such situations. To be honest, I'm perfectly
> happy for xfs_db to exit gracefully in this situation and to leave it at
> that. I think the majority of cases where this problem occurs, the next
> logical step is to run xfs_repair. I suggested the flag approach more
> because I think it's more appropriate to do things like fabricate fs
> geometry behind a flag rather than by default. The larger point is that
> if we want this kind of enhancement, it should probably be driven more
> by a use case than an unfortunate (and probably rare) bug. I don't see
> why we need to complicate the bug fix with the fancy enhancement.

*nod*  ok, I had understood the flag idea backwards-ly I guess.

I do think that mere mortal invocations via xfs_admin need to be
handled in this "ignore agcount" case, though...

-Eric
 
> Brian
> 
>> (*) unless you are invoking it via xfs_admin.sh, dammit.  We sure wouldn't
>> want xfs_admin to exit happily, having updated only one AG.  Dammit!
>>
>> Perhaps it should set exitcode, and then xfs_admin could do something
>> like:
>>
>> 	xfs_db -c quit $DEV
>>
>> first, and check that db is able to initialize sanely before using it again
>> to perform normal admin functions.
>>
>> -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 v3 4/5] xfs_db: sanitize geometry on load

[Brian Foster]

On Fri, Jan 13, 2017 at 09:51:20PM -0600, Eric Sandeen wrote:
> On 1/13/17 9:44 PM, Brian Foster wrote:
> > On Fri, Jan 13, 2017 at 08:25:47PM -0600, Eric Sandeen wrote:
> >> On 1/13/17 7:35 AM, Brian Foster wrote:
> >>>> +		fprintf(stderr,
> >>>> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
> >>>> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
> >>>> +	} else {
> >>>> +		fprintf(stderr,
> >>>> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> >>>> +			progname, fsdevice);
> >>>> +	}
> >>> For reasons like the above, I think xfs_db shouldn't be in the business
> >>> of repair like validation (xfs_check notwithstanding). That said,
> >>> dropping into a fixed single AG mode seems less risky than trying to
> >>> surmise a valid geometry. I'd get rid of the "this might be your
> >>> agcount" messaging entirely though and just replace it with something
> >>> that explicitly states the filesystem is corrupted, the runtime geometry
> >>> is invalid and that the user should probably run xfs_repair before doing
> >>> anything.
> >>
> >> So keep in mind that xfs_db is for people with super xfs powers. (*)
> >>
> >> I wouldn't suggest repair, I'd start with 1 ag to avoid the OOM, state 
> >> that clearly, and punt the problem to the admin with no other specific
> >> suggestions.
> >>
> >>> I still like the idea of the single AG mode thing as a command line flag
> >>> rather than default behavior because it requires user acknowledgement,
> >>> but this is a debug tool after all, so I'll defer to Eric on that. I do
> >>> think that if we create this kind of invalid runtime mode, this should
> >>> be split into two patches. First, a bugfix patch for the core OOM
> >>> problem (i.e., detect a wacky superblock and exit). Second, replace the
> >>> exit with the single AG runtime mode thing.
> >>
> >> Well, the problem with a flag, I think, is that you might have already
> >> unwittingly OOMed your box to find out that you need it.
> >> Rebooting to try again with a flag sucks.
> >>
> > 
> > I don't see how that is relevant. I'm not suggesting a
> > --please-don't-oom-in-case-of-corruption flag. :) As mentioned
> > previously, I think the bug fix here is a simple patch to detect the
> > bogus superblock and exit gracefully rather than go off the rails and
> > end up OOM killed.
> 
> sorry, misunderstood the 
> "idea of the single AG mode thing as a command line flag" idea, I guess.
>  
> > From there the OOM is irrelevant and we can optionally enhance xfs_db to
> > try and allow it to run in such situations. To be honest, I'm perfectly
> > happy for xfs_db to exit gracefully in this situation and to leave it at
> > that. I think the majority of cases where this problem occurs, the next
> > logical step is to run xfs_repair. I suggested the flag approach more
> > because I think it's more appropriate to do things like fabricate fs
> > geometry behind a flag rather than by default. The larger point is that
> > if we want this kind of enhancement, it should probably be driven more
> > by a use case than an unfortunate (and probably rare) bug. I don't see
> > why we need to complicate the bug fix with the fancy enhancement.
> 
> *nod*  ok, I had understood the flag idea backwards-ly I guess.
> 
> I do think that mere mortal invocations via xfs_admin need to be
> handled in this "ignore agcount" case, though...
> 

We're talking about invocations intended to modify things (i.e., version
flags, fs uuid, etc.), right? If so, wouldn't we want those things to
fail if the superblock is busted?

Brian

> -Eric
>  
> > Brian
> > 
> >> (*) unless you are invoking it via xfs_admin.sh, dammit.  We sure wouldn't
> >> want xfs_admin to exit happily, having updated only one AG.  Dammit!
> >>
> >> Perhaps it should set exitcode, and then xfs_admin could do something
> >> like:
> >>
> >> 	xfs_db -c quit $DEV
> >>
> >> first, and check that db is able to initialize sanely before using it again
> >> to perform normal admin functions.
> >>
> >> -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
> 
> --
> 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 v3 4/5] xfs_db: sanitize geometry on load

[Eric Sandeen]

On 1/14/17 6:53 AM, Brian Foster wrote:
>> I do think that mere mortal invocations via xfs_admin need to be
>> handled in this "ignore agcount" case, though...
>>
> We're talking about invocations intended to modify things (i.e., version
> flags, fs uuid, etc.), right? If so, wouldn't we want those things to
> fail if the superblock is busted?

Yes - I'm saying that if xfs_db continues with only 1 ag, those tools
will not work as expected.  (they normally modify all superblocks).

setting exitcode to 1 in the corrupted agcount case would allow
xfs_admin to return failure as well, for example.

-Eric

> Brian
> 

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

[Brian Foster]

On Sat, Jan 14, 2017 at 08:59:00AM -0600, Eric Sandeen wrote:
> On 1/14/17 6:53 AM, Brian Foster wrote:
> >> I do think that mere mortal invocations via xfs_admin need to be
> >> handled in this "ignore agcount" case, though...
> >>
> > We're talking about invocations intended to modify things (i.e., version
> > flags, fs uuid, etc.), right? If so, wouldn't we want those things to
> > fail if the superblock is busted?
> 
> Yes - I'm saying that if xfs_db continues with only 1 ag, those tools
> will not work as expected.  (they normally modify all superblocks).
> 

Right..

> setting exitcode to 1 in the corrupted agcount case would allow
> xfs_admin to return failure as well, for example.
> 

Ok, that makes sense to me. I wasn't sure if by "handled," you meant we
wanted those operations to try and do something useful in this
situation.

Brian

> -Eric
> 
> > Brian
> > 

[PATCH 5/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>
---
 repair/sb.c         |   19 ++++++++++++++-----
 repair/xfs_repair.c |    5 ++++-
 2 files changed, 18 insertions(+), 6 deletions(-)


diff --git a/repair/sb.c b/repair/sb.c
index 004702c..d784420 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -395,21 +395,26 @@ 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_inodelog < XFS_DINODE_MIN_LOG ||
+		sb->sb_inodelog > XFS_DINODE_MAX_LOG ||
+		sb->sb_inodelog != libxfs_log2_roundup(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_inopblock != howmany(sb->sb_blocksize,sb->sb_inodesize))
 		return(XR_BAD_INO_SIZE_DATA);
 
+	if (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog)
+		return XR_BAD_INO_SIZE_DATA;
+
 	if (xfs_sb_version_hassector(sb))  {
 
 		/* check to make sure log sector is legal 2^N, 9 <= N <= 15 */
@@ -494,6 +499,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..8d4be83 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -621,7 +621,10 @@ is_multidisk_filesystem(
 	if (!sbp->sb_unit)
 		return false;
 
-	ASSERT(sbp->sb_width);
+	/* Stripe unit but no stripe width?  Something's funny here... */
+	if (!sbp->sb_width)
+		return false;
+
 	return true;
 }
 

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

[Eric Sandeen]

On 1/11/17 9:06 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.
> 
> The superblock field fuzzer (xfs/1301) triggers all these segfaults.

ok, thinking out loud below.  Mostly fine but question at the end.

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/sb.c         |   19 ++++++++++++++-----
>  repair/xfs_repair.c |    5 ++++-
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/repair/sb.c b/repair/sb.c
> index 004702c..d784420 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -395,21 +395,26 @@ 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 so this is just proper macro replacement, all good.

>  		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_inodelog < XFS_DINODE_MIN_LOG ||
> +		sb->sb_inodelog > XFS_DINODE_MAX_LOG ||
> +		sb->sb_inodelog != libxfs_log2_roundup(sb->sb_inodesize))
> +		return XR_BAD_INO_SIZE_DATA;
> +

missing parentheses on the return.  (I kid!)

Ok, we haven't checked out inodesize yet, so if it's wrong, the above
might return XR_BAD_INO_SIZE_DATA even if sb_inodelog is ok.
I suppose that's fine.

hm at some point I wonder if this should more closely match
xfs_mount_validate_sb (or vice versa)

That function does:

            sbp->sb_inodesize != (1 << sbp->sb_inodelog)                ||

which is the reverse I guess.

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

Ok, this doesn't cross-check inodesize vs. inodelog, but I guess that's
already done above.

> +	if (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog)
> +		return XR_BAD_INO_SIZE_DATA;
> +

oh yeah that too.

I do kind of wonder if we shouldn't just match that mount code more
closely, and just do

if (sbp->sb_inodesize < XFS_DINODE_MIN_SIZE                     ||
    sbp->sb_inodesize > XFS_DINODE_MAX_SIZE                     ||
    sbp->sb_inodelog < XFS_DINODE_MIN_LOG                       ||
    sbp->sb_inodelog > XFS_DINODE_MAX_LOG                       ||
    sbp->sb_inodesize != (1 << sbp->sb_inodelog)                ||
    sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE                    ||
    sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog))
	return XR_BAD_INO_SIZE_DATA;

>  	if (xfs_sb_version_hassector(sb))  {
>  
>  		/* check to make sure log sector is legal 2^N, 9 <= N <= 15 */
> @@ -494,6 +499,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;
> +

OK anyway, if any of this fails we go and take a vote from secondaries,
should be fine.

>  	return(XR_OK);
>  }
>  
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 5c79fd9..8d4be83 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -621,7 +621,10 @@ is_multidisk_filesystem(
>  	if (!sbp->sb_unit)
>  		return false;
>  
> -	ASSERT(sbp->sb_width);
> +	/* Stripe unit but no stripe width?  Something's funny here... */
> +	if (!sbp->sb_width)
> +		return false;
> +

verify_sb did already sanitize this...

        /*
         * verify stripe alignment fields if present
         */
        if (xfs_sb_version_hasdalign(sb)) {
                if ((!sb->sb_unit && sb->sb_width) ||
                    (sb->sb_unit && sb->sb_agblocks % sb->sb_unit))
                        return(XR_BAD_SB_UNIT);
                if ((sb->sb_unit && !sb->sb_width) ||
                    (sb->sb_width && sb->sb_unit && sb->sb_width % sb->sb_unit))
                        return(XR_BAD_SB_WIDTH);
        }

so we do this when we start up:

main
    get_sb
        verify_sb
    ...
    is_multidisk_filesystem

by then it should have been ok, what path did you hit this on?

Seems like we shouldn't be making decisions about "is multidisk"
here, we should be saying "bad superblock" somewhere earlier, no?

I mean, bailing with false is fine I /guess/ but I think the ASSERT
implies that we should have already verified that both or none
are set...

-Eric

>  	return true;
>  }
>  
> 
> --
> 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 5/5] xfs_repair: strengthen geometry checks

[Darrick J. Wong]

On Fri, Jan 13, 2017 at 08:13:17PM -0600, Eric Sandeen wrote:
> On 1/11/17 9:06 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.
> > 
> > The superblock field fuzzer (xfs/1301) triggers all these segfaults.
> 
> ok, thinking out loud below.  Mostly fine but question at the end.
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  repair/sb.c         |   19 ++++++++++++++-----
> >  repair/xfs_repair.c |    5 ++++-
> >  2 files changed, 18 insertions(+), 6 deletions(-)
> > 
> > 
> > diff --git a/repair/sb.c b/repair/sb.c
> > index 004702c..d784420 100644
> > --- a/repair/sb.c
> > +++ b/repair/sb.c
> > @@ -395,21 +395,26 @@ 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 so this is just proper macro replacement, all good.
> 
> >  		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_inodelog < XFS_DINODE_MIN_LOG ||
> > +		sb->sb_inodelog > XFS_DINODE_MAX_LOG ||
> > +		sb->sb_inodelog != libxfs_log2_roundup(sb->sb_inodesize))
> > +		return XR_BAD_INO_SIZE_DATA;
> > +
> 
> missing parentheses on the return.  (I kid!)

:P

> Ok, we haven't checked out inodesize yet, so if it's wrong, the above
> might return XR_BAD_INO_SIZE_DATA even if sb_inodelog is ok.
> I suppose that's fine.
> 
> hm at some point I wonder if this should more closely match
> xfs_mount_validate_sb (or vice versa)
> 
> That function does:
> 
>             sbp->sb_inodesize != (1 << sbp->sb_inodelog)                ||
> 
> which is the reverse I guess.
> 
> >  	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);
> 
> Ok, this doesn't cross-check inodesize vs. inodelog, but I guess that's
> already done above.
> 
> > +	if (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog)
> > +		return XR_BAD_INO_SIZE_DATA;
> > +
> 
> oh yeah that too.
> 
> I do kind of wonder if we shouldn't just match that mount code more
> closely, and just do
> 
> if (sbp->sb_inodesize < XFS_DINODE_MIN_SIZE                     ||
>     sbp->sb_inodesize > XFS_DINODE_MAX_SIZE                     ||
>     sbp->sb_inodelog < XFS_DINODE_MIN_LOG                       ||
>     sbp->sb_inodelog > XFS_DINODE_MAX_LOG                       ||
>     sbp->sb_inodesize != (1 << sbp->sb_inodelog)                ||
>     sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE                    ||
>     sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize) ||
>     (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog))
> 	return XR_BAD_INO_SIZE_DATA;

Ick, yes, I'll just replace all that with this.

> >  	if (xfs_sb_version_hassector(sb))  {
> >  
> >  		/* check to make sure log sector is legal 2^N, 9 <= N <= 15 */
> > @@ -494,6 +499,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;
> > +
> 
> OK anyway, if any of this fails we go and take a vote from secondaries,
> should be fine.
> 
> >  	return(XR_OK);
> >  }
> >  
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 5c79fd9..8d4be83 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -621,7 +621,10 @@ is_multidisk_filesystem(
> >  	if (!sbp->sb_unit)
> >  		return false;
> >  
> > -	ASSERT(sbp->sb_width);
> > +	/* Stripe unit but no stripe width?  Something's funny here... */
> > +	if (!sbp->sb_width)
> > +		return false;
> > +
> 
> verify_sb did already sanitize this...
> 
>         /*
>          * verify stripe alignment fields if present
>          */
>         if (xfs_sb_version_hasdalign(sb)) {
>                 if ((!sb->sb_unit && sb->sb_width) ||
>                     (sb->sb_unit && sb->sb_agblocks % sb->sb_unit))
>                         return(XR_BAD_SB_UNIT);
>                 if ((sb->sb_unit && !sb->sb_width) ||
>                     (sb->sb_width && sb->sb_unit && sb->sb_width % sb->sb_unit))
>                         return(XR_BAD_SB_WIDTH);
>         }
> 
> so we do this when we start up:
> 
> main
>     get_sb
>         verify_sb
>     ...
>     is_multidisk_filesystem
> 
> by then it should have been ok, what path did you hit this on?
> 
> Seems like we shouldn't be making decisions about "is multidisk"
> here, we should be saying "bad superblock" somewhere earlier, no?
> 
> I mean, bailing with false is fine I /guess/ but I think the ASSERT
> implies that we should have already verified that both or none
> are set...

Start with a filesystem with sunit = swidth = 0 and !hasdalign.

# xfs_db -x -c 'sb 0' -c 'fuzz -d sb_unit random' /dev/XXX

Note that we /don't/ turn on XFS_SB_VERSION_DALIGNBIT here, so the
verify_sb check doesn't reject the sb.  But then is_multidisk_filesystem
fails to look for hasdalign and just goes ahead and uses sunit/swidth,
triggering the assert.

--D

> 
> -Eric
> 
> >  	return true;
> >  }
> >  
> > 
> > --
> > 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 6/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 db133d7..729ce66 100644
--- a/db/init.c
+++ b/db/init.c
@@ -269,7 +269,6 @@ main(
 	char	**v;
 	int	start_iocur_sp;
 
-	pushfile(stdin);
 	init(argc, argv);
 	start_iocur_sp = iocur_sp;
 
@@ -284,6 +283,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..3255f27 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,13 +162,15 @@ fetchline_internal(void)
 
 	rval = NULL;
 	for (rlen = iscont = 0; ; ) {
-		if (inputstacksize == 1) {
+		if (interactive_input()) {
 			if (iscont)
 				dbprintf("... ");
 			else
 				dbprintf(get_prompt(), progname);
 			fflush(stdin);
 		}
+		if (curinput == NULL)
+			return NULL;
 		if (seenint() ||
 		    (!fgets(buf, sizeof(buf), curinput) &&
 		     ferror(curinput) && seenint())) {
@@ -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;
 }
 

[PATCH 7/5] xfs_repair.8: document dirty log conditions

[Darrick J. Wong]

Add a section describing what is a dirty log, why xfs_repair won't touch
such things, and what one can do to clear the condition and check the
filesystem.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man8/xfs_repair.8 |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/man/man8/xfs_repair.8 b/man/man8/xfs_repair.8
index 1b4d9e3..e4ca88e 100644
--- a/man/man8/xfs_repair.8
+++ b/man/man8/xfs_repair.8
@@ -510,6 +510,25 @@ will return a status of 1 if filesystem corruption was detected and
 0 if no filesystem corruption was detected.
 .B xfs_repair
 run without the \-n option will always return a status code of 0.
+.SH DIRTY LOGS
+Due to the design of the XFS log, a dirty log can only be replayed on a
+machine having the same CPU architecture as the machine which was
+writing to the log.
+xfs_repair cannot replay a dirty log and will return a status code of 2
+when it detects a dirty log.
+.PP
+In this situation, the log can be replayed by mounting and immediately
+unmounting the filesystem on the same class of machine that crashed.
+Please make sure that the machine's hardware is reliable before
+replaying to avoid compounding the problems.
+.PP
+If mounting fails, the log can be erased by running xfs_repair with the
+-L option.
+All metadata updates in progress at the time of the crash will be lost,
+which may cause significant filesystem damage.
+This should
+.B only
+be used as a last resort.
 .SH BUGS
 The filesystem to be checked and repaired must have been
 unmounted cleanly using normal system administration procedures

Re: [PATCH 7/5] xfs_repair.8: document dirty log conditions

[Eric Sandeen]

On 1/12/17 1:34 PM, Darrick J. Wong wrote:
> Add a section describing what is a dirty log, why xfs_repair won't touch
> such things, and what one can do to clear the condition and check the
> filesystem.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks, this seems fine.

<resists urge to nitpick> ;)
<reserves right to last-minute commit mods>  :)

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

> ---
>  man/man8/xfs_repair.8 |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/man/man8/xfs_repair.8 b/man/man8/xfs_repair.8
> index 1b4d9e3..e4ca88e 100644
> --- a/man/man8/xfs_repair.8
> +++ b/man/man8/xfs_repair.8
> @@ -510,6 +510,25 @@ will return a status of 1 if filesystem corruption was detected and
>  0 if no filesystem corruption was detected.
>  .B xfs_repair
>  run without the \-n option will always return a status code of 0.
> +.SH DIRTY LOGS
> +Due to the design of the XFS log, a dirty log can only be replayed on a
> +machine having the same CPU architecture as the machine which was
> +writing to the log.
> +xfs_repair cannot replay a dirty log and will return a status code of 2
> +when it detects a dirty log.
> +.PP
> +In this situation, the log can be replayed by mounting and immediately
> +unmounting the filesystem on the same class of machine that crashed.
> +Please make sure that the machine's hardware is reliable before
> +replaying to avoid compounding the problems.
> +.PP
> +If mounting fails, the log can be erased by running xfs_repair with the
> +-L option.
> +All metadata updates in progress at the time of the crash will be lost,
> +which may cause significant filesystem damage.
> +This should
> +.B only
> +be used as a last resort.
>  .SH BUGS
>  The filesystem to be checked and repaired must have been
>  unmounted cleanly using normal system administration procedures
>