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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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 ;)).

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

> +    },
> +    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?

> +
> +	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? :)

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