Date: Mon, 4 Feb 2013 06:59:19 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andriy Gapon <avg@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Kirk McKusick <mckusick@FreeBSD.org> Subject: Re: svn commit: r246289 - head/sys/ufs/ffs Message-ID: <20130204062149.U2673@besplex.bde.org> In-Reply-To: <510E9D47.2030403@FreeBSD.org> References: <201302031716.r13HGXNP060303@svn.freebsd.org> <510E9D47.2030403@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 3 Feb 2013, Andriy Gapon wrote: > on 03/02/2013 19:16 Kirk McKusick said the following: >> Author: mckusick >> Date: Sun Feb 3 17:16:32 2013 >> New Revision: 246289 >> URL: http://svnweb.freebsd.org/changeset/base/246289 >> >> Log: >> For UFS2 i_blocks is unsigned. The current "sanity" check that it >> has gone below zero after the blocks in its inode are freed is a >> no-op which the compiler fails to warn about because of the use of >> the DIP macro. Change the sanity check to compare the number of > > Just a note that clang actually warned about this one. > It has a few more similar warnings for ufs/ffs code. I wondered how the DIP macro hid the warning. >> blocks being freed against the value i_blocks. If the number of >> blocks being freed exceeds i_blocks, just set i_blocks to zero. >> >> Reported by: Pedro Giffuni (pfg@) >> MFC after: 2 weeks Perhaps the larger bugs pointed to this warning were lost in translation: - di_blocks overflows for ffs1. This is now physically possible. di_blocks has type int32_t, as required to match st_blocks in the old struct stat (both will overflow at the same point). Since di_blocks counts DEV_BSIZE-blocks, overflow occurs at file size about 1TB for files without many holes. - st_blocks overflows for the old stat() syscall. For the new stat() syscall, st_blocks has type blkcnt_t = int64_t, so overflow is not physically possible. But cvtstat() silently overflows when the 64-bit st_blocks doesn't fit in the 32-bit one. - di_blocks for ffs2 has type uint64_t. This has a sign mismatch with both the ffs1 di_blocks and blkcnt_t. blkcnt_t is specified to be signed. i_blocks for ffs has the same type as di_blocks for ffs2 (unsigned ...), so it is mismatched too. The sign mismatches should cause more compiler warnings. These would point to further overflow possibilities. However, overflow is physically impossible even for va_bytes = i_blocks * DEV_BSIZE. So there are no extra overflows from the sign mismatches. Using the unsigned types just asks for more sign extension bugs like the one fixed here. ext2fs has the same bug. It is unnecessary in ext2fs, since there are no fragments so i_blocks can just count in units of fs blocks and these are naturally limited by the fs block type. The fs block type is uint32_t in ext2fs and int32_t in ffs1. So ext2fs really needs an unsigned type for i_blocks to get an extra bit without going to 64 bits. shell-init: could not get current directory: No such file or directory. Avoiding overflow depends on st_blocks having more than 32+9 value bits, so 32-bit stat() could still overflow and needs a clamp. However, the disk format is to store i_count in DEV_BSIZE units, so this doesn't work -- it has the same overflow problem as cvtstat(), at 32 bits instead of 31. Limiting the file size to ~1TB is not a good fix for this, since 1TB non-sparse files are not very common or useful, and the limit would also apply to sparse files. So block allocators should check that i_blocks won't overflow before increasing it. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130204062149.U2673>