Re: [PATCH v3 06/10] monitor: release the lock before calling close()

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Tue, Feb 14, 2023 at 05:36:32PM +0400, Marc-André Lureau wrote:
> > Hi
> > 
> > On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > > marcandre.lureau@redhat.com writes:
> > >
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > >
> > > > As per comment, presumably to avoid syscall in critical section.
> > > >
> > > > Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros")
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >  monitor/fds.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/monitor/fds.c b/monitor/fds.c
> > > > index 26b39a0ce6..03c5e97c35 100644
> > > > --- a/monitor/fds.c
> > > > +++ b/monitor/fds.c
> > > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> > > >          return;
> > > >      }
> > > >
> > > > -    QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> > > > +    qemu_mutex_lock(&cur_mon->mon_lock);
> > > >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> > > >          if (strcmp(monfd->name, fdname) != 0) {
> > > >              continue;
> > > > @@ -88,6 +88,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> > > >
> > > >          tmp_fd = monfd->fd;
> > > >          monfd->fd = fd;
> > > > +        qemu_mutex_unlock(&cur_mon->mon_lock);
> > > >          /* Make sure close() is outside critical section */
> > > >          close(tmp_fd);
> > > >          return;
> > > > @@ -98,6 +99,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> > > >      monfd->fd = fd;
> > > >
> > > >      QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> > > > +    qemu_mutex_unlock(&cur_mon->mon_lock);
> > > >  }
> > > >
> > > >  void qmp_closefd(const char *fdname, Error **errp)
> > >
> > > This confused me.  I think I understand now, but let's double-check.
> > >
> > > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it
> > > extended the criticial section beyond the close(), invalidating the
> > > comment.  Correct?
> > 
> > Correct
> > 
> > > Did it actually break anything?
> > 
> > Not that I know of (David admitted over IRC that this was not intended)
> 
> Conceptually the only risk here is that 'close()' blocks for a
> prolonged period of time, which prevents another thread from
> acquiring the mutex.
> 
> First, the chances of close() blocking are incredibly low for
> socket FDs which have not yet been used to transmit data. It
> would require a malicious mgmt app to pass an unexpected FD
> type that could block but that's quite hard, and we consider
> the QMP client be a trusted entity anyway.

I agree it's unlikely; I'm not sure it actually requires something
malicious though; e.g. a managmeent app that is itself blocked,
a socket connection connection over a dead network etc are the ones
we're worrying about - stuff that's not so much slow
as either deadlocked or taking minutes for recovery/timeout.

Dave

> As for another thread blocking on the mutex I'm not convinced
> that'll happen either. The FD set is scoped to the current
> monitor. Almost certainly the FD is going to be consumed by
> a later QMP device-add/object-add command, in the same thread.
> Processing of that later QMP command will be delayed regardless
> of whether the close is inside or outside the critical section.
> 
> AFAICT keeping close() oujtside the critical section serves
> no purpose and we could just stick with the lock guard and
> delete the comment.
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK