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

[Raphael Norwitz <raphael.norwitz@nutanix.com>]

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
>