Following the discusion on misc@ and a diff from tedu@ [1], here's a bit more work cleaning up the issue in vmd(8) to prevent vmd dying if a user tries to create a vm with memory above the rlimit.
I changed tedu's diff a bit to make it less verbose and print a value that's human readable using fmt_scaled(3). It also only prints the message about data limits if the error was ENOMEM. I've also incorporated some feedback off list and follow the error condition further through vmd. Now vmctl receives a descriptive error as well. OK or feedback? An example trying to start a vm named "test": Before ====== vmctl: $ doas vmctl start -Lc -d vm/openbsd.qcow2 -m4G test vmctl: pipe closed vmd: $ doas vmd -d startup test: could not allocate guest memory - exiting: Cannot allocate memory vmm: read vcp id priv exiting, pid 36084 control exiting, pid 58866 (vmd exits, a dove cries) After ===== vmctl: $ doas vmctl start -Lc -d vm/openbsd.qcow2 -m4G test vmctl: start vm command failed: Cannot allocate memory vmd: $ doas obj/vmd -d startup test: could not allocate guest memory (data limit is 4.0G) test: failed to start vm (vmd still alive) -dv [1] https://marc.info/?l=openbsd-misc&m=164581487723923&w=2 diff e7fa9d3941282eed56a0d5808179cb0e321faae6 /usr/src blob - 4c6c99f1133cec7cb1e38dfd22e595e4d2023842 file + usr.sbin/vmd/vm.c --- usr.sbin/vmd/vm.c +++ usr.sbin/vmd/vm.c @@ -26,6 +26,7 @@ #include <sys/socket.h> #include <sys/time.h> #include <sys/mman.h> +#include <sys/resource.h> #include <dev/ic/i8253reg.h> #include <dev/isa/isareg.h> @@ -292,17 +293,24 @@ start_vm(struct vmd_vm *vm, int fd) ret = alloc_guest_mem(vcp); if (ret) { + struct rlimit lim; + char buf[FMT_SCALED_STRSIZE]; + if (ret == ENOMEM && getrlimit(RLIMIT_DATA, &lim) == 0) { + if (fmt_scaled(lim.rlim_cur, buf) == 0) + fatalx("could not allocate guest memory (data " + "limit is %s)", buf); + } errno = ret; - fatal("could not allocate guest memory - exiting"); + fatal("could not allocate guest memory"); } ret = vmm_create_vm(vcp); current_vm = vm; /* send back the kernel-generated vm id (0 on error) */ - if (write(fd, &vcp->vcp_id, sizeof(vcp->vcp_id)) != + if (atomicio(vwrite, fd, &vcp->vcp_id, sizeof(vcp->vcp_id)) != sizeof(vcp->vcp_id)) - fatal("write vcp id"); + fatal("failed to send created vm id to vmm process"); if (ret) { errno = ret; @@ -319,10 +327,9 @@ start_vm(struct vmd_vm *vm, int fd) fatal("pledge"); if (vm->vm_state & VM_STATE_RECEIVED) { - ret = read(vm->vm_receive_fd, &vrp, sizeof(vrp)); - if (ret != sizeof(vrp)) { + ret = atomicio(read, vm->vm_receive_fd, &vrp, sizeof(vrp)); + if (ret != sizeof(vrp)) fatal("received incomplete vrp - exiting"); - } vrs = vrp.vrwp_regs; } else { /* blob - a60291c17f1f5bb94572f531bdc7e6b2f6b707d5 file + usr.sbin/vmd/vmd.c --- usr.sbin/vmd/vmd.c +++ usr.sbin/vmd/vmd.c @@ -399,9 +399,9 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc } if (vmr.vmr_result) { + log_warnx("%s: failed to start vm", vcp->vcp_name); + vm_remove(vm, __func__); errno = vmr.vmr_result; - log_warn("%s: failed to start vm", vcp->vcp_name); - vm_remove(vm, __func__); break; } blob - eb75b4c587884ec43704420ef4172386a5b39bd9 file + usr.sbin/vmd/vmm.c --- usr.sbin/vmd/vmm.c +++ usr.sbin/vmd/vmm.c @@ -51,6 +51,7 @@ #include "vmd.h" #include "vmm.h" +#include "atomicio.h" void vmm_sighdlr(int, short, void *); int vmm_start_vm(struct imsg *, uint32_t *, pid_t *); @@ -145,7 +146,7 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, st case IMSG_VMDOP_START_VM_END: res = vmm_start_vm(imsg, &id, &pid); /* Check if the ID can be mapped correctly */ - if ((id = vm_id2vmid(id, NULL)) == 0) + if (res == 0 && (id = vm_id2vmid(id, NULL)) == 0) res = ENOENT; cmd = IMSG_VMDOP_START_VM_RESPONSE; break; @@ -615,7 +616,8 @@ vmm_start_vm(struct imsg *imsg, uint32_t *id, pid_t *p struct vmd_vm *vm; int ret = EINVAL; int fds[2]; - size_t i, j; + pid_t vm_pid; + size_t i, j, sz; if ((vm = vm_getbyvmid(imsg->hdr.peerid)) == NULL) { log_warnx("%s: can't find vm", __func__); @@ -635,18 +637,18 @@ vmm_start_vm(struct imsg *imsg, uint32_t *id, pid_t *p fatal("socketpair"); /* Start child vmd for this VM (fork, chroot, drop privs) */ - ret = fork(); + vm_pid = fork(); /* Start child failed? - cleanup and leave */ - if (ret == -1) { + if (vm_pid == -1) { log_warnx("%s: start child failed", __func__); ret = EIO; goto err; } - if (ret > 0) { + if (vm_pid > 0) { /* Parent */ - vm->vm_pid = ret; + vm->vm_pid = vm_pid; close(fds[1]); for (i = 0 ; i < vcp->vcp_ndisks; i++) { @@ -674,10 +676,16 @@ vmm_start_vm(struct imsg *imsg, uint32_t *id, pid_t *p } /* Read back the kernel-generated vm id from the child */ - if (read(fds[0], &vcp->vcp_id, sizeof(vcp->vcp_id)) != - sizeof(vcp->vcp_id)) - fatal("read vcp id"); + sz = atomicio(read, fds[0], &vcp->vcp_id, sizeof(vcp->vcp_id)); + if (sz != sizeof(vcp->vcp_id)) { + log_debug("%s: failed to receive vm id from vm %s", + __func__, vcp->vcp_name); + /* If the vm cannot allocate memory, we end up here. */ + ret = ENOMEM; + goto err; + } + /* If the vm id is 0, the vmm(4) failed to create the vm. */ if (vcp->vcp_id == 0) goto err;
