1. Patchew
  2. Libvirt
  3. View series

[PATCH] bhyve: lock domain object while handling monitor events

Roman Bogorodskiy posted 1 patch 2 weeks, 4 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20260806080333.2762-1-bogorodskiy@gmail.com
src/bhyve/bhyve_monitor.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
[PATCH] bhyve: lock domain object while handling monitor events
Posted by Roman Bogorodskiy 2 weeks, 4 days ago 
virBhyveProcessStop() calls virBhyveDomainObjStopWorker(), which expects
the domain object to be locked. It temporarily releases the lock while
stopping the event thread and acquires it again before returning.

bhyveMonitorIO() called the process stop and restart paths without
holding the domain lock. As a result, the lock acquired by
virBhyveDomainObjStopWorker() was never released, causing subsequent
domain API calls to hang after the guest exited.

Lock the domain object while processing the bhyve process exit event and
release it after the stop or restart operation completes.

Fixes: 0041788857dafa46e047c09c90039209a642cb85 ("bhyve: clean up event thread")
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
---
 src/bhyve/bhyve_monitor.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/bhyve/bhyve_monitor.c b/src/bhyve/bhyve_monitor.c
index a24696cad5..a7d7588ee5 100644
--- a/src/bhyve/bhyve_monitor.c
+++ b/src/bhyve/bhyve_monitor.c
@@ -140,11 +140,13 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque)
     }
 
     if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {
+        virObjectLock(vm);
+
         if ((pid_t)kev.ident != vm->pid) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("event from unexpected proc %1$ju!=%2$ju"),
                            (uintmax_t)vm->pid, (uintmax_t)kev.ident);
-            return;
+            goto cleanup;
         }
 
         name = vm->def->name;
@@ -169,6 +171,9 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque)
                 virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN, false);
             }
         }
+
+ cleanup:
+        virObjectUnlock(vm);
     }
 }
 
-- 
2.52.0
Re: [PATCH] bhyve: lock domain object while handling monitor events
Posted by Martin Kletzander via Devel 2 weeks, 3 days ago 
On Thu, Aug 06, 2026 at 10:03:33AM +0200, Roman Bogorodskiy wrote:
>virBhyveProcessStop() calls virBhyveDomainObjStopWorker(), which expects
>the domain object to be locked. It temporarily releases the lock while
>stopping the event thread and acquires it again before returning.
>
>bhyveMonitorIO() called the process stop and restart paths without
>holding the domain lock. As a result, the lock acquired by
>virBhyveDomainObjStopWorker() was never released, causing subsequent
>domain API calls to hang after the guest exited.
>
>Lock the domain object while processing the bhyve process exit event and
>release it after the stop or restart operation completes.
>
>Fixes: 0041788857dafa46e047c09c90039209a642cb85 ("bhyve: clean up event thread")
>Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
>---
> src/bhyve/bhyve_monitor.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/src/bhyve/bhyve_monitor.c b/src/bhyve/bhyve_monitor.c
>index a24696cad5..a7d7588ee5 100644
>--- a/src/bhyve/bhyve_monitor.c
>+++ b/src/bhyve/bhyve_monitor.c
>@@ -140,11 +140,13 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque)
>     }
>
>     if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {

If this condition is false you do not lock the vm, but ...

>+        virObjectLock(vm);
>+
>         if ((pid_t)kev.ident != vm->pid) {
>             virReportError(VIR_ERR_INTERNAL_ERROR,
>                            _("event from unexpected proc %1$ju!=%2$ju"),
>                            (uintmax_t)vm->pid, (uintmax_t)kev.ident);
>-            return;
>+            goto cleanup;
>         }
>
>         name = vm->def->name;
>@@ -169,6 +171,9 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque)
>                 virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN, false);
>             }
>         }
>+
>+ cleanup:
>+        virObjectUnlock(vm);

