[PATCH] dm: Implement set_read_only

[PATCH] dm: Implement set_read_only

[Łukasz Patron]

This lets us change the read-only flag for device mapper block devices
via the BLKROSET ioctl.

Signed-off-by: Łukasz Patron <priv.luk@gmail.com>
---
 drivers/md/dm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 87bb90303435..538a93e596d7 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -410,6 +410,12 @@ static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return dm_get_geometry(md, geo);
 }
 
+static int dm_blk_set_read_only(struct block_device *bdev, bool ro)
+{
+	set_disk_ro(bdev->bd_disk, ro);
+	return 0;
+}
+
 static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
 			    struct block_device **bdev)
 {
@@ -3666,6 +3672,7 @@ static const struct block_device_operations dm_blk_dops = {
 	.release = dm_blk_close,
 	.ioctl = dm_blk_ioctl,
 	.getgeo = dm_blk_getgeo,
+	.set_read_only = dm_blk_set_read_only,
 	.report_zones = dm_blk_report_zones,
 	.pr_ops = &dm_pr_ops,
 	.owner = THIS_MODULE
-- 
2.46.0

Re: [PATCH] dm: Implement set_read_only

[Mikulas Patocka]

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



On Wed, 21 Aug 2024, Łukasz Patron wrote:

> This lets us change the read-only flag for device mapper block devices
> via the BLKROSET ioctl.
> 
> Signed-off-by: Łukasz Patron <priv.luk@gmail.com>
> ---
>  drivers/md/dm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 87bb90303435..538a93e596d7 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -410,6 +410,12 @@ static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>  	return dm_get_geometry(md, geo);
>  }
>  
> +static int dm_blk_set_read_only(struct block_device *bdev, bool ro)
> +{
> +	set_disk_ro(bdev->bd_disk, ro);
> +	return 0;
> +}
> +
>  static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
>  			    struct block_device **bdev)
>  {
> @@ -3666,6 +3672,7 @@ static const struct block_device_operations dm_blk_dops = {
>  	.release = dm_blk_close,
>  	.ioctl = dm_blk_ioctl,
>  	.getgeo = dm_blk_getgeo,
> +	.set_read_only = dm_blk_set_read_only,
>  	.report_zones = dm_blk_report_zones,
>  	.pr_ops = &dm_pr_ops,
>  	.owner = THIS_MODULE
> -- 
> 2.46.0

Hi

Device mapper already calls set_disk_ro in the do_resume function. So, the 
problem here is that the value set using set_read_only will be overwritten 
as soon as a new table will be loaded.

I'd like to ask why is this patch needed? Why do you want to set read-only 
status using this ioctl instead of using the existing table flag?

If this is needed, we need to add another flag that is being set by 
dm_blk_set_read_only, so that dm_blk_set_read_only and dm_resume won't 
step over each other's changes.

Mikulas

Re: [PATCH] dm: Implement set_read_only

[Łukasz Patron]

Hi

 >I'd like to ask why is this patch needed? Why do you want to set read-only
 >status using this ioctl instead of using the existing table flag?

I basically just wanted to be able to use `blockdev --setrw {}` on
Android for a block device that had its table mapped as read only. I
believe that used to work on 5.10 or so, but not anymore.

 >If this is needed, we need to add another flag that is being set by
 >dm_blk_set_read_only, so that dm_blk_set_read_only and dm_resume won't
 >step over each other's changes.

The following diff should address that, however I noticed that
set_disk_ro() itself, triggers an uevent message that makes upstream
lvm2/udev/10-dm.rules.in <http://10-dm.rules.in> disable a dm device, so 
not sure if this is
good to have, after all.

--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -412,6 +412,19 @@ static int dm_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
  
  static int dm_blk_set_read_only(struct block_device *bdev, bool ro)
  {
+	struct mapped_device *md = bdev->bd_disk->private_data;
+	int srcu_idx;
+	struct dm_table *table;
+
+	table = dm_get_live_table(md, &srcu_idx);
+	if (table) {
+		if (ro)
+			table->mode &= ~BLK_OPEN_WRITE;
+		else
+			table->mode = ~BLK_OPEN_WRITE;
+	}
+	dm_put_live_table(md, srcu_idx);
+
  	set_disk_ro(bdev->bd_disk, ro);
  	return 0;
  }


On Tue, Aug 27, 2024 at 7:52 PM Mikulas Patocka <mpatocka@redhat.com> wrote:



    On Wed, 21 Aug 2024, Łukasz Patron wrote:

     > This lets us change the read-only flag for device mapper block
    devices
     > via the BLKROSET ioctl.
     >
     > Signed-off-by: Łukasz Patron <priv.luk@gmail.com>
     > ---
     >  drivers/md/dm.c | 7 +++++++
     >  1 file changed, 7 insertions(+)
     >
     > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
     > index 87bb90303435..538a93e596d7 100644
     > --- a/drivers/md/dm.c
     > +++ b/drivers/md/dm.c
     > @@ -410,6 +410,12 @@ static int dm_blk_getgeo(struct block_device
    *bdev, struct hd_geometry *geo)
     >       return dm_get_geometry(md, geo);
     >  }
     >
     > +static int dm_blk_set_read_only(struct block_device *bdev, bool ro)
     > +{
     > +     set_disk_ro(bdev->bd_disk, ro);
     > +     return 0;
     > +}
     > +
     >  static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
     >                           struct block_device **bdev)
     >  {
     > @@ -3666,6 +3672,7 @@ static const struct block_device_operations
    dm_blk_dops = {
     >       .release = dm_blk_close,
     >       .ioctl = dm_blk_ioctl,
     >       .getgeo = dm_blk_getgeo,
     > +     .set_read_only = dm_blk_set_read_only,
     >       .report_zones = dm_blk_report_zones,
     >       .pr_ops = &dm_pr_ops,
     >       .owner = THIS_MODULE
     > --
     > 2.46.0

    Hi

    Device mapper already calls set_disk_ro in the do_resume function.
    So, the
    problem here is that the value set using set_read_only will be
    overwritten
    as soon as a new table will be loaded.

    I'd like to ask why is this patch needed? Why do you want to set
    read-only
    status using this ioctl instead of using the existing table flag?

    If this is needed, we need to add another flag that is being set by
    dm_blk_set_read_only, so that dm_blk_set_read_only and dm_resume won't
    step over each other's changes.

    Mikulas

Re: [PATCH] dm: Implement set_read_only

[Mikulas Patocka]

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



On Wed, 28 Aug 2024, Łukasz Patron wrote:

> Hi
> 
> >I'd like to ask why is this patch needed? Why do you want to set read-only
> >status using this ioctl instead of using the existing table flag?
> 
> I basically just wanted to be able to use `blockdev --setrw {}` on
> Android for a block device that had its table mapped as read only. I
> believe that used to work on 5.10 or so, but not anymore.

Yes, I looked at the older kernel and it will just flip the read-only flag 
regardress of whether the driver supports it or not.

What specific partition do you need to write to? Is it possible to just 
reload the table instead of using blockdev --setrw?

Is it required for rooting the phone or for some other activity?

> >If this is needed, we need to add another flag that is being set by
> >dm_blk_set_read_only, so that dm_blk_set_read_only and dm_resume won't
> >step over each other's changes.
> 
> The following diff should address that, however I noticed that
> set_disk_ro() itself, triggers an uevent message that makes upstream
> lvm2/udev/10-dm.rules.in <http://10-dm.rules.in> disable a dm device, so not
> sure if this is
> good to have, after all.

This patch doesn't address that - when someone loads a new table and then 
does suspend+resume to swap the table, set_disk_ro will be called and the 
value specified by dm_blk_set_read_only will be overwritten.

Another problem is that if the table is read-only and you forcefully flip 
it to read-write, then the underlying devices will still be read-only and 
you would be writing to them. This is something that shouldn't be done. 
Unforunatelly, older lvm does that - so the kernel just prints a warning 
instead of rejecting the write. But I just don't want to add more places 
where we are writing to read-only devices.

Mikulas

> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -412,6 +412,19 @@ static int dm_blk_getgeo(struct block_device *bdev,
> struct hd_geometry *geo)
>   static int dm_blk_set_read_only(struct block_device *bdev, bool ro)
>  {
> +	struct mapped_device *md = bdev->bd_disk->private_data;
> +	int srcu_idx;
> +	struct dm_table *table;
> +
> +	table = dm_get_live_table(md, &srcu_idx);
> +	if (table) {
> +		if (ro)
> +			table->mode &= ~BLK_OPEN_WRITE;
> +		else
> +			table->mode = ~BLK_OPEN_WRITE;
> +	}
> +	dm_put_live_table(md, srcu_idx);
> +
>  	set_disk_ro(bdev->bd_disk, ro);
>  	return 0;
>  }