[pve-devel] [PATCH v2 container] create_vm: avoid premature write_config caused by update_pct_config

[Fabian Ebner]

I'll re-send this as part of the lock series v2.

On 4/30/20 11:33 AM, Fabian Ebner wrote:
> by moving the write_config calls from vmconfig_*_pending to their
> call sites. The single other call site for update_pct_config in
> update_vm is also adapted. The first write_config call in
> vmconfig_hotplug_pending was redundant as the code between it and
> the second write_config call shouldn't be able to die. This allowed
> to drop the $changes variable as well.
> 
> The update_pct_config call lead to a write_config call and so the
> configuration file was created before it was intended to be created.
> 
> When the CFS is updated in between the write_config call and the
> PVE::Cluster::check_vmid_unused call in create_and_lock_config,
> the container file would already exist and so creation would
> fail after writing out a basically empty config.
> (Meaning this is necessary for the proposed "update CFS after
> obtaining a configuration file lock" changes)
> 
> Even worse, a race was possible for two containers created with the
> same ID at the same time:
> Assuming the initial PVE::Cluster::check_vmid_unused check in the
> parameter verification passes for both create_vm calls, the later one
> would potentially overwrite the earlier configuration file with its
> update_pct_config call.
> 
> Additionally, the file read for $old_config was always the one written
> by update_pct_config. Meaning that for a create_vm call with force=1,
> already existing old volumes were not removed.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
> 
> Changes from v1:
>      * instead of re-ordering, move the write_config
>        calls to the (grand-)parent call sites.
> 
> There are now two places where write_config is immediately followed
> by load_config. I didn't want to change the semantics for the
> non-create_vm call sites, so I left them there. Are there any
> possible side effects? Otherwise, the load_config calls could be
> dropped as a follow-up. Note that the third place in lxc-pve-poststop-hook
> has no load_config call.
> 
>   src/PVE/API2/LXC/Config.pm |  1 +
>   src/PVE/LXC.pm             |  1 +
>   src/PVE/LXC/Config.pm      | 10 ----------
>   src/lxc-pve-poststop-hook  |  5 ++++-
>   4 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
> index 42e16d1..73fec36 100644
> --- a/src/PVE/API2/LXC/Config.pm
> +++ b/src/PVE/API2/LXC/Config.pm
> @@ -204,6 +204,7 @@ __PACKAGE__->register_method({
>   	    my $running = PVE::LXC::check_running($vmid);
>   
>   	    my $errors = PVE::LXC::Config->update_pct_config($vmid, $conf, $running, $param, \@delete, \@revert);
> +	    PVE::LXC::Config->write_config($vmid, $conf);
>   	    $conf = PVE::LXC::Config->load_config($vmid);
>   
>   	    PVE::LXC::update_lxc_config($vmid, $conf);
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index efbd1d9..2b41ae6 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -2173,6 +2173,7 @@ sub vm_start {
>       if (scalar(keys %{$conf->{pending}})) {
>   	my $storecfg = PVE::Storage::config();
>   	PVE::LXC::Config->vmconfig_apply_pending($vmid, $conf, $storecfg);
> +	PVE::LXC::Config->write_config($vmid, $conf);
>   	$conf = PVE::LXC::Config->load_config($vmid); # update/reload
>       }
>   
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 1443f87..dcc8755 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1161,19 +1161,13 @@ sub vmconfig_hotplug_pending {
>   	$errors->{$opt} = "unable to hotplug $opt: $msg";
>       };
>   
> -    my $changes;
>       foreach my $opt (sort keys %{$conf->{pending}}) { # add/change
>   	next if $selection && !$selection->{$opt};
>   	if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
>   	    $conf->{$opt} = delete $conf->{pending}->{$opt};
> -	    $changes = 1;
>   	}
>       }
>   
> -    if ($changes) {
> -	$class->write_config($vmid, $conf);
> -    }
> -
>       my $cgroup = PVE::LXC::CGroup->new($vmid);
>   
>       # There's no separate swap size to configure, there's memory and "total"
> @@ -1258,8 +1252,6 @@ sub vmconfig_hotplug_pending {
>   	    delete $conf->{pending}->{$opt};
>   	}
>       }
> -
> -    $class->write_config($vmid, $conf);
>   }
>   
>   sub vmconfig_apply_pending {
> @@ -1316,8 +1308,6 @@ sub vmconfig_apply_pending {
>   	    $conf->{$opt} = delete $conf->{pending}->{$opt};
>   	}
>       }
> -
> -    $class->write_config($vmid, $conf);
>   }
>   
>   my $rescan_volume = sub {
> diff --git a/src/lxc-pve-poststop-hook b/src/lxc-pve-poststop-hook
> index 1dba48c..2683ae2 100755
> --- a/src/lxc-pve-poststop-hook
> +++ b/src/lxc-pve-poststop-hook
> @@ -51,7 +51,10 @@ PVE::LXC::Tools::lxc_hook('post-stop', 'lxc', sub {
>   
>       my $config_updated = 0;
>       if ($conf->{pending}) {
> -	eval { PVE::LXC::Config->vmconfig_apply_pending($vmid, $conf, $storage_cfg) };
> +	eval {
> +	    PVE::LXC::Config->vmconfig_apply_pending($vmid, $conf, $storage_cfg);
> +	    PVE::LXC::Config->write_config($vmid, $conf);
> +	};
>   	warn "$@" if $@;
>   	PVE::LXC::update_lxc_config($vmid, $conf);
>   	$config_updated = 1;
>