Re: [pve-devel] [PATCH qemu-server v2 7/9] refactor: move rng related code into its own module

[Fiona Ebner <f.ebner@proxmox.com>]

Am 29.01.25 um 16:53 schrieb Filip Schauer:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  PVE/QemuServer.pm       |  83 +++---------------------
>  PVE/QemuServer/Makefile |   1 +
>  PVE/QemuServer/RNG.pm   | 135 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 145 insertions(+), 74 deletions(-)
>  create mode 100644 PVE/QemuServer/RNG.pm
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 5cde94a1..93e65825 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -61,6 +61,7 @@ use PVE::QemuServer::Memory qw(get_current_memory);
>  use PVE::QemuServer::Monitor qw(mon_cmd);
>  use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr print_pcie_root_port parse_hostpci);
>  use PVE::QemuServer::QMPHelpers qw(qemu_deviceadd qemu_devicedel qemu_objectadd qemu_objectdel);
> +use PVE::QemuServer::RNG;
>  use PVE::QemuServer::USB;
>  
>  my $have_sdn;
> @@ -249,39 +250,6 @@ my $spice_enhancements_fmt = {
>      },
>  };
>  
> -my $rng_fmt = {
> -    source => {
> -	type => 'string',
> -	enum => ['/dev/urandom', '/dev/random', '/dev/hwrng'],
> -	default_key => 1,
> -	description => "The file on the host to gather entropy from. In most cases '/dev/urandom'"
> -	    ." should be preferred over '/dev/random' to avoid entropy-starvation issues on the"
> -	    ." host. Using urandom does *not* decrease security in any meaningful way, as it's"
> -	    ." still seeded from real entropy, and the bytes provided will most likely be mixed"
> -	    ." with real entropy on the guest as well. '/dev/hwrng' can be used to pass through"
> -	    ." a hardware RNG from the host.",
> -    },
> -    max_bytes => {
> -	type => 'integer',
> -	description => "Maximum bytes of entropy allowed to get injected into the guest every"
> -	    ." 'period' milliseconds. Prefer a lower value when using '/dev/random' as source. Use"
> -	    ." `0` to disable limiting (potentially dangerous!).",
> -	optional => 1,
> -
> -	# default is 1 KiB/s, provides enough entropy to the guest to avoid boot-starvation issues
> -	# (e.g. systemd etc...) while allowing no chance of overwhelming the host, provided we're
> -	# reading from /dev/urandom
> -	default => 1024,
> -    },
> -    period => {
> -	type => 'integer',
> -	description => "Every 'period' milliseconds the entropy-injection quota is reset, allowing"
> -	    ." the guest to retrieve another 'max_bytes' of entropy.",
> -	optional => 1,
> -	default => 1000,
> -    },
> -};
> -
>  my $meta_info_fmt = {
>      'ctime' => {
>  	type => 'integer',

Does not apply, because this got moved recently ;)

> @@ -724,7 +692,7 @@ EODESCR
>      },
>      rng0 => {
>  	type => 'string',
> -	format => $rng_fmt,
> +	format => $PVE::QemuServer::RNG::rng_fmt,
>  	description => "Configure a VirtIO-based Random Number Generator.",
>  	optional => 1,
>      },

Could instead use the standard option you define in the RNG module.

> @@ -2078,16 +2046,6 @@ sub parse_vga {
>      return $res;
>  }
>  
> -sub parse_rng {
> -    my ($value) = @_;
> -
> -    return if !$value;
> -
> -    my $res = eval { parse_property_string($rng_fmt, $value) };
> -    warn $@ if $@;
> -    return $res;
> -}
> -
>  sub parse_meta_info {
>      my ($value) = @_;
> 

Does not apply, because this got moved recently ;)

> @@ -3940,20 +3898,14 @@ sub config_to_command {
>  	}
>      }
>  
> -    my $rng = $conf->{rng0} ? parse_rng($conf->{rng0}) : undef;
> +    my $rng = $conf->{rng0} ? PVE::QemuServer::RNG::parse_rng($conf->{rng0}) : undef;
>      if ($rng && $version_guard->(4, 1, 2)) {
> -	check_rng_source($rng->{source});
> -
> -	my $max_bytes = $rng->{max_bytes} // $rng_fmt->{max_bytes}->{default};
> -	my $period = $rng->{period} // $rng_fmt->{period}->{default};
> -	my $limiter_str = "";
> -	if ($max_bytes) {
> -	    $limiter_str = ",max-bytes=$max_bytes,period=$period";
> -	}
> -
> -	my $rng_addr = print_pci_addr("rng0", $bridges, $arch, $machine_type);
> -	push @$devices, '-object', "rng-random,filename=$rng->{source},id=rng0";
> -	push @$devices, '-device', "virtio-rng-pci,rng=rng0$limiter_str$rng_addr";
> +	my $rng_object = PVE::QemuServer::RNG::print_rng_object('rng0', $rng);
> +	my $rng_device = PVE::QemuServer::RNG::print_rng_device(
> +	    'rng0', $rng, $bridges, $arch, $machine_type
> +	);

Style nit: that's not how multiline function calls are usually wrapped
in our Perl code base:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Arguments

> +	push @$devices, '-object', $rng_object;
> +	push @$devices, '-device', $rng_device;
>      }
>  
>      my $spice_port;

Would be nice to have the moving to a dedicated module be separate from
adding the new helpers.

> @@ -4235,23 +4187,6 @@ sub config_to_command {
>      return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices) : $cmd;
>  }
>  
> -sub check_rng_source {
> -    my ($source) = @_;
> -
> -    # mostly relevant for /dev/hwrng, but doesn't hurt to check others too
> -    die "cannot create VirtIO RNG device: source file '$source' doesn't exist\n"
> -	if ! -e $source;
> -
> -    my $rng_current = '/sys/devices/virtual/misc/hw_random/rng_current';
> -    if ($source eq '/dev/hwrng' && file_read_firstline($rng_current) eq 'none') {
> -	# Needs to abort, otherwise QEMU crashes on first rng access. Note that rng_current cannot
> -	# be changed to 'none' manually, so once the VM is past this point, it's no longer an issue.
> -	die "Cannot start VM with passed-through RNG device: '/dev/hwrng' exists, but"
> -	    ." '$rng_current' is set to 'none'. Ensure that a compatible hardware-RNG is attached"
> -	    ." to the host.\n";
> -    }
> -}
> -
>  sub spice_port {
>      my ($vmid) = @_;
>  
> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
> index 89d12091..72c287fc 100644
> --- a/PVE/QemuServer/Makefile
> +++ b/PVE/QemuServer/Makefile
> @@ -1,4 +1,5 @@
>  SOURCES=PCI.pm		\
> +	RNG.pm		\
>  	USB.pm		\
>  	Memory.pm	\
>  	ImportDisk.pm	\
> diff --git a/PVE/QemuServer/RNG.pm b/PVE/QemuServer/RNG.pm
> new file mode 100644
> index 00000000..f7a62f3b
> --- /dev/null
> +++ b/PVE/QemuServer/RNG.pm
> @@ -0,0 +1,135 @@
> +package PVE::QemuServer::RNG;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::QemuServer::PCI qw(print_pci_addr);
> +use PVE::JSONSchema;
> +use PVE::Tools qw(file_read_firstline);
> +use base 'Exporter';

Style nit: The QemuServer::PCI module should be grouped below and I'd
prefer having a blank before it:
https://lore.proxmox.com/pve-devel/e24881cf-bfd2-4063-bde2-99f41031f0f0@proxmox.com/
Having the 'use base' be last is fine, but I'd also prefer a blank
before it.

> +
> +our @EXPORT_OK = qw(
> +parse_rng
> +check_rng_source
> +print_rng_device
> +print_rng_object
> +);
> +
> +our $rng_fmt = {
> +    source => {
> +	type => 'string',
> +	enum => ['/dev/urandom', '/dev/random', '/dev/hwrng'],
> +	default_key => 1,
> +	optional => 1,

Adding the optional should not be part of this patch.

> +	description => "The file on the host to gather entropy from. In most cases '/dev/urandom'"
> +	    ." should be preferred over '/dev/random' to avoid entropy-starvation issues on the"
> +	    ." host. Using urandom does *not* decrease security in any meaningful way, as it's"
> +	    ." still seeded from real entropy, and the bytes provided will most likely be mixed"
> +	    ." with real entropy on the guest as well. '/dev/hwrng' can be used to pass through"
> +	    ." a hardware RNG from the host.",

The part about /dev/random is outdated as you pointed out in v1 ;)

> +    },
> +    max_bytes => {
> +	type => 'integer',
> +	description => "Maximum bytes of entropy allowed to get injected into the guest every"
> +	    ." 'period' milliseconds. Prefer a lower value when using '/dev/random' as source. Use"
> +	    ." `0` to disable limiting (potentially dangerous!).",


The part about /dev/random is outdated as you pointed out in v1 ;)

> +	optional => 1,
> +
> +	# default is 1 KiB/s, provides enough entropy to the guest to avoid boot-starvation issues
> +	# (e.g. systemd etc...) while allowing no chance of overwhelming the host, provided we're
> +	# reading from /dev/urandom
> +	default => 1024,
> +    },
> +    period => {
> +	type => 'integer',
> +	description => "Every 'period' milliseconds the entropy-injection quota is reset, allowing"
> +	    ." the guest to retrieve another 'max_bytes' of entropy.",
> +	optional => 1,
> +	default => 1000,
> +    },
> +};
> +
> +PVE::JSONSchema::register_format('pve-qm-rng', $rng_fmt);

