linux-kernel - Re: [PATCH] dm: Implement set_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0a02f887-aa70-4c7a-be58-3920596c175e@gmail.com>
Date: Wed, 28 Aug 2024 11:21:02 +0200
From: Łukasz Patron <priv.luk@...il.com>
To: Mikulas Patocka <mpatocka@...hat.com>
Cc: Alasdair Kergon <agk@...hat.com>, Mike Snitzer <snitzer@...nel.org>,
 dm-devel@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dm: Implement set_read_only

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@...hat.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@...il.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