diff mbox

[4/5] xfs_db: sanitize geometry on load

Message ID 148419041340.32674.2646685552269275817.stgit@birch.djwong.org
State Superseded, archived
Headers show

Commit Message

Darrick J. Wong Jan. 12, 2017, 3:06 a.m. UTC
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(-)



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

Comments

Eric Sandeen Jan. 12, 2017, 2:34 p.m. UTC | #1
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
> 
--
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
Brian Foster Jan. 12, 2017, 3:09 p.m. UTC | #2
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
--
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
Darrick J. Wong Jan. 12, 2017, 8:41 p.m. UTC | #3
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
--
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
diff mbox

Patch

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