This might not be a problem in practice.
vmd(8) hands us a lease with "infinity" lease time. This is expresed
as UINT32_MAX, i.e. 2^32-1. dhcpleased(8) does not handle infinity
explicitly, it's just a very long lease time (136 years).
When we configure the lease we enter the BOUND state. After 0.5 *
lease time (T1) we transition to RENEWING:
RFC 2131, 4.3.2 DHCPREQUEST message:
o DHCPREQUEST generated during RENEWING state:
'server identifier' MUST NOT be filled in, 'requested IP address'
option MUST NOT be filled in, 'ciaddr' MUST be filled in with
client's IP address. In this situation, the client is completely
configured, and is trying to extend its lease. *This message will
be unicast*, so no relay agents will be involved in its
transmission. Because 'giaddr' is therefore not filled in, the
DHCP server will trust the value in 'ciaddr', and use it when
replying to the client.
This is the only state where we send unicast messages.
After 0.875 * lease_time (T2) we transition to REBINDING which is
again broadcast.
Now these are very long timeouts. In particular we transition to
RENEWING after 68 years...
One can trigger a transition from BOUND to renewing via
dhcpleasectl(8)'s send request command. It will basically do the next
state transition, in this case to RENEWING and will be stuck there
because vmd will ignore out request.
Our lease is however still valid, so everything "just works".
Maybe the problem is with the send request command. I don't know yet
what to do with it. Maybe it should transition to INIT state. This is
what dhclient(8) is doing when one re-starts it on an interface.
So vmd(8) is not behaving correctly as dhcp server. I don't know if
this needs fixing though if it's too complicated.
On Wed, Mar 24, 2021 at 05:47:36PM -0400, Dave Voutila wrote:
> 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
>
> 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.
>
> So in summary, if this diff works, you shouldn't notice a difference ;-)
>
> Thanks,
> -Dave
>
> [1] https://marc.info/?l=openbsd-cvs&m=161643049815177&w=2
> [2] https://marc.info/?l=openbsd-cvs&m=161652157122736&w=2
>
>
> Index: config.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/config.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 config.c
> --- config.c 19 Mar 2021 09:29:33 -0000 1.60
> +++ config.c 24 Mar 2021 15:42:13 -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: dhcp.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/dhcp.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 dhcp.c
> --- dhcp.c 27 Dec 2018 19:51:30 -0000 1.8
> +++ dhcp.c 24 Mar 2021 15:42:13 -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: priv.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/priv.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 priv.c
> --- priv.c 28 Feb 2021 22:56:09 -0000 1.16
> +++ priv.c 24 Mar 2021 15:42:14 -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: virtio.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v
> retrieving revision 1.82
> diff -u -p -r1.82 virtio.c
> --- virtio.c 11 Dec 2019 06:45:16 -0000 1.82
> +++ virtio.c 24 Mar 2021 15:42:14 -0000
> @@ -1510,7 +1510,7 @@ vionet_notify_tx(struct vionet_dev *dev)
> log_debug("vionet: wrong source address %s for vm %d",
> ether_ntoa((struct ether_addr *)
> eh->ether_shost), dev->vm_id);
> - else if (dev->local && dhcpsz == 0 &&
> + else if (dev->local &&
> (dhcpsz = dhcp_request(dev, pkt, pktsz, &dhcppkt)) != -1) {
> log_debug("vionet: dhcp request,"
> " local response size %zd", dhcpsz);
> @@ -2033,6 +2033,32 @@ virtio_init(struct vmd_vm *vm, int child
> vmmci.pci_id = id;
>
> 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[ETHER_ADDR_LEN])
> +{
> + 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
> Index: virtio.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/virtio.h,v
> retrieving revision 1.36
> diff -u -p -r1.36 virtio.h
> --- virtio.h 7 Jan 2021 17:11:38 -0000 1.36
> +++ virtio.h 24 Mar 2021 15:42:14 -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[ETHER_ADDR_LEN]);
>
> int vmmci_io(int, uint16_t, uint32_t *, uint8_t *, void *, uint8_t);
> int vmmci_dump(int);
> Index: vm.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vm.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 vm.c
> --- vm.c 19 Mar 2021 09:29:33 -0000 1.60
> +++ vm.c 24 Mar 2021 15:42:14 -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: vmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
> retrieving revision 1.120
> diff -u -p -r1.120 vmd.c
> --- vmd.c 27 Jan 2021 07:21:54 -0000 1.120
> +++ vmd.c 24 Mar 2021 15:42:14 -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: vmd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.h,v
> retrieving revision 1.102
> diff -u -p -r1.102 vmd.h
> --- vmd.h 19 Mar 2021 09:29:33 -0000 1.102
> +++ vmd.h 24 Mar 2021 15:42:14 -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: vmm.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmm.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 vmm.c
> --- vmm.c 2 Mar 2021 02:56:22 -0000 1.97
> +++ vmm.c 24 Mar 2021 15:42:14 -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);
>
--
I'm not entirely sure you are real.
