Bug 251035

Summary: ZFS: Allow 64 bit ZFS to support 32 bit ioctls (Wine)
Product: Base System Reporter: Damjan Jovanovic <damjan.jov>
Component: kernAssignee: freebsd-fs (Nobody) <fs>
Status: Open ---    
Severity: Affects Some People CC: allanjude, grahamperrin, jrtc27
Priority: --- Keywords: needs-qa
Version: 12.2-RELEASEFlags: koobs: maintainer-feedback? (allanjude)
koobs: mfc-stable13?
koobs: mfc-stable12?
Hardware: Any   
OS: Any   
Attachments:
Description Flags
convert 32 bit zfs_iocparm_t to 64 bit none

Description Damjan Jovanovic 2020-11-11 06:35:59 UTC 
Created attachment 219540 [details]
convert 32 bit zfs_iocparm_t to 64 bit

When 32 bit applications use libzfs to access ZFS data, they always fail in zfs_open(), with 32 bit "zfs list" and "zpool list" crashing.

This is because /usr/src/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c function zfsdev_ioctl() checks for:

---snip---
if (len != sizeof(zfs_iocparm_t)) {
---snip---

On amd64, sizeof(zfs_iocparm_t) is 24, as in /usr/src/sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.h we have:

---snip---
typedef struct zfs_iocparm {
        uint32_t        zfs_ioctl_version;
        uint64_t        zfs_cmd;
        uint64_t        zfs_cmd_size;
} zfs_iocparm_t;
---snip---

which has its fields aligned to 8 byte boundaries. On i386, they are aligned to 4 byte boundaries, sizeof(zfs_iocparm_t) is only 20, and every ioctl fails with EINVAL.

This affects Wine as it runs many 32 bit apps. Since Windows is case-insensitive, Wine has to slowly scan the entire directory when doing file I/O on a case-sensitive filesystem, to match filenames case-insensitively. ZFS can create filesystems with casesensitivity=insensitive, allowing Wine to skip the slow scan and speeding up file I/O considerably for some apps. When developing a patch to Wine to check that casesensitivity property and use it when insensitive (https://source.winehq.org/patches/data/195726) I noticed zfs_open() always fails, which led me to this bug.

The attached patch correctly converts the 32 bit zfs_iocparm_t to 64 bit, and allows at least Wine to work correctly. I haven't yet had a chance to test whether it fixes 32 bit versions of the "zpool" and "zfs" tools (which probably use more elaborate ioctls).
Comment 1 Andriy Gapon freebsd_committer freebsd_triage 2020-11-11 08:09:44 UTC 
zfs-devel@ is disused.
Comment 2 Damjan Jovanovic 2020-11-12 05:18:42 UTC 
With further tests, I see that 32 bit "zpool" and "zfs" commands go from crashing to fully working with this patch. I can successfully zpool create, zpool destroy, zpool list, zpool status, zfs list, zfs create, zfs get all, etc.
Comment 3 Kubilay Kocak freebsd_committer freebsd_triage 2021-08-14 02:26:41 UTC 
Is this something that should go upstream (OpenZFS) ?
Comment 4 Damjan Jovanovic 2021-08-17 00:55:39 UTC 
(In reply to Kubilay Kocak from comment #3)

That code has changed a lot in FreeBSD 13 and upstream.

32 bit "zpool list" still crashes, as per truss:

21250: openat(AT_FDCWD,"/dev/zfs",O_RDWR,00)	 = 5 (0x5)
21250: ioctl(3,0xc0145a04 { IORW 0x5a('Z'), 4, 20 },0xffffb338) ERR#22 'Invalid argument'

where /var/log/messages has:

Aug 17 02:37:31 pc kernel: len 20 vecnum: 4 sizeof (zfs_cmd_t) 4528

probably from:

        if (len != sizeof (zfs_iocparm_t)) {
                printf("len %d vecnum: %d sizeof (zfs_cmd_t) %ju\n",
                    len, vecnum, (uintmax_t)sizeof (zfs_cmd_t));
                return (EINVAL);
        }

in sys/contrib/openzfs/module/os/freebsd/zfs/kmod_core.c

That has another bug in logging, as zfs_iocparm_t and zfs_cmd_t are different structs. We should be printing sizeof(zfs_iocparm_t), not sizeof(zfs_cmd_t).

This bug probably doesn't affect Linux, as it uses zfs_cmd_t directly, which has the same field sizes/alignments on 32 and 64 bit already. We wrap zfs_cmd_t in our zfs_iocparm_t, and add this bug in the process.

It will take me a while to test a new patch.
Comment 5 Jessica Clarke freebsd_committer freebsd_triage 2022-02-21 18:25:57 UTC 
This is wrong for anything other than X86; it's only i386 that has this stupid ABI quirk. PowerPC freebsd32 would break if you added that unconditionally, for example, as would MIPS if MFC'ed to stable branches (though not sure how well supported freebsd32 is there).
Comment 6 Graham Perrin freebsd_committer freebsd_triage 2023-04-08 19:08:03 UTC 
This is amongst bug reports that need special attention; see <https://lists.freebsd.org/archives/freebsd-fs/2023-April/002047.html>.

With last year's 12.4 as the final RELEASE from stable/12, please: 

* is there – realistically – anything more to do on the FreeBSD side for stable/12?

(In reply to Damjan Jovanovic from comment #4)

Please, what's the current situation with regard to upstream?