nfs: don't truncate directory cookies to 32-bits in the NFS server

[asomers]

Summary

In NFSv2, the directory cookie was 32-bits. NFSv3 widened it to
64-bits and SVN r22521 widened the corresponding argument in
VOP_READDIR, but FreeBSD's NFS server continued to treat the cookies as
32-bits, and 0-extended to fill the field on the wire. Nobody ever
noticed, because every in-tree file system generates cookies that fit
comfortably within 32-bits.

PR: 260375
MFC after: 2 weeks

Test Plan

Manual testing with a fuse file system that uses 64-bit directory offsets, while watching the directory offsets that nfs supplies to VOP_READDIR with dtrace.

[rmacklem]

u_long is 4 bytes on i386. I just checked.
The VOP_READDIR() argument needs to
be changed to uint64_t **.

[asomers]

• Change VOP_READDIR's cookies argument to a **uint64_t

[pauamma_gundo.com]

Language and grammar in the manual page change LGTM and for once (don't get used to it) I could tell they appear consistent with the integer size increase in the source code. I can't attest to anything else, though.

[jhb]

I would perhaps split this into a few commits and follow @mav's advice from the mailing list:

1. Change NFS to send the full long which will fix mounts from 64-bit servers.

2. Fix places to use sizeof(**cookies) instead of hardcoding the type.

3. Change the type from u_long too uint64_t.

Changes 1 + 2 can be MFC'd, only 3 is the one that probably can't be merged.

[asomers]

In D33404#756027, @jhb wrote:

I would perhaps split this into a few commits and follow @mav's advice from the mailing list:

1. Change NFS to send the full long which will fix mounts from 64-bit servers.

2. Fix places to use sizeof(**cookies) instead of hardcoding the type.

3. Change the type from u_long too uint64_t.

Changes 1 + 2 can be MFC'd, only 3 is the one that probably can't be merged.

If change 3 will never be MFCd, then there's no reason to MFC change 2, I would think. The only purpose of Change 2 is to make change like Change 3 easier and less risky.

[mav]

In D33404#756137, @asomers wrote:

If change 3 will never be MFCd, then there's no reason to MFC change 2, I would think. The only purpose of Change 2 is to make change like Change 3 easier and less risky.

I can think of diff reduction between head and stable to not complicate possible later merges. Other than that -- sure, it does not buy much.

[rmacklem]

Just fyi, if you MFC #1, it won't work correctly for 32bit arches.
txdr_hyper() is a macro that assumes the first argument is 64bits.

You could easily fix this in a variety of ways. The cleanest might
be to replace the macro with an inline function that specifies
the first argument as uint64_t, so the compiler takes care of it.
(It is scary to think that this code is sooo old it predates inline functions,;-)

Btw, I am now testing the patch and it works fine sofar.
I'll comment again once I'm done testing.

[asomers]

In D33404#756215, @rmacklem wrote:

Just fyi, if you MFC #1, it won't work correctly for 32bit arches.
txdr_hyper() is a macro that assumes the first argument is 64bits.

You could easily fix this in a variety of ways. The cleanest might
be to replace the macro with an inline function that specifies
the first argument as uint64_t, so the compiler takes care of it.
(It is scary to think that this code is sooo old it predates inline functions,;-)

Btw, I am now testing the patch and it works fine sofar.
I'll comment again once I'm done testing.

Why won't txdr_hyper work on 32-bit arches? It looks to me like it will work, unless the compiler is "smart" enough to warn that right shifting a 32-bit value by 32-bits is useless.

[rmacklem]

Yea, txdr_hyper() does seem to work for i386 and a 32bit first argument,
if you ignore the warning...

warning: shift count >= width of type [-shift-count-overflow]

Still, might be nice to fix it so it doesn't do that, if you do plan on
MFC'ng #1

[rmacklem]

Looks ok to me and I didn't find problems when testing
ufs, msdosfs and cd9660 for i386, amd64 w.r.t. patch.
--> Turns out there is a bug in cd9660, but it predates

this patch. A "ls -R" misses the last entry in the top
level directory when done on the client end of the
mount. I can reproduce this easily and will chase it
someday soon, but the unpatched kernel behaves the
same way, so it isn't this patch.

I agree, as suggested by jhb@, that you might want to
split it into separate patches.

[asomers]

• nfs: better type safety for txdr_hyper

[asomers]

@rmacklem I changed txdr_hyper as you suggested. Before committing I'll squash this with the first. That way there will only be two commits total, one to MFC and one not to.

[rmacklem]

sys/fs/nfs/xdr_subs.h

102

You could take some of the ugly stuff out,
especially on the left side of the assignment.
ie. Just t[0], t[1] should be ok.

Also, there's no need for "f" to be bracketed "(f)".

[asomers]

sys/fs/nfs/xdr_subs.h

102

Oops, I thought I fixed that, but I forgot that I need to fix it in two places.

[rmacklem]

sys/fs/nfs/xdr_subs.h

96–101

Oh, and I don't know if it matters, but
making inline functions "static" seems to
be the tradition.

[asomers]

• fixup to "better type safety for txdr_hpyer"

[asomers]

• Make txdr_hyper "static inline"

[jhb]

One other reason btw for splitting patches (e.g. the 1 vs 2) is that it makes the actual functional changes narrower so that if someone finds a regression in the future and has to bisect there's a potentially smaller change to deal with in identifying the bug.

[rmacklem]

This one looks ok to me.