Re: [dpdk-dev] [PATCH 1/2] crypto/dpaa_sec: improve memory freeing

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Lukasz,

Thanks for the detailed analysis.
Series
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

Applied to dpdk-next-crypto

Thanks.
> 
> This patch fixes management of memory for authentication
> and encryption keys.
> There were two issues with former state of implementation:
> 
> 1) Invalid access to dpaa_sec_session union members
>     The dpaa_sec_session structure includes an anonymous union:
>     union {
>         struct {...} aead_key;
>         struct {
>             struct {...} cipher_key;
>             struct {...} auth_key;
>         };
>     };
>     Depending on the used algorithm a rte_zmalloc() function
>     allocated memory that was kept in aead_key, cipher_key
>     or auth_key. However every time the memory was released,
>     rte_free() was called only on cipher and auth keys, even
>     if pointer to allocated memory was stored in aead_key.
> 
>     The C language specification defines such behavior as undefined.
>     As the cipher_key and aead_key are similar, have same sizes and
>     alignment, it has worked, but it's directly against C specification.
> 
>     This patch fixes this, providing a free_session_data() function
>     to free the keys data. It verifies which algorithm was used
>     (aead or auth+cipher) and frees proper part of the union.
> 
> 2) Some keys might have been freed multiple times
>     In functions like: dpaa_sec_cipher_init(), dpaa_sec_auth_init(),
>     dpaa_sec_chain_init(), dpaa_sec_aead_init() keys data were freed
>     before returning due to some error conditions. However the pointers
>     were not zeroed causing another calls to ret_free from higher
>     layers of code. This causes an error log about invalid memory address
>     to be printed.
> 
>     This patch fixes it by making only one layer responsible for freeing
>     memory:
>     * dpaa_sec_set_session_parameters() for allocations made in:
>         - dpaa_sec_cipher_init()
>         - dpaa_sec_auth_init()
>         - dpaa_sec_chain_init()
>         - dpaa_sec_aead_init()
>     * dpaa_sec_set_ipsec_session() for allocations made in:
>         - dpaa_sec_ipsec_aead_init()
>         - dpaa_sec_ipsec_proto_init()
>     * dpaa_sec_set_pdcp_session() for allocations made in:
>         - dpaa_sec_set_pdcp_session() /*only one layer in case of pdcp*/
> 
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---