public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* segment fault when running xfs_check -s
@ 2010-06-21  2:55 xiaowei hu
  2010-06-21  5:38 ` Dave Chinner
  0 siblings, 1 reply; 3+ messages in thread
From: xiaowei hu @ 2010-06-21  2:55 UTC (permalink / raw)
  To: xfs; +Cc: wen.gang.wang

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?

I tried the latest code ,also have this problem.


thanks,
Xiaowei



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: segment fault when running xfs_check -s
  2010-06-21  2:55 segment fault when running xfs_check -s xiaowei hu
@ 2010-06-21  5:38 ` Dave Chinner
  2010-06-22  7:17   ` xiaowei hu
  0 siblings, 1 reply; 3+ messages in thread
From: Dave Chinner @ 2010-06-21  5:38 UTC (permalink / raw)
  To: xiaowei hu; +Cc: wen.gang.wang, xfs

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@fromorbit.com

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: segment fault when running xfs_check -s
  2010-06-21  5:38 ` Dave Chinner
@ 2010-06-22  7:17   ` xiaowei hu
  0 siblings, 0 replies; 3+ messages in thread
From: xiaowei hu @ 2010-06-22  7:17 UTC (permalink / raw)
  To: Dave Chinner; +Cc: wen.gang.wang, xfs

Hi Dave,

I tried this patch,it fixes this bug, thank you very much:)

Regards,
Xiaowei



On Mon, 2010-06-21 at 15:38 +1000, Dave Chinner wrote:
> 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@fromorbit.com
> 
> ---
>  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);


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-06-22  7:15 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-21  2:55 segment fault when running xfs_check -s xiaowei hu
2010-06-21  5:38 ` Dave Chinner
2010-06-22  7:17   ` xiaowei hu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox