newnfs client and statfs

[Rick Macklem]

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

Well, both RFC1813 (NFSv3) and RFC3530 (NFSv4) specify the value on
the wire (sf_abytes) as uint64_t. Therefore a negative value can't
be represented safely and non-FreeBSD clients/servers would be
confused by cheating and putting the negative value on the wire.
(I see you mention this further down.)
The new server is broken in that it does not
check for a negative value. It seems that the best approach for the
server would be to send a 0 when f_bavail < 0. What else can you do
without "cheating" and representing the value in a way that would be
non-interoperable with non-BSD NFS clients?

I agree the above is broken for the case where the high order bit
of sf_abytes is set. How about the following code?

  sbp->f_bavail = (sfp->f_abytes & OFF_MAX) / NFS_FABLKSIZE;

(Yea, I see later in the message that you don't think
 OFF_MAX is the appropriate
 way to represent the largest positive value that can be stored
 in int64_t. As you'll see below, I don't know the correct way to
 express this constant and would be happy to hear how to do it?
 See below for more on this.)

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

Well, as I noted above, all I think can be done is have the server reply
0 for the case where f_bavail is negative. (If the specs don't support
negative values, that's all there is to it, I think?)

> % + 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.
> 
Well, there are a LOT of places where the code uses u_quad_t to represent
what is now uint64_t. What can I say. I wrote this code over about 10years
(based on even much older code) and, being an old K&R C guy assumed that
u_quad_t was the way to declare an unsigned 64bit value. If/when u_quad_t
doesn't define an unsigned 64bit value like uint64_t does, I'll need a
lot of warning, because I have a LOT of editting to do.

Other than that, the RFCs specify sf_tfiles as uint64_t and
"struct statfs" has f_files as a uint64_t. So, unless there are plans to
make it signed on FreeBSD, I don't see a problem here?

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

Ok, sf_ffiles is defined as uint64_t on the wire. Therefore there is
no sign bit. The problem is that it could be a larger positive value
than FreeBSD supports. All I wanted this code to do is make it the
largest positive value that will fit in int64_t. (I used OFF_MAX
because you suggested in a previous email that that was preferable
to 0x7fffffffffffffffLLU for nm_maxfilesize. I don't see anything
like INT64_MAX, UINT64_MAX in FreeBSD's limits.h)
Would

   if (sfp->sf_ffiles > UINT64_MAX)
       sbp->f_ffree = INT64_MAX;
   else
       sbp->f_ffree = sfp->sf_ffiles;

- except there isn't a UINT64_MAX, INT64_MAX defined in sys/*.h as
  far as I can see. How do I express these constants? Do I have to
  convert 0x7ffffffffffffff to decimal and use that?

> % } 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.
> 
Ok, I could take the casts off. I think the effect would be that, for the
case where sf_bavail has its high order (bit 31) set, it will be seen as
a larger positive value. (sf_bavail is u_int32_t) This would be correct
per the RFCs, since RFC1094 defines the fields as uint32_t. Now, if
servers were "cheating" and putting the negative values in the field on
the wire, it will change the semantics a bit.

I'll admit I tend to feel that the safest thing is to just leave it
the way it is, since no one is complaining about the semantics and I'd
rather not "break" anything by fixing the semantics to agree with thr RFC.

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

As above, I have to disagree with this. If the RFCs say it can't be
negative, then sending negative values as 0 is all that can be done,
as far as I can see. (I think the old server got this case correct
and the new server needs to be fixed.)

> (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.
> 
At this point, I like to leave the old server unchanged. That way, if
anyone runs into problems w.r.t. differences in semantics (even if they
seem to violate the RFC), they can switch to the old server while things
get sorted out.

rick