Bug 251035 - ZFS: Allow 64 bit ZFS to support 32 bit ioctls (Wine)
Summary: ZFS: Allow 64 bit ZFS to support 32 bit ioctls (Wine)
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.2-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-fs (Nobody)
URL:
Keywords: needs-qa
Depends on:
Blocks:
 
Reported: 2020-11-11 06:35 UTC by Damjan Jovanovic
Modified: 2022-02-21 18:25 UTC (History)
2 users (show)

See Also:
koobs: maintainer-feedback? (allanjude)
koobs: mfc-stable13?
koobs: mfc-stable12?
koobs: mfc-stable11?


Attachments
convert 32 bit zfs_iocparm_t to 64 bit (1.62 KB, patch)
2020-11-11 06:35 UTC, Damjan Jovanovic
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 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 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).