[Devel] [PATCH vz10 v4 2/2] fs: enforce cgroup permissions for bdevs on mount

[Konstantin Khorenko]

On 11/19/25 17:14, Aleksei Oladko wrote:
> 
> On 11/19/25 4:44 PM, Konstantin Khorenko wrote:
>> On 11/18/25 00:14, Aleksei Oladko wrote:
>>> In mount operations, cgroup device access permissions are checked only
>>> when a new super_block is created. If the device has already been
>>> mounted,
>>> no permission check is performed since the super_block already exists.
>>> As a result, a process belonging to a cgroup without access permissions
>>> to the device can still perform a mount operation successfully.
>>>
>>> To reproduce:
>>>    # mount /dev/loop0 /mnt/test0
>>>    # echo $$ > /sys/fs/cgroup/machine.slice/CTID/cgroup.procs
>>>    # mount /dev/loop0 /mnt/test1
>>>    #
>>>
>>> If the device was not mounted before, the mount operation fails
>>> as expected:
>>>    # echo $$ > /sys/fs/cgroup/machine.slice/CTID/cgroup.procs
>>>    # mount /dev/loop0 /mnt/test1
>>> mount: /mnt/test1: permission denied.
>>>
>>> This patch adds a cgroup permission check for the mounting process
>>> before obtaining the device's super_block. As a result, even if the
>>> device has already been mounted and the super_block exists, the mount
>>> operation will be denied when the process lacks the appropriate cgroup
>>> permissions.
>>>
>>> v2: removed redundant cgroup permission check when creating a new
>>> superblock in setup_dbev_super.
>>> v3: no changes
>>> v4: revert v2
>>>
>>> https://virtuozzo.atlassian.net/browse/VSTOR-105978
>>>
>>> Signed-off-by: Aleksei Oladko <aleksey.oladko at virtuozzo.com>
>>> ---
>>>    block/blk.h            |  1 -
>>>    fs/super.c             | 14 +++++++++++++-
>>>    include/linux/blkdev.h |  1 +
>>>    include/linux/fs.h     |  1 +
>>>    4 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/blk.h b/block/blk.h
>>> index c718e4291db0..8610d87e5050 100644
>>> --- a/block/blk.h
>>> +++ b/block/blk.h
>>> @@ -727,7 +727,6 @@ static inline void bio_issue_init(struct
>>> bio_issue *issue,
>>>    void bdev_release(struct file *bdev_file);
>>>    int bdev_open(struct block_device *bdev, blk_mode_t mode, void
>>> *holder,
>>>              const struct blk_holder_ops *hops, struct file *bdev_file);
>>> -int bdev_permission(dev_t dev, blk_mode_t mode, void *holder);
>>>      void blk_integrity_generate(struct bio *bio);
>>>    void blk_integrity_verify(struct bio *bio);
>>> diff --git a/fs/super.c b/fs/super.c
>>> index faf37790544e..6a2a88f07c12 100644
>>> --- a/fs/super.c
>>> +++ b/fs/super.c
>>> @@ -1407,6 +1407,18 @@ struct super_block *sget_dev(struct fs_context
>>> *fc, dev_t dev)
>>>    }
>>>    EXPORT_SYMBOL(sget_dev);
>>>    +struct super_block *sget_dev_check(struct fs_context *fc, dev_t dev)
>>> +{
>>> +    int ret;
>>> +
>>> +    blk_mode_t mode = sb_open_mode(fc->sb_flags) | BLK_OPEN_MOUNT;
>>> +    ret = bdev_permission(dev, mode, fc);
>>> +    if (ret)
>>> +        return ERR_PTR(ret);
>>> +    return sget_dev(fc, dev);
>>> +}
>>> +EXPORT_SYMBOL(sget_dev_check);
>>> +
>>>    #ifdef CONFIG_BLOCK
>>>    /*
>>>     * Lock the superblock that is holder of the bdev. Returns the
>>> superblock
>>> @@ -1664,7 +1676,7 @@ int get_tree_bdev_flags(struct fs_context *fc,
>>>            return error;
>>>        }
>>>        fc->sb_flags |= SB_NOSEC;
>>> -    s = sget_dev(fc, dev);
>>> +    s = sget_dev_check(fc, dev);
>>
>> Please, leave the original function name as a wrapper and rename the
>> original function to, say, _sget_dev().
>> s/sget_dev_check/sget_dev/
>> s/sget_dev/_sget_dev/
>>
>> The rationale: imagine the kernel will have another place with call to
>> sget_dev().
>> We never catch the moment such a new call appears, and thus the new
>> place will skip the BLK_OPEN_MOUNT check.
>>
>> On the other hand - in case the generic func sget_dev() becomes a
>> wrapper - the BLK_OPEN_MOUNT will be in place for any new call.
> get_dev is called from mtd_get_sb, and cgroup permission checks are not
> needed there, so a new function was introduced

On the other hand there is a filesystem nilfs2 which is an ordinary fs like ext4 and it also calls 
sget_dev() and which definitely needs our extra check for BLK_OPEN_MOUNT and not covered right now.

drivers/mtd/mtdsuper.c: sb = sget_dev(fc, MKDEV(MTD_BLOCK_MAJOR, mtd->index));
fs/nilfs2/super.c:      s = sget_dev(fc, dev);

i agree that MTD devices should skip this check though.

=> please create a specific function _sget_dev() or something else and use it in drivers/mtd/mtdsuper.c.

If some other function appears which calls sget_dev(), it would be better we prohibit the access 
because or an extra (probably not needed) check - and get a report/issue and fix it - than have a 
calltrace with missing check and never know about that.

>>
>>>        if (IS_ERR(s))
>>>            return PTR_ERR(s);
>>>    diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>> index 64c28dbb44b4..4a66f6aa0441 100644
>>> --- a/include/linux/blkdev.h
>>> +++ b/include/linux/blkdev.h
>>> @@ -1666,6 +1666,7 @@ struct file *bdev_file_open_by_path(const char
>>> *path, blk_mode_t mode,
>>>    int bd_prepare_to_claim(struct block_device *bdev, void *holder,
>>>            const struct blk_holder_ops *hops);
>>>    void bd_abort_claiming(struct block_device *bdev, void *holder);
>>> +int bdev_permission(dev_t dev, blk_mode_t mode, void *holder);
>>>      /* just for blk-cgroup, don't use elsewhere */
>>>    struct block_device *blkdev_get_no_open(dev_t dev);
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index b6f7809cad29..1f535909524c 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -2608,6 +2608,7 @@ struct super_block *sget(struct
>>> file_system_type *type,
>>>                int (*set)(struct super_block *,void *),
>>>                int flags, void *data);
>>>    struct super_block *sget_dev(struct fs_context *fc, dev_t dev);
>>> +struct super_block *sget_dev_check(struct fs_context *fc, dev_t dev);
>>>      /* Alas, no aliases. Too much hassle with bringing module.h
>>> everywhere */
>>>    #define fops_get(fops) ({                        \
>>