Re: [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.eb...@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;



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