From: David Hildenbrand <david@redhat.com>
To: Raphael Norwitz <raphael.norwitz@nutanix.com>
Cc: "Michael S . Tsirkin" <mst@redhat.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"Coiby Xu" <coiby.xu@gmail.com>,
"Stefan Hajnoczi" <stefanha@redhat.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Marc-André Lureau" <marcandre.lureau@redhat.com>
Subject: Re: [PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd
Date: Thu, 14 Oct 2021 09:06:51 +0200 [thread overview]
Message-ID: <9391f500-70be-26cf-bcfc-591d3ee84d4e@redhat.com> (raw)
In-Reply-To: <20211014052918.GB21284@raphael-debian-dev>
On 14.10.21 07:29, Raphael Norwitz wrote:
> On Wed, Oct 13, 2021 at 11:51:24AM +0200, David Hildenbrand wrote:
>> On 13.10.21 11:48, Stefan Hajnoczi wrote:
>>> On Tue, Oct 12, 2021 at 08:38:32PM +0200, David Hildenbrand wrote:
>>>> We end up not closing the file descriptor, resulting in leaking one
>>>> file descriptor for each VHOST_USER_REM_MEM_REG message.
>>>>
>>>> Fixes: 875b9fd97b34 ("Support individual region unmap in libvhost-user")
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
>>>> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Coiby Xu <coiby.xu@gmail.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>> subprojects/libvhost-user/libvhost-user.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
>>>> index bf09693255..bb5c3b3280 100644
>>>> --- a/subprojects/libvhost-user/libvhost-user.c
>>>> +++ b/subprojects/libvhost-user/libvhost-user.c
>>>> @@ -839,6 +839,8 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>>>> vu_panic(dev, "Specified region not found\n");
>>>> }
>>>> + close(vmsg->fds[0]);
>>>
>>> Does anything check that exactly 1 fd was received? For example,
>>> vu_set_log_fd_exec() does:
>>>
>>> if (vmsg->fd_num != 1) {
>>> vu_panic(dev, "Invalid log_fd message");
>>> return false;
>>> }
>>>
>>> I think that's necessary both to make vhost-user master development
>>> easier and because fds[] is not initialized to -1.
>
> Ack - will add that.
>
>>
>> Similarly, vu_add_mem_reg() assumes exactly one was sent AFAIKS.
>
> Ack
>
>>
>> If we panic, do we still have to call vmsg_close_fds() ?
>>
>
> I think so. What else will close the FDs?
>
> AFAICT a vu_panic does not imply that the overall process has to die if that's
> what you mean. What if one process is exposing multiple devices and only one of
> them panics?
So IIUC, you'll send some patches to tackle the fd checks?
While at it, we might want to simplify VHOST_USER_REM_MEM_REG.
I have a patch there that needs tweaking to cover the point Stefan raised
regarding duplicate ranges. We might want to do the memmove within the loop
instead and drop the "break" to process all elements.
commit 34d71b6531c74a61442432b37e5829a76a7017c5
Author: David Hildenbrand <david@redhat.com>
Date: Tue Oct 12 13:25:43 2021 +0200
libvhost-user: Simplify VHOST_USER_REM_MEM_REG
Let's avoid having to manually copy all elements. Copy only the ones
necessary to close the hole and perform the operation in-place without
a second array.
Signed-off-by: David Hildenbrand <david@redhat.com>
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 7b0e40256e..499c31dc68 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -796,10 +796,8 @@ static inline bool reg_equal(VuDevRegion *vudev_reg,
static bool
vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
- int i, j;
- bool found = false;
- VuDevRegion shadow_regions[VHOST_USER_MAX_RAM_SLOTS] = {};
VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m;
+ int i;
DPRINT("Removing region:\n");
DPRINT(" guest_phys_addr: 0x%016"PRIx64"\n",
@@ -811,28 +809,27 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
DPRINT(" mmap_offset 0x%016"PRIx64"\n",
msg_region->mmap_offset);
- for (i = 0, j = 0; i < dev->nregions; i++) {
- if (!reg_equal(&dev->regions[i], msg_region)) {
- shadow_regions[j].gpa = dev->regions[i].gpa;
- shadow_regions[j].size = dev->regions[i].size;
- shadow_regions[j].qva = dev->regions[i].qva;
- shadow_regions[j].mmap_addr = dev->regions[i].mmap_addr;
- shadow_regions[j].mmap_offset = dev->regions[i].mmap_offset;
- j++;
- } else {
- found = true;
+ for (i = 0; i < dev->nregions; i++) {
+ if (reg_equal(&dev->regions[i], msg_region)) {
VuDevRegion *r = &dev->regions[i];
void *m = (void *) (uintptr_t) r->mmap_addr;
if (m) {
munmap(m, r->size + r->mmap_offset);
}
+ break;
}
}
- if (found) {
- memcpy(dev->regions, shadow_regions,
- sizeof(VuDevRegion) * VHOST_USER_MAX_RAM_SLOTS);
+ if (i < dev->nregions) {
+ /*
+ * Shift all affected entries by 1 to close the hole at index i and
+ * zero out the last entry.
+ */
+ memmove(dev->regions + i, dev->regions + i + 1,
+ sizeof(VuDevRegion) * (dev->nregions - i - 1));
+ memset(dev->regions + dev->nregions - 1, 0,
+ sizeof(VuDevRegion));
DPRINT("Successfully removed a region\n");
dev->nregions--;
vmsg_set_reply_u64(vmsg, 0);
On a related note, I proposed in a RFC series to increase the memslot count:
https://lkml.kernel.org/r/20211013103330.26869-1-david@redhat.com
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2021-10-14 7:07 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-12 18:38 [PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd David Hildenbrand
2021-10-13 6:06 ` Raphael Norwitz
2021-10-13 9:48 ` Stefan Hajnoczi
2021-10-13 9:51 ` David Hildenbrand
2021-10-14 5:29 ` Raphael Norwitz
2021-10-14 7:06 ` David Hildenbrand [this message]
2021-10-14 15:53 ` Raphael Norwitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9391f500-70be-26cf-bcfc-591d3ee84d4e@redhat.com \
--to=david@redhat.com \
--cc=coiby.xu@gmail.com \
--cc=marcandre.lureau@redhat.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=raphael.norwitz@nutanix.com \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).