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

[Thomas Lamprecht]

On 5/5/20 10:27 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 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.
> 
> Even worse, a race was possible for two containers created with the
> same ID at the same time:

This can never happen, the pmxcfs *enforces* that only one <VMID>.conf
can exist at the same time, what you mean is something completely different
than "two CTs created with the same ID at the same time", there will never
be two, only one.

> 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.
After overwriting there's still only one such VMID..

 
> 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 4eb1a14..e079208 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;
>