[Devel] [PATCH hci-8.1 v2 0/1] block: let container root set the block size of and flush its device

[Devel] [PATCH hci-8.1 v2 0/1] block: let container root set the block size of and flush its device

[Eva Kurchatova]

Patch for task https://virtuozzo.atlassian.net/browse/VSTOR-151057

Changes in v2:

- Device in read-only cannot be flushed or have its block size changed
  from CT, depending on cgroup grant. Host path unchanged.
- Device with mounted filesystem denies flush from a container with
  EBUSY. Container syncs its own mounts via the filesystem.
- Host and container told apart by ve_is_super(), not by a failing
  capable().
- Corrected the claims in the commit message.

Link to v1: http://lore.virtuozzo.com/kernel/20261021175410.2339797-1-eva.kurchatova@virtuozzo.com/T/#u

Eva Kurchatova (1):
  block: let container root set the block size of and flush its device

 block/ioctl.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

-- 
2.56.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v2 1/1] block: let container root set the block size of and flush its device

[Eva Kurchatova]

BLKBSZSET and BLKFLSBUF check CAP_SYS_ADMIN in the initial user
namespace, so root in a container cannot set the block size of a block
device it has been given, nor flush its buffers, and mkfs.xfs on a loop
device there stops with "cannot set blocksize 512 on block device
/dev/loopN: Permission denied", or with "Flushing the data device
failed, err=13" once it gets past the first.

Check the capability relative to the container, as the other block
code adapted for containers does, and let container root through only
where its device cgroup grants write access to the device: a device
handed to a container read-only may be held by a filesystem outside
it, and neither its cache nor its block size should be touched from
inside. Device with mounted filesystem denies flush from a container
with EBUSY, as block size change does, and a container syncs its own
mounts through filesystem.

https://virtuozzo.atlassian.net/browse/VSTOR-137668
Feature: ve: allow various syscalls in CT root userns

Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
---
 block/ioctl.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index a3c53b04e1e3..def89aa19904 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -9,6 +9,7 @@
 #include <linux/backing-dev.h>
 #include <linux/fs.h>
 #include <linux/blktrace_api.h>
+#include <linux/ve.h>
 #include <linux/pr.h>
 #include <linux/uaccess.h>
 #include <linux/pagemap.h>
@@ -416,16 +417,31 @@ static int blkdev_pr_clear(struct block_device *bdev, blk_mode_t mode,
 	return ops->pr_clear(bdev, c.key);
 }
 
