Re: [pve-devel] [PATCH qemu-server v4 6/6] feature #1027: virtio-9p & virtio-fs support

[Markus Frank <m.frank@proxmox.com>]

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
> 
>