From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 8BA4E9183 for ; Tue, 1 Aug 2017 14:36:30 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 0487520A79; Tue, 1 Aug 2017 08:36:30 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute1.internal (MEProxy); Tue, 01 Aug 2017 08:36:30 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=ZRjaoHKYcmAcvh6 pU8CBE+1bf5nvjuDJX6rdYkWW8V0=; b=m3pHht6Z2D0r91VeaYjLXF5tq6z656q SWChtHherePseHAGShgI5t5QrPkWVAV95vnG1Lcb9RJBYLSkbl8krWzhds2jI7rI FNOWIxSCuwOWH2ACSXhOXrhJV7xGFebMTFMLgpVmJVCyzh2yYf301lCy09ecjVIB JmLMwU7s76zw= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc:x-sasl-enc; s= fm1; bh=ZRjaoHKYcmAcvh6pU8CBE+1bf5nvjuDJX6rdYkWW8V0=; b=FCQQmi1+ 86E25UDJU/kBtAShFpq4smPzNbwVL55Fohao8jZZow2nMPDvAukP1PNaOmo48Bn+ qx0UJX8ml8iKuJhnPJH3vc+Lh65I5qcpPCvq8kAFebqzAQkdJ0KAGCHnAEF8fgpu MgHiINuqVxAKxXDSodGKBlR3kfFA0dD/k99Q7lhew20oDoZ4bUTfM0toHSJQ+W/2 nh2yBxc6JMlIR3Q/YwlNa43kEeuS8sm3MoDloE+fSJhvpLGXCGqNKFXJvkzxaqoB MfLw3tdfPBGzroVFjhSUTvb3jJ+5D5s9zrYIjV6LS7VPnirVTRwNVAev340Irw1y rH2PWXGlmCsXHQ== X-ME-Sender: X-Sasl-enc: HtvvYuLbmC4DfPMbn5R3AZEer/q6aMayU7EfiyrOjhZe 1501590989 Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id B583A7E7AD; Tue, 1 Aug 2017 08:36:29 -0400 (EDT) From: Thomas Monjalon To: "De Lara Guarch, Pablo" , "Gonzalez Monroy, Sergio" Cc: dev@dpdk.org, Jan Blunck Date: Tue, 01 Aug 2017 14:36:29 +0200 Message-ID: <4707898.VbmNOgW1sf@xps> In-Reply-To: References: <20170731023050.28956-1-pablo.de.lara.guarch@intel.com> <0de23010-5d28-68a9-9349-eea9cad6ccb7@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 12:36:30 -0000 01/08/2017 12:48, De Lara Guarch, Pablo: > From: Gonzalez Monroy, Sergio > > 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. > > > > 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. > > > > >> 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? > > > > 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 release > (only one function, rte_cryptodev_create_vdev, will be removed). Given that there is a new release every 3 months, I prefer the safe way. Anyway, if a function is going to be removed, the API and ABI will change.