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

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

[NeilBrown]

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


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

Reported-and-Tested-by: Nanda Kishore Chinnaram <Nanda_Kishore_Chinna@dell.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/md/md.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 22894303d335..9fe930109012 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2093,6 +2093,10 @@ static int bind_rdev_to_array(struct md_rdev *rdev, struct mddev *mddev)
 	if (find_rdev(mddev, rdev->bdev->bd_dev))
 		return -EEXIST;
 
+	if ((bdev_read_only(rdev->bdev) || bdev_read_only(rdev->meta_bdev)) &&
+	    mddev->pers)
+		return -EROFS;
+
 	/* make sure rdev->sectors exceeds mddev->dev_sectors */
 	if (!test_bit(Journal, &rdev->flags) &&
 	    rdev->sectors &&
@@ -5345,6 +5349,13 @@ int md_run(struct mddev *mddev)
 			continue;
 		sync_blockdev(rdev->bdev);
 		invalidate_bdev(rdev->bdev);
+		if (mddev->ro != 1 &&
+		    (bdev_read_only(rdev->bdev) ||
+		     bdev_read_only(rdev->meta_bdev))) {
+			mddev->ro = 1;
+			if (mddev->gendisk)
+				set_disk_ro(mddev->gendisk, 1);
+		}
 
 		/* perform some consistency tests on the device.
 		 * We don't want the data to overlap the metadata,
@@ -5569,6 +5580,9 @@ static int do_md_run(struct mddev *mddev)
 static int restart_array(struct mddev *mddev)
 {
 	struct gendisk *disk = mddev->gendisk;
+	struct md_rdev *rdev;
+	bool has_journal = false;
+	bool has_readonly = false;
 
 	/* Complain if it has no devices */
 	if (list_empty(&mddev->disks))
@@ -5577,24 +5591,21 @@ static int restart_array(struct mddev *mddev)
 		return -EINVAL;
 	if (!mddev->ro)
 		return -EBUSY;
-	if (test_bit(MD_HAS_JOURNAL, &mddev->flags)) {
-		struct md_rdev *rdev;
-		bool has_journal = false;
-
-		rcu_read_lock();
-		rdev_for_each_rcu(rdev, mddev) {
-			if (test_bit(Journal, &rdev->flags) &&
-			    !test_bit(Faulty, &rdev->flags)) {
-				has_journal = true;
-				break;
-			}
-		}
-		rcu_read_unlock();
 
+	rcu_read_lock();
+	rdev_for_each_rcu(rdev, mddev) {
+		if (test_bit(Journal, &rdev->flags) &&
+		    !test_bit(Faulty, &rdev->flags))
+			has_journal = true;
+		if (bdev_read_only(rdev->bdev))
+			has_readonly = true;
+	}
+	rcu_read_unlock();
+	if (test_bit(MD_HAS_JOURNAL, &mddev->flags) && !has_journal)
 		/* Don't restart rw with journal missing/faulty */
-		if (!has_journal)
 			return -EINVAL;
-	}
+	if (has_readonly)
+		return -EROFS;
 
 	mddev->safemode = 0;
 	mddev->ro = 0;
-- 
2.12.2


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

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

[Shaohua Li]

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?

Thanks,
Shaohua
 
> Reported-and-Tested-by: Nanda Kishore Chinnaram <Nanda_Kishore_Chinna@dell.com>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  drivers/md/md.c | 41 ++++++++++++++++++++++++++---------------
>  1 file changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 22894303d335..9fe930109012 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2093,6 +2093,10 @@ static int bind_rdev_to_array(struct md_rdev *rdev, struct mddev *mddev)
>  	if (find_rdev(mddev, rdev->bdev->bd_dev))
>  		return -EEXIST;
>  
> +	if ((bdev_read_only(rdev->bdev) || bdev_read_only(rdev->meta_bdev)) &&
> +	    mddev->pers)
> +		return -EROFS;
> +
>  	/* make sure rdev->sectors exceeds mddev->dev_sectors */
>  	if (!test_bit(Journal, &rdev->flags) &&
>  	    rdev->sectors &&
> @@ -5345,6 +5349,13 @@ int md_run(struct mddev *mddev)
>  			continue;
>  		sync_blockdev(rdev->bdev);
>  		invalidate_bdev(rdev->bdev);
> +		if (mddev->ro != 1 &&
> +		    (bdev_read_only(rdev->bdev) ||
> +		     bdev_read_only(rdev->meta_bdev))) {
> +			mddev->ro = 1;
> +			if (mddev->gendisk)
> +				set_disk_ro(mddev->gendisk, 1);
> +		}
>  
>  		/* perform some consistency tests on the device.
>  		 * We don't want the data to overlap the metadata,
> @@ -5569,6 +5580,9 @@ static int do_md_run(struct mddev *mddev)
>  static int restart_array(struct mddev *mddev)
>  {
>  	struct gendisk *disk = mddev->gendisk;
> +	struct md_rdev *rdev;
> +	bool has_journal = false;
> +	bool has_readonly = false;
>  
>  	/* Complain if it has no devices */
>  	if (list_empty(&mddev->disks))
> @@ -5577,24 +5591,21 @@ static int restart_array(struct mddev *mddev)
>  		return -EINVAL;
>  	if (!mddev->ro)
>  		return -EBUSY;
> -	if (test_bit(MD_HAS_JOURNAL, &mddev->flags)) {
> -		struct md_rdev *rdev;
> -		bool has_journal = false;
> -
> -		rcu_read_lock();
> -		rdev_for_each_rcu(rdev, mddev) {
> -			if (test_bit(Journal, &rdev->flags) &&
> -			    !test_bit(Faulty, &rdev->flags)) {
> -				has_journal = true;
> -				break;
> -			}
> -		}
> -		rcu_read_unlock();
>  
> +	rcu_read_lock();
> +	rdev_for_each_rcu(rdev, mddev) {
> +		if (test_bit(Journal, &rdev->flags) &&
> +		    !test_bit(Faulty, &rdev->flags))
> +			has_journal = true;
> +		if (bdev_read_only(rdev->bdev))
> +			has_readonly = true;
> +	}
> +	rcu_read_unlock();
> +	if (test_bit(MD_HAS_JOURNAL, &mddev->flags) && !has_journal)
>  		/* Don't restart rw with journal missing/faulty */
> -		if (!has_journal)
>  			return -EINVAL;
> -	}
> +	if (has_readonly)
> +		return -EROFS;
>  
>  	mddev->safemode = 0;
>  	mddev->ro = 0;
> -- 
> 2.12.2
> 

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

[NeilBrown]

[-- 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 --]

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

[Anthony Youngman]

On 15/04/17 05:45, NeilBrown wrote:
> 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.

anthony@crappit:~/gitstuff/linux-stable> git grep -a blkdev_get_by_dev > 
blkdev_get_by_dev.txt
anthony@crappit:~/gitstuff/linux-stable> cat blkdev_get_by_dev.txt
drivers/block/xen-blkback/xenbus.c:     bdev = 
blkdev_get_by_dev(vbd->pdevice, vbd->readonly ?
drivers/md/dm.c:        bdev = blkdev_get_by_dev(dev, td->dm_dev.mode | 
FMODE_EXCL, _claim_ptr);
drivers/md/md.c:        bdev = blkdev_get_by_dev(dev, 
FMODE_READ|FMODE_WRITE|FMODE_EXCL,
drivers/mtd/devices/block2mtd.c:                bdev = 
blkdev_get_by_dev(devt, mode, dev);
fs/block_dev.c: * blkdev_get_by_dev - open a block device by device number
fs/block_dev.c:struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t 
mode, void *holder)
fs/block_dev.c:EXPORT_SYMBOL(blkdev_get_by_dev);
fs/ext4/super.c:        bdev = blkdev_get_by_dev(dev, 
FMODE_READ|FMODE_WRITE|FMODE_EXCL, sb);
fs/f2fs/super.c: 
blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev,
fs/jfs/jfs_logmgr.c:    bdev = blkdev_get_by_dev(sbi->logdev, 
FMODE_READ|FMODE_WRITE|FMODE_EXCL,
fs/nfs/blocklayout/dev.c:       bdev = blkdev_get_by_dev(dev, FMODE_READ 
| FMODE_WRITE, NULL);
fs/reiserfs/journal.c:          journal->j_dev_bd = 
blkdev_get_by_dev(jdev, blkdev_mode,
include/linux/fs.h:extern struct block_device *blkdev_get_by_dev(dev_t 
dev, fmode_t mode,
kernel/power/swap.c:    hib_resume_bdev = 
blkdev_get_by_dev(swsusp_resume_device,
anthony@crappit:~/gitstuff/linux-stable>

So, if I'm mad enough to take this on, does that mean going through all 
of these checking/fixing them to test for a return code of -EACCESS?

No promises that I will, I keep on wanting to do this sort of stuff and 
then life gets in the way, but if I can't do it it'll go on the list of 
projects on the raid wiki.

Cheers,
Wol

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

[NeilBrown]

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

On Sat, Apr 15 2017, Anthony Youngman wrote:

> On 15/04/17 05:45, NeilBrown wrote:
>> 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.
>
> anthony@crappit:~/gitstuff/linux-stable> git grep -a blkdev_get_by_dev > 
> blkdev_get_by_dev.txt
> anthony@crappit:~/gitstuff/linux-stable> cat blkdev_get_by_dev.txt
> drivers/block/xen-blkback/xenbus.c:     bdev = 
> blkdev_get_by_dev(vbd->pdevice, vbd->readonly ?
> drivers/md/dm.c:        bdev = blkdev_get_by_dev(dev, td->dm_dev.mode | 
> FMODE_EXCL, _claim_ptr);
> drivers/md/md.c:        bdev = blkdev_get_by_dev(dev, 
> FMODE_READ|FMODE_WRITE|FMODE_EXCL,
> drivers/mtd/devices/block2mtd.c:                bdev = 
> blkdev_get_by_dev(devt, mode, dev);
> fs/block_dev.c: * blkdev_get_by_dev - open a block device by device number
> fs/block_dev.c:struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t 
> mode, void *holder)
> fs/block_dev.c:EXPORT_SYMBOL(blkdev_get_by_dev);
> fs/ext4/super.c:        bdev = blkdev_get_by_dev(dev, 
> FMODE_READ|FMODE_WRITE|FMODE_EXCL, sb);
> fs/f2fs/super.c: 
> blkdev_get_by_dev(sbi->sb->s_bdev->bd_dev,
> fs/jfs/jfs_logmgr.c:    bdev = blkdev_get_by_dev(sbi->logdev, 
> FMODE_READ|FMODE_WRITE|FMODE_EXCL,
> fs/nfs/blocklayout/dev.c:       bdev = blkdev_get_by_dev(dev, FMODE_READ 
> | FMODE_WRITE, NULL);
> fs/reiserfs/journal.c:          journal->j_dev_bd = 
> blkdev_get_by_dev(jdev, blkdev_mode,
> include/linux/fs.h:extern struct block_device *blkdev_get_by_dev(dev_t 
> dev, fmode_t mode,
> kernel/power/swap.c:    hib_resume_bdev = 
> blkdev_get_by_dev(swsusp_resume_device,
> anthony@crappit:~/gitstuff/linux-stable>
>
> So, if I'm mad enough to take this on, does that mean going through all 
> of these checking/fixing them to test for a return code of -EACCESS?

Yes, where "fixing" means understanding how the bdev is used in
each case, and what mode is really required and making it behave
correctly in all circumstances.
Maybe that means just propagating the error upwards.
Maybe it means retrying with only FMODE_READ, and recording that no
write access was available.  At the very least, this fact would be used
to pass the right fmode_t to blkdev_put.
Maybe it means always using FMODE_READ because FMODE_WRITE isn't
actually needed (though that seems unlikely).

Maybe it also means bd_write_holder could be changed to a counter
instead of a flag, if we can convince ourselves that the use of @mode
in blkdev_get/put() is no longer "too fragile".

NeilBrown

>
> No promises that I will, I keep on wanting to do this sort of stuff and 
> then life gets in the way, but if I can't do it it'll go on the list of 
> projects on the raid wiki.
>
> Cheers,
> Wol

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

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

[Shaohua Li]

On Sat, Apr 15, 2017 at 02:45:31PM +1000, Neil Brown wrote:
> 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.

It's unfortunate we need the hack, but anyway I applied this. I'm wonding how
useful a ro array is. At least ro array with .sync_request is dangerous because
we could read inconsistent data.

Thanks,
Shaohua

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

[NeilBrown]

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

On Thu, Apr 20 2017, Shaohua Li wrote:

> On Sat, Apr 15, 2017 at 02:45:31PM +1000, Neil Brown wrote:
>> 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.
>
> It's unfortunate we need the hack, but anyway I applied this. I'm wonding how
> useful a ro array is. At least ro array with .sync_request is dangerous because
> we could read inconsistent data.

For a RAID0, a ro array is as useful as any other ro device.

For RAID1 etc it isn't so obvious, but the code tries to do the
correct thing I think.  e.g. if a RAID1 is not known to be in-sync, then
reads will all come from the "first" device, so data should not
inconsistent.
If that first device fails, you will start getting reads from the second
device, which could be a problem.
I think it is better to allow access despite possible problems rather
than denying the possibility of read-only access altogether.

Thanks,
NeilBrown

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