Page MenuHomeFreeBSD

geom_disk / scsi_da: deny opening write-protected disks for writing
ClosedPublic

Authored by avg on Dec 4 2017, 10:30 AM.

Details

Summary

scsi_xpt: save the device specific field of the mode parameters header
Save the device specific field of the mode parameters header while
querying Control extension page during PROBE_MODE_SENSE stage.
Allow access to the saved value via a new XPT_DEV_ADVINFO sub-type.
Consider saving more of the header or, perhaps, both the whole header
and the page.

geom_disk: deny opening disk for writing if it's marked as write-protected
A new disk(9) flag is added to mark write protected disks.
Another alternative would be to add another parameter to d_open, so
that the open mode could be passed to it and the disk drivers could
make the decision internally.

scsi_da: set write-protected flag based on mode paramter header
Check if the disk is write protected using bit 7 of the device specific
field in the mode parameter header returned by MODE SENSE and saved by
scsi_xpt.
Consider if we also need / want to check SWP bit (SCP_SWP) in the
Control mode page (0x0A) that we query at PROBE_MODE_SENSE stage.
Also, see:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=224037#c17

Test Plan

So far, tested with an iSCSI disk having option readonly on in ctl.conf.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avg created this revision.Dec 4 2017, 10:30 AM
avg added a comment.Dec 4 2017, 10:32 AM

This is a proof of concept, needs discussion and refinement.

cem added a reviewer: scottl.Dec 4 2017, 7:10 PM

I would like to see this change or something like it land in the tree. I don't think the concept is as controversial as having mount fallback to RO.

sys/cam/scsi/scsi_da.c
5620 ↗(On Diff #36182)

Does this bit have a named constant?

And is this bit sufficient?

sys/geom/geom_disk.h
134 ↗(On Diff #36182)

Could also take 0x40

avg added a comment.Dec 4 2017, 8:43 PM
In D13360#279048, @cem wrote:

I would like to see this change or something like it land in the tree. I don't think the concept is as controversial as having mount fallback to RO.

I am sure that you will be proven wrong.
In FreeBSD, anything that gets noticed becomes controversial.
Unless it's way too large or way too complex to comprehend.

sys/cam/scsi/scsi_da.c
5620 ↗(On Diff #36182)

Does this bit have a named constant?

There is no scsi-wide constant for it in FreeBSD. After all it is in device-specific byte.
scsi_sa has its own definition, SMH_SA_WP.

And is this bit sufficient?

I don't know.

avg updated this revision to Diff 36237.Dec 5 2017, 2:02 PM

Move mode header querying from scsi_xpt to scsi_da.

First of all, the xpt code queries the control page only for specific
configurations. Second, the page may be unsupported.

scsi_da now always queries the all pages page that describes other
available pages and should be supported by a lot more of hardware.
Also, scsi_da now takes into account the supported command width
when issuing MODE SENSE.

cem added a comment.Dec 5 2017, 7:20 PM

Looks fine to me but I'd like someone more familiar with CAM to look over it too.

avg added a reviewer: imp.Dec 5 2017, 11:07 PM
imp requested changes to this revision.Dec 5 2017, 11:45 PM

What happens if I flip the write protect switch off? This doesn't seem to contemplate that...

sys/cam/scsi/scsi_da.c
2540 ↗(On Diff #36237)

why remove this?

4294 ↗(On Diff #36237)

Name for this bit?

4297 ↗(On Diff #36237)

Why is clearing needed? If so, we should clear it above the if, so we don't have to clear it at the else

5335 ↗(On Diff #36237)

We never seem to go into RC16 state.

sys/geom/geom_disk.c
128 ↗(On Diff #36237)

NODEV? That doesn't sound right.

This revision now requires changes to proceed.Dec 5 2017, 11:45 PM
avg added a comment.Dec 6 2017, 9:10 AM
In D13360#279553, @imp wrote:

What happens if I flip the write protect switch off? This doesn't seem to contemplate that...

Anything I can experiment with is either permanently write-protected or has a physical switch that's impossible to flip without first ejecting the device, so I cannot tell for sure.
But I hope that there will be a unit attention sense code or something like that and that will trigger the reprobe sequence.
In that case we should be able to detect the change.

sys/cam/scsi/scsi_da.c
2540 ↗(On Diff #36237)

It's not actually removed, it's moved to DA_CCB_PROBE_WP handler, because DA_STATE_PROBE_WP is now always the first stage. Previously, either DA_STATE_PROBE_RC or DA_STATE_PROBE_RC16 could be the first probe stage.

4294 ↗(On Diff #36237)

We don't have a name for it yet.
I can create one. Should i?

4297 ↗(On Diff #36237)

I think that there can be cases where we get some notification from the target and re-probe its properties.
So, the change of the bit might change (hypothetically).

Whether to clear the bit before 'if' or in 'else', I think that that's pure a matter of personal taste.
But if you insist, I can make the change.

5335 ↗(On Diff #36237)

See the end of the new DA_CCB_PROBE_WP clause above.

sys/geom/geom_disk.c
128 ↗(On Diff #36237)

Operation not supported by device
To me, it seems like it fits, but if there is anything that fits better, I will use it.
Do you have any suggestions?

mav accepted this revision.Dec 8 2017, 5:41 PM

Looks good to me, just a couple of comments.

We should not need to to check SWP, since according to SPC-5 setting SWP should also set WP.

sys/cam/scsi/scsi_da.c
4286 ↗(On Diff #36237)

I think here could better work (softc->minimum_cmd_size > 6).

sys/geom/geom_disk.c
128 ↗(On Diff #36237)

I see places using EROFS for that.

avg added inline comments.Dec 12 2017, 3:22 PM
sys/cam/scsi/scsi_da.c
4286 ↗(On Diff #36237)

Okay, will do.

sys/geom/geom_disk.c
128 ↗(On Diff #36237)

I've also noticed that ENODEV, EROFS and EACCESS are used to signal the condition in various drivers.
To me, ENODEV made the most sense as other error codes are easily confused with file-system level logical errors. But if you think that EROFS is better, I will use it.

Please also see D13361.

avg added a comment.Dec 12 2017, 3:26 PM

@imp Warner, do you have any further questions or requests?

cem added inline comments.Dec 12 2017, 6:08 PM
sys/geom/geom_disk.c
128 ↗(On Diff #36237)

I like EROFS best, even if this is below the filesystem layer. After all, any filesystem using this media cannot mount R/W either, so a EROFS error makes some sense whether it originates in the filesystem or the media.

imp added inline comments.Dec 12 2017, 6:23 PM
sys/geom/geom_disk.c
128 ↗(On Diff #36237)

Me too. We should do a pass through the tree to regularize this return value from all the storage drivers so we are consistent...

avg added inline comments.Dec 12 2017, 8:29 PM
sys/geom/geom_disk.c
128 ↗(On Diff #36237)

Okay, I will make the change.
But I am still somewhat concerned about the (end-)user experience given strerror:

Mount operation failed: Read-only filesystem

vs

Mount operation failed: Operation not supported by device

This seems like a case where we need a new error code, e.g., ERODEV.

avg updated this revision to Diff 36586.Dec 14 2017, 1:18 PM

address comments:

  • ENODEV -> EROFS
  • better minimum_cmd_size comparison
mav accepted this revision.Dec 14 2017, 9:49 PM

Looks good to me.

sys/geom/geom_disk.c
128 ↗(On Diff #36237)

I'd say that "Read-only filesystem" is much more informative for end-user, even if it goes not about file system.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 15 2018, 11:20 AM
This revision was automatically updated to reflect the committed changes.