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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A8DD699FFA for ; Fri, 5 May 2023 10:27:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8FDCB25734 for ; Fri, 5 May 2023 10:27:11 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 5 May 2023 10:27:10 +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 AD98B47599 for ; Fri, 5 May 2023 10:27:09 +0200 (CEST) Message-ID: Date: Fri, 5 May 2023 10:27:07 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 From: Markus Frank To: Proxmox VE development discussion , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20230425102136.85334-1-m.frank@proxmox.com> <20230425102136.85334-7-m.frank@proxmox.com> <1683188359.qalzq3g083.astroid@yuna.none> Content-Language: en-US In-Reply-To: <1683188359.qalzq3g083.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.689 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_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -4.28 Looks like a legit reply (A) 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 - 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: Fri, 05 May 2023 08:27:11 -0000 Thanks for your review. On 5/4/23 10:39, Fabian Grünbichler wrote: > On April 25, 2023 12:21 pm, Markus Frank wrote: >> adds support for sharing directorys with a guest vm >> >> 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 ;)> Except that it does not need a daemon to be started, no. I will remove it. >> 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. > >> >> Signed-off-by: Markus Frank >> --- >> PVE/API2/Qemu.pm | 19 +++++++ >> PVE/QemuServer.pm | 141 ++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 160 insertions(+) >> >> 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 = sub { >> # the user needs Disk and PowerMgmt privileges to change the vmstate >> # 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 =~ 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 = sub { >> return 1; >> }; >> >> +my $check_vm_dir_perm = sub { >> + my ($rpcenv, $authuser, $param) = @_; >> + >> + return 1 if $authuser eq 'root@pam'; >> + >> + foreach my $opt (keys %{$param}) { >> + if ($opt =~ m/^sharedfiles\d$/) { >> + my $sharedfiles = PVE::QemuServer::parse_sharedfiles($param->{$opt}); >> + $rpcenv->check_dir_perm($authuser, $sharedfiles->{dirid}, ['Map.Use']); >> + } >> + } >> + return 1; >> +}; >> + >> __PACKAGE__->register_method({ >> name => 'vmlist', >> path => '', >> @@ -876,6 +892,7 @@ __PACKAGE__->register_method({ >> >> &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]); >> >> + &$check_vm_dir_perm($rpcenv, $authuser, $param); >> &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param); >> &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param); >> >> @@ -1576,6 +1593,8 @@ my $update_vm_api = sub { >> >> &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, undef, [keys %$param]); >> >> + &$check_vm_dir_perm($rpcenv, $authuser, $param); >> + >> &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param); >> >> my $updatefn = 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 = { >> }, >> }; >> >> +my $sharedfiles_fmt = { >> + type => { >> + type => 'string', >> + default_key => 1, >> + enum => ['virtio-9p', 'virtio-fs'], >> + description => "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 => "virtio-sharedfiles-type", >> + optional => 1, > > why is this optional? I forgot to delete > >> + }, >> + dirid => { >> + type => 'string', >> + description => "dirid of directory you want to share with the guest VM", >> + format_description => "virtio-sharedfiles-dirid", >> + optional => 1, > > why is this optional? > >> + }, >> + tag => { >> + type => 'string', >> + description => "tag name for mounting in the guest VM", >> + format_description => "virtio-sharedfiles-tag", >> + optional => 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 = { >> 'ctime' => { >> type => 'integer', >> @@ -832,6 +857,7 @@ while (my ($k, $v) = each %$confdesc) { >> >> my $MAX_USB_DEVICES = 14; >> my $MAX_NETS = 32; >> +my $MAX_SHAREDFILES = 10; >> my $MAX_SERIAL_PORTS = 4; >> my $MAX_PARALLEL_PORTS = 3; >> my $MAX_NUMA = 8; >> @@ -974,6 +1000,12 @@ my $netdesc = { >> description => "Specify network devices.", >> }; >> >> +my $sharedfilesdesc = { >> + optional => 1, >> + type => 'string', format => $sharedfiles_fmt, >> + description => "share files between host and guest", >> +}; >> + >> PVE::JSONSchema::register_standard_option("pve-qm-net", $netdesc); >> >> my $ipconfig_fmt = { >> @@ -1035,6 +1067,10 @@ for (my $i = 0; $i < $MAX_NETS; $i++) { >> $confdesc_cloudinit->{"ipconfig$i"} = $ipconfigdesc; >> } >> >> +for (my $i = 0; $i < $MAX_SHAREDFILES; $i++) { >> + $confdesc->{"sharedfiles$i"} = $sharedfilesdesc; >> +} >> + >> foreach my $key (keys %$confdesc_cloudinit) { >> $confdesc->{$key} = $confdesc_cloudinit->{$key}; >> } >> @@ -1988,6 +2024,16 @@ sub parse_net { >> return $res; >> } >> >> +sub parse_sharedfiles { >> + my ($value) = @_; >> + >> + return if !$value; >> + my $res = eval { parse_property_string($sharedfiles_fmt, $value) }; >> + >> + warn $@ if $@; >> + return $res; >> +} >> + >> # ipconfigX ip=cidr,gw=ip,ip6=cidr,gw6=ip >> sub parse_ipconfig { >> my ($data) = @_; >> @@ -4105,6 +4151,44 @@ sub config_to_command { >> push @$devices, '-device', $netdevicefull; >> } >> >> + my $onevirtiofs = 0; >> + for (my $i = 0; $i < $MAX_SHAREDFILES; $i++) { > > what about hotplug support? > >> + my $sharedfilesstr = "sharedfiles$i"; >> + >> + next if !$conf->{$sharedfilesstr}; >> + my $sharedfiles = parse_sharedfiles($conf->{$sharedfilesstr}); >> + next if !$sharedfiles; >> + >> + die $sharedfilesstr.' needs a type (virtio-9p or virtio-fs)' if !$sharedfiles->{type}; >> + die $sharedfilesstr.' needs a dirid of a directory to share' if !$sharedfiles->{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? I thought so because I have read somewhere it will not work. Also do not know why qemu returns this error when noma and virtiofsd is configured: kvm: -chardev socket,id=virtfs0,path=/var/run/virtiofsd/vm100-fs0: Failed to connect to '/var/run/virtiofsd/vm100-fs0': Connection refused I will try to find a solution. > >> + >> + my $path = PVE::DirConfig::extract_dir_path($nodename, $sharedfiles->{dirid}); >> + >> + if ($sharedfiles->{type} eq 'virtio-9p') { >> + push @$devices, '-fsdev', "local,security_model=passthrough,id=fsdev$i" >> + .",path=$path"; >> + push @$devices, '-device', "virtio-9p-pci,id=fs9p$i,fsdev=fsdev$i" >> + .",mount_tag=$sharedfiles->{tag}"; >> + } >> + if ($sharedfiles->{type} eq 'virtio-fs') { >> + push @$devices, '-chardev', "socket,id=virtfs$i,path=/var/run/virtiofsd/vm$vmid-fs$i"; >> + push @$devices, '-device', 'vhost-user-fs-pci,queue-size=1024' >> + .",chardev=virtfs$i,tag=$sharedfiles->{tag}"; >> + $onevirtiofs = 1; >> + } >> + } >> + >> + if ($onevirtiofs) { >> + push @$devices, '-object', 'memory-backend-file,id=mem' >> + .",size=$conf->{memory}M,mem-path=/dev/shm,share=on"; >> + push @$devices, '-numa', 'node,memdev=mem'; >> + } >> + >> if ($conf->{ivshmem}) { >> my $ivshmem = parse_property_string($ivshmem_fmt, $conf->{ivshmem}); >> >> @@ -4190,6 +4274,41 @@ sub config_to_command { >> return wantarray ? ($cmd, $vollist, $spice_port) : $cmd; >> } >> >> +sub start_virtiofs { > > how/when are they stopped again? :) virtiofsd stops itself automatically together with qemu. > >> + my ($vmid, $path, $fsid, $dirid) = @_; >> + >> + my $socket_path_root = "/var/run/virtiofsd"; >> + mkdir $socket_path_root; >> + my $socket_path = "$socket_path_root/vm$vmid-fs$fsid"; >> + unlink($socket_path); >> + my $socket = IO::Socket::UNIX->new( >> + Type => SOCK_STREAM, >> + Local => $socket_path, >> + Listen => 1, >> + ) or die "cannot create socket - $!\n"; >> + >> + my $flags = 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 = $socket->fileno(); >> + >> + my $pid = fork(); >> + if ($pid == 0) { >> + # TODO replace with rust implementation in bookworm >> + run_command(['/usr/lib/kvm/virtiofsd', "--fd=$fd", >> + '-o', "source=$path", '-o', 'cache=always']); >> + 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) = @_; >> >> @@ -5747,6 +5866,23 @@ sub vm_start_nolock { >> my ($cmd, $vollist, $spice_port) = config_to_command($storecfg, $vmid, >> $conf, $defaults, $forcemachine, $forcecpu, $params->{'pbs-backing'}); >> >> + my $nodename = nodename(); >> + my @sockets; >> + for (my $i = 0; $i < $MAX_SHAREDFILES; $i++) { >> + my $sharedfilesstr = "sharedfiles$i"; >> + >> + next if !$conf->{$sharedfilesstr}; >> + my $sharedfiles = parse_sharedfiles($conf->{$sharedfilesstr}); >> + next if !$sharedfiles; >> + >> + my $path = PVE::DirConfig::extract_dir_path($nodename, $sharedfiles->{dirid}); >> + >> + if ($sharedfiles && $sharedfiles->{type} eq 'virtio-fs' && !$conf->{numa}) { >> + my $socket = start_virtiofs($vmid, $path, $i, $sharedfiles->{dirid}); >> + push @sockets, $socket; >> + } >> + } >> + >> my $migration_ip; >> my $get_migration_ip = sub { >> my ($nodename) = @_; >> @@ -6094,6 +6230,11 @@ sub vm_start_nolock { >> >> PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'post-start'); >> >> + foreach my $socket (@sockets) { >> + shutdown($socket, 2); >> + close($socket); >> + } >> + >> return $res; >> } >> >> -- >> 2.30.2 >> >> >> >> _______________________________________________ >> pve-devel mailing list >> pve-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >> >> >> > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > >