[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: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 06/10] monitor: release the lock before calling close() |
Date: |
Tue, 14 Feb 2023 17:23:08 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
Daniel P. Berrangé <berrange@redhat.com> writes:
> 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.
Makes sense to me.
There's another one in monitor_add_fd().
Both are from Peter's commit 9409fc05fe2 "monitor: protect mon->fds with
mon_lock". Peter, do you remember why you took the trouble to keep
close() outside the critical section? I know it's been a while...
- [PATCH v3 03/10] tests/docker: fix a win32 error due to portability, (continued)
[PATCH v3 07/10] qapi: implement conditional command arguments, marcandre . lureau, 2023/02/07