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

[Bruce Evans]

On Mon, 1 Jul 2013, Pedro F. Giffuni wrote:

> Log:
>  Change i_gen in UFS to an unsigned type.
>
>  Further simplify the i_gen calculation for older disks.
>  Having a zero here is not really a problem and this is more
>  similar to what is done in newfs_random().

This seems to be a minor problem.

> Modified: head/sys/ufs/ffs/ffs_vfsops.c
> ==============================================================================
> --- head/sys/ufs/ffs/ffs_vfsops.c	Mon Jul  1 14:45:03 2013	(r252466)
> +++ head/sys/ufs/ffs/ffs_vfsops.c	Mon Jul  1 14:49:23 2013	(r252467)
> @@ -1791,7 +1791,7 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags
> 	 * already have one. This should only happen on old filesystems.
> 	 */

The comment is wronger than before.  This should (really shouldn't, but
does) happen happens on new file systems under the following conditions:
- if newfs initialized i_gen (other than by bzeroing it) and arc4random()
   returned 0 in newfs.  We used to fix up this case.  In the previous
   version, we failed to fix up this case when arc4random() returns
   UINT32_MAX.  Now we fail to fix it up when arc4random() returns 0.
   The combined chance of failure is 1 in 2**64.
- if newfs didn't initialize i_gen.  We used to fix up this case.  Now
   we fail to fix it up, similarly to the above, except the chance of
   failure when newfs didn't do the initialization is 1 in 2**32.

> 	if (ip->i_gen == 0) {
> -		ip->i_gen = arc4random() + 1;
> +		ip->i_gen = arc4random();
> 		if ((vp->v_mount->mnt_flag & MNT_RDONLY) == 0) {
> 			ip->i_flag |= IN_MODIFIED;
> 			DIP_SET(ip, i_gen, ip->i_gen);

When we fail to fix up i_gen, we keep using i_gen = 0 until the vnode is
recycled.  When the vnode is reused again after recycling it, we execute
the above code again.  Normally it doesn't fail again.  So the inode gets
a new generation number even though the inode wasn't reallocated.

The MNT_RDONLY case gives a similar bug (when ip->i_gen needs fixing).
Now i_gen is normally "fixed" to nonzero, but only for the in-core inode
since the disk inode is read-only.  After recycling, the inode again
usually gets a new inode number although it wasn't reallocated.

ffs_valloc() is more careful to never allocate i_gen = 0 (unless it was
changed recently), but is/was still buggy.  The old version was:

% 	if (ip->i_gen == 0 || ++ip->i_gen == 0)
% 		ip->i_gen = arc4random() / 2 + 1;

It prefers to just add 1, without ever producing 0.  When it wraps
around to 0, it could advance to 1, but it uses arc4random(), apparently
just to share code with the initially-zero case.  It was careful to
actually change i_gen in the wrapping case.  Then i_gen is initially
0xFFFFFFFF; arc4random() might give that value again, so the value
returned by arc4random() was divided by 2 to make it quite different
(at most 0x7FFFFFFF); then 1 was added to keep it away from 0 (this
also depends on dividing by 2).  This had nothing to do with the type
being signed -- arc4random() / 2 + 1 can still overflow the signed
type with a probability of 1 in 2**31, and incrementing i_gen overflows
with a probability of up to 1/2.

I think ffs_valloc() still has the (i_gen == 0 || ++i_gen == 0) logic,
but it doesn't work without the division and addition.

Bruce