"Timothy Beaver" <realbeaver9...@danwin1210.de> writes:
> Hi folks,
>
> Dave Voutila <d...@sisu.io> writes:
>> Lucas <lu...@sexy.is> writes:
>>
>> >>Fix:
>> > Dunno. I have the theory that this "broke" (idk if it worked
>> > before or not) after "vmd(8): introduce multi-process model for
>> > virtio devices." Haven't found out yet a way to notify vionet
>> > process about the config: calls into vionet_main (triggered from
>> > vm_main) happen before configuration parsing but after init, so
>> > it gets the default value of 100.64.0.0/10. Haven't checked it
>> > neither, but I also predict that "vmctl load" changes won't be
>> > reflected.
>>
>> Thanks, I'll look to reproduce. There's definitely a message type for
>> mac address setting, but I honestly don't know how we were handling this
>> prior to the split out into a device process. This might need to be a
>> new imsg for any runtime changes via "vmctl load".
>
> I've been able to reproduce this issue as well on my own server, and
> was able to get things working properly with the patch inlined below.
>
> Some details: as alluded to above, the vm processes (and thus the
> device processes they spawn, including vionet) don't have an up to
> date copy of the config (including the 'local prefix[6]'). I was able
> to solve this without adding a new message type - by stashing the
> currently configured interface prefix into the vmop_create_params
> struct when config_setvm() is called. From here, the prefix flows
> down from vmm -> vm -> vionet, where tweaked address calculation
> functions (vm_priv_addr and vm_priv_addr8) extract this information
> rather than falling back on the (incorrect) global config.
Thanks for the effort, but I have some reservations about your
approach as it's more akin to a bandaid than an improvement given where
vmd is going with my recent changes for multi-process emulation.
In short:
1) vmop_create_params contains settings specific to a vm and the local
prefix settings are currently global. I don't want to mix semantics
if we don't have to. Plus, as you call out below, a configuration
reload via `vmctl reload` will not be handled by your approach.
2) you're adding a new usage of the current_vm extern, which i'm trying
to phase out as much as possible (there's too much implicit global
state throughout vmd). Adding another extern to dhcp.c, a source of
many headaches in the past, is not something I want to do.
Some notes below in the diff, but in general I'd much rather prefer a
message-based approach that allows us to remove even more dependence on
the crufty global state (env, current_vm).
I recommend looking at how the host mac is set asynchronously with all
virtio network device processes.
>
> I briefly tested this patch and confirmed things now work as they
> should - more thorough testing is forthcoming. A downside of
> this approach vs. the messaging approach is that a vmctl load will
> only change the prefix setting for VMs created in the future. I'm
> not sure whether this is a problem / what the utility of changing
> the prefix for existing VMs on the fly would be.
>
> I'm new to contributing to vmd and OpenBSD in general, so of course
> I'm open to any critiques - thanks!
>
> Index: config.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/config.c,v
> retrieving revision 1.71
> diff -u -p -u -p -r1.71 config.c
> --- config.c 28 Apr 2023 19:46:42 -0000 1.71
> +++ config.c 14 May 2023 17:04:42 -0000
> @@ -210,9 +210,10 @@ config_getreset(struct vmd *env, struct
> * Returns 0 on success, error code on failure.
> */
> int
> -config_setvm(struct privsep *ps, struct vmd_vm *vm, uint32_t peerid,
> uid_t uid)
> +config_setvm(struct vmd *env, struct vmd_vm *vm, uint32_t peerid, uid_t uid)
> {
> int diskfds[VM_MAX_DISKS_PER_VM][VM_MAX_BASE_PER_DISK];
> + struct privsep *ps = &env->vmd_ps;
> struct vmd_if *vif;
> struct vmop_create_params *vmc = &vm->vm_params;
> struct vm_create_params *vcp = &vmc->vmc_params;
> @@ -279,6 +280,13 @@ config_setvm(struct privsep *ps, struct
>
> vm->vm_peerid = peerid;
> vm->vm_uid = uid;
> +
> + /*
> + * Plumb configured localprefix through to vmc so it
> + * makes it to child processes
> + */
> + vmc->vmc_localprefix = env->vmd_cfg.cfg_localprefix;
> + vmc->vmc_localprefix6 = env->vmd_cfg.cfg_localprefix6;
We don't tend to do implicit assignment like this for structures.
>
> /*
> * From here onward, all failures need cleanup and use goto fail
> Index: dhcp.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/dhcp.c,v
> retrieving revision 1.12
> diff -u -p -u -p -r1.12 dhcp.c
> --- dhcp.c 27 Apr 2023 22:47:27 -0000 1.12
> +++ dhcp.c 14 May 2023 17:04:42 -0000
> @@ -41,6 +41,7 @@
>
> static const uint8_t broadcast[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
> extern struct vmd *env;
> +extern struct vmd_vm *current_vm;
>
This is where I don't want to add more global state. I'd rather any
runtime configuration details required for device emulation be part of
the device struct, so in this case members of virtio_dev. This would
also let us drop usage of the global `env` polluting dhcp.c.
> ssize_t
> dhcp_request(struct virtio_dev *dev, char *buf, size_t buflen, char **obuf)
> @@ -147,8 +148,7 @@ dhcp_request(struct virtio_dev *dev, cha
> }
>
> if ((client_addr.s_addr =
> - vm_priv_addr(&env->vmd_cfg,
> - dev->vm_vmid, vionet->idx, 1)) == 0)
> + vm_priv_addr(current_vm, vionet->idx, 1)) == 0)
> return (-1);
> memcpy(&resp.yiaddr, &client_addr,
> sizeof(client_addr));
> @@ -156,8 +156,8 @@ dhcp_request(struct virtio_dev *dev, cha
> sizeof(client_addr));
> ss2sin(&pc.pc_dst)->sin_port = htons(CLIENT_PORT);
>
> - if ((server_addr.s_addr = vm_priv_addr(&env->vmd_cfg, dev->vm_vmid,
> - vionet->idx, 0)) == 0)
> + if ((server_addr.s_addr =
> + vm_priv_addr(current_vm, vionet->idx, 0)) == 0)
> return (-1);
> memcpy(&resp.siaddr, &server_addr, sizeof(server_addr));
> memcpy(&ss2sin(&pc.pc_src)->sin_addr, &server_addr,
> Index: priv.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/priv.c,v
> retrieving revision 1.22
> diff -u -p -u -p -r1.22 priv.c
> --- priv.c 28 Jan 2023 14:40:53 -0000 1.22
> +++ priv.c 14 May 2023 17:04:42 -0000
> @@ -466,8 +466,7 @@ vm_priv_ifconfig(struct privsep *ps, str
> sin4->sin_family = AF_INET;
> sin4->sin_len = sizeof(*sin4);
> if ((sin4->sin_addr.s_addr =
> - vm_priv_addr(&env->vmd_cfg,
> - vm->vm_vmid, i, 0)) == 0)
> + vm_priv_addr(vm, i, 0)) == 0)
> return (-1);
>
> inet_ntop(AF_INET, &sin4->sin_addr,
> @@ -493,8 +492,7 @@ vm_priv_ifconfig(struct privsep *ps, str
> sin6 = ss2sin6(&vfr.vfr_addr);
> sin6->sin6_family = AF_INET6;
> sin6->sin6_len = sizeof(*sin6);
> - if (vm_priv_addr6(&env->vmd_cfg,
> - vm->vm_vmid, i, 0, &sin6->sin6_addr) == -1)
> + if (vm_priv_addr6(vm, i, 0, &sin6->sin6_addr) == -1)
> return (-1);
>
> inet_ntop(AF_INET6, &sin6->sin6_addr,
> @@ -565,9 +563,9 @@ vm_priv_brconfig(struct privsep *ps, str
> }
>
> uint32_t
> -vm_priv_addr(struct vmd_config *cfg, uint32_t vmid, int idx, int isvm)
> +vm_priv_addr(struct vmd_vm *vm, int idx, int isvm)
To improve things here instead of just shuffling things around, I'd
rather see us passing in just what this function needs to compute the
address.
> {
> - struct address *h = &cfg->cfg_localprefix;
> + struct address *h = &vm->vm_params.vmc_localprefix;
> in_addr_t prefix, mask, addr;
>
> /*
> @@ -580,7 +578,7 @@ vm_priv_addr(struct vmd_config *cfg, uin
> mask = prefixlen2mask(h->prefixlen);
>
> /* 2. Encode the VM ID as a per-VM subnet range N, 100.64.N.0/24. */
> - addr = vmid << 8;
> + addr = vm->vm_vmid << 8;
>
> /*
> * 3. Assign a /31 subnet M per VM interface, 100.64.N.M/31.
> @@ -603,7 +601,7 @@ vm_priv_addr(struct vmd_config *cfg, uin
> */
> if (prefix != (addr & mask) || idx >= 0x7f) {
> log_warnx("%s: dhcp address range exceeded,"
> - " vm id %u interface %d", __func__, vmid, idx);
> + " vm id %u interface %d", __func__, vm->vm_vmid, idx);
> return (0);
> }
>
> @@ -611,10 +609,9 @@ vm_priv_addr(struct vmd_config *cfg, uin
> }
>
> int
> -vm_priv_addr6(struct vmd_config *cfg, uint32_t vmid,
> - int idx, int isvm, struct in6_addr *in6_addr)
> +vm_priv_addr6(struct vmd_vm *vm, int idx, int isvm, struct in6_addr
> *in6_addr)
> {
> - struct address *h = &cfg->cfg_localprefix6;
> + struct address *h = &vm->vm_params.vmc_localprefix6;
> struct in6_addr addr, mask;
> uint32_t addr4;
>
> @@ -626,7 +623,7 @@ vm_priv_addr6(struct vmd_config *cfg, ui
> prefixlen2mask6(h->prefixlen, &mask);
>
> /* 2. Encode the VM IPv4 address as subnet, fd00::NN:NN:0:0/96. */
> - if ((addr4 = vm_priv_addr(cfg, vmid, idx, 1)) == 0)
> + if ((addr4 = vm_priv_addr(vm, idx, 1)) == 0)
> return (0);
> memcpy(&addr.s6_addr[8], &addr4, sizeof(addr4));
>
> Index: vmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
> retrieving revision 1.149
> diff -u -p -u -p -r1.149 vmd.c
> --- vmd.c 13 May 2023 23:15:28 -0000 1.149
> +++ vmd.c 14 May 2023 17:04:42 -0000
> @@ -142,7 +142,7 @@ vmd_dispatch_control(int fd, struct priv
> }
>
> /* Try to start the launch of the VM. */
> - res = config_setvm(ps, vm, imsg->hdr.peerid,
> + res = config_setvm(env, vm, imsg->hdr.peerid,
> vm->vm_params.vmc_owner.uid);
> if (res)
> cmd = IMSG_VMDOP_START_VM_RESPONSE;
> @@ -318,7 +318,7 @@ vmd_dispatch_control(int fd, struct priv
> close(imsg->fd);
> } else {
> vm->vm_state |= VM_STATE_RECEIVED;
> - config_setvm(ps, vm, imsg->hdr.peerid,
> + config_setvm(env, vm, imsg->hdr.peerid,
> vmc.vmc_owner.uid);
> log_debug("%s: sending fd to vmm", __func__);
> proc_compose_imsg(ps, PROC_VMM, -1,
> @@ -494,7 +494,7 @@ vmd_dispatch_vmm(int fd, struct privsep_
> } else {
> /* Stop VM instance but keep the tty open */
> vm_stop(vm, 1, __func__);
> - config_setvm(ps, vm, (uint32_t)-1, vm->vm_uid);
> + config_setvm(env, vm, (uint32_t)-1, vm->vm_uid);
> }
>
> /* The error is meaningless for deferred responses */
> @@ -995,7 +995,7 @@ start_vm_batch(int fd, short type, void
> break;
> }
> vm->vm_state &= ~VM_STATE_WAITING;
> - config_setvm(&env->vmd_ps, vm, -1, vm->vm_params.vmc_owner.uid);
> + config_setvm(env, vm, -1, vm->vm_params.vmc_owner.uid);
> }
> log_debug("%s: done starting vms", __func__);
> }
> Index: vmd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.h,v
> retrieving revision 1.122
> diff -u -p -u -p -r1.122 vmd.h
> --- vmd.h 13 May 2023 23:15:28 -0000 1.122
> +++ vmd.h 14 May 2023 17:04:42 -0000
> @@ -195,6 +195,13 @@ struct vmop_owner {
> int64_t gid;
> };
>
> +struct address {
> + struct sockaddr_storage ss;
> + int prefixlen;
> + TAILQ_ENTRY(address) entry;
> +};
> +TAILQ_HEAD(addresslist, address);
> +
> struct vmop_create_params {
> struct vm_create_params vmc_params;
> unsigned int vmc_flags;
> @@ -238,6 +245,8 @@ struct vmop_create_params {
> char vmc_ifgroup[VM_MAX_NICS_PER_VM][IF_NAMESIZE];
> unsigned int vmc_ifrdomain[VM_MAX_NICS_PER_VM];
> uint8_t vmc_macs[VM_MAX_NICS_PER_VM][6];
> + struct address vmc_localprefix;
> + struct address vmc_localprefix6;
>
> struct vmop_owner vmc_owner;
>
> @@ -341,13 +350,6 @@ struct name2id {
> };
> TAILQ_HEAD(name2idlist, name2id);
>
> -struct address {
> - struct sockaddr_storage ss;
> - int prefixlen;
> - TAILQ_ENTRY(address) entry;
> -};
> -TAILQ_HEAD(addresslist, address);
> -
> #define SUN_PATH_LEN (sizeof(((struct sockaddr_un *)NULL)->sun_path))
> struct vmd_agentx {
> int ax_enabled;
> @@ -473,9 +475,8 @@ int priv_findname(const char *, const c
> int priv_validgroup(const char *);
> int vm_priv_ifconfig(struct privsep *, struct vmd_vm *);
> int vm_priv_brconfig(struct privsep *, struct vmd_switch *);
> -uint32_t vm_priv_addr(struct vmd_config *, uint32_t, int, int);
> -int vm_priv_addr6(struct vmd_config *, uint32_t, int, int,
> - struct in6_addr *);
> +uint32_t vm_priv_addr(struct vmd_vm *, int, int);
> +int vm_priv_addr6(struct vmd_vm *, int, int, struct in6_addr *);
>
> /* vmm.c */
> void vmm(struct privsep *, struct privsep_proc *);
> @@ -505,7 +506,7 @@ int config_setconfig(struct vmd *);
> int config_getconfig(struct vmd *, struct imsg *);
> int config_setreset(struct vmd *, unsigned int);
> int config_getreset(struct vmd *, struct imsg *);
> -int config_setvm(struct privsep *, struct vmd_vm *, uint32_t, uid_t);
> +int config_setvm(struct vmd *, struct vmd_vm *, uint32_t, uid_t);
> int config_getvm(struct privsep *, struct imsg *);
> int config_getdisk(struct privsep *, struct imsg *);
> int config_getif(struct privsep *, struct imsg *);
