Page MenuHomeFreeBSD

loader: disk/part api needs to use uint64_t offsets
ClosedPublic

Authored by tsoome on Dec 4 2016, 12:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jun 24, 8:07 PM
Unknown Object (File)
Sun, Jun 16, 11:50 AM
Unknown Object (File)
Sun, Jun 16, 11:50 AM
Unknown Object (File)
Sun, Jun 16, 11:50 AM
Unknown Object (File)
Sun, Jun 16, 11:50 AM
Unknown Object (File)
Sun, Jun 16, 11:49 AM
Unknown Object (File)
Sun, Jun 16, 11:49 AM
Unknown Object (File)
Sun, Jun 16, 11:49 AM
Subscribers

Details

Summary

The disk_* and part_* api is using 64bit values for media size and
offsets. However, the current api is using off_t type, which is signed
64-bit int.

In this context the signed media size does not make any sense, and
the offsets are used to mark absolute, not relative locations.

Also, the data from GPT partition table and some other sources is
already using uint64_t data type, so using signed off_t can cause sign
issues.

As part of code review, I have found the parts of the mips/uboot/usb/userboot
may need more analysis as those parts are internally using off_t.

Test Plan

Only tested the compile so far.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tsoome retitled this revision from to loader: disk/part api needs to use uint64_t offsets.
tsoome updated this object.
tsoome edited the test plan for this revision. (Show Details)
tsoome added reviewers: allanjude, imp.

bd_ioctl changed off_t to uint64_t

So why not size_t in all the right places?

In D8710#180507, @imp wrote:

So why not size_t in all the right places?

hm, thats another option of course. I only used uint64_t because its used already in many places and its telling us about the size and sign. I personally don't mind size_t either. The one thing what is clear is that this type can not be signed and has to be 64-bit;)

size_t doesn't work since it is not always 64-bits. OTOH, uoff_t gives an unsigned off_t so it might be a natural fit for having an unsigned off_t-sized value.

In D8710#180823, @jhb wrote:

size_t doesn't work since it is not always 64-bits. OTOH, uoff_t gives an unsigned off_t so it might be a natural fit for having an unsigned off_t-sized value.

Except the uoff_t is guarded with _KERNEL and therefore also out.

tsoome edited edge metadata.

updated struct dentry

I don't remember why I chose off_t, but AFAIR, daddr_t is signed and used in many places.
Also there was a small tool in the tools/tools/bootparttest, that may need in some changes, but it seems it is already broken by some early change.

In D8710#189382, @ae wrote:

I don't remember why I chose off_t, but AFAIR, daddr_t is signed and used in many places.
Also there was a small tool in the tools/tools/bootparttest, that may need in some changes, but it seems it is already broken by some early change.

Well, just for better background - since the loader is standalone and completely isolated (except that we are sharing headers), I would keep other bits strictly separate. The motivation for this update is from the loader port to illumos, where it was pointed that there are no checks implemented... And while working with checks, the sign issue did pop up. Also note that we do not really expect that normal disks will grow that large any time soon, but instead, what we do have seen is broken firmware. So this patch is to help out to remove signed values, as media size and partition size are strictly positive values.

usb disk ioctl should provide DIOCGSECTORSIZE and DIOCGMEDIASIZE instead of
IOCTL_GET_BLOCK_SIZE and IOCTL_GET_BLOCKS (which are not used by anything).

svn update to revision 313042.

Yea, I'm mostly find with this. Not sure why we can't just make off_t or size_t or our own invented type always 64-bit, but not grumbly enough about it to make any more waves. So go ahead and commit.

This revision was automatically updated to reflect the committed changes.