* [PATCH] block: deny zone management ioctl on mounted fs @ 2020-05-14 16:26 Johannes Thumshirn 2020-05-15 4:52 ` Damien Le Moal 0 siblings, 1 reply; 5+ messages in thread From: Johannes Thumshirn @ 2020-05-14 16:26 UTC (permalink / raw) To: Jens Axboe Cc: linux-block @ vger . kernel . org, Damien Le Moal, Christoph Hellwig, Coly Li, Johannes Thumshirn If a user submits a zone management ioctl from user-space, like a zone reset and a file-system (like zonefs or f2fs) is mounted on the zoned block device, the zone will get reset and the file-system's cached value of the zone's write-pointer becomes invalid. Subsequent writes to this zone from the file-system will result in unaligned writes and the drive will error out. Deny zone management ioctls when a super_block is found on the block device. Reported-by: Coly Li <colyli@suse.de> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- Is there a better way to check for a mounted FS than get_super()/drop_super()? block/blk-zoned.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/block/blk-zoned.c b/block/blk-zoned.c index 23831fa8701d..6923695ec414 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -325,6 +325,7 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd, unsigned long arg) { void __user *argp = (void __user *)arg; + struct super_block *sb; struct request_queue *q; struct blk_zone_range zrange; enum req_opf op; @@ -345,6 +346,12 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode, if (!(mode & FMODE_WRITE)) return -EBADF; + sb = get_super(bdev); + if (sb) { + drop_super(sb); + return -EINVAL; + } + if (copy_from_user(&zrange, argp, sizeof(struct blk_zone_range))) return -EFAULT; -- 2.24.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] block: deny zone management ioctl on mounted fs 2020-05-14 16:26 [PATCH] block: deny zone management ioctl on mounted fs Johannes Thumshirn @ 2020-05-15 4:52 ` Damien Le Moal 2020-05-15 5:09 ` Coly Li 0 siblings, 1 reply; 5+ messages in thread From: Damien Le Moal @ 2020-05-15 4:52 UTC (permalink / raw) To: Johannes Thumshirn, Jens Axboe Cc: linux-block @ vger . kernel . org, Christoph Hellwig, Coly Li On 2020/05/15 1:26, Johannes Thumshirn wrote: > If a user submits a zone management ioctl from user-space, like a zone > reset and a file-system (like zonefs or f2fs) is mounted on the zoned > block device, the zone will get reset and the file-system's cached value > of the zone's write-pointer becomes invalid. > > Subsequent writes to this zone from the file-system will result in > unaligned writes and the drive will error out. > > Deny zone management ioctls when a super_block is found on the block > device. Zone management ioctls can only be executed by users that have SYS_CAP_ADMIN capabilities. If these start doing stupid things, the system is probably in for a lot of troubles beyond what this patch is trying to prevent. In addition, there are so many other ways that a mounted zoned block device can be corrupted beyond these ioctls that I am not sure this patch is very useful. E.g. any raw block device write in a zone would also cause the FS to see unaligned writes, and any other raw block device access is also possible for SYS_CAP_ADMIN users. Preventing only these ioctls does not really improve anything I think. That may even be harmful has that would prevent things like inline file system check utilities to run. > > Reported-by: Coly Li <colyli@suse.de> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > > Is there a better way to check for a mounted FS than get_super()/drop_super()? > > block/blk-zoned.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/block/blk-zoned.c b/block/blk-zoned.c > index 23831fa8701d..6923695ec414 100644 > --- a/block/blk-zoned.c > +++ b/block/blk-zoned.c > @@ -325,6 +325,7 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode, > unsigned int cmd, unsigned long arg) > { > void __user *argp = (void __user *)arg; > + struct super_block *sb; > struct request_queue *q; > struct blk_zone_range zrange; > enum req_opf op; > @@ -345,6 +346,12 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode, > if (!(mode & FMODE_WRITE)) > return -EBADF; > > + sb = get_super(bdev); > + if (sb) { > + drop_super(sb); > + return -EINVAL; > + } > + > if (copy_from_user(&zrange, argp, sizeof(struct blk_zone_range))) > return -EFAULT; > > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] block: deny zone management ioctl on mounted fs 2020-05-15 4:52 ` Damien Le Moal @ 2020-05-15 5:09 ` Coly Li 2020-05-15 5:25 ` Damien Le Moal 0 siblings, 1 reply; 5+ messages in thread From: Coly Li @ 2020-05-15 5:09 UTC (permalink / raw) To: Damien Le Moal Cc: Johannes Thumshirn, Jens Axboe, linux-block @ vger . kernel . org, Christoph Hellwig On 2020/5/15 12:52, Damien Le Moal wrote: > On 2020/05/15 1:26, Johannes Thumshirn wrote: >> If a user submits a zone management ioctl from user-space, like a zone >> reset and a file-system (like zonefs or f2fs) is mounted on the zoned >> block device, the zone will get reset and the file-system's cached value >> of the zone's write-pointer becomes invalid. >> >> Subsequent writes to this zone from the file-system will result in >> unaligned writes and the drive will error out. >> >> Deny zone management ioctls when a super_block is found on the block >> device. > > Zone management ioctls can only be executed by users that have SYS_CAP_ADMIN > capabilities. If these start doing stupid things, the system is probably in for > a lot of troubles beyond what this patch is trying to prevent. > > In addition, there are so many other ways that a mounted zoned block device can > be corrupted beyond these ioctls that I am not sure this patch is very useful. > E.g. any raw block device write in a zone would also cause the FS to see > unaligned writes, and any other raw block device access is also possible for > SYS_CAP_ADMIN users. Preventing only these ioctls does not really improve > anything I think. That may even be harmful has that would prevent things like > inline file system check utilities to run. > > The problem I encountered was, after I write 8KB data into seq/0 file, I want to re-write from offset 0. At that moment I didn't know to use 'truncate -s 0' to reset the write pointer of this zone file, so I use 'blkzone reset' to reset the write pointer of seq zone 0, and I saw the write pointer was reset to 0. But I still was not able to write data into seq/0 file on offset 0. Then I decided to reset all the zones by command 'blkzone reset -o 0 -c <zones number>', then the command hung for 20+ minutes and not response. From the kernel message I saw quite a log error message (an example is on pastbin: https://pastebin.com/ZFFNsaE0) In my mind, there are 2 methods to reset a zone, one is from blkzone, one is from truncate on zonefs. I guess I am not the first/last one which thinks the two method should work both, and has no idea when the above error encountered. Reject blkzone reset command when the zoned SMR drive is mounted by zonefs, it is OK to me to avoid confusion and further mistake. IMHO, This is a solution at least. Thanks. Coly Li >> >> Reported-by: Coly Li <colyli@suse.de> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> --- >> >> Is there a better way to check for a mounted FS than get_super()/drop_super()? >> >> block/blk-zoned.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/block/blk-zoned.c b/block/blk-zoned.c >> index 23831fa8701d..6923695ec414 100644 >> --- a/block/blk-zoned.c >> +++ b/block/blk-zoned.c >> @@ -325,6 +325,7 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode, >> unsigned int cmd, unsigned long arg) >> { >> void __user *argp = (void __user *)arg; >> + struct super_block *sb; >> struct request_queue *q; >> struct blk_zone_range zrange; >> enum req_opf op; >> @@ -345,6 +346,12 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode, >> if (!(mode & FMODE_WRITE)) >> return -EBADF; >> >> + sb = get_super(bdev); >> + if (sb) { >> + drop_super(sb); >> + return -EINVAL; >> + } >> + >> if (copy_from_user(&zrange, argp, sizeof(struct blk_zone_range))) >> return -EFAULT; >> >> > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] block: deny zone management ioctl on mounted fs 2020-05-15 5:09 ` Coly Li @ 2020-05-15 5:25 ` Damien Le Moal 2020-05-15 5:34 ` Coly Li 0 siblings, 1 reply; 5+ messages in thread From: Damien Le Moal @ 2020-05-15 5:25 UTC (permalink / raw) To: Coly Li Cc: Johannes Thumshirn, Jens Axboe, linux-block @ vger . kernel . org, Christoph Hellwig On 2020/05/15 14:09, Coly Li wrote: > On 2020/5/15 12:52, Damien Le Moal wrote: >> On 2020/05/15 1:26, Johannes Thumshirn wrote: >>> If a user submits a zone management ioctl from user-space, like a zone >>> reset and a file-system (like zonefs or f2fs) is mounted on the zoned >>> block device, the zone will get reset and the file-system's cached value >>> of the zone's write-pointer becomes invalid. >>> >>> Subsequent writes to this zone from the file-system will result in >>> unaligned writes and the drive will error out. >>> >>> Deny zone management ioctls when a super_block is found on the block >>> device. >> >> Zone management ioctls can only be executed by users that have SYS_CAP_ADMIN >> capabilities. If these start doing stupid things, the system is probably in for >> a lot of troubles beyond what this patch is trying to prevent. >> >> In addition, there are so many other ways that a mounted zoned block device can >> be corrupted beyond these ioctls that I am not sure this patch is very useful. >> E.g. any raw block device write in a zone would also cause the FS to see >> unaligned writes, and any other raw block device access is also possible for >> SYS_CAP_ADMIN users. Preventing only these ioctls does not really improve >> anything I think. That may even be harmful has that would prevent things like >> inline file system check utilities to run. >> >> > > The problem I encountered was, after I write 8KB data into seq/0 file, I > want to re-write from offset 0. At that moment I didn't know to use > 'truncate -s 0' to reset the write pointer of this zone file, so I use > 'blkzone reset' to reset the write pointer of seq zone 0, and I saw the > write pointer was reset to 0. But I still was not able to write data > into seq/0 file on offset 0. Then I decided to reset all the zones by > command 'blkzone reset -o 0 -c <zones number>', then the command hung > for 20+ minutes and not response. From the kernel message I saw quite a > log error message (an example is on pastbin: https://pastebin.com/ZFFNsaE0) > > In my mind, there are 2 methods to reset a zone, one is from blkzone, > one is from truncate on zonefs. I guess I am not the first/last one > which thinks the two method should work both, and has no idea when the > above error encountered. Well yes, that is correct. These are methods to reset zones. But for a mounted disk, any raw block device operation can corrupt the file system on it. That is a principle that remains true for zoned block devices. Resenting a zone directly on the device without the FS being aware of the operation will (and does) corrupt the FS. Same for raw disk writes vs file writes on any mounted disk... > > Reject blkzone reset command when the zoned SMR drive is mounted by > zonefs, it is OK to me to avoid confusion and further mistake. IMHO, > This is a solution at least. libblkid now includes patches supporting zonefs detection, so yes, we can patch blkzone to reject zone management operations if the device is mounted. We need the same for f2fs and dm-zoned too. Time to clean that up. Will do. > > Thanks. > > Coly Li > >>> >>> Reported-by: Coly Li <colyli@suse.de> >>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >>> --- >>> >>> Is there a better way to check for a mounted FS than get_super()/drop_super()? >>> >>> block/blk-zoned.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c >>> index 23831fa8701d..6923695ec414 100644 >>> --- a/block/blk-zoned.c >>> +++ b/block/blk-zoned.c >>> @@ -325,6 +325,7 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode, >>> unsigned int cmd, unsigned long arg) >>> { >>> void __user *argp = (void __user *)arg; >>> + struct super_block *sb; >>> struct request_queue *q; >>> struct blk_zone_range zrange; >>> enum req_opf op; >>> @@ -345,6 +346,12 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode, >>> if (!(mode & FMODE_WRITE)) >>> return -EBADF; >>> >>> + sb = get_super(bdev); >>> + if (sb) { >>> + drop_super(sb); >>> + return -EINVAL; >>> + } >>> + >>> if (copy_from_user(&zrange, argp, sizeof(struct blk_zone_range))) >>> return -EFAULT; >>> >>> >> >> > > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] block: deny zone management ioctl on mounted fs 2020-05-15 5:25 ` Damien Le Moal @ 2020-05-15 5:34 ` Coly Li 0 siblings, 0 replies; 5+ messages in thread From: Coly Li @ 2020-05-15 5:34 UTC (permalink / raw) To: Damien Le Moal Cc: Johannes Thumshirn, Jens Axboe, linux-block @ vger . kernel . org, Christoph Hellwig On 2020/5/15 13:25, Damien Le Moal wrote: > On 2020/05/15 14:09, Coly Li wrote: >> On 2020/5/15 12:52, Damien Le Moal wrote: >>> On 2020/05/15 1:26, Johannes Thumshirn wrote: >>>> If a user submits a zone management ioctl from user-space, like a zone >>>> reset and a file-system (like zonefs or f2fs) is mounted on the zoned >>>> block device, the zone will get reset and the file-system's cached value >>>> of the zone's write-pointer becomes invalid. >>>> >>>> Subsequent writes to this zone from the file-system will result in >>>> unaligned writes and the drive will error out. >>>> >>>> Deny zone management ioctls when a super_block is found on the block >>>> device. >>> >>> Zone management ioctls can only be executed by users that have SYS_CAP_ADMIN >>> capabilities. If these start doing stupid things, the system is probably in for >>> a lot of troubles beyond what this patch is trying to prevent. >>> >>> In addition, there are so many other ways that a mounted zoned block device can >>> be corrupted beyond these ioctls that I am not sure this patch is very useful. >>> E.g. any raw block device write in a zone would also cause the FS to see >>> unaligned writes, and any other raw block device access is also possible for >>> SYS_CAP_ADMIN users. Preventing only these ioctls does not really improve >>> anything I think. That may even be harmful has that would prevent things like >>> inline file system check utilities to run. >>> >>> >> >> The problem I encountered was, after I write 8KB data into seq/0 file, I >> want to re-write from offset 0. At that moment I didn't know to use >> 'truncate -s 0' to reset the write pointer of this zone file, so I use >> 'blkzone reset' to reset the write pointer of seq zone 0, and I saw the >> write pointer was reset to 0. But I still was not able to write data >> into seq/0 file on offset 0. Then I decided to reset all the zones by >> command 'blkzone reset -o 0 -c <zones number>', then the command hung >> for 20+ minutes and not response. From the kernel message I saw quite a >> log error message (an example is on pastbin: https://pastebin.com/ZFFNsaE0) >> >> In my mind, there are 2 methods to reset a zone, one is from blkzone, >> one is from truncate on zonefs. I guess I am not the first/last one >> which thinks the two method should work both, and has no idea when the >> above error encountered. > > Well yes, that is correct. These are methods to reset zones. But for a mounted > disk, any raw block device operation can corrupt the file system on it. That is > a principle that remains true for zoned block devices. Resenting a zone directly > on the device without the FS being aware of the operation will (and does) > corrupt the FS. Same for raw disk writes vs file writes on any mounted disk... > >> >> Reject blkzone reset command when the zoned SMR drive is mounted by >> zonefs, it is OK to me to avoid confusion and further mistake. IMHO, >> This is a solution at least. > > libblkid now includes patches supporting zonefs detection, so yes, we can patch > blkzone to reject zone management operations if the device is mounted. We need > the same for f2fs and dm-zoned too. Time to clean that up. Will do. Yes, this is a solution. Thanks. Coly Li ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-05-15 5:34 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-14 16:26 [PATCH] block: deny zone management ioctl on mounted fs Johannes Thumshirn 2020-05-15 4:52 ` Damien Le Moal 2020-05-15 5:09 ` Coly Li 2020-05-15 5:25 ` Damien Le Moal 2020-05-15 5:34 ` Coly Li
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.