... you try to unlock it anyway.  The commit message sounds like it
might even be wanted, which I doubt.  But even if it was, such functions
are source of a lot of problems.

>     }
> }
>
>-- 
>2.52.0
>
Re: [PATCH] bhyve: lock domain object while handling monitor events
Posted by Roman Bogorodskiy 2 weeks, 3 days ago 
  Martin Kletzander wrote:

> On Thu, Aug 06, 2026 at 10:03:33AM +0200, Roman Bogorodskiy wrote:
> > virBhyveProcessStop() calls virBhyveDomainObjStopWorker(), which expects
> > the domain object to be locked. It temporarily releases the lock while
> > stopping the event thread and acquires it again before returning.
> > 
> > bhyveMonitorIO() called the process stop and restart paths without
> > holding the domain lock. As a result, the lock acquired by
> > virBhyveDomainObjStopWorker() was never released, causing subsequent
> > domain API calls to hang after the guest exited.
> > 
> > Lock the domain object while processing the bhyve process exit event and
> > release it after the stop or restart operation completes.
> > 
> > Fixes: 0041788857dafa46e047c09c90039209a642cb85 ("bhyve: clean up event thread")
> > Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> > ---
> > src/bhyve/bhyve_monitor.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/bhyve/bhyve_monitor.c b/src/bhyve/bhyve_monitor.c
> > index a24696cad5..a7d7588ee5 100644
> > --- a/src/bhyve/bhyve_monitor.c
> > +++ b/src/bhyve/bhyve_monitor.c
> > @@ -140,11 +140,13 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque)
> >     }
> > 
> >     if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {
> 
> If this condition is false you do not lock the vm, but ...
> 
> > +        virObjectLock(vm);
> > +
> >         if ((pid_t)kev.ident != vm->pid) {
> >             virReportError(VIR_ERR_INTERNAL_ERROR,
> >                            _("event from unexpected proc %1$ju!=%2$ju"),
> >                            (uintmax_t)vm->pid, (uintmax_t)kev.ident);
> > -            return;
> > +            goto cleanup;
> >         }
> > 
> >         name = vm->def->name;
> > @@ -169,6 +171,9 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque)
> >                 virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN, false);
> >             }
> >         }
> > +
> > + cleanup:
> > +        virObjectUnlock(vm);
> 
> ... you try to unlock it anyway.  The commit message sounds like it
> might even be wanted, which I doubt.  But even if it was, such functions
> are source of a lot of problems.

Hm, it might look like that in a diff, but both virObjectLock() and
virObjectUnlock() are called in body of
"if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {".

So I believe there should be no unmatched lock/unlock calls, if I'm not
missing something.
Re: [PATCH] bhyve: lock domain object while handling monitor events
Posted by Martin Kletzander via Devel 2 weeks, 3 days ago 
On Thu, Aug 06, 2026 at 11:19:23AM +0200, Roman Bogorodskiy wrote:
>  Martin Kletzander wrote:
>
>> On Thu, Aug 06, 2026 at 10:03:33AM +0200, Roman Bogorodskiy wrote:
>> > virBhyveProcessStop() calls virBhyveDomainObjStopWorker(), which expects
>> > the domain object to be locked. It temporarily releases the lock while
>> > stopping the event thread and acquires it again before returning.
>> >
>> > bhyveMonitorIO() called the process stop and restart paths without
>> > holding the domain lock. As a result, the lock acquired by
>> > virBhyveDomainObjStopWorker() was never released, causing subsequent
>> > domain API calls to hang after the guest exited.
>> >
>> > Lock the domain object while processing the bhyve process exit event and
>> > release it after the stop or restart operation completes.
>> >
>> > Fixes: 0041788857dafa46e047c09c90039209a642cb85 ("bhyve: clean up event thread")
>> > Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
>> > ---
>> > src/bhyve/bhyve_monitor.c | 7 ++++++-
>> > 1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/bhyve/bhyve_monitor.c b/src/bhyve/bhyve_monitor.c
>> > index a24696cad5..a7d7588ee5 100644
>> > --- a/src/bhyve/bhyve_monitor.c
>> > +++ b/src/bhyve/bhyve_monitor.c
>> > @@ -140,11 +140,13 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque)
>> >     }
>> >
>> >     if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {
>>
>> If this condition is false you do not lock the vm, but ...
>>
>> > +        virObjectLock(vm);
>> > +
>> >         if ((pid_t)kev.ident != vm->pid) {
>> >             virReportError(VIR_ERR_INTERNAL_ERROR,
>> >                            _("event from unexpected proc %1$ju!=%2$ju"),
>> >                            (uintmax_t)vm->pid, (uintmax_t)kev.ident);
>> > -            return;
>> > +            goto cleanup;
>> >         }
>> >
>> >         name = vm->def->name;
>> > @@ -169,6 +171,9 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque)
>> >                 virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN, false);
>> >             }
>> >         }
>> > +
>> > + cleanup:
>> > +        virObjectUnlock(vm);
>>
>> ... you try to unlock it anyway.  The commit message sounds like it
>> might even be wanted, which I doubt.  But even if it was, such functions
>> are source of a lot of problems.
>
>Hm, it might look like that in a diff, but both virObjectLock() and
>virObjectUnlock() are called in body of
>"if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {".
>
>So I believe there should be no unmatched lock/unlock calls, if I'm not
>missing something.
>

Oh yeah, it sure looks like it now.  I misrepresented that since having
goto labels somewhere else than at the top level of the function is a
bit misleading.  The patch applies the way you said and it is correct.

I think I would still rather prefer avoiding the possible error-prone
style and maybe switch it around a bit.  Few ideas:

a) Reverse the condition:

if (kev.filter != EVFILT_PROC || (kev.fflags & NOTE_EXIT) == 0)
     return;

virObjectLock(vm);
if ((pid_t)kev.ident != vm->pid) {
     virReportError(VIR_ERR_INTERNAL_ERROR,
                    _("event from unexpected proc %1$ju!=%2$ju"),
                    (uintmax_t)vm->pid, (uintmax_t)kev.ident);
     goto cleanup;
}

...

b) Handle the critical section in a separate function:

if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {
     virObjectLock(vm);
     virBhyveProcessHandleExitEvent(vm, kev);
     virObjectUnlock(vm);
}

...

Or just keep it as is if you're fine with it.  It just stuck out to me
when I noticed it.  So either way

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Re: [PATCH] bhyve: lock domain object while handling monitor events
Posted by Roman Bogorodskiy 2 weeks, 3 days ago 
  Martin Kletzander wrote:

> Oh yeah, it sure looks like it now.  I misrepresented that since having
> goto labels somewhere else than at the top level of the function is a
> bit misleading.  The patch applies the way you said and it is correct.
> 
> I think I would still rather prefer avoiding the possible error-prone
> style and maybe switch it around a bit.  Few ideas:
> 
> a) Reverse the condition:
> 
> if (kev.filter != EVFILT_PROC || (kev.fflags & NOTE_EXIT) == 0)
>     return;
> 
> virObjectLock(vm);
> if ((pid_t)kev.ident != vm->pid) {
>     virReportError(VIR_ERR_INTERNAL_ERROR,
>                    _("event from unexpected proc %1$ju!=%2$ju"),
>                    (uintmax_t)vm->pid, (uintmax_t)kev.ident);
>     goto cleanup;
> }
> 
> ...

Thanks, I re-wrote it this way, it definitely looks better and less confusing.

> b) Handle the critical section in a separate function:
> 
> if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {
>     virObjectLock(vm);
>     virBhyveProcessHandleExitEvent(vm, kev);
>     virObjectUnlock(vm);
> }
> 
> ...
> 
> Or just keep it as is if you're fine with it.  It just stuck out to me
> when I noticed it.  So either way
> 
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.