xfs
[Top] [All Lists]

Re: segment fault when running xfs_check -s

To: xiaowei hu <xiaowei.hu@xxxxxxxxxx>
Subject: Re: segment fault when running xfs_check -s
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 21 Jun 2010 15:38:03 +1000
Cc: xfs@xxxxxxxxxxx, wen.gang.wang@xxxxxxxxxx
In-reply-to: <1277088918.20616.81.camel@redfuture-desktop>
References: <1277088918.20616.81.camel@redfuture-desktop>
User-agent: Mutt/1.5.20 (2009-06-14)
On Mon, Jun 21, 2010 at 10:55:18AM +0800, xiaowei hu wrote:
> Hi,
> 
> Recently I was playing with xfsprogs, and did some test on xfs_check
> tools in this mothod:
> 1.make a xfs fs on one partition.
> mkfs.xfs -f /dev/sda2
> 2.randomly modify some blocks on this volume using xfs_db.
> xfs_db -x -c blockget -c "blocktrash -3 -s 36 -n 81" /dev/sda2
> 3.run xfs_check -s
> xfs_check -s /dev/sda2
> 
> the output saying:
> [root@test1 ~]# xfs_check -s /dev/sda2
> bad version number 0x78 for inode 135
> bad magic number 0xa855 for inode 154
> bad magic number 0x5024 for inode 165
> /usr/local/bin/xfs_check: line 28:   769 Segmentation fault      xfs_db
> $DBOPTS -F -i -p xfs_check -c "blockget$OPTS" $1
> 
> if I run xfs_check without -s option, don't have this fault.
> 
> Is this a known problem?

No.

> I tried the latest code ,also have this problem.

Yeah, the code determining whether to output a message is broken:

2651         if (!isfree) {
2652                 id = find_inode(ino, 1);
2653                 bno = XFS_INO_TO_FSB(mp, ino);
2654                 blkmap = NULL;
2655         }

find_inode() checks a list created by the "-i <ino #>" command line
option, so will always return NULL for this test case. Most of the checks
do this:

2657                 if (!sflag || isfree || id->ilist || CHECK_BLIST(bno))

sflag is set by the "-s" command line option, which is why this
problem requires that to be set - the output checks don't get past
this otherwise.

isfree is a parameter passed into to say whether the inode is
allocated or not. The code hence assumes that if !isfree, then id !=
NULL, which is certainly not the case. Hence it can blow up if id =
NULL.

The line it crashed on:

2670         if (isfree) {
2671                 if (idic.di_nblocks != 0) {
2672  >>>>>>                 if (!sflag || id->ilist || CHECK_BLIST(bno))

Looks like a clear case of copy-n-paste error - if isfree is set, the
id _must_ be null and we go kaboom.

It looks like none of the code in this function actually checks for
whether id is valid or not after find_inode and dereferencing the
reurn pointer.

The patch below should fix the problem.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

---
 db/check.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/db/check.c b/db/check.c
index 7620d9c..e0dcf63 100644
--- a/db/check.c
+++ b/db/check.c
@@ -2619,6 +2619,7 @@ process_inode(
        xfs_qcnt_t              ic = 0;
        xfs_qcnt_t              rc = 0;
        xfs_dqid_t              dqprid;
+       int                     v = 0;
        static char             okfmts[] = {
                0,                              /* type 0 unused */
                1 << XFS_DINODE_FMT_DEV,        /* FIFO */
@@ -2653,15 +2654,16 @@ process_inode(
                bno = XFS_INO_TO_FSB(mp, ino);
                blkmap = NULL;
        }
+       v = (!sflag || (id && id->ilist) || CHECK_BLIST(bno));
        if (idic.di_magic != XFS_DINODE_MAGIC) {
-               if (!sflag || isfree || id->ilist || CHECK_BLIST(bno))
+               if (isfree || v)
                        dbprintf(_("bad magic number %#x for inode %lld\n"),
                                idic.di_magic, ino);
                error++;
                return;
        }
        if (!XFS_DINODE_GOOD_VERSION(idic.di_version)) {
-               if (!sflag || isfree || id->ilist || CHECK_BLIST(bno))
+               if (isfree || v)
                        dbprintf(_("bad version number %#x for inode %lld\n"),
                                idic.di_version, ino);
                error++;
@@ -2669,7 +2671,7 @@ process_inode(
        }
        if (isfree) {
                if (idic.di_nblocks != 0) {
-                       if (!sflag || id->ilist || CHECK_BLIST(bno))
+                       if (v)
                                dbprintf(_("bad nblocks %lld for free inode "
                                         "%lld\n"),
                                        idic.di_nblocks, ino);
@@ -2680,21 +2682,22 @@ process_inode(
                else
                        nlink = idic.di_nlink;
                if (nlink != 0) {
-                       if (!sflag || id->ilist || CHECK_BLIST(bno))
+                       if (v)
                                dbprintf(_("bad nlink %d for free inode 
%lld\n"),
                                        nlink, ino);
                        error++;
                }
                if (idic.di_mode != 0) {
-                       if (!sflag || id->ilist || CHECK_BLIST(bno))
+                       if (v)
                                dbprintf(_("bad mode %#o for free inode 
%lld\n"),
                                        idic.di_mode, ino);
                        error++;
                }
                return;
        }
+
        if (be32_to_cpu(dip->di_next_unlinked) != NULLAGINO) {
-               if (!sflag || isfree || id->ilist || CHECK_BLIST(bno))
+               if (v)
                        dbprintf(_("bad next unlinked %#x for inode %lld\n"),
                                be32_to_cpu(dip->di_next_unlinked), ino);
                error++;
@@ -2704,27 +2707,27 @@ process_inode(
         */
        if ((((idic.di_mode & S_IFMT) >> 12) > 15) ||
            (!(okfmts[(idic.di_mode & S_IFMT) >> 12] & (1 << idic.di_format)))) 
{
-               if (!sflag || id->ilist || CHECK_BLIST(bno))
+               if (v)
                        dbprintf(_("bad format %d for inode %lld type %#o\n"),
                                idic.di_format, id->ino, idic.di_mode & S_IFMT);
                error++;
                return;
        }
        if ((unsigned int)XFS_DFORK_ASIZE(dip, mp) >= XFS_LITINO(mp))  {
-               if (!sflag || id->ilist)
+               if (v)
                        dbprintf(_("bad fork offset %d for inode %lld\n"),
                                idic.di_forkoff, id->ino);
                error++;
                return;
        }
        if ((unsigned int)idic.di_aformat > XFS_DINODE_FMT_BTREE)  {
-               if (!sflag || id->ilist)
+               if (v)
                        dbprintf(_("bad attribute format %d for inode %lld\n"),
                                idic.di_aformat, id->ino);
                error++;
                return;
        }
-       if (verbose || id->ilist || CHECK_BLIST(bno))
+       if (verbose || v)
                dbprintf(_("inode %lld mode %#o fmt %s "
                         "afmt %s "
                         "nex %d anex %d nblk %lld sz %lld%s%s%s%s%s%s%s\n"),
@@ -2844,20 +2847,20 @@ process_inode(
        }
        totblocks = totdblocks + totiblocks + atotdblocks + atotiblocks;
        if (totblocks != idic.di_nblocks) {
-               if (!sflag || id->ilist || CHECK_BLIST(bno))
+               if (v)
                        dbprintf(_("bad nblocks %lld for inode %lld, counted "
                                 "%lld\n"),
                                idic.di_nblocks, id->ino, totblocks);
                error++;
        }
        if (nextents != idic.di_nextents) {
-               if (!sflag || id->ilist || CHECK_BLIST(bno))
+               if (v)
                        dbprintf(_("bad nextents %d for inode %lld, counted 
%d\n"),
                                idic.di_nextents, id->ino, nextents);
                error++;
        }
        if (anextents != idic.di_anextents) {
-               if (!sflag || id->ilist || CHECK_BLIST(bno))
+               if (v)
                        dbprintf(_("bad anextents %d for inode %lld, counted "
                                 "%d\n"),
                                idic.di_anextents, id->ino, anextents);

<Prev in Thread] Current Thread [Next in Thread>