| Message ID | 20220106064717.7477-6-raphael.norwitz@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up error handling in libvhost-user memory mapping | expand |
On 06.01.22 07:47, Raphael Norwitz wrote: > Today if QEMU (or any other VMM) has sent multiple copies of the same > region to a libvhost-user based backend and then attempts to remove the > region, only one instance of the region will be removed, leaving stale > copies of the region in dev->regions[]. > > This change resolves this by having vu_rem_mem_reg() iterate through all > regions in dev->regions[] and delete all matching regions. > > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > --- > subprojects/libvhost-user/libvhost-user.c | 26 ++++++++++++----------- > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > index 0fe3aa155b..14482484d3 100644 > --- a/subprojects/libvhost-user/libvhost-user.c > +++ b/subprojects/libvhost-user/libvhost-user.c > @@ -809,6 +809,7 @@ static bool > vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { > VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m; > int i; > + bool found = false; > > if (vmsg->fd_num != 1 || > vmsg->size != sizeof(vmsg->payload.memreg)) { > @@ -835,21 +836,22 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { > munmap(m, r->size + r->mmap_offset); > } > > - break; > + /* > + * 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--; > + i--; > + > + found = true; Maybe add a comment like /* Continue the search for eventual duplicates. */
On Thu, Jan 06, 2022 at 06:47:36AM +0000, Raphael Norwitz wrote: > Today if QEMU (or any other VMM) has sent multiple copies of the same > region to a libvhost-user based backend and then attempts to remove the > region, only one instance of the region will be removed, leaving stale > copies of the region in dev->regions[]. > > This change resolves this by having vu_rem_mem_reg() iterate through all > regions in dev->regions[] and delete all matching regions. > > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > --- > subprojects/libvhost-user/libvhost-user.c | 26 ++++++++++++----------- > 1 file changed, 14 insertions(+), 12 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Mon, Jan 10, 2022 at 09:58:01AM +0100, David Hildenbrand wrote: > On 06.01.22 07:47, Raphael Norwitz wrote: > > Today if QEMU (or any other VMM) has sent multiple copies of the same > > region to a libvhost-user based backend and then attempts to remove the > > region, only one instance of the region will be removed, leaving stale > > copies of the region in dev->regions[]. > > > > This change resolves this by having vu_rem_mem_reg() iterate through all > > regions in dev->regions[] and delete all matching regions. > > > > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > > --- > > subprojects/libvhost-user/libvhost-user.c | 26 ++++++++++++----------- > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c > > index 0fe3aa155b..14482484d3 100644 > > --- a/subprojects/libvhost-user/libvhost-user.c > > +++ b/subprojects/libvhost-user/libvhost-user.c > > @@ -809,6 +809,7 @@ static bool > > vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { > > VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m; > > int i; > > + bool found = false; > > > > if (vmsg->fd_num != 1 || > > vmsg->size != sizeof(vmsg->payload.memreg)) { > > @@ -835,21 +836,22 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { > > munmap(m, r->size + r->mmap_offset); > > } > > > > - break; > > + /* > > + * 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--; > > + i--; > > + > > + found = true; > > Maybe add a comment like > > /* Continue the search for eventual duplicates. */ Ack - I'll add it. > > > -- > Thanks, > > David / dhildenb >
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c index 0fe3aa155b..14482484d3 100644 --- a/subprojects/libvhost-user/libvhost-user.c +++ b/subprojects/libvhost-user/libvhost-user.c @@ -809,6 +809,7 @@ static bool vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { VhostUserMemoryRegion m = vmsg->payload.memreg.region, *msg_region = &m; int i; + bool found = false; if (vmsg->fd_num != 1 || vmsg->size != sizeof(vmsg->payload.memreg)) { @@ -835,21 +836,22 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) { munmap(m, r->size + r->mmap_offset); } - break; + /* + * 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--; + i--; + + found = true; } } - 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--; + if (found) { vmsg_set_reply_u64(vmsg, 0); } else { vu_panic(dev, "Specified region not found\n");
Today if QEMU (or any other VMM) has sent multiple copies of the same region to a libvhost-user based backend and then attempts to remove the region, only one instance of the region will be removed, leaving stale copies of the region in dev->regions[]. This change resolves this by having vu_rem_mem_reg() iterate through all regions in dev->regions[] and delete all matching regions. Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> --- subprojects/libvhost-user/libvhost-user.c | 26 ++++++++++++----------- 1 file changed, 14 insertions(+), 12 deletions(-)