On 18.04.2017 19:08, Peltonen, Janne (Nokia - FI/Espoo) wrote:
> Hi,
Thank you for your review!
>
>>>> + /* 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.
>
> I also thought SYNC crypto was meant there in the FIXME comment.
>
> Using ASYNC crypto to provide ASYNC IPsec is clearly possible. Maybe
> it cannot be done as efficiently as one might want, but one could
> e.g. do an async crypto-op, and then upon its completion create and
> queue an IPsec completion event. Event queuing would occur twice.
This would require assistance from the application code and/or scheduler.
>
> However, since the IPsec implementation is part of the ODP implementation,
> it does not necessarily have to restrict itself to the public ODP
> crypto API but can use internal APIs too, which may make things easier.
Yes, but currently I'd prefer to stay away from changing crypto API.
Petri said, that he has some ideas, so we might discuss that later.
>
>>>> + /* 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);
>
> Note that for AES-GCM one cannot use random IV since IV values must
> never be reused in GCM. With AES-GCM a counter or alike would work.
AES-GCM definitely needs more work, as you've stated in another email.
>
> Janne
>
>
>> -----Original Message-----
>> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Dmitry
>> Eremin-
>> Solenikov
>> Sent: Friday, April 14, 2017 3:00 PM
>> To: Bill Fischofer <bill.fischo...@linaro.org>
>> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
>> Subject: Re: [lng-odp] [API-NEXT][RFC][rebased] linux-gen: ipsec: draft IPsec
>> implementation
>>
>> 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
--
With best wishes
Dmitry
