[PATCH] blkzone: deny destructive ioctls on busy blockdev

[PATCH] blkzone: deny destructive ioctls on busy blockdev

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

Open the block device file in exclusive mode for submitting these ioctls.
If a file-system is mounted the kernel will return -EBUSY and we can't
continue issuing the ioctl.

Reported-by: Coly Li <colyli@suse.de>
Cc: Damien Le Moal <Damien.LeMoal@wdc.com>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 sys-utils/blkzone.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sys-utils/blkzone.c b/sys-utils/blkzone.c
index 8d9de2e1bba7..a4b59754616e 100644
--- a/sys-utils/blkzone.c
+++ b/sys-utils/blkzone.c
@@ -301,7 +301,9 @@ static int blkzone_action(struct blkzone_control *ctl)
 	if (!zonesize)
 		errx(EXIT_FAILURE, _("%s: unable to determine zone size"), ctl->devname);
 
-	fd = init_device(ctl, O_WRONLY);
+	fd = init_device(ctl, O_WRONLY | O_EXCL);
+	if (fd < 0)
+		errx(EXIT_FAILURE, _("%s: unable to open"), ctl->devname);
 
 	if (ctl->offset & (zonesize - 1))
 		errx(EXIT_FAILURE, _("%s: offset %" PRIu64 " is not aligned "
-- 
2.24.1

Re: [PATCH] blkzone: deny destructive ioctls on busy blockdev

[Chris Hofstaedtler]

* Johannes Thumshirn <johannes.thumshirn@wdc.com> [200515 10:41]:
> 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.

"error out" meaning what exactly?

> Open the block device file in exclusive mode for submitting these ioctls.
> If a file-system is mounted the kernel will return -EBUSY and we can't
> continue issuing the ioctl.

Isn't this something the kernel should enforce, then?

What's to stop anybody from calling the ioctl from another tool,
without using O_EXCL?

Chris

Re: [PATCH] blkzone: deny destructive ioctls on busy blockdev

[Johannes Thumshirn]

On 15/05/2020 10:50, Chris Hofstaedtler wrote:
> * Johannes Thumshirn <johannes.thumshirn@wdc.com> [200515 10:41]:
>> 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.
> 
> "error out" meaning what exactly?

The drive will report an Unaligned Write error.

> 
>> Open the block device file in exclusive mode for submitting these ioctls.
>> If a file-system is mounted the kernel will return -EBUSY and we can't
>> continue issuing the ioctl.
> 
> Isn't this something the kernel should enforce, then?

I did a patch for the kernel yesterday [1] enforcing this limitation, but as
Damien said it's SYS_CAP_ADMIN and with great power comes great responsibility.
We're also allowing other raw block device accesses on block devices.

[1] https://lore.kernel.org/linux-block/BY5PR04MB69006DE86D1050620B5EDAA4E7BD0@BY5PR04MB6900.namprd04.prod.outlook.com/

> What's to stop anybody from calling the ioctl from another tool,
> without using O_EXCL?

Nothing, but still we don't need to make one's life hard by letting blkzone go 
havoc with FS internal caching. If another tool does zone resets, it's up to them
to check for mounted block devices.

Re: [PATCH] blkzone: deny destructive ioctls on busy blockdev

[Karel Zak]

On Fri, May 15, 2020 at 09:53:46AM +0000, Johannes Thumshirn wrote:
> On 15/05/2020 10:50, Chris Hofstaedtler wrote:
> > * Johannes Thumshirn <johannes.thumshirn@wdc.com> [200515 10:41]:
> >> 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.
> > 
> > "error out" meaning what exactly?
> 
> The drive will report an Unaligned Write error.
> 
> > 
> >> Open the block device file in exclusive mode for submitting these ioctls.
> >> If a file-system is mounted the kernel will return -EBUSY and we can't
> >> continue issuing the ioctl.
> > 
> > Isn't this something the kernel should enforce, then?
> 
> I did a patch for the kernel yesterday [1] enforcing this limitation, but as
> Damien said it's SYS_CAP_ADMIN and with great power comes great responsibility.
> We're also allowing other raw block device accesses on block devices.
> 
> [1] https://lore.kernel.org/linux-block/BY5PR04MB69006DE86D1050620B5EDAA4E7BD0@BY5PR04MB6900.namprd04.prod.outlook.com/
> 
> > What's to stop anybody from calling the ioctl from another tool,
> > without using O_EXCL?
> 
> Nothing, but still we don't need to make one's life hard by letting blkzone go 
> havoc with FS internal caching. If another tool does zone resets, it's up to them
> to check for mounted block devices.

O_EXCL good idea, it nothing unusual that you can do crazy things with
devices with mounted filesystem (see for example fdisk(s), wipefs, ...). 

And sometimes it is valid use-case to do "bad" things and in this case
we have --force option in our tools (for example to avoid O_EXCL).

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] blkzone: deny destructive ioctls on busy blockdev

[Karel Zak]

On Fri, May 15, 2020 at 05:41:33PM +0900, Johannes Thumshirn wrote:
>  sys-utils/blkzone.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Applied, thanks.

> +	fd = init_device(ctl, O_WRONLY | O_EXCL);
> +	if (fd < 0)
> +		errx(EXIT_FAILURE, _("%s: unable to open"), ctl->devname);

I have removed this error message. It's already within init_device().

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com