[dpdk-dev] [RFC v2 5/9] ipsec: add SA data-path API

Ananyev, Konstantin konstantin.ananyev at intel.com
Mon Oct 22 00:01:48 CEST 2018


Hi Jerin,

> 
> Hi Konstantin,
> 
> Overall it looks good, but some comments on event mode integration in
> performance effective way.
> 
> >
> > Introduce Security Association (SA-level) data-path API
> > Operates at SA level, provides functions to:
> >     - initialize/teardown SA object
> >     - process inbound/outbound ESP/AH packets associated with the given SA
> >       (decrypt/encrypt, authenticate, check integrity,
> >       add/remove ESP/AH related headers and data, etc.).
> >
> > Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal at intel.com>
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev at intel.com>
> > ---
> >  lib/librte_ipsec/Makefile              |   2 +
> >  lib/librte_ipsec/meson.build           |   4 +-
> >  lib/librte_ipsec/rte_ipsec.h           | 154 +++++++++++++++++++++++++++++++++
> >  lib/librte_ipsec/rte_ipsec_version.map |   3 +
> >  lib/librte_ipsec/sa.c                  |  98 ++++++++++++++++++++-
> >  lib/librte_ipsec/sa.h                  |   3 +
> >  lib/librte_ipsec/ses.c                 |  45 ++++++++++
> >  7 files changed, 306 insertions(+), 3 deletions(-)
> >  create mode 100644 lib/librte_ipsec/rte_ipsec.h
> >  create mode 100644 lib/librte_ipsec/ses.c
> >
> > diff --git a/lib/librte_ipsec/Makefile b/lib/librte_ipsec/Makefile
> > index 7758dcc6d..79f187fae 100644
> > --- a/lib/librte_ipsec/Makefile
> > +++ b/lib/librte_ipsec/Makefile
> > @@ -17,8 +17,10 @@ LIBABIVER := 1
> >
> >  # all source are stored in SRCS-y
> >  SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += sa.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += ses.c
> >
> >  # install header files
> > +SYMLINK-$(CONFIG_RTE_LIBRTE_IPSEC)-include += rte_ipsec.h
> >  SYMLINK-$(CONFIG_RTE_LIBRTE_IPSEC)-include += rte_ipsec_sa.h
> >
> >  include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/lib/librte_ipsec/meson.build b/lib/librte_ipsec/meson.build
> > index 52c78eaeb..6e8c6fabe 100644
> > --- a/lib/librte_ipsec/meson.build
> > +++ b/lib/librte_ipsec/meson.build
> > @@ -3,8 +3,8 @@
> >
> >  allow_experimental_apis = true
> >
> > -sources=files('sa.c')
> > +sources=files('sa.c', 'ses.c')
> >
> > -install_headers = files('rte_ipsec_sa.h')
> > +install_headers = files('rte_ipsec.h', 'rte_ipsec_sa.h')
> >
> >  deps += ['mbuf', 'net', 'cryptodev', 'security']
> > diff --git a/lib/librte_ipsec/rte_ipsec.h b/lib/librte_ipsec/rte_ipsec.h
> > new file mode 100644
> > index 000000000..5c9a1ed0b
> > --- /dev/null
> > +++ b/lib/librte_ipsec/rte_ipsec.h
> > @@ -0,0 +1,154 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Intel Corporation
> > + */
> > +
> > +#ifndef _RTE_IPSEC_H_
> > +#define _RTE_IPSEC_H_
> > +
> > +/**
> > + * @file rte_ipsec.h
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * RTE IPsec support.
> > + * librte_ipsec provides a framework for data-path IPsec protocol
> > + * processing (ESP/AH).
> > + * IKEv2 protocol support right now is out of scope of that draft.
> > + * Though it tries to define related API in such way, that it could be adopted
> > + * by IKEv2 implementation.
> > + */
> > +
> > +#include <rte_ipsec_sa.h>
> > +#include <rte_mbuf.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +struct rte_ipsec_session;
> > +
> > +/**
> > + * IPsec session specific functions that will be used to:
> > + * - prepare - for input mbufs and given IPsec session prepare crypto ops
> > + *   that can be enqueued into the cryptodev associated with given session
> > + *   (see *rte_ipsec_crypto_prepare* below for more details).
> > + * - process - finalize processing of packets after crypto-dev finished
> > + *   with them or process packets that are subjects to inline IPsec offload
> > + *   (see rte_ipsec_process for more details).
> > + */
> > +struct rte_ipsec_sa_func {
> > +       uint16_t (*prepare)(const struct rte_ipsec_session *ss,
> > +                               struct rte_mbuf *mb[],
> > +                               struct rte_crypto_op *cop[],
> > +                               uint16_t num);
> > +       uint16_t (*process)(const struct rte_ipsec_session *ss,
> > +                               struct rte_mbuf *mb[],
> > +                               uint16_t num);
> 
> IMO, It makes sense to have separate function pointers for inbound and
> outbound so that, implementation would be clean and we can avoid a
> "if" check.

SA object by itself is unidirectional (either inbound or outbound), so
I don't think we need 2 function pointers here.
Yes, right now, inside ipsec lib we select functions by action_type only,
but it doesn't have to stay that way.
It could be action_type, direction, mode (tunnel/transport), event/poll, etc.
Let say inline_proto_process() could be split into:
inline_proto_outb_process() and inline_proto_inb_process() and 
rte_ipsec_sa_func.process will point to appropriate one.
I probably will change things that way for next version.

> 
> > +};
> > +
> > +/**
> > + * rte_ipsec_session is an aggregate structure that defines particular
> > + * IPsec Security Association IPsec (SA) on given security/crypto device:
> > + * - pointer to the SA object
> > + * - security session action type
> > + * - pointer to security/crypto session, plus other related data
> > + * - session/device specific functions to prepare/process IPsec packets.
> > + */
> > +struct rte_ipsec_session {
> > +
> > +       /**
> > +        * SA that session belongs to.
> > +        * Note that multiple sessions can belong to the same SA.
> > +        */
> > +       struct rte_ipsec_sa *sa;
> > +       /** session action type */
> > +       enum rte_security_session_action_type type;
> > +       /** session and related data */
> > +       union {
> > +               struct {
> > +                       struct rte_cryptodev_sym_session *ses;
> > +               } crypto;
> > +               struct {
> > +                       struct rte_security_session *ses;
> > +                       struct rte_security_ctx *ctx;
> > +                       uint32_t ol_flags;
> > +               } security;
> > +       };
> > +       /** functions to prepare/process IPsec packets */
> > +       struct rte_ipsec_sa_func func;
> > +};
> 
> IMO, it can be cache aligned as it is used in fast path.

