Re: [dpdk-dev] [RFC] ipsec: new library for IPsec data-path processing

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Anoob,

> Hi Konstantin,
> 
> Few comments. Please see inline.
> 
> Thanks,
> 
> Anoob
> 
> On 24-08-2018 22:23, Konstantin Ananyev wrote:
> > External Email
> >
> > This RFC introduces a new library within DPDK: librte_ipsec.
> > The aim is to provide DPDK native high performance library for IPsec
> > data-path processing.
> > The library is supposed to utilize existing DPDK crypto-dev and
> > security API to provide application with transparent IPsec processing API.
> > The library is concentrated on data-path protocols processing (ESP and AH),
> > IKE protocol(s) implementation is out of scope for that library.
> > Though hook/callback mechanisms will be defined to allow integrate it
> > with existing IKE implementations.
> > Due to quite complex nature of IPsec protocol suite and variety of user
> > requirements and usage scenarios a few API levels will be provided:
> > 1) Security Association (SA-level) 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.).
> > 2) Security Association Database (SAD) API
> >      API to create/manage/destroy IPsec SAD.
> >      While DPDK IPsec library plans to have its own implementation,
> >      the intention is to keep it as independent from the other parts
> >      of IPsec library as possible.
> >      That is supposed to give users the ability to provide their own
> >      implementation of the SAD compatible with the other parts of the
> >      IPsec library.
> > 3) IPsec Context (CTX) API
> >      This is supposed to be a high-level API, where each IPsec CTX is an
> >      abstraction of 'independent copy of the IPsec stack'.
> >      CTX owns set of SAs, SADs and assigned to it crypto-dev queues, etc.
> >      and provides:
> >      - de-multiplexing stream of inbound packets to particular SAs and
> >        further IPsec related processing.
> >      - IPsec related processing for the outbound packets.
> >      - SA add/delete/update functionality
> [Anoob]: Security Policy is an important aspect of IPsec. An IPsec
> library without Security Policy API would be incomplete. For inline
> protocol offload, the final SP-SA check(selector check) is the only
> IPsec part being done by ipsec-secgw now. Would make sense to add that
> also in the library.

You mean here, that we need some sort of SPD implementation, correct?

> > Current RFC concentrates on SA-level API only (1),
> > detailed discussion for 2) and 3) will be subjects for separate RFC(s).
> >
> > SA (low) level API
> > ==================
> >
> > API described below operates on SA level.
> > It provides functionality that allows user for given SA to process
> > inbound and outbound IPsec packets.
> > To be more specific:
> > - for inbound ESP/AH packets perform decryption, authentication,
> >    integrity checking, remove ESP/AH related headers
> [Anoob] Anti-replay check would also be required.

Yep, anti-replay and ESN support is implied as part of "integrity checking".
Probably I have to be more specific here.

> > - for outbound packets perform payload encryption, attach ICV,
> >    update/add IP headers, add ESP/AH headers/trailers,
> >    setup related mbuf felids (ol_flags, tx_offloads, etc.).
> [Anoob] Do we have any plans to handle ESN expiry? Some means to
> initiate an IKE renegotiation? I'm assuming application won't be aware
> of the sequence numbers, in this case.
> > - initialize/un-initialize given SA based on user provided parameters.
> >
> > Processed inbound/outbound packets could be grouped by user provided
> > flow id (opaque 64-bit number associated by user with given SA).
> >
> > SA-level API is based on top of crypto-dev/security API and relies on them
> > to perform actual cipher and integrity checking.
> > Due to the nature of crypto-dev API (enqueue/deque model) we use
> > asynchronous API for IPsec packets destined to be processed
> > by crypto-device:
> > rte_ipsec_crypto_prepare()->rte_cryptodev_enqueue_burst()->
> > rte_cryptodev_dequeue_burst()->rte_ipsec_crypto_process().
> > Though for packets destined for inline processing no extra overhead
> > is required and simple and synchronous API: rte_ipsec_inline_process()
> > is introduced for that case.
> [Anoob] The API should include event-delivery as a crypto-op completion
> mechanism as well. The application could configure the event crypto
> adapter and then enqueue and dequeue to crypto device using events (via
> event dev).

Not sure what particular extra API you think is required here?
As I understand in both cases (with or without event crypto-adapter) we still have to:
 1) fill crypto-op properly
 2) enqueue it to crypto-dev (via eventdev or directly)
3)  receive processed by crypto-dev crypto-op (either via eventdev or directly)
4) check crypto-op status, do further post-processing if any

So #1 and #4 (SA-level API respnibility) remain the same for both cases.

> > The following functionality:
> >    - match inbound/outbound packets to particular SA
> >    - manage crypto/security devices
> >    - provide SAD/SPD related functionality
> >    - determine what crypto/security device has to be used
> >      for given packet(s)
> > is out of scope for SA-level API.
> >
> > Below is the brief (and simplified) overview of expected SA-level
> > API usage.
> >
> > /* allocate and initialize SA */
> > size_t sz = rte_ipsec_sa_size();
> > struct rte_ipsec_sa *sa = rte_malloc(sz);
> > struct rte_ipsec_sa_prm prm;
> > /* fill prm */
> > rc = rte_ipsec_sa_init(sa, &prm);
> > if (rc != 0) { /*handle error */}
> > .....
> >
> > /* process inbound/outbound IPsec packets that belongs to given SA */
> >
> > /* inline IPsec processing was done for these packets */
> > if (use_inline_ipsec)
> >         n = rte_ipsec_inline_process(sa, pkts, nb_pkts);
> > /* use crypto-device to process the packets */
> > else {
> >       struct rte_crypto_op *cop[nb_pkts];
> >       struct rte_ipsec_group grp[nb_pkts];
> >
> >        ....
> >       /* prepare crypto ops */
> >       n = rte_ipsec_crypto_prepare(sa, pkts, cops, nb_pkts);
> >       /* enqueue crypto ops to related crypto-dev */
> >       n =  rte_cryptodev_enqueue_burst(..., cops, n);
> >       if (n != nb_pkts) { /*handle failed packets */}
> >       /* dequeue finished crypto ops from related crypto-dev */
> >       n = rte_cryptodev_dequeue_burst(..., cops, nb_pkts);
> >       /* finish IPsec processing for associated packets */
> >       n = rte_ipsec_crypto_process(cop, pkts, grp, n);
> [Anoob] Does the SA based grouping apply to both inbound and outbound?

Yes, the plan is to have it available for both cases.

> >       /* now we have <n> group of packets grouped by SA flow id  */
> >      ....
> >   }
> > ...
> >
> > /* uninit given SA */
> > rte_ipsec_sa_fini(sa);
> >
> > Planned scope for 18.11:
> > ========================
> >
> > - SA-level API definition
> > - ESP tunnel mode support (both IPv4/IPv6)
> > - Supported algorithms: AES-CBC, AES-GCM, HMAC-SHA1, NULL.
> > - UT
> [Anoob] What is UT?

Unit-Test


> > Note: Still WIP, so not all planned for 18.11 functionality is in place.
> >
> > Post 18.11:
> > ===========
> > - ESP transport mode support (both IPv4/IPv6)
> > - update examples/ipsec-secgw to use librte_ipsec
> > - SAD and high-level API definition and implementation
> >
> >
> > Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
> > Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> >   config/common_base                     |   5 +
> >   lib/Makefile                           |   2 +
> >   lib/librte_ipsec/Makefile              |  24 +
> >   lib/librte_ipsec/meson.build           |  10 +
> >   lib/librte_ipsec/pad.h                 |  45 ++
> >   lib/librte_ipsec/rte_ipsec.h           | 245 +++++++++
> >   lib/librte_ipsec/rte_ipsec_version.map |  13 +
> >   lib/librte_ipsec/sa.c                  | 921 +++++++++++++++++++++++++++++++++
> >   lib/librte_net/rte_esp.h               |  10 +-
> >   lib/meson.build                        |   2 +
> >   mk/rte.app.mk                          |   2 +
> >   11 files changed, 1278 insertions(+), 1 deletion(-)
> >   create mode 100644 lib/librte_ipsec/Makefile
> >   create mode 100644 lib/librte_ipsec/meson.build
> >   create mode 100644 lib/librte_ipsec/pad.h
> >   create mode 100644 lib/librte_ipsec/rte_ipsec.h
> >   create mode 100644 lib/librte_ipsec/rte_ipsec_version.map
> >   create mode 100644 lib/librte_ipsec/sa.c
> <snip>
> > +static inline uint16_t
> > +esp_outb_tun_prepare(struct rte_ipsec_sa *sa, struct rte_mbuf *mb[],
> > +       struct rte_crypto_op *cop[], uint16_t num)
> > +{
> > +       int32_t rc;
> > +       uint32_t i, n;
> > +       union sym_op_data icv;
> > +
> > +       n = esn_outb_check_sqn(sa, num);
> > +
> > +       for (i = 0; i != n; i++) {
> > +
> > +               sa->sqn++;
> [Anoob] Shouldn't this be done atomically?

If we want to have MT-safe API for SA-datapath API, then yes.
Though it would make things more complicated here, especially for inbound with anti-replay support.
I think it is doable (spin-lock?), but would cause extra overhead and complexity.
Right now I am not sure it really worth it - comments/suggestions are welcome.
What probably could be a good compromise - runtime decision per SA basis (at sa_init()),
do we need an ST or MT behavior for given SA.

> > +               sa->iv.v8 = rte_cpu_to_be_64(sa->sqn);
> > +
> > +               /* update the packet itself */
> > +               rc = esp_outb_tun_pkt_prepare(sa, mb[i], &icv);
> > +               if (rc < 0) {
> > +                       rte_errno = -rc;
> > +                       break;
> > +               }
> > +
> > +               /* update crypto op */
> > +               esp_outb_tun_cop_prepare(cop[i], sa, mb[i], &icv, rc);
> > +       }
> > +
> > +       return i;
> > +}
> >
> <snip>

Thanks
Konstantin