I've fixed leak of file descriptors in 'vhost_user_remove_reconnect()'
and sent v3.
On 21.07.2016 11:31, Ilya Maximets wrote:
> Currently while calling of 'rte_vhost_driver_unregister()' connection
> to QEMU will not be closed. This leads to inability to register driver
> again and reconnect to same virtual machine.
>
> This scenario is reproducible with OVS. While executing of the following
> command vhost port will be re-created (will be executed
> 'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()')
> network will be broken and QEMU possibly will crash:
>
> ovs-vsctl set Interface vhost1 ofport_request=15
>
> Fix this by closing all established connections on driver unregister and
> removing of pending connections from reconnection list.
>
> Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
>
> Acked-by: Yuanhan Liu <yuanhan.liu at linux.intel.com>
> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
> ---
> lib/librte_vhost/vhost_user/fd_man.c | 15 ++++++--
> lib/librte_vhost/vhost_user/fd_man.h | 2 +-
> lib/librte_vhost/vhost_user/vhost-net-user.c | 56
> ++++++++++++++++++++++++----
> 3 files changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/lib/librte_vhost/vhost_user/fd_man.c
> b/lib/librte_vhost/vhost_user/fd_man.c
> index c691339..2d3eeb7 100644
> --- a/lib/librte_vhost/vhost_user/fd_man.c
> +++ b/lib/librte_vhost/vhost_user/fd_man.c
> @@ -132,8 +132,10 @@ fdset_init(struct fdset *pfdset)
> if (pfdset == NULL)
> return;
>
> - for (i = 0; i < MAX_FDS; i++)
> + for (i = 0; i < MAX_FDS; i++) {
> pfdset->fd[i].fd = -1;
> + pfdset->fd[i].dat = NULL;
> + }
> pfdset->num = 0;
> }
>
> @@ -166,14 +168,16 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb,
> fd_cb wcb, void *dat)
>
> /**
> * Unregister the fd from the fdset.
> + * Returns context of a given fd or NULL.
> */
> -void
> +void *
> fdset_del(struct fdset *pfdset, int fd)
> {
> int i;
> + void *dat = NULL;
>
> if (pfdset == NULL || fd == -1)
> - return;
> + return NULL;
>
> do {
> pthread_mutex_lock(&pfdset->fd_mutex);
> @@ -181,13 +185,17 @@ fdset_del(struct fdset *pfdset, int fd)
> i = fdset_find_fd(pfdset, fd);
> if (i != -1 && pfdset->fd[i].busy == 0) {
> /* busy indicates r/wcb is executing! */
> + dat = pfdset->fd[i].dat;
> pfdset->fd[i].fd = -1;
> pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
> + pfdset->fd[i].dat = NULL;
> pfdset->num--;
> i = -1;
> }
> pthread_mutex_unlock(&pfdset->fd_mutex);
> } while (i != -1);
> +
> + return dat;
> }
>
> /**
> @@ -203,6 +211,7 @@ fdset_del_slot(struct fdset *pfdset, int index)
>
> pfdset->fd[index].fd = -1;
> pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
> + pfdset->fd[index].dat = NULL;
> pfdset->num--;
>
> pthread_mutex_unlock(&pfdset->fd_mutex);
> diff --git a/lib/librte_vhost/vhost_user/fd_man.h
> b/lib/librte_vhost/vhost_user/fd_man.h
> index 74ecde2..bd66ed1 100644
> --- a/lib/librte_vhost/vhost_user/fd_man.h
> +++ b/lib/librte_vhost/vhost_user/fd_man.h
> @@ -60,7 +60,7 @@ void fdset_init(struct fdset *pfdset);
> int fdset_add(struct fdset *pfdset, int fd,
> fd_cb rcb, fd_cb wcb, void *dat);
>
> -void fdset_del(struct fdset *pfdset, int fd);
> +void *fdset_del(struct fdset *pfdset, int fd);
>
> void fdset_event_dispatch(struct fdset *pfdset);
>
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c
> b/lib/librte_vhost/vhost_user/vhost-net-user.c
> index f0ea3a2..8c6a096 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -60,6 +60,7 @@
> struct vhost_user_socket {
> char *path;
> int listenfd;
> + int connfd;
> bool is_server;
> bool reconnect;
> };
> @@ -277,11 +278,13 @@ vhost_user_add_connection(int fd, struct
> vhost_user_socket *vsocket)
>
> RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
>
> + vsocket->connfd = fd;
> conn->vsocket = vsocket;
> conn->vid = vid;
> ret = fdset_add(&vhost_user.fdset, fd, vhost_user_msg_handler,
> NULL, conn);
> if (ret < 0) {
> + vsocket->connfd = -1;
> free(conn);
> close(fd);
> RTE_LOG(ERR, VHOST_CONFIG,
> @@ -329,6 +332,7 @@ vhost_user_msg_handler(int connfd, void *dat, int *remove)
> RTE_LOG(ERR, VHOST_CONFIG,
> "vhost read incorrect message\n");
>
> + vsocket->connfd = -1;
> close(connfd);
> *remove = 1;
> free(conn);
> @@ -635,6 +639,7 @@ rte_vhost_driver_register(const char *path, uint64_t
> flags)
> goto out;
> memset(vsocket, 0, sizeof(struct vhost_user_socket));
> vsocket->path = strdup(path);
> + vsocket->connfd = -1;
>
> if ((flags & RTE_VHOST_USER_CLIENT) != 0) {
> vsocket->reconnect = !(flags & RTE_VHOST_USER_NO_RECONNECT);
> @@ -664,6 +669,29 @@ out:
> return ret;
> }
>
> +static bool
> +vhost_user_remove_reconnect(struct vhost_user_socket *vsocket)
> +{
> + int found = false;
> + struct vhost_user_reconnect *reconn, *next;
> +
> + pthread_mutex_lock(&reconn_list.mutex);
> +
> + for (reconn = TAILQ_FIRST(&reconn_list.head);
> + reconn != NULL; reconn = next) {
> + next = TAILQ_NEXT(reconn, next);
> +
> + if (reconn->vsocket == vsocket) {
> + TAILQ_REMOVE(&reconn_list.head, reconn, next);
> + free(reconn);
> + found = true;
> + break;
> + }
> + }
> + pthread_mutex_unlock(&reconn_list.mutex);
> + return found;
> +}
> +
> /**
> * Unregister the specified vhost socket
> */
> @@ -672,20 +700,34 @@ rte_vhost_driver_unregister(const char *path)
> {
> int i;
> int count;
> + struct vhost_user_connection *conn;
>
> pthread_mutex_lock(&vhost_user.mutex);
>
> for (i = 0; i < vhost_user.vsocket_cnt; i++) {
> - if (!strcmp(vhost_user.vsockets[i]->path, path)) {
> - if (vhost_user.vsockets[i]->is_server) {
> - fdset_del(&vhost_user.fdset,
> - vhost_user.vsockets[i]->listenfd);
> - close(vhost_user.vsockets[i]->listenfd);
> + struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
> +
> + if (!strcmp(vsocket->path, path)) {
> + if (vsocket->is_server) {
> + fdset_del(&vhost_user.fdset, vsocket->listenfd);
> + close(vsocket->listenfd);
> unlink(path);
> + } else if (vsocket->reconnect) {
> + vhost_user_remove_reconnect(vsocket);
> + }
> +
> + conn = fdset_del(&vhost_user.fdset, vsocket->connfd);
> + if (conn) {
> + RTE_LOG(INFO, VHOST_CONFIG,
> + "free connfd = %d for device '%s'\n",
> + vsocket->connfd, path);
> + close(vsocket->connfd);
> + vhost_destroy_device(conn->vid);
> + free(conn);
> }
>
> - free(vhost_user.vsockets[i]->path);
> - free(vhost_user.vsockets[i]);
> + free(vsocket->path);
> + free(vsocket);
>
> count = --vhost_user.vsocket_cnt;
> vhost_user.vsockets[i] = vhost_user.vsockets[count];
>
