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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
