From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B8F88A00BE; Tue, 10 May 2022 11:00:49 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A4E1D41156; Tue, 10 May 2022 11:00:49 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id 0ACB94069D for ; Tue, 10 May 2022 11:00:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1652173248; x=1683709248; h=from:to:subject:date:message-id:references:in-reply-to: content-transfer-encoding:mime-version; bh=Gmm9ks/Fwzvb79RiAY8BiksBqPiXbJR3HJvdXIFMSZQ=; b=hqRYiedIYtLL/2JBnbOwl0eUymIM8/jMVSjvaDeIH2lDGsGBIeSfGw0m m+V1ocCSrr9hO8ZEa19VZcNqWqfyLYvFDByadiFvuEHR69zbVbVE3Wq3n kTKOAHWzbw7uXRl966hwX9cz9oRix4cteO2JR5Lyxgl+2VrfrR4Pg3Ihm 4VFj0sOU8M+//uLwRBLaqJJ0DhQ6RHvtN7bm5sxAoYG7N/VuzESE9izv1 wxZv6vaugMyETsuO52GqvPzPQILgZg+yR5p00NhXmbXtqBvF53+T2/HTL /B41cZysTKN6hrfvK5h1gxkGDJo/6DOgO2NtXbwufJuCsJOhm3/qY+uM6 Q==; X-IronPort-AV: E=McAfee;i="6400,9594,10342"; a="294541706" X-IronPort-AV: E=Sophos;i="5.91,214,1647327600"; d="scan'208";a="294541706" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 May 2022 02:00:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.91,214,1647327600"; d="scan'208";a="593398044" Received: from fmsmsx605.amr.corp.intel.com ([10.18.126.85]) by orsmga008.jf.intel.com with ESMTP; 10 May 2022 02:00:46 -0700 Received: from fmsmsx611.amr.corp.intel.com (10.18.126.91) by fmsmsx605.amr.corp.intel.com (10.18.126.85) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.27; Tue, 10 May 2022 02:00:46 -0700 Received: from fmsmsx612.amr.corp.intel.com (10.18.126.92) by fmsmsx611.amr.corp.intel.com (10.18.126.91) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.27; Tue, 10 May 2022 02:00:45 -0700 Received: from fmsmsx612.amr.corp.intel.com ([10.18.126.92]) by fmsmsx612.amr.corp.intel.com ([10.18.126.92]) with mapi id 15.01.2308.027; Tue, 10 May 2022 02:00:45 -0700 From: "Hu, Jiayu" To: Maxime Coquelin , "dev@dpdk.org" , "Xia, Chenbo" , "Ding, Xuan" , "Jiang, Cheng1" , "Pai G, Sunil" , "david.marchand@redhat.com" Subject: RE: [PATCH] vhost: add runtime locking check in unsafe APIs Thread-Topic: [PATCH] vhost: add runtime locking check in unsafe APIs Thread-Index: AQHYZEeH0aIO9XGqwEqYKNoeeJ75M60Xydyg Date: Tue, 10 May 2022 09:00:45 +0000 Message-ID: <0003ad4c95bc4057938a9979efce4889@intel.com> References: <20220510082528.1229104-1-maxime.coquelin@redhat.com> In-Reply-To: <20220510082528.1229104-1-maxime.coquelin@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-reaction: no-action dlp-version: 11.6.401.20 dlp-product: dlpe-windows x-originating-ip: [10.239.127.36] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi Maxime, This is a good idea to add the lock check below. But I have two questions: First, rte_vhost_clear_queue_thread_unsafe() is thread unsafe. Why doesn't add the check for it? Second, dev->notify_ops->destroy_device() is called without taking the lock. If vhost applications try to clear inflight packets or even unregister asynchronous data-path in this callback, rather than in dev->notify_ops->vring_state_changed(), asynchronous APIs below will return -1. How to handle this situation? Thanks, Jiayu > -----Original Message----- > From: Maxime Coquelin > Sent: Tuesday, May 10, 2022 4:25 PM > To: dev@dpdk.org; Xia, Chenbo ; Ding, Xuan > ; Hu, Jiayu ; Jiang, Cheng1 > ; Pai G, Sunil ; > david.marchand@redhat.com > Cc: Maxime Coquelin > Subject: [PATCH] vhost: add runtime locking check in unsafe APIs >=20 > This patch adds runtime checks in unsafe Vhost async APIs, to ensure the > access lock is taken. >=20 > The detection won't work every time, as another thread could take the loc= k, > but it would help to detect misuse of these unsafe API. >=20 > Signed-off-by: Maxime Coquelin > --- > lib/vhost/vhost.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) >=20 > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index > df0bb9d043..39cbeb415c 100644 > --- a/lib/vhost/vhost.c > +++ b/lib/vhost/vhost.c > @@ -1732,6 +1732,12 @@ > rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id= ) > if (unlikely(vq =3D=3D NULL || !dev->async_copy)) > return -1; >=20 > + if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) { > + VHOST_LOG_CONFIG(ERR, "(%s) %s() called without access > lock taken.\n", > + dev->ifname, __func__); > + return -1; > + } > + > return async_channel_register(vid, queue_id); } >=20 > @@ -1796,6 +1802,12 @@ > rte_vhost_async_channel_unregister_thread_unsafe(int vid, uint16_t > queue_id) > if (vq =3D=3D NULL) > return -1; >=20 > + if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) { > + VHOST_LOG_CONFIG(ERR, "(%s) %s() called without access > lock taken.\n", > + dev->ifname, __func__); > + return -1; > + } > + > if (!vq->async) > return 0; >=20 > @@ -1925,6 +1937,12 @@ rte_vhost_async_get_inflight_thread_unsafe(int > vid, uint16_t queue_id) > if (vq =3D=3D NULL) > return ret; >=20 > + if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) { > + VHOST_LOG_CONFIG(ERR, "(%s) %s() called without access > lock taken.\n", > + dev->ifname, __func__); > + return -1; > + } > + > if (!vq->async) > return ret; >=20 > -- > 2.35.1