Dave Voutila writes:
> Dave Voutila writes: > >> tech@, >> >> While migrating an existing -current vm to use dhcpleased(8), one of the >> issues discovered was the dhcp/bootp handling built into vmd(8) for >> local interfaces was improperly missing packets sent to the ethernet >> address of the host-side tap(4) device. (vmd(8) was only looking for >> broadcast packets.) >> >> The following diff: >> - includes the host-side tap(4) lladdr in the packet filtering logic for >> intercepting vio(4) dhcp/bootp communication >> - removes a conditional check (dhcpsz == 0) pointed out by deraadt@ that >> could contribute to missed packets while iterating through the vionet >> tx ring > > The conditional check (dhcpsz == 0) was committed. The diff below is > updated to apply to the latest copy of the src tree. > >> >> Because of vmd(8)'s priv-sep design, the approach taken is to pass a >> duplicate of the opened tap(4) fd to the "priv" process that is >> unpledged and responsible for setting up networking. A response imsg >> travels between processes, being forwarded until it gets to the intended >> vm process. >> >> For those looking to test the diff, some steps to follow are: >> >> 0. Don't apply the patch yet. >> 1. Use an OpenBSD guest running a recent snapshot that has the latest >> dhcpleased(8)[1] and dhcpleasectl(8)[2] changes committed by florian@. >> 2. Configure the guest for using dhcpleased(8), ideally via >> /etc/hostname.vio0 containing: >> >> inet autoconf >> up >> >> 3. Ensure you get an IP assigned from vmd(8) after boot. >> 4. Check the lease with `dhcpleasectl show interface vio0`. It should >> report [Bound] and have a _very_ long lease time. >> 5. Force a renewal with `dhcpleasectl send request vio0`. >> 6. Check again like in step 4. It will be "stuck" in a [Renewing] state. >> 7. Shut down the guest, vmd(8), and apply the patch. >> 8. Repeat steps 4-6, but the guest should no longer be "stuck" in >> [Renewing] and should report [Bound] after forced renewal. >> >> You can also turn up debug logging for vmd(8) and should see >> corresponding messages of: >> >> vionet: dhcp request, local response size 342 >> >> If you do not, the packet was not intercepted by vmd(8). >> >> I have tested this diff with a variety of conditions, including multiple >> emulated nics per guest. Specifically 1 nic using dhcp/bootp resolved by >> vmd(8) and the other nic connected to a virtual switch using veb(4) and >> an instance of dhcpd(8) running on the host. Also ran a snapshot upgrade >> with no issues. > > I haven't received any issue reports as of yet. > No issue reports. mlarkin@ asked for a change the function signature of vionet_set_hostmac (change array to pointer). OK? >> >> [1] https://marc.info/?l=openbsd-cvs&m=161643049815177&w=2 >> [2] https://marc.info/?l=openbsd-cvs&m=161652157122736&w=2 >> Index: usr.sbin/vmd/config.c =================================================================== RCS file: /cvs/src/usr.sbin/vmd/config.c,v retrieving revision 1.60 diff -u -p -r1.60 config.c --- usr.sbin/vmd/config.c 19 Mar 2021 09:29:33 -0000 1.60 +++ usr.sbin/vmd/config.c 29 Mar 2021 21:19:09 -0000 @@ -227,6 +227,7 @@ config_setvm(struct privsep *ps, struct char base[PATH_MAX]; unsigned int unit; struct timeval tv, rate, since_last; + struct vmop_addr_req var; errno = 0; @@ -499,6 +500,12 @@ config_setvm(struct privsep *ps, struct proc_compose_imsg(ps, PROC_VMM, -1, IMSG_VMDOP_START_VM_IF, vm->vm_vmid, tapfds[i], &i, sizeof(i)); + + memset(&var, 0, sizeof(var)); + var.var_vmid = vm->vm_vmid; + var.var_nic_idx = i; + proc_compose_imsg(ps, PROC_PRIV, -1, IMSG_VMDOP_PRIV_GET_ADDR, + vm->vm_vmid, dup(tapfds[i]), &var, sizeof(var)); } if (!(vm->vm_state & VM_STATE_RECEIVED)) Index: usr.sbin/vmd/dhcp.c =================================================================== RCS file: /cvs/src/usr.sbin/vmd/dhcp.c,v retrieving revision 1.8 diff -u -p -r1.8 dhcp.c --- usr.sbin/vmd/dhcp.c 27 Dec 2018 19:51:30 -0000 1.8 +++ usr.sbin/vmd/dhcp.c 29 Mar 2021 21:19:09 -0000 @@ -57,8 +57,11 @@ dhcp_request(struct vionet_dev *dev, cha if ((offset = decode_hw_header(buf, buflen, 0, &pc, HTYPE_ETHER)) < 0) return (-1); - if (memcmp(pc.pc_smac, dev->mac, ETHER_ADDR_LEN) != 0 || - memcmp(pc.pc_dmac, broadcast, ETHER_ADDR_LEN) != 0) + if (memcmp(pc.pc_dmac, broadcast, ETHER_ADDR_LEN) != 0 && + memcmp(pc.pc_dmac, dev->hostmac, ETHER_ADDR_LEN) != 0) + return (-1); + + if (memcmp(pc.pc_smac, dev->mac, ETHER_ADDR_LEN) != 0) return (-1); if ((offset = decode_udp_ip_header(buf, buflen, offset, &pc)) < 0) Index: usr.sbin/vmd/priv.c =================================================================== RCS file: /cvs/src/usr.sbin/vmd/priv.c,v retrieving revision 1.16 diff -u -p -r1.16 priv.c --- usr.sbin/vmd/priv.c 28 Feb 2021 22:56:09 -0000 1.16 +++ usr.sbin/vmd/priv.c 29 Mar 2021 21:19:09 -0000 @@ -92,6 +92,8 @@ priv_dispatch_parent(int fd, struct priv struct ifaliasreq ifra; struct in6_aliasreq in6_ifra; struct if_afreq ifar; + struct vmop_addr_req vareq; + struct vmop_addr_result varesult; char type[IF_NAMESIZE]; switch (imsg->hdr.type) { @@ -115,6 +117,7 @@ priv_dispatch_parent(int fd, struct priv break; case IMSG_VMDOP_CONFIG: case IMSG_CTL_RESET: + case IMSG_VMDOP_PRIV_GET_ADDR: break; default: return (-1); @@ -244,6 +247,22 @@ priv_dispatch_parent(int fd, struct priv if (ioctl(env->vmd_fd6, SIOCAIFADDR_IN6, &in6_ifra) == -1) log_warn("SIOCAIFADDR_IN6"); + break; + case IMSG_VMDOP_PRIV_GET_ADDR: + IMSG_SIZE_CHECK(imsg, &vareq); + memcpy(&vareq, imsg->data, sizeof(vareq)); + + varesult.var_vmid = vareq.var_vmid; + varesult.var_nic_idx = vareq.var_nic_idx; + + /* resolve lladdr for the tap(4) and send back to parent */ + if (ioctl(imsg->fd, SIOCGIFADDR, &varesult.var_addr) != 0) + log_warn("SIOCGIFADDR"); + else + proc_compose_imsg(ps, PROC_PARENT, -1, + IMSG_VMDOP_PRIV_GET_ADDR_RESPONSE, imsg->hdr.peerid, + -1, &varesult, sizeof(varesult)); + close(imsg->fd); break; case IMSG_VMDOP_CONFIG: config_getconfig(env, imsg); Index: usr.sbin/vmd/virtio.c =================================================================== RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v retrieving revision 1.83 diff -u -p -r1.83 virtio.c --- usr.sbin/vmd/virtio.c 26 Mar 2021 17:40:03 -0000 1.83 +++ usr.sbin/vmd/virtio.c 29 Mar 2021 21:19:09 -0000 @@ -2034,6 +2034,31 @@ virtio_init(struct vmd_vm *vm, int child evtimer_set(&vmmci.timeout, vmmci_timeout, NULL); } +/* + * vionet_set_hostmac + * + * Sets the hardware address for the host-side tap(4) on a vionet_dev. + * + * This should only be called from the event-loop thread + * + * vm: pointer to the current vmd_vm instance + * idx: index into the array of vionet_dev's for the target vionet_dev + * addr: ethernet address to set + */ +void +vionet_set_hostmac(struct vmd_vm *vm, unsigned int idx, uint8_t *addr) +{ + struct vmop_create_params *vmc = &vm->vm_params; + struct vm_create_params *vcp = &vmc->vmc_params; + struct vionet_dev *dev; + + if (idx > vcp->vcp_nnics) + fatalx("vionet_set_hostmac"); + + dev = &vionet[idx]; + memcpy(dev->hostmac, addr, sizeof(dev->hostmac)); +} + void virtio_shutdown(struct vmd_vm *vm) { Index: usr.sbin/vmd/virtio.h =================================================================== RCS file: /cvs/src/usr.sbin/vmd/virtio.h,v retrieving revision 1.36 diff -u -p -r1.36 virtio.h --- usr.sbin/vmd/virtio.h 7 Jan 2021 17:11:38 -0000 1.36 +++ usr.sbin/vmd/virtio.h 29 Mar 2021 21:19:09 -0000 @@ -208,6 +208,7 @@ struct vionet_dev { uint32_t vm_vmid; int irq; uint8_t mac[6]; + uint8_t hostmac[6]; int idx; int lockedmac; @@ -298,6 +299,7 @@ void vionet_notify_rx(struct vionet_dev int vionet_notify_tx(struct vionet_dev *); void vionet_process_rx(uint32_t); int vionet_enq_rx(struct vionet_dev *, char *, ssize_t, int *); +void vionet_set_hostmac(struct vmd_vm *, unsigned int, uint8_t *); int vmmci_io(int, uint16_t, uint32_t *, uint8_t *, void *, uint8_t); int vmmci_dump(int); Index: usr.sbin/vmd/vm.c =================================================================== RCS file: /cvs/src/usr.sbin/vmd/vm.c,v retrieving revision 1.60 diff -u -p -r1.60 vm.c --- usr.sbin/vmd/vm.c 19 Mar 2021 09:29:33 -0000 1.60 +++ usr.sbin/vmd/vm.c 29 Mar 2021 21:19:09 -0000 @@ -394,6 +394,7 @@ vm_dispatch_vmm(int fd, short event, voi { struct vmd_vm *vm = arg; struct vmop_result vmr; + struct vmop_addr_result var; struct imsgev *iev = &vm->vm_iev; struct imsgbuf *ibuf = &iev->ibuf; struct imsg imsg; @@ -470,6 +471,16 @@ vm_dispatch_vmm(int fd, short event, voi imsg_flush(¤t_vm->vm_iev.ibuf); _exit(0); } + break; + case IMSG_VMDOP_PRIV_GET_ADDR_RESPONSE: + IMSG_SIZE_CHECK(&imsg, &var); + memcpy(&var, imsg.data, sizeof(var)); + + log_debug("%s: received tap addr %s for nic %d", + vm->vm_params.vmc_params.vcp_name, + ether_ntoa((void *)var.var_addr), var.var_nic_idx); + + vionet_set_hostmac(vm, var.var_nic_idx, var.var_addr); break; default: fatalx("%s: got invalid imsg %d from %s", Index: usr.sbin/vmd/vmd.c =================================================================== RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v retrieving revision 1.120 diff -u -p -r1.120 vmd.c --- usr.sbin/vmd/vmd.c 27 Jan 2021 07:21:54 -0000 1.120 +++ usr.sbin/vmd/vmd.c 29 Mar 2021 21:19:09 -0000 @@ -58,6 +58,7 @@ void vmd_shutdown(void); int vmd_control_run(void); int vmd_dispatch_control(int, struct privsep_proc *, struct imsg *); int vmd_dispatch_vmm(int, struct privsep_proc *, struct imsg *); +int vmd_dispatch_priv(int, struct privsep_proc *, struct imsg *); int vmd_check_vmh(struct vm_dump_header *); int vm_instance(struct privsep *, struct vmd_vm **, @@ -70,7 +71,7 @@ struct vmd *env; static struct privsep_proc procs[] = { /* Keep "priv" on top as procs[0] */ - { "priv", PROC_PRIV, NULL, priv }, + { "priv", PROC_PRIV, vmd_dispatch_priv, priv }, { "control", PROC_CONTROL, vmd_dispatch_control, control }, { "vmm", PROC_VMM, vmd_dispatch_vmm, vmm, vmm_shutdown }, }; @@ -542,6 +543,24 @@ vmd_dispatch_vmm(int fd, struct privsep_ } IMSG_SIZE_CHECK(imsg, &res); proc_forward_imsg(ps, imsg, PROC_CONTROL, -1); + break; + default: + return (-1); + } + + return (0); +} + +int +vmd_dispatch_priv(int fd, struct privsep_proc *p, struct imsg *imsg) +{ + struct vmop_addr_result var; + + switch (imsg->hdr.type) { + case IMSG_VMDOP_PRIV_GET_ADDR_RESPONSE: + IMSG_SIZE_CHECK(imsg, &var); + memcpy(&var, imsg->data, sizeof(var)); + proc_forward_imsg(p->p_ps, imsg, PROC_VMM, -1); break; default: return (-1); Index: usr.sbin/vmd/vmd.h =================================================================== RCS file: /cvs/src/usr.sbin/vmd/vmd.h,v retrieving revision 1.102 diff -u -p -r1.102 vmd.h --- usr.sbin/vmd/vmd.h 19 Mar 2021 09:29:33 -0000 1.102 +++ usr.sbin/vmd/vmd.h 29 Mar 2021 21:19:09 -0000 @@ -120,6 +120,8 @@ enum imsg_type { IMSG_VMDOP_PRIV_IFADDR, IMSG_VMDOP_PRIV_IFADDR6, IMSG_VMDOP_PRIV_IFRDOMAIN, + IMSG_VMDOP_PRIV_GET_ADDR, + IMSG_VMDOP_PRIV_GET_ADDR_RESPONSE, IMSG_VMDOP_VM_SHUTDOWN, IMSG_VMDOP_VM_REBOOT, IMSG_VMDOP_CONFIG, @@ -156,6 +158,17 @@ struct vmop_ifreq { char vfr_value[VM_NAME_MAX]; struct sockaddr_storage vfr_addr; struct sockaddr_storage vfr_mask; +}; + +struct vmop_addr_req { + uint32_t var_vmid; + unsigned int var_nic_idx; +}; + +struct vmop_addr_result { + uint32_t var_vmid; + unsigned int var_nic_idx; + uint8_t var_addr[ETHER_ADDR_LEN]; }; struct vmop_owner { Index: usr.sbin/vmd/vmm.c =================================================================== RCS file: /cvs/src/usr.sbin/vmd/vmm.c,v retrieving revision 1.97 diff -u -p -r1.97 vmm.c --- usr.sbin/vmd/vmm.c 2 Mar 2021 02:56:22 -0000 1.97 +++ usr.sbin/vmd/vmm.c 29 Mar 2021 21:19:09 -0000 @@ -109,6 +109,7 @@ vmm_dispatch_parent(int fd, struct privs struct vmop_id vid; struct vmop_result vmr; struct vmop_create_params vmc; + struct vmop_addr_result var; uint32_t id = 0, peerid = imsg->hdr.peerid; pid_t pid = 0; unsigned int mode, flags; @@ -331,6 +332,18 @@ vmm_dispatch_parent(int fd, struct privs if ((id = vm_id2vmid(id, NULL)) == 0) res = ENOENT; cmd = IMSG_VMDOP_START_VM_RESPONSE; + break; + case IMSG_VMDOP_PRIV_GET_ADDR_RESPONSE: + IMSG_SIZE_CHECK(imsg, &var); + memcpy(&var, imsg->data, sizeof(var)); + if ((vm = vm_getbyvmid(var.var_vmid)) == NULL) { + res = ENOENT; + break; + } + /* Forward hardware address details to the guest vm */ + imsg_compose_event(&vm->vm_iev, + imsg->hdr.type, imsg->hdr.peerid, imsg->hdr.pid, + imsg->fd, &var, sizeof(var)); break; default: return (-1);
