[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 06/10] monitor: release the lock before calling close()
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PATCH v3 06/10] monitor: release the lock before calling close() |
Date: |
Tue, 28 Feb 2023 18:51:03 +0000 |
User-agent: |
Mutt/2.2.9 (2022-11-12) |
* 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
- [PATCH v3 04/10] osdep: implement qemu_socketpair() for win32, (continued)
[PATCH v3 07/10] qapi: implement conditional command arguments, marcandre . lureau, 2023/02/07
- Re: [PATCH v3 07/10] qapi: implement conditional command arguments, Markus Armbruster, 2023/02/09
- Re: [PATCH v3 07/10] qapi: implement conditional command arguments, Markus Armbruster, 2023/02/17
- Re: [PATCH v3 07/10] qapi: implement conditional command arguments, Marc-André Lureau, 2023/02/18
- Re: [PATCH v3 07/10] qapi: implement conditional command arguments, Markus Armbruster, 2023/02/20
- Re: [PATCH v3 07/10] qapi: implement conditional command arguments, Marc-André Lureau, 2023/02/22
- Re: [PATCH v3 07/10] qapi: implement conditional command arguments, Markus Armbruster, 2023/02/22
- Re: [PATCH v3 07/10] qapi: implement conditional command arguments, Marc-André Lureau, 2023/02/22
- Re: [PATCH v3 07/10] qapi: implement conditional command arguments, Marc-André Lureau, 2023/02/27