Bug 260375 - NFS server truncates directory cookies to 32-bits
Summary: NFS server truncates directory cookies to 32-bits
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 13.0-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-12-13 03:37 UTC by Alan Somers
Modified: 2022-01-03 05:55 UTC (History)
1 user (show)

See Also:
asomers: mfc-stable13+
asomers: mfc-stable12+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Somers freebsd_committer freebsd_triage 2021-12-13 03:37:48 UTC
VOP_READDIR can return a list of cookies that store seek offsets for the directory entries returned in uio.  Each cookie is a 64-bit value, matching the 64-bit d_off field in struct dirent.  However, The NFS server truncates these cookies to 32-bits.  It happens in both nfsrvd_readdir and nfsrvd_readdirplus.  The relevant lines are these:

*tl++ = 0;
*tl = txdr_unsigned(*cookiep);

That works just fine for UFS, where dirent offsets correspond to byte offsets within a densely packed array.  And it works just fine for ZFS, where dirent offsets seem to come from a 29-bit hash function.  But it fails for the FUSE file system I'm developing, where the dirent offset uses all 64 bits.

According to RFC-1813, the NFSv3 standard, the cookie3 data type is a uint64 variable.  I can't figure out any good reason why the NFS server should truncate it.  It's probably just a legacy thing.  I'll try to come up with a patch.
Comment 1 Rick Macklem freebsd_committer freebsd_triage 2021-12-13 03:54:25 UTC
The last argument to VOP_READDIR() returns the cookies
for a given local file system. The type is "u_long **" and
for some architectures u_long is 32bits (I think?).

Also, for NFSv2, the directory offset cookie is only
32bits. Admittedly, NFSv2 is not used much any more and
it could be 64bits for NFSv3, NFSV4.

Maybe VOP_READDIR() should be revised to return a
"uint64_t **" cookie list?

Do you feel like doing that? It means messing with all
the in tree file systems, at least a little bit.
Comment 2 Alan Somers freebsd_committer freebsd_triage 2021-12-13 04:04:04 UTC
I don't think that changing the signature of VOP_READDIR is necessary to fix the NFS server.  On architectures where u_long is 32-bits, the NFS server will simply 0-extend it to 64-bits before placing it on the wire.

OTOH, ever since the ino64 project added a d_off field to struct dirent, I think the cookies array is redundant.  We could remove it entirely from VOP_READDIR, and just populate the NFS cookie from d_off.  That's 64-bits on all architectures.
Comment 3 Rick Macklem freebsd_committer freebsd_triage 2021-12-13 23:48:08 UTC
I think a mailing list (like freebsd-current@) should be asked.
Personally, I do not see why 32bit arches like i386 should not
be fixed, so that they can return 64bit directory offsets if,
as you say, some fuse file systems need them.

Is fuse usable on 64bit systems only?

It may be true that the cookies can be acquired from d_off,
assuming all exportable file systems are filling in d_off
correctly. I have not looked at it.
Comment 4 commit-hook freebsd_committer freebsd_triage 2021-12-16 03:55:55 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=32fbc5d824f51f97220bc5c61a23e0bf3ff2b470

