Date: Sun, 1 May 2011 22:04:52 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: rmacklem@freebsd.org, fs@freebsd.org Subject: Re: newnfs client and statfs Message-ID: <20110501184904.S975@besplex.bde.org> In-Reply-To: <149943048.820546.1304211668413.JavaMail.root@erie.cs.uoguelph.ca> References: <149943048.820546.1304211668413.JavaMail.root@erie.cs.uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 30 Apr 2011, Rick Macklem wrote:
> Oops, I never noticed that the "struct statfs" fields had been bumped
> to 64bits. I've attached a patch for the client. Could you please test
> it? (I'll look in case the server has a similar problem.)
Sigh, bugs in this area are very old and still present.
% --- fs/nfsclient/nfs_clport.c.sav 2011-04-30 20:16:39.000000000 -0400
% +++ fs/nfsclient/nfs_clport.c 2011-04-30 20:45:16.000000000 -0400
% @@ -39,6 +39,7 @@ __FBSDID("$FreeBSD: head/sys/fs/nfsclien
% * be the easiest way to handle the port.
% */
% #include <sys/hash.h>
% +#include <sys/limits.h>
Only needed to implement a bug.
% #include <fs/nfs/nfsport.h>
% #include <netinet/if_ether.h>
% #include <net/if_types.h>
% @@ -838,20 +839,14 @@ void
% nfscl_loadsbinfo(struct nfsmount *nmp, struct nfsstatfs *sfp, void *statfs)
% {
% struct statfs *sbp = (struct statfs *)statfs;
% - nfsquad_t tquad;
%
% if (nmp->nm_flag & (NFSMNT_NFSV3 | NFSMNT_NFSV4)) {
% sbp->f_bsize = NFS_FABLKSIZE;
% - tquad.qval = sfp->sf_tbytes;
% - sbp->f_blocks = (long)(tquad.qval / ((u_quad_t)NFS_FABLKSIZE));
% - tquad.qval = sfp->sf_fbytes;
% - sbp->f_bfree = (long)(tquad.qval / ((u_quad_t)NFS_FABLKSIZE));
% - tquad.qval = sfp->sf_abytes;
% - sbp->f_bavail = (long)(tquad.qval / ((u_quad_t)NFS_FABLKSIZE));
% - tquad.qval = sfp->sf_tfiles;
% - sbp->f_files = (tquad.lval[0] & 0x7fffffff);
% - tquad.qval = sfp->sf_ffiles;
% - sbp->f_ffree = (tquad.lval[0] & 0x7fffffff);
This mail is too short to describe all the bugs on the above. The old nfs
client still has the following ones:
- bogus variable tquad
- bogus and broken masking for f_files by 0x7fffffff. v3 can pass us a
count >= 2**31. The bogus masking breaks such counts. When f_files
was only long, we had to do something for values larger than LONG_MAX.
We should have clamped to LONG_MAX. (See cvtstatfs() which does this
now for the corresponding problem for ostatfs().) Instead, we bogusly
cast. 0x7fffffff is just a misspelling of LONG_MAX which happens to
be correct for 32-bit 2's complement longs. That combined with the
server also being limited to 32 bits is the one case where the cast
works as intended, and even then it is quite broken -- it just loses
the top bit of values between 2**31 and 2**32-1. Perhaps the protocol
prohibits such values, but at least FreeBSD servers take null care not
to send them -- see below.
- bogus and even more broken masking for f_ffree. This was broken even
when f_ffree was long and long was 32 bits. Then the mask just
destroys the sign bit, which a non-broken server will have passed us as
the top bit in a 32-bit unsigned value.
% + sbp->f_blocks = sfp->sf_tbytes / NFS_FABLKSIZE;
% + sbp->f_bfree = sfp->sf_fbytes / NFS_FABLKSIZE;
% + sbp->f_bavail = sfp->sf_abytes / NFS_FABLKSIZE;
The conversion for f_bavail still has sign extension bugs. f_bavail
can be negative on the server. A non-broken (FreeBSD) server passes
us this negative value as a uint64_t value with the top bit set. It
will be >= 2**63 as an unsigned value and dividing by NFS_FABSLBKSIZE =
2**9 makes it between 2**54 and 2**55-1; all trace of its signeness is
lost, exccept we know that garbage values in the range 2**54 to 2**55-1
mean this overflow error.
The old nfs client still has this bug.
Old versions of the old nfs client had broken scaling which paniced
trying to use the values near 2**54 given by the above overflow bugs.
See statfs_scale_blocks() for non-broken scaling for the corresponding
problem for ostatfs().
Someone broke the old FreeBSD nfs server to work around the broken FreeBSD
nfs client. It remains broken :-(. This bug is missing in the new nfs
server -- it just passes the server's f_bavail :-), without paying quite
enough attention to the sign bit.
There is of cause a portability problem. We need to export negative
values from FreeBSD servers to FreeBSD clients without breaking other
combinations. The combination of the new nfs server with really old
old nfs clients is broken :-). NetBSD's nfsclient has explicit code
to try to handle this problem. I couldn't see how it could work --
negative values must be passed in some way, and there is nothing better
than passing them as (large) positive values mod 2**64. IIRC, NetBSD
changes the values but this cannot work since it loses info. Hmm,
there are 3 fields to use (f_blocks, f_bfree and f_bavail). These
provide some redundancy, but neither NetBSD's code nor anything that
I could think of worked to recover ffs's negative avail counts from
nonnegative values in these fields, and frobbing these fields would
be unportable anyway. Both the nfs protocol and POSIX's statvfs() (?)
API and types seem to be incapable of handing ffs's negative f_bavail
counts (POSIX only has unsigned block counts...).
% + sbp->f_files = sfp->sf_tfiles;
Now correct, almost. As for the other fields, it tacitly assumes that
the type of the lvalue is larger than the type of the lvalue. Both
happen to be 64 bits here. For the signed fields, there assumption
is strictly incorrect, since the lvalue is 64 bit signed while the
rvalue is 64 bits unsigned. By "not paying quite enough attention to
the sign bit" in the above, I mean that it is tacitly assumed that
we can start with a 64-bit signed value, convert it to u_quad_t
(another type error -- should not assume that u_quad_t is uint64_t),
pass it through the nfs protocol, convert back to int64_t (or forget
to convert back, as above), and recover the original value. This
deserves special care since it abuses the protocol.
% + sbp->f_ffree = (sfp->sf_ffiles & OFF_MAX);
Any masking here is logically wrong, and in practice just destroys the
sign bit, as described above for the 0x7fffffff mask with old 32 bit
systems. Masking with OFF_MAX has additional logic errors. OFF_MAX
is the maximum value for an off_t, but none of the types here has
anything to do with off_t.
% } else if ((nmp->nm_flag & NFSMNT_NFSV4) == 0) {
% sbp->f_bsize = (int32_t)sfp->sf_bsize;
% sbp->f_blocks = (int32_t)sfp->sf_blocks;
I think this is just the v2 case. The old nfs client uses essentially
the same bogus casts. No casts should be used (clamping should be
used), but if we use casts it may be possible to use non-bogus ones.
I think these are just no casts for the unsigned fields but int32_t
for the signed ones. The v2 protocol is limited to 32 bits, and we
can easily represent any 32-bit value since we have 64-bit fields for
the lvalues. We just need to be careful with the sign bit (in the
31th bit of an unsigned value in the sfp fields), but can keep the
31th bit as an unsigned bit without problems now that the statfs fields
are 64 bits. Casting for the unsigned fields now just breaks the value
unnecessarily if the protocol manages to pass the 31th bit as a value
bit for such fields.
Servers should pay even more attention to unrepresentable bits than to
sign bis, but pay considerably less. Both the old and the new nfs server
blindly truncate f_bfree, etc., to 32 bits in the v2 case (except the old
nfs server corrupts negative f_bavail to 0). (For v3, they tacitly assume
that no truncation occurs on conversion to 64 bits.)
The old nfs server also gratuitously breaks the file counts (f_files
etc.) for the v3 case. It should use txdr_hyper(), but uses exdr_unsigned()
plus extra code to lose 32 bits. This is fixed in NetBSD and in the
new nfs server.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110501184904.S975>