All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.