[pve-devel] [PATCH qemu-server] fix #5284: diallow moving vm disks to storages not meant for images

[pve-devel] [PATCH qemu-server] fix #5284: diallow moving vm disks to storages not meant for images

[Daniel Kral]

Adds a check if the target storage of a VM disk move between two
different storages supports the content type 'images'. Without the
check, it will move the disk image to storages which do not support VM
images, which causes the VM to fail at startup (for any non-unused
disks).

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
There is a content type check for any used volume (e.g. 'scsi0', ...) at
PVE/QemuServer.pm:3964 in the subroutine `config_to_command` which will
(at least) make the VM fail to start unless the volumes are moved to a
storage that supports images again.

Also, just as a note, moving the disk to storages that do not support
the `vdisk_alloc` method in the storage plugin (like PBS) also
rightfully fail before and after this change.

 PVE/API2/Qemu.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index d25a79fe..b6ba9d21 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4409,6 +4409,9 @@ __PACKAGE__->register_method({
 	    );
 	} elsif ($storeid) {
 	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
+	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
+	    raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
+		if !$scfg->{content}->{images};
 
 	    $load_and_check_move->(); # early checks before forking/locking
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server] fix #5284: diallow moving vm disks to storages not meant for images

[Filip Schauer]

I tried to move a VM disk from a directory storarge to another directory
storage that does not support the content type 'images'.

```
$ qm disk move 103 efidisk0 local2
400 Parameter verification failed.
storage: storage 'local2' does not support vm images
qm disk move <vmid> <disk> [<storage>] [OPTIONS]
```

As expected, it was prevented.

Trying to move a VM disk to a Proxmox Backup Server now returns a more
fitting error message.

Before the patch:

```
$ qm disk move 103 efidisk0 store1 --delete
create full clone of drive efidisk0 (local:103/vm-103-disk-0.qcow2)
storage migration failed: can't allocate space in pbs storage
```

After the patch:

```
$ qm disk move 103 efidisk0 store1 --delete
400 Parameter verification failed.
storage: storage 'store1' does not support vm images
qm disk move <vmid> <disk> [<storage>] [OPTIONS]
```

PS: The title has a typo: "diallow"

Tested-By: Filip Schauer <f.schauer@proxmox.com>

On 29/08/2024 16:26, Daniel Kral wrote:
> Adds a check if the target storage of a VM disk move between two
> different storages supports the content type 'images'. Without the
> check, it will move the disk image to storages which do not support VM
> images, which causes the VM to fail at startup (for any non-unused
> disks).
>
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> There is a content type check for any used volume (e.g. 'scsi0', ...) at
> PVE/QemuServer.pm:3964 in the subroutine `config_to_command` which will
> (at least) make the VM fail to start unless the volumes are moved to a
> storage that supports images again.
>
> Also, just as a note, moving the disk to storages that do not support
> the `vdisk_alloc` method in the storage plugin (like PBS) also
> rightfully fail before and after this change.
>
>   PVE/API2/Qemu.pm | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index d25a79fe..b6ba9d21 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4409,6 +4409,9 @@ __PACKAGE__->register_method({
>   	    );
>   	} elsif ($storeid) {
>   	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
> +	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +	    raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
> +		if !$scfg->{content}->{images};
>   
>   	    $load_and_check_move->(); # early checks before forking/locking
>   


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server] fix #5284: diallow moving vm disks to storages not meant for images

[Thomas Lamprecht]

Am 29/08/2024 um 16:26 schrieb Daniel Kral:
> Adds a check if the target storage of a VM disk move between two
> different storages supports the content type 'images'. Without the
> check, it will move the disk image to storages which do not support VM
> images, which causes the VM to fail at startup (for any non-unused
> disks).
> 
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
> There is a content type check for any used volume (e.g. 'scsi0', ...) at
> PVE/QemuServer.pm:3964 in the subroutine `config_to_command` which will
> (at least) make the VM fail to start unless the volumes are moved to a
> storage that supports images again.
> 
> Also, just as a note, moving the disk to storages that do not support
> the `vdisk_alloc` method in the storage plugin (like PBS) also
> rightfully fail before and after this change.
> 
>  PVE/API2/Qemu.pm | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index d25a79fe..b6ba9d21 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4409,6 +4409,9 @@ __PACKAGE__->register_method({
>  	    );
>  	} elsif ($storeid) {
>  	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
> +	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +	    raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
> +		if !$scfg->{content}->{images};

Hmm, code wise it looks OK, but not so sure if this is the best place
to check, I'd rather look into either moving this into the $load_and_check_move
closure or into the PVE::QemuServer::clone_disk method, avoiding such an issue
for all other call sites too, albeit one would need to evaluate those call sites
if it does not break an existing usecase when disallowing this everywhere.

btw. did you check if containers are also affected by this bug?

>  
>  	    $load_and_check_move->(); # early checks before forking/locking
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server] fix #5284: diallow moving vm disks to storages not meant for images

[Daniel Kral]

On 9/6/24 18:44, Thomas Lamprecht wrote:
> Hmm, code wise it looks OK, but not so sure if this is the best place
> to check, I'd rather look into either moving this into the $load_and_check_move
> closure or into the PVE::QemuServer::clone_disk method, avoiding such an issue
> for all other call sites too, albeit one would need to evaluate those call sites
> if it does not break an existing usecase when disallowing this everywhere.

I will take a closer look into this issue, because - as you already 
pointed out - there are some other commands that allocate disk images, 
but do not check for the storage's content type(s).

In particular it was for moving disks and cloning disks / VMs, but I 
will also check the behavior at other disk allocations with respect to 
the context of the call sites.

> btw. did you check if containers are also affected by this bug?

I forgot to mention it, but I did. I discovered that `pve-container` has 
a check in a "proxy" subroutine (`PVE::LXC::alloc_disk`) that will in 
turn call `PVE::Storage::vdisk_alloc` if it's okay to do so.

I had a talk with Fiona about this and we agreed that it would make 
sense to have a similar subroutine here as well. I will follow up with 
another patch (series) for this in the following days.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server] fix #5284: diallow moving vm disks to storages not meant for images

[Daniel Kral]

On 8/29/24 16:26, Daniel Kral wrote:
> Adds a check if the target storage of a VM disk move between two
> different storages supports the content type 'images'. Without the
> check, it will move the disk image to storages which do not support VM
> images, which causes the VM to fail at startup (for any non-unused
> disks).

This patch is superseded by another RFC patch series at [1].

[1] 
https://lore.proxmox.com/pve-devel/20240916163839.236908-1-d.kral@proxmox.com/


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel