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

[Pedro Giffuni]

(Moving the discussion to freebsd-fs)

Hello;


----- Messaggio originale -----
> Da: Bruce Evans 
> 
...
>>  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.
> 

The comparison is perfectly legal for UFS1 so perhaps
gcc gives the "benefit of the doubt" to avoid false positives.

>>>    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.
> 

Surely not anywhere near a complete solution but perhaps it wouldn't
be incompatible to change i_blocks and friends to be unsigned in UFS1.
That is something that remains to be completed in ext2fs, but according
to fsx there are bigger problems there at this time.

Pedro.