Bill,
Thanks a lot for the review! I will send updated patches in few hours
hopefully.
On 14.04.2017 00:42, Bill Fischofer wrote:
> Next version should be marked API-NEXT, whether or not it is still an
Yes, that was an error from my side.
> On Tue, Apr 11, 2017 at 8:41 AM, Dmitry Eremin-Solenikov
> <dmitry.ereminsoleni...@linaro.org> wrote:
>> For now it's only a preview with the following limitation:
>> - No inline processing support
>> - No SA lookups
>> - Only IPv4 support
>> - No tunnel support
>> - No header modification according to RFCs
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsoleni...@linaro.org>
>> ---
>> platform/linux-generic/include/odp_internal.h | 4 +
>> .../linux-generic/include/odp_ipsec_internal.h | 71 ++
>> platform/linux-generic/include/protocols/ip.h | 52 +
>> platform/linux-generic/odp_event.c | 5 +
>> platform/linux-generic/odp_init.c | 13 +
>> platform/linux-generic/odp_ipsec.c | 1172
>> +++++++++++++++++++-
>> 6 files changed, 1287 insertions(+), 30 deletions(-)
>> create mode 100644 platform/linux-generic/include/odp_ipsec_internal.h
>>
>> diff --git a/platform/linux-generic/include/odp_internal.h
>> b/platform/linux-generic/include/odp_internal.h
>> index 05c8a422..fd7848ac 100644
>> --- a/platform/linux-generic/include/odp_internal.h
>> +++ b/platform/linux-generic/include/odp_internal.h
>> @@ -71,6 +71,7 @@ enum init_stage {
>> CLASSIFICATION_INIT,
>> TRAFFIC_MNGR_INIT,
>> NAME_TABLE_INIT,
>> + IPSEC_INIT,
>> MODULES_INIT,
>> ALL_INIT /* All init stages completed */
>> };
>> @@ -130,6 +131,9 @@ int _odp_ishm_init_local(void);
>> int _odp_ishm_term_global(void);
>> int _odp_ishm_term_local(void);
>>
>> +int odp_ipsec_init_global(void);
>> +int odp_ipsec_term_global(void);
>> +
>> int _odp_modules_init_global(void);
>>
>> int cpuinfo_parser(FILE *file, system_info_t *sysinfo);
>> diff --git a/platform/linux-generic/include/odp_ipsec_internal.h
>> b/platform/linux-generic/include/odp_ipsec_internal.h
>> new file mode 100644
>> index 00000000..c7620b88
>> --- /dev/null
>> +++ b/platform/linux-generic/include/odp_ipsec_internal.h
>> @@ -0,0 +1,71 @@
>> +/* Copyright (c) 2017, Linaro Limited
>> + * All rights reserved.
>> + *
>> + * SPDX-License-Identifier: BSD-3-Clause
>> + */
>> +
>> +/**
>> + * @file
>> + *
>> + * ODP internal IPsec routines
>> + */
>> +
>> +#ifndef ODP_IPSEC_INTERNAL_H_
>> +#define ODP_IPSEC_INTERNAL_H_
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>> +#include <odp/api/std_types.h>
>> +#include <odp/api/plat/strong_types.h>
>> +
>> +/** @ingroup odp_ipsec
>> + * @{
>> + */
>> +
>> +typedef ODP_HANDLE_T(odp_ipsec_op_result_event_t);
>> +
>> +#define ODP_IPSEC_OP_RESULT_EVENT_INVALID \
>> + _odp_cast_scalar(odp_ipsec_op_result_event_t, 0xffffffff)
>> +
>> +/**
>> + * Get ipsec_op_result_event handle from event
>> + *
>> + * Converts an ODP_EVENT_IPSEC_RESULT_EVENT type event to an IPsec result
>> event.
>> + *
>> + * @param ev Event handle
>> + *
>> + * @return IPsec result handle
>> + *
>> + * @see odp_event_type()
>> + */
>> +odp_ipsec_op_result_event_t
>> odp_ipsec_op_result_event_from_event(odp_event_t ev);
>
> The odp_ipsec_result() API is already defined. No need to invent a new one.
This function (and few next ones) is an internal API, specific to this
implementation, used exactly to implement odp_ipsec_result() and
odp_event_free().
If we are talking about this prototype, it is used inside
odp_ipsec_result() to convert from generic odp_event_t to type-specific
implementation.
>
>> +
>> +/**
>> + * Convert IPsec result event handle to event
>> + *
>> + * @param res IPsec result handle
>> + *
>> + * @return Event handle
>> + */
>> +odp_event_t odp_ipsec_op_result_event_to_event(odp_ipsec_op_result_event_t
>> res);
>> +
>> +/**
>> + * Free IPsec result event
>> + *
>> + * Frees the ipsec_op_result_event into the ipsec_op_result_event pool it
>> was allocated from.
>> + *
>> + * @param res IPsec result handle
>> + */
>> +void odp_ipsec_op_result_event_free(odp_ipsec_op_result_event_t res);
>
> For consistency with other free routines this would be
> odp_ipsec_result_free(), and odp_event_free() would be extended to
> cover this event type as well.
I'll drop _event part from function and type names.
> These APIs cannot be defined here, they need to be in
> include/odp/api/spec/ipsec.h
No. They are internal platform-specific ones. They are not intended for
the application writers.
>> + case ODP_EVENT_IPSEC_RESULT:
>> +
>> odp_ipsec_op_result_event_free(odp_ipsec_op_result_event_from_event(event));
>
> For consistency with other names this should be:
> odp_ipsec_result_free(odp_ipsec_result_from_event(event));
>
> The cast function in the other direction would be odp_ipsec_result_to_event().
Fine.
>> +typedef struct ipsec_sa_t {
>> + odp_ticketlock_t lock ODP_ALIGNED_CACHE;
>> + int reserved;
>
> Why does a brand new struct need a reserved field like this?
Is is_reserved better?
>
>> + odp_ipsec_sa_t ipsec_sa_hdl;
>> + uint32_t ipsec_sa_idx;
>> +
>> + odp_crypto_session_t session;
>> + odp_bool_t in_place;
>> + void *context;
>> + odp_queue_t queue;
>> +
>> + uint32_t ah_icv_len;
>> + uint32_t esp_iv_len;
>> + uint32_t esp_block_len;
>> + uint32_t spi;
>> + uint32_t seq;
>> + uint8_t iv[MAX_IV_LEN]; /**< ESP IV storage */
>> +} ipsec_sa_t;
>> +
>> +typedef struct ipsec_sa_table_t {
>> + ipsec_sa_t ipsec_sa[ODP_CONFIG_IPSEC_SAS];
>> + odp_shm_t shm;
>> +} ipsec_sa_table_t;
>> +
>> +static ipsec_sa_table_t *ipsec_sa_tbl;
>> +
>> +typedef struct odp_ipsec_ctx_s odp_ipsec_ctx_t;
>> +
>> +typedef void (*odp_ipsecproc_t)(odp_packet_t pkt, odp_ipsec_ctx_t *ctx);
>
> The odp_ prefix should only be used for external ODP APIs. Use
> _odp_ipsecproc_t for internal "ODP" routines, or just have private
> names without a prefix. Same comment throughout for other internal
> names that use the odp_ prefix here.
> I realize we weren't always consistent in this convention, but we're
> trying to keep to it for new development.
Fine with me, I'll try to review code and change names accordingly.
Should I also prefix my internal result/event names with _odp_ rather
than just odp_?
>> +
>> +static odp_pool_t odp_odp_ipsec_ctx_pool = ODP_POOL_INVALID;
>
> The duplicated prefix (odp_odp_...) looks odd here. Isn't one prefix
> sufficient?
Oops.
>> +
>> + /* Create context buffer pool */
>> + params.buf.size = sizeof(odp_ipsec_ctx_t);
>> + params.buf.align = 0;
>> + params.buf.num = SHM_CTX_POOL_BUF_COUNT;
>
> Why this constant? Shouldn't this be something more IPsec-specific?
Fixed.
>
>> + params.type = ODP_POOL_BUFFER;
>> +
>> + odp_odp_ipsec_ctx_pool = odp_pool_create("odp_odp_ipsec_ctx_pool",
>> ¶ms);
>> + if (ODP_POOL_INVALID == odp_odp_ipsec_ctx_pool) {
>> + ODP_ERR("Error: context pool create failed.\n");
>> + return -1;
>> + }
>> +
>> + params.buf.size = sizeof(odp_ipsec_op_result_event_hdr_t);
>> + params.buf.align = 0;
>> + params.buf.num = SHM_CTX_POOL_BUF_COUNT;
>> + params.type = ODP_POOL_BUFFER;
>> +
>> + odp_ipsec_op_result_pool =
>> odp_pool_create("odp_ipsec_op_result_pool", ¶ms);
>> + if (ODP_POOL_INVALID == odp_ipsec_op_result_pool) {
>> + ODP_ERR("Error: result pool create failed.\n");
>> + odp_pool_destroy(odp_odp_ipsec_ctx_pool);
>
> Need at least a (void) here or else an RC check to keep tools like
> Coverity happy.
I noticed that no other code inside platform/linux-generic uses
odp_pool_create/odp_pool_destroy. Am I using them correctly here? Should
I use any other memory management API?
>> +
>> + for (i = 0; i < ODP_CONFIG_IPSEC_SAS; i++) {
>> + ipsec_sa = ipsec_sa_entry(i);
>> +
>> + odp_ticketlock_lock(&ipsec_sa->lock);
>> + if (ipsec_sa->reserved) {
>> + ODP_ERR("Not destroyed ipsec_sa: %u\n",
>> ipsec_sa->ipsec_sa_idx);
>> + rc = -1;
>> + }
>> + ipsec_sa->reserved = 1;
>> + odp_ticketlock_unlock(&ipsec_sa->lock);
>> + }
>
> Looks like this loop should go ahead of the odp_pool_destroy. Best to
> keep the term sequence a mirror of the init sequence.
Ack
>
>> +
>> + ret = odp_shm_free(ipsec_sa_tbl->shm);
>> + if (ret < 0) {
>> + ODP_ERR("shm free failed");
>> + rc = -1;
>> + }
>
> You're missing an odp_pool_destroy() for the odp_ipsec_op_result_pool here
Added
>> + rc = odp_crypto_capability(&crypto_capa);
>> + if (rc < 0)
>> + return rc;
>> +
>> + capa->max_num_sa = ODP_CONFIG_IPSEC_SAS;
>> + capa->op_mode_sync = 2;
>
> The spec is documented this way, but magic numbers like this always
> look somewhat jarring. We should be using enums for this. We should
> propose an IPsec API change for that.
Sending separate patch now.
>> + /* Setup parameters and call crypto library to create session */
>> + crypto_param.op = (ODP_IPSEC_DIR_INBOUND == param->dir) ?
>> + ODP_CRYPTO_OP_DECODE :
>> + ODP_CRYPTO_OP_ENCODE;
>> + crypto_param.auth_cipher_text = 1;
>> +
>> + // FIXME: is it possible to use ASYNC crypto with ASYNC IPsec?
>
> Did you mean to say SYNC crypto here, since that's what you're doing?
No. I meant what I said. Using ASYNC crypto events to to power ASYNC
IPsec events. Unfortunately it doesn't seem possible ATM.
>> + /* Generate an IV */
>> + if (ipsec_sa->esp_iv_len) {
>> + crypto_param.iv.data = ipsec_sa->iv;
>> + crypto_param.iv.length =
>> odp_random_data(crypto_param.iv.data, ipsec_sa->esp_iv_len, 1);
>
> The odp_random_data() API has been changed. This call should now be:
> crypto_param.iv.length = odp_random_data(crypto_param.iv.data,
> ipsec_sa->esp_iv_len, ODP_RANDOM_CRYPTO);
Ack.
>
>> + if (crypto_param.iv.length != ipsec_sa->esp_iv_len)
>> + goto error;
>> + }
>> +
>> + if (odp_crypto_session_create(&crypto_param, &ipsec_sa->session,
>> &ses_create_rc))
>> + goto error;
>> + if (ODP_CRYPTO_SES_CREATE_ERR_NONE != ses_create_rc)
>> + goto error;
>> +
>> + return ipsec_sa->ipsec_sa_hdl;
>> +
>> +error:
>> + odp_ticketlock_lock(&ipsec_sa->lock);
>> + ipsec_sa->reserved = 1;
>> + odp_ticketlock_unlock(&ipsec_sa->lock);
>
> Since you reserve with reserve_ipsec_sa() you should create a
> free_ipsec_sa() counterpart to keep things at the same semantic level.
> Also since reserve_ipsec_sa() changes ipsec_sa->reserved from 0 to 1,
> I'd assume that free() should set it back to 0, no?
Yes.
>
>>
>> return ODP_IPSEC_SA_INVALID;
>> }
>> @@ -68,41 +404,790 @@ int odp_ipsec_sa_disable(odp_ipsec_sa_t sa)
>>
>> int odp_ipsec_sa_destroy(odp_ipsec_sa_t sa)
>> {
>> - (void)sa;
>> + ipsec_sa_t *ipsec_sa = ipsec_sa_entry_from_hdl(sa);
>> + int rc = 0;
>>
>> - return -1;
>> + odp_ticketlock_lock(&ipsec_sa->lock);
>> + if (ipsec_sa->reserved) {
>> + ODP_ERR("Destroying unallocated ipsec_sa: %u\n",
>> ipsec_sa->ipsec_sa_idx);
>> + rc = -1;
>> + } else {
>> + if (odp_crypto_session_destroy(ipsec_sa->session) < 0) {
>> + ODP_ERR("Error destroying crypto session for
>> ipsec_sa: %u\n", ipsec_sa->ipsec_sa_idx);
>> + rc = -1;
>> + }
>> +
>> + ipsec_sa->reserved = 1;
>
> Again having a common wrapper for reserving/freeing SAs makes sense.
> Also since the area is cleared at init, reserved == 0 means free, no?
Oops.
Yes, I'm adding free function for the ipsec_sa.
>
>> + }
>> + odp_ticketlock_unlock(&ipsec_sa->lock);
>> +
>> + return rc;
>> +}
>> +
>> +#define ipv4_data_p(ip) ((uint8_t *)((_odp_ipv4hdr_t *)ip + 1))
>> +#define ipv4_data_len(ip) (odp_be_to_cpu_16(ip->tot_len) -
>> sizeof(_odp_ipv4hdr_t))
>
> These simple definitions don't work if IP options are present. You
> should use the IP hdr len field of the IP hdr to get the actual size.
> (_ODP_IPV4HDR_IHL).
Ack.
>
>> +static inline
>> +void ipv4_adjust_len(_odp_ipv4hdr_t *ip, int adj)
>> +{
>> + ip->tot_len = odp_cpu_to_be_16(odp_be_to_cpu_16(ip->tot_len) + adj);
>> +}
>> +
>> +/**
>> + * Verify crypto operation completed successfully
>> + *
>> + * @param status Pointer to cryto completion structure
>> + *
>> + * @return true if all OK else false
>> + */
>> +static inline
>> +odp_bool_t is_crypto_compl_status_ok(odp_crypto_compl_status_t *status)
>> +{
>> + if (status->alg_err != ODP_CRYPTO_ALG_ERR_NONE)
>> + return false;
>> + if (status->hw_err != ODP_CRYPTO_HW_ERR_NONE)
>> + return false;
>> + return true;
>
> This could be simplified as:
> return status->alg_err == ODP_CRYPTO_ALG_ERR_NONE && status->hw_err ==
> ODP_CRYPTO_HW_ERR_NONE;
And inlined then.
>> +
>> +odp_ipsec_op_result_event_t
>> odp_ipsec_op_result_event_from_event(odp_event_t ev)
>> +{
>> + if (odp_unlikely(ODP_EVENT_INVALID == ev))
>> + return ODP_IPSEC_OP_RESULT_EVENT_INVALID;
>> +
>> + if (odp_event_type(ev) != ODP_EVENT_IPSEC_RESULT)
>> + ODP_ABORT("Event not an IPsec result");
>> +
>> + return (odp_ipsec_op_result_event_t)ev;
>> +}
>
> The type here is odp_ipsec_result_t, so as noted earlier this should
> be named odp_ipsec_result_from_event() and use
> ODP_IPSEC_RESULT_INVALID as the invalid value.
Ack
>
>> +
>> +static odp_ipsec_op_result_event_hdr_t
>> *ipsec_op_result_event_hdr_from_buf(odp_buffer_t buf)
>> +{
>> + return (odp_ipsec_op_result_event_hdr_t *)(void
>> *)buf_hdl_to_hdr(buf);
>
> I'd simplify the internal struct name to ipsec_result_hdr_t or even
> ipsec_result_t. No odp_ prefix, or perhaps _odp.
This was c&p from buffer/packet/timeout code, where internal types also
have odp_ prefix. Changing this instance now. Should we change other
places also?
>
>> +}
>> +
>> +static odp_ipsec_op_result_event_hdr_t
>> *ipsec_op_result_event_hdr(odp_ipsec_op_result_event_t res)
>
> Better:
> static ipsec_result_t *ipsec_result_from_event(odp_ipsec_result_t res)
It is not _from_event, because it doesn't consume odp_event_t.
Changing to ipsec_result_hdr().
>> +{
>> + odp_event_t ev = odp_ipsec_op_result_event_to_event(res);
>> + odp_ipsec_op_result_event_hdr_t *res_hdr;
>> +
>> + res_hdr = ipsec_op_result_event_hdr(res);
>> + while (NULL != res_hdr->ctx) {
>> + odp_ipsec_ctx_t *ctx = res_hdr->ctx;
>> +
>> + res_hdr->ctx = ctx->next;
>> + odp_ipsec_free_pkt_ctx(ctx);
>> + }
>> +
>> + odp_buffer_free(odp_buffer_from_event(ev));
>> +}
>
> We'll likely need corresponding odp_ipsec_status_alloc/free() routines
> for those as well.
Later, when I will be working on inline implementation.
>> + /*
>> + * Finish cipher by finding ESP trailer and processing
>> + *
>> + * FIXME: ESP authentication ICV not supported
>> + */
>> + if (ctx->esp_offset) {
>> + uint8_t *eop = (uint8_t *)(ip) +
>> odp_be_to_cpu_16(ip->tot_len);
>> + _odp_esptrl_t *esp_t = (_odp_esptrl_t *)(eop) - 1;
>> +
>> + ip->proto = esp_t->next_header;
>> + trl_len += esp_t->pad_len + sizeof(*esp_t);
>> + }
>> +
>> + /* We have a tunneled IPv4 packet */
>> + if (ip->proto == _ODP_IPV4) {
>> + odp_packet_pull_head(pkt, sizeof(*ip));
>
> Again, need to account for possible IP options.
Ack.
>
>> + } else {
>> + /* Finalize the IPv4 header */
>> + ipv4_adjust_len(ip, -(hdr_len + trl_len));
>> + ip->ttl = ctx->ip_ttl;
>> + ip->tos = ctx->ip_tos;
>> + ip->frag_offset = odp_cpu_to_be_16(ctx->ip_frag_offset);
>> + ip->chksum = 0;
>> + _odp_ipv4_csum_update(pkt);
>> +
>> + /* Correct the packet length and move payload into position
>> */
>> + memmove(((uint8_t *)ip) + hdr_len, ip, sizeof(*ip));
>
> You should use odp_packet_move_data() here as that routine
> automagically handles any packet segmentation.
Thank you for the pointer.
>> +static
>> +int odp_ipsec_in_single(odp_packet_t pkt,
>> + odp_ipsec_sa_t sa,
>> + odp_ipsec_ctx_t *ctx)
>
> odp_ prefix is particularly confusing here since the external API is
> odp_ipsec_in()
Well. This is an internal worker for odp_ipsec_in() and
odp_ipsec_in_enq(). Do you really want for me to change it to
ipsec_in_single()?
>> +
>> + /* Check IP header for IPSec protocols and look it up */
>> + if (_ODP_IPPROTO_AH == ip->proto) {
>> + ah = (_odp_ahhdr_t *)in;
>> + in += ((ah)->ah_len + 2) * 4;
>> + } else if (_ODP_IPPROTO_ESP == ip->proto) {
>> + esp = (_odp_esphdr_t *)in;
>> + in += sizeof(_odp_esphdr_t);
>> + } else {
>> + ctx->status.error.proto = 1;
>> + return -1;
>> + }
>
> We already have the odp_packet_has_ipsec() API that queries
> IPsec-related fields set by the classifier. Not sure if that's useful
> here.
odp_packet_has_ipsec() just checks if the flag is set. But here I need
pointers to headers.
>> +
>> + /* Set IPv4 length before authentication */
>> + if (!odp_packet_push_tail(pkt, hdr_len + trl_len)) {
>> + ctx->status.error.alg = 1;
>> + return -1;
>> + }
>
> Use odp_packet_extend_tail() here as that will handle additional
> segment adds if current tailroom is insufficient.
Ack
>> + if (!ip->id) {
>> + /* re-init tunnel hdr id */
>> + ret = odp_random_data((uint8_t *)ctx->tun_hdr_id,
>> + sizeof(*ctx->tun_hdr_id),
>> + 1);
>
> Use new form of this API:
> ret = odp_random_data((uint8_t *)ctx->tun_hdr_id,
> sizeof(*ctx->tun_hdr_id), ODP_RANDOM_CRYPTO);
>
>> + if (ret != sizeof(*ctx->tun_hdr_id))
>> + abort();
>
> Need to recover and set appropriate RC here. OK as an initial
> prototype, but mark as TODO.
>
>
Ack.
>> int odp_ipsec_out_inline(const odp_ipsec_op_param_t *op_param,
>> @@ -117,9 +1202,36 @@ int odp_ipsec_out_inline(const odp_ipsec_op_param_t
>> *op_param,
>> int odp_ipsec_result(odp_ipsec_op_result_t *result, odp_event_t event)
>> {
>> (void)result;
>> - (void)event;
>>
>> - return -1;
>> + odp_ipsec_op_result_event_t ipsec_ev;
>
> odp_ipsec_result_t ipsec_ev;
>
>> + odp_ipsec_op_result_event_hdr_t *res_hdr;
>> + unsigned out_pkt;
>> + odp_ipsec_ctx_t *ctx;
>> +
>> + ipsec_ev = odp_ipsec_op_result_event_from_event(event);
>
> ipsec_ev = odp_ipsec_result_from_event(event);
>
>> + if (ODP_IPSEC_OP_RESULT_EVENT_INVALID == ipsec_ev)
>
> ODP_IPSEC_RESULT_INVALID
ok.
--
With best wishes
Dmitry
