Re: [md PATCH] md: handle read-only member devices better.

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2046 bytes --]

On Wed, Apr 12 2017, Shaohua Li wrote:

> On Thu, Apr 13, 2017 at 08:53:48AM +1000, Neil Brown wrote:
>> 
>> 1/ If an array has any read-only devices when it is started,
>>    the array itself must be read-only
>> 2/ A read-only device cannot be added to an array after it is
>>    started.
>> 3/ Setting an array to read-write should not succeed
>>    if any member devices are read-only
>
> Didn't get these. We call md_import_device() first to open under layer disk. We
> always use FMOD_READ|FMOD_WRITE to open the disk. So if the disk is ro,
> md_import_device should fail, we don't add the disk to the array. Why would we
> have such issues?
>

Because life isn't always as simple as we might like it to be. :-(

md_import_device() calls lock_rdev() which calls blkdev_get_by_dev().

blkdev_get_by_dev() doesn't pay much attention to the mode, nor does
blkdev_get() which it calls.  The main place where FMODE_WRITE could be
rejected on a read-only device is in the device's 'open()' function.  A
few open functions do check for read-only, but it isn't at all
consistent.
scsi/sd.c does, block/loop.c doesn't, nor does nvme.  Most drivers seem
to ignore the mode.

blkdev_get_by_path() has

	if ((mode & FMODE_WRITE) && bdev_read_only(bdev)) {
		blkdev_put(bdev, mode);
		return ERR_PTR(-EACCES);
	}

so when you open a device by path name you always get this check, but
not when you open a device by device-number like md does.
It is worth having a look at
Commit: e51900f7d38c ("block: revert block_dev read-only check")
from 2011.  The bdev_read_only() check was in blkdev_get() for a while,
but it was moved out because doing that broke md and dm and others.

So at present, callers of blkdev_get_by_dev() need to do their own
bdev_read_only() tests before writing.
We could discuss where in md.c is the best place to put them, but unless
you want to take on a largish project to 'fix' (or audit) all callers of
blkdev_get_by_dev(), they need to go in md somewhere.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]