[PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd

[PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd

[David Hildenbrand]

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]);
+
     return true;
 }
 
-- 
2.31.1

Re: [PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd

[Raphael Norwitz]

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>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.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]);
> +
>      return true;
>  }
>  
> -- 
> 2.31.1
> 

Re: [PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

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.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd

[David Hildenbrand]

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.

Similarly, vu_add_mem_reg() assumes exactly one was sent AFAIKS.

If we panic, do we still have to call vmsg_close_fds() ?

-- 
Thanks,

David / dhildenb

Re: [PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd

[Raphael Norwitz]

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?

> -- 
> Thanks,
> 
> David / dhildenb
> 

Re: [PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd

[David Hildenbrand]

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

Re: [PATCH v1] libvhost-user: fix VHOST_USER_REM_MEM_REG not closing the fd

[Raphael Norwitz]

On Thu, Oct 14, 2021 at 09:06:51AM +0200, David Hildenbrand wrote:
> 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?
>

Yes

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

Sure - let me include this in the series.

> 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://lore.kernel.org/all/20211013103330.26869-1-david@redhat.com/T/#mbaa35cebb311e7ab9c029f9f99fb2ba41e993b9f
>

Thanks for pointing that out. I don't see any problem with bumping the
memslot count. I don't expect it will conflict with anything I'm doing
but will keep it in mind.

> -- 
> Thanks,
> 
> David / dhildenb
>