+/* Container root may administer only a device its device cgroup lets it write */
+static bool blkdev_admin_capable(struct block_device *bdev)
+{
+	if (!ve_capable(CAP_SYS_ADMIN))
+		return false;
+
+	return ve_is_super(get_exec_env()) ||
+	       !bdev_permission(bdev->bd_dev, BLK_OPEN_WRITE, NULL);
+}
+
 static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
 		unsigned long arg)
 {
-	if (!capable(CAP_SYS_ADMIN))
+	if (!blkdev_admin_capable(bdev))
 		return -EACCES;
 
 	mutex_lock(&bdev->bd_holder_lock);
-	if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync)
+	if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync) {
+		/* A filesystem on the device is synced by its own mount, not from a container */
+		if (!ve_is_super(get_exec_env())) {
+			mutex_unlock(&bdev->bd_holder_lock);
+			return -EBUSY;
+		}
 		bdev->bd_holder_ops->sync(bdev);
-	else {
+	} else {
 		mutex_unlock(&bdev->bd_holder_lock);
 		sync_blockdev(bdev);
 	}
@@ -527,11 +543,12 @@ static int blkdev_bszset(struct file *file, blk_mode_t mode,
 {
 	// this one might be file_inode(file)->i_rdev - a rare valid
 	// use of file_inode() for those.
-	dev_t dev = I_BDEV(file->f_mapping->host)->bd_dev;
+	struct block_device *bdev = I_BDEV(file->f_mapping->host);
+	dev_t dev = bdev->bd_dev;
 	struct file *excl_file;
 	int ret, n;
 
-	if (!capable(CAP_SYS_ADMIN))
+	if (!blkdev_admin_capable(bdev))
 		return -EACCES;
 	if (!argp)
 		return -EINVAL;
-- 
2.56.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 1/1] block: let container root set the block size of and flush its device

[no-reply]

> BLKBSZSET and BLKFLSBUF check CAP_SYS_ADMIN in the initial user
> namespace, so root in a container cannot set the block size of a block
> device it has been given, nor flush its buffers, and mkfs.xfs on a loop
> device there stops with "cannot set blocksize 512 on block device
> /dev/loopN: Permission denied", or with "Flushing the data device
> failed, err=13" once it gets past the first.
> 
> Check the capability relative to the container, as the other block
> code adapted for containers does, and let container root through only
> where its device cgroup grants write access to the device: a device
> handed to a container read-only may be held by a filesystem outside
> it, and neither its cache nor its block size should be touched from
> inside. Device with mounted filesystem denies flush from a container
> with EBUSY, as block size change does, and a container syncs its own
> mounts through filesystem.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-137668
> Feature: ve: allow various syscalls in CT root userns
> 
> Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022022554.2427456-1-eva.kurchatova@virtuozzo.com?part=1


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 1/1] block: let container root set the block size of and flush its device

[Pavel Tikhomirov]

On 10/22/26 04:25, Eva Kurchatova wrote:
>  static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
>  		unsigned long arg)
>  {
> -	if (!capable(CAP_SYS_ADMIN))
> +	if (!blkdev_admin_capable(bdev))
>  		return -EACCES;
>  
>  	mutex_lock(&bdev->bd_holder_lock);
> -	if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync)
> +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync) {
> +		/* A filesystem on the device is synced by its own mount, not from a container */
> +		if (!ve_is_super(get_exec_env())) {
> +			mutex_unlock(&bdev->bd_holder_lock);
> +			return -EBUSY;
> +		}
>  		bdev->bd_holder_ops->sync(bdev);

This looks a bit overprotective, if we allow writing to this disk directly, why not allow syncing it directly, mounted fs or not.

Not a blocker though, fine either way.

Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 1/1] block: let container root set the block size of and flush its device

[Konstantin Khorenko]

On 10/23/26 12:52, Pavel Tikhomirov wrote:
> On 10/22/26 04:25, Eva Kurchatova wrote:
>>  static int blkdev_flushbuf(struct block_device *bdev, unsigned cmd,
>>  		unsigned long arg)
>>  {
>> -	if (!capable(CAP_SYS_ADMIN))
>> +	if (!blkdev_admin_capable(bdev))
>>  		return -EACCES;
>>  
>>  	mutex_lock(&bdev->bd_holder_lock);
>> -	if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync)
>> +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->sync) {
>> +		/* A filesystem on the device is synced by its own mount, not from a container */
>> +		if (!ve_is_super(get_exec_env())) {
>> +			mutex_unlock(&bdev->bd_holder_lock);
>> +			return -EBUSY;
>> +		}
>>  		bdev->bd_holder_ops->sync(bdev);
> 
> This looks a bit overprotective, if we allow writing to this disk directly, why not allow syncing it directly, mounted fs or not.
> 
> Not a blocker though, fine either way.
> 
> Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

> This looks a bit overprotective, if we allow writing to this disk
> directly, why not allow syncing it directly, mounted fs or not.

Agreed, we will open this path.

It is reachable only with a device cgroup write grant on that very
device, bdev_permission(BLK_OPEN_WRITE) is checked at ioctl time. CT
disks from vzctl have W (write for mount only), so they stay at -EACCES.

With a write grant the CT can already overwrite the filesystem under
the mount. Forcing its writeback is strictly less than that.

fs_bdev_sync() is what syncfs() does: sync_filesystem() under s_umount
shared, with bd_holder_lock dropped before. Nothing the host does not
already get from any syncfs() on that mount.

invalidate_bdev() drops clean pages only, the dirty ones went through
the sync first, and busy buffers of a mounted fs are skipped. The cost
is a metadata re-read, not data.

The only thing it bypasses is the VE sync filter in ksys_sync(), and
raw write access to the device makes that filter moot anyway. On the
CT's own mounted loop it restores host parity: blockdev --flushbufs
behaves as on the host.

> Not a blocker though, fine either way.
>
> Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

Thanks.


--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel