From: Mike Larkin <mlarkin@nested.page>
Subject: Re: vmd(8): fix race condition with vcpu pause barrier
To: Dave Voutila <dv@sisu.io>
Cc: tech@openbsd.org
Date: Sun, 30 Nov 2025 11:09:22 -0800

On Thu, Nov 27, 2025 at 01:18:04PM -0500, Dave Voutila wrote:
> The vm pause function currently allocates and frees a pthread barrier
> object at pause time, probably because at one point the idea the # of
> vcpus could be dynamic.
>
> While we can save a few bytes doing it this way if the vm is never
> paused, it's far simpler to initialize and destroy this barrier as part
> of the greater vm lifecycle before vcpus are launched and after they are
> destroyed.
>
> If you want to trigger this race condition easily, insert a `sleep(1)`
> or the like in pause_vm(). Boot a ramdisk kernel guest and at the shell
> prompt run something cpu heavy like `while true; do date; done`. Then
> use vmctl(8) to try pausing the vm. The vcpu thread will try to
> pthread_barrier_wait on an uninitialized barrier object and fail. Your
> vm is now borked. :D
>
> ok?
>

ok mlarkin

> -dv
>
>
> diffstat refs/heads/master refs/heads/vmd-pause-barrier
>  M  usr.sbin/vmd/vm.c  |  11+  16-
>
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff refs/heads/master refs/heads/vmd-pause-barrier
> commit - 076e7c3b0745cd2dca97eb8708c522b384d17250
> commit + b2f4db7d6f1ac0311376b308e3583696da731802
> blob - 0b4f7ae3d7e42e451453e9c910de7d8fbcf137ff
> blob + 8fe9a134d16644cfe2a403c71cd76e7321ac7fef
> --- usr.sbin/vmd/vm.c
> +++ usr.sbin/vmd/vm.c
> @@ -442,14 +442,6 @@ pause_vm(struct vmd_vm *vm)
>  	current_vm->vm_state |= VM_STATE_PAUSED;
>  	mutex_unlock(&vm_mtx);
>
> -	ret = pthread_barrier_init(&vm_pause_barrier, NULL,
> -	    vm->vm_params.vmc_params.vcp_ncpus + 1);
> -	if (ret) {
> -		log_warnx("%s: cannot initialize pause barrier (%d)",
> -		    __progname, ret);
> -		return;
> -	}
> -
>  	for (n = 0; n < vm->vm_params.vmc_params.vcp_ncpus; n++) {
>  		ret = pthread_cond_broadcast(&vcpu_run_cond[n]);
>  		if (ret) {
> @@ -465,13 +457,6 @@ pause_vm(struct vmd_vm *vm)
>  		return;
>  	}
>
> -	ret = pthread_barrier_destroy(&vm_pause_barrier);
> -	if (ret) {
> -		log_warnx("%s: could not destroy pause barrier (%d)",
> -		    __progname, ret);
> -		return;
> -	}
> -
>  	pause_vm_md(vm);
>  }
>
> @@ -623,6 +608,12 @@ run_vm(struct vmop_create_params *vmc, struct vcpu_reg
>  		return (ENOMEM);
>  	}
>
> +	ret = pthread_barrier_init(&vm_pause_barrier, NULL, vcp->vcp_ncpus + 1);
> +	if (ret) {
> +		log_warnx("cannot initialize pause barrier (%d)", ret);
> +		return (ret);
> +	}
> +
>  	log_debug("%s: starting %zu vcpu thread(s) for vm %s", __func__,
>  	    vcp->vcp_ncpus, vcp->vcp_name);
>
> @@ -781,11 +772,15 @@ run_vm(struct vmop_create_params *vmc, struct vcpu_reg
>  		}
>  		mutex_unlock(&vm_mtx);
>  		if (i == vcp->vcp_ncpus)
> -			return (ret);
> +			break;
>
>  		/* Some more threads to wait for, start over */
>  	}
>
> +	ret = pthread_barrier_destroy(&vm_pause_barrier);
> +	if (ret)
> +		log_warnx("could not destroy pause barrier (%d)", ret);
> +
>  	return (ret);
>  }
>