cvs commit: src/sys/nfs nfs_vfsops.c

[Bruce Evans]

On Fri, 21 Jan 2005, Peter Wemm wrote:

> peter       2005-01-21 01:23:25 UTC
>
>   FreeBSD src repository
>
>   Modified files:        (Branch: RELENG_4)
>     sys/nfs              nfs_vfsops.c
>   Log:
>   Yet another pass on trying to fix the nfs statfs large-fs blocksize scaler.
>   Casting the result of the 64 bit division to 32 bits (thus discarding the
>   upper 32 bits) and then looking at the truncated result to try and figure
>   out if the untruncated result would fit in 32 bits was utterly useless.
>
>   I am still not sure that it is right, but it has a chance of working now.
>   I'm not at all sure about the sign handling.  NFSv3 only reports positive
>   values here, but correctness of handling the 63/64 bit signs on nfs
>   volumes is not a problem we'll likely have to deal with for some time. I
>   think the "most correct" test is for an unsigned division testing for
>   exceeding LONG_MAX, since we should never end up with a negative number to
>   compare against LONG_MIN.

I sent many mails with better sign handling and many other details,
including ones about the panic implemented in this commit.  The panic
here is just a minor variation of the one in rev.1.133 which was "fixed"
in rev.1.135 using bogus casts to annull buggy code.  Now the modified
casts have no effect so the code is active again.  It was reactivated
in a slightly different way in -current a month or two ago, but has
been backed out there, thus reducing the main bug in this area in
-current to wrong values for negative available space (e.g., 2^55-1
instead of -1 for -1 available blocks).

>   values here, but correctness of handling the 63/64 bit signs on nfs
>   volumes is not a problem we'll likely have to deal with for some time. I
>   think the "most correct" test is for an unsigned division testing for
>   exceeding LONG_MAX, since we should never end up with a negative number to
>   compare against LONG_MIN.

No, negative numbers happen every day for fs_bavail with non-broken servers.
The v3 server has been broken in -current but remains unbroken in RELENG_4.

% Index: nfs_vfsops.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/nfs/Attic/nfs_vfsops.c,v
% retrieving revision 1.91.2.8
% retrieving revision 1.91.2.9
% diff -u -2 -r1.91.2.8 -r1.91.2.9
% --- nfs_vfsops.c	10 Jul 2004 10:28:08 -0000	1.91.2.8
% +++ nfs_vfsops.c	21 Jan 2005 01:23:25 -0000	1.91.2.9
% @@ -294,16 +294,16 @@
%  			sbp->f_bsize = bsize;
%  			tquad = fxdr_hyper(&sfp->sf_tbytes);
% -			if (((long)(tquad / bsize) > LONG_MAX) ||
% -			    ((long)(tquad / bsize) < LONG_MIN))
% +			if (((quad_t)(tquad / bsize) > LONG_MAX) ||
% +			    ((quad_t)(tquad / bsize) < LONG_MIN))
%  				continue;

Casting to quad_t has no effect, since bsize begins at 512 so dividing
by it always reduces tquad far below QUAD_MAX.  The result of the
division alone is always nonnegative so the comparison with LONG_MIN
has no effect either.  Removing the null code and obfuscatory parentheses
gives:

			if (tquad / bsize > LONG_MAX)
				continue;

This is the same as in rev.1.133 except for it not having excessive
parentheses.

A panic can occur due to continuing here, but is unlikely.  If tquad is
nearly 2^64, then there is no value of bsize that can reduce the block
count to less than LONG_MAX on machines with 32-bit longs.  Then
bsize wants to be doubled endlessly, but it first overflows on doubling
of 2^30 and then causes a panic for division by 0 on doubling of 2^31.
But physical file systems cannot have sizes anywhere near 2^64 bytes yet.

My version avoids the panic using explicit bounds checking for bsize.

%  			sbp->f_blocks = tquad / bsize;
%  			tquad = fxdr_hyper(&sfp->sf_fbytes);
% -			if (((long)(tquad / bsize) > LONG_MAX) ||
% -			    ((long)(tquad / bsize) < LONG_MIN))
% +			if (((quad_t)(tquad / bsize) > LONG_MAX) ||
% +			    ((quad_t)(tquad / bsize) < LONG_MIN))
%  				continue;

As above.

%  			sbp->f_bfree = tquad / bsize;
%  			tquad = fxdr_hyper(&sfp->sf_abytes);
% -			if (((long)(tquad / bsize) > LONG_MAX) ||
% -			    ((long)(tquad / bsize) < LONG_MIN))
% +			if (((quad_t)(tquad / bsize) > LONG_MAX) ||
% +			    ((quad_t)(tquad / bsize) < LONG_MIN))
%  				continue;

Now the panic can very easily occur.  Non-broken servers pass negative
available space counts mod 2^64.  Then tquad is 2^64-epsilon, so
tquad / bsize is initially 2^55-epsilon1 and never smaller than
2^33-epsilon2.  So on machines with 32-bit longs, there is no possible
value of bsize which can reduce 2^64-epsilon to less that LONG_MAX,
and doubling bsize causes a panic as above.

Fixing the bugs consists mainly of removing parentheses:

			if ((quad_t)tquad / bsize > LONG_MAX ||
			    (quad_t)tquad / bsize < LONG_MIN)
				continue;

%  			sbp->f_bavail = tquad / bsize;

tquad must also be converted to a signed type before this division too.
In my version, tquad is a quad_t and holds the divided value so that
the division and its ugly cast doesn't have to be repeated for the
range checking, but this is not quite right since tquad really should
be an unsigned type except for handling f_bavail.

Bruce