Since you register the format here, you don't need to make $rng_fmt
shared with 'our' or? (Still need to include the module in QemuServer.pm
to have registering come first).

> +
> +our $rngdesc = {
> +    type => 'string',
> +    format => $rng_fmt,
> +    optional => 1,
> +    description => "Configure a VirtIO-based Random Number Generator.",
> +};
> +PVE::JSONSchema::register_standard_option('pve-qm-rng', $rngdesc);
> +
> +sub parse_rng {
> +    my ($value) = @_;
> +
> +    return if !$value;
> +
> +    my $res = eval { PVE::JSONSchema::parse_property_string($rng_fmt, $value) };
> +    warn $@ if $@;
> +
> +    my $source = $res->{source};

Unused variable, should be part of a later patch.

> +
> +    return $res;
> +}
> +
> +sub check_rng_source {
> +    my ($source) = @_;
> +
> +    # mostly relevant for /dev/hwrng, but doesn't hurt to check others too
> +    die "cannot create VirtIO RNG device: source file '$source' doesn't exist\n"
> +	if ! -e $source;
> +
> +    my $rng_current = '/sys/devices/virtual/misc/hw_random/rng_current';
> +    if ($source eq '/dev/hwrng' && file_read_firstline($rng_current) eq 'none') {
> +	# Needs to abort, otherwise QEMU crashes on first rng access. Note that rng_current cannot
> +	# be changed to 'none' manually, so once the VM is past this point, it's no longer an issue.
> +	die "Cannot start VM with passed-through RNG device: '/dev/hwrng' exists, but"
> +	    ." '$rng_current' is set to 'none'. Ensure that a compatible hardware-RNG is attached"
> +	    ." to the host.\n";
> +    }
> +}
> +
> +sub get_rng_source_path {
> +    my ($rng) = @_;
> +
> +    my $source = $rng->{source};
> +
> +    if (defined($source)) {
> +	return $source;
> +    }
> +
> +    return;
> +}
> +
> +sub print_rng_device {

I'd add _commandline to reduce potential for confusion

> +    my ($id, $rng, $bridges, $arch, $machine) = @_;
> +
> +    return if !$rng;

Isn't failing better? IMHO caller should first check that it has
something to print rather than check the result.

> +
> +    my $source_path = get_rng_source_path($rng);
> +    check_rng_source($source_path);

Since the source is not part of the result, maybe not check it here, but
only in print_rng_object()?

> +
> +    my $max_bytes = $rng->{max_bytes} // $rng_fmt->{max_bytes}->{default};
> +    my $period = $rng->{period} // $rng_fmt->{period}->{default};
> +    my $limiter_str = "";
> +    if ($max_bytes) {
> +	$limiter_str = ",max-bytes=$max_bytes,period=$period";
> +    }
> +
> +    my $rng_addr = print_pci_addr($id, $bridges, $arch, $machine);
> +
> +    return "virtio-rng-pci,rng=$id$limiter_str$rng_addr";
> +}
> +
> +sub print_rng_object {

I'd add _commandline to reduce potential for confusion

> +    my ($id, $rng) = @_;
> +
> +    return if !$rng;

Isn't failing better? IMHO caller should first check that it has
something to print rather than check the result.

> +
> +    my $source_path = get_rng_source_path($rng);
> +    check_rng_source($source_path);
> +
> +    return "rng-random,filename=$source_path,id=$id";
> +}
> +
> +1;



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel