Page MenuHomeFreeBSD

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

Authored by avg on Dec 4 2017, 10:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 15, 4:29 PM
Unknown Object (File)
Fri, Mar 15, 4:15 PM
Unknown Object (File)
Fri, Mar 15, 4:15 PM
Unknown Object (File)
Fri, Mar 15, 4:15 PM
Unknown Object (File)
Fri, Mar 15, 4:15 PM
Unknown Object (File)
Mon, Mar 11, 6:53 PM
Unknown Object (File)
Wed, Feb 28, 11:53 PM
Unknown Object (File)
Feb 8 2024, 3:21 AM
Subscribers
None

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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

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.

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.

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

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
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?

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.

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.

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

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.

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...

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.

address comments:

  • ENODEV -> EROFS
  • better minimum_cmd_size comparison

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.