DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Xia, Chenbo" <chenbo.xia@intel.com>
To: Gaoxiang Liu <gaoxiangliu0@163.com>,
	"maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"liugaoxiang@huawei.com" <liugaoxiang@huawei.com>
Subject: Re: [dpdk-dev] [PATCH v5] vhost: fix crash on port deletion
Date: Thu, 26 Aug 2021 08:37:45 +0000	[thread overview]
Message-ID: <MN2PR11MB4063C7748EB0D7F41FC90FF49CC79@MN2PR11MB4063.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210820154615.551-1-gaoxiangliu0@163.com>

Hi Gaoxiang,

> -----Original Message-----
> From: Gaoxiang Liu <gaoxiangliu0@163.com>
> Sent: Friday, August 20, 2021 11:46 PM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; liugaoxiang@huawei.com; Gaoxiang Liu <gaoxiangliu0@163.com>
> Subject: [PATCH v5] vhost: fix crash on port deletion
> 
> The rte_vhost_driver_unregister() and vhost_user_read_cb()
> can be called at the same time by 2 threads.
> when memory of vsocket is freed in rte_vhost_driver_unregister(),
> the invalid memory of vsocket is accessd in vhost_user_read_cb().
> It's a bug of both mode for vhost as server or client.
> 
> Eg vhostuser port is created as server.
> Thread1 calls rte_vhost_driver_unregister().
> Before the listen fd is deleted from poll waiting fds,
> "vhost-events" thread then calls vhost_user_server_new_connection(),
> then a new conn fd is added in fdset when trying to reconnect.
> "vhost-events" thread then calls vhost_user_read_cb() and
> access invalid memory of socket while thread1 frees the memory of
> vsocket.
> 
> Eg vhostuser port is created as client.
> Thread1 calls rte_vhost_driver_unregister().
> Before vsocket of reconn is deleted from reconn list,
> "vhost_reconn" thread then calls vhost_user_add_connection()
> then a new conn fd is added in fdset when trying to reconnect.
> "vhost-events" thread then calls vhost_user_read_cb() and
> access invalid memory of socket while thread1 frees the memory of
> vsocket.
> 
> The fix is to move the "fdset_try_del" in front of free memory of conn,
> then avoid the race condition.
> 
> The core trace is:
> Program terminated with signal 11, Segmentation fault.

Thanks for the detailed commit log. It looks better now and easier to understand.

And please make the some words more formal. Like 'Eg' -> 'E.g.,'

> 
> Fixes: 52d874dc6705 ("vhost: fix crash on closing in client mode")
> 
> Signed-off-by: Gaoxiang Liu <liugaoxiang@huawei.com>

Please add a new line with " --- " (as I showed you in v4). It should look like:


Signed-off-by: Gaoxiang Liu <liugaoxiang@huawei.com>
---

v2:
* Fix coding style issues.

If you don’t do this, the description will be shown in the commit log, which is
not wanted.

> 
> v2:
> * Fix coding style issues.
> 
> v3:
> * Add detailed log.
> 
> v4:
> * Add the reason when vhostuser port is created as server.
> 
> v5:
> * Add detailed log when vhostuser port is created as client.
> ---
>  lib/vhost/socket.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
> index 5d0d728d5..2eb8fcadd 100644
> --- a/lib/vhost/socket.c
> +++ b/lib/vhost/socket.c
> @@ -1024,6 +1024,20 @@ rte_vhost_driver_unregister(const char *path)
>  	for (i = 0; i < vhost_user.vsocket_cnt; i++) {
>  		struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
> 
> +		if (vsocket->is_server) {
> +			/*
> +			 * If r/wcb is executing, release vhost_user's
> +			 * mutex lock, and try again since the r/wcb
> +			 * may use the mutex lock.
> +			 */
> +			if (fdset_try_del(&vhost_user.fdset, vsocket->socket_fd) ==
> -1) {
> +				pthread_mutex_unlock(&vhost_user.mutex);
> +				goto again;
> +			}
> +		} else if (vsocket->reconnect) {
> +			vhost_user_remove_reconnect(vsocket);
> +		}
> +
>  		if (!strcmp(vsocket->path, path)) {

You should first check the param 'path' before doing anything, so please move the
strcmp too.

>  			pthread_mutex_lock(&vsocket->conn_mutex);
>  			for (conn = TAILQ_FIRST(&vsocket->conn_list);
> @@ -1056,21 +1070,8 @@ rte_vhost_driver_unregister(const char *path)
>  			pthread_mutex_unlock(&vsocket->conn_mutex);
> 
>  			if (vsocket->is_server) {
> -				/*
> -				 * If r/wcb is executing, release vhost_user's
> -				 * mutex lock, and try again since the r/wcb
> -				 * may use the mutex lock.
> -				 */
> -				if (fdset_try_del(&vhost_user.fdset,
> -						vsocket->socket_fd) == -1) {
> -					pthread_mutex_unlock(&vhost_user.mutex);
> -					goto again;
> -				}
> -
>  				close(vsocket->socket_fd);
>  				unlink(path);

Any reason why we don't move this?

Thanks,
Chenbo

> -			} else if (vsocket->reconnect) {
> -				vhost_user_remove_reconnect(vsocket);
>  			}
> 
>  			pthread_mutex_destroy(&vsocket->conn_mutex);
> --
> 2.32.0
> 


  reply	other threads:[~2021-08-26  8:37 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-07  8:25 [dpdk-dev] [PATCH] vhost: fix coredump " Gaoxiang Liu
2021-08-07 23:12 ` [dpdk-dev] [PATCH v2] " Gaoxiang Liu
2021-08-13 14:02   ` [dpdk-dev] [PATCH] vhost: fix crash on port deletion The rte_vhost_driver_unregister() and vhost_user_read_cb() can be called at the same time by 2 threads. Eg thread1 calls rte_vhost_driver_unregister() and frees memory of "conn". Because socket fd has not been deleted from poll waiting fds, "vhost-events" thread calls fdset_event_dispatch, then calls vhost_user_read_cb(), and accesses invalid memory of "conn" Gaoxiang Liu
2021-08-13 14:22   ` [dpdk-dev] [PATCH] vhost: fix crash on port deletion Gaoxiang Liu
2021-08-16  6:44     ` Xia, Chenbo
2021-08-20 15:53       ` Gaoxiang Liu
2021-08-18 16:08     ` [dpdk-dev] [PATCH v4] " Gaoxiang Liu
2021-08-20 15:46       ` [dpdk-dev] [PATCH v5] " Gaoxiang Liu
2021-08-26  8:37         ` Xia, Chenbo [this message]
2021-08-27 14:19         ` [dpdk-dev] [PATCH v6] " Gaoxiang Liu
2021-08-31  5:37           ` Xia, Chenbo
2021-09-02 15:38             ` Gaoxiang Liu
2021-09-06  3:18               ` Xia, Chenbo
2021-09-06  3:32                 ` Xia, Chenbo
2021-09-06  3:54                 ` Gaoxiang Liu
2021-09-02 15:45           ` [dpdk-dev] [PATCH v7] " Gaoxiang Liu
2021-09-06  3:24             ` Xia, Chenbo
2021-09-06  5:19             ` Xia, Chenbo
2021-09-14 11:29             ` Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MN2PR11MB4063C7748EB0D7F41FC90FF49CC79@MN2PR11MB4063.namprd11.prod.outlook.com \
    --to=chenbo.xia@intel.com \
    --cc=dev@dpdk.org \
    --cc=gaoxiangliu0@163.com \
    --cc=liugaoxiang@huawei.com \
    --cc=maxime.coquelin@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).