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

[Daniel P . Berrangé]

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.

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 :|