loader: allow fs modules to use ioctl()
ClosedPublic
Actions

Authored by tsoome on Feb 20 2025, 3:31 PM.

Details

Reviewers

manu
imp

Commits

rGccf7b62bd88c: loader: allow fs modules to use ioctl()

Summary

Currently only directly opened disk device is allowed to access
ioctl() - that is, when file has F_RAW flag set.

The problem is, file systems might need to determine the device parameters,
and we could use already opened device descriptor to communicate this
information.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tsoome created this revision.Feb 20 2025, 3:31 PM

Herald added a reviewer: manu. · View Herald TranscriptFeb 20 2025, 3:31 PM

Herald added subscribers: dab, imp. · View Herald Transcript

tsoome requested review of this revision.Feb 20 2025, 3:31 PM

Harbormaster completed remote builds in B62541: Diff 151244.Feb 20 2025, 3:31 PM

tsoome added a child revision: D49078: loader: dosfs should be able to work with different sector sizes.Feb 20 2025, 3:31 PM

I thought that we checked for raw because we couldn't guarantee that all other implementations had the right pre-req to allow this safely.
I remember getting burned by something similar because we weren't completely consistent... Maybe f->f_dev isn't always set? It's been a while.

In D49077#1120387, @imp wrote:

I thought that we checked for raw because we couldn't guarantee that all other implementations had the right pre-req to allow this safely.
I remember getting burned by something similar because we weren't completely consistent... Maybe f->f_dev isn't always set? It's been a while.

Well, thats why I did add f_dev check - if it is NULL, we can not do much anyhow. And we do return EIO, same as before. We use this feature in dosfs.c to read sector size; of course the alternative would be to take disk name, open with raw flag, get ioctl done and close it... but we already do have fd of open disk device.

In D49077#1120524, @tsoome wrote:

In D49077#1120387, @imp wrote:

I thought that we checked for raw because we couldn't guarantee that all other implementations had the right pre-req to allow this safely.
I remember getting burned by something similar because we weren't completely consistent... Maybe f->f_dev isn't always set? It's been a while.

Well, thats why I did add f_dev check - if it is NULL, we can not do much anyhow. And we do return EIO, same as before. We use this feature in dosfs.c to read sector size; of course the alternative would be to take disk name, open with raw flag, get ioctl done and close it... but we already do have fd of open disk device.

Ooops. I missed this coming in during the week...

So I think this will be OK. It will certainly fail safely. It does expose a small hole in our device model (The Filesystem mounted on The Partition: no chance for one to many relationships w/o gymnastics to hide that in the device layer. But even with them, I think this is fine to commit as is. While I can construct weird edge cases, I think moving forward on this will be more useful than ensuring the weird edge cases are taken care of.

This revision is now accepted and ready to land.Sun, Mar 2, 10:12 PM

Closed by commit rGccf7b62bd88c: loader: allow fs modules to use ioctl() (authored by tsoome). · Explain WhyMon, Mar 3, 9:54 PM

This revision was automatically updated to reflect the committed changes.

tsoome added a commit: rGccf7b62bd88c: loader: allow fs modules to use ioctl().