* [PATCH v2 0/2] qemu-img convert: Unshare write permission for source @ 2021-04-22 16:43 Kevin Wolf 2021-04-22 16:43 ` [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open() Kevin Wolf 2021-04-22 16:43 ` [PATCH v2 2/2] qemu-img convert: Unshare write permission for source Kevin Wolf 0 siblings, 2 replies; 7+ messages in thread From: Kevin Wolf @ 2021-04-22 16:43 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, qemu-devel v2: - Still share BLK_PERM_CONSISTENT_READ. It doesn't hurt users that don't want their image modified in parallel and it fixes some use cases covered by iotests. Kevin Wolf (2): block: Add BDRV_O_NO_SHARE for blk_new_open() qemu-img convert: Unshare write permission for source include/block/block.h | 1 + block/block-backend.c | 19 +++++++++++++------ qemu-img.c | 2 +- 3 files changed, 15 insertions(+), 7 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open() 2021-04-22 16:43 [PATCH v2 0/2] qemu-img convert: Unshare write permission for source Kevin Wolf @ 2021-04-22 16:43 ` Kevin Wolf 2021-04-22 18:41 ` Eric Blake 2021-04-23 9:43 ` Vladimir Sementsov-Ogievskiy 2021-04-22 16:43 ` [PATCH v2 2/2] qemu-img convert: Unshare write permission for source Kevin Wolf 1 sibling, 2 replies; 7+ messages in thread From: Kevin Wolf @ 2021-04-22 16:43 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, qemu-devel 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. + */ 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; -- 2.30.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open() 2021-04-22 16:43 ` [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open() Kevin Wolf @ 2021-04-22 18:41 ` Eric Blake 2021-04-23 9:43 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 7+ messages in thread From: Eric Blake @ 2021-04-22 18:41 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: qemu-devel On 4/22/21 11:43 AM, 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 */ permissions With the typo fix, Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open() 2021-04-22 16:43 ` [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open() Kevin Wolf 2021-04-22 18:41 ` Eric Blake @ 2021-04-23 9:43 ` Vladimir Sementsov-Ogievskiy 2021-04-23 13:58 ` Kevin Wolf 1 sibling, 1 reply; 7+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-04-23 9:43 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open() 2021-04-23 9:43 ` Vladimir Sementsov-Ogievskiy @ 2021-04-23 13:58 ` Kevin Wolf 0 siblings, 0 replies; 7+ messages in thread From: Kevin Wolf @ 2021-04-23 13:58 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu-block Am 23.04.2021 um 11:43 hat Vladimir Sementsov-Ogievskiy geschrieben: > 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). I read that paragraph as referring to xen_disk and blockdev_init() only, which still don't pass BDRV_O_NO_SHARE after this series. The comment explains why they don't have to pass it. Kevin ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] qemu-img convert: Unshare write permission for source 2021-04-22 16:43 [PATCH v2 0/2] qemu-img convert: Unshare write permission for source Kevin Wolf 2021-04-22 16:43 ` [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open() Kevin Wolf @ 2021-04-22 16:43 ` Kevin Wolf 2021-04-23 15:17 ` Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 7+ messages in thread From: Kevin Wolf @ 2021-04-22 16:43 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, qemu-devel For a successful conversion of an image, we must make sure that its content doesn't change during the conversion. A special case of this is using the same image file both as the source and as the destination. If both input and output format are raw, the operation would just be useless work, with other formats it is a sure way to destroy the image. This will now fail because the image file can't be opened a second time for the output when opening it for the input has already acquired file locks to unshare BLK_PERM_WRITE. Nevertheless, if there is some reason in a special case why it is actually okay to allow writes to the image while it is being converted, -U can still be used to force sharing all permissions. Note that for most image formats, BLK_PERM_WRITE would already be unshared by the format driver, so this only really makes a difference for raw source images (but any output format). Reported-by: Xueqiang Wei <xuwei@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- qemu-img.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qemu-img.c b/qemu-img.c index babb5573ab..a5993682aa 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2146,7 +2146,7 @@ static void set_rate_limit(BlockBackend *blk, int64_t rate_limit) static int img_convert(int argc, char **argv) { - int c, bs_i, flags, src_flags = 0; + int c, bs_i, flags, src_flags = BDRV_O_NO_SHARE; const char *fmt = NULL, *out_fmt = NULL, *cache = "unsafe", *src_cache = BDRV_DEFAULT_CACHE, *out_baseimg = NULL, *out_filename, *out_baseimg_param, *snapshot_name = NULL; -- 2.30.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] qemu-img convert: Unshare write permission for source 2021-04-22 16:43 ` [PATCH v2 2/2] qemu-img convert: Unshare write permission for source Kevin Wolf @ 2021-04-23 15:17 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 7+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-04-23 15:17 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: qemu-devel 22.04.2021 19:43, Kevin Wolf wrote: > For a successful conversion of an image, we must make sure that its > content doesn't change during the conversion. > > A special case of this is using the same image file both as the source > and as the destination. If both input and output format are raw, the > operation would just be useless work, with other formats it is a sure > way to destroy the image. This will now fail because the image file > can't be opened a second time for the output when opening it for the > input has already acquired file locks to unshare BLK_PERM_WRITE. > > Nevertheless, if there is some reason in a special case why it is > actually okay to allow writes to the image while it is being converted, > -U can still be used to force sharing all permissions. > > Note that for most image formats, BLK_PERM_WRITE would already be > unshared by the format driver, so this only really makes a difference > for raw source images (but any output format). > > Reported-by: Xueqiang Wei <xuwei@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > qemu-img.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/qemu-img.c b/qemu-img.c > index babb5573ab..a5993682aa 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -2146,7 +2146,7 @@ static void set_rate_limit(BlockBackend *blk, int64_t rate_limit) > > static int img_convert(int argc, char **argv) > { > - int c, bs_i, flags, src_flags = 0; > + int c, bs_i, flags, src_flags = BDRV_O_NO_SHARE; > const char *fmt = NULL, *out_fmt = NULL, *cache = "unsafe", > *src_cache = BDRV_DEFAULT_CACHE, *out_baseimg = NULL, > *out_filename, *out_baseimg_param, *snapshot_name = NULL; > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-23 15:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-22 16:43 [PATCH v2 0/2] qemu-img convert: Unshare write permission for source Kevin Wolf 2021-04-22 16:43 ` [PATCH v2 1/2] block: Add BDRV_O_NO_SHARE for blk_new_open() Kevin Wolf 2021-04-22 18:41 ` Eric Blake 2021-04-23 9:43 ` Vladimir Sementsov-Ogievskiy 2021-04-23 13:58 ` Kevin Wolf 2021-04-22 16:43 ` [PATCH v2 2/2] qemu-img convert: Unshare write permission for source Kevin Wolf 2021-04-23 15:17 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.