[dpdk-dev] [PATCH v2] vhost: fix driver unregister for client mode

[Ilya Maximets]

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];
>