Re: [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open()

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

22.04.2021 19:43, Kevin Wolf wrote:
> Normally, blk_new_open() just shares all permissions. This was fine
> originally when permissions only protected against uses in the same
> process because no other part of the code would actually get to access
> the block nodes opened with blk_new_open(). However, since we use it for
> file locking now, unsharing permissions becomes desirable.
> 
> Add a new BDRV_O_NO_SHARE flag that is used in blk_new_open() to unshare
> any permissions that can be unshared.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/block/block.h |  1 +
>   block/block-backend.c | 19 +++++++++++++------
>   2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index b3f6e509d4..735db05a39 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -101,6 +101,7 @@ typedef struct HDGeometry {
>       uint32_t cylinders;
>   } HDGeometry;
>   
> +#define BDRV_O_NO_SHARE    0x0001 /* don't share permissons */
>   #define BDRV_O_RDWR        0x0002
>   #define BDRV_O_RESIZE      0x0004 /* request permission for resizing the node */
>   #define BDRV_O_SNAPSHOT    0x0008 /* open the file read only and save writes in a snapshot */
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 413af51f3b..61a10ea860 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -398,15 +398,19 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
>       BlockBackend *blk;
>       BlockDriverState *bs;
>       uint64_t perm = 0;
> +    uint64_t shared = BLK_PERM_ALL;
>   
> -    /* blk_new_open() is mainly used in .bdrv_create implementations and the
> -     * tools where sharing isn't a concern because the BDS stays private, so we
> -     * just request permission according to the flags.
> +    /*
> +     * blk_new_open() is mainly used in .bdrv_create implementations and the
> +     * tools where sharing isn't a major concern because the BDS stays private
> +     * and the file is generally not supposed to be used by a second process,
> +     * so we just request permission according to the flags.
>        *
>        * The exceptions are xen_disk and blockdev_init(); in these cases, the
>        * caller of blk_new_open() doesn't make use of the permissions, but they
>        * shouldn't hurt either. We can still share everything here because the
> -     * guest devices will add their own blockers if they can't share. */
> +     * guest devices will add their own blockers if they can't share.
> +     */

Probably old comment "We can still share everything" is a bit in conflict with commit message (and new logic).

>       if ((flags & BDRV_O_NO_IO) == 0) {
>           perm |= BLK_PERM_CONSISTENT_READ;
>           if (flags & BDRV_O_RDWR) {
> @@ -416,8 +420,11 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
>       if (flags & BDRV_O_RESIZE) {
>           perm |= BLK_PERM_RESIZE;
>       }
> +    if (flags & BDRV_O_NO_SHARE) {
> +        shared = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
> +    }
>   
> -    blk = blk_new(qemu_get_aio_context(), perm, BLK_PERM_ALL);
> +    blk = blk_new(qemu_get_aio_context(), perm, shared);
>       bs = bdrv_open(filename, reference, options, flags, errp);
>       if (!bs) {
>           blk_unref(blk);
> @@ -426,7 +433,7 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
>   
>       blk->root = bdrv_root_attach_child(bs, "root", &child_root,
>                                          BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
> -                                       blk->ctx, perm, BLK_PERM_ALL, blk, errp);
> +                                       blk->ctx, perm, shared, blk, errp);
>       if (!blk->root) {
>           blk_unref(blk);
>           return NULL;
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir