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

[Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>]

W dniu 09.05.2020 o 23:58, Akhil Goyal pisze:
> Hi Lukasz,
>
> Thanks for the detailed analysis.
> Series
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
>
> Applied to dpdk-next-crypto
>
> Thanks.
I am a fan of big commit messages. Just let me know when I'll cross the 
line withe these novels ;)
Thank you
>> 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>
>> ---

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com