commit 32fbc5d824f51f97220bc5c61a23e0bf3ff2b470
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2021-12-13 03:57:14 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2021-12-16 03:54:57 +0000

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

    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.

    Also, have better type safety for txdr_hyper.  Turn it into an inline
    function that type-checks its arguments.  Prevents warnings about
    shift-count-overflow.

    PR:             260375
    MFC after:      2 weeks
    Reviewed by:    rmacklem
    Differential Revision: https://reviews.freebsd.org/D33404

 sys/fs/nfs/xdr_subs.h           | 10 ++++++----
 sys/fs/nfsserver/nfs_nfsdport.c | 13 ++++++-------
 sys/fs/nfsserver/nfs_nfsdsubs.c |  6 +++---
 sys/nfs/xdr_subs.h              | 12 +++++++-----
 4 files changed, 22 insertions(+), 19 deletions(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2021-12-16 03:55:55 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=b214fcceacad6b842545150664bd2695c1c2b34f

commit b214fcceacad6b842545150664bd2695c1c2b34f
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2021-12-14 02:37:27 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2021-12-16 03:54:57 +0000

    Change VOP_READDIR's cookies argument to a **uint64_t

    The cookies argument is only used by the NFS server.  NFSv2 defines the
    cookie as 32 bits on the wire, but NFSv3 increased it to 64 bits.  Our
    VOP_READDIR, however, has always defined it as u_long, which is 32 bits
    on some architectures.  Change it to 64 bits on all architectures.  This
    doesn't matter for any in-tree file systems, but it matters for some
    FUSE file systems that use 64-bit directory cookies.

    PR:             260375
    Reviewed by:    rmacklem
    Differential Revision: https://reviews.freebsd.org/D33404

 share/man/man9/VOP_READDIR.9                             |  6 +++---
 sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c |  8 ++++----
 sys/fs/cd9660/cd9660_vnops.c                             |  9 ++++-----
 sys/fs/ext2fs/ext2_lookup.c                              |  2 +-
 sys/fs/fuse/fuse_internal.c                              |  6 +++---
 sys/fs/fuse/fuse_internal.h                              |  4 ++--
 sys/fs/fuse/fuse_vnops.c                                 |  4 ++--
 sys/fs/msdosfs/msdosfs_vnops.c                           |  4 ++--
 sys/fs/nfsserver/nfs_nfsdport.c                          |  4 ++--
 sys/fs/smbfs/smbfs_vnops.c                               |  2 +-
 sys/fs/tmpfs/tmpfs.h                                     |  2 +-
 sys/fs/tmpfs/tmpfs_subr.c                                |  2 +-
 sys/fs/tmpfs/tmpfs_vnops.c                               |  2 +-
 sys/fs/udf/udf_vnops.c                                   |  7 +++----
 sys/fs/unionfs/union_vnops.c                             | 10 +++++-----
 sys/kern/uipc_mqueue.c                                   |  2 +-
 sys/kern/vfs_subr.c                                      |  2 +-
 sys/kern/vnode_if.src                                    |  2 +-
 sys/sys/param.h                                          |  2 +-
 sys/ufs/ufs/ufs_vnops.c                                  |  4 ++--
 20 files changed, 41 insertions(+), 43 deletions(-)
Comment 6 commit-hook freebsd_committer freebsd_triage 2022-01-03 03:26:20 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=8ec62f06143a54dcb917cba4f24cf7c7a614265a

commit 8ec62f06143a54dcb917cba4f24cf7c7a614265a
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2021-12-13 03:57:14 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2022-01-03 03:09:15 +0000

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

    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.

    Also, have better type safety for txdr_hyper.  Turn it into an inline
    function that type-checks its arguments.  Prevents warnings about
    shift-count-overflow.

    PR:             260375
    Reviewed by:    rmacklem
    Differential Revision: https://reviews.freebsd.org/D33404

    (cherry picked from commit 32fbc5d824f51f97220bc5c61a23e0bf3ff2b470)

 sys/fs/nfs/xdr_subs.h           | 10 ++++++----
 sys/fs/nfsserver/nfs_nfsdport.c | 13 ++++++-------
 sys/fs/nfsserver/nfs_nfsdsubs.c |  6 +++---
 sys/nfs/xdr_subs.h              | 12 +++++++-----
 4 files changed, 22 insertions(+), 19 deletions(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2022-01-03 05:44:43 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=374e8226d2252da6421a05313a7ac223fc00c432

commit 374e8226d2252da6421a05313a7ac223fc00c432
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2021-12-13 03:57:14 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2022-01-03 05:37:51 +0000

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

    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.

    Also, have better type safety for txdr_hyper.  Turn it into an inline
    function that type-checks its arguments.  Prevents warnings about
    shift-count-overflow.

    PR:             260375
    Reviewed by:    rmacklem
    Differential Revision: https://reviews.freebsd.org/D33404

    (cherry picked from commit 32fbc5d824f51f97220bc5c61a23e0bf3ff2b470)

 sys/fs/nfs/xdr_subs.h           | 10 ++++++----
 sys/fs/nfsserver/nfs_nfsdport.c | 13 ++++++-------
 sys/fs/nfsserver/nfs_nfsdsubs.c |  6 +++---
 sys/nfs/xdr_subs.h              | 12 +++++++-----
 4 files changed, 22 insertions(+), 19 deletions(-)