svn commit: r246289 - head/sys/ufs/ffs

[Bruce Evans]

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