Date: Tue, 17 Dec 2002 16:55:11 -0800 From: Kirk McKusick <mckusick@beastie.mckusick.com> To: Bruce Evans <bde@zeta.org.au> Cc: Julian Elischer <julian@elischer.org>, Kris Kennaway <kris@obsecurity.org>, Robert Watson <rwatson@tislabs.com>, fs@FreeBSD.ORG Subject: Re:panic: ffs_vfree: range: dev = ad4s1c, ino = -1690809896, fs = /mnt2 Message-ID: <200212180055.gBI0tB59018624@beastie.mckusick.com> In-Reply-To: Your message of "Fri, 06 Dec 2002 20:53:55 %2B1100." <20021206201556.B8366-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Now that the 6.0 trunk has defrosted, I have made your suggested change.
Kirk McKusick
=-=-=-=-=
Date: Fri, 6 Dec 2002 20:53:55 +1100 (EST)
From: Bruce Evans <bde@zeta.org.au>
To: Julian Elischer <julian@elischer.org>
cc: Kirk McKusick <mckusick@beastie.mckusick.com>,
Kris Kennaway <kris@obsecurity.org>, Robert Watson <rwatson@tislabs.com>,
<fs@FreeBSD.ORG>
Subject: Re: panic: ffs_vfree: range: dev = ad4s1c, ino = -1690809896, fs =
/mnt2
In-Reply-To: <Pine.BSF.4.21.0212052133000.38887-100000@InterJet.elischer.org>
X-ASK-Info: Whitelist match
On Thu, 5 Dec 2002, Julian Elischer wrote:
> This is one of the reasons that I think such fields should all be
> defined as unsigned to start with..
This is actually an example of why naive conversion to use unsigned is
usually wrong. The bug seems to have nothing to do with signedness,
except for a bug in the debugging message.
> > Well it is not at all clear how the dirpref routine came up with
> > such an out of whack inode preference (2604157400 when the filesystem
> > has only 3538944 inodes), but the following fix should catch it and
> > make it harmless. I have submitted the patch to release engineering.
> >
> > Kirk McKusick
> >
> > =-=-=-=-=
> >
> > Index: ffs_alloc.c
> > ===================================================================
> > RCS file: /usr/ncvs/src/sys/ufs/ffs/ffs_alloc.c,v
> > retrieving revision 1.102
> > diff -c -r1.102 ffs_alloc.c
> > *** ffs_alloc.c 2002/09/19 03:55:30 1.102
> > --- ffs_alloc.c 2002/12/06 01:15:50
> > ***************
> > *** 841,847 ****
> > ipref = ffs_dirpref(pip);
> > else
> > ipref = pip->i_number;
> > ! if (ipref >= fs->fs_ncg * fs->fs_ipg)
> > ipref = 0;
> > cg = ino_to_cg(fs, ipref);
> > /*
> > --- 841,847 ----
> > ipref = ffs_dirpref(pip);
> > else
> > ipref = pip->i_number;
> > ! if ((unsigned)ipref >= fs->fs_ncg * fs->fs_ipg)
> > ipref = 0;
> > cg = ino_to_cg(fs, ipref);
> > /*
> >
This change has no effect on any supported system, since `ipref' is
already an unsigned type (ino_t == uint32_t), which is in fact identical
with unsigned on all supported systems.
The analysis is slightly more complicated on exotic systems. Many
things are broken on systems with ints smaller than 32 bits, and the
change increases the breakage by clipping the LHS (if the filesystem
has more than UINT_MAX inodes). The change causes at most slightly
different signed-versus unsigned comparision warnings on systems with
ints larger than 32 bits. The RHS multiplies 2 int32_t's and the
multiplication may in theory overflow, but newfs presumably won't
create filesystems for which it overflows (ones with more than 2^31-1
inodes), so there should be no signedness problems in practice.
Related cosmetic bugs:
(1) `unsigned' is not correctly misspelled u_int.
(2) the original panic message has a wrong function name and a wrong printf
format for printing ino_t's.
Te following patch fixes (2) and a nearby non-misspelling of `unsigned'
(the only other one in ffs_alloc.c).
%%%
Index: ffs_alloc.c
===================================================================
RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_alloc.c,v
retrieving revision 1.103
diff -u -2 -r1.103 ffs_alloc.c
--- ffs_alloc.c 6 Dec 2002 02:08:46 -0000 1.103
+++ ffs_alloc.c 6 Dec 2002 09:13:26 -0000
@@ -1649,5 +1683,5 @@
*/
static int
-ffs_isfreeblock(struct fs *fs, unsigned char *cp, ufs1_daddr_t h)
+ffs_isfreeblock(struct fs *fs, u_char *cp, ufs1_daddr_t h)
{
@@ -1897,6 +1931,6 @@
}
if ((u_int)ino >= fs->fs_ipg * fs->fs_ncg)
- panic("ffs_vfree: range: dev = %s, ino = %d, fs = %s",
- devtoname(dev), ino, fs->fs_fsmnt);
+ panic("ffs_freefile: range: dev = %s, ino = %lu, fs = %s",
+ devtoname(dev), (u_long)ino, fs->fs_fsmnt);
if ((error = bread(devvp, cgbno, (int)fs->fs_cgsize, NOCRED, &bp))) {
brelse(bp);
@@ -1916,5 +1950,5 @@
(u_long)ino + cg * fs->fs_ipg, fs->fs_fsmnt);
if (fs->fs_ronly == 0)
- panic("ffs_vfree: freeing free inode");
+ panic("ffs_freefile: freeing free inode");
}
clrbit(inosused, ino);
%%%
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-fs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200212180055.gBI0tB59018624>