Re: [PATCH v2 2/2] VMD: Prevent vmd crashing when stopping a stopped vm
On 2017-09-05 23:55, Mike Larkin wrote:
> On Mon, Sep 04, 2017 at 12:03:31AM -0700, Carlos Cardenas wrote:
>> * Fix logic handling stopping a VM. Prevents VMD from crashing.
>> * Add additional error code to notify the user that a vm cannot be
>> stopped when not running.
>> * Add additional log_debug statements.
>>
>
> See below. Also this diff has tabs vs spaces problems like the
> previous one.
>
> If we fix the style issues and you can shed some light on the part
> I noted below, I think we can get both these diffs in.
>
> -ml
>
>> diff --git usr.sbin/vmctl/vmctl.c usr.sbin/vmctl/vmctl.c
>> index 64d82ca847d..5fb7fbfd74c 100644
>> --- usr.sbin/vmctl/vmctl.c
>> +++ usr.sbin/vmctl/vmctl.c
>> @@ -439,12 +439,19 @@ terminate_vm_complete(struct imsg *imsg, int *ret)
>> vmr = (struct vmop_result *)imsg->data;
>> res = vmr->vmr_result;
>> if (res) {
>> -errno = res;
>> -if (res == ENOENT)
>> -warnx("vm not found");
>> -else
>> -warn("terminate vm command failed");
>> -*ret = EIO;
>> + switch (res) {
>> + case VMD_VM_STOP_INVALID:
>> + warnx("cannot stop vm that is not running");
>> + *ret = EINVAL;
>> + break;
>> + case ENOENT:
>> + warnx("vm not found");
>> + *ret = EIO;
>> + break;
>> + default:
>> + warn("terminate vm command failed");
>> + *ret = EIO;
>> + }
>> } else {
>> warnx("sent request to terminate vm %d", vmr->vmr_id);
>> *ret = 0;
>> @@ -453,6 +460,7 @@ terminate_vm_complete(struct imsg *imsg, int *ret)
>> warnx("unexpected response received from vmd");
>> *ret = EINVAL;
>> }
>> +errno = *ret;
>>
>> return (1);
>> }
>> diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
>> index 22da6d58a7b..1240339db52 100644
>> --- usr.sbin/vmd/vmd.h
>> +++ usr.sbin/vmd/vmd.h
>> @@ -54,6 +54,7 @@
>> #define VMD_BIOS_MISSING1001
>> #define VMD_DISK_MISSING1002
>> #define VMD_DISK_INVALID1003
>> +#define VMD_VM_STOP_INVALID 1004
>>
>> /* 100.64.0.0/10 from rfc6598 (IPv4 Prefix for Shared Address Space) */
>> #define VMD_DHCP_PREFIX "100.64.0.0/10"
>> diff --git usr.sbin/vmd/vmm.c usr.sbin/vmd/vmm.c
>> index d3233147cff..66083a5f959 100644
>> --- usr.sbin/vmd/vmm.c
>> +++ usr.sbin/vmd/vmm.c
>> @@ -169,10 +169,9 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p,
>> struct imsg *imsg)
>> else
>> res = 0;
>> } else {
>> -/* Terminate VMs that are unknown or shutting down */
>> -vtp.vtp_vm_id = vm_vmid2id(vm->vm_vmid, vm);
>> -res = terminate_vm();
>> -vm_remove(vm);
>> +log_debug("%s: cannot stop vm that is not running",
>> +__func__);
>> +res = VMD_VM_STOP_INVALID;
>
> Is this really what we want? (Was this the part that was crashing vmd?)
> This will break (I think) stopping a vm, then while it is shutting down
> during vmmci shutdown processing, stopping it again to force kill it.
>
> Is the problem that we are doing vm_remove unconditionally, regardless of
> the result of the previous calls? (Eg, what if vm_vmid2id or terminate_vm
> failed?)
>
Using the previous patch in the series, this is what is observed when
attempting to stop a running vm (normal use case):
control_dispatch_imsg vid: 1, name: , uid: 0
proc_compose_imsg: about to compose_event to PROC 0
proc_compose_imsg: about to compose_event to PROC 2
vmm_dispatch_parent: recv'ed TERMINATE_VM for 1
vmm_dispatch_parent: sending shutdown request to vm 1
proc_compose_imsg: about to compose_event to PROC 0
vmd_dispatch_vmm: forwarding TERMINATE VM for 1
proc_compose_imsg: about to compose_event to PROC 1
control_close
vmm_sighdlr: handling signal
vmm_sighdlr: attempting to terminate vm 1
terminate_vm: terminating vmid 1
proc_compose_imsg: about to compose_event to PROC 0
vmm_sighdlr: calling vm_remove
vm_remove: removing vm 1 from running config
vm_remove: calling vm_stop
vm_stop: stopping vm 1
vmd_dispatch_vmm: handling TERMINATE_EVENT for 1
vmd_dispatch_vmm: about to stop vm 1
vm_stop: stopping vm 1
For the non-normal cases:
* Stopping a non-running vm:
** if vm is NULL (means vmd and control has the vm in it's list but vmm
doesn't) -> vmd crash
** if vm is not NULL but is missing a VM managed by vmm (like attempting
to stop a non-running vm that has been configured by vm.conf),
Re: [PATCH v2 2/2] VMD: Prevent vmd crashing when stopping a stopped vm
On Mon, Sep 04, 2017 at 12:03:31AM -0700, Carlos Cardenas wrote:
> * Fix logic handling stopping a VM. Prevents VMD from crashing.
> * Add additional error code to notify the user that a vm cannot be
> stopped when not running.
> * Add additional log_debug statements.
>
See below. Also this diff has tabs vs spaces problems like the
previous one.
If we fix the style issues and you can shed some light on the part
I noted below, I think we can get both these diffs in.
-ml
> diff --git usr.sbin/vmctl/vmctl.c usr.sbin/vmctl/vmctl.c
> index 64d82ca847d..5fb7fbfd74c 100644
> --- usr.sbin/vmctl/vmctl.c
> +++ usr.sbin/vmctl/vmctl.c
> @@ -439,12 +439,19 @@ terminate_vm_complete(struct imsg *imsg, int *ret)
> vmr = (struct vmop_result *)imsg->data;
> res = vmr->vmr_result;
> if (res) {
> - errno = res;
> - if (res == ENOENT)
> - warnx("vm not found");
> - else
> - warn("terminate vm command failed");
> - *ret = EIO;
> + switch (res) {
> + case VMD_VM_STOP_INVALID:
> + warnx("cannot stop vm that is not running");
> + *ret = EINVAL;
> + break;
> + case ENOENT:
> + warnx("vm not found");
> + *ret = EIO;
> + break;
> + default:
> + warn("terminate vm command failed");
> + *ret = EIO;
> + }
> } else {
> warnx("sent request to terminate vm %d", vmr->vmr_id);
> *ret = 0;
> @@ -453,6 +460,7 @@ terminate_vm_complete(struct imsg *imsg, int *ret)
> warnx("unexpected response received from vmd");
> *ret = EINVAL;
> }
> + errno = *ret;
>
> return (1);
> }
> diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> index 22da6d58a7b..1240339db52 100644
> --- usr.sbin/vmd/vmd.h
> +++ usr.sbin/vmd/vmd.h
> @@ -54,6 +54,7 @@
> #define VMD_BIOS_MISSING 1001
> #define VMD_DISK_MISSING 1002
> #define VMD_DISK_INVALID 1003
> +#define VMD_VM_STOP_INVALID 1004
>
> /* 100.64.0.0/10 from rfc6598 (IPv4 Prefix for Shared Address Space) */
> #define VMD_DHCP_PREFIX "100.64.0.0/10"
> diff --git usr.sbin/vmd/vmm.c usr.sbin/vmd/vmm.c
> index d3233147cff..66083a5f959 100644
> --- usr.sbin/vmd/vmm.c
> +++ usr.sbin/vmd/vmm.c
> @@ -169,10 +169,9 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p,
> struct imsg *imsg)
> else
> res = 0;
> } else {
> - /* Terminate VMs that are unknown or shutting down */
> - vtp.vtp_vm_id = vm_vmid2id(vm->vm_vmid, vm);
> - res = terminate_vm();
> - vm_remove(vm);
> +log_debug("%s: cannot stop vm that is not running",
> +__func__);
> +res = VMD_VM_STOP_INVALID;
Is this really what we want? (Was this the part that was crashing vmd?)
This will break (I think) stopping a vm, then while it is shutting down
during vmmci shutdown processing, stopping it again to force kill it.
Is the problem that we are doing vm_remove unconditionally, regardless of
the result of the previous calls? (Eg, what if vm_vmid2id or terminate_vm
failed?)
> }
> cmd = IMSG_VMDOP_TERMINATE_VM_RESPONSE;
> break;
> @@ -346,6 +345,8 @@ vmm_sighdlr(int sig, short event, void *arg)
>
> vmid = vm->vm_params.vmc_params.vcp_id;
> vtp.vtp_vm_id = vmid;
> +log_debug("%s: attempting to terminate vm
> %d",
> +__func__, vm->vm_vmid);
> if (terminate_vm() == 0) {
> memset(, 0, sizeof(vmr));
> vmr.vmr_result = ret;
> @@ -505,7 +506,7 @@ vmm_dispatch_vm(int fd, short event, void *arg)
> * supplied vm_terminate_params structure (vtp->vtp_vm_id)
> *
> * Parameters
> - * vtp: vm_create_params struct containing the ID of the VM to terminate
> + * vtp: vm_terminate_params struct containing the ID of the VM to terminate
> *
> * Return values:
> * 0: success
> @@ -515,6 +516,7 @@ vmm_dispatch_vm(int fd, short event, void *arg)
> int
> terminate_vm(struct vm_terminate_params *vtp)
> {
> +log_debug("%s: terminating vmid %d", __func__, vtp->vtp_vm_id);
> if (ioctl(env->vmd_fd, VMM_IOC_TERM, vtp) < 0)
> return (errno);
>
> --
> 2.14.1
>
[PATCH v2 2/2] VMD: Prevent vmd crashing when stopping a stopped vm
* Fix logic handling stopping a VM. Prevents VMD from crashing.
* Add additional error code to notify the user that a vm cannot be
stopped when not running.
* Add additional log_debug statements.
diff --git usr.sbin/vmctl/vmctl.c usr.sbin/vmctl/vmctl.c
index 64d82ca847d..5fb7fbfd74c 100644
--- usr.sbin/vmctl/vmctl.c
+++ usr.sbin/vmctl/vmctl.c
@@ -439,12 +439,19 @@ terminate_vm_complete(struct imsg *imsg, int *ret)
vmr = (struct vmop_result *)imsg->data;
res = vmr->vmr_result;
if (res) {
- errno = res;
- if (res == ENOENT)
- warnx("vm not found");
- else
- warn("terminate vm command failed");
- *ret = EIO;
+ switch (res) {
+ case VMD_VM_STOP_INVALID:
+ warnx("cannot stop vm that is not running");
+ *ret = EINVAL;
+ break;
+ case ENOENT:
+ warnx("vm not found");
+ *ret = EIO;
+ break;
+ default:
+ warn("terminate vm command failed");
+ *ret = EIO;
+ }
} else {
warnx("sent request to terminate vm %d", vmr->vmr_id);
*ret = 0;
@@ -453,6 +460,7 @@ terminate_vm_complete(struct imsg *imsg, int *ret)
warnx("unexpected response received from vmd");
*ret = EINVAL;
}
+ errno = *ret;
return (1);
}
diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
index 22da6d58a7b..1240339db52 100644
--- usr.sbin/vmd/vmd.h
+++ usr.sbin/vmd/vmd.h
@@ -54,6 +54,7 @@
#define VMD_BIOS_MISSING 1001
#define VMD_DISK_MISSING 1002
#define VMD_DISK_INVALID 1003
+#define VMD_VM_STOP_INVALID1004
/* 100.64.0.0/10 from rfc6598 (IPv4 Prefix for Shared Address Space) */
#define VMD_DHCP_PREFIX"100.64.0.0/10"
diff --git usr.sbin/vmd/vmm.c usr.sbin/vmd/vmm.c
index d3233147cff..66083a5f959 100644
--- usr.sbin/vmd/vmm.c
+++ usr.sbin/vmd/vmm.c
@@ -169,10 +169,9 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, struct
imsg *imsg)
else
res = 0;
} else {
- /* Terminate VMs that are unknown or shutting down */
- vtp.vtp_vm_id = vm_vmid2id(vm->vm_vmid, vm);
- res = terminate_vm();
- vm_remove(vm);
+log_debug("%s: cannot stop vm that is not running",
+__func__);
+res = VMD_VM_STOP_INVALID;
}
cmd = IMSG_VMDOP_TERMINATE_VM_RESPONSE;
break;
@@ -346,6 +345,8 @@ vmm_sighdlr(int sig, short event, void *arg)
vmid = vm->vm_params.vmc_params.vcp_id;
vtp.vtp_vm_id = vmid;
+log_debug("%s: attempting to terminate vm %d",
+__func__, vm->vm_vmid);
if (terminate_vm() == 0) {
memset(, 0, sizeof(vmr));
vmr.vmr_result = ret;
@@ -505,7 +506,7 @@ vmm_dispatch_vm(int fd, short event, void *arg)
* supplied vm_terminate_params structure (vtp->vtp_vm_id)
*
* Parameters
- * vtp: vm_create_params struct containing the ID of the VM to terminate
+ * vtp: vm_terminate_params struct containing the ID of the VM to terminate
*
* Return values:
* 0: success
@@ -515,6 +516,7 @@ vmm_dispatch_vm(int fd, short event, void *arg)
int
terminate_vm(struct vm_terminate_params *vtp)
{
+log_debug("%s: terminating vmid %d", __func__, vtp->vtp_vm_id);
if (ioctl(env->vmd_fd, VMM_IOC_TERM, vtp) < 0)
return (errno);
--
2.14.1
