Bug 18874
| Summary: | [2TB] 32bit NFS servers export wrong negative values to 64bit clients | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | alex <alex> | ||||
| Component: | kern | Assignee: | freebsd-fs (Nobody) <fs> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | CC: | ngie, rmacklem | ||||
| Priority: | Normal | ||||||
| Version: | 5.0-CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
|
Description
alex
2000-05-29 16:30:00 UTC
You, alex@big.endian.de, were spotted writing this on Mon, May 29, 2000 at 05:24:23PM +0200: > NFS Server wants to export that fs: > /dev/ad0s4g 2482878 2361105 -76857 103% /usr > > NFS client sees: > neutron:/usr/obj 2482878 2361105 18014398509405127 0% /usr/obj Gotta love those signed<-->unsigned bugs ;) This seemed interesting enough for me to rummage around in sources, but I don't really have a working setup with Alpha NFS to test this against. I wonder if you could try this patch, which seems to do The Right Thing to me. I won't be surprised if it fails to work at all though :) A brief explanation: tquad is 64-bit unsigned, and holds the available bytes value which is really signed long. Suppose the real value is negative. If we divide it by NFS_FABLKSIZE first, and then cast to long, we risk turning it into a positive value by division. --- /usr/src/sys/nfs/nfs_vfsops.c Sun Aug 29 12:30:31 1999 +++ nfs_vfsops.c Mon May 29 13:08:50 2000 @@ -297,7 +297,7 @@ fxdr_hyper(&sfp->sf_fbytes, &tquad); sbp->f_bfree = (long)(tquad / ((u_quad_t)NFS_FABLKSIZE)); fxdr_hyper(&sfp->sf_abytes, &tquad); - sbp->f_bavail = (long)(tquad / ((u_quad_t)NFS_FABLKSIZE)); + sbp->f_bavail = ((long)tquad) / (long)NFS_FABLKSIZE); sbp->f_files = (fxdr_unsigned(int32_t, sfp->sf_tfiles.nfsuquad[1]) & 0x7fffffff); sbp->f_ffree = (fxdr_unsigned(int32_t, -- Anatoly Vorobey, mellon@pobox.com http://pobox.com/~mellon/ "Angels can fly because they take themselves lightly" - G.K.Chesterton Thus spake Anatoly Vorobey (mellon@pobox.com): > Gotta love those signed<-->unsigned bugs ;) Isn't that 32bit signed vs. 64bit signed? > but I don't really have a working setup with Alpha NFS to test this against. On the client, I guess? (the patch). Will it break the i386 nfs? > A brief explanation: tquad is 64-bit unsigned, and holds the available > bytes value which is really signed long. Suppose the real value is negative. > If we divide it by NFS_FABLKSIZE first, and then > cast to long, we risk turning it into a positive value by division. Yes, but what about i386, where long is only 32bit and can't hold that value? Alex -- I need a new ~/.sig. You, Alexander Langer, were spotted writing this on Mon, May 29, 2000 at 07:21:40PM +0200: > Thus spake Anatoly Vorobey (mellon@pobox.com): > > > Gotta love those signed<-->unsigned bugs ;) > > Isn't that 32bit signed vs. 64bit signed? I thought that was the case at first too, but it doesn't appear so. NFS transfers available values for statfs as 32-bit unsigned in v2, and 64-bit unsigned in v3, regardless of platforms it runs on (your setup uses NFSv3, right?). See the nfs_statfs structure in sys/nfs/nfsproto.h . Now the bug says that either the server puts the value in nfs_statfs incorrectly (maybe, but I couldn't find any fault with the sources in nfs_serv.c), or the client takes it out incorrectly (I think that's likely, hence the attached patch), or the xdr conversion routines work incorrectly for quad values (very unlikely, would've broken lots of things in NFS). > > but I don't really have a working setup with Alpha NFS to test this against. > > On the client, I guess? (the patch). Will it break the i386 nfs? Yes, on the client, and no, shouldn't break anything at all. > > A brief explanation: tquad is 64-bit unsigned, and holds the available > > bytes value which is really signed long. Suppose the real value is negative. > > If we divide it by NFS_FABLKSIZE first, and then > > cast to long, we risk turning it into a positive value by division. > > Yes, but what about i386, where long is only 32bit and can't hold that > value? Hmm. I haven't thought of that. A simple test I made on i386 showed that (long)(u_quad_t)(long)(-100) == -100, so I guess the conversion works fine, even though long is 32bit, and I can't really explain why at the moment :) If you want to be extra sure, you can cast tquad to quad_t before casting it to long. Then it'll hold the real negative value which must be correctly converted into long. I know that you can point out that Alpha's long was bigger than i386's long -- but there's not much you can do about *that*, since you have to start from struct statfs on Alpha and finish with struct statfs on i386, and f_bavail is long on both. You'll just have to hope that the underlying filesystem restricts this value to 32 bits or something. -- Anatoly Vorobey, mellon@pobox.com http://pobox.com/~mellon/ "Angels can fly because they take themselves lightly" - G.K.Chesterton Thus spake Anatoly Vorobey (mellon@pobox.com): > it to long. Then it'll hold the real negative value which must be correctly > converted into long. I know that you can point out that Alpha's long was > bigger than i386's long -- but there's not much you can do about *that*, > since you have to start from struct statfs on Alpha and finish with struct > statfs on i386, and f_bavail is long on both. You'll just have to hope that > the underlying filesystem restricts this value to 32 bits or something. What about taking quad_t and not u_quat_t? I think that would be _much_ more appreciated, isn't it? (for the conversion) Alex -- I need a new ~/.sig. You, Anatoly Vorobey, were spotted writing this on Mon, May 29, 2000 at 01:34:46PM -0400: > Hmm. I haven't thought of that. A simple test I made on i386 showed > that (long)(u_quad_t)(long)(-100) == -100, so I guess the conversion > works fine, even though long is 32bit, and I can't really explain why > at the moment :) Can't I? I can ;) If the negative value is small enough to fit into 32bit, then it'll get converted via u_quad_t just fine, because the high quadword will only contain 1's. The conversion from u_quad_t to long will just chop off the high word: Scenario A. Source long is 32-bit, destination long is 32-bit: (src long)-10 is 0xfffffff6 (u_quad_t)(src long)-10 is 0x00000000fffffff6 (dst long)(u_quad_t)(src long)-10 is 0xfffffff6 == -10 Senario B. Source long is 64-bit, destination long is 32-bit: (src long)-10 is 0xfffffffffffffff6 (u_quad_t)(dst long)-10 is 0xfffffffffffffff6 (dst long)(u_quad_t)(src long)-10 is 0xfffffff6 == -10 Scenario C. Source long is 32-bit, destination long is 64-bit: (src long)-10 is 0xfffffff6 (u_quad_t)(src long)-10 is 0x00000000fffffff6 (dst long)(u_quad_t)(src long)-10 is 0x00000000fffffff6 == 4294967286 Scenario D. Source long is 64-bit, destination long is 64-bit, left to the reader ;) Bummer, scenario C shouldn't work at all -- can you check? But this isn't your case, your case is scenario B IIRC -- and the conversion should work fine, my patch corrects the problem which might happen because the value gets divided before the third stage, when the compiler still thinks it's unsigned 64bit. -- Anatoly Vorobey, mellon@pobox.com http://pobox.com/~mellon/ "Angels can fly because they take themselves lightly" - G.K.Chesterton Thus spake Anatoly Vorobey (mellon@pobox.com): > Bummer, scenario C shouldn't work at all -- can you check? But this > isn't your case, your case is scenario B IIRC -- and the conversion > should work fine, my patch corrects the problem which might happen > because the value gets divided before the third stage, when the compiler > still thinks it's unsigned 64bit. I'll test soon. At the moment I'm building world. I'll also try the quad_t cast instead of the long cast. Thanks anyways. Alex -- I need a new ~/.sig. You, Alexander Langer, were spotted writing this on Mon, May 29, 2000 at 07:48:21PM +0200: > Thus spake Anatoly Vorobey (mellon@pobox.com): > > > it to long. Then it'll hold the real negative value which must be correctly > > converted into long. I know that you can point out that Alpha's long was > > bigger than i386's long -- but there's not much you can do about *that*, > > since you have to start from struct statfs on Alpha and finish with struct > > statfs on i386, and f_bavail is long on both. You'll just have to hope that > > the underlying filesystem restricts this value to 32 bits or something. > > What about taking quad_t and not u_quat_t? > > I think that would be _much_ more appreciated, isn't it? (for the > conversion) NFS seems to transfer everything unsigned, and I think it's a reasonable decision. It gets cast back to signed on the receiving end if needed. I think that casting to quad_t *before* casting to u_quad_t would solve the problem I've outlined as Scenario C in my previous mail. That change would have to be inserted in nfs_serv.c, in the server nfs_statfs() call. But I haven't even verified that the problem exists yet (I've no access to any Alphas anyway). I don't see how using quad_t would help in *your* case, on the other hand. -- Anatoly Vorobey, mellon@pobox.com http://pobox.com/~mellon/ "Angels can fly because they take themselves lightly" - G.K.Chesterton Thus spake Anatoly Vorobey (mellon@pobox.com): > But I haven't even verified that the problem exists yet (I've no access > to any Alphas anyway). I don't see how using quad_t would help in > *your* case, on the other hand. Not in my case, but the i386 case. Since long is only 32bit, you need to cast that to 64bit signed in order to work. As I said, I'll try. Alex -- I need a new ~/.sig. State Changed From-To: open->feedback Does this still happen in recent STABLEs? Responsible Changed From-To: freebsd-bugs->peter I'll finish this if it isn't already done. Here's a routine to sign-extend a 32-bit number to a 64-bit number.
long long sgn_ex_3264(long old_num) /*Prototype: long long sgn_ex_3264(long)*/
{
long tmp_num=old_num;
long long tmp_num_2=0LL;
if(tmp_num & 0x80000000L)
{
tmp_num_2=0xffffffff00000000LL | ((long long)old_num)
}
else
{
tmp_num_2=(long long)old_num
}
return(tmp_num_2);
}
Responsible Changed From-To: peter->tjr tjr has a patch for this Tim, any news on this one? -- Andre Andre Oppermann wrote: > Tim, > > any news on this one? > Not really - I don't have the hardware to test this on. I've attached my best guess at a patch, but I haven't even compile-tested it yet. Tim Thus spake Tim Robbins (tjr@freebsd.org): > Not really - I don't have the hardware to test this on. I've attached my > best guess at a patch, but I haven't even compile-tested it yet. I might give you an account on my Alpha once I'm back in town to test this. Alex State Changed From-To: feedback->suspended Suspended until I can find a way to test patches. State Changed From-To: suspended->patched Fixed in -current by mux@. Responsible Changed From-To: tjr->freebsd-bugs I am no longer working on this. On Fri, 23 Apr 2004, Tim J. Robbins wrote: > Synopsis: 32bit NFS servers export wrong negative values to 64bit clients > > State-Changed-From-To: suspended->patched > State-Changed-By: tjr > State-Changed-When: Fri Apr 23 06:15:28 PDT 2004 > State-Changed-Why: > Fixed in -current by mux@. The fix is wrong, since it breaks sending negative block counts to clients which understand them. BSD clients have supported this since at least 4.4BSD, but the code is broken for the nfsv3 case. This is fixed for small negative values in NetBSD by casting to a signed type as discussed in the followup to this PR. Sign extension bugs are closely related to overflow bugs here and elsewhere. Overflow occurs for block counts that represent byte counts >= 1TB (PR 56606). FreeBSD has wrong fixes for this. See the PR followup for more details. The bugs have now mostly moved to cvtstatfs(). They now also affect callers of the old statfs() family. Overflow problems in nfs_statfs() should have gone away, since 64-bit byte counts easily fit in 64-bit block counters after dividing by the 512. However, nfs still thinks that block counters are longs and it makes messes trying to get the 64-bit counts to fit in possibly-32-bit longs. See the followup to PR 56606 for a hopefully working version. This code needs to move to RELENG_4's nfs_statfs() and -current's cvtstatfs(). cvstatfs() is simple and broken. It attempts to truncate large values but has some sign bugs: % static void % cvtstatfs(td, nsp, osp) % struct thread *td; % struct statfs *nsp; % struct ostatfs *osp; % { % % bzero(osp, sizeof(*osp)); % osp->f_bsize = MIN(nsp->f_bsize, LONG_MAX); % osp->f_iosize = MIN(nsp->f_iosize, LONG_MAX); % osp->f_blocks = MIN(nsp->f_blocks, LONG_MAX); % osp->f_bfree = MIN(nsp->f_bfree, LONG_MAX); % osp->f_bavail = MIN(nsp->f_bavail, LONG_MAX); % osp->f_files = MIN(nsp->f_files, LONG_MAX); % osp->f_ffree = MIN(nsp->f_ffree, LONG_MAX); Mount of the fields in the new statfs struct are (bogusly) unsigned where they used to be signed. The clamps work for these. However, f_bavail and f_free are signed, so the above assignments to their "old" values overflow if their "new" values are < LONG_MIN. Bruce State Changed From-To: patched->closed thishad been done ages ago. State Changed From-To: closed->open Bruce reported that I was wrong, reopen the ticket (Thanks Bruce) Responsible Changed From-To: freebsd-bugs->freebsd-fs Over to maintainer(s). Rick: is this still an issue? The NFSv3 and NFSv4 RFCs specify the field as 64bit unsigned on the wire. To "cheat" and put a negative value in it will break non-BSD clients like Solaris. The new/current FreeBSD server checks for a negative value for f_bavail and puts 0 on the wire if it is negative. The new/current FreeBSD client divides the unsigned 64bit value off the wire by NFS_FABLKSIZE before assigning it to the 64bit signed f_bavail, so it can never be negative (because the unsigned value fits in 63bits after the divide). I think this can be closed, rick Since this cannot happen with the new NFS client and server, I figure it can be closed. |
