From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A234A99F06 for ; Thu, 4 May 2023 10:39:34 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 817201BBA1 for ; Thu, 4 May 2023 10:39:34 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 4 May 2023 10:39:33 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 28A5547280 for ; Thu, 4 May 2023 10:39:33 +0200 (CEST) Date: Thu, 04 May 2023 10:39:25 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230425102136.85334-1-m.frank@proxmox.com> <20230425102136.85334-7-m.frank@proxmox.com> In-Reply-To: <20230425102136.85334-7-m.frank@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1683188359.qalzq3g083.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.075 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemuserver.pm, proxmox.com, qemu.pm, gitlab.io, linux-kvm.org] Subject: Re: [pve-devel] [PATCH qemu-server v4 6/6] feature #1027: virtio-9p & virtio-fs support X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 May 2023 08:39:34 -0000 On April 25, 2023 12:21 pm, Markus Frank wrote: > adds support for sharing directorys with a guest vm >=20 > virtio-9p can be simply started with qemu 9p is not really maintained anymore upstream AFAIK (only "Odd Fixes"), and had security issues in the past. Is there a good reason for supporting it (we didn't want to in the past ;)). > virtio-fs needs virtiofsd to be started it also is not compatible with migration, including savevm (so, snapshots with RAM, suspend to disk). until that changes, checks need to be added to error out early when attempting to do such an action. >=20 > Signed-off-by: Markus Frank > --- > PVE/API2/Qemu.pm | 19 +++++++ > PVE/QemuServer.pm | 141 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 160 insertions(+) >=20 > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index 587bb22..810e124 100644 > --- a/PVE/API2/Qemu.pm > +++ b/PVE/API2/Qemu.pm > @@ -639,6 +639,8 @@ my $check_vm_modify_config_perm =3D sub { > # the user needs Disk and PowerMgmt privileges to change the vmstat= e > # also needs privileges on the storage, that will be checked later > $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Disk', = 'VM.PowerMgmt' ]); > + } elsif ($opt =3D~ m/^sharedfiles\d$/) { > + # needs $param->{$opt} so checkout $check_vm_dir_perm this is not quite correct - you should still check the permission for modifying the VM config here (VM.Config.Options or VM.Config.Disk?). of course, like for other things, there are extra checks later on to see whether you are allowed to add/remove/modify a particular shareddir usage. disks also require storage permissions which are not handled here, for example, but the general 'allowed to change disk aspects of VM' is. > } else { > # catches hostpci\d+, args, lock, etc. > # new options will be checked here > @@ -649,6 +651,20 @@ my $check_vm_modify_config_perm =3D sub { > return 1; > }; > =20 > +my $check_vm_dir_perm =3D sub { > + my ($rpcenv, $authuser, $param) =3D @_; > + > + return 1 if $authuser eq 'root@pam'; > + > + foreach my $opt (keys %{$param}) { > + if ($opt =3D~ m/^sharedfiles\d$/) { > + my $sharedfiles =3D PVE::QemuServer::parse_sharedfiles($param->{$op= t}); > + $rpcenv->check_dir_perm($authuser, $sharedfiles->{dirid}, ['Map.Use= ']); > + } > + } > + return 1; > +}; > + > __PACKAGE__->register_method({ > name =3D> 'vmlist', > path =3D> '', > @@ -876,6 +892,7 @@ __PACKAGE__->register_method({ > =20 > &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ k= eys %$param]); > =20 > + &$check_vm_dir_perm($rpcenv, $authuser, $param); > &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $pa= ram); > &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param= ); > =20 > @@ -1576,6 +1593,8 @@ my $update_vm_api =3D sub { > =20 > &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, undef, [key= s %$param]); > =20 > + &$check_vm_dir_perm($rpcenv, $authuser, $param); > + > &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param)= ; > =20 > my $updatefn =3D sub { > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm > index c1d0fd2..fc07311 100644 > --- a/PVE/QemuServer.pm > +++ b/PVE/QemuServer.pm > @@ -274,6 +274,31 @@ my $rng_fmt =3D { > }, > }; > =20 > +my $sharedfiles_fmt =3D { > + type =3D> { > + type =3D> 'string', > + default_key =3D> 1, > + enum =3D> ['virtio-9p', 'virtio-fs'], > + description =3D> "sharedfiles via" > + ." virtio-9p (https://www.linux-kvm.org/page/9p_virtio)" > + ." or virtio-fs (https://virtio-fs.gitlab.io/howto-qemu.html)", > + format_description =3D> "virtio-sharedfiles-type", > + optional =3D> 1, why is this optional? > + }, > + dirid =3D> { > + type =3D> 'string', > + description =3D> "dirid of directory you want to share with the guest V= M", > + format_description =3D> "virtio-sharedfiles-dirid", > + optional =3D> 1, why is this optional? > + }, > + tag =3D> { > + type =3D> 'string', > + description =3D> "tag name for mounting in the guest VM", > + format_description =3D> "virtio-sharedfiles-tag", > + optional =3D> 1, is the tag actually optional? > + }, see my comments in the manager patch, I think there are quite a few more knobs that we might want to expose here. > +}; > + > my $meta_info_fmt =3D { > 'ctime' =3D> { > type =3D> 'integer', > @@ -832,6 +857,7 @@ while (my ($k, $v) =3D each %$confdesc) { > =20 > my $MAX_USB_DEVICES =3D 14; > my $MAX_NETS =3D 32; > +my $MAX_SHAREDFILES =3D 10; > my $MAX_SERIAL_PORTS =3D 4; > my $MAX_PARALLEL_PORTS =3D 3; > my $MAX_NUMA =3D 8; > @@ -974,6 +1000,12 @@ my $netdesc =3D { > description =3D> "Specify network devices.", > }; > =20 > +my $sharedfilesdesc =3D { > + optional =3D> 1, > + type =3D> 'string', format =3D> $sharedfiles_fmt, > + description =3D> "share files between host and guest", > +}; > + > PVE::JSONSchema::register_standard_option("pve-qm-net", $netdesc); > =20 > my $ipconfig_fmt =3D { > @@ -1035,6 +1067,10 @@ for (my $i =3D 0; $i < $MAX_NETS; $i++) { > $confdesc_cloudinit->{"ipconfig$i"} =3D $ipconfigdesc; > } > =20 > +for (my $i =3D 0; $i < $MAX_SHAREDFILES; $i++) { > + $confdesc->{"sharedfiles$i"} =3D $sharedfilesdesc; > +} > + > foreach my $key (keys %$confdesc_cloudinit) { > $confdesc->{$key} =3D $confdesc_cloudinit->{$key}; > } > @@ -1988,6 +2024,16 @@ sub parse_net { > return $res; > } > =20 > +sub parse_sharedfiles { > + my ($value) =3D @_; > + > + return if !$value; > + my $res =3D eval { parse_property_string($sharedfiles_fmt, $value) }= ; > + > + warn $@ if $@; > + return $res; > +} > + > # ipconfigX ip=3Dcidr,gw=3Dip,ip6=3Dcidr,gw6=3Dip > sub parse_ipconfig { > my ($data) =3D @_; > @@ -4105,6 +4151,44 @@ sub config_to_command { > push @$devices, '-device', $netdevicefull; > } > =20 > + my $onevirtiofs =3D 0; > + for (my $i =3D 0; $i < $MAX_SHAREDFILES; $i++) { what about hotplug support? > + my $sharedfilesstr =3D "sharedfiles$i"; > + > + next if !$conf->{$sharedfilesstr}; > + my $sharedfiles =3D parse_sharedfiles($conf->{$sharedfilesstr}); > + next if !$sharedfiles; > + > + die $sharedfilesstr.' needs a type (virtio-9p or virtio-fs)' if !$share= dfiles->{type}; > + die $sharedfilesstr.' needs a dirid of a directory to share' if !$share= dfiles->{dirid}; > + die $sharedfilesstr.' needs a mount tag' if !$sharedfiles->{tag}; so I guess all three are not optional ;) > + > + if ($sharedfiles->{type} eq 'virtio-fs' && $conf->{numa}) { > + die "disable numa to use virtio-fs or use virtio-9p instead"; > + } okay, that is an interesting restriction. is that just because it requires some other, complex changes, or is it not working at all? > + > + my $path =3D PVE::DirConfig::extract_dir_path($nodename, $sharedfiles->= {dirid}); > + > + if ($sharedfiles->{type} eq 'virtio-9p') { > + push @$devices, '-fsdev', "local,security_model=3Dpassthrough,id=3D= fsdev$i" > + .",path=3D$path"; > + push @$devices, '-device', "virtio-9p-pci,id=3Dfs9p$i,fsdev=3Dfsdev= $i" > + .",mount_tag=3D$sharedfiles->{tag}"; > + } > + if ($sharedfiles->{type} eq 'virtio-fs') { > + push @$devices, '-chardev', "socket,id=3Dvirtfs$i,path=3D/var/run/v= irtiofsd/vm$vmid-fs$i"; > + push @$devices, '-device', 'vhost-user-fs-pci,queue-size=3D1024' > + .",chardev=3Dvirtfs$i,tag=3D$sharedfiles->{tag}"; > + $onevirtiofs =3D 1; > + } > + } > + > + if ($onevirtiofs) { > + push @$devices, '-object', 'memory-backend-file,id=3Dmem' > + .",size=3D$conf->{memory}M,mem-path=3D/dev/shm,share=3Don"; > + push @$devices, '-numa', 'node,memdev=3Dmem'; > + } > + > if ($conf->{ivshmem}) { > my $ivshmem =3D parse_property_string($ivshmem_fmt, $conf->{ivshmem}); > =20 > @@ -4190,6 +4274,41 @@ sub config_to_command { > return wantarray ? ($cmd, $vollist, $spice_port) : $cmd; > } > =20 > +sub start_virtiofs { how/when are they stopped again? :) > + my ($vmid, $path, $fsid, $dirid) =3D @_; > + > + my $socket_path_root =3D "/var/run/virtiofsd"; > + mkdir $socket_path_root; > + my $socket_path =3D "$socket_path_root/vm$vmid-fs$fsid"; > + unlink($socket_path); > + my $socket =3D IO::Socket::UNIX->new( > + Type =3D> SOCK_STREAM, > + Local =3D> $socket_path, > + Listen =3D> 1, > + ) or die "cannot create socket - $!\n"; > + > + my $flags =3D fcntl($socket, F_GETFD, 0) > + or die "failed to get file descriptor flags: $!\n"; > + fcntl($socket, F_SETFD, $flags & ~FD_CLOEXEC) > + or die "failed to remove FD_CLOEXEC from file descriptor\n"; > + > + my $fd =3D $socket->fileno(); > + > + my $pid =3D fork(); > + if ($pid =3D=3D 0) { > + # TODO replace with rust implementation in bookworm > + run_command(['/usr/lib/kvm/virtiofsd', "--fd=3D$fd", > + '-o', "source=3D$path", '-o', 'cache=3Dalways']); > + POSIX::_exit(0); > + } elsif (!defined($pid)) { > + die "could not fork to start virtiofsd"; > + } > + > + # return socket to keep it alive, > + # so that qemu will wait for virtiofsd to start > + return $socket; > +} > + > sub check_rng_source { > my ($source) =3D @_; > =20 > @@ -5747,6 +5866,23 @@ sub vm_start_nolock { > my ($cmd, $vollist, $spice_port) =3D config_to_command($storecfg, $v= mid, > $conf, $defaults, $forcemachine, $forcecpu, $params->{'pbs-backing'}); > =20 > + my $nodename =3D nodename(); > + my @sockets; > + for (my $i =3D 0; $i < $MAX_SHAREDFILES; $i++) { > + my $sharedfilesstr =3D "sharedfiles$i"; > + > + next if !$conf->{$sharedfilesstr}; > + my $sharedfiles =3D parse_sharedfiles($conf->{$sharedfilesstr}); > + next if !$sharedfiles; > + > + my $path =3D PVE::DirConfig::extract_dir_path($nodename, $sharedfiles->= {dirid}); > + > + if ($sharedfiles && $sharedfiles->{type} eq 'virtio-fs' && !$conf->{num= a}) { > + my $socket =3D start_virtiofs($vmid, $path, $i, $sharedfiles->{diri= d}); > + push @sockets, $socket; > + } > + } > + > my $migration_ip; > my $get_migration_ip =3D sub { > my ($nodename) =3D @_; > @@ -6094,6 +6230,11 @@ sub vm_start_nolock { > =20 > PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'post-start'); > =20 > + foreach my $socket (@sockets) { > + shutdown($socket, 2); > + close($socket); > + } > + > return $res; > } > =20 > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20