Good point, will add.

> 
> > +
> > +/**
> > + * Checks that inside given rte_ipsec_session crypto/security fields
> > + * are filled correctly and setups function pointers based on these values.
> > + * @param ss
> > + *   Pointer to the *rte_ipsec_session* object
> > + * @return
> > + *   - Zero if operation completed successfully.
> > + *   - -EINVAL if the parameters are invalid.
> > + */
> > +int __rte_experimental
> > +rte_ipsec_session_prepare(struct rte_ipsec_session *ss);
> > +
> > +/**
> > + * For input mbufs and given IPsec session prepare crypto ops that can be
> > + * enqueued into the cryptodev associated with given session.
> > + * expects that for each input packet:
> > + *      - l2_len, l3_len are setup correctly
> > + * Note that erroneous mbufs are not freed by the function,
> > + * but are placed beyond last valid mbuf in the *mb* array.
> > + * It is a user responsibility to handle them further.
> > + * @param ss
> > + *   Pointer to the *rte_ipsec_session* object the packets belong to.
> > + * @param mb
> > + *   The address of an array of *num* pointers to *rte_mbuf* structures
> > + *   which contain the input packets.
> > + * @param cop
> > + *   The address of an array of *num* pointers to the output *rte_crypto_op*
> > + *   structures.
> > + * @param num
> > + *   The maximum number of packets to process.
> > + * @return
> > + *   Number of successfully processed packets, with error code set in rte_errno.
> > + */
> > +static inline uint16_t __rte_experimental
> > +rte_ipsec_crypto_prepare(const struct rte_ipsec_session *ss,
> > +       struct rte_mbuf *mb[], struct rte_crypto_op *cop[], uint16_t num)
> > +{
> > +       return ss->func.prepare(ss, mb, cop, num);
> > +}
> > +
> > +/**
> > + * Finalise processing of packets after crypto-dev finished with them or
> > + * process packets that are subjects to inline IPsec offload.
> > + * Expects that for each input packet:
> > + *      - l2_len, l3_len are setup correctly
> > + * Output mbufs will be:
> > + * inbound - decrypted & authenticated, ESP(AH) related headers removed,
> > + * *l2_len* and *l3_len* fields are updated.
> > + * outbound - appropriate mbuf fields (ol_flags, tx_offloads, etc.)
> > + * properly setup, if necessary - IP headers updated, ESP(AH) fields added,
> > + * Note that erroneous mbufs are not freed by the function,
> > + * but are placed beyond last valid mbuf in the *mb* array.
> > + * It is a user responsibility to handle them further.
> > + * @param ss
> > + *   Pointer to the *rte_ipsec_session* object the packets belong to.
> > + * @param mb
> > + *   The address of an array of *num* pointers to *rte_mbuf* structures
> > + *   which contain the input packets.
> > + * @param num
> > + *   The maximum number of packets to process.
> > + * @return
> > + *   Number of successfully processed packets, with error code set in rte_errno.
> > + */
> > +static inline uint16_t __rte_experimental
> > +rte_ipsec_process(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],
> > +       uint16_t num)
> > +{
> > +       return ss->func.process(ss, mb, num);
> > +}
> 
> Since we have separate functions and  different application path for different mode
> and the arguments also differ. I think, It is better to integrate
> event mode like following
> 
> static inline uint16_t __rte_experimental
> rte_ipsec_event_process(const struct rte_ipsec_session *ss, struct rte_event *ev[], uint16_t num)
> {
>        return ss->func.event_process(ss, ev, num);
> }

To fulfill that, we can either have 2 separate function pointers:
uint16_t (*pkt_process)( const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],uint16_t num);
uint16_t (*event_process)( const struct rte_ipsec_session *ss, struct rte_event *ev[],uint16_t num);

Or we can keep one function pointer, but change it to accept just array of pointers:
uint16_t (*process)( const struct rte_ipsec_session *ss, void *in[],uint16_t num);
and then make session_prepare() to choose a proper function based on input.

I am ok with both schemes, but second one seems a bit nicer to me. 

> 
> This is to,
> 1) Avoid Event mode application code duplication
> 2) Better I$ utilization rather moving event specific and mbuff
> specific at different code locations
> 3) Better performance as inside one function pointer we can do things
> in one shot rather splitting the work to application and library.
> 4) Event mode has different modes like ATQ, non ATQ etc, These things
> we can abstract through exiting function pointer scheme.
> 5) atomicity & ordering problems can be sorted out internally with the events,
> having one function pointer for event would be enough.
> 
> We will need some event related info (event dev, port, atomic queue to
> use etc) which need to be added in rte_ipsec_session *ss as UNION so it
> wont impact the normal mode. This way, all the required functionality of this library
> can be used with event-based model.
 
Yes, to support event model, I imagine ipsec_session might need to
contain some event specific data.
I am absolutely ok with that idea in general.
Related fields can be added to the ipsec_session structure as needed,
together with actual event mode implementation. 

>
> See below some implementation thoughts on this.
> 
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_IPSEC_H_ */
> > diff --git a/lib/librte_ipsec/rte_ipsec_version.map b/lib/librte_ipsec/rte_ipsec_version.map
> > +const struct rte_ipsec_sa_func *
> > +ipsec_sa_func_select(const struct rte_ipsec_session *ss)
> > +{
> > +       static const struct rte_ipsec_sa_func tfunc[] = {
> > +               [RTE_SECURITY_ACTION_TYPE_NONE] = {
> > +                       .prepare = lksd_none_prepare,
> > +                       .process = lksd_none_process,
> > +               },
> > +               [RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO] = {
> > +                       .prepare = NULL,
> > +                       .process = inline_crypto_process,
> > +               },
> > +               [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL] = {
> > +                       .prepare = NULL,
> > +                       .process = inline_proto_process,
> > +               },
> > +               [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL] = {
> > +                       .prepare = lksd_proto_prepare,
> > +                       .process = lksd_proto_process,
> > +               },
> 
>              [RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL][EVENT] = {
>                     .prepare = NULL,
>                     .process = NULL,
>                     .process_evt = lksd_event_process,
>              },
>              [RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL][EVENT] = {
>                     .prepare = NULL,
>                     .process = NULL,
>                     .process_evt = inline_event_process,
>              },
> 
> Probably add one more dimension in array to choose event/poll?

That's a static function/array, surely we can have here as many dimensions as we need to.
As I said below, will probably need to select a function based on direction, mode, etc. anyway.
NP to have extra logic to select event/mbuf based functions.

> 
> 
> > +       };
> > +
> > +       if (ss->type >= RTE_DIM(tfunc))
> > +               return NULL;
> > +
> > +       return tfunc + ss->type;
> > +}
> > diff --git a/lib/librte_ipsec/sa.h b/lib/librte_ipsec/sa.h
> > index ef030334c..13a5a68f3 100644
> > --- a/lib/librte_ipsec/sa.h
> > +++ b/lib/librte_ipsec/sa.h
> > @@ -72,4 +72,7 @@ struct rte_ipsec_sa {
> >
> >  } __rte_cache_aligned;
> >
> > +const struct rte_ipsec_sa_func *
> > +ipsec_sa_func_select(const struct rte_ipsec_session *ss);
> > +
> >  #endif /* _SA_H_ */
> > diff --git a/lib/librte_ipsec/ses.c b/lib/librte_ipsec/ses.c
> > new file mode 100644
> > index 000000000..afefda937
> > --- /dev/null
> > +++ b/lib/librte_ipsec/ses.c
> > @@ -0,0 +1,45 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018 Intel Corporation
> > + */
> > +
> > +#include <rte_ipsec.h>
> > +#include "sa.h"
> > +
> > +static int
> > +session_check(struct rte_ipsec_session *ss)
> > +{
> > +       if (ss == NULL)
> > +               return -EINVAL;
> > +
> > +       if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) {
> > +               if (ss->crypto.ses == NULL)
> > +                       return -EINVAL;
> > +       } else if (ss->security.ses == NULL || ss->security.ctx == NULL)
> > +               return -EINVAL;
> > +
> > +       return 0;
> > +}
> > +
> > +int __rte_experimental
> > +rte_ipsec_session_prepare(struct rte_ipsec_session *ss)
> > +{
> 
> Probably add one more argument to choose event vs poll so that
> above function pointers can be selected.
> 
> or have different API like rte_ipsec_use_mode(EVENT) or API
> other slow path scheme to select the mode.

Yes, we would need something here. 
I think we can have some field inside ipsec_session that defines
which input types (mbuf/event) session expects.
I suppose we would need such field anyway - as you said above,
ipsec_session most likely will contain a union for event/non-event related data. 

Konstantin



More information about the dev mailing list