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

[Fabian Ebner]

On 5/5/20 12:02 PM, Thomas Lamprecht wrote:
> 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.
> 

I meant having two create_vm API calls with the same VMID parameter at 
the same time. I didn't mean to imply that two containers would 
successfully be created. Sorry about that.

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