Page MenuHomeFreeBSD

loader: zfs reader must use uint64_t instead of off_t
AbandonedPublic

Authored by tsoome on Apr 26 2018, 9:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 6 2024, 8:44 AM
Unknown Object (File)
Jan 6 2024, 8:44 AM
Unknown Object (File)
Jan 6 2024, 8:44 AM
Unknown Object (File)
Jan 6 2024, 8:31 AM
Unknown Object (File)
Dec 20 2023, 3:33 AM
Unknown Object (File)
Aug 24 2023, 12:16 AM
Unknown Object (File)
Aug 17 2023, 6:34 AM
Unknown Object (File)
Aug 13 2023, 6:13 AM
Subscribers

Details

Reviewers
allanjude
imp
dim
Summary

The off_t is signed 64-bit type in 32-bit build environment, but zfs reader
must be able to deal with 64-bit unsigned offsets.

Test Plan

need 4TB+ disk...

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17295
Build 17132: arc lint + arc unit

Event Timeline

stand/zfs/zfsimpl.c
411

Taking this function as an example, how could this possibly work now?
If off_t is 32 bit and signed, then the maximum possible offset is just 2 GB (note GB, not TB).
I can certainly boot from media where the required data is located far beyond that offset.

I think that on FreeBSD off_t is always 64 bit.

In D15207#322036, @avg wrote:

I think that on FreeBSD off_t is always 64 bit.

Yes, thats correct, it is 8 bytes (did verify). However, there is still issue of sign. Of course even with signed case we have quite a large max value there...

In D15207#322036, @avg wrote:

I think that on FreeBSD off_t is always 64 bit.

Yes, thats correct, it is 8 bytes (did verify). However, there is still issue of sign. Of course even with signed case we have quite a large max value there...

I think that the difference between 2⁶⁴ and 2⁶³ is negligible for all practical purposes.

In D15207#322069, @avg wrote:
In D15207#322036, @avg wrote:

I think that on FreeBSD off_t is always 64 bit.

Yes, thats correct, it is 8 bytes (did verify). However, there is still issue of sign. Of course even with signed case we have quite a large max value there...

I think that the difference between 2⁶⁴ and 2⁶³ is negligible for all practical purposes.

yes and no. the initial motivator did pop up from the case where vmware was suggesting we have 256PB disk (bogus value). So it is possible we can see weird numbers there and in that sense, it would be nice at least to have correct math, especially because tracking the actual sizes for the types is painful enough... Indeed, while with correct values we wont see the difference, the practical reasoning there is about all possible bogus values we get from "modern" PC's... sad but thats the life.

gptboot.c was missing the switch to uint64_t

I don't like this change at all. There's no practical difference, and this diverges us from the normal unix APIs.

In D15207#334555, @imp wrote:

I don't like this change at all. There's no practical difference, and this diverges us from the normal unix APIs.

there is a problem - zfs is *not* written to use "normal" unix API's. at all. So what we have is the code where unsigned type is swapped to signed and there are virtually no checks against sign wraps, overflows etc. Of course part of the issue is that there is no unsigned off_t and there is always the confusion how many bits that particular type has...

In D15207#334555, @imp wrote:

I don't like this change at all. There's no practical difference, and this diverges us from the normal unix APIs.

there is a problem - zfs is *not* written to use "normal" unix API's. at all. So what we have is the code where unsigned type is swapped to signed and there are virtually no checks against sign wraps, overflows etc. Of course part of the issue is that there is no unsigned off_t and there is always the confusion how many bits that particular type has...

Can you point to where this is happening so I can take a peek and see where the issues lie and if we can cope with a harmless cast to unsigned at the entry / top of the ZFS code rather than change our APIs to cope.

Well, we haven't seen any problems from this code. Except for that oddball system you mentioned earlier.
Not sure if this much churn is really warranted.