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

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

Details

Reviewers

cem
mav
scottl
imp

Commits

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

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 Passed

Unit

No Test Coverage

Build Status

Buildable 13558
Build 13782: arc lint + arc unit

Event Timeline

avg created this revision.Dec 4 2017, 10:30 AM

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
5709	Does this bit have a named constant? And is this bit sufficient?
sys/geom/geom_disk.h
136	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
5709	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.

avg added a reviewer: imp.Dec 5 2017, 11:07 PM

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

sys/cam/scsi/scsi_da.c
2536	why remove this?
4311	Name for this bit?
4314	Why is clearing needed? If so, we should clear it above the if, so we don't have to clear it at the else
5352	We never seem to go into RC16 state.
sys/geom/geom_disk.c
130	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
2536	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.
4311	We don't have a name for it yet. I can create one. Should i?
4314	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.
5352	See the end of the new `DA_CCB_PROBE_WP` clause above.
sys/geom/geom_disk.c
130	`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
4303	I think here could better work (softc->minimum_cmd_size > 6).
sys/geom/geom_disk.c
130	I see places using EROFS for that.

avg added inline comments.Dec 12 2017, 3:22 PM

sys/cam/scsi/scsi_da.c
4303	Okay, will do.
sys/geom/geom_disk.c
130	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?

cem added inline comments.Dec 12 2017, 6:08 PM

sys/geom/geom_disk.c
130	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
130	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
130	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`.