From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <m.frank@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Fri, 5 May 2023 10:27:09 +0200 (CEST)
Message-ID: <e1d870c7-f1ff-da61-8859-84414f2ed30a@proxmox.com>
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 <m.frank@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
=?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= <f.gruenbichler@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <m.frank@proxmox.com>
>> ---
>> 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
>
>