Image below user cases:
case 1:
```
$ qemu-img create -f raw source.raw 1G
$ qemu-img create -f qcow2 -F raw -b source.raw ./source.raw
qemu-img info source.raw
image: source.raw
file format: qcow2
virtual size: 193K (197120 bytes)
disk size: 196K
cluster_size: 65536
backing file: source.raw <<<<<<
backing file format: raw
Format specific information:
compat: 1.1
lazy refcounts: false
refcount bits: 16
corrupt: false
```
case 2:
```
$ qemu-img create -f raw source.raw 1G
$ ln -sf source.raw destination.qcow2
$ qemu-img create -f qcow2 -F raw -b source.raw ./destination.qcow2
qemu-img info source.raw
image: source.raw
file format: qcow2 <<<<<<
virtual size: 2.0G (2147483648 bytes)
disk size: 196K
cluster_size: 65536
backing file: source.raw
backing file format: raw
Format specific information:
compat: 1.1
lazy refcounts: false
refcount bits: 16
corrupt: false
```
Generally, we don't expect to corrupte the source.raw anyway, while
actually it does.
Here we check their inode number instead of file name.
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
v2: utilize stat() instead of realpath() (Daniel)
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
block.c | 39 ++++++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 9ad725d205..db4ae57959 100644
--- a/block.c
+++ b/block.c
@@ -6431,6 +6431,37 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
return true;
}
+static bool validate_backing_file(const char *filename,
+ const char *backing_file, Error **errp)
+{
+ struct stat filename_stat, backing_stat;
+
+ if (backing_file[0] == '\0') {
+ error_setg(errp, "Expected backing file name, got empty string");
+ goto out;
+ }
+
+ /* check whether filename and backing_file are refering to the same file */
+ if (stat(backing_file, &backing_stat) == -1) {
+ error_setg(errp, "Cannot stat backing file %s", backing_file);
+ goto out;
+ }
+ if (stat(filename, &filename_stat) == -1) {
+ /* Simply consider filename doesn't exist, no need to further check */
+ return true;
+ }
+ if ((filename_stat.st_dev == backing_stat.st_dev) &&
+ (filename_stat.st_ino == backing_stat.st_ino)) {
+ error_setg(errp, "Error: Trying to create an image with the "
+ "same filename as the backing file");
+ goto out;
+ }
+
+ return true;
+out:
+ return false;
+}
+
void bdrv_img_create(const char *filename, const char *fmt,
const char *base_filename, const char *base_fmt,
char *options, uint64_t img_size, int flags, bool quiet,
@@ -6507,13 +6538,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
if (backing_file) {
- if (!strcmp(filename, backing_file)) {
- error_setg(errp, "Error: Trying to create an image with the "
- "same filename as the backing file");
- goto out;
- }
- if (backing_file[0] == '\0') {
- error_setg(errp, "Expected backing file name, got empty string");
+ if (!validate_backing_file(filename, backing_file, errp)) {
goto out;
}
}
--
2.30.2
On Tue, May 11, 2021 at 01:55:18PM +0800, Li Zhijian wrote:
> Image below user cases:
> case 1:
> ```
> $ qemu-img create -f raw source.raw 1G
> $ qemu-img create -f qcow2 -F raw -b source.raw ./source.raw
> qemu-img info source.raw
> image: source.raw
> file format: qcow2
> virtual size: 193K (197120 bytes)
> disk size: 196K
> cluster_size: 65536
> backing file: source.raw <<<<<<
> backing file format: raw
> Format specific information:
> compat: 1.1
> lazy refcounts: false
> refcount bits: 16
> corrupt: false
> ```
>
> case 2:
> ```
> $ qemu-img create -f raw source.raw 1G
> $ ln -sf source.raw destination.qcow2
> $ qemu-img create -f qcow2 -F raw -b source.raw ./destination.qcow2
> qemu-img info source.raw
> image: source.raw
> file format: qcow2 <<<<<<
> virtual size: 2.0G (2147483648 bytes)
> disk size: 196K
> cluster_size: 65536
> backing file: source.raw
> backing file format: raw
> Format specific information:
> compat: 1.1
> lazy refcounts: false
> refcount bits: 16
> corrupt: false
> ```
> Generally, we don't expect to corrupte the source.raw anyway, while
> actually it does.
>
> Here we check their inode number instead of file name.
>
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>
> ---
> v2: utilize stat() instead of realpath() (Daniel)
>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
> block.c | 39 ++++++++++++++++++++++++++++++++-------
> 1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9ad725d205..db4ae57959 100644
> --- a/block.c
> +++ b/block.c
> @@ -6431,6 +6431,37 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
> return true;
> }
>
> +static bool validate_backing_file(const char *filename,
> + const char *backing_file, Error **errp)
> +{
> + struct stat filename_stat, backing_stat;
> +
> + if (backing_file[0] == '\0') {
> + error_setg(errp, "Expected backing file name, got empty string");
> + goto out;
> + }
> +
> + /* check whether filename and backing_file are refering to the same file */
> + if (stat(backing_file, &backing_stat) == -1) {
> + error_setg(errp, "Cannot stat backing file %s", backing_file);
> + goto out;
> + }
> + if (stat(filename, &filename_stat) == -1) {
> + /* Simply consider filename doesn't exist, no need to further check */
> + return true;
> + }
> + if ((filename_stat.st_dev == backing_stat.st_dev) &&
> + (filename_stat.st_ino == backing_stat.st_ino)) {
> + error_setg(errp, "Error: Trying to create an image with the "
> + "same filename as the backing file");
> + goto out;
> + }
> +
> + return true;
> +out:
> + return false;
> +}
> +
> void bdrv_img_create(const char *filename, const char *fmt,
> const char *base_filename, const char *base_fmt,
> char *options, uint64_t img_size, int flags, bool quiet,
> @@ -6507,13 +6538,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>
> backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
> if (backing_file) {
> - if (!strcmp(filename, backing_file)) {
> - error_setg(errp, "Error: Trying to create an image with the "
> - "same filename as the backing file");
> - goto out;
> - }
> - if (backing_file[0] == '\0') {
> - error_setg(errp, "Expected backing file name, got empty string");
> + if (!validate_backing_file(filename, backing_file, errp)) {
> goto out;
> }
> }
Thinking about this again, this seems to be quite high in the generic block
layer code. As such I don't think we can assume that the backing file here
is actually a plain file on disk. IIUC the backing file could still be any
of the block drivers. Only once we get down into the protocol specific
drivers can be validate the type of backend.
I'm not sure what the right way to deal with that is, so perhaps Kevin or
Max can make a suggestion.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Am 11.05.2021 um 10:35 hat Daniel P. Berrangé geschrieben:
> On Tue, May 11, 2021 at 01:55:18PM +0800, Li Zhijian wrote:
> > Image below user cases:
> > case 1:
> > ```
> > $ qemu-img create -f raw source.raw 1G
> > $ qemu-img create -f qcow2 -F raw -b source.raw ./source.raw
> > qemu-img info source.raw
> > image: source.raw
> > file format: qcow2
> > virtual size: 193K (197120 bytes)
> > disk size: 196K
> > cluster_size: 65536
> > backing file: source.raw <<<<<<
> > backing file format: raw
> > Format specific information:
> > compat: 1.1
> > lazy refcounts: false
> > refcount bits: 16
> > corrupt: false
> > ```
> >
> > case 2:
> > ```
> > $ qemu-img create -f raw source.raw 1G
> > $ ln -sf source.raw destination.qcow2
> > $ qemu-img create -f qcow2 -F raw -b source.raw ./destination.qcow2
> > qemu-img info source.raw
> > image: source.raw
> > file format: qcow2 <<<<<<
> > virtual size: 2.0G (2147483648 bytes)
> > disk size: 196K
> > cluster_size: 65536
> > backing file: source.raw
> > backing file format: raw
> > Format specific information:
> > compat: 1.1
> > lazy refcounts: false
> > refcount bits: 16
> > corrupt: false
> > ```
> > Generally, we don't expect to corrupte the source.raw anyway, while
> > actually it does.
> >
> > Here we check their inode number instead of file name.
> >
> > Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> >
> > ---
> > v2: utilize stat() instead of realpath() (Daniel)
> >
> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> > ---
> > block.c | 39 ++++++++++++++++++++++++++++++++-------
> > 1 file changed, 32 insertions(+), 7 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 9ad725d205..db4ae57959 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -6431,6 +6431,37 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
> > return true;
> > }
> >
> > +static bool validate_backing_file(const char *filename,
> > + const char *backing_file, Error **errp)
> > +{
> > + struct stat filename_stat, backing_stat;
> > +
> > + if (backing_file[0] == '\0') {
> > + error_setg(errp, "Expected backing file name, got empty string");
> > + goto out;
> > + }
> > +
> > + /* check whether filename and backing_file are refering to the same file */
> > + if (stat(backing_file, &backing_stat) == -1) {
> > + error_setg(errp, "Cannot stat backing file %s", backing_file);
> > + goto out;
> > + }
> > + if (stat(filename, &filename_stat) == -1) {
> > + /* Simply consider filename doesn't exist, no need to further check */
> > + return true;
> > + }
> > + if ((filename_stat.st_dev == backing_stat.st_dev) &&
> > + (filename_stat.st_ino == backing_stat.st_ino)) {
> > + error_setg(errp, "Error: Trying to create an image with the "
> > + "same filename as the backing file");
> > + goto out;
> > + }
> > +
> > + return true;
> > +out:
> > + return false;
> > +}
> > +
> > void bdrv_img_create(const char *filename, const char *fmt,
> > const char *base_filename, const char *base_fmt,
> > char *options, uint64_t img_size, int flags, bool quiet,
> > @@ -6507,13 +6538,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> >
> > backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
> > if (backing_file) {
> > - if (!strcmp(filename, backing_file)) {
> > - error_setg(errp, "Error: Trying to create an image with the "
> > - "same filename as the backing file");
> > - goto out;
> > - }
> > - if (backing_file[0] == '\0') {
> > - error_setg(errp, "Expected backing file name, got empty string");
> > + if (!validate_backing_file(filename, backing_file, errp)) {
> > goto out;
> > }
> > }
>
> Thinking about this again, this seems to be quite high in the generic block
> layer code. As such I don't think we can assume that the backing file here
> is actually a plain file on disk. IIUC the backing file could still be any
> of the block drivers. Only once we get down into the protocol specific
> drivers can be validate the type of backend.
Yes, you definitely can't assume that filename is really a local file
name here. It could be any other protocol supported by QEMU, or even use
the json: pseudo-protocol.
> I'm not sure what the right way to deal with that is, so perhaps Kevin or
> Max can make a suggestion.
Can we just keep the backing file open with write permissions unshared
so that locking will fail for the new image? Or would that error
condition be detected too late so that the image would already be
truncated?
Kevin
On 12/05/2021 23.10, Kevin Wolf wrote:
> Am 11.05.2021 um 10:35 hat Daniel P. Berrangé geschrieben:
>> On Tue, May 11, 2021 at 01:55:18PM +0800, Li Zhijian wrote:
>>> void bdrv_img_create(const char *filename, const char *fmt,
>>> const char *base_filename, const char *base_fmt,
>>> char *options, uint64_t img_size, int flags, bool quiet,
>>> @@ -6507,13 +6538,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>>>
>>> backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
>>> if (backing_file) {
>>> - if (!strcmp(filename, backing_file)) {
>>> - error_setg(errp, "Error: Trying to create an image with the "
>>> - "same filename as the backing file");
>>> - goto out;
>>> - }
>>> - if (backing_file[0] == '\0') {
>>> - error_setg(errp, "Expected backing file name, got empty string");
>>> + if (!validate_backing_file(filename, backing_file, errp)) {
>>> goto out;
>>> }
>>> }
>> Thinking about this again, this seems to be quite high in the generic block
>> layer code. As such I don't think we can assume that the backing file here
>> is actually a plain file on disk. IIUC the backing file could still be any
>> of the block drivers. Only once we get down into the protocol specific
>> drivers can be validate the type of backend.
> Yes, you definitely can't assume that filename is really a local file
> name here. It could be any other protocol supported by QEMU, or even use
> the json: pseudo-protocol.
>
>> I'm not sure what the right way to deal with that is, so perhaps Kevin or
>> Max can make a suggestion.
> Can we just keep the backing file open with write permissions unshared
> so that locking will fail for the new image?
*
Not sure if i have understood. In my understanding, open(2) cannot support 'open with write permissions unshared',
it has to cooperate with flock(2)/fcntl(2) to accomplish writing exclusively.
Currently, qemu block also doesn't support 'open with write permissions unshared', but i found something:
#define BDRV_O_NO_SHARE0x0001 /* don't share permissions */
And I have tried below changes and expect the block fails to write the image.
@@ -6563,7 +6563,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
assert(full_backing);
/* backing files always opened read-only */
- back_flags = flags;
+ back_flags = flags | BDRV_O_NO_SHARE;
back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
backing_options = qdict_new();
But in practice, the image is created successfully.
So do you mean we should implement a new flag like 'BDRV_O_NO_SHARE_WR' to handle this
*
Thanks
Zhijian
> Or would that error
> condition be detected too late so that the image would already be
> truncated?
>
> Kevin
>
>
>
© 2016 - 2024 Red Hat, Inc.