Re: [dpdk-dev] [PATCH v2] cryptodev: fix NULL pointer dereference

[Thomas Monjalon <thomas@monjalon.net>]

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 <pablo.de.lara.guarch@intel.com>
> > >>>> ---
> > >>>>
> > >>>> 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.