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