From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 0F6807CB6 for ; Tue, 1 Aug 2017 12:48:51 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP; 01 Aug 2017 03:48:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,305,1498546800"; d="scan'208";a="294131561" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by fmsmga004.fm.intel.com with ESMTP; 01 Aug 2017 03:48:50 -0700 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX107.ger.corp.intel.com (163.33.3.99) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 1 Aug 2017 11:48:49 +0100 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.133]) by irsmsx112.ger.corp.intel.com ([169.254.1.42]) with mapi id 14.03.0319.002; Tue, 1 Aug 2017 11:48:49 +0100 From: "De Lara Guarch, Pablo" To: "Gonzalez Monroy, Sergio" , "Thomas Monjalon" CC: "dev@dpdk.org" , Jan Blunck Thread-Topic: [PATCH v2] cryptodev: fix NULL pointer dereference Thread-Index: AQHTCqmH7I/BK7qKwUiRD8ncazYAx6JvOLeAgAAW2yA= Date: Tue, 1 Aug 2017 10:48:48 +0000 Message-ID: References: <20170731023050.28956-1-pablo.de.lara.guarch@intel.com> <7230298.g0yYOUbsxP@xps> <5207907.Bg9B3aH5F5@xps> <0de23010-5d28-68a9-9349-eea9cad6ccb7@intel.com> In-Reply-To: <0de23010-5d28-68a9-9349-eea9cad6ccb7@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Aug 2017 10:48:52 -0000 > -----Original Message----- > From: Gonzalez Monroy, Sergio > Sent: Tuesday, August 1, 2017 11:18 AM > To: Thomas Monjalon > Cc: De Lara Guarch, Pablo ; > dev@dpdk.org > Subject: Re: [PATCH v2] cryptodev: fix NULL pointer dereference >=20 > On 01/08/2017 10:35, Thomas Monjalon wrote: > > 01/08/2017 10:13, Sergio Gonzalez Monroy: > >> On 31/07/2017 20:33, Thomas Monjalon wrote: > >>> 31/07/2017 11:18, Pablo de Lara: > >>>> When register a crypto driver, a cryptodev driver structure was > >>>> being allocated, using malloc. > >>>> Since this call may fail, it is safer to allocate this memory > >>>> statically in each PMD, so driver registration will never fail. > >>>> > >>>> Coverity issue: 158645 > >>>> > >>>> Fixes: 7a364faef185 ("cryptodev: remove crypto device type > >>>> enumeration") > >>>> > >>>> Signed-off-by: Pablo de Lara > >>>> --- > >>>> > >>>> Changes in v2: > >>>> > >>>> - Allocate statically the cryptodev driver structure, > >>>> instead of using malloc, that can potentially fail. > >>>> > >>>> drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 5 ++++- > >>>> drivers/crypto/aesni_mb/rte_aesni_mb_pmd.c | 6 +++++- > >>>> drivers/crypto/armv8/rte_armv8_pmd.c | 9 ++++++--- > >>>> drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 5 ++++- > >>>> drivers/crypto/kasumi/rte_kasumi_pmd.c | 5 ++++- > >>>> drivers/crypto/null/null_crypto_pmd.c | 5 ++++- > >>>> drivers/crypto/openssl/rte_openssl_pmd.c | 5 ++++- > >>>> drivers/crypto/qat/rte_qat_cryptodev.c | 7 +++++-- > >>>> drivers/crypto/scheduler/scheduler_pmd.c | 5 ++++- > >>>> drivers/crypto/snow3g/rte_snow3g_pmd.c | 5 ++++- > >>>> drivers/crypto/zuc/rte_zuc_pmd.c | 5 ++++- > >>>> lib/librte_cryptodev/rte_cryptodev.c | 18 +++++-----------= - > >>>> lib/librte_cryptodev/rte_cryptodev.h | 20 ----------------= --- > >>>> lib/librte_cryptodev/rte_cryptodev_pmd.h | 30 > +++++++++++++++++++++++++++++ > >>>> 14 files changed, 83 insertions(+), 47 deletions(-) > >>> This is a big change for a small/unlikely issue. > >>> The main benefit of this patch is an allocation cleanup. > >>> I think it is better to wait 17.11 cycle to integrate it. > >> We initially thought of exit given that it is a constructor and if > >> you fail to allocate memory at this stage, things are likely not > >> going to work out anyway. > > You don't know how the application wants to manage it. >=20 > IMHO setting an internal variable indicating an error in constructors and > then reporting the problem during EAL init seems overly complex. > I think the proposed change is a cleaner solution. >=20 > >> The patch is an API change, do we really want to break again (we are > >> breaking in this release) next release? > > Good question. Any opinions? >=20 > Merge the patch unless there are already outstanding and/or planned > changes for the next release that are going to break ABI/API? There is another patchset that was postponed for next release, because the compilation was broken in one of the patches (just double checked and it is= easy to fix), and by then, I thought that no ABI/API was being broken, but it will be (my bad here). This is the patchset I am talking about: [PATCH v2 0/4] cryptodev vdev changes for -rc2 http://dpdk.org/ml/archives/dev/2017-July/071160.html So we have two options here: 1 - Get both patches now, since we are breaking the ABI in this release (as= Sergio pointed out). 2 - Postpone both changes to next release. I would go for option 1, as there are no other changes expected for next re= lease (only one function, rte_cryptodev_create_vdev, will be removed). Thanks, Pablo >=20 >=20 > Thanks, > Sergio