Page MenuHomeFreeBSD

md driver compat32: fix structure padding for arm, powerpc
ClosedPublic

Authored by karels on Aug 6 2023, 7:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
May 1 2024, 11:55 PM
Unknown Object (File)
May 1 2024, 10:22 PM
Unknown Object (File)
May 1 2024, 9:49 PM
Unknown Object (File)
May 1 2024, 9:49 PM
Unknown Object (File)
May 1 2024, 9:49 PM
Unknown Object (File)
Dec 20 2023, 7:23 AM
Unknown Object (File)
Dec 12 2023, 3:04 AM
Unknown Object (File)
Nov 7 2023, 1:12 AM
Subscribers

Details

Summary

Because the 32-bit md_ioctl structure contains 64-bit members, arm
and powerpc add padding to a multiple of 8. i386 doesn't do this.
The md_ioctl32 definition was correct for amd64/i386 without padding,
but wrong for arm64 and powerpc64. Make packed conditional on
amd64, and test for the expected size on non-amd64. Note that
mdconfig is used in the ATF test suite. Note, I verified the
structure size for powerpc, but was unable to test.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

karels requested review of this revision.Aug 6 2023, 7:59 PM
sys/dev/md/md.c
150

Hm, this shouldn't be packed, md_ioctl isn't. Dropping packed would fix this instead.

sys/dev/md/md.c
150

That would break i386 compatibility on amd64.

sys/dev/md/md.c
150

Even on amd64 it shouldn't be packed, just packed(4). Though ideally we'd have an __aligned(4) freebsd32_uint64_t type (and an off_t variant) for that to avoid the #ifdefs everywhere they're needed.

sys/dev/md/md.c
150

I'm not sure what you are proposing for this fix. I could switch to ifdef'ing packed/packed(4) here; I see freebsd32_misc.c does that in a few places. A general solution seems like a much bigger change that we need now. But these structures are constructed by hand in any case.

sys/dev/md/md.c
150

Yes, #ifdef __amd64__ __packed(4) #endif

Change from explicit padding to using packed only on amd64.

sys/dev/md/md.c
150

I see no provision for __packed(4); the compiler chokes on the construct. I used the existing packed syntax with ifdefs.

sys/dev/md/md.c
150

Hm, interesting, you can't give packed an integer, but you can for the MS-style pragma variant accepted by both GCC and Clang, which OpenZFS uses for its 32-bit ioctl compat:

#ifdef __amd64__
#pragma pack(push, 4)
#endif
struct md_ioctl32 {
        ...
};
#ifdef __amd64__
#pragma pack(pop)
#endif

But making it more pedantically correct for amd64 is a separate issue that doesn't need to be done here, just ifdef'ing what's already done for amd64 seems fine.

LGTM provided commit message is altered to reflect the updated diff

This revision is now accepted and ready to land.Aug 6 2023, 10:31 PM

LGTM provided commit message is altered to reflect the updated diff

The commit message has already been updated, on the review as well as in git.

LGTM provided commit message is altered to reflect the updated diff

The commit message has already been updated, on the review as well as in git.

So it has; I should have checked it again when the page refreshed after leaving my first comment