Util-Linux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] blkzone: deny destructive ioctls on busy blockdev
@ 2020-05-15  8:41 Johannes Thumshirn
  2020-05-15  8:50 ` Chris Hofstaedtler
  2020-05-15 12:58 ` Karel Zak
  0 siblings, 2 replies; 5+ messages in thread
From: Johannes Thumshirn @ 2020-05-15  8:41 UTC (permalink / raw)
  To: Karel Zak; +Cc: util-linux, Johannes Thumshirn, Coly Li, Damien Le Moal

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] blkzone: deny destructive ioctls on busy blockdev
  2020-05-15  8:41 [PATCH] blkzone: deny destructive ioctls on busy blockdev Johannes Thumshirn
@ 2020-05-15  8:50 ` Chris Hofstaedtler
  2020-05-15  9:53   ` Johannes Thumshirn
  2020-05-15 12:58 ` Karel Zak
  1 sibling, 1 reply; 5+ messages in thread
From: Chris Hofstaedtler @ 2020-05-15  8:50 UTC (permalink / raw)
  To: Johannes Thumshirn; +Cc: Karel Zak, util-linux, Coly Li, Damien Le Moal

* 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] blkzone: deny destructive ioctls on busy blockdev
  2020-05-15  8:50 ` Chris Hofstaedtler
@ 2020-05-15  9:53   ` Johannes Thumshirn
  2020-05-15 10:37     ` Karel Zak
  0 siblings, 1 reply; 5+ messages in thread
From: Johannes Thumshirn @ 2020-05-15  9:53 UTC (permalink / raw)
  To: Chris Hofstaedtler; +Cc: Karel Zak, util-linux, Coly Li, Damien Le Moal

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] blkzone: deny destructive ioctls on busy blockdev
  2020-05-15  9:53   ` Johannes Thumshirn
@ 2020-05-15 10:37     ` Karel Zak
  0 siblings, 0 replies; 5+ messages in thread
From: Karel Zak @ 2020-05-15 10:37 UTC (permalink / raw)
  To: Johannes Thumshirn
  Cc: Chris Hofstaedtler, util-linux, Coly Li, Damien Le Moal

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] blkzone: deny destructive ioctls on busy blockdev
  2020-05-15  8:41 [PATCH] blkzone: deny destructive ioctls on busy blockdev Johannes Thumshirn
  2020-05-15  8:50 ` Chris Hofstaedtler
@ 2020-05-15 12:58 ` Karel Zak
  1 sibling, 0 replies; 5+ messages in thread
From: Karel Zak @ 2020-05-15 12:58 UTC (permalink / raw)
  To: Johannes Thumshirn; +Cc: util-linux, Coly Li, Damien Le Moal

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-15  8:41 [PATCH] blkzone: deny destructive ioctls on busy blockdev Johannes Thumshirn
2020-05-15  8:50 ` Chris Hofstaedtler
2020-05-15  9:53   ` Johannes Thumshirn
2020-05-15 10:37     ` Karel Zak
2020-05-15 12:58 ` Karel Zak

Util-Linux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/util-linux/0 util-linux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 util-linux util-linux/ https://lore.kernel.org/util-linux \
		util-linux@vger.kernel.org
	public-inbox-index util-linux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.util-